#6755 closed (fixed)
Model Inheritance doesn't work in the admin.
Reported by: | anonymous | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Keywords: | ||
Cc: | evh293@…, newijk@…, erwin@…, robillard.etienne@…, ekellner@…, someone@…, philipp@…, cmawebsite@…, wonlay@…, brooks.travis@…, jamespic@…, mateusz@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Under QS-RF, inherited models don't work in the admin. See Malcolm's message: http://groups.google.com/group/django-developers/msg/c0fb00dabb70c660
Attachments (4)
Change History (55)
comment:1 by , 17 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 17 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Sorry, I hope that this comment helps you to reproduce the behaviour that I think that is a bug.
Reproducing de bug
- Suppose a django app with this model:
class Place(models.Model): name = models.CharField(max_length=50) address = models.CharField(max_length=80) def __unicode__(self): return u"%s the place" % self.name class Admin: pass class Restaurant(Place): serves_hot_dogs = models.BooleanField() serves_pizza = models.BooleanField() def __unicode__(self): return u"%s the restaurant" % self.name class Admin: pass
- Now, go to the admin site (<address to your app>/admin) and create a new Restaurant.
- Then, try edit the restaurant created in the previous step. Modify some field value and press "save". Now when go to the Restaurant list appears TWO restaurants: the save on tha previous step creates a new restaurant instead of edit the first.
NOTE
If define the primary key of the parent class(Place) explicitly the edition functionality works well:
class Place(models.Model): id = models.AutoField('id', primary_key=True, editable=False) name = models.CharField(max_length=50) address = models.CharField(max_length=80) def __unicode__(self): return u"%s the place" % self.name class Admin: pass class Restaurant(Place): serves_hot_dogs = models.BooleanField() serves_pizza = models.BooleanField() def __unicode__(self): return u"%s the restaurant" % self.name class Admin: pass
comment:3 by , 17 years ago
I believe this is a known problem. You are using queryset-refactor, model inheritance, and admin. Per Malcolm's note yesterday on the status of queryset-refactor:
http://groups.google.com/group/django-developers/msg/c0fb00dabb70c660
"model inheritance is not expected to work in admin at the moment".
comment:4 by , 17 years ago
Component: | Uncategorized → Admin interface |
---|
comment:5 by , 17 years ago
Description: | modified (diff) |
---|---|
Summary: | Admin site update error → Model Inheritance doesn't work in the admin. |
Triage Stage: | Unreviewed → Accepted |
As Karen said, MI doesn't yet work in the admin; updating the ticket to indicate that.
IMO, also, we could simply wait for newforms-admin to fix this; dunno how much that'll annoy people.
comment:6 by , 17 years ago
I don't think we want to merge half working stuff into the trunk, what I have heard suggested is merging qs-rf into nfa, and then when nfa merges we get both into trunk.
comment:7 by , 17 years ago
Alex, that isn't what is going to happen. Describing something as "half working" isn't accurate. It's fully working for the features implemented. The extra feature of being able to use it via admin is not implemented (making it completely equivalent to, say, generic relations). Since model inheritance is an entirely new feature, it's not going to hurt existing code for it not to be part of admin yet. Given the amount of work involved, I'm intending to make the newforms-admin changes required after queryset-refactor has been merged to trunk.
comment:8 by , 17 years ago
Version: | queryset-refactor → newforms-admin |
---|
comment:9 by , 17 years ago
Note that I'm not pushing the development burden here onto the newforms-admin guys. I'll do the work for this when I get some time. My last change was just to make sure it shows up in the right list of things that need doing.
comment:10 by , 17 years ago
#7138 (list_filter ignores foreign keys to inherited models) was closed as duplicate of this.
comment:11 by , 17 years ago
Cc: | added |
---|
comment:12 by , 17 years ago
Has patch: | set |
---|---|
Version: | newforms-admin → SVN |
We just ran into this problem today. Here is a small patch to fix it, rather self-explanatory I think, but you're welcome if you need some explanations...
comment:13 by , 16 years ago
Cc: | added |
---|
comment:14 by , 16 years ago
Cc: | added |
---|
comment:15 by , 16 years ago
Needs tests: | set |
---|
Hi,
Here's a simple use case (and traceback below)
from django.contrib.auth.models import User class Member(User): # a "multi-table" model, inheriting from the User model ...
Traceback: File "/home/erob/work/mos_australia/contrib/django/core/handlers/base.py" in get_response 82. response = callback(request, *callback_args, **callback_kwargs) File "/home/erob/work/mos_australia/contrib/django/contrib/admin/views/decorators.py" in _checklogin 62. return view_func(request, *args, **kwargs) File "/home/erob/work/mos_australia/contrib/django/views/decorators/cache.py" in _wrapped_view_func 44. response = view_func(request, *args, **kwargs) File "/home/erob/work/mos_australia/contrib/django/contrib/admin/views/main.py" in change_list 757. cl = ChangeList(request, model) File "/home/erob/work/mos_australia/contrib/django/contrib/admin/views/main.py" in __init__ 582. self.get_results(request) File "/home/erob/work/mos_australia/contrib/django/contrib/admin/views/main.py" in get_results 640. result_list = list(self.query_set) File "/home/erob/work/mos_australia/contrib/django/db/models/query.py" in __len__ 55. self._result_cache.extend(list(self._iter)) File "/home/erob/work/mos_australia/contrib/django/db/models/query.py" in iterator 162. for row in self.query.results_iter(): File "/home/erob/work/mos_australia/contrib/django/db/models/sql/query.py" in results_iter 200. for rows in self.execute_sql(MULTI): File "/home/erob/work/mos_australia/contrib/django/db/models/sql/query.py" in execute_sql 1466. cursor.execute(sql, params) File "/home/erob/work/mos_australia/contrib/django/db/backends/util.py" in execute 18. return self.cursor.execute(sql, params) File "/home/erob/work/mos_australia/contrib/django/db/backends/sqlite3/base.py" in execute 136. return Database.Cursor.execute(self, query, params) Exception Type: OperationalError at /admin/accounts/memberprofile/ Exception Value: no such column: accounts_member.id
Please note that I've applied the patch outlined above but it fails to solve
this problem. As a side note, if I add the following line to my Member class::
id = AutoField('id', primary_key=True, ...)
The exception value would express a different (but still apparently missing) column.
I did not tried with the newforms-admin branch, only trunk for now.
Any suggestions besides posting this on a mailing-list ?
Thanks,
Etienne
comment:16 by , 16 years ago
Patch needs improvement: | set |
---|
comment:17 by , 16 years ago
Cc: | added |
---|---|
Needs tests: | unset |
I encountered this as well, and I have a unit test that for it. The unit test identifies the problem I am having, and the patch does indeed fix it.
The problem is when you use a OneToOne field with the parent link option, so that the primary key has a different name in the parent and child tables. if the object you're working with has an existing primary key (object.pk is set), I don't think the sql shouldgenerate an insert instead of an update on the parent table.
class Place(models.Model): name = models.CharField(max_length=50, core=True) address = models.CharField(max_length=80, null=True) class Restaurant(Place): """ >>> p = Place.objects.create(name="Alice's", address="1 Main St", pk=1) >>> p.id 1 >>> r = Restaurant(serves_hot_dogs=True, serves_pizza=True) >>> r.pk = 1 >>> r.save() >>> r.pk 1 >>> r.place_id 1 """ serves_hot_dogs = models.BooleanField() serves_pizza = models.BooleanField() place = models.OneToOneField(Place, parent_link=True)
follow-up: 20 comment:18 by , 16 years ago
Etienne, I can't reproduce your problem (r7569 from trunk + my patch), with the following (and simple) models.py:
from django.contrib.auth.models import User class Member(User): class Admin: pass # Automatically filled, don't have to bother the user with this Member._meta.get_field('user_ptr').blank = True
Can you provide the full snippet causing the problem ?
follow-up: 21 comment:19 by , 16 years ago
Needs tests: | set |
---|
ekellner, I don't think you're testing the problem stated here. You're testing "what if the user create only the child class and manually make the association with the already existing parent class", or, in other words, "can we create the base and the child separately and make the link ad-hoc ?". Honestly, I don't know what's the "right thing to do" for this corner case. IMO, creating the base and the parent at different transactions is just *ugly*, and should be discouraged by stating that "behavior for this is not guaranteed and may change for the sake of django internal matters". You don't have to create a Place if you want to create a Restaurant, just create a Restaurant and django will create the Place for you. But I'm not a django dev, and they'll obviously have the last word, so I'd like to have their opinion : my patch don't allow to make this ad-hoc association, and I hardly see how to fix this bug, with allowing this corner-case and without making substantial core changes (far too much for my superficial knowledge of the django code, anyway). So if they think that the behavior change caused by my patch is just unacceptable, I just have to give up on this problem -- and sooner is better ;).
This problem is just "if base PK is unknown, it should be guessed from child PK, but instead django say 'no PK ? No problem, I'll just create a new record'"
Anyway, I'll try to make a test-case for this problem tomorrow.
comment:20 by , 16 years ago
Replying to sloonz:
Etienne, I can't reproduce your problem (r7569 from trunk + my patch), with the following (and simple) models.py:
from django.contrib.auth.models import User class Member(User): class Admin: pass # Automatically filled, don't have to bother the user with this Member._meta.get_field('user_ptr').blank = TrueCan you provide the full snippet causing the problem ?
Sloonz, here's the full snippet:
class Member(User): """The Member model""" objects = MemberManager() def __repr__(self): return "<Member: %s>" % self.username def __unicode__(self): return u"%s" % self.username def get_profile(self): """ This method shall return the ``MemberProfile`` object for the current (``Member``) instance. """ try: obj = MemberProfile._default_manager.get(user__id=self.id) except MemberProfile.DoesNotExist: raise Http404 else: if obj is not None: return obj class Meta: verbose_name = 'member' verbose_name_plural = 'members' app_label = 'accounts' db_table = 'accounts_member' class Admin: pass class MemberProfile(models.Model): user = models.ForeignKey(Member, unique=True, raw_id_admin=True) # more profile fields here
Notice that we overrided get_profile
in the Member class, since this
is what we want to do. Then I can call get_profile as usual and it should returns
the related profile for a given Member
instance:
>>> auth_user = Member(username='sloonz') >>> profile_data = MemberProfile(user=auth_user) >>> profile_data.save() >>> # Try retrieving the member data with get_profile >>> self.failUnlessEqual(profile_data == auth_user.get_profile(), True)
comment:21 by , 16 years ago
Replying to sloonz:
This problem is just "if base PK is unknown, it should be guessed from child PK, but instead django say 'no PK ? No problem, I'll just create a new record'"
I think you're right sloonz, in that this pinpoints the problem, but I think I am leaning towards a stronger statement. I don't think it's possible for the base PK to be unknown. It's model inheritence, and it has to be the same as the child PK. I could understand the viewpoint that says, if the code has gotten to this point, there's already something wrong.
comment:22 by , 16 years ago
Needs tests: | unset |
---|
@erob
Still can't reproduce your problem, but:
- I don't have the definition of MemberManager, I just commented out this line
- profile_data != auth_user.get_profile() because you didn't defined a cmp method, so your test is equivalent to "profile_data is auth_user.get_profile()". Since django doesn't reuse previous instances, this test will always fail
- with your code, I have a strange bug: adding a member in an empty database (only with the first user) overrides the first user...
Can you describe the exact steps to reproduce your bug ? Have you tried with a shiny new DB ?
@ekellner
It's model inheritence, and it has to be the same as the child PK
No, not in the case of multiple inheritance (yet the current implementation seems to be bogus on multiple inheritance) : if C inherits from A and B, since we can have A.pk != B.pk we can't have A.pk = C.pk and C.pk = B.pk.
comment:23 by , 16 years ago
Keywords: | nfa-blocker added |
---|---|
Version: | SVN → newforms-admin |
Making this an NFA-blocker, as per http://groups.google.com/group/django-developers/msg/02b044140a5f759f
comment:24 by , 16 years ago
I read Jacob's message to mean this should be nfa-someday, not nfa-blocker. If it can be done after the merge to trunk, then it shouldn't be nfa-blocker.
comment:26 by , 16 years ago
@Karen - You are correct. However, nfa-someday marks lot of things that _wont_ be on the list for v1.0, so to make sure it doesn't get forgotten, I stuck it in nfa-blocker. If it ends up happening post-merge, so be it.
by , 16 years ago
Attachment: | nfa_modelinheritance.1.diff added |
---|
comment:27 by , 16 years ago
I have tested the first use case in newforms-admin it the problem doesn't seem to exist there. Can people start banging against newforms-admin and report some specific issues they see. It will help get this done faster. I have attached a patch that would make sense for ModelForms and model inheritance.
comment:28 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | django-admin-nodup_new.patch added |
---|
sloonz's, compatible with newer revisions (currently 7634)
comment:29 by , 16 years ago
milestone: | → 1.0 alpha |
---|
comment:30 by , 16 years ago
Cc: | added |
---|
comment:31 by , 16 years ago
Cc: | added |
---|
comment:32 by , 16 years ago
Cc: | added |
---|
comment:33 by , 16 years ago
Cc: | added |
---|
comment:34 by , 16 years ago
Cc: | added |
---|
comment:35 by , 16 years ago
Although sloonz patch to prevent duplication works fine but was not merged with trunk, i don't understand the last line of code:
# Automatically filled, don't have to bother the user with this Member._meta.get_field('user_ptr').blank = True
Shouldn't it prevent the admin site to show the "User ptr" field for this model? It doesn't here with rev 7928. Note that i used the same exact proof-code from sloonz.
comment:36 by , 16 years ago
Keywords: | nfa-blocker removed |
---|
Removing the nfa-blocker keyword since this isn't a blocker, but since we have tagged it with 1.0 alpha that will be enough to ensure it is not forgotten. This should be dealt with once newforms-admin is merged to trunk.
comment:38 by , 16 years ago
Cc: | added |
---|
comment:39 by , 16 years ago
Version: | newforms-admin → SVN |
---|
comment:40 by , 16 years ago
Cc: | added |
---|
comment:41 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:42 by , 16 years ago
Status: | new → assigned |
---|
comment:43 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:44 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
The patch doesn't generate valid SQL on MySQL or either PostgreSQL backend. I'm fixing it now, but reopening in case anybody else notices and wonders if we've seen it.
comment:45 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
comment:46 by , 16 years ago
comment:47 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:49 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:50 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Mmm, it is indeed working on sqlite3 for at least the buildbot and me locally. Please open a separate ticket with more information about what "not working" means to you.
We're going to need a lot more information to be able to debug this one. Please read our guidelines for submitting bugs, and especially this:
Feel free to reopen with more info if you've got it.