Opened 6 years ago

Closed 6 years ago

#10785 closed (duplicate)

ForeignKey get_db_prep_lookup doesn't work for custom primary key if full object isn't passed

Reported by: Alex Owned by: mtredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: bretthoerner@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

This is a slight variant on #7823. That fixed it so Foo.objects.filter(bar=bar_obj) works, however Foo.objects.filter(bar_id=bar_obj.pk) fails(assuming bar has some sort of custom primary key).

Attachments (2)

custom_pk.diff (3.1 KB) - added by Alex 6 years ago.
Patch that solved the problem. 2 problems with it: 1) The tests are in the wrong place, they're there since that's where the issue was originally reported to me. 2) The tests require py2.5 since that's when the UUID module was added
custom_pk.2.diff (5.3 KB) - added by Alex 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by bretthoerner

  • Cc bretthoerner@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Changed 6 years ago by Alex

Patch that solved the problem. 2 problems with it: 1) The tests are in the wrong place, they're there since that's where the issue was originally reported to me. 2) The tests require py2.5 since that's when the UUID module was added

comment:2 Changed 6 years ago by Alex

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by Alex

That second query should be Foo.objects.filter(bar=bar_obj.pk) .

Changed 6 years ago by Alex

comment:4 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Patch needs improvement set
  • Status changed from new to assigned

Are you intending to fix the tests?

I'll incorporate fixing the main problem into the work I'm doing for #10243 -- get_db_prep_lookup() on related fields is broken in a couple of interesting ways right now and needs heavy hammer surgery that I'm in the middle of. But a test case that actually demonstrates the problem is going to save time, so a proper patch against the test suite would be appreciated.

comment:5 Changed 6 years ago by Alex

I just fixed up those tests, I originally uploaded the first version imperfect because I wanted to get it up and for the gentleman who reported the bug to me to be able to see it easily.

comment:6 Changed 6 years ago by russellm

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

(In [10952]) Fixed #10785 -- Corrected a case for foreign key lookup where the related object is a custom primary key. Thanks to Alex Gaynor for the patch.

comment:7 Changed 6 years ago by russellm

For the record - I've spoken with Malcolm about the underlying problem here, and we've opted to paper over the immediate problem in the interests of getting v1.1 out the door. We will need to revisit get_db_prep_lookup in the 1.2 timeframe.

comment:8 Changed 6 years ago by russellm

(In [10953]) [1.0.X] Fixed #10785 -- Corrected a case for foreign key lookup where the related object is a custom primary key. Thanks to Alex Gaynor for the patch.

Merge of r10952 from trunk.

comment:9 Changed 6 years ago by ccahoon

(In [11000]) Fixed #10785 -- Corrected a case for foreign key lookup where the related object is a custom primary key. Thanks to Alex Gaynor for the patch.

comment:10 Changed 6 years ago by russellm

(In [11007]) Fixed #11311 -- Reverted [10952], Refs #10785. Changeset [10952] caused problems with m2m relations between models that had non-integer primary keys. Thanks to Ronny for the report and test case.

comment:11 Changed 6 years ago by russellm

(In [11008]) [1.0.X] Fixed #11311 -- Reverted [10952], Refs #10785. Changeset [10952] caused problems with m2m relations between models that had non-integer primary keys. Thanks to Ronny for the report and test case.

comment:12 Changed 6 years ago by russellm

  • milestone 1.1 deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened

As noted in the previous two commit comments, this change was reverted due to introducing some other problems. Those problems were much more mainstream than this one, so fixing those issues takes priority. The test case has been left in place (mostly because it partly served to test the behavior introduced in #7823), but is commented out for now.

Deeper investigation has revealed that this isn't something we can paper over - the problem is actually quite deep. Note to self: Listen to Malcolm. :-)

Given that this is an edge case, there is a workaround (use the object instead of the PK), and the actual solution will be very complex (and will potentially introduce all sorts of other #11311-esque breakages) I'm going to defer this from the v1.1 milestone.

comment:13 Changed 6 years ago by russellm

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

Since this is a smaller part of a much bigger problem, I'm going to close this as a duplicate of a new ticket (#11319) that covers the bigger issue.

Note: See TracTickets for help on using tickets.
Back to Top