Code

Opened 4 years ago

Closed 4 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: master
Severity: Keywords: signals, m2m_changed
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description (last modified by ramiro)

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 4 years ago.
ticket13022.diff (1.4 KB) - added by halldor89 4 years ago.
An attempt at fixing this issue.

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by benc

comment:1 Changed 4 years ago by ramiro

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

reformatted description.

comment:2 Changed 4 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

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.

Changed 4 years ago by halldor89

An attempt at fixing this issue.

comment:3 Changed 4 years ago by halldor89

  • Has patch set
  • Needs tests set
  • Patch needs improvement set
  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 Changed 4 years ago by halldor89

  • Component changed from django.contrib.admin to Database layer (models, ORM)

comment:5 Changed 4 years ago by russellm

  • milestone 1.2 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

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.

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.