ForeignKey get_db_prep_lookup doesn't work for custom primary key

Patch

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.

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

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.

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.

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.

Patch updated to current trunk

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.

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

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!

Fixed in [8690]