Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #6707 -- Added RelatedManager.set() and made descriptors' __set__ use it. #2500

Merged
merged 2 commits into from Feb 5, 2015

Conversation

Projects
None yet
8 participants
Member

loic commented Mar 31, 2014

No description provided.

Owner

timgraham commented May 26, 2014

Is there a ticket? Should we leave this PR open?

Member

loic commented May 26, 2014

Ticket is https://code.djangoproject.com/ticket/6707.

It's missing docs and I'd like to add a few more tests, but mostly it's waiting for feedback.

Member

loic commented Sep 26, 2014

buildbot, test this please.

@timgraham timgraham changed the title from Added RelatedManager.set() and made descriptors' __set__ use it. to Fixed #6707 -- Added RelatedManager.set() and made descriptors' __set__ use it. Sep 29, 2014

Member

loic commented Sep 29, 2014

buildbot, test this please.

@carljm carljm and 2 others commented on an outdated diff Sep 29, 2014

django/contrib/contenttypes/fields.py
@@ -530,6 +522,27 @@ def _clear(self, queryset, bulk):
obj.delete()
_clear.alters_data = True
+ def set(self, *objs, **kwargs):
+ clear = kwargs.pop('clear', False)
+
+ db = router.db_for_write(self.model, instance=self.instance)
+ with transaction.atomic(using=db, savepoint=False):
+ if clear:
+ self.clear()
+ self.add(*objs)
+ else:
+ old_objs = set(self.using(db).all())
@carljm

carljm Sep 29, 2014

Owner

I think this algorithm could be implemented using .values_list('pk', flat=True) in place of .all() when querying the existing objects, and thus avoid needless model-instantiation overhead.

@loic

loic Sep 29, 2014

Member

Good point.

@loic

loic Sep 29, 2014

Member

Actually RelatedManager.remove() and GenericRelatedObjectManager don't handle pks only object instances.

@carljm

carljm Sep 29, 2014

Owner

Good point. Looking at their implementation, it seems that might not be hard to change, but likely that should be explored separately.

@MarkusH

MarkusH Feb 4, 2015

Member

Care to open a ticket and add a comment so that it won't be forgotten?

@collinanderson collinanderson commented on the diff Jan 26, 2015

docs/ref/models/relations.txt
@@ -135,12 +135,28 @@ Related objects reference
:class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also
accepts the ``bulk`` keyword argument.
+ .. method:: set(obj1, [obj2, ...])
+
+ .. versionadded:: 1.8
+
+ Replace the set of related objects::
+
+ >>> new_list = [obj1, obj2, obj3]
+ >>> e.related_set.set(new_list)
@collinanderson

collinanderson Jan 26, 2015

Contributor

This is the API I would expect, but why is it implemented as .set(*object_list) rather than .set(object_list)?

@collinanderson

collinanderson Jan 26, 2015

Contributor

Ohh, probably to be consistent with .add(*object_list)? Still, it seems to me set(object_list) would be the most useful.

@loic

loic Jan 27, 2015

Member

For consistency yes, I'd be quite reluctant breaking it.

@loic

loic Feb 5, 2015

Member

For posterity: I've changed my mind on this and consensus emerged in favor of set(objs) instead of set(*objs).

Thanks Collin for raising the issue!

@akaariai akaariai and 1 other commented on an outdated diff Feb 4, 2015

docs/ref/contrib/admin/index.txt
@@ -2206,6 +2206,10 @@ model to represent the many-to-many relationship, you must tell Django's admin
to *not* display this widget - otherwise you will end up with two widgets on
your admin page for managing the relation.
+Note that when using this technique the :data:`m2m_changed` signals aren't
+triggered. This is because as far as the admin is concerned, ``through`` is
+just a model with two foreign key fields rather than a many-to-many relation.
@akaariai

akaariai Feb 4, 2015

Member

This documents existing behavior, right?

@loic

loic Feb 4, 2015

Member

Yes, I've put it in a different commit.

Member

akaariai commented Feb 4, 2015

Looks good to me.

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/ref/models/relations.txt
@@ -135,12 +135,31 @@ Related objects reference
:class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also
accepts the ``bulk`` keyword argument.
+ .. method:: set(obj1, [obj2, ...])
+
+ .. versionadded:: 1.9

@timgraham timgraham and 1 other commented on an outdated diff Feb 4, 2015

docs/ref/models/relations.txt
@@ -135,12 +135,31 @@ Related objects reference
:class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also
accepts the ``bulk`` keyword argument.
+ .. method:: set(obj1, [obj2, ...])
@timgraham

timgraham Feb 4, 2015

Owner

should clear=False be listed here?

@loic

loic Feb 4, 2015

Member

I wondered about it, we didn't do it for bulk=True/False so I didn't do it here. Should I add it?

@loic

loic Feb 4, 2015

Member

Works for me, I'll add another commit which fixes it for bulk as well.

@loic

loic Feb 4, 2015

Member

Ok now I remember why I didn't add it for bulk, it's not available on all relational managers.

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/ref/models/relations.txt
+ .. method:: set(obj1, [obj2, ...])
+
+ .. versionadded:: 1.9
+
+ Replace the set of related objects::
+
+ >>> new_list = [obj1, obj2, obj3]
+ >>> e.related_set.set(*new_list)
+
+ This method accepts a ``clear`` argument to control how to perform the
+ operation. If ``False`` (the default), the elements missing from the
+ new set are removed using ``remove()`` and only the new ones are added.
+ If ``clear=True``, the ``clear()`` method is called instead and the
+ whole set is added at once.
+
+ Note that ``set()`` being a compound operation it is subjected to race
@timgraham

timgraham Feb 4, 2015

Owner

Note that since set is a compound operation, it is subject to...

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/ref/models/relations.txt
@@ -158,6 +177,11 @@ new iterable of objects to it::
>>> e.related_set = new_list
If the foreign key relationship has ``null=True``, then the related manager
-will first call ``clear()`` to disassociate any existing objects in the related
-set before adding the contents of ``new_list``. Otherwise the objects in
-``new_list`` will be added to the existing related object set.
+will first disassociate any existing objects in the related set before adding
+the contents of ``new_list``. Otherwise the objects in ``new_list`` will be
+added to the existing related object set.
+
+.. versionchanged:1.9
+
+In earlier versions direct assignment used to perform ``clear()`` followed by
@timgraham

timgraham Feb 4, 2015

Owner

indent contents
In earlier versions,

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/ref/models/relations.txt
@@ -158,6 +177,11 @@ new iterable of objects to it::
>>> e.related_set = new_list
If the foreign key relationship has ``null=True``, then the related manager
-will first call ``clear()`` to disassociate any existing objects in the related
-set before adding the contents of ``new_list``. Otherwise the objects in
-``new_list`` will be added to the existing related object set.
+will first disassociate any existing objects in the related set before adding
+the contents of ``new_list``. Otherwise the objects in ``new_list`` will be
+added to the existing related object set.
+
+.. versionchanged:1.9
+
+In earlier versions direct assignment used to perform ``clear()`` followed by
+``add()``, it now performs a ``set()`` with the keyword argument ``clear=False``.
@timgraham

timgraham Feb 4, 2015

Owner

. It now
or: "; it now"

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/releases/1.9.txt
@@ -127,7 +127,9 @@ Management Commands
Models
^^^^^^
-* ...
+* Added the :meth:`django.db.models.fields.related.RelatedManager.set()` method
@timgraham

timgraham Feb 4, 2015

Owner

RelatedManager.set() <django.db.models.fields.related.RelatedManager.set> rather then rendering the entire path?

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/releases/1.9.txt
@@ -127,7 +127,9 @@ Management Commands
Models
^^^^^^
-* ...
+* Added the :meth:`django.db.models.fields.related.RelatedManager.set()` method
+ to the related managers created by ``ForeignKey`` and ``GenericForeignKey``
@timgraham

timgraham Feb 4, 2015

Owner

FK, GFK, and M2M (add commas)

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/releases/1.9.txt
@@ -165,7 +167,24 @@ Backwards incompatible changes in 1.9
deprecation timeline for a given feature, its removal may appear as a
backwards incompatible change.
-...
+Related set direct assignment
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:ref:`direct assignment <direct-assignment>`) used to perform a ``clear()``
@timgraham

timgraham Feb 4, 2015

Owner

Direct assignment

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/releases/1.9.txt
+Related set direct assignment
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:ref:`direct assignment <direct-assignment>`) used to perform a ``clear()``
+followed by a call to ``add()``. This caused needlessly large data changes
+and prevented using the :data:`~django.db.models.signals.m2m_changed` signal
+to track individual changes in many-to-many relations.
+
+Direct assignment now relies on the the new
+:meth:`django.db.models.fields.related.RelatedManager.set()` method on
+related managers which by default only processes changes between the
+existing related set and the one that's newly assigned. The previous behavior
+can be restored by replacing direct assignment by a call to ``set()`` with
+the keyword argument ``clear=True``.
+
+``ModelForm`` and therefore ``ModelAdmin`` internally rely on direct assignment
@timgraham

timgraham Feb 4, 2015

Owner

, and therefore ModelAdmin,

@timgraham timgraham commented on an outdated diff Feb 4, 2015

docs/releases/1.9.txt
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:ref:`direct assignment <direct-assignment>`) used to perform a ``clear()``
+followed by a call to ``add()``. This caused needlessly large data changes
+and prevented using the :data:`~django.db.models.signals.m2m_changed` signal
+to track individual changes in many-to-many relations.
+
+Direct assignment now relies on the the new
+:meth:`django.db.models.fields.related.RelatedManager.set()` method on
+related managers which by default only processes changes between the
+existing related set and the one that's newly assigned. The previous behavior
+can be restored by replacing direct assignment by a call to ``set()`` with
+the keyword argument ``clear=True``.
+
+``ModelForm`` and therefore ``ModelAdmin`` internally rely on direct assignment
+for many-to-many relations and as a consequence will now display the new
@timgraham

timgraham Feb 4, 2015

Owner

will now display -> now use

@timgraham timgraham and 1 other commented on an outdated diff Feb 4, 2015

tests/many_to_many/tests.py
@@ -332,6 +332,45 @@ def test_remove(self):
])
self.assertQuerysetEqual(self.a3.publications.all(), [])
+ def test_set(self):
+ self.p2.article_set.set(self.a4, self.a3)
+ self.assertQuerysetEqual(self.p2.article_set.all(),
+ [
@timgraham

timgraham Feb 4, 2015

Owner

I'd prefer if you kept the same indent style as the above tests (but I guess you've copied the style of the surrounding code)

@loic

loic Feb 4, 2015

Member

I don't like these either since they aren't pep8 compliant (E128) but the whole file (and the similar test packages) use them, so I'd like to keep them consistent.

@timgraham timgraham and 1 other commented on an outdated diff Feb 4, 2015

tests/many_to_one_null/tests.py
@@ -74,6 +74,22 @@ def test_remove_from_wrong_set(self):
self.assertRaises(Reporter.DoesNotExist, self.r.article_set.remove, self.a4)
self.assertQuerysetEqual(self.r2.article_set.all(), ['<Article: Fourth>'])
+ def test_set(self):
+ # Use manager.set() to allocate ForeignKey. Null is legal, so existing
+ # members of set that are not in the assignment set are set null
@timgraham

timgraham Feb 4, 2015

Owner

of the set
set to null
adding period

@loic

loic Feb 4, 2015

Member

This was copied from another test, fixed in both locations.

Member

loic commented Feb 4, 2015

Thinking about it a bit more, maybe set() should accept an iterable rather than *objs, @collinanderson raised the issue in #2500 (comment).

I used *objs for consistency with add() and remove() but those two make sense for a single object, less so for set(). What do you think @akaariai, @carljm, @timgraham?

Owner

timgraham commented Feb 4, 2015

Did you see https://code.djangoproject.com/ticket/13240 which proposes that for related methods?

Member

loic commented Feb 4, 2015

@timgraham wasn't aware of it. Can't say I like the idea of flattening all iterables like done in that patch, although I would accept special-casing when there is only 1 object and that object is an iterable (we should keep in mind that it would probably break models with __iter__).

Regarding set(objs) vs set(*objs), I'm 40/60 in favor of the latter :

  • When I think of a set that I just want to assign, then I find set(objs) nicer.
  • When I think of it as a shorthand for clear() + add(), then I really want consistency with add() (so set(*objs)).
Owner

carljm commented Feb 5, 2015

I also think .set(objs) is better than .set(*objs). I think the strongest parallel for .set() isn't .add(), it's "what you retrieve", which is an iterable.

(It's a tangent, but I also don't like methods that accept either and magically flatten iterables.)

Member

loic commented Feb 5, 2015

I think the strongest parallel for .set() isn't .add(), it's "what you retrieve", which is an iterable.

That's very true.

Member

charettes commented Feb 5, 2015

I'd also argue that you're more likely to use add() for a single object than you are for set(). Most of the time you'll pass a collection of items when using set() since it's the many part of the relationship.

I think say set() shares more with bulk_create() than add() in this regard.

loic added some commits Jan 29, 2015

@loic loic Fixed #6707 -- Added RelatedManager.set() and made descriptors' __set…
…__ use it.

Thanks Anssi Kääriäinen, Carl Meyer, Collin Anderson, and Tim Graham for the reviews.
71ada3a
@loic loic Documented that m2m_changed isn't triggered when M2M is displayed as …
…admin inline.

Refs #6707.
9d104a2

@loic loic merged commit 9d104a2 into django:master Feb 5, 2015

1 check was pending

docs Merged build started.
Details

Would it be possible to have the above fix applied to the 1.8 branch ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment