#25227 Add utility method `get_updated_model()` to `ModelForm`

[Javier Candeira]

Hi, I'm the author of Ticket #25227 Add utility method `get_updated_model()` to `ModelForm` and its accompanying patch.

Ticket: https://code.djangoproject.com/ticket/25227

Patch: https://github.com/django/django/pull/5105

Here's the writeup to save everyone a click:

Add utility method get_updated_model() to ModelForm

Additionally, add utility method get_updated_models() to FormSet

Rationale:

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) prepare the model, and b) save it to the database.

  - It doesn't name its effect or return well.

All these problems are addressed in the current patch. Changes:

 - Implement ModelForm.get_updated_model()

 - Implement FormSet.get_updated_models()

 - Refactor ModelForm.save() and FormSet.save() to allow the above.

 - Both the tests and contrib.auth have been modified: any call to save(commit=False) for the purpose of obtaining a populated model has been replaced by get_updated_model(). Tests still pass, I'm confident it's a successful refactor.

- 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 ModelForm.save(commit=False) in the codebase have been left alone wherever it was used for its side effects and not for its return value.

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

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

[Tim Graham]

Discouraging the use of super() seems like a bad idea that could limit flexibility in the future. I think Django's documentation describes the behavior pretty well. Perhaps the Django Girls tutorial could be improved instead. I don't recall having trouble understanding how this worked when I learned Django and introducing a new way to duplicate functionality of a 10 year old pattern doesn't seem worth it to me.

https://docs.djangoproject.com/en/1.8/topics/forms/modelforms/#the-save-method

[Javier Candeira]

On Friday, 7 August 2015 01:57:46 UTC+10, Tim Graham wrote:

Discouraging the use of super() seems like a bad idea that could limit flexibility in the future. 

Fair enough, but I'm not discouraging the use of super(). As I point out in the ticket, the recommended pattern already ignores super() in the commit=True path of the code, since it suggests this:

class MyForm(ModelForm):

    def save(commit=True):

        model = super(MyForm, self).save(commit=False)

        # do stuff to model

        if commit:

            model.save()

            form.save_m2m()

Instead of this:

class MyForm(ModelForm):

    def save(commit=True):

        model = super(MyForm, self).save(commit=False)

        # do stuff to model

        if commit:

            super(MyForm, self).save()

These two are equivalent, the second one would actually use super() for saving and committing. But Django prefers not to.

So if there is a reason for rejecting my patch, "discouraging use of super()" should hardly be it.

I could also include a documentation patch recommending the use of super() for the commit=True path of save(), but I think practicality beats purity, Django seems to agree, and this is the better code:

class MyForm(ModelForm):

    def save(commit=True):

        model = self.get_updated_model()

        # do stuff to model

        if commit:

            model.save()

            form.save_m2m() 

> I think Django's documentation describes the behavior pretty well. Perhaps the Django Girls tutorial could be improved instead. I don't recall having trouble understanding how this worked when I learned Django and introducing a new way to duplicate functionality of a 10 year old pattern doesn't seem worth it to me.

> https://docs.djangoproject.com/en/1.8/topics/forms/modelforms/#the-save-method

Django documentation is stellar. This means that the "save(save=False)" API antipattern is very well documented indeed. This doesn't make it less of an antipattern for beginners, however well we explain it or, indeed, however well us-who-already-understand-it already understand it. I had been programming for decades when I learnt Django. Many people following the tutorials haven't. In some cases, they haven't even been alive for one decade. 

I'd like to hear the opinion of people who teach newcomers to programming, but let me also point this out: in separating the commit=False and commit=True paths of ModelForm.save() into the two functions get_updated_instance() and save_instance(), this refactor also enhances the readability of the code to a prospective Django contributor diving into the source code for the first time.

Thanks,

Javier

[Marc Tamlyn]

I agree that this is an anitpattern. I'm not fond of `get_updated_instance()` as a name, I'm not sure what the correct option is here. Arguably we should split save() into two parts - ModelForm.update_instance() and ModelForm.commit(). ModelForm.save() simply calls these two functions, with the latter only if commit=True.

update_instance() could return the instance, but I'd prefer to suggest manipulating form.instance in the view.

***

Really though, I'd like to see the Django (and Django girls) tutorials not use this pattern at all - the pattern of moving this logic out of the form and into the view is the true antipattern here. In the case of the example in the Django girls tutorial, it's a canonical example of adding the user to the saved instance. There are in fact two better ways to achieve this.

One is to pass in an instance in creation of the form - `PostForm(instance=Post(user=request.user))`.

The other, and my preferred option, is that the PostForm takes `user` as a kwarg, and does this automatically in its save method. This encapsulates the logic of the form completely, and also means you can add extra validation at the form level. For example, you could check whether the user is an admin or not and allow admins to post longer messages than logged in users, or include links.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4246611e-2e41-4680-9905-63dc0aa63147%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Javier Candeira]

On Friday, 7 August 2015 07:19:01 UTC+10, Marc Tamlyn wrote:

I agree that this is an anitpattern. I'm not fond of `get_updated_instance()` as a name, I'm not sure what the correct option is here. Arguably we should split save() into two parts - ModelForm.update_instance() and ModelForm.commit(). ModelForm.save() simply calls these two functions, with the latter only if commit=True.

get_updated_instance(form, instance, *args) is the module-level function, parallell to save_instance(form, instance, *args). The exposed method I suggest is ModelForm.get_updated_model(self), which fits imho because at this point we are coding in a domain where talking about forms and models. 

update_instance() could return the instance, but I'd prefer to suggest manipulating form.instance in the view.

This would be one(of many) more OO way of doing it, having update_model() be a mutator method that doesn't return an instance, and then accessing the instance through regular attribute access:

class MyForm(ModelForm):

    def save():

        # this would return None:

        form.update_model()  

        # in case we're editing and not creating

        form.model.author = form.model.author if form.model.author else request.user  

        form.model.last_edited_by = request.user

        super(MyForm, self).save()

Note that I'm not advocating this, because I didn't want to make breaking changes. I just wanted to make things clearer for beginners without breaking backwards compatibility for existing users.

Also, the above assumes the work is still going to be done in the view. Putting all the work in the form sounds better, as you say:

One is to pass in an instance in creation of the form - `PostForm(instance=Post(user=request.user))`.

This looks like a poster case for a default value. In the field definition, pass an argument "default = get_user_from_current_request" where get_user_from_current_request() does what it says**, and done. But, for other cases (or for overriding the default in this case), your second option is better.

The other, and my preferred option, is that the PostForm takes `user` as a kwarg, and does this automatically in its save method. This encapsulates the logic of the form completely, and also means you can add extra validation at the form level. For example, you could check whether the user is an admin or not and allow admins to post longer messages than logged in users, or include links.

Agreed. Something like "if input would validate, take it, but if not, put this instead". However, this would be a different ticket and a different pull request.

Since this is my first proposed contribution, would you mind eyeball-linting my code and telling me what you think, independently of whether the PR gets accepted or not?

Thanks again,

Javier Candeira

 

** Note: I haven't looked whether something like this is easy to do, but if it isn't, I feel another itch to scratch coming up.

[Tim Graham]

I took a look at the code, found a bug, and proposed some simplifications in https://github.com/django/django/pull/5111

The simplifications help make it clearer what get_updated_model() would do in the usual case:
1. Throw an error if the form has any errors: "The model could not be changed because the data didn't validate."
2. Adds a save_m2m() method to the form.

It doesn't actually update the instance at all, so I guess the proposal is a bit misleading (unless I missed something).

p.s. You can give your code a "self-review" with the patch review checklist: https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code/submitting-patches/#patch-review-checklist

[Javier Candeira]

On Friday, 7 August 2015 08:38:33 UTC+10, Tim Graham wrote:

I took a look at the code, found a bug, and proposed some simplifications in https://github.com/django/django/pull/5111

Thanks, will check. 

The simplifications help make it clearer what get_updated_model() would do in the usual case:
1. Throw an error if the form has any errors: "The model could not be changed because the data didn't validate."
2. Adds a save_m2m() method to the form.

It doesn't actually update the instance at all, so I guess the proposal is a bit misleading (unless I missed something).

Yeah, I thought it did, but what it changes is the form (adding the save_m2m method). Maybe change name to get_model()?

 

p.s. You can give your code a "self-review" with the patch review checklist: https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code/submitting-patches/#patch-review-checklist

Thanks. I've noticed that github tells me my code doesn't pass flake8 either. I've recently changed to Spacemacs and couldn't get flake to work with it yesterday. I'll try before I update the PR/Ticket and write here again.

Cheers,

[Javier Candeira]

What I said on the tracker for #25241:

Thanks for this. You're cleaning here a lot of of cruft that I left in my own #25227 because I didn't know whether it was used from other parts of Django.

I assume this [#25241] is as good as merged. I'll rebase my patch on your one before continuing with the #25227 discussion.

Cheers,

Javier

On Friday, 7 August 2015 08:38:33 UTC+10, Tim Graham wrote:

[Tim Graham]

I don't think 'get_model()' makes it any more obvious about the two things that happen as compared to the name 'super().save(commit=False)'.

If we reevaluate as to whether super().save(commit=False) is needed at all, I think the forms.errors check isn't of much value (you will only hit that if you have in error in your code where you forgot to call is_valid()) and save_m2m() could possibly be regular method that doesn't need to be conditionally added. But really, I don't think the current situation is so bad that we need to churn this. It doesn't seem feasible for the commit keyword to be deprecated, so now we would two ways of doing something.

[Javier Candeira]

Hi, Tim, 

Thanks for taking the time to address my ticket and patch.

At this point I'm aware that I might be doing this just to practice writing well-formed contributions to Django. At the very least I'll have learnt a lot about how Django works on the inside, both the code and the project.

However, other developers have agreed with me that the save(commit=False) is an antipattern. My goal is to remove that from tutorials, not to add a get_updated_model() method. This was only a means to an end.

For that reason I still think it's worth for me to have one more go at the problem, and see if I can propose a different solution.

If we reevaluate as to whether super().save(commit=False) is needed at all, I think the forms.errors check isn't of much value (you will only hit that if you have in error in your code where you forgot to call is_valid()) and save_m2m() could possibly be regular method that doesn't need to be conditionally added. But really, I don't think the current situation is so bad that we need to churn this. It doesn't seem feasible for the commit keyword to be deprecated, so now we would two ways of doing something.

In that case, a possible approach could be to turn `save(commit=False)` into a does-nothing path, and suggest that the new way of doing things is:

class ThingForm(ModelForm):

    def save(self, commit=True):

        self.instance.author = request.user

        if commit:

            super(ThingForm, self).save()

As a first step to eventually deprecating commit, since it doesn't do anything useful except return the instance, and suggest that the recommended idiom is:

class ThingForm(ModelForm):

    def save(self):

        self.instance.author = request.user

        super(ThingForm, self).save()

[Tim Graham]

That sounds like a more promising proposal in my mind.

So in a view instead of:

if form.is_valid():
   obj = form.save(commit=False)
   obj.author = request.user
   obj.save()
   form.save_m2m()

it might look like:

if form.is_valid():
   form.instance.author = request.user
   form.save()

I'm not sure if this will work (or result in simpler code) for model formsets though as there is the interaction with deleted formsets to consider.

[Curtis Maloney]

Just my $0.02... I quite like the idea of adding update_instance and
commit, with save calling both...

To me, this provides a clean and obvious place to customise updating
the instance before saving... as well as a clear place to get an
updated instance without saving.

--
Curtis

> https://groups.google.com/d/msgid/django-developers/6f16fc4e-e6aa-4d61-90ec-d6f78b55cff8%40googlegroups.com.

[Martin Owens]

Add utility method get_updated_model() to ModelForm

I think the problem I have with the name here is that I often think of the Model as the class definition and not the instance object that the model makes. When I got passing around model classes like a mad django diva, I might get confused if I think get_updated_model might return a model class instead of an instance.

Could we use something simple like 'update()' and 'commit()' to which save would call both?

Martin Owens,

[Patryk Zawadzki]

2015-08-07 16:51 GMT+02:00 Martin Owens <doct...@gmail.com>:
> Could we use something simple like 'update()' and 'commit()' to which save
> would call both?

Or `.apply()` and `.commit()` not to suggest that the form gets
updated in any way.

--
Patryk Zawadzki
I solve problems.

[Javier Candeira]

On Saturday, 8 August 2015 02:38:09 UTC+10, Patryk Zawadzki wrote:

2015-08-07 16:51 GMT+02:00 Martin Owens <doct...@gmail.com>:
> Could we use something simple like 'update()' and 'commit()' to which save
> would call both?

Or `.apply()` and `.commit()` not to suggest that the form gets
updated in any way.

I like `.apply()`, since it can either suggest that it's applying the form's data to the instance, or applies the validation rules to the form data. 

J

[Javier Candeira]

Ok, so I rebased on Tim Graham's refactor and bugfix of ModelForm, and now the API is as follows:

`.save()` commits to the database. In `ModelForm`, it returns the associated instance, and in `FormSet`, the list of associated instances. It runs `.apply()` first.

`.apply()` just returns the instance or the associated instances. It's also the bit that throws exceptions when data doesn't pass validation, and the part that prepares the m2m() method when the saving is not going to happen right now.

`.save(commit=False)` is the same as `.apply()`, they're synonyms.

`.save(commit=True)` is the same as `.save()`. 

I have also changed the title description of the Ticket, because I realised that the goal was to avoid the `save(commit=False)`, not to add the new method with a particular name. The new method was just a way to get at the goal.

Now in a view for a ThingForm you can do:

if form.is_valid():

    thing = form.apply()

    do_stuff_to(thing)

    form.save()

or 

if form.is_valid():

    thing = form.apply()

    do_stuff_to(thing)

    thing.save()

    form.save_m2m()

or even this one, though it goes against tradition by ignoring the returned instance from form.save():

if form.is_valid():

    form.apply()

    do_stuff_to(form.instance)

    form.save()   

And, in a subclass of the Thing model:

class Gizmo(Thing):

    def save(self, commit=True):

        gizmo = self.apply()

        do_stuff_to(gizmo)

        if commit:

            return super(Gizmo, self).save()

        else:

            return gizmo

or you can override the apply() method, which to me looks cleaner, but if we were to go this way we'd require further intervention in the docs:

class Gizmo(Thing):

    def apply(self):

        gizmo = super(Gizmo, self).apply()

        do_stuff_to(gizmo)

        return gizmo

Depending on what the stuff done to gizmo is, whether it's instance manipulation prior to giving it to the developer in the view, or prior to committing it to the database after the user has manipulated it.

Cheers,

Javier

[Tim Graham]

"I like `.apply()`, since it can either suggest that it's applying the form's data to the instance, or applies the validation rules to the form data."

I don't think it does either of those things though? I don't find the name intuitive for what it does. I think the purpose of "save(commit=False)" is for the case when you need to modify a model instance *outside* of the form. If you can modify the instance inside the form, just do so in save().

What happened to the idea of removing the need for the commit keyword arg that you presented earlier? As I said, I thought that was pretty good, except some investigation of whether or not it'll work well for formsets is needed.

[Javier Candeira]

On Monday, 10 August 2015 21:36:27 UTC+10, Tim Graham wrote:

"I like `.apply()`, since it can either suggest that it's applying the form's data to the instance, or applies the validation rules to the form data."

I don't think it does either of those things though? I don't find the name intuitive for what it does. I think the purpose of "save(commit=False)" is for the case when you need to modify a model instance *outside* of the form. If you can modify the instance inside the form, just do so in save().

So the only thing it does is throw an exception if there is a validation error, and prepare m2m() so the m2m data will be saved later? 

 

What happened to the idea of removing the need for the commit keyword arg that you presented earlier? As I said, I thought that was pretty good, except some investigation of whether or not it'll work well for formsets is needed.

Good catch. I got distracted rebasing from your patch, then I realised I don't know how deprecation works inside the Django project, and somewhere along the way I guess I suffered a loss of ambition.

I'll give the patch another spin, try to understand what the commit=False codepath does that can be done when instantiating the form so save() doesn't need the commit keyword. 

What's your opinion on turning `ModelForm.instance` into a property that would get/set an _instance attribute, and perform the same side effects as save(commit=False) perform now when returning the model instance? To me this looks like it could work with `FormSet.instances` too, and it would avoid having to find a name for the method, since `apply` and `get_updated_instance` did not work. `.instance` just gets and sets instance/instances, and does whatever preparation is needed in the background.

Also, is there anywhere I have missed in the documentation that explains how deprecation works in the project? If we go for such a break with the existing API, my patch would need to keep the current API working for a while, and the documentation would have to document both APIs in parallel for that time, then deprecate the `commit=False` path.

Thanks,

J

[Javier Candeira]

On Monday, 10 August 2015 22:30:51 UTC+10, Javier Candeira wrote:

Also, is there anywhere I have missed in the documentation that explains how deprecation works in the project? If we go for such a break with the existing API, my patch would need to keep the current API working for a while, and the documentation would have to document both APIs in parallel for that time, then deprecate the `commit=False` path.

Answering myself: http://django.readthedocs.org/en/latest/internals/release-process.html#deprecation-policy

J

[Tim Graham]

Yes, the idea is that ModelForm.save() method could become (after deprecation finishes):

def save(self):
    """
    Save this form's self.instance object and M2M data. Return the model
    instance.
    """
    if self.errors:
        raise ValueError(
            "The %s could not be %s because the data didn't validate." % (
                self.instance._meta.object_name,
                'created' if self.instance._state.adding else 'changed',
            )
        )
    self.instance.save()
    self._save_m2m()
    return self.instance

Instead of calling super().save(commit=False) to retrieve form.instance, you can just manipulate form.instance directly and then call super().save(). I don't see a need for the _instance getters/setters you proposed.

There's also a deprecation checklist to help when writing a patch:
https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code/submitting-patches/#deprecating-a-feature

[Carl Meyer]

Hi Tim,

On 08/10/2015 11:07 AM, Tim Graham wrote:
> Yes, the idea is that ModelForm.save() method could become (after
> deprecation finishes):
>
> def save(self):
> """
> Save this form's self.instance object and M2M data. Return the model
> instance.
> """
> if self.errors:
> raise ValueError(
> "The %s could not be %s because the data didn't validate." % (
> self.instance._meta.object_name,
> 'created' if self.instance._state.adding else 'changed',
> )
> )
> self.instance.save()
> self._save_m2m()
> return self.instance
>
> Instead of calling super().save(commit=False) to retrieve form.instance,
> you can just manipulate form.instance directly and then call
> super().save(). I don't see a need for the _instance getters/setters you
> proposed.
>
> There's also a deprecation checklist to help when writing a patch:
> https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code/submitting-patches/#deprecating-a-feature

IMO this is an example of the sort of low-value deprecation of a
widely-used API that gets people upset at us for wasting their time.

I agree that .save(commit=False) isn't the best or best-named API ever,
but it's been in Django since the beginning, it's _extremely_ widely
used, and (having taught Django classes to new users), I don't think it
ranks very high on the list of Django APIs that cause confusion in practice.

I wouldn't mind an update to the docs to recommend manipulating
`form.instance` and then calling `form.save()` (in place of `obj =
form.save(commit=False); obj.save(); form.save_m2m()`). I don't think
this requires any code changes; it's been true for a while (since
model-validation I think?) that the actual instance-updating happens
already in `_post_clean()`.

This could put us on the path toward an eventual deprecation of
`commit=False`, but I think we should be very slow to actually deprecate it.

Carl

[Tim Graham]

Yes, I agree that a documentation change should be sufficient (although I still didn't look at the formsets situation).

[Javier Candeira]

Ok, I'm heading for work now, will give it a spin this evening.

Thanks everyone for your comments!

J

[Tom Christie]

I'm not super convinced that form.instance is widely better than form.save(commit=False).

That's even more true if we're not sufficiently convinced by it that we'd be deprecating the existing style.

It would:

* Promote making views making in-place changes on the instance -> doesn't tend to be a good style.

* Expose the not-quite-an-instance without m2m attributes in what feels like a more casual way than the more verbose form.save(commit=False).

* Allow for (mostly) erroneous use of form.instance.save()

Don't feel that strongly about it, and wouldn't be able to make a proper judgement call in the absence of a pull request, but it's not terribly clear to me what we're gaining here. 

[Stan]

Unless I missed something here, I am strongly -1 on this PR:

1) Form.save(commit=False) is obvious: don't propagate to the database, stop at the object level (I'll manage that later, I don't want to hit the database 2 times, there are some dependencies I need to manage etc).
When I read that line, I know immediately what happens, what is triggered and what is not. There is no switch in my brain like: "Wait? What? save but don't save ?". Newbie or not, if you do get the concept of ORM, this implementation is rather elegant.

2) ModelForm.get_updated_model()rise questions when I read it. I am not sure of what it does, what it get from what. Too magical.

3) You are gonna break a lot of code out there!

4) I know you spend a lot of time here, but you don't solve any problem.

--
Stan