Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: Duplicate entry, | Needs documentation: 0
add, remove, ManyToManyField | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* stage: Design decision needed => Accepted
Comment:
I think this fix makes sense. The patch should be made aware of
from_field/to_field if possible, even it is not possible to create such
ManyToManys currently this feature will very likely appear some day.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: Duplicate entry, | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Patch needs improvement per @akaariai's comment as well as tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:9>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
What's the use case for this functionality? Do we do similar type coercion
elsewhere?
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:10>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
I think we should fix this on the basis that using b.a.add('1') works
partly, but the duplication check doesn't work. Either we should reject
strings where ints are required, or handle type coercions fully.
To fix this, we should coerce the input values to right type right at the
beginning of add() and remove(). The same applies likely also to reverse
foreign key sets. Still, we need to convert to the primary key type of the
target model, that is, the code should work also when A had a custom
primary key (for example, A.name was primary key).
Tests for m2m.add can be found from
https://github.com/akaariai/django/commit/b1308c114f601db2f3f0c8d76c55acd966a14672.
Similar tests should be added for .remove(), and for reverse foreign key
.add() and .remove().
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:11>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
As noted in duplicate #27249, calling `Field.to_python()` on the input
might work.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:12>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Baptiste Mispelon):
I was unable to reproduce this on a recent version (tag `3.0rc1` for
example).
I'm using the following test case (same models as in the original ticket):
{{{
from django.db import IntegrityError
from django.test import TestCase
from .models import A, B
class Ticket8467TestCase(TestCase):
def test_integrityerror(self):
pk = A.objects.create().pk
b = B.objects.create()
b.a.add(str(pk))
with self.assertRaises(IntegrityError):
b.a.add(str(pk))
}}}
This test fails (meaning that an `IntegrityError` is not raised) on
`3.0rc1` but passes for `2.2`.
Using `git bisect`, I tracked it down to this commit:
28712d8acfffa9cdabb88cb610bae14913fa185d.
However it's not immediately clear to me why that commit would fix this
ticket so I'm unsure whether I should close this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:13>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I'm pretty sure we can consider this fixed by
28712d8acfffa9cdabb88cb610bae14913fa185d.
Even if we don't perform the conversion to the pk type in
[https://github.com/django/django/blob/master/django/db/models/fields/related_descriptors.py#L1066-L1067
the branch handling non-model instances] of `_get_target_ids` we now
ignore conflicts on bulk insertions so the collision on insertion is now
ignored.
I guess we could adjust the branch as an optimization if we wanted to by
calling `target_field.to_python(obj)` by re-purposing this ticket as an
optimization.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:14>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
A regression test for the above optimization could be to
`assertNumQueries(1)` on the `b.a.add(str(pk))` call when a `m2m_changed`
signal is registered for the relationship. That is to disable the ''fast''
insertion mechanism that is enabled on all backends except for Oracle.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:15>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Actually, this is more than an optimization because it will still crash
after 28712d8acfffa9cdabb88cb610bae14913fa185d on backends that don't have
the `supports_ignore_conflicts` feature flag enabled (only Oracle).
I'll submit a PR for it.
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:16>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Type: New feature | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
https://github.com/django/django/pull/12123
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:17>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Keywords: Duplicate entry, | Triage Stage: Accepted
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"379bf1a2d41494360d86bc3cf8adc482abca5d63" 379bf1a2]:
{{{
#!CommitTicketReference repository=""
revision="379bf1a2d41494360d86bc3cf8adc482abca5d63"
Fixed #8467 -- Prevented crash when adding existent m2m relation with an
invalid type.
This was an issue anymore on backends that allows conflicts to be
ignored (Refs #19544) as long the provided values were coercible to the
expected type. However on the remaining backends that don't support
this feature, namely Oracle, this could still result in an
IntegrityError.
By attempting to coerce the provided values to the expected types in
Python beforehand we allow the existing value set intersection in
ManyRelatedManager._get_missing_target_ids to prevent the problematic
insertion attempts.
Thanks Baptiste Mispelon for triaging this old ticket against the
current state of the master branch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:19>
Django
the pk type if they are not.
-------------------------------------+-------------------------------------
Reporter: Wonlay | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
add, remove, ManyToManyField |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"8cc711999ab839c1a93cb228b35beac4a454fafc" 8cc7119]:
{{{
#!CommitTicketReference repository=""
revision="8cc711999ab839c1a93cb228b35beac4a454fafc"
Refs #8467 -- Added test for RelatedManager.add()/remove() with an invalid
type.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/8467#comment:18>