#14043 closed Bug (fixed)
Incorrect and/or confusing behaviour with nullable OneToOneField
Reported by: | theevilgeek | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | OneToOneField, cascading delete, nullable |
Cc: | George Sakkis | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Attempting to "null" out a nullable OneToOneField before deleting the related object fails to prevent a cascading delete (i.e., both objects are still deleted whereas it seems only the related object ought to be deleted).
Example code:
# Note: using Django trunk ###### MODELS ###### class Person(models.Model): age = models.PositiveIntegerField() def die(self): self.soul.become_ghost() self.delete() class Soul(models.Model): person = models.OneToOneField(Person, null=True) is_alive = models.BooleanField(default=True) def become_ghost(self): self.person = None self.is_alive = False self.save() ###### TESTCASE (INTERACTIVE) ###### # Type a few commands in "python manage.py shell" >>> from app.models import Person, Soul >>> >>> bob = Person.objects.create(age=34) >>> bobs_soul = Soul.objects.create(person=bob) # Let's see what's happening in MySQL (switching programs...) mysql> select * from app_person; +----+-----+ | id | age | +----+-----+ | 2 | 34 | +----+-----+ 1 row in set (0.00 sec) mysql> select * from app_soul; +----+-----------+----------+ | id | person_id | is_alive | +----+-----------+----------+ | 2 | 2 | 1 | +----+-----------+----------+ 1 row in set (0.00 sec) # Okay, that looks good; let's kill him (switching programs again...) >>> bob.die() # Back to MySQL mysql> select * from app_person; Empty set (0.00 sec) mysql> select * from app_soul; Empty set (0.00 sec) ### Huh!??! Why is app_soul being deleted?
Attachments (1)
Change History (17)
follow-up: 5 comment:1 by , 14 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:2 by , 14 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:3 by , 14 years ago
Ugh, selecting "accept ticket" makes me the ticket owner. What's the way to say "I confirm the ticket is legit" without marking me as responsible for it ?
comment:4 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
@gsakkis - You use the triage stage, not the action. Confusing, I know... :-)
comment:5 by , 14 years ago
Triage Stage: | Accepted → Design decision needed |
---|
Replying to gsakkis:
Why do an update if it is going to be deleted right after ?
Ugh, scratch that, I missed the self.save()
in become_ghost()
, that's where the UPDATE comes from; nothing to do with self.delete()
.
Back to the topic, it comes down to Django's indiscriminate ON DELETE CASCADE behavior, regardless of whether the ForeignKey/OneToOneField is nullable. IMO it would make more sense to treat nullable keys as ON DELETE SET NULL but that would most likely be backwards incompatible at this point. Changing to design decision needed, just in case.
comment:6 by , 14 years ago
Has patch: | set |
---|---|
Triage Stage: | Design decision needed → Ready for checkin |
by , 14 years ago
Attachment: | ticket-14043.patch added |
---|
Moved tests from select_related_onetoone to one_to_one_regress
comment:7 by , 14 years ago
milestone: | → 1.3 |
---|
comment:8 by , 14 years ago
Looks like this is solved here. I'm adding a little more information for anyone who is trying to work around this in a pre-1.3 version of Django. The following modification fixed a similar problem on my model.
class Person(models.Model): age = models.PositiveIntegerField() def die(self): self.soul.become_ghost() # Update self here, so that self.soul is unavailalbe in the object passed to delete() # If self is left alone, then self.soul points to a python object representing a relationship # that was deleted from the database in the call to become_ghost(). self = Person.objects.get(pk=self.pk) # OR, the following to work more generally: # self = self.__class__.objects.get(pk=self.pk) self.delete() class Soul(models.Model): person = models.OneToOneField(Person, null=True) is_alive = models.BooleanField(default=True) def become_ghost(self): self.person = None self.is_alive = False self.save()
I'd suggest it as a documentation change for earlier versions, but it's so hack-y.
comment:9 by , 14 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
The provided test case doesn't validate the problem described. If you revert the change to related.py, the test case doesn't fail.
comment:10 by , 13 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:13 by , 11 years ago
I can confirm that this does still happen in master. I tested with this in one_to_one_regress:
def test_nullable_o2o_delete(self): u = UndergroundBar.objects.create(place=self.p1) self.p1.delete() self.assertTrue(UndergroundBar.objects.filter(pk=u.pk).exists()) self.assertIsNone(UndergroundBar.objects.get(pk=u.pk).place)
The attached patch isn't correct at all, it is explicitly breaking the link before delete and then confirming that delete doesn't cascade after that. That isn't the problem, the problem is that OneToOneField that is nullable should be set to None on delete, not cascaded.
comment:14 by , 11 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Confirmed here too, it's definitely non obvious, if not outright a bug. Also the SQL being issued on
bob.delete()
hints that something is fishy:Why do an update if it is going to be deleted right after ?