Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #9475 -- Allowed add(), create(), etc for m2m with through #9609

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 26 additions & 48 deletions django/db/models/fields/related_descriptors.py
Original file line number Diff line number Diff line change
Expand Up @@ -906,32 +906,20 @@ def get_prefetch_queryset(self, instances, queryset=None):
False,
)

def add(self, *objs):
if not rel.through._meta.auto_created:
opts = self.through._meta
raise AttributeError(
"Cannot use add() on a ManyToManyField which specifies an "
"intermediary model. Use %s.%s's Manager instead." %
(opts.app_label, opts.object_name)
)
def add(self, *objs, through_defaults=None):
self._remove_prefetched_objects()
db = router.db_for_write(self.through, instance=self.instance)
with transaction.atomic(using=db, savepoint=False):
self._add_items(self.source_field_name, self.target_field_name, *objs)
self._add_items(
self.source_field_name, self.target_field_name, *objs, intermediate_values=through_defaults)

# If this is a symmetrical m2m relation to self, add the mirror entry in the m2m table
if self.symmetrical:
self._add_items(self.target_field_name, self.source_field_name, *objs)
self._add_items(
self.target_field_name, self.source_field_name, *objs, intermediate_values=through_defaults)
add.alters_data = True

def remove(self, *objs):
if not rel.through._meta.auto_created:
opts = self.through._meta
raise AttributeError(
"Cannot use remove() on a ManyToManyField which specifies "
"an intermediary model. Use %s.%s's Manager instead." %
(opts.app_label, opts.object_name)
)
self._remove_prefetched_objects()
self._remove_items(self.source_field_name, self.target_field_name, *objs)
remove.alters_data = True
Expand All @@ -955,15 +943,7 @@ def clear(self):
)
clear.alters_data = True

def set(self, objs, *, clear=False):
if not rel.through._meta.auto_created:
opts = self.through._meta
raise AttributeError(
"Cannot set values on a ManyToManyField which specifies an "
"intermediary model. Use %s.%s's Manager instead." %
(opts.app_label, opts.object_name)
)

def set(self, objs, *, clear=False, through_defaults=None):
# Force evaluation of `objs` in case it's a queryset whose value
# could be affected by `manager.clear()`. Refs #19816.
objs = tuple(objs)
Expand All @@ -972,7 +952,7 @@ def set(self, objs, *, clear=False):
with transaction.atomic(using=db, savepoint=False):
if clear:
self.clear()
self.add(*objs)
self.add(*objs, through_defaults=through_defaults)
else:
old_ids = set(self.using(db).values_list(self.target_field.target_field.attname, flat=True))

Expand All @@ -988,49 +968,47 @@ def set(self, objs, *, clear=False):
new_objs.append(obj)

self.remove(*old_ids)
self.add(*new_objs)
self.add(*new_objs, through_defaults=through_defaults)
set.alters_data = True

def create(self, **kwargs):
# This check needs to be done here, since we can't later remove this
# from the method lookup table, as we do with add and remove.
if not self.through._meta.auto_created:
opts = self.through._meta
raise AttributeError(
"Cannot use create() on a ManyToManyField which specifies "
"an intermediary model. Use %s.%s's Manager instead." %
(opts.app_label, opts.object_name)
)
def create(self, through_defaults=None, **kwargs):
db = router.db_for_write(self.instance.__class__, instance=self.instance)
new_obj = super(ManyRelatedManager, self.db_manager(db)).create(**kwargs)
self.add(new_obj)
self.add(new_obj, through_defaults=through_defaults)
return new_obj
create.alters_data = True

def get_or_create(self, **kwargs):
def get_or_create(self, through_defaults=None, **kwargs):
db = router.db_for_write(self.instance.__class__, instance=self.instance)
obj, created = super(ManyRelatedManager, self.db_manager(db)).get_or_create(**kwargs)
# We only need to add() if created because if we got an object back
# from get() then the relationship already exists.
if created:
self.add(obj)
self.add(obj, through_defaults=through_defaults)
return obj, created
get_or_create.alters_data = True

def update_or_create(self, **kwargs):
def update_or_create(self, through_defaults=None, **kwargs):
db = router.db_for_write(self.instance.__class__, instance=self.instance)
obj, created = super(ManyRelatedManager, self.db_manager(db)).update_or_create(**kwargs)
# We only need to add() if created because if we got an object back
# from get() then the relationship already exists.
if created:
self.add(obj)
self.add(obj, through_defaults=through_defaults)
return obj, created
update_or_create.alters_data = True

def _add_items(self, source_field_name, target_field_name, *objs):
def _add_items(self, source_field_name, target_field_name, *objs, intermediate_values=None):
# source_field_name: the PK fieldname in join table for the source object
# target_field_name: the PK fieldname in join table for the target object
# *objs - objects to add. Either object instances, or primary keys of object instances.
# *objs: objects to add. Either object instances, or primary keys of object instances.
# through_defaults: Extra values for fields on the through model.
# Either a dict of defaults, or a list of dictionaries for each obj
intermediate_values = intermediate_values or {}
if isinstance(intermediate_values, dict):
intermediate_values = [
intermediate_values for obj in objs
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check to be sure len(intermediate_values) == len(objs).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did length checking we'd want to compare len(intermediate_values) == len(new_ids) since these are the list getting zipped, and that won't really help because (I missed this originally) new_ids being a set which has all existing members removed makes the zip untenable.


# If there aren't any objects, there is nothing to do.
from django.db.models import Model
Expand Down Expand Up @@ -1080,11 +1058,11 @@ def _add_items(self, source_field_name, target_field_name, *objs):

# Add the ones that aren't there already
self.through._default_manager.using(db).bulk_create([
self.through(**{
self.through(**dict(extra_values, **{
'%s_id' % source_field_name: self.related_val[0],
'%s_id' % target_field_name: obj_id,
})
for obj_id in new_ids
}))
for obj_id, extra_values in zip(new_ids, intermediate_values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep in mind new_ids could be smaller than objs, messing up index order so intermediate_values won't match up, right? Also, new_ids is a set, so order is lost.

Copy link
Author

@astandley astandley Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, missed both those points originally. Clearly needs a rework....

])

if self.reverse or source_field_name == self.source_field_name:
Expand Down
37 changes: 29 additions & 8 deletions docs/ref/models/relations.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Related objects reference
In this example, the methods below will be available both on
``topping.pizza_set`` and on ``pizza.toppings``.

.. method:: add(*objs, bulk=True)
.. method:: add(*objs, bulk=True, through_defaults=None)

Adds the specified model objects to the related object set.

Expand All @@ -62,7 +62,14 @@ Related objects reference
some custom logic when a relationship is created, listen to the
:data:`~django.db.models.signals.m2m_changed` signal.

.. method:: create(**kwargs)
The ``through_defaults`` argument can be used to specify values for
the new :ref:`an intermediate model <intermediary-manytomany>` instance.

.. versionchanged:: 2.1

The ``through_defaults`` argument was added.

.. method:: create(through_defaults=None, **kwargs)

Creates a new object, saves it and puts it in the related object set.
Returns the newly created object::
Expand Down Expand Up @@ -92,6 +99,13 @@ Related objects reference
parameter ``blog`` to ``create()``. Django figures out that the new
``Entry`` object's ``blog`` field should be set to ``b``.

The ``through_defaults`` argument can be used to specify values for
the new :ref:`an intermediate model <intermediary-manytomany>` instance.

.. versionchanged:: 2.1

The ``through_defaults`` argument was added.

.. method:: remove(*objs)

Removes the specified model objects from the related object set::
Expand Down Expand Up @@ -125,6 +139,11 @@ Related objects reference
:data:`~django.db.models.signals.post_save` signals and comes at the
expense of performance.

.. versionchanged:: 2.1

The ``remove()`` method is now allowed on relationships with
custom intermediate models.

.. method:: clear()

Removes all objects from the related object set::
Expand All @@ -139,7 +158,7 @@ Related objects reference
:class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also
accepts the ``bulk`` keyword argument.

.. method:: set(objs, bulk=True, clear=False)
.. method:: set(objs, bulk=True, clear=False, through_defaults=None)

Replace the set of related objects::

Expand All @@ -158,18 +177,20 @@ Related objects reference
race conditions. For instance, new objects may be added to the database
in between the call to ``clear()`` and the call to ``add()``.

The ``through_defaults`` argument can be used to specify values for
new :ref:`an intermediate model <intermediary-manytomany>` instances.

.. note::

Note that ``add()``, ``create()``, ``remove()``, ``clear()``, and
``set()`` all apply database changes immediately for all types of
related fields. In other words, there is no need to call ``save()``
on either end of the relationship.

Also, if you are using :ref:`an intermediate model
<intermediary-manytomany>` for a many-to-many relationship, then the
``add()``, ``create()``, ``remove()``, and ``set()`` methods are
disabled.

If you use :meth:`~django.db.models.query.QuerySet.prefetch_related`,
the ``add()``, ``remove()``, ``clear()``, and ``set()`` methods clear
the prefetched cache.

.. versionchanged:: 2.1

The ``through_defaults`` argument was added.
6 changes: 6 additions & 0 deletions docs/releases/2.1.txt
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ Migrations
Models
~~~~~~

* The ``add()``, ``create()``, ``get_or_create()``, ``update_or_create()``,
``set()``, and ``remove()`` methods are now allowed on many-to-many
relationships with custom intermediate models. There is a
``through_defaults`` argument for specifying values for new intermediate
models.

* Models can now use ``__init_subclass__()`` from :pep:`487`.

* A ``BinaryField`` may now be set to ``editable=True`` if you wish to include
Expand Down
5 changes: 1 addition & 4 deletions docs/topics/db/examples/many_to_many.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ objects, and a ``Publication`` has multiple ``Article`` objects:
ordering = ('headline',)

What follows are examples of operations that can be performed using the Python
API facilities. Note that if you are using :ref:`an intermediate model
<intermediary-manytomany>` for a many-to-many relationship, some of the related
manager's methods are disabled, so some of these examples won't work with such
models.
API facilities.

Create a couple of ``Publications``::

Expand Down
52 changes: 25 additions & 27 deletions docs/topics/db/models.txt
Original file line number Diff line number Diff line change
Expand Up @@ -511,37 +511,36 @@ the intermediate model::
>>> beatles.members.all()
<QuerySet [<Person: Ringo Starr>, <Person: Paul McCartney>]>

Unlike normal many-to-many fields, you *can't* use ``add()``, ``create()``,
or ``set()`` to create relationships::

>>> # The following statements will not work
>>> beatles.members.add(john)
>>> beatles.members.create(name="George Harrison")
>>> beatles.members.set([john, paul, ringo, george])

Why? You can't just create a relationship between a ``Person`` and a ``Group``
- you need to specify all the detail for the relationship required by the
``Membership`` model. The simple ``add``, ``create`` and assignment calls
don't provide a way to specify this extra detail. As a result, they are
disabled for many-to-many relationships that use an intermediate model.
The only way to create this type of relationship is to create instances of the
intermediate model.

The :meth:`~django.db.models.fields.related.RelatedManager.remove` method is
disabled for similar reasons. For example, if the custom through table defined
by the intermediate model does not enforce uniqueness on the
``(model1, model2)`` pair, a ``remove()`` call would not provide enough
information as to which intermediate model instance should be deleted::
You can also use ``add()``, ``create()``, or ``set()`` to create relationships,
as long as your specify ``through_defaults`` for any required fields::

>>> beatles.members.add(john, through_defaults={'date_joined': date(1960, 8, 1)})
>>> beatles.members.create(name="George Harrison", through_defaults={'date_joined': date(1960, 8, 1)})
>>> beatles.members.set([john, paul, ringo, george], through_defaults={'date_joined': date(1960, 8, 1)})

For ``add()`` and ``set()`` you may either specify a single 'default' dictionary or a list of dictionaries
if you wish to specify details for each object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... If we do this, maybe the kwarg should just be called intermediate_values everywhere (that can be either a single dict or list of dicts)

I feel like passing in two separate lists and relying matching indexes is a little messy, and looking at the code, I've already found some possible bugs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, I suppose the "through_defaults" does make a lot of sense for pretty much everywhere except add() and create(). So nevermind. I think _add_items should keep using through_defaults as the kwarg to be consistant. You could use intermediate_values as a local variable if you want.

Copy link
Author

@astandley astandley Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that passing two lists is messy as can be. My original intention was to only allow the dictionary of defaults, but then I realized that being able to set different values for different objects could be valuable. Mucked it up pretty hard after that.
It's hard to find clean ways to add values without breaking the interface. Clearly needs more thought :/

Good point about intermediate_values. I'm really not sure why I used a different name. Could have just been I forgot to update it to through_defaults.

@collinanderson Thanks for the feedback!


>>> dates_joined = [{'date_joined': date(1957, 3, 1)},{'date_joined': date(1957, 7, 1)}]
>>> beatles.members.add([john, paul], through_defaults=dates_joined)
>>> beatles.members.set([john, paul], through_defaults=dates_joined)

If the custom through table defined by the intermediate model does not enforce
uniqueness on the ``(model1, model2)`` pair, allowing multiple values, the
:meth:`~django.db.models.fields.related.RelatedManager.remove` call will
remove all intermediate model instances::

>>> Membership.objects.create(person=ringo, group=beatles,
... date_joined=date(1968, 9, 4),
... invite_reason="You've been gone for a month and we miss you.")
>>> beatles.members.all()
<QuerySet [<Person: Ringo Starr>, <Person: Paul McCartney>, <Person: Ringo Starr>]>
>>> # This will not work because it cannot tell which membership to remove
>>> # This deletes both of the intermediate model instances for Ringo Starr
>>> beatles.members.remove(ringo)
>>> beatles.members.all()
<QuerySet [<Person: Paul McCartney>]>

However, the :meth:`~django.db.models.fields.related.RelatedManager.clear`
The :meth:`~django.db.models.fields.related.RelatedManager.clear`
method can be used to remove all many-to-many relationships for an instance::

>>> # Beatles have broken up
Expand All @@ -550,10 +549,9 @@ method can be used to remove all many-to-many relationships for an instance::
>>> Membership.objects.all()
<QuerySet []>

Once you have established the many-to-many relationships by creating instances
of your intermediate model, you can issue queries. Just as with normal
many-to-many relationships, you can query using the attributes of the
many-to-many-related model::
Once you have established the many-to-many relationships, you can issue
queries. Just as with normal many-to-many relationships, you can query using
the attributes of the many-to-many-related model::

# Find all the groups with a member whose name starts with 'Paul'
>>> Group.objects.filter(members__name__startswith='Paul')
Expand Down
7 changes: 7 additions & 0 deletions tests/m2m_through/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class Group(models.Model):
through='TestNoDefaultsOrNulls',
related_name="testnodefaultsnonulls",
)
required_through = models.ManyToManyField(Person, through='RequiredThrough', related_name='required_through')

class Meta:
ordering = ('name',)
Expand Down Expand Up @@ -69,6 +70,12 @@ class TestNoDefaultsOrNulls(models.Model):
nodefaultnonull = models.CharField(max_length=5)


class RequiredThrough(models.Model):
person = models.ForeignKey(Person, models.CASCADE)
group = models.ForeignKey(Group, models.CASCADE)
required = models.IntegerField()


class PersonSelfRefM2M(models.Model):
name = models.CharField(max_length=5)
friends = models.ManyToManyField('self', through="Friendship", symmetrical=False)
Expand Down