ForeignKey get_db_prep_lookup doesn't work for custom primary key

[Ivan Sagalaev]

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.

[Ivan Sagalaev]

Patch

[Ivan Sagalaev]

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.

[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):
    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

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

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

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

[Ivan Sagalaev]

Patch updated to current trunk

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

[Ivan Sagalaev]

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

[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

2. 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!

[Malcolm Tredinnick]

Fixed in [8690]

[Jacob]

Milestone 1.0 deleted