#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: | 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)
Change History (40)
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
by , 15 years ago
Attachment: | 9475.m2m_add_remove.r11739.diff added |
---|
comment:2 by , 15 years ago
follow-up: 4 comment:3 by , 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".
comment:4 by , 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 , 15 years ago
Patch needs improvement: | unset |
---|---|
Version: | 1.0 → SVN |
by , 15 years ago
Attachment: | 9475-r11858.diff added |
---|
follow-up: 8 comment:7 by , 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.
follow-up: 9 comment:8 by , 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.
follow-up: 10 comment:9 by , 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.
comment:10 by , 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 , 15 years ago
Attachment: | 9475.m2m_add_remove.r12009.diff added |
---|
by , 15 years ago
Attachment: | 9475.m2m_add_remove.r12281.diff added |
---|
by , 15 years ago
Attachment: | smart_m2mfield.py added |
---|
partial solution as a separate module for non-patched Django
comment:11 by , 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 , 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 , 14 years ago
Easy pickings: | unset |
---|---|
Severity: | → Normal |
Type: | → Uncategorized |
#8334 is a duplicate.
comment:14 by , 14 years ago
Type: | Uncategorized → New feature |
---|
by , 14 years ago
Attachment: | 9475.m2m_add_remove.r16133.diff added |
---|
comment:15 by , 14 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | unset |
comment:16 by , 13 years ago
Cc: | added |
---|---|
UI/UX: | unset |
comment:17 by , 9 years ago
Summary: | add(), create(), etc. should be supported by intermedite ManyToMany model with extra attributes if extra fields can be calculated → add(), create(), etc. should be supported by intermediate ManyToMany model with extra attributes if extra fields can be calculated |
---|
comment:18 by , 9 years ago
Cc: | added |
---|
comment:19 by , 9 years ago
Cc: | added |
---|
comment:20 by , 8 years ago
Cc: | added |
---|
comment:21 by , 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.
comment:22 by , 8 years ago
comment:23 by , 8 years ago
Also by the course of fixing this bug we should eliminate error messages for ModelForm
s like:
AttributeError: Cannot set values on a ManyToManyField which specifies an intermediary model. Use post_app.PostInTimeline's Manager instead.
comment:25 by , 7 years ago
Needs documentation: | unset |
---|
comment:26 by , 7 years ago
Cc: | 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 , 6 years ago
Cc: | added |
---|
comment:28 by , 6 years ago
Cc: | added |
---|
comment:29 by , 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
The patch adds
can_add
andcan_remove
kwargs toManyToManyField
. Both default toNone
, which means:can_remove=True
, iff the relevant foreign keys on the intermediary model areunique_together
.can_add=True
, iff all extra fields on the intermediary model have a default value, are nullable, or haveauto_now(_add)?=True
.If
can_add
isTrue
,add()
,create()
, and assignment are available for managers/descriptors.If
can_remove
isTrue
,remove()
is available on managers.