Promote modifying form.instance directly instead of using ModelForm.save(commit=False)

[Javier Candeira]

While doing the djangogirls tutorial, I noticed that ModelForm.save(commit=False) is given to newcomers as a reasonable way to acquire a form's populated model. This is an antipattern for several reasons:

• It's counterintuitive. To a newcomer, it's the same as save(save=no).
• It's a leaky abstraction. Understanding it requires understanding that the save() method does two things: a) return the model, and b) save it to the database.
• It doesn't name its effect or return well.

The first two issues are addressed in the current patch. About the third, the mailing list discussion convinced me to avoid the .get_updated_model() name. Instead, apply() was used.

Changes:

• Implement ModelForm.apply() as alternative to .save(commit=False)
• Implement FormSet.apply() as alternative to .save(commit=False)
• Refactor ModelForm.save() and FormSet.save() to allow the above.
• New tests have been added for the new methods in different contexts (from a ModelForm, from a FormSet, etc).
• documentation has also been modified to showcase the new methods.

Notes:

Uses of either .save(commit=False) in the codebase have been left alone wherever it was used for its side effects (the return value was not used).

The Djangogirls tutorial has a PR that depends on the changes in the present one:

​https://github.com/DjangoGirls/tutorial/pull/450

[Javier Candeira]

Note that ​https://github.com/DjangoGirls/tutorial/pull/450 depends on the present patch.

The changeset there explains the rationale for this addition better than anything else.

[Anssi Kääriäinen]

These methods seem like a good addition. The save(do_not_save=True) API isn't the best one in Django...

However we do need dedicated tests for the new methods as they are part of public API. Also, we need dedicated documentation for the new methods.

I don't see what side effects save(commit=False) has when compared to get_updated_model[s]()?

[Tim Graham]

This seems a bit confusing to me in that it hides the fact that you are calling super(), and I have never found the current API confusing (of course, I may be in the minority). Could you please write to the DevelopersMailingList to get some more opinions?

[Javier Candeira]

Replying to akaariai:

These methods seem like a good addition. The save(do_not_save=True) API isn't the best one in Django...

I'm glad I'm not the only one to see that.

However we do need dedicated tests for the new methods as they are part of public API. Also, we need dedicated documentation for the new methods.

Right. I'll get to both immediately. I thought the documentation would be picked up from the docstrings. I'll ask on IRC where to add the documentation for the new method if I can't find where.

I don't see what side effects save(commit=False) has when compared to get_updated_model[s]()?

First of all, thanks for your questions, which have made me check the repo again, and find required changes for this ticket which I had missed. Also, I'm kind of new to Django, so I may be wrong here... but I don't think I am.

Two reasons I think there are side effects:

1. The code in the tests calls save(commit=False) in two different ways, which makes me think that whoever wrote that code knows there are two reasons to call it.

./src/django$ rgrep "save(commit=False)"
(...)
tests/auth_tests/test_forms.py:        form.save(commit=False)
tests/auth_tests/test_forms.py:        form.save(commit=False)
tests/auth_tests/test_forms.py:        form.save(commit=False)
tests/auth_tests/test_forms.py:        form.save(commit=False)
tests/model_forms/tests.py:        price = form.save(commit=False)
tests/model_forms/tests.py:        c1 = f.save(commit=False)
tests/model_forms/tests.py:        new_art = f.save(commit=False)
tests/model_forms/tests.py:        doc = form.save(commit=False)
tests/model_forms/tests.py:        doc = form.save(commit=False)
tests/model_forms/tests.py:        doc = form.save(commit=False)
(...)

Sometimes the method is called for its return value (an instance of django.model), and some times it's not assigned to anything, which makes me think that the author of the code knows there are side effects in play.

2. From reading the code while writing the patch, I remember that, for instance, a ModelForm gets updated, e.g., with a save_m2m method that knows what information it should commit when form.save() is called with the default commit=True.

[Anssi Kääriäinen]

It seems some core developers don't find this change necessary. Before doing more work on this ticket you should post to the django-developers mailing list and check if we can get a consensus on moving forward with this.

[Javier Candeira]

Replying to akaariai:

It seems some core developers don't find this change necessary. Before doing more work on this ticket you should post to the django-developers mailing list and check if we can get a consensus on moving forward with this.

Well, I've finished all work on this ticket already. So I'll write to the list with what I have, and I'll take my lumps. Thanks a lot for your attention.

[Javier Candeira]

Hi, Tim, Thanks for your feedback.

Replying to timgraham:

This seems a bit confusing to me in that it hides the fact that you are calling super(),

I started to write a reply telling you why super() was incidental yada yada and, in justifying it, I found out that there was a bug in my code.

In my code as it was at the time of your comment, if you subclass a ModelForm, override save(), then call get_updated_model() on an instance of the subclass, it will call the _save() in the parent and ignore the overriden save(). That was bad.

Now that I've fixed it, and added a heap more tests and documentation, I have a much better explanation for *not* calling super.

All three of these chunks of code are equivalent (I've taken care to test all three, they are in tests/model_form_sets/tests.py):

        class PoemForm_currentAPI(forms.ModelForm):
            def save(self, commit=True):
                poem = super(PoemForm2, self).save(commit=False)
                poem.name = "%s by %s" % (poem.name, poem.poet.name)
                if commit:
                    poem.save()
                    self.save_m2m
                return poem

        class PoemForm_get_updated_model(forms.ModelForm):
            def save(self, commit=True):
                poem = self.get_updated_model()
                poem.name = "%s by %s" % (poem.name, poem.poet.name)
                if commit:
                    poem.save()
                    self.save_m2m
                return poem

        class PoemForm_save_via_form_not_model(forms.ModelForm):
            def save(self, commit=True):
                poem = self.get_updated_model()
                poem.name = "%s by %s" % (poem.name, poem.poet.name)
                if commit:
                    super(PoemForm_save_via_form_not_model, self).save()
                    # self.save_m2m called at the superclass's save()!
                return poem

You call super() when you need to access functionality of the same-named method in the superclass, because code reuse is better than code duplication. But super() is optional if you don't need access to such functionality.

Before the current patch, overriding save() needed to call save(commit=False) via the superclass's save() method to get the updated models. This call is still available for backwards compatibility, and about half the tests still use it.

Since all the functionality of the save(commit=False) path of the superclass is now available via get_updated_models(), so no need to call super() when reading a form's instance.

If one is overriding the commit=False path, they can do that by calling get_updated_model() and changing the instance before saving it or by overriding get_updated_model(), which is where the commit=False action is. They would then have to call super() to get to the superclass's get_updated_model().

(This is all now in the documentation for this patch)

Note that the current idiom in use in Django doesn't use the superclass's ModelForm.save() for actually committing the form's data. It calls the model instance's save() directly, and then it calls form.save_m2m(). The functionality that super() provides there is not considered important enough, and the little bit of duplicated code in calling self.save_m2m() is considered better for how it expresses the code's intent.

and I have never found the current API confusing (of course, I may be in the minority).

I only noticed that in the context of doing the djangogirls tutorial as a facilitator. I may be new to Django, but I'm not new to programming, so I understand why it's done or, at least, what it does under the covers. So not confusing to me either.

It's however interesting to see what a complete newcomer does with the same information from the tutorial. They have to take it as a dogma of faith, as one more place where computing is counterintuitive and not at all DWIM. I overheard one of the other facilitators apologising for the save(but_really_save=False) contradiction, and I cought myself also saying "this isn't good, but...".

In my opinion, any API that you have to apologise for to a new student is worth changing.

Could you please write to the DevelopersMailingList to get some more opinions?

I'm doing it now that I've made the changes prompted by akaariai, and refactored with more tests to make sure that I haven't introduced a new bug.

Thanks again for your time and attention.

[Tim Graham]

Tentatively accepting as a documentation issue based on latest discussion ​on django-developers.

[Tim Graham]

There wasn't consensus to move forward with this.