Code

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6886 closed (fixed)

Assigning a Model Instance to a Foreign Key Attribute Doesn't Cache the Instance

Reported by: Andy McCurdy <sedrik@…> Owned by: jacob
Component: Database layer (models, ORM) Version: master
Severity: Keywords: Foreign Key
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently, when assigning a model instance to a forkeign key attribute doesn't cache the model instance being assigned. Rather, it simply deletes the prior cached attribute.

This causes two undesired behaviors:

  • If code later in the execution process references the FK attribute rather than the model instance, a sql query is run to fetch the result, because it's not cached.
  • It makes race conditions easy in post-save signals, where in many cases it's easy to end up with multiple instances of the same object.

Oddly enough, it seems GenericForeignKey gets this right. On line# 92 of django/contrib/contenttypes/generic.py, the instance being assigned gets cached on the model. However, on line# 225 of django/db/models/fields/related.py, the cached attribute simple gets deleted.

I believe line# 225 should be changed to save the instance being assigned on the cached attribute.

Attachments (1)

6886.patch (3.8 KB) - added by jacob 6 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 6 years ago by PhiR

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to PhiR
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

See #17 and #5514. This is not strictly a duplicate but will be solved when those are closed.

comment:2 Changed 6 years ago by jacob

  • Owner changed from PhiR to jacob
  • Status changed from new to assigned

As Andy says, the fix is easy (see the patch I just attached). However, I'm not 100% sure of the ramifications, so I'm going to leave this open for the moment and see if I can't get someone else to take a peek and figure out if this is correct or just papering over a bigger issue.

comment:3 Changed 6 years ago by jacob

After discussion on django-dev, I've made a patch that somewhat changes FK assignment to really fix this issue. Specifically:

  • Raise a ValueError if you try to assign an object of the wrong type to a FK.
  • Raise a ValueError if you try to assign None to an FK that's null=False.
  • Cache the value in __set__.

See the tests is the patch for examples.

Again, This is slightly backwards-incompatible, but only the second point -- dealing with None -- is likely to bite anyone. And then only if they're assigning None to a ForeignKey(null=False). I think it's a neccisary breakage, but I'm not committing just yet to let anyone with complaints speak up.

Changed 6 years ago by jacob

comment:4 Changed 6 years ago by jacob

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

(In [7574]) Fixed #6886: Tightened up ForeignKey and OneToOne field assignment. Specifically:

  • Raise a ValueError if you try to assign the wrong type of object.
  • Raise a ValueError if you try to assign None to a field not specified with null=True.
  • Cache the set value at set time instead of just at lookup time.

This is a slightly backwards-incompatible change; see BackwardsIncompatibleChanges for more details.

comment:5 Changed 6 years ago by gwilson

(In [8185]) Fixed #8070 -- Cache related objects passed to Model init as keyword arguments. Also:

  • Model init no longer performs a database query to refetch the related objects it is passed.
  • Model init now caches unsaved related objects correctly, too. (Previously, accessing the field would raise DoesNotExist error for null=False fields.)
  • Added tests for assigning None to null=True ForeignKey fields (refs #6886).

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.