[Django] #23934: A bug in _create_formsets causes ModelAdmin methods to receive wrong argument values
Django
argument values
-------------------------------+---------------------------------------
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Severity: Normal | Keywords: ModelAdmin admin add_view
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------------------------
Hi. I have a bug to report as well as a solution to propose.
Let's take a look at the ModelAdmin API. Here we have a method,
get_formsets_with_inlines:
https://docs.djangoproject.com/en/1.7/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets_with_inlines
and another method which depends on how get_formsets_with_inlines is
executed; the get_fieldsets method:
https://docs.djangoproject.com/en/1.7/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_fieldsets.
"Depends" means that get_formsets_with_inlines makes decisions which
affect the way get_fieldsets is executed or even if it is executed at all.
First, get_formsets_with_inlines. Though it is not explicitly stated in
the documentation what the "obj" argument means, we can deduce it from the
example and other ModelAdmin methods' documentation (e.g. the previously
mentioned get_fieldsets). All circumstances given, the "obj" argument is
the model being edited during the add/change view. As mentioned in
get_fieldsets' documentation, obj is None when we are in an add/view. We
can guess this should also take place inside get_formsets_with_inlines,
and the example proves it (it says that maybe we'd want to display a
particular inline only inside the change view, and in the code we check
for this particular inline being iterated through along with obj being
None).
In the get_fieldsets method's documentation it is explicitly stated what
obj means and when it's None, and it behaves exactly the same way as in
get_formsets_with_inlines.
Further in this post I'll provide a simple project setup for the purpose
of demonstrating this ticket's issue. It will prove that indeed previously
mentioned methods behave like it is documented and said in previous
paragraphs... most of the time :)
For now let's look at ModelAdmin's code and analyze a particular flow
case, the one that's interesting for us. We're interested in the
changeform_view method which is being called by add_view or change_view
with object_id's value being None if it's add_view, not None otherwise.
What's interesting is what happens if we are adding a new model instance
and we're already after filling the form stage, so request.method ==
'POST'. Let's assume for simplicity that the form was filled correctly.
Let's also assume that the save_form method won't be replaced with a
virtual call to save_form in some derived class.
After some initial preparations, in L1428 we call _create_formsets with
current request as the first argument and new_object as the second
argument, where new_object is a valid model instance created from the
form's data, saved in memory but not yet commited to the database as
implied by previous assumptions.
Inside _create_formsets, L1740 we call get_formsets_with_inlines. The
first argument is, of course, the current request. Now the question is, do
we pass a second argument? In L1738 we make that decision based on if
obj.pk resolves to True. What exactly does this condition check? Let's go
back to our previous analysis. We deduced that get_formsets_with_inlines
has obj=None if we're adding a new object - _create_formsets then passes
only one argument, the request (it could pass two arguments, the second
being None, but it never does; L1738, for example, assumes that obj is
never None). So what we would like to have here is to check if we are
dealing with an object that we created just a while ago. This is
consistent with our previous analysis and with Django 1.6's behavior
(get_formsets, which is now replaced by get_formsets_with_inlines, indeed
had obj=None if and only if it was called from change_view).
So does this condition give correct results? Sometimes. It works as
intended in a typical use case. We have a model, we fill some of its'
fields in the form, we push 'Save' on the model creation page inside
Django Admin. Normally, the form doesn't have the primary key field's
value passed into it. That's the recommended way in Django, although the
other way is also allowed. When we don't pass the primary key explicitly
in the form then indeed 'if obj.pk' resolves to False, because 'pk' wasn't
generated yet.
Unfortunately, the 'if' resolves to True if we pass the primary key
field's value ourselves and get_formsets_with_inlines is then called with
obj not being None, which is inconsistent with the usual behavior. That
causes problems. Firstly, of course, the documentation is now wrong.
Secondly, even if we could write code which is somehow immune to this bug,
what about legacy code? I am currently trying to port a project from
Django 1.6 to 1.7 and there we have a situation where the assumption about
obj being None in get_formsets during model creation causes big problems.
Let's now take a look at a simple project I created to demonstrate this
issue: https://github.com/kbr-/_create_formsets-bug-demo.
It's a standard Django 1.7 project setup with Python 3 in which testapp
was created and added to settings.py, in which 4 models were added with
corresponding admin classes. If you don't have python 3, create a
virtualenv. Then clone the project, run ./manage.py migrate, ./manage.py
createsuperuser and finally ./manage.py runserver. You can now look at the
messages written to the console.
First, let's go to the admin, login and start adding a new 'good model'.
The console says that 'GoodModelAdmin.get_formsets_with_inlines' and
'RelatedToGoodModelInline.get_fieldsets' were called, both with obj =
None. The form asks for new good model's name and we can also add some
inlines. Let's provide a name (it's an integer) and click 'Save'. The
console again says that these methods were called with obj = None. This is
consistent with the documentation and our analysis.
Now edit admin.py in testapp, comment out the 'super' call and uncomment
the originally commented four lines inside
GoodModelAdmin.get_formsets_with_inlines. This is the example provided in
get_formsets_with_inlines' documentation: we don't want the inline to be
edited during model creation. Now go to the admin and add another good
model. As expected, there are no inlines. Provide a name (different than
before) and click 'Save'. The console provided us valid debug info:
get_formsets_with_inlines was called twice (first during GET, second
during POST) with obj=None, and get_fieldsets wasn't called a single time.
So far so good.
Now let's add a bad model. The form shows up and console says what it
should've said. Let's provide an Id (which is also a primary key!) and
click 'Save'. Now look into the console. As we can see, during POST both
get_formsets_with_inlines and get_fieldsets were called with obj being
equal to the Id we provided (the result of calling __str__). I hope I
already convinced you that this is wrong. But still not a disaster,
nothing happened, right?
Now again edit admin.py in testapp and do the same thing for BadModelAdmin
you previously did for GoodModelAdmin. Go to the 'Add bad model' form. For
now we get the same thing we had when testing GoodModelAdmin. Now provide
an Id and click 'Save'. The console says that we called
get_formsets_with_inlines with obj not being None. Then the thing we
certainly didn't want to happen happened: get_fieldsets on the inline was
called. And a weird exception was thrown.
So this is it. If I understand correctly, the solution is simple. I
provided it here: https://github.com/django/django/pull/3637
Apply it in your Django copy and see that the expected thing happens when
we try to create a new 'bad model'.
--
Ticket URL: <https://code.djangoproject.com/ticket/23934>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage:
add_view | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:1>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage:
add_view | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
Are you saying that in 1.6 you could tell whether or not you were "adding"
based on whether an object was passed in, and in 1.7 it's not possible to
tell whether you're "adding" or "changing"?
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:2>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage:
add_view | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by kbr-):
Yes. As I said, in 1.6, the get_formsets ModelAdmin method had obj==None
if and only if we were "adding". Basing on that we could, for example,
decide if we want to edit particular inlines during addition (it's the
example given in the documentation:
https://docs.djangoproject.com/en/1.7/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_formsets;
the same example should work for 1.7).
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:3>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage: Accepted
add_view | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* severity: Normal => Release blocker
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
I guess the regression was introduced in
402b4a7a20a4f00fce0f01cdc3f5f97967fdb935. If you can add a test and
release notes in docs/releases/1.7.2.txt we can backport the fix and
include it on the next minor release.
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:4>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage: Accepted
add_view | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by kbr-):
Done. Check out the pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:5>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.7
Keywords: ModelAdmin admin | Triage Stage: Accepted
add_view | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:6>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.7
Severity: Release blocker | Resolution: fixed
Keywords: ModelAdmin admin | Triage Stage: Accepted
add_view | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"0623f4dea46eefba46efde6c6528f7d813ef4391"]:
{{{
#!CommitTicketReference repository=""
revision="0623f4dea46eefba46efde6c6528f7d813ef4391"
Fixed #23934 -- Fixed regression in admin views obj parameter.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:7>
Django
argument values
Reporter: kbr- | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: 1.7
Severity: Release blocker | Resolution: fixed
Keywords: ModelAdmin admin | Triage Stage: Accepted
add_view | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"ccc30ffe57e5f1e4dbda371b8a1d00c19a3150aa"]:
{{{
#!CommitTicketReference repository=""
revision="ccc30ffe57e5f1e4dbda371b8a1d00c19a3150aa"
[1.7.x] Fixed #23934 -- Fixed regression in admin views obj parameter.
Backport of 0623f4dea46eefba46efde6c6528f7d813ef4391 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23934#comment:8>