Opened 14 years ago

Closed 11 years ago

#12337 closed Bug (fixed)

save_m2m() does not honor save_instance's exclude argument

Reported by: Margie Roginski Owned by: Stephen Burrows
Component: Forms Version: dev
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@… 14 years ago.
fix
forms_save_m2m_exclude2.diff (928 bytes ) - added by Stephen Burrows 12 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 by Russell Keith-Magee, 14 years ago

Resolution: invalid
Status: newclosed
Triage Stage: UnreviewedAccepted

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

comment:2 by Margie Roginski, 14 years ago

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 by James Bennett, 14 years ago

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 by brainrape@…, 14 years ago

Resolution: invalid
Status: closedreopened
Version: 1.1SVN

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.

by brainrape@…, 14 years ago

Attachment: forms_save_m2m_exclude.diff added

fix

comment:5 by Thejaswi Puthraya, 14 years ago

Component: UncategorizedDatabase layer (models, ORM)

comment:6 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:7 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 by Stephen Burrows, 12 years ago

Component: Database layer (models, ORM)Forms
Owner: changed from nobody to Stephen Burrows
Status: reopenednew

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.

by Stephen Burrows, 12 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.

comment:10 by anonymous, 11 years ago

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 by Stephen Burrows, 11 years ago

Has patch: set

I've issued a pull request for the changes.

comment:12 by Stephen Burrows, 11 years ago

Status: newassigned

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

comment:13 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In e2518fdf46a687454066b8a75263c9019b1e965d:

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

Thanks margieroginski for the report.

Note: See TracTickets for help on using tickets.
Back to Top