Opened 14 years ago

Closed 14 years ago

#13022 closed (wontfix)

when saving a model with m2m field in the admin, 'clear' and 'add' m2m_changed signals are fired even when there is no change.

Reported by: benc Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: signals, m2m_changed
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

I've built a simple test project with one app and two models, Model1 and Model2.
Model2 has a ManyToManyField to Model1.
I've created one Model1 instance and one Model2 instance with a relation to the Model1 instance.

When saving the Model2 instance in the admin, even without a change, m2m_changed fires twice.
I think it shouldn't fire at all when the admin form is saved without a change:

sender  <class 'testproject.testapp.models.Model2_models1'>
instance  Model2 object
action  clear
model  <class 'testproject.testapp.models.Model1'>
sender  <class 'testproject.testapp.models.Model2_models1'>
instance  Model2 object
action  add
model  <class 'testproject.testapp.models.Model1'>

Attachments (2)

testproject.tar.gz (7.5 KB ) - added by benc 14 years ago.
ticket13022.diff (1.4 KB ) - added by halldor89 14 years ago.
An attempt at fixing this issue.

Download all attachments as: .zip

Change History (7)

by benc, 14 years ago

Attachment: testproject.tar.gz added

comment:1 by Ramiro Morales, 14 years ago

Description: modified (diff)

reformatted description.

comment:2 by Russell Keith-Magee, 14 years ago

Resolution: wontfix
Status: newclosed

I'm going to call this working as intended.

The admin code is doing an assignment of the form obj.m2m = [new values], which internally is 2 signals - clear and add signal. This is completely accurate behaviour, and there isn't much we can do to fix this. In order to avoid sending the signals, we need to query the database to do a diff between submitted values and new values, which is potentially an expensive operation.

by halldor89, 14 years ago

Attachment: ticket13022.diff added

An attempt at fixing this issue.

comment:3 by halldor89, 14 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set
Resolution: wontfix
Status: closedreopened

Even though the current behavior might be intended it does render the m2m_changed signal unusable. It makes it completely impossible to use django.forms to save data that should trigger m2m_changed to database (if it's not only performing a basic task like updating other database rows).
I put together a small patch that modifies the _ _set_ _ method of (Reverse)ManyRelatedObjectsDescriptor so that it performs that one extra query to get current ids and then uses sets to do a diff between submitted and new values. Since the m2m.clear() can also be kinda expensive and m2m.add(*values) does one insert per item I think that one query and some basic calculation will pay off.

It would, however, be awesome if someone more skilled than me could improve the patch (since I'm not sure how efficient sets are etc., it was just the easiest fix I could think of).

comment:4 by halldor89, 14 years ago

Component: django.contrib.adminDatabase layer (models, ORM)

comment:5 by Russell Keith-Magee, 14 years ago

milestone: 1.2
Resolution: wontfix
Status: reopenedclosed

Hyperbole will get you nowhere. The current behavior doesn't render anything *unusable*. You are correctly notified of the signals - you just get more signal responses that you might want. This may be inconvenient, and it may result in more database activity that is desirable, but it isn't *unusable*.

That said, the optimization you describe is worth pursuing - which is why it's open as ticket #6707. The patch you present is essentially the same as the one on that ticket, but that approach is a little naive; at the very least, it neglects the multiple extra queries that occur during _add_items() that already do set subtraction to avoid duplicate writes to the m2m table.

Closing as wontfix again; the original report was specifically about an admin problem, which is wontfix; optimizing m2m is ticket #6707, which is current DDN.

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