Opened 16 years ago

Closed 16 years ago

Last modified 16 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: dev
Severity: Keywords: Foreign Key
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

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 16 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 by Philippe Raoult, 16 years ago

Owner: changed from nobody to Philippe Raoult
Triage Stage: UnreviewedAccepted

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

comment:2 by Jacob, 16 years ago

Owner: changed from Philippe Raoult to Jacob
Status: newassigned

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 by Jacob, 16 years ago

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.

by Jacob, 16 years ago

Attachment: 6886.patch added

comment:4 by Jacob, 16 years ago

Resolution: fixed
Status: assignedclosed

(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 by Gary Wilson, 16 years ago

(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).
Note: See TracTickets for help on using tickets.
Back to Top