Opened 16 years ago

Closed 6 years ago

Last modified 4 years ago

#9475 closed New feature (fixed)

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

Reported by: omat@… Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: robinchew@…, someuniquename@…, piethon@…, naczelnik-djangoproject@…, Collin Anderson, astandley, Rich Rauenzahn, Zach Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (6)

9475.m2m_add_remove.r11739.diff (22.6 KB ) - added by Johannes Dollinger 15 years ago.
9475-r11858.diff (17.8 KB ) - added by Travis Cline <travis.cline@…> 15 years ago.
9475.m2m_add_remove.r12009.diff (22.7 KB ) - added by Johannes Dollinger 15 years ago.
9475.m2m_add_remove.r12281.diff (24.4 KB ) - added by Johannes Dollinger 15 years ago.
smart_m2mfield.py (6.1 KB ) - added by Antti Kaihola 15 years ago.
partial solution as a separate module for non-patched Django
9475.m2m_add_remove.r16133.diff (25.1 KB ) - added by Johannes Dollinger 14 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

by Johannes Dollinger, 15 years ago

comment:2 by Johannes Dollinger, 15 years ago

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.

comment:3 by Russell Keith-Magee, 15 years ago

Patch needs improvement: set

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".

in reply to:  3 comment:4 by anonymous, 15 years ago

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.

comment:5 by anonymous, 15 years ago

Patch needs improvement: unset
Version: 1.0SVN

comment:6 by Johannes Dollinger, 15 years ago

Has patch: set
Patch needs improvement: set

...

by Travis Cline <travis.cline@…>, 15 years ago

Attachment: 9475-r11858.diff added

comment:7 by Travis Cline <travis.cline@…>, 15 years ago

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

in reply to:  7 ; comment:8 by Johannes Dollinger, 15 years ago

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.

in reply to:  8 ; comment:9 by Travis Cline <travis.cline@…>, 15 years ago

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.

in reply to:  9 comment:10 by anonymous, 15 years ago

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 ?

by Johannes Dollinger, 15 years ago

by Johannes Dollinger, 15 years ago

by Antti Kaihola, 15 years ago

Attachment: smart_m2mfield.py added

partial solution as a separate module for non-patched Django

comment:11 by Antti Kaihola, 15 years ago

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.

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

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.

comment:13 by Johannes Dollinger, 14 years ago

Easy pickings: unset
Severity: Normal
Type: Uncategorized

#8334 is a duplicate.

comment:14 by Johannes Dollinger, 14 years ago

Type: UncategorizedNew feature

by Johannes Dollinger, 14 years ago

comment:15 by Johannes Dollinger, 14 years ago

Needs documentation: set
Patch needs improvement: unset

comment:16 by Robin, 13 years ago

Cc: robinchew@… added
UI/UX: unset

comment:17 by Tim Graham, 9 years ago

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

comment:18 by Evstifeev Roman, 9 years ago

Cc: someuniquename@… added

comment:19 by Matt C, 9 years ago

Cc: piethon@… added

comment:20 by Adam Dobrawy, 8 years ago

Cc: naczelnik-djangoproject@… added

comment:21 by Dylan Young, 8 years ago

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 dict for add/create/set and lookups dict for remove).

Can someone clarify?

EDIT: I see this was mentioned above, but no one responded. I'll happily take this on if there is interest.

Last edited 8 years ago by Dylan Young (previous) (diff)

comment:23 by Victor Porton, 8 years ago

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.

comment:24 by Collin Anderson, 7 years ago

Cc: Collin Anderson added

comment:25 by Collin Anderson, 7 years ago

Needs documentation: unset

comment:26 by astandley, 7 years ago

Cc: astandley added
Needs tests: set

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.

comment:27 by Rich Rauenzahn, 6 years ago

Cc: Rich Rauenzahn added

comment:28 by Zach, 6 years ago

Cc: Zach added

comment:29 by Collin Anderson, 6 years ago

Needs tests: unset

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

comment:30 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In 769355c7:

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

comment:31 by Tim Graham <timograham@…>, 6 years ago

In d212bc0:

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

comment:32 by Tim Graham <timograham@…>, 6 years ago

In dbcf2ff:

Refs #9475 -- Simplified dictionary unpacking.

comment:33 by GitHub <noreply@…>, 4 years ago

In 284bde3f:

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

comment:34 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 8aedad69:

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

Backport of 284bde3fbe07485d64289e28014a4eada68aef91 from master

Note: See TracTickets for help on using tickets.
Back to Top