Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#14043 closed Bug (fixed)

Incorrect and/or confusing behaviour with nullable OneToOneField

Reported by: theevilgeek Owned by: Anssi Kääriäinen <akaariai@…>
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)

ticket-14043.patch (1.8 KB ) - added by George Sakkis 13 years ago.
Moved tests from select_related_onetoone to one_to_one_regress

Download all attachments as: .zip

Change History (17)

comment:1 by George Sakkis, 14 years ago

Cc: George Sakkis added
Owner: changed from nobody to George Sakkis
Status: newassigned

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:

In [11]: bob.delete()
UPDATE `lib_soul`
SET `person_id` = NULL
WHERE `id` IN (4)

DELETE
FROM `lib_soul`
WHERE `id` IN (4)

DELETE
FROM `lib_person`
WHERE `id` IN (2)

Why do an update if it is going to be deleted right after ?

comment:2 by George Sakkis, 14 years ago

Owner: George Sakkis removed
Status: assignednew

comment:3 by George Sakkis, 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 Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

@gsakkis - You use the triage stage, not the action. Confusing, I know... :-)

in reply to:  1 comment:5 by George Sakkis, 14 years ago

Triage Stage: AcceptedDesign 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 George Sakkis, 13 years ago

Has patch: set
Triage Stage: Design decision neededReady for checkin

by George Sakkis, 13 years ago

Attachment: ticket-14043.patch added

Moved tests from select_related_onetoone to one_to_one_regress

comment:7 by anonymous, 13 years ago

milestone: 1.3

comment:8 by Arthur Hebert, 13 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 Russell Keith-Magee, 13 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:11 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by Anssi Kääriäinen, 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 Anssi Kääriäinen <akaariai@…>, 11 years ago

Owner: set to Anssi Kääriäinen <akaariai@…>
Resolution: fixed
Status: newclosed

In b53ed351b34918e337cbf26773998dafc6f82f4d:

Fixed #14043 -- Made sure nullable o2o delete works as expected

There was an old complaint about nullable one-to-one field cascading
even when the o2o field was saved to None value before the deletion.
Added an test to verify this doesn't happen.

Also some PEP 8 cleanup.

comment:15 by Andrew Godwin <andrew@…>, 11 years ago

In b773ef8fa0ef09641abf376ca8d4083d8966ec52:

Fixed #14043 -- Made sure nullable o2o delete works as expected

There was an old complaint about nullable one-to-one field cascading
even when the o2o field was saved to None value before the deletion.
Added an test to verify this doesn't happen.

Also some PEP 8 cleanup.

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