Django

Code

Ticket #6755 (reopened)

Opened 4 months ago

Last modified 7 hours ago

Model Inheritance doesn't work in the admin.

Reported by: anonymous Assigned to: nobody
Milestone: 1.0 alpha Component: Admin interface
Version: SVN Keywords:
Cc: evh293@freemail.hu, newijk@gmail.com, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com, jamespic@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

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

django-admin-nodup.patch (0.8 kB) - added by sloonz on 05/21/08 14:36:41.
Fix (just a try...)
models.py (0.5 kB) - added by sloonz on 06/05/08 16:19:44.
Regression test
nfa_modelinheritance.1.diff (0.5 kB) - added by brosner on 06/12/08 18:25:15.
django-admin-nodup_new.patch (0.9 kB) - added by Michael Placentra II <someone@michaelplacentra2.net> on 06/14/08 16:47:27.
sloonz's, compatible with newer revisions (currently 7634)

Change History

03/11/08 14:13:22 changed by jacob

  • status changed from new to closed.
  • needs_better_patch changed.
  • resolution set to invalid.
  • needs_tests changed.
  • needs_docs changed.

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.

03/12/08 05:24:13 changed by anonymous

  • status changed from closed to reopened.
  • resolution deleted.

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

03/12/08 06:10:25 changed by Karen Tracey <kmtracey@gmail.com>

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

03/17/08 12:37:49 changed by vegpuff

  • component changed from Uncategorized to Admin interface.

03/31/08 13:56:23 changed by jacob

  • summary changed from Admin site update error to Model Inheritance doesn't work in the admin..
  • description changed.
  • 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.

03/31/08 14:30:58 changed 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.

04/14/08 21:12:02 changed 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.

04/26/08 22:06:41 changed by mtredinnick

  • version changed from queryset-refactor to newforms-admin.

04/26/08 22:13:49 changed 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.

04/30/08 05:43:07 changed by ramiro

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

05/09/08 09:42:33 changed by Hangya

  • cc set to evh293@freemail.hu.

05/21/08 14:36:00 changed by sloonz

  • has_patch set to 1.
  • 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...

05/21/08 14:36:41 changed by sloonz

  • attachment django-admin-nodup.patch added.

Fix (just a try...)

05/28/08 07:56:49 changed by erwinelling

  • cc changed from evh293@freemail.hu to evh293@freemail.hu, erwin@eight.nl.

05/31/08 10:53:54 changed by erob

  • cc changed from evh293@freemail.hu, erwin@eight.nl to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com.

05/31/08 12:11:30 changed by erob

  • needs_tests set to 1.

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

05/31/08 12:12:10 changed by erob

  • needs_better_patch set to 1.

06/03/08 05:04:14 changed by ekellner <ekellner@gmail.com>

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com.
  • needs_tests deleted.

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 ) 06/03/08 13:49:27 changed 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 ?

(follow-up: ↓ 21 ) 06/03/08 14:41:12 changed by sloonz

  • needs_tests set to 1.

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 ) 06/03/08 15:58:04 changed 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)

(in reply to: ↑ 19 ) 06/04/08 06:48:37 changed by ekellner <ekellner@gmail.com>

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.

06/05/08 16:18:17 changed by sloonz

  • needs_tests deleted.

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

06/05/08 16:19:44 changed by sloonz

  • attachment models.py added.

Regression test

06/11/08 22:25:31 changed by russellm

  • keywords set to nfa-blocker.
  • version changed from SVN to newforms-admin.

06/11/08 22:29:33 changed by Karen Tracey <kmtracey@gmail.com>

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.

06/11/08 22:30:22 changed by Karen Tracey <kmtracey@gmail.com>

(unless the meanings have been changed?)

06/11/08 22:35:24 changed 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.

06/12/08 18:25:15 changed by brosner

  • attachment nfa_modelinheritance.1.diff added.

06/12/08 18:26:34 changed 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.

06/14/08 15:31:48 changed by someone@michaelplacentra2.net

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net.

06/14/08 16:47:27 changed by Michael Placentra II <someone@michaelplacentra2.net>

  • attachment django-admin-nodup_new.patch added.

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

06/16/08 10:49:28 changed by garcia_marc

  • milestone set to 1.0 alpha.

06/29/08 06:26:53 changed by philwo

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de.

07/07/08 10:58:05 changed by anonymous

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com.

07/13/08 03:56:07 changed by Wonlay

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com.

07/14/08 13:36:54 changed by anonymous

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com.

07/15/08 15:52:44 changed by anonymous

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com to evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com, jamespic@gmail.com.

07/15/08 15:58:11 changed 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.

07/15/08 15:59:43 changed by brosner

  • keywords deleted.

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.

07/17/08 09:22:50 changed by anonymous

Works with both brosner's and sloonz patchs

07/19/08 09:18:41 changed by anonymous

  • cc changed from evh293@freemail.hu, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com, jamespic@gmail.com to evh293@freemail.hu, newijk@gmail.com, erwin@eight.nl, robillard.etienne@gmail.com, ekellner@gmail.com, someone@michaelplacentra2.net, philipp@igowo.de, cmawebsite@gmail.com, wonlay@gmail.com, brooks.travis@gmail.com, jamespic@gmail.com.

07/19/08 16:38:54 changed by Alex

  • version changed from newforms-admin to SVN.

Add/Change #6755 (Model Inheritance doesn't work in the admin.)




Change Properties
Action