Opened 11 years ago

Closed 11 years ago

Last modified 2 years ago

#19524 closed Bug (fixed)

DoesNotExist exception when adding object in admin with inline object

Reported by: qcwxezdas@… Owned by: Aymeric Augustin
Component: contrib.admin Version: 1.5-beta-1
Severity: Release blocker Keywords: admin, DoesNotExist, inlines
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

models.py

class Service(models.Model):
    provider = models.ForeignKey("services.ServiceProvider",
                                 verbose_name=_("Service provider"),
                                 related_name="services")
    title = models.CharField(_("Title"), max_length=150)
    # ... other fields ...

class EntertainmentType(models.Model):
    title = models.CharField(max_length=150, unique=True)

    def __unicode__(self):
        return self.title

    class Meta:
        ordering = ('title',)


class Entertainment(Service):
    types = models.ManyToManyField(EntertainmentType, verbose_name=_("Types"),
                                   related_name="entertainments")

    description = models.TextField(_("Description"), blank=True)
    # ... other fields ...

    def __unicode__(self):
        return self.title

    class Meta:
        ordering = ('title',)


class EntertainmentPhoto(models.Model):
    entertainment = models.ForeignKey(Entertainment, related_name="photos")
    description = models.TextField(_("Description"), max_length=1000,
                                   blank=True)
    photo = ImageWithThumbsField(_("Photo"), sizes=(),
                                 upload_to="services/photos/entertainment")
    is_main = models.BooleanField(verbose_name=_("Main"), default=False)

    class Meta:
        ordering = ('-is_main',)
        verbose_name = _("Photo")
        verbose_name_plural = _("Photos")

admin.py

from django.contrib import admin
from services.entertainment.models import Entertainment, EntertainmentType, EntertainmentPhoto

class EntertainmentTypeAdmin(admin.ModelAdmin):
    pass

class EntertainmentPhotoInline(admin.TabularInline):
    model = EntertainmentPhoto

class EntertainmentAdmin(admin.ModelAdmin):
    filter_horizontal = ["types"]
    list_display = Entertainment.list_display
    list_filter = ["provider", "types", ]
    inlines = [EntertainmentPhotoInline]

admin.site.register(Entertainment, EntertainmentAdmin)
admin.site.register(EntertainmentType, EntertainmentTypeAdmin)

If I add the Entertainment object without any EntertainmentPhoto inlines then everything wents correct. I am ABLE to add inline photos when editing created instance.

However, if I add photos when instance is not created (in add view), I get the following:

Environment:


Request Method: POST
Request URL: http://77.79.59.184:8000/admin/entertainment/entertainment/add/?provider=1

Django Version: 1.5b2
Python Version: 2.7.2
Installed Applications:
['south',
 'gunicorn',
 'django_extensions',
 'django.contrib.databrowse',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'django.contrib.admin',
 'authentication',
 'framework',
 'framework.menu',
 'services',
 'maps',
 'services.entertainment',
 'services.otherservices']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware')


Traceback:
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  116.                         response = callback(request, *callback_args, **callback_kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/contrib/admin/options.py" in wrapper
  370.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  89.         response = view_func(request, *args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/contrib/admin/sites.py" in inner
  202.             return view(request, *args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapper
  25.             return bound_func(*args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/utils/decorators.py" in bound_func
  21.                 return func(self, *args2, **kwargs2)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/db/transaction.py" in inner
  208.                 return func(*args, **kwargs)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/contrib/admin/options.py" in add_view
  1045.                 self.save_related(request, form, formsets, False)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/contrib/admin/options.py" in save_related
  762.             self.save_formset(request, form, formset, change=change)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/contrib/admin/options.py" in save_formset
  750.         formset.save()
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/forms/models.py" in save
  494.         return self.save_existing_objects(commit) + self.save_new_objects(commit)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/forms/models.py" in save_new_objects
  628.             self.new_objects.append(self.save_new(form, commit=commit))
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/forms/models.py" in save_new
  728.         pk_value = getattr(self.instance, self.fk.rel.field_name)
File "/Users/aemdy/virtualenvs/django1.5/lib/python2.7/site-packages/django/db/models/fields/related.py" in __get__
  389.             raise self.field.rel.to.DoesNotExist

Exception Type: DoesNotExist at /admin/entertainment/entertainment/add/
Exception Value: 

Attachments (3)

bug.zip (16.7 KB ) - added by qcwxezdas@… 11 years ago.
Sample project where I was able to get that exception.
19524.testcase.diff (3.6 KB ) - added by Aymeric Augustin 11 years ago.
19524.patch (4.4 KB ) - added by Aymeric Augustin 11 years ago.

Download all attachments as: .zip

Change History (11)

by qcwxezdas@…, 11 years ago

Attachment: bug.zip added

Sample project where I was able to get that exception.

comment:1 by Claude Paroz, 11 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

I've been able to reproduce the bug, which does not happen on 1.4.x branch. Bisecting the code, the faulty commit seems to be [b90d4e5b749be3b].

comment:2 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

Most likely the admin was relying on the ORM incorrectly returning None instead raising DoesNotExist.

comment:3 by Aymeric Augustin, 11 years ago

The bug happens when the parent model inherits another model.

I'm attaching a minimal failing test case.

by Aymeric Augustin, 11 years ago

Attachment: 19524.testcase.diff added

comment:4 by Aymeric Augustin, 11 years ago

And here's a patch.

by Aymeric Augustin, 11 years ago

Attachment: 19524.patch added

comment:5 by Aymeric Augustin, 11 years ago

I finally understood the reason for this regression.

Django has been caching related objects for a long time, but in an inconsistent way. Among other fixes, during the 1.5 development cycle, I added the ability to cache the absence of a related object. This is necessary for select_related to operate correctly on nullable foreign keys.

Unfortunately, I missed the case of unsaved objects. Before my changes, caching never worked on unsaved objects: since their pk was None, nothing was cached, and the relation was refreshed at each access, until both objects were in the database and the relation could be correctly loaded (and cached).

Now, the lack of a related object can be accidentally and mistakenly cached before the objects are saved. Hence the spurious DoesNotExist exception.


There are two approches to fix this problem:

1) Prevent the cache from being incorrectly populated in the first place:

  • This problem only occurs when the programmer (or Django) is accessing a relation that isn't saved to the database yet. That will return a wrong result. Sometimes it doesn't matter; still, it's a programming mistake that ought to be fixed.
  • Here, the problem is triggered by queryset.filter(**{self.fk.name: self.instance}) is BaseInlineFormSet.__init__. This eventually calls RelatedField._pk_trace, who follows the chain of related objects, populating caches in the process. Calling .filter(xxx=unsaved_instance) is a programming error, but Django currently doesn't detect it (see #17541).

2) Invalidate the cache when the relation is saved (in case it's been accidentally populated):

  • That's what my first patch does.
  • It mixes concerns between the related object descriptors and the base model class a bit :/
  • I've tried to audit for other instances of this problem but it's very difficult to achieve through code inspection.

Even though the first fix would be sufficient for this issue, I think the second one is worth including for extra safety. It's just too easy to accidentally populate the cache before saving an object, and it doesn't cost much to invalidate at this point.

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In e9c24bef74e55729b190cf07e0ac452aa4c86fcd:

Fix #19524 -- Incorrect caching of parents of unsaved model instances.

Thanks qcwxezdas for the report. Refs #13839.

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 5097d3c5faab2b6582c4cebee2b265fcdbb893eb:

[1.5.x] Fix #19524 -- Incorrect caching of parents of unsaved model instances.

Thanks qcwxezdas for the report. Refs #13839.

Backport of e9c24be.

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 18245b94:

Refs #7488, Refs #19524 -- Removed obsolete ModelInheritanceTest.test_issue_7488() test.

Obsolete since e9c24bef74e55729b190cf07e0ac452aa4c86fcd.

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