Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#7823 closed (fixed)

ForeignKey get_db_prep_lookup doesn't work for custom primary key

Reported by: isagalaev Owned by: mtredinnick
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:

Description

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(...)
Child.objects.filter(parent=p)

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 isagalaev 7 years ago.
Patch
7823.2.diff (1.4 KB) - added by isagalaev 7 years ago.
Patch updated to current trunk
7823.2.experimental.diff (1.2 KB) - added by isagalaev 7 years ago.
Same as 7823.2.diff but more elegant and may be less robust?

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by isagalaev

Patch

comment:1 Changed 7 years ago by isagalaev

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

BTW... Looks like I'm missing something though. Field.get_db_prep_lookup returns a list for exact lookup. However documentaion (http://www.djangoproject.com/documentation/custom_model_fields/) 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 7 years ago by isagalaev

  • Has patch set

comment:3 Changed 7 years ago by isagalaev

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):
    try:
        v, field = getattr(value, value._meta.pk.name)
        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 7 years ago by ericholscher

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 7 years ago by mtredinnick

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 7 years ago by isagalaev

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 7 years ago by mtredinnick

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 7 years ago by isagalaev

Patch updated to current trunk

comment:8 Changed 7 years ago by isagalaev

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 7 years ago by isagalaev

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

comment:9 Changed 7 years ago by isagalaev

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 7 years ago by mtredinnick

Fixed in [8690]

comment:11 Changed 7 years ago by mtredinnick

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

comment:12 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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