[Django] #25672: Related ManyToMany fields with custom intermediary model should not disable remove method.
Django
remove method.
----------------------------------------------+----------------------------
Reporter: Antwan86 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: 1.8
Severity: Normal | Keywords: manytomany
| related
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------------
It is currently impossible to add or create related item to a many-to-many
relationship with custom intermediary model, with reason, as the custom
model extra fields won't be populated.
[https://github.com/django/django/blob/1.8.5/docs/topics/db/models.txt#L508
This is documented and explained].
However, it is also not possible to remove related items from the
collection, and the only reason given in the doc is : ''"The remove()
method is disabled for similar reasons"''.
I've try to find a justification for this though the code/docs/history but
I couldn't and this seems unjustified for me.
All the informations needed to remove the relation are provided if you
just use this method e.g.
`mysourceobject.relatedobjects.remove(targetobject)`.
I don't see why
[https://github.com/django/django/blob/1.8.5/django/db/models/fields/related.py#L982
it is disabled], and therefore would like to ask for a design change here,
or an explanation.
--
Ticket URL: <https://code.djangoproject.com/ticket/25672>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
remove method.
Reporter: Antwan86 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
Keywords: manytomany related | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Russ called out the `remove()` method in ticket:6095#comment:33 but I fail
to understand that reasoning as well. There doesn't seem to be any problem
modify the relevant tests (patch attached - would need doc updates too).
Are we missing something?
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:1>
Django
remove method.
Reporter: Antwan86 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "25672.diff" added.
Django
remove method.
Reporter: Antwan86 | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by bmispelon):
I had a chat with Russ and he raised the following point. Consider a
custome through table that (unlike the auto-generated one) doesn't enforce
unicity on the `(model1, model2)` pair (which is a valid usecase).
Something like this:
{{{#!python
class Pizza(models.Model):
name = models.CharField(max_length=50)
class Customer(models.Model):
name = models.CharField(max_length=50)
orders = models.ManyToManyField('Pizza', through='Order')
class Order(models.Model):
pizza = models.ForeignKey('Pizza')
customer = models.ForeignKey('Customer')
ordered_on = models.DateTimeField(default=timezone.now)
}}}
Doing `some_customer.orders.remove(some_pizza)` doesn't give enough
information as to which `Order` instance should actually be deleted.
It seems to me that we should just better document the reason behind this,
considering it's not exactly obvious.
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:2>
Django
disable the remove() method.
--------------------------------------+------------------------------------
Reporter: Antwan86 | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: manytomany related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* component: Database layer (models, ORM) => Documentation
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thanks Baptiste!
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:3>
Django
disable the remove() method.
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: manytomany related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* Attachment "25672Docs.diff" added.
Django
disable the remove() method.
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: manytomany related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by benred42):
Took a stab at a documentation patch to clarify this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:4>
Django
disable the remove() method.
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:5>
Django
disable the remove() method.
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: manytomany related | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by benred42):
Made a PR with for my patch (PR 5728
https://github.com/django/django/pull/5728)
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:6>
Django
disable the remove() method.
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: manytomany related | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d508326e2f7d35973e2662109e34b425cb108b81" d508326]:
{{{
#!CommitTicketReference repository=""
revision="d508326e2f7d35973e2662109e34b425cb108b81"
Fixed #25672 -- Clarified why related ManyToManyFields with a custom
intermediate model disable the remove() method.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25672#comment:7>