Opened 6 years ago

Closed 3 years ago

Last modified 3 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: master
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 6 years ago.
Moved tests from select_related_onetoone to one_to_one_regress

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by George Sakkis

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 Changed 6 years ago by George Sakkis

Owner: George Sakkis deleted
Status: assignednew

comment:3 Changed 6 years ago by George Sakkis

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 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

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

comment:5 in reply to:  1 Changed 6 years ago by George Sakkis

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 Changed 6 years ago by George Sakkis

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

Changed 6 years ago by George Sakkis

Attachment: ticket-14043.patch added

Moved tests from select_related_onetoone to one_to_one_regress

comment:7 Changed 6 years ago by anonymous

milestone: 1.3

comment:8 Changed 6 years ago by Arthur Hebert

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 Changed 6 years ago by Russell Keith-Magee

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 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

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

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 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

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 Changed 3 years ago by Andrew Godwin <andrew@…>

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