Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#6755 closed (fixed)

Model Inheritance doesn't work in the admin.

Reported by: anonymous Owned by: mtredinnick
Component: contrib.admin Version: master
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: UI/UX:

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 6 years ago.
Fix (just a try...)
models.py (523 bytes) - added by sloonz 6 years ago.
Regression test
nfa_modelinheritance.1.diff (546 bytes) - added by brosner 6 years ago.
django-admin-nodup_new.patch (883 bytes) - added by Michael Placentra II <someone@…> 6 years ago.
sloonz's, compatible with newer revisions (currently 7634)

Download all attachments as: .zip

Change History (55)

comment:1 Changed 6 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 Changed 6 years ago by anonymous

  • Resolution invalid deleted
  • Status changed from closed to reopened

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 Changed 6 years ago by Karen Tracey <kmtracey@…>

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 Changed 6 years ago by vegpuff

  • Component changed from Uncategorized to Admin interface

comment:5 Changed 6 years ago by jacob

  • Description modified (diff)
  • Summary changed from Admin site update error to Model Inheritance doesn't work in the admin.
  • Triage Stage changed from Unreviewed to 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 Changed 6 years ago by Alex

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 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by mtredinnick

  • Version changed from queryset-refactor to newforms-admin

comment:9 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by ramiro

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

comment:11 Changed 6 years ago by Hangya

  • Cc evh293@… added

comment:12 Changed 6 years ago by sloonz

  • Has patch set
  • Version changed from newforms-admin to 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...

Changed 6 years ago by sloonz

Fix (just a try...)

comment:13 Changed 6 years ago by erwinelling

  • Cc erwin@… added

comment:14 Changed 6 years ago by erob

  • Cc robillard.etienne@… added

comment:15 Changed 6 years ago by erob

  • 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 Changed 6 years ago by erob

  • Patch needs improvement set

comment:17 Changed 6 years ago by ekellner <ekellner@…>

  • 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 follow-up: Changed 6 years ago by 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 ?

comment:19 follow-up: Changed 6 years ago by sloonz

  • 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 in reply to: ↑ 18 Changed 6 years ago by erob

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)

comment:21 in reply to: ↑ 19 Changed 6 years ago by ekellner <ekellner@…>

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 Changed 6 years ago by sloonz

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

Changed 6 years ago by sloonz

Regression test

comment:23 Changed 6 years ago by russellm

  • Keywords nfa-blocker added
  • Version changed from SVN to newforms-admin

comment:24 Changed 6 years ago by Karen Tracey <kmtracey@…>

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 Changed 6 years ago by Karen Tracey <kmtracey@…>

(unless the meanings have been changed?)

comment:26 Changed 6 years ago by russellm

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

Changed 6 years ago by brosner

comment:27 Changed 6 years ago by brosner

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 Changed 6 years ago by someone@…

  • Cc someone@… added

Changed 6 years ago by Michael Placentra II <someone@…>

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

comment:29 Changed 6 years ago by garcia_marc

  • milestone set to 1.0 alpha

comment:30 Changed 6 years ago by philwo

  • Cc philipp@… added

comment:31 Changed 6 years ago by anonymous

  • Cc cmawebsite@… added

comment:32 Changed 6 years ago by Wonlay

  • Cc wonlay@… added

comment:33 Changed 6 years ago by anonymous

  • Cc brooks.travis@… added

comment:34 Changed 6 years ago by anonymous

  • Cc jamespic@… added

comment:35 Changed 6 years ago by anonymous

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 Changed 6 years ago by brosner

  • 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 Changed 6 years ago by anonymous

Works with both brosner's and sloonz patchs

comment:38 Changed 6 years ago by anonymous

  • Cc newijk@… added

comment:39 Changed 6 years ago by Alex

  • Version changed from newforms-admin to SVN

comment:40 Changed 6 years ago by munhitsu

  • Cc mateusz@… added

comment:41 Changed 6 years ago by jacob

  • Owner changed from nobody to jacob
  • Status changed from reopened to new

comment:42 Changed 6 years ago by jacob

  • Status changed from new to assigned

comment:43 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:44 Changed 6 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 6 years ago by mtredinnick

  • Owner changed from jacob to mtredinnick
  • Status changed from reopened to new

comment:46 Changed 6 years ago by mtredinnick

(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 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 6 years ago by anonymous

Not working on SQLite3 backend yet.

comment:49 Changed 6 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:50 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from reopened to 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.

comment:51 Changed 3 years ago by jacob

  • milestone 1.0 alpha deleted

Milestone 1.0 alpha deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.