Opened 13 years ago

Closed 16 months ago

#8467 closed New feature (fixed)

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

Reported by: Wonlay Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: Duplicate entry, add, remove, ManyToManyField
Cc: wonlay@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

smart_many2many_add_remove.patch (1.2 KB) - added by Wonlay 13 years ago.

Download all attachments as: .zip

Change History (20)

Changed 13 years ago by Wonlay

comment:1 Changed 13 years ago by James Bennett

milestone: 1.0post-1.0

comment:2 Changed 12 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:3 Changed 12 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 10 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:5 Changed 9 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 Changed 9 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 Changed 8 years ago by Aymeric Augustin

Component: Core (Other)Database layer (models, ORM)

comment:8 Changed 8 years ago by Anssi Kääriäinen

Triage Stage: Design decision neededAccepted

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.

comment:9 Changed 8 years ago by Tim Graham

Needs tests: set
Patch needs improvement: set

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

comment:10 Changed 5 years ago by Tim Graham

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

comment:11 Changed 5 years ago by 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().

comment:12 Changed 4 years ago by Tim Graham

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

comment:13 Changed 16 months ago 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.

comment:14 Changed 16 months ago 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 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.

comment:15 Changed 16 months ago 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.

comment:16 Changed 16 months ago 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.

comment:17 Changed 16 months ago by Simon Charette

Needs tests: unset
Patch needs improvement: unset

comment:18 Changed 16 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 8cc7119:

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

comment:19 Changed 16 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top