Code

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#9023 closed (fixed)

OneToOneField delete() problem

Reported by: TheShark Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: OneToOneField delete cascade
Cc: juriejanbotha@…, david Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Accessing the automatic reverse relationship on an instance (w.sprocket in the following example) causes some type of caching to happen which results in my Sprocket getting caught up in a cascade delete() of a Widget erroneously. The following example works without the print statement, and my Sprocket s exists at the end. If I 'print w.sprocket' before clearing the relationship, my Sprocket gets deleted along with my Widget when it shouldn't.

class Widget(models.Model):
    name = models.CharField(max_length=10)

    def __unicode__(self):
        return u'%s(%s)'%(self.name,self.pk)

class Sprocket(models.Model):
    name = models.CharField(max_length=10)
    w = models.OneToOneField(Widget, null=True, blank=True)

    def __unicode__(self):
        return u'%s(%s)'%(self.name,self.pk)

And the following sequence of operations:

w=Widget(name="Some Widget")
w.save()
s=Sprocket(name="Some Sprocket")
s.w=w
s.save()

assert Widget.objects.all().count()==1
assert Sprocket.objects.all().count()==1

#print w.sprocket
s.w=None
s.save()
w.delete()

assert Widget.objects.all().count()==0
assert Sprocket.objects.all().count()==1

Attachments (3)

onetoone_cache_clear.diff (2.2 KB) - added by juriejan 5 years ago.
First sloppy version of the patch (needs feedback)
9023-test.diff (2.7 KB) - added by adurdin 5 years ago.
Regression test to exemplify the bug.
9023.diff (3.3 KB) - added by jacob 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by kratorius

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by juriejan

First sloppy version of the patch (needs feedback)

comment:3 Changed 5 years ago by juriejan

  • Cc juriejanbotha@…, oliver@…, cmlenz@…, gary.wilson@…, mhf@…, tom@…, django@…, brosner@…, tom85@…, dan.popovici@…, django@…, lnielsen@…, to, patrick.lauber@…, mpjung@…, django@…, remco@…, barbuzaster@…, flavio.curella@…, semente@…, stephane@… added
  • Has patch set
  • Needs tests set
  • Owner changed from nobody to juriejan
  • Patch needs improvement set

I've created a sloppy patch to fix this issue. It's my first whack at working on the DB layer (or making a serious contribution) so please bear with me.

I figured that we need a reference to the SingleRelatedObjectDescriptor that is related to the ReverseSingleRelatedObjectDescriptor for the OnToOne field so that we can access it from the 'set' method of the ReverseSingleRelatedObjectDescriptor to remove the cache from the object that the relation previously pointed to. So I added the 'related_object_descriptor' attribute to the RelatedField object. Not sure if I should rather add the attribute in a more appropriate place, like in RelatedObject or ReverseSingleRelatedObjectDescriptor itself.

comment:4 Changed 5 years ago by obeattie

  • Cc oliver@… removed

I didn't subscribe to this :\

comment:5 Changed 5 years ago by juriejan

  • Cc cmlenz@…, gary.wilson@…, mhf@…, tom@…, django@…, brosner@…, tom85@…, dan.popovici@…, django@…, lnielsen@…, to, patrick.lauber@…, mpjung@…, django@…, remco@…, barbuzaster@…, flavio.curella@…, semente@…, stephane@… removed

comment:6 Changed 5 years ago by juriejan

  • Version changed from 1.0 to SVN

Changed 5 years ago by adurdin

Regression test to exemplify the bug.

comment:7 Changed 5 years ago by adurdin

  • Needs tests unset

Changed 5 years ago by jacob

comment:8 Changed 5 years ago by jacob

Working patch attached. I'm not sure I'm 100% happy with the approach: in some cases it can make obj.o2o = None perform a database hit, which is annoying and counter-intuitive.

comment:9 Changed 5 years ago by Alex

We can remove the extra SQL query by essentially unrolling the getattr call. We only need to update the related object if the obj is already loaded, so can check this by poking around in dict.

comment:10 Changed 5 years ago by juriejan

  • Owner changed from juriejan to nobody

comment:11 Changed 5 years ago by david

  • Cc david added

comment:12 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [11009]) Fixed #9023 -- Corrected a problem where cached attribute values would cause a delete to cascade to a related object even when the relationship had been set to None. Thanks to TheShark for the report and test case, and to juriejan and Jacob for their work on the patch.

comment:13 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.