#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 by , 10 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 10 years ago
The root cause might be a duplicate of #24712 which was also fixed by the bisected commit.
comment:3 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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 by , 10 years ago
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 by , 10 years ago
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 by , 10 years ago
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 by , 10 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
PR. I left some suggestions for improving the tests.
Fixed on master by b68212f539f206679580afbfd008e7d329c9cd31, but backporting that to 1.8 probably isn't an option.