Opened 8 years ago

Last modified 5 days ago

#8467 new New feature

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: master
Severity: Normal Keywords: Duplicate entry, add, remove, ManyToManyField
Cc: wonlay@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
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 8 years ago.

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by Wonlay

comment:1 Changed 8 years ago by James Bennett

milestone: 1.0post-1.0
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:3 Changed 8 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

comment:4 Changed 5 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:5 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 Changed 4 years ago by Aymeric Augustin

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

comment:8 Changed 4 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 3 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 11 months ago by Tim Graham

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

comment:11 Changed 11 months 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 5 days ago by Tim Graham

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

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