Django

Code

Ticket #6886 (closed: fixed)

Opened 5 months ago

Last modified 1 month ago

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

Reported by: Andy McCurdy <sedrik@gmail.com> Assigned to: jacob
Milestone: Component: Database wrapper
Version: SVN Keywords: Foreign Key
Cc: Triage Stage: Accepted
Has patch: 0 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

6886.patch (3.8 kB) - added by jacob on 05/31/08 14:21:08.

Change History

03/25/08 15:49:24 changed by PhiR

  • owner changed from nobody to PhiR.
  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

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

05/30/08 20:22:05 changed 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.

05/31/08 14:20:57 changed 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.

05/31/08 14:21:08 changed by jacob

  • attachment 6886.patch added.

06/04/08 19:39:32 changed by jacob

  • status changed from assigned to closed.
  • resolution set to fixed.

(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.

08/01/08 18:17:00 changed 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/Change #6886 (Assigning a Model Instance to a Foreign Key Attribute Doesn't Cache the Instance)




Change Properties
Action