[Django] #36415: Add a public API to unregister model field lookups

[Django]

#36415: Add a public API to unregister model field lookups
-------------------------------------+-------------------------------------
Reporter: Tim | Owner: Tim Graham
Graham |
Type: New | Status: assigned
feature |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
[https://github.com/django/django/blob/e44e8327d3d88d86895735c0e427102063ff5b55/django/db/models/query_utils.py#L339-L361due
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:

{{{#!python
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:
{{{#!diff
diff --git a/django/db/models/query_utils.py
b/django/db/models/query_utils.py
index f0ae810f47..4df1324825 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -337,13 +337,11 @@ def register_instance_lookup(self, lookup,
lookup_name=None):
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,7 +351,9 @@ def _unregister_instance_lookup(self, lookup,
lookup_name=None):
"""
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36415>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36415: Add a public API to unregister model field lookups
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: Tim
| Graham
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

Given there are few cases where
[https://github.com/search?q=repo%3Adjango%2Fdjango+_unregister_lookup+path%3Atests&type=code
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36415#comment:1>

[Django]

#36415: Add a public API to unregister model field lookups
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ahmed Ibrahim):

Is this still going on, or should I take over?
--
Ticket URL: <https://code.djangoproject.com/ticket/36415#comment:2>

[Django]

#36415: Add a public API to unregister model field lookups
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
| Graham
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I'll deassign the ticket if I'm not going to return to it. It's not an
easy task anyway.
--
Ticket URL: <https://code.djangoproject.com/ticket/36415#comment:3>