Opened 17 years ago

Closed 16 years ago

Last modified 13 years ago

#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 Jacob)

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)

django-admin-nodup.patch (800 bytes ) - added by sloonz 16 years ago.
Fix (just a try...)
models.py (523 bytes ) - added by sloonz 16 years ago.
Regression test
nfa_modelinheritance.1.diff (546 bytes ) - added by Brian Rosner 16 years ago.
django-admin-nodup_new.patch (883 bytes ) - added by Michael Placentra II <someone@…> 16 years ago.
sloonz's, compatible with newer revisions (currently 7634)

Download all attachments as: .zip

Change History (55)

comment:1 by Jacob, 17 years ago

Resolution: invalid
Status: newclosed

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:

Do write complete, reproducible, specific bug reports. Include as much information as you possibly can, complete with code snippets, test cases, etc. This means including a clear, concise description of the problem, and a clear set of instructions for replicating the problem. A minimal example that illustrates the bug in a nice small test case is the best possible bug report.

Feel free to reopen with more info if you've got it.

comment:2 by anonymous, 17 years ago

Resolution: invalid
Status: closedreopened

Sorry, I hope that this comment helps you to reproduce the behaviour that I think that is a bug.

Reproducing de bug

  1. 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
    
  1. Now, go to the admin site (<address to your app>/admin) and create a new Restaurant.
  2. 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 Karen Tracey <kmtracey@…>, 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 Vegpuff, 17 years ago

Component: UncategorizedAdmin interface

comment:5 by Jacob, 17 years ago

Description: modified (diff)
Summary: Admin site update errorModel Inheritance doesn't work in the admin.
Triage Stage: UnreviewedAccepted

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 Alex Gaynor, 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 Malcolm Tredinnick, 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 Malcolm Tredinnick, 17 years ago

Version: queryset-refactornewforms-admin

comment:9 by Malcolm Tredinnick, 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 Ramiro Morales, 17 years ago

#7138 (list_filter ignores foreign keys to inherited models) was closed as duplicate of this.

comment:11 by Ronkay János Péter, 17 years ago

Cc: evh293@… added

comment:12 by sloonz, 16 years ago

Has patch: set
Version: newforms-adminSVN

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...

by sloonz, 16 years ago

Attachment: django-admin-nodup.patch added

Fix (just a try...)

comment:13 by erwinelling, 16 years ago

Cc: erwin@… added

comment:14 by Etienne Robillard, 16 years ago

Cc: robillard.etienne@… added

comment:15 by Etienne Robillard, 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 Etienne Robillard, 16 years ago

Patch needs improvement: set

comment:17 by ekellner <ekellner@…>, 16 years ago

Cc: ekellner@… 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)

comment:18 by sloonz, 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 ?

comment:19 by sloonz, 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.

in reply to:  18 comment:20 by Etienne Robillard, 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 = True

Can 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)

in reply to:  19 comment:21 by ekellner <ekellner@…>, 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 sloonz, 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.

by sloonz, 16 years ago

Attachment: models.py added

Regression test

comment:23 by Russell Keith-Magee, 16 years ago

Keywords: nfa-blocker added
Version: SVNnewforms-admin

comment:24 by Karen Tracey <kmtracey@…>, 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:25 by Karen Tracey <kmtracey@…>, 16 years ago

(unless the meanings have been changed?)

comment:26 by Russell Keith-Magee, 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 Brian Rosner, 16 years ago

Attachment: nfa_modelinheritance.1.diff added

comment:27 by Brian Rosner, 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 someone@…, 16 years ago

Cc: someone@… added

by Michael Placentra II <someone@…>, 16 years ago

sloonz's, compatible with newer revisions (currently 7634)

comment:29 by Marc Garcia, 16 years ago

milestone: 1.0 alpha

comment:30 by Philipp Wollermann, 16 years ago

Cc: philipp@… added

comment:31 by anonymous, 16 years ago

Cc: cmawebsite@… added

comment:32 by Wonlay, 16 years ago

Cc: wonlay@… added

comment:33 by anonymous, 16 years ago

Cc: brooks.travis@… added

comment:34 by anonymous, 16 years ago

Cc: jamespic@… added

comment:35 by anonymous, 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 Brian Rosner, 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:37 by anonymous, 16 years ago

Works with both brosner's and sloonz patchs

comment:38 by anonymous, 16 years ago

Cc: newijk@… added

comment:39 by Alex Gaynor, 16 years ago

Version: newforms-adminSVN

comment:40 by munhitsu, 16 years ago

Cc: mateusz@… added

comment:41 by Jacob, 16 years ago

Owner: changed from nobody to Jacob
Status: reopenednew

comment:42 by Jacob, 16 years ago

Status: newassigned

comment:43 by Jacob, 16 years ago

Resolution: fixed
Status: assignedclosed

(In [8033]) Fixed #6755: model inheritance now works in the admin. Thanks, sloonz and Michael Placentra.

comment:44 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: closedreopened

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 Malcolm Tredinnick, 16 years ago

Owner: changed from Jacob to Malcolm Tredinnick
Status: reopenednew

comment:46 by Malcolm Tredinnick, 16 years ago

(In [8051]) Fixed the tests from [8033] to work for the PostgreSQL backends (the primary
key value isn't guaranteed to be the same across all backends). Still some
breakage on MySQL because of the way it treats booleans, but that's another
issue.

Refs #6755.

comment:47 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

Problem turned out to be that the tests were bad, not the patch content itself. That was fixed (for reasonably large values of "fixed") by [8050] and [8051].

comment:48 by anonymous, 16 years ago

Not working on SQLite3 backend yet.

comment:49 by anonymous, 16 years ago

Resolution: fixed
Status: closedreopened

comment:50 by Jacob, 16 years ago

Resolution: fixed
Status: reopenedclosed

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.

comment:51 by Jacob, 13 years ago

milestone: 1.0 alpha

Milestone 1.0 alpha deleted

Note: See TracTickets for help on using tickets.
Back to Top