Opened 8 years ago

Last modified 10 months 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


For example:

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

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

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

# This line works fine:

# But this one will raise 'Duplicate entry' error:

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

Changed 8 years ago by Wonlay

comment:1 Changed 8 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 8 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:3 Changed 8 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 5 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:5 Changed 5 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 5 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 3 years ago by aaugustin

  • Component changed from Core (Other) to Database layer (models, ORM)

comment:8 Changed 3 years ago by akaariai

  • Triage Stage changed from Design decision needed to Accepted

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 timo

  • Needs tests set
  • Patch needs improvement set

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

comment:10 Changed 10 months ago by timgraham

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

comment:11 Changed 10 months ago 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, was primary key).

Tests for m2m.add can be found from Similar tests should be added for .remove(), and for reverse foreign key .add() and .remove().

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