Code

Opened 3 years ago

Closed 18 months ago

#15146 closed Bug (duplicate)

Reverse relations for unsaved objects include objects with NULL foreign key

Reported by: treyh Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Model with parent relation and children reverse relation:

class TestModel(models.Model):
    parent = models.ForeignKey('self', blank=True, null=True, related_name='children')

Bug found while using model:

t = TestModel()
t.save()
u = TestModel()
u.children.all() # Incorrectly returns: [<TestModel: TestModel object>]
u.save()
u.children.all() # Correctly returns: []

Attachments (3)

tests.diff (1.6 KB) - added by elbarto 3 years ago.
Tests added.
tests.2.diff (1.6 KB) - added by elbarto 3 years ago.
New tests added.
patch.diff (926 bytes) - added by elbarto 3 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by treyh

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Reverse relations include unsaved objects (NULL id) to Reverse relations include objects with NULL foreign key

comment:2 Changed 3 years ago by treyh

  • Summary changed from Reverse relations include objects with NULL foreign key to Reverse relations for unsaved objects include objects with NULL foreign key

comment:3 Changed 3 years ago by russellm

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Triage Stage changed from Unreviewed to Accepted

This is an edge case that exists when an object has been created, but not saved; at that point, the primary key of the object is NULL, so the reverse query does a lookup with that value.

The fix here will be to prevent related object lookups when pk=None.

comment:4 Changed 3 years ago by elbarto

  • Owner changed from nobody to elbarto

Changed 3 years ago by elbarto

Tests added.

Changed 3 years ago by elbarto

New tests added.

Changed 3 years ago by elbarto

comment:5 Changed 3 years ago by elbarto

  • Has patch set

comment:6 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 3 years ago by julien

  • Patch needs improvement set

Could you combine the fix and tests into one single diff patch?

comment:8 Changed 3 years ago by elbarto

  • Easy pickings unset
  • Owner changed from elbarto to nobody

comment:9 Changed 2 years ago by lukeplant

  • UI/UX unset

Ticket #17541 is a duplicate. I don't know which one to close at this point.

comment:10 Changed 18 months ago by stavros

As I said on the other ticket, please upgrade the severity of this. It has just caused entirely sane-looking code to delete over half our database rows. This needs to be fixed ASAP, the patch has been submitted over two years ago.

comment:11 Changed 18 months ago by russellm

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

Closing this one as a duplicate of #17541. It doesn't really matter which one we leave open, as long as there's only one ticket per problem.

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.