Opened 10 years ago

Closed 10 years ago

Last modified 13 months 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@… 10 years ago.
Sample project where I was able to get that exception.
19524.testcase.diff (3.6 KB) - added by Aymeric Augustin 10 years ago.
19524.patch (4.4 KB) - added by Aymeric Augustin 10 years ago.

Download all attachments as: .zip

Change History (11)

Changed 10 years ago by qcwxezdas@…

Attachment: bug.zip added

Sample project where I was able to get that exception.

comment:1 Changed 10 years ago by Claude Paroz

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 Changed 10 years ago by Aymeric Augustin

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 Changed 10 years ago by Aymeric Augustin

The bug happens when the parent model inherits another model.

I'm attaching a minimal failing test case.

Changed 10 years ago by Aymeric Augustin

Attachment: 19524.testcase.diff added

comment:4 Changed 10 years ago by Aymeric Augustin

And here's a patch.

Changed 10 years ago by Aymeric Augustin

Attachment: 19524.patch added

comment:5 Changed 10 years ago by Aymeric Augustin

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 Changed 10 years ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 10 years ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 13 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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