For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Wonlay]

For example:

class A(models.Model):
	name = models.CharField(blank=True)

class B(models.Model):
	a = models.ManyToManyField(A)

a = A(name='aaa')
a.save()
b = B()
b.save()

# This line works fine:
b.a.add('1')

# But this one will raise 'Duplicate entry' error:
b.a.add('1')

This is caused by '1' is not an integer, the duplication checking code in add() fails when checking the string type '1'.

The same problem is applied to the remove() method.

If we convert the objects to its pk type of the model, code will run correctly.

And my patch is attached.

[(none)]

Milestone post-1.0 deleted

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Anssi Kääriäinen]

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.

[Tim Graham]

Patch needs improvement per @akaariai's comment as well as tests.

[Tim Graham]

What's the use case for this functionality? Do we do similar type coercion elsewhere?

[Anssi Kääriäinen]

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().

[Tim Graham]

As noted in duplicate #27249, calling Field.to_python() on the input might work.

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

[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 ​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.

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

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

[Simon Charette]

​https://github.com/django/django/pull/12123

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8cc7119:

Refs #8467 -- Added test for RelatedManager.add()/remove() with an invalid type.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 379bf1a2:

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.