#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.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/01070196fe4e2d00-aa110f59-09a5-443c-8a71-d3c0988a5406-000000%40eu-central-1.amazonses.com.
