Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#7823 closed (fixed)

ForeignKey get_db_prep_lookup doesn't work for custom primary key

Reported by: Ivan Sagalaev Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


If a parent model has a user-defined field as a primary key:

class Parent(models.Model):
    user_id = CutomUserField(primary_key=True)

class Child(models.Model):
    parent = models.ForeignKey(Parent)

then this query will fail:

p = Parent(...)

This happens because get_db_prep_lookup doesn't get called on p.user_id and its value (which is a custom user class) gets passed as an sql parameter into db.

Patch follows.

P.S. A bug is very practical: we have custom non-model user objects instead of contrib.auth.models.User and can't reference them from models with custom UserField because many queries using those models would break.

Attachments (3)

7823.diff (994 bytes) - added by Ivan Sagalaev 9 years ago.
7823.2.diff (1.4 KB) - added by Ivan Sagalaev 8 years ago.
Patch updated to current trunk
7823.2.experimental.diff (1.2 KB) - added by Ivan Sagalaev 8 years ago.
Same as 7823.2.diff but more elegant and may be less robust?

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by Ivan Sagalaev

Attachment: 7823.diff added


comment:1 Changed 9 years ago by Ivan Sagalaev

BTW... Looks like I'm missing something though. Field.get_db_prep_lookup returns a list for exact lookup. However documentaion ( shows an example where get_db_prep_lookup returns single value. This is why there's a check for isinstance(v, list) in the patch.

comment:2 Changed 9 years ago by Ivan Sagalaev

Has patch: set

comment:3 Changed 8 years ago by Ivan Sagalaev

Malcolm, you were talking about some better way to do this and I'd like to shae what I was trying also.

First, to get rid of isinstance(..., list) I think we should change all the get_db_prep_lookup's to return lists. Currently it's not the case even for built-in fields (see DateField, DecimalField for example).

Then if the type returning will be uniform the inner loop would could be changed for simpler recursion. Something like this:

def pk_trace(value):
        v, field = getattr(value,
        if lookup_type in ['in', 'range']:
            v = [v]
        return field.get_db_prep_lookup(lookup_type, v)[0]
    except AttributeError:
        return value

comment:4 Changed 8 years ago by Eric Holscher

milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:5 Changed 8 years ago by Malcolm Tredinnick

I was just about to commit this and then I ran the full test suite. And it failed. The patch has problems in, at least, the many_to_one, many_to_many and queries tests. If somebody can find a robust fix quickly, I'll commit this before 1.0, since we support custom field subclasses and this is necessary for that. But it's not going to hold up 1.0 if something doesn't happen fast.

comment:6 Changed 8 years ago by Ivan Sagalaev

I remember tests were passing when I made the patch so I think it's not something conceptually broken here. I'll look into it today.

comment:7 Changed 8 years ago by Malcolm Tredinnick

Thanks, Ivan. It may be failing now as a result of get_db_prep_lookup() having been refactored a while back (mostly into using get_db_prep_value() wherever possible. We might have to add back one or two get_db_prep_lookup() overrides, like I did in [8526]. That's just a guess, though, since I haven't looked too deeply at this.

Changed 8 years ago by Ivan Sagalaev

Attachment: 7823.2.diff added

Patch updated to current trunk

comment:8 Changed 8 years ago by Ivan Sagalaev

Attached new patch that doesn't break tests. Unfortuantely various list/not list checks should stay intact because of the nature of get_db_prep_lookup that accepts both lists and single values. I couldn't make it actually work in a more general way so decided to leave it until some lucky day when I'll have another epiphany.

Changed 8 years ago by Ivan Sagalaev

Attachment: 7823.2.experimental.diff added

Same as 7823.2.diff but more elegant and may be less robust?

comment:9 Changed 8 years ago by Ivan Sagalaev

when I'll have another epiphany

Don't know if it was it but I've made a patch without ugly 'isinstance' checks that works and passes tests. However! There are two gotchas.

  1. I took liberty in calling get_db_prep_lookup on pk fields with 'exact' lookup_type always. This means that when doing filter(parent__in=[parent1, parent2, parent3] each pk value from a list would be looked up with get_db_prep_lookup('exact', v) instead of get_db_prep_lookup('in', [v]). I think that it's not that important because I can't invent a real use-case where 'in' lookups would yield essentially different results than a list of 'exact's
  1. This patch actually relies on the fact that 'exact' lookups return a list. It is now consistent throughout Django after introducing get_db_prep_value and general implementation of lookup logic. But we may break some user code that was returning scalar values on exact lookups. But I actually think it's a good thing because inconsistencies are bad and now it's better time to break it than after 1.0

Anyway, Malcolm, the choice is yours!

comment:10 Changed 8 years ago by Malcolm Tredinnick

Fixed in [8690]

comment:11 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

comment:12 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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