Code

Opened 4 years ago

Closed 11 months ago

#12337 closed Bug (fixed)

save_m2m() does not honor save_instance's exclude argument

Reported by: margieroginski Owned by: melinath
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

save_m2m() does not seem to honor the exclude argument that is passed in to save_instance. I have a form that includes a particular m2m field, however I am trying to avoid saving that field. To do this I am setting self._meta.exclude explicitly, just prior to calling the form's save method. After saving the task, I do the standard save_m2m() to save the m2m fields. I find that non-m2m fields are correctly excluded from the save if specified in this way via self._meta.exclude, but that m2m fields are not excluded from the save if specified this way. This seems inconsistent.

I suppose it could be rejected as a bug based on the fact that exclude is a model meta attribute, and if used exactly as specified in the doc, I wouldn't have the field in my form at all, so this wouldn't come up.

Here is some background on what I am doing to make a case for fixing this. I have an app sort of like the admin app. Say I have a model called 'foo'. User A views it and user B views it. User A then changes field 'status', and saves it. User B then changes field 'priority' and saves it. I need to avoid having user B's save change the 'status' field, since user B didn't modify that field. I am using the hidden initial functionality to detect that user B has not changed the status field, and then I am setting self._meta.exclude to all fields in the form that were not changed by the user. This all works fine the fields are non-m2m fields, but breaks down when an m2m field is placed in self._meta.exclude.

Currently save_m2m() is defined like this:

    def save_m2m():
        opts = instance._meta
        cleaned_data = form.cleaned_data
        for f in opts.many_to_many:
            if fields and f.name not in fields:
                continue
            if f.name in cleaned_data:
                f.save_form_data(instance, cleaned_data[f.name])

I find that if I define it like this instead, the m2m fields correctly are omitted from the save if they are in exclude. This seems like more consistent and useful behavior to me.

    def save_m2m():
        opts = instance._meta
        cleaned_data = form.cleaned_data
        for f in opts.many_to_many:
            if fields and f.name not in fields:
                continue
            if exclude and f.name in exclude:   <=== added this if clause
                continue
            if f.name in cleaned_data:
                f.save_form_data(instance, cleaned_data[f.name]) 

Attachments (2)

forms_save_m2m_exclude.diff (509 bytes) - added by brainrape@… 4 years ago.
fix
forms_save_m2m_exclude2.diff (928 bytes) - added by melinath 2 years ago.
I've adjusted the patch slightly to also actually *pass* the exclude value from the form's Meta to the save_instance function.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed
  • Triage Stage changed from Unreviewed to Accepted

Your suspicion is correct - you can't dynamically change the Meta class and expect those changes to be reflected in the model.

comment:2 Changed 4 years ago by margieroginski

Are you sure it even works to specify an m2m field in the exclude list? Since exclude is not even used in save_m2m, it seems it would not honor exclude even if specified statically.

In any case, I don't see why you wouldn't want to honor exclude on an m2m field when specified dynamically as I described in my example above. It seems like very inconsistent behavior to not honor exclude for an m2m field and it seems that a simple and backward compatible fix like the one I suggested to could be added to solve the problem.

I don't agree this should be closed, I hope you will reconsider it.

Thanks,
Margie

comment:3 Changed 4 years ago by ubernostrum

As Russ pointed out, the problem here is the technique you're trying to use: changing Meta partway through the life cycle of the form isn't, and won't be, supported. If you need to change the behavior of the form dynamically, you'll need to use a different approach (and for help on that try the django-users mailing list rather than the ticket tracker).

comment:4 Changed 4 years ago by brainrape@…

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from 1.1 to SVN

I'm not trying to alter a form dynamically, but this bug still bites me, when i'm overriding a ManyToManyField with an intermediary model and adding it to exclude.
I'm attaching a patch.

Changed 4 years ago by brainrape@…

fix

comment:5 Changed 4 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to Database layer (models, ORM)

comment:6 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:9 Changed 2 years ago by melinath

  • Component changed from Database layer (models, ORM) to Forms
  • Owner changed from nobody to melinath
  • Status changed from reopened to new

Just ran across this myself. My use case looks like this:

class MyOtherModel(Model):
    pass

class MyThroughModel(Model):
    other = models.ForeignKey(MyOtherModel)
    model = models.ForeignKey('MyModel')
    # Plus some metadata fields that are handled automatically.

class MyModel(Model):
    m2m = ManyToManyField(MyOtherModel, through=MyThroughModel)

class MyForm(ModelForm):
    m2m = ModelMultipleChoiceField(queryset=MyOtherModel.objects.all())

    class Meta:
        model = MyModel
        exclude = ('m2m',)

    def save(self, commit=True):
        # ... special behavior to handle the field.

With this setup, I am trying to exclude the default generated field and its behavior. In this example, the m2m field name is used in both the model definition and the form definition to represent a queryset of MyOtherModel instances.

However, I could theoretically exclude the field and put a CharField in its place. The current save_m2m code would then blindly try to assign the value of that CharField to the m2m field on the model.

This is because rather than explicitly checking whether the field is being excluded, the code assumes that if the field's name is in the cleaned_data, it was not excluded. This is not consistent with how fields and exclude are handled elsewhere in the codebase (for example in construct_instance).

As far as I can tell, this would have fit well in 64ea5af/r8756, which added exclude support to the then-save_instance function. My guess is that it was left out as an oversight rather than intentionally.

Changed 2 years ago by melinath

I've adjusted the patch slightly to also actually *pass* the exclude value from the form's Meta to the save_instance function.

comment:10 Changed 19 months ago by anonymous

I've set this up patch up as a branch on github off master. Apparently I never added tests... so I'm not issuing a pull request yet.

https://github.com/melinath/django/tree/ticket/12337

comment:11 Changed 19 months ago by melinath

  • Has patch set

I've issued a pull request for the changes.

comment:12 Changed 19 months ago by melinath

  • Status changed from new to assigned

Forgot to link to the pull request: https://github.com/django/django/pull/413

comment:13 Changed 11 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In e2518fdf46a687454066b8a75263c9019b1e965d:

Fixed #12337 - Honor ModelForm.Meta.exclude when saving ManyToManyFields.

Thanks margieroginski for the report.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.