add(), create(), etc. should be supported by intermediate ManyToMany model with extra attributes if extra fields can be calculated

[omat@…]

Intermedite ManyToMany model with extra attributes should support add(), create(), etc. when the extra fields have default values or are calculated.

For example, for the following models:

class Person(models.Model):
    name = models.CharField(max_length=128)

class Group(models.Model):
    name = models.CharField(max_length=128)
    members = models.ManyToManyField(Person, through='Membership')

class Membership(models.Model):
    person = models.ForeignKey(Person)
    group = models.ForeignKey(Group)
    added = models.DateField(auto_now_add=True)

This should be possible:

Group.members.add(Person.objects.create(name='Joe'))

Similarly, if the attributes are calculated in the custom save() method or if they have a default value, those methods should be supported.

Maybe, simply removing the extra checks will do. So is it is the values are not sufficient, a db error will be raised anyway.

[Johannes Dollinger]

The patch adds can_add and can_remove kwargs to ManyToManyField. Both default to None, which means:

• can_remove=True, iff the relevant foreign keys on the intermediary model are unique_together.

• can_add=True, iff all extra fields on the intermediary model have a default value, are nullable, or have auto_now(_add)?=True.

If can_add is True, add(), create(), and assignment are available for managers/descriptors.

If can_remove is True, remove() is available on managers.

[Russell Keith-Magee]

Thanks for the patch, However, this isn't something that should require a manual 'can_add' flag. We know all the relevant details - which fields are unique, which fields have defaults etc. We can therefore compute the value that can_add and can_remove provide manually in this patch.

As a secondary issue, there is also an API issue around the call to add(). If you read the discussion on #6095 - the ticket that originally introduced m2m intermediates - there was some discussion about whether add() could be modified to allow people to add m2m entries *with* intermediate data. If we're going to close this ticket, it would be nice to get some resolution on this closely related issue, even if it's just to say "no".

[anonymous]

Replying to russellm:

Thanks for the patch, However, this isn't something that should require a manual 'can_add' flag. We know all the relevant details - which fields are unique, which fields have defaults etc. We can therefore compute the value that can_add and can_remove provide manually in this patch.

As a secondary issue, there is also an API issue around the call to add(). If you read the discussion on #6095 - the ticket that originally introduced m2m intermediates - there was some discussion about whether add() could be modified to allow people to add m2m entries *with* intermediate data. If we're going to close this ticket, it would be nice to get some resolution on this closely related issue, even if it's just to say "no".

Exposing those flags (a two line change) allows fields on intermediary models to be filled in by save() and "bulk delete" behavior if the foreign keys are not unique_together. The ability to explicitly disable add() or remove() would be nice, but you could just say "don't do that then".

Regarding your secondary issue, I'm in favour of allowing add() to just work for every m2m (Favoured color:

add(obj1, obj2, extra_field1="foo", extra_field2="bar")`

). But that does not translate cleanly to create() and assignment. So the distinction between simple intermediary models and more complex ones probably won't go away.

If some kind of add(..., extra=...) support is in scope for this ticket, I'll happily add it.

[Johannes Dollinger]

...

[Travis Cline <travis.cline@…>]

Attached is an alternate approach. Will also attach a similar approach but with an attempt to fall back to a reasonable default.

[Johannes Dollinger]

Replying to Travis Cline <travis.cline@gmail.com>:

Attached is an alternate approach. Will also attach a similar approach but with an attempt to fall back to a reasonable default.

I don't believe the flag should be on Meta. The intermediary model doesn't know which FK fields are used for the m2m relation (at least in theory there may be more than one m2m relation through the same model). And you would introduce a model option that has no meaning for most models.

[Travis Cline <travis.cline@…>]

Replying to emulbreh:

Replying to Travis Cline <travis.cline@gmail.com>:

Attached is an alternate approach. Will also attach a similar approach but with an attempt to fall back to a reasonable default.

I don't believe the flag should be on Meta. The intermediary model doesn't know which FK fields are used for the m2m relation (at least in theory there may be more than one m2m relation through the same model). And you would introduce a model option that has no meaning for most models.

Yeah, you're right. Not nearly the right place for that to live. I hope to follow up with a patch that isn't so whack soon.

[anonymous]

Replying to Travis Cline <travis.cline@gmail.com>:

Replying to emulbreh:

Replying to Travis Cline <travis.cline@gmail.com>:

Attached is an alternate approach. Will also attach a similar approach but with an attempt to fall back to a reasonable default.

I don't believe the flag should be on Meta. The intermediary model doesn't know which FK fields are used for the m2m relation (at least in theory there may be more than one m2m relation through the same model). And you would introduce a model option that has no meaning for most models.

Yeah, you're right. Not nearly the right place for that to live. I hope to follow up with a patch that isn't so whack soon.

What's wrong with 9475.m2m_add_remove.r11739.diff ?

[Antti Kaihola]

partial solution as a separate module for non-patched Django

[Antti Kaihola]

The above patch is what I've been cooking as a minimal solution to get this functionality on top of non-patched Django. It should be enough if default= is provided for all extra fields. I hope it makes sense to attach it here for convenience of those not willing to patch.

[Russell Keith-Magee]

On a related note -- the same limitation applies to admin; an m2m with a manually specified intermediate is prevented from using the pretty admin MultipleSelect widget.

[Johannes Dollinger]

#8334 is a duplicate.

[Dylan Young]

I'm a little confused as to why we don't just add an extra optional kwarg to these methods to support custom through models (i.e. defaults for add/create/set and lookups for remove).

Can someone clarify?

[Collin Anderson]

proof of concept: ​https://github.com/django/django/compare/master...collinanderson:ticket9475

[Victor Porton]

Also by the course of fixing this bug we should eliminate error messages for ModelForms like:

AttributeError: Cannot set values on a ManyToManyField which specifies an intermediary model. Use post_app.PostInTimeline's Manager instead.

[Collin Anderson]

Pull Request: ​https://github.com/django/django/pull/8981

[astandley]

​​https://github.com/django/django/pull/8981 lacks tests for successful add(), create(), etc for ManyToManyFields where extra fields are required on the through model.
The required_through field is only tested for failure.

[Collin Anderson]

I just rebased this patch, updated version numbers, and added fail and success tests for add, create, set, get_or_create, and update_or_create. ​https://github.com/django/django/pull/8981

[Tim Graham <timograham@…>]

In 769355c7:

Fixed #9475 -- Allowed RelatedManager.add(), create(), etc. for m2m with a through model.

[Tim Graham <timograham@…>]

In d212bc0:

Refs #9475 -- Fixed typo, used unpacking generalization, and made through_defaults kwarg-only.

[Tim Graham <timograham@…>]

In dbcf2ff:

Refs #9475 -- Simplified dictionary unpacking.

[GitHub <noreply@…>]

In 284bde3f:

Refs #9475 -- Linked through_default docs to related managers methods.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8aedad69:

[3.1.x] Refs #9475 -- Linked through_default docs to related managers methods.

Backport of 284bde3fbe07485d64289e28014a4eada68aef91 from master