New logic in ModelAdmin.add_view for model validation needs some re-thinking

[Brian Rosner]

The new code added in [12098] to ModelAdmin.add_view where it saves the object, but deletes it if formsets don't validate isn't going to fly well. This adds quite a bit of overhead in cases where a model's save and delete methods are non-trivial let alone the extra database activity. We really need to find a better way to accomplish this. I haven't yet worked why this is needed after the call to form.is_valid.

[jkocherhans]

form.is_valid has nothing to do with it because it's not about validating the form/model, it's about validating inlines. The inline models need a FK to their parent to pass model-validation. They can't get that FK value unless their parent *has* an PK, which you can't get without saving the parent model. Transactions would be a nice solution, but we can't use them (thanks, MyISAM.)

Some other options I see would be to:

• Exclude the FK field from model validation.
• Use some sort of sentinel value for the FK that will pass model validation, and set the real value later on.
• Only force the parent model commit if there are inlines.
• Use transactions if the db supports them, fall back to deletion if not.

None of those options are obviously any better to me, but I don't think the first three are really any worse either. The first two options have potential to cause exceptions at the database level. The third probably doesn't even really help that much, and I think the 4th is a non-starter. Maybe there's something Honza and I haven't thought about though.

Here are a few relevant spots in the code if anyone wants to dig into this. I won't be able to for a couple of days.

ForeignKey's validate method:
​http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py?rev=12098#L737

Model.full_validate(exclude=[]):
​http://code.djangoproject.com/browser/django/trunk/django/db/models/base.py?rev=12098#L788

Saving the parent object:
​http://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py?rev=12098#L764

Deleting it if formset validation fails:
​http://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py?rev=12098#L786

[Alex Gaynor]

The first 2 options seem massively better to me, you aren't incurring DB traffic that is totally unnecessary (and puts the DB in an inconsistent state). I'd like the first option, I know model-validation is designed to do a full model-validate at is_valid() time, but I think it needs to be possible to exclude fields from that entirely.

[Luke Plant]

Aside from the performance problem, another problem with this is if custom behaviour has been added to the model that is executed when the object is saved (or deleted). This is commonly achieved using signals or by overriding the 'save' method. Whether or not it is good practice, there will be cases where saving/deleting an object will have consequences outside the database — i.e. consequences that can't be rolled back even by transactions.

This consideration rules out the third and fourth solutions.

[jkocherhans]

So I've been working on this for a couple of hours, trying to get the form NOT to validate FKs in inlines. I thought maybe the patch from #12521 would get me all the way there but it doesn't, because the FK to the the parent IS displayed in the form as a hidden field, so validation happens, and fails on the model field.

I know I helped implement inline formsets, but for the life of me, I can't figure out why that field would be in the form. Is there some use case when the user should be able to change the FK value? Is it just for unique_together validation? I think [9297] is where it first went into the code, but I don't remember if we did something similar before that or not.

I think I can live with special casing InlineForeignKeyField validation. The other alternatives I see are letting the ValidationError happen and ignoring it (a hackier hack in my book), or trying to get rid of the hidden FK field all together (prohibitively difficult).

[jkocherhans]

Sorry, but combine this with the patch from #12521. Different problems, dependent solution.

[Russell Keith-Magee]

@jkocherhans - IIRC the reason the FK is a hidden field in the form is resolve objects in the presence of a custom order_by. If your inlines are displayed sorted according to a field other than the PK, then you need something fixed to ensure that the order in which they are updated matches the order in which they were originally sent. This has the added bonus of providing some protection against parallel edits (i.e., if I GET the edit page, then you submit a POST that deletes an object, my POST will fail because it contains invalid objects.

There is also a use case where end-users can modify the FK - when the FK has a to_column, and isn't a reference to the primary key. It's not a common use case, but it does exist.

[Luke Plant]

There are other problems with the logic change here. The removal of the call to save_model will cause major breakage (just spent several hours tracking a regression in django-cms2, and I was lucky, as the new behaviour was to throw an exception, and it could have been a silent bug). In short, before this changeset you could use save_model to fix up the instance before it hit the DB, or do other arbitrary work, which seemed to be the whole point of save_model. Now the method isn't called at all from add_view.

[Luke Plant]

Also, even without the logic problems, simply the change in method signature of save_form is a breaking change that would need to be listed in backwards incompatibilities if we keep this change. (Subclasses of ModelAdmin that implement save_form break with this change, since they don't have the commit keyword argument.

[jkocherhans]

(In [12206]) Fixed #12512. Changed ModelForm to stop performing model validation on fields that are not part of the form. Thanks, Honza Kral and Ivan Sagalaev.
This reverts some admin and test changes from [12098] and also fixes #12507, #12520, #12552 and #12553.

[Jacob]

Milestone 1.2 deleted