Code

Opened 6 years ago

Last modified 3 years ago

#9475 new New feature

add(), create(), etc. should be supported by intermedite 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@… 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 emulbreh 5 years ago.
9475-r11858.diff (17.8 KB) - added by Travis Cline <travis.cline@…> 5 years ago.
9475.m2m_add_remove.r12009.diff (22.7 KB) - added by emulbreh 5 years ago.
9475.m2m_add_remove.r12281.diff (24.4 KB) - added by emulbreh 4 years ago.
smart_m2mfield.py (6.1 KB) - added by akaihola 4 years ago.
partial solution as a separate module for non-patched Django
9475.m2m_add_remove.r16133.diff (25.1 KB) - added by emulbreh 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 5 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by emulbreh

comment:2 Changed 5 years ago by emulbreh

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 follow-up: Changed 5 years ago by russellm

  • 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 5 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 5 years ago by anonymous

  • Patch needs improvement unset
  • Version changed from 1.0 to SVN

comment:6 Changed 5 years ago by emulbreh

  • Has patch set
  • Patch needs improvement set

...

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

comment:7 follow-up: Changed 5 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 ; follow-up: Changed 5 years ago by 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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 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 5 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 5 years ago by emulbreh

Changed 4 years ago by emulbreh

Changed 4 years ago by akaihola

partial solution as a separate module for non-patched Django

comment:11 Changed 4 years ago by akaihola

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 4 years ago by russellm

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 3 years ago by emulbreh

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized

#8334 is a duplicate.

comment:14 Changed 3 years ago by emulbreh

  • Type changed from Uncategorized to New feature

Changed 3 years ago by emulbreh

comment:15 Changed 3 years ago by emulbreh

  • Needs documentation set
  • Patch needs improvement unset

comment:16 Changed 3 years ago by robin

  • Cc robinchew@… added
  • UI/UX unset

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.