A bug in _create_formsets causes ModelAdmin methods to receive wrong argument values

[kbr-]

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

[Collin Anderson]

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

[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).

[Tim Graham]

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.

[kbr-]

Done. Check out the pull request.

[Tim Graham <timograham@…>]

In 0623f4dea46eefba46efde6c6528f7d813ef4391:

Fixed #23934 -- Fixed regression in admin views obj parameter.

[Tim Graham <timograham@…>]

In ccc30ffe57e5f1e4dbda371b8a1d00c19a3150aa:

[1.7.x] Fixed #23934 -- Fixed regression in admin views obj parameter.

Backport of 0623f4dea46eefba46efde6c6528f7d813ef4391 from master