Opened 5 years ago

Closed 2 years ago

Last modified 2 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: gsakkis 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 gsakkis 5 years ago.
Moved tests from select_related_onetoone to one_to_one_regress

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 5 years ago by gsakkis

  • Cc gsakkis added
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to gsakkis
  • Patch needs improvement unset
  • Status changed from new to assigned

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 5 years ago by gsakkis

  • Owner gsakkis deleted
  • Status changed from assigned to new

comment:3 Changed 5 years ago by gsakkis

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 5 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 in reply to: ↑ 1 Changed 5 years ago by gsakkis

  • Triage Stage changed from Accepted to 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 Changed 5 years ago by gsakkis

  • Has patch set
  • Triage Stage changed from Design decision needed to Ready for checkin

Changed 5 years ago by gsakkis

Moved tests from select_related_onetoone to one_to_one_regress

comment:7 Changed 5 years ago by anonymous

  • milestone set to 1.3

comment:8 Changed 5 years ago by ahebert

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 4 years ago by russellm

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 2 years ago by akaariai

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

  • Owner set to Anssi Kääriäinen <akaariai@…>
  • Resolution set to fixed
  • Status changed from new to closed

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 2 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