Opened 8 years ago

Closed 7 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 Gaynor Owned by: Malcolm Tredinnick
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 Gaynor 8 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 Gaynor 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by Brett Hoerner

Cc: bretthoerner@… added

Changed 8 years ago by Alex Gaynor

Attachment: custom_pk.diff added

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 8 years ago by Alex Gaynor

Has patch: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 8 years ago by Alex Gaynor

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

Changed 8 years ago by Alex Gaynor

Attachment: custom_pk.2.diff added

comment:4 Changed 8 years ago by Malcolm Tredinnick

Owner: changed from nobody to Malcolm Tredinnick
Patch needs improvement: set
Status: newassigned

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 8 years ago by Alex Gaynor

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 7 years ago by Russell Keith-Magee

Resolution: fixed
Status: assignedclosed

(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 7 years ago by Russell Keith-Magee

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 7 years ago by Russell Keith-Magee

(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 7 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 7 years ago by Russell Keith-Magee

(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 7 years ago by Russell Keith-Magee

(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 7 years ago by Russell Keith-Magee

milestone: 1.1
Resolution: fixed
Status: closedreopened

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 7 years ago by Russell Keith-Magee

Resolution: duplicate
Status: reopenedclosed

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