Add a public API to unregister model field lookups

[Tim Graham]

Motivated by the ongoing work to build a MongoDB backend, I'm interested in writing code like this:
EmbeddedModelField._unregister_lookup(GreaterThanOrEqual)
(a lot of the built-in lookups aren't applicable to EmbeddedModelField... I think we only want exact.)

As far as I can tell, Field._unregister_lookup() isn't a public API ​to concerns about thread-safety.

Simon Charette provided this guidance:

As far as I know we don't run tests in a multi-threaded environment (it's multi-process based) so I'm not sure what this comment is referring to.

If you implement this logic I suspect you'll need to use a notion of sentinel / tombstone to denote unregistration to work like you intend as GreaterThanOrEqual is registered for Field as well.

The following code might explain why the current situation is broken:

from django.db.models import Field
from django.db.models.lookups import GreaterThanOrEqual

class EmbeddedModelField(Field):
   ...

>>> EmbeddedModelField._unregister_lookup(GreaterThanOrEqual)
>>> assert "gte" not in EmbeddedModelField.get_lookups()
>>> assert "gte" in Field.get_lookups()

I think that's what the "thread-safe" mention was referring to, it was more along the lines that _unregister_lookup() is broken if it doesn't have a corresponding register_lookup() call to restore its state because the unregistration logic doesn't ensure to create a per-class override prior to deletion.

A potential solution to this problem would be to avoid any form of deletion of registered lookups and use a UnregisteredLookup sentinel (or simply None) instead:

django/db/models/query_utils.py

@@ -337 +337 @@
     register_class_lookup = classmethod(register_class_lookup)
 
     def _unregister_class_lookup(cls, lookup, lookup_name=None):
-        """
-        Remove given lookup from cls lookups. For use in tests only as it's
-        not thread-safe.
-        """
         if lookup_name is None:
             lookup_name = lookup.lookup_name
-        del cls.class_lookups[lookup_name]
+        if "class_lookups" not in cls.__dict__:
+            cls.class_lookups = {}
+        cls.class_lookups[lookup_name] = None
         cls._clear_cached_class_lookups()
 
     def _unregister_instance_lookup(self, lookup, lookup_name=None):
@@ -353 +351 @@
         """
         if lookup_name is None:
             lookup_name = lookup.lookup_name
-        del self.instance_lookups[lookup_name]
+        if "instance_lookups" not in self.__dict__:
+            self.instance_lookups = {}
+        self.instance_lookups[lookup_name] = None
 
     _unregister_lookup = class_or_instance_method(
         _unregister_class_lookup, _unregister_instance_lookup

None is a suitable sentinel as both get_lookup and get_transform treat it as a missing entry.

[Simon Charette]

Given there are few cases where ​we could use this internally and a registration interface already exists I think it's work adding as it's quite hard to get unregistration right now that we have class and instance level lookups support.

[Ahmed Ibrahim]

Is this still going on, or should I take over?

[Tim Graham]

I'll deassign the ticket if I'm not going to return to it. It's not an easy task anyway.