Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#24698 closed Bug (fixed)

Relation clear fails silently when using OneToOneField+UUIDField

Reported by: Julian Bez Owned by: Abhaya Agarwal
Component: Database layer (models, ORM) Version: 1.8
Severity: Release blocker Keywords: uuidfield onetoonefield manytomanyfield
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have the following simplified model:

class Lead(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    name = models.CharField(max_length=255)


class LeadInfo(models.Model):
    lead = models.OneToOneField(Lead, primary_key=True)
    url = models.URLField("Blog URL", blank=True)
    topics = models.ManyToManyField("Topic", blank=True)


class Topic(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    name = models.CharField(max_length=255)

There seems to be a problem with the clear() on the ManyToManyField. Django tries to delete entries, but uses the uuid with hyphens, although it previously saved the uuid without. The clear does nothing, and the next add will then fail. (This becomes a problem in ModelForms, as they clear relations before updating). This is on MySQL.

>>> l = Lead.objects.create(name="Lead 1")
>>> topic = Topic.objects.create(name="Topic 1")
>>> info = LeadInfo.objects.create(lead=l, url="bla")
>>> info.topics.add(topic)
>>> info.topics.clear()
>>> info.topics.add(topic)
IntegrityError: (1062, "Duplicate entry '5357d002c2d74e1899b845a596e10cd8-15572c1b20234a6caa917cb200a2436' for key 'leadinfo_id'")

I've traced it down to Django making a wrong query for the delete, but I cannot tell why the hyphens end up in there, whereas in the INSERT it does them right:

DELETE FROM `uuidtest_leadinfo_topics` WHERE `uuidtest_leadinfo_topics`.`leadinfo_id` = '5357d002-c2d7-4e18-99b8-45a596e10cd8'";
INSERT INTO `uuidtest_leadinfo_topics` (`leadinfo_id`, `topic_id`) VALUES ('5357d002c2d74e1899b845a596e10cd8', '15572c1b20234a6caa917cb200a2436b');

Change History (9)

comment:1 Changed 3 years ago by Tim Graham

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Fixed on master by b68212f539f206679580afbfd008e7d329c9cd31, but backporting that to 1.8 probably isn't an option.

comment:2 Changed 3 years ago by Tim Graham

The root cause might be a duplicate of #24712 which was also fixed by the bisected commit.

comment:3 Changed 3 years ago by Abhaya Agarwal

Owner: changed from nobody to Abhaya Agarwal
Status: newassigned

The root cause for both this and #24712 is indeed same: missing get_db_prep_value on ForeignKey which was added in the bisected commit. Working on a patch.

comment:4 Changed 3 years ago by Abhaya Agarwal

Thinking about it, since a ForeignKey can refer to any unique field on the related model, this change is likely to be backward incompatible. The behavior will change in case of all the fields which implement a get_db_prep_value (ex: DateTimeField). Although it doesn't seem a common use case to have foreign keys pointing to fields other than IntegerField and UUIDField.

The patch is simple but can it be applied in 1.8.x branch?

comment:5 Changed 3 years ago by Abhaya Agarwal

Other option is to patch the get_db_prep_lookup on the Lookup class and do a special case fix for related fields pointing to UUIDField. That will not change the behavior for other fields. In any case, the proper fix is already in place in master for future releases.

comment:6 Changed 3 years ago by Anssi Kääriäinen

It seems clear that not calling get_db_prep_value is a bug. While it might be backwards incompatible, we don't guarantee backwards incompatibility for bugs. So, unless it is clear that this is likely to break existing installations running 1.8, then I think we can go ahead and fix this for all field types.

It would be interesting to find cases which this change breaks. Can you show something involving a date field which worked before, but doesn't work after your patch?

Also, if you have a patch to solve this, please do make a pull request. If you don't think your PR is actually ready to be merged, you can add [WIP] to the title of the pull request so that reviewers know this isn't ready for commit yet.

comment:7 Changed 3 years ago by Tim Graham

Has patch: set
Patch needs improvement: set

PR. I left some suggestions for improving the tests.

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 290c9d6:

[1.8.x] Fixed #24698, #24712 -- Added ForeignKey.get_db_prep_value()

Fixed crashes with ForeignKey to UUIDField and inheritance with UUIDField
primary keys.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 7c7b855:

[1.8.x] Refs #24698, #24712 -- Forwardported ForeignKey.get_db_prep_value() test and release notes.

Fixed in master by b68212f539f206679580afbfd008e7d329c9cd31.

Forwardport of 290c9d665490d80b0a1b648fb022190d7dc375fc from stable/1.8.x

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