Fixed #22936 -- Obsoleted Field.get_prep_lookup()/get_db_prep_lookup()

[claudep]

https://code.djangoproject.com/ticket/22936

[timgraham]

Great, would it be much trouble to break the work into several commits? For example, I envisioned that moving the BooleanField stuff to the admin listfilter could stand on its own. I think it could make review easier as well as bisecting possible regressions or untested behaviors that might change with a big patch like this. f you feel there's no value in this, I won't insist.

I believe we need a deprecation path for custom fields that use get_prep_lookup.

[claudep]

I admit the boolean filter stuff could stand on its own, but apart from that, it's a bit difficult to split because changes are closely related.

For the deprecation path, if a custom field is using get_prep_lookup it will simply not be called any longer, and I think that this will be fine most of the time. I have no precise idea about a use case where it would fail.

[timgraham]

Okay, let's see what the ORM guys think. I also asked on django-developers if anyone who implements these methods could test this PR.

[akaariai]

The reason get_prep_lookup() was left as it is was support for old-style lookups. +1 to the change, much cleaner this way.

[timgraham]

Should we add anything about the new logic to docs/ref/models/lookups.txt? How about a mention in the release notes? I'll be surprised if no projects need to make some changes to adapt for this change. I tried to test with this one: https://github.com/potatolondon/djangae/blob/f8429792aea7248f3090ada910db4f82d57bf873/djangae/fields/iterable.py#L31-L49 but couldn't get that test suite easily running with Django master.

[akaariai]

Hmmh, what if a field requires special preparation for one of the built-in lookups?

I guess in this rare case one could add a subclass of the built-in lookup with custom preparation code, and register that as an overriding lookup for the field. Something like:

class MyField(Field):
    ...

class MyFieldExact(Exact):
    def get_prep_lookup(self):
          return do_custom_stuff_for_myfield()

MyField.register_lookup(MyFieldExact)

Do we need to document something like this as an upgrade path?

[timgraham]

Sounds good to me.

[timgraham]

I think this test is obsolete now.

[timgraham]

I've finished removing most of the internal usage of get_db_prep_lookup() in #6493. I think that's ready for merge, then you can rebase your patch and merge it. At that point we'd be ready to tackle removal or deprecation of get_db_prep_lookup(), I think.

[claudep]

The plan looks good.

[claudep]

Tim, feel free to take the patch to complete/polish it.

[timgraham]

Hopefully claudep#5 takes care of the remaining needs.

[goblinJoel]

@claudep and @akaariai , I apologize for commenting on an old PR, but I'm finally working on migrating from 1.9 to 1.10 now. Do I understand correctly that lookups will call custom fields' get_prep_value() methods now instead of get_prep_lookup()? There was no conversation on the issue, the release notes seem to assume a different use case than mine, and I can't find any "removed in Django 1.10" or similar notes in the documentation on this.

My uses are:

Custom field 1: get_prep_lookup() just calls value.to_db_value() if the value is an object of the type the field is meant to store (as opposed to a string, etc.), then calls super.

Custom field 2: get_prep_lookup() grabs the attribute that we actually want to serialize (if present on the value, which it would be if the value is an object of the type the field is meant to store as opposed to a string, etc.), then calls super.

Do I need to do anything aside from delete the get_prep_lookup()s? In both of my cases, get_prep_value() would produce the same result.

Apologies if this is the wrong place to ask this question or if I missed existing documentation on this.

[timgraham]

It's not really the best place to ask. You could use our support channels. Also, Django is open source so you can inspect how it works. :-)

[goblinJoel]

I'll look at those support channels, thanks. Inspecting the code was how I got as far as thinking they call get_prep_value() now, but I wasn't sure if I was looking at the right stuff.