Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#28543 closed Bug (fixed)

ModelForm.initial is affected while its bound instance's m2m field be set with new data

Reported by: Wonder Owned by: nobody
Component: Database layer (models, ORM) Version: 1.11
Severity: Normal Keywords: form
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I found Django's admin log always doesn't include m2m-fields' change. So I looked into django source and found here: https://github.com/django/django/blob/1.11.4/django/contrib/admin/options.py#L1410.

After this line self.save_related(request, form, formsets, not add), form.initial is also changed to the saved version, then affect form.changed_data.

Then I wrote some test code to reproduce the issue:

import datetime as dt
from django.contrib.auth.forms import UserChangeForm
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Group

User = get_user_model()

g1=Group.objects.get_or_create(name='g1')[0]
g2=Group.objects.get_or_create(name='g2')[0]
user = User.objects.get_or_create(username='wonder')[0]
user.groups.set([g1])

form = UserChangeForm({
    'username': 'wonder',
    'date_joined': dt.datetime.now(),
}, instance=user)
assert form.is_valid()

form.initial['groups']  # Out: <QuerySet [<Group: g1>]>
user.groups.set([g2])
form.initial['groups']  # Out: <QuerySet [<Group: g2>]>

Note the two comments in the code.

Change History (6)

comment:1 Changed 3 years ago by Tim Graham

The issue of ManyToManyFields not appearing in the admin's change history is fixed in master by #27998. Is there another issue present? I'm unsure since your steps to reproduce don't involve the admin.

comment:2 Changed 3 years ago by Wonder

The issue of ManyToManyFields not appearing in the admin's change history is fixed in master by #27998. Is there another issue present? I'm unsure since your steps to reproduce don't involve the admin.

Yes, It may affect not only m2m-fields change in admin.

Last edited 3 years ago by Wonder (previous) (diff)

comment:3 Changed 3 years ago by Tim Graham

Component: UncategorizedDatabase layer (models, ORM)
Has patch: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

The lazy behavior of ManyToManyField.value_from_object() seems inconsistent with other fields. Fixing this would allow reverting the admin-specific fix for #27998.

PR

comment:4 Changed 3 years ago by Tim Graham

I found that #27998 is a regression in Django 1.10 due to ded502024191053475bac811d365ac29dca1db61. The proposed patch here fixes that issue in a better way (at the form level, rather than only in the admin) but, with respect to the decision of how to fix that on stable/1.11.x, changing the return type of ManyToManyField.value_from_object() may be too risky for a stable release. Perhaps a less invasive fix for 1.11.x would restore the queryset to list conversion for ManyToManyFields in model_to_dict().

comment:5 Changed 3 years ago by GitHub <noreply@…>

Resolution: fixed
Status: newclosed

In e5bd585:

Fixed #28543 -- Prevented ManyToManyField.value_from_object() from being lazy.

Previously, it was a QuerySet which could reevaluate to a new value if the
model's data changes. This is inconsistent with other Field.value_from_object()
methods.

This allows reverting the fix in the admin for refs #27998.

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

In 20c0339:

[1.11.x] Fixed #27998, #28543 -- Restored logging of ManyToManyField changes in admin's object history.

And prevented ManyToManyField initial data in model forms from being affected
by subsequent model changes.

Regression in 56a55566a791a11420fe96f745b7489e756fc931.

Partial backport of e5bd585c6eb1e13e2f8aac030b33c077b0b70c05 and
15b465c584f49a1d43b6c18796f83521ee4ffc22 from master

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