Opened 17 years ago

Closed 14 years ago

Last modified 12 years ago

#5390 closed Uncategorized (fixed)

Add signals to ManyRelatedManager

Reported by: Ludovico Magnocavallo <ludo@…> Owned by: rvdrijst
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: manytomanyfield feature signals
Cc: sjulean@…, django@…, sean@…, patrick.lauber@…, chpapa@…, thiago.salves@…, oliver@…, rvdrijst, piranha@…, palewire@…, maxischmeii@…, django@…, lvscar@…, brent.hagany@…, matiassurdi@…, Mika Marttila, niran@…, eromirou, tomasz.elendt@…, django@…, francois@…, David Larlet, chris@…, django.tickets@…, muanis@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There's currently no way to run some custom code when the ManyRelatedManager adds, removes, or clears objects. Extending the ManyRelatedManager is very hard, since it's dynamically created each time, and the ManyRelatedObjectsDescriptor prevents any access to it from its container class. A simple solution is to add three signals, and hook them into the dispatching mechanism when adding, removing, or deleting a related object.

A typical use case for attaching custom code to a MayRelatedManager is blog categories: having a num_posts field in the Category models, and updating it each time a category is added/removed to/from a post, speeds up considerably a simple category list that excludes empty categories, and counts only published posts. The difference in resource usage is huge -- one query with two simple filters against one query for the list, plus one for each category (yes, you can cache the output, but think eg of a multi-blog app where posts are frequently updated, and a cache miss is very expensive), and the resulting code is much simpler.

A simple patch is attached to this ticket, I have tested it briefly against some of my models and it appears to work ok.

Attachments (12)

related_manager.diff (2.9 KB ) - added by Ludovico Magnocavallo <ludo@…> 17 years ago.
related_manager.2.diff (2.9 KB ) - added by ludo@… 17 years ago.
better patch, checked against django regression tests
related.diff (2.7 KB ) - added by Ludovico Magnocavallo <ludo@…> 17 years ago.
Cleaner variant of patch #2
models.py (3.0 KB ) - added by Ludovico Magnocavallo <ludo@…> 17 years ago.
Test models
tests.py (1.6 KB ) - added by Ludovico Magnocavallo <ludo@…> 17 years ago.
Test cases
m2msignals_doc.txt (1001 bytes ) - added by ludo@… 16 years ago.
patch for the db-api docs
related.field_name.diff (4.3 KB ) - added by murkt 16 years ago.
related.diff with added field_name parameter (signals and dispatcher are already imported)
m2m_signals.diff (4.4 KB ) - added by Alexander Solovyov 16 years ago.
m2m signals patch, updated against signals refactoring
complete-patch.diff (16.7 KB ) - added by rvdrijst 15 years ago.
A complete patch with code, docs and tests for an m2m_changed signal.
5390_against_11745.diff (15.8 KB ) - added by frans 14 years ago.
thks Ludovico, this time with the tests
5390-against-12033.diff (18.0 KB ) - added by frans 14 years ago.
updated diff with correct clear() management and more recent version of trunk
t5390-r12120.1.diff (13.1 KB ) - added by Russell Keith-Magee 14 years ago.
Revised implementation of m2m signals

Download all attachments as: .zip

Change History (77)

by Ludovico Magnocavallo <ludo@…>, 17 years ago

Attachment: related_manager.diff added

by ludo@…, 17 years ago

Attachment: related_manager.2.diff added

better patch, checked against django regression tests

comment:1 by Ludovico Magnocavallo <ludo@…>, 17 years ago

Sending source_col and target_col to the dispatcher is probably unnecessary.

by Ludovico Magnocavallo <ludo@…>, 17 years ago

Attachment: related.diff added

Cleaner variant of patch #2

by Ludovico Magnocavallo <ludo@…>, 17 years ago

Attachment: models.py added

Test models

by Ludovico Magnocavallo <ludo@…>, 17 years ago

Attachment: tests.py added

Test cases

comment:2 by Ludovico Magnocavallo <ludo@…>, 17 years ago

I attached a cleaner patch that does away with sending the field names, and some test cases with the necessary models.

comment:3 by Philippe Raoult, 17 years ago

Keywords: feature added

comment:4 by James Bennett, 17 years ago

#1069 and #2861 were duplicates.

comment:5 by James Bennett, 17 years ago

#3854 was a duplicate.

comment:6 by Silviu Julean <sjulean@…>, 16 years ago

Cc: sjulean@… added

I am using this patch to monitor user-group mapping in contrib.auth, and it works without any glitches.

comment:7 by Erik Karulf, 16 years ago

Cc: django@… added

I am also using the patch to monitor contrib.auth user-group relationships with no problems.

comment:8 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:9 by sean, 16 years ago

I'm using the patch quite a lot and up to now it works great. I really think this functionality should go into core, simply because it complements the other signals nicely and enables a developer to fully monitor all database changes (given that the orm is used).

comment:10 by murkt, 16 years ago

Cc: vsevolod.solovyov@… added
Needs documentation: set

Patch misses documentation. Notes about m2m signals should go into db-api.txt, section Many-to-many relationships.

by ludo@…, 16 years ago

Attachment: m2msignals_doc.txt added

patch for the db-api docs

comment:11 by Ludovico Magnocavallo <ludo@…>, 16 years ago

Needs documentation: unset

comment:12 by sean, 16 years ago

Cc: sean@… added

As I said before the patch works, but I just found a piece of missing functionality.
By using related_name it is possible to have 2 or more Many2many fields to the same model, but using the dispatcher there is no way to distinguish between the relations, or am I missing something here?
A possible solution would be to send the the name of the join_table along with the signal, but that seems rather hackish.

comment:13 by murkt, 16 years ago

It's possible to add one parameter to RelatedManager.__init__()field_name, and send it along with the signal. With the help of field_name we can easily get field instance itself: [f for f in MyModel._meta.many_to_many if f.name == field_name][0], or access RelatedManager: getattr(instance, field_name).

by murkt, 16 years ago

Attachment: related.field_name.diff added

related.diff with added field_name parameter (signals and dispatcher are already imported)

comment:14 by Patrick Lauber <patrick.lauber@…>, 16 years ago

Cc: patrick.lauber@… added

comment:15 by anonymous, 16 years ago

Cc: chpapa@… added

comment:16 by anonymous, 16 years ago

Cc: thiago.salves@… added

by Alexander Solovyov, 16 years ago

Attachment: m2m_signals.diff added

m2m signals patch, updated against signals refactoring

comment:17 by anonymous, 16 years ago

Cc: oliver@… added

+1 on this, it would be so useful right now for me; too bad it isn't in yet :(

comment:18 by rvdrijst, 15 years ago

Cc: rvdrijst added

#6778 was a duplicate

comment:19 by rvdrijst, 15 years ago

Keywords: signals added
milestone: post-1.0
Needs documentation: set
Patch needs improvement: set

I have started a discussion on django-developers concerning this ticket and the limitations I have encountered:
http://groups.google.com/group/django-developers/browse_thread/thread/afe6ad7994d868ba

Hopefully, this will push development and get this feature in the 1.1alhpa scheduled for january '09.

If you are interested, please read and comment there. If discussion is fruitful, I will claim the ticket and improve the patch, docs and tests.

Additionally, not related to the functionality but to the documentation: the db-api.txt is not the right place for signal documentation (there is no talk of pre/post_save signals there either). The correct place seems docs/ref/signals.txt. Also, not only the names of the signals, but also the extra arguments will need to be described.

comment:20 by Alexander Solovyov, 15 years ago

Cc: piranha@… added

by rvdrijst, 15 years ago

Attachment: complete-patch.diff added

A complete patch with code, docs and tests for an m2m_changed signal.

comment:21 by rvdrijst, 15 years ago

Needs documentation: unset
Owner: changed from nobody to rvdrijst
Patch needs improvement: unset
Status: newassigned

I have attached a new patch that combines the changes to the code, the documentation and the tests in one file.

The biggest change is that there is now only one signal, m2m_changed, that has an 'action' parameter (based on a comment of Malcolm Tredinnick.
The other parameters have changed somewhat, please discuss problems/suggestions on the django-developer thread mentioned above, where I have also posted a short explanation of the new parameters.

And thanks to Ludovico for the initial implementation.

comment:22 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:23 by wmdmark, 15 years ago

+1 Please include this fix in the upcoming release. I'm having to do some awful code hacks to get around this.

in reply to:  23 ; comment:24 by James Bennett, 15 years ago

Replying to wmdmark:

+1 Please include this fix in the upcoming release. I'm having to do some awful code hacks to get around this.

You do realize the feature freeze for 1.1 has passed already, right (in fact, 1.1 is behind schedule because we delayed a bit for a late-running feature)? No new feature checkins will happen until after the release, at which point proposals for inclusion in 1.2 will open.

If you're really interested in getting a feature into Django, it's in your best interest to get involved on the developers' list and pay attention to the release schedule.

comment:25 by palewire, 15 years ago

Cc: palewire@… added

comment:26 by schmeii, 15 years ago

Cc: maxischmeii@… added

comment:27 by Ulrich Petri, 15 years ago

Cc: django@… added

comment:28 by lvscar, 15 years ago

Cc: lvscar@… added

comment:29 by Brent Hagany, 15 years ago

Cc: brent.hagany@… added

comment:30 by Matias Surdi, 15 years ago

Cc: matiassurdi@… added

in reply to:  24 comment:31 by schmilblick, 15 years ago

+1
Has anyone tried the patch in production? Is it applied in the trunk?

in reply to:  24 comment:32 by Mika Marttila, 15 years ago

Cc: Mika Marttila added

Replying to ubernostrum:

If you're really interested in getting a feature into Django, it's in your best interest to get involved on the developers' list and pay attention to the release schedule.

+1 it's not a feature, it's a bug fix.

comment:33 by Niran Babalola, 15 years ago

Cc: niran@… added

comment:34 by eromirou, 15 years ago

Cc: eromirou added

comment:35 by Tomasz Elendt, 15 years ago

Cc: tomasz.elendt@… added

comment:36 by twold, 15 years ago

milestone: 1.2

Hello. What's the status of this bug? rvdrijst's patch seems to solve all the problems (at least mine, that is), it is neatly written, etc. Is there any reason for not including it into the trunk, thus finally solving this bug? I would really like to see it fixed in 1.2, because it feels really awkward patching django core on production server...

comment:37 by jedie, 15 years ago

Cc: django@… added

comment:38 by schmilblick, 14 years ago

The patch solved my problems as well, but like twold wrote; it's really awkward patching stuff in prod.
Status? 1.2?

comment:39 by frans, 14 years ago

Cc: francois@… added

comment:40 by frans, 14 years ago

I found that to have the expected behaviour when using the admin, I had to hack ReverseManyRelatedObjectsDescriptor and ManyRelatedObjectsDescriptor in django/db/fields/related.py to replace, in __set__,

        manager = self.__get__(instance)
        manager.clear()
        manager.add(*value)

by

        manager = self.__get__(instance)
        previous=set(manager.all())
        new=set(value)
        added=new-previous
        removed=previous-new
        manager.add(*added)
        manager.remove(*removed)

comment:41 by schmilblick, 14 years ago

How does the merger of Alex Gaynor's GSoC project affect this? Patch invalid, no longer needed or neither?

comment:42 by Mika Marttila, 14 years ago

What is Alex Gaynor's GSoC project?

(It's Google Summer of Code project that aimed to bring multiple database support to Django's ORM, refactoring ManyToManyField along the way)

comment:43 by frans, 14 years ago

Patch needs improvement: set

From a quick roam through the patch and the code @rev 11736 in trunk, it wouldn't take a lot to adapt.
Will do if I find the time.

comment:44 by frans, 14 years ago

the commenter Mark says in the link posted by aom Seems like it would [solve #5390] since you'd basically be catching signals from the intermediary models. . Will test and report.

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

@frans - No - the m2m refactor doesn't solve #5390. The signals are at a different granularity. The proposal for this ticket is to send a 'change' signal whenever the m2m field is changed; if you allow signals on the m2m intermediate, you get a signal for each and every object you assign. So -

author.books = [1,2,3,4]

Would generate 4 signals if you listened to the intermediate model, but 1 based on the proposal from this ticket.

It is for this reason that save and delete signals on auto-generated m2m models are explicitly disabled.

comment:46 by David Larlet, 14 years ago

Cc: David Larlet added

I know that part of the code, I can help if you do not find the time frans.

comment:47 by frans, 14 years ago

added the patch. Seems to be passing tests (with only a quick try) ok.
I didn't add the change mentioned in my comment above with replacing in __set__ clear and add by add and remove.
It is a question of religion, I guess. Do we consider set should clear/add what you set, or add/remove the corresponding entries? I guess clear/add makes slightly less DB access.
It makes sense for me to do add and remove instead of clear and add (because clear causes in one of my apps to delete some objects based on clear that I really don't want to see deleted each time the admin sets the m2m field) but if you guys have a different opinion, I'm happy with that. What shall we do?

comment:48 by David Larlet, 14 years ago

It looks like tests are missing from your patch (don't know if it's intentional though).

No opinion about clear/add/remove, I'm not sure I get all elements to understand.

by frans, 14 years ago

Attachment: 5390_against_11745.diff added

thks Ludovico, this time with the tests

comment:49 by anonymous, 14 years ago

Cc: chris@… added

comment:50 by murkt, 14 years ago

Cc: vsevolod.solovyov@… removed

comment:51 by Thomas Güttler, 14 years ago

Cc: hv@… added

comment:52 by Xia Kai(夏恺), 14 years ago

This is really a great patch! However, when this signal is used in admin, as the setattr method is called in function save_form_data, Django would removed everything in a ManyToManyField, and then add every object back. Thus the remove method provided in this would not be called in admin view. This is very confusing and misleading IMHO. I think someone would need to make some change to some of the functions in django/db/models/fields/related.py to make sure the remove method is called properly.

comment:53 by frans, 14 years ago

@xiaket : see my comment on 11/08/09 12:35:51, it precisely covers that.
If you add the change I mention there, it will behave as you describe/expect.

in reply to:  53 comment:54 by Xia Kai(夏恺), 14 years ago

Replying to frans:

I'm so sorry, after my stupid post I found your solution, which, I boldly suggest, should have been included in the last patch.

comment:55 by frans, 14 years ago

following xiaket's private email, solution in comment above should be amended to deal better with clear()

However, I think there is a minor problem about you approach: the clear signal would not be correctly issued. So I modified the code:

        manager = self.__get__(instance)
        previous=set(manager.all())
        new=set(value)
        if not new:
            manager.clear()
        else:
            added = new - previous
            removed = previous - new
            if removed:
                manager.remove(*removed)
            if added:
                manager.add(*added)

will update the patch as soon as I find time to restore my repository to a clean state...

comment:56 by anonymous, 14 years ago

Cc: django.tickets@… added

by frans, 14 years ago

Attachment: 5390-against-12033.diff added

updated diff with correct clear() management and more recent version of trunk

comment:57 by frans, 14 years ago

Note : diff above (5390-against-12033.diff) is not tested yet..

by Russell Keith-Magee, 14 years ago

Attachment: t5390-r12120.1.diff added

Revised implementation of m2m signals

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

Resolution: fixed
Status: assignedclosed

(In [12223]) Fixed #5390 -- Added signals for m2m operations. Thanks to the many people (including, most recently, rvdrijst and frans) that have contributed to this patch.

comment:59 by muanis, 14 years ago

Cc: muanis@… added

in reply to:  59 comment:60 by frans, 14 years ago

I certainly don't want to be a killjoy but it looks like r12223 still uses the following for set()

manager.clear()  
manager.add(*value)  

instead of

        previous=set(manager.all())  
        new=set(value)  
        if not new:  
          manager.clear()  
        else:  
          added=new-previous  
          removed=previous-new  
        if added :  
          manager.add(*added)  
        if removed :   
          manager.remove(*removed)  

which may lead, unless I'm missing something, to the problems listed in my initial comment and Xiaket's

If this is right, should we reopen the ticket?

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

No, you shouldn't reopen the ticket. This "problem" is an orthogonal change/optimization reported as #6707. The extent to which this is a "problem" was discussed in #13022.

comment:62 by frans, 14 years ago

good point. Sorry for the confusion, I should have done my homework better.

comment:63 by Félix Delval, 13 years ago

Patch needs improvement: unset

I had problems finding if this patch made it to the 1.2 release. It has, here is the link to the documentation :

http://docs.djangoproject.com/en/dev/ref/signals/#m2m-changed

Thank you all.

comment:64 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

comment:65 by Thomas Güttler, 12 years ago

Cc: hv@… removed
Easy pickings: unset
Severity: Normal
Type: Uncategorized
UI/UX: unset
Note: See TracTickets for help on using tickets.
Back to Top