Opened 8 years ago

Last modified 5 months ago

#9475 new New feature

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: master
Severity: Normal Keywords:
Cc: robinchew@…, someuniquename@…, piethon@…, naczelnik-djangoproject@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
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 7 years ago.
9475-r11858.diff (17.8 KB) - added by Travis Cline <travis.cline@…> 7 years ago.
9475.m2m_add_remove.r12009.diff (22.7 KB) - added by Johannes Dollinger 7 years ago.
9475.m2m_add_remove.r12281.diff (24.4 KB) - added by Johannes Dollinger 7 years ago.
smart_m2mfield.py (6.1 KB) - added by Antti Kaihola 7 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 5 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by Jacob

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Changed 7 years ago by Johannes Dollinger

comment:2 Changed 7 years ago by 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.

comment:3 Changed 7 years ago by Russell Keith-Magee

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

comment:4 in reply to:  3 Changed 7 years ago by 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.

comment:5 Changed 7 years ago by anonymous

Patch needs improvement: unset
Version: 1.0SVN

comment:6 Changed 7 years ago by Johannes Dollinger

Has patch: set
Patch needs improvement: set

...

Changed 7 years ago by Travis Cline <travis.cline@…>

Attachment: 9475-r11858.diff added

comment:7 Changed 7 years ago by 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.

comment:8 in reply to:  7 ; Changed 7 years ago by 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.

comment:9 in reply to:  8 ; Changed 7 years ago by 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.

comment:10 in reply to:  9 Changed 7 years ago by 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 ?

Changed 7 years ago by Johannes Dollinger

Changed 7 years ago by Johannes Dollinger

Changed 7 years ago by Antti Kaihola

Attachment: smart_m2mfield.py added

partial solution as a separate module for non-patched Django

comment:11 Changed 7 years ago by 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.

comment:12 Changed 6 years ago by 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.

comment:13 Changed 5 years ago by Johannes Dollinger

Easy pickings: unset
Severity: Normal
Type: Uncategorized

#8334 is a duplicate.

comment:14 Changed 5 years ago by Johannes Dollinger

Type: UncategorizedNew feature

Changed 5 years ago by Johannes Dollinger

comment:15 Changed 5 years ago by Johannes Dollinger

Needs documentation: set
Patch needs improvement: unset

comment:16 Changed 5 years ago by Robin

Cc: robinchew@… added
UI/UX: unset

comment:17 Changed 12 months ago by Tim Graham

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 Changed 8 months ago by Evstifeev Roman

Cc: someuniquename@… added

comment:19 Changed 6 months ago by Matt C

Cc: piethon@… added

comment:20 Changed 5 months ago by Adam Dobrawy

Cc: naczelnik-djangoproject@… added
Note: See TracTickets for help on using tickets.
Back to Top