Opened 3 years ago
Closed 3 years ago
#33626 closed Cleanup/optimization (fixed)
RegisterLookupMixin._unregister_lookup() should clear the lookup cache.
Reported by: | Himanshu Balasamanta | Owned by: | Himanshu Balasamanta |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.0 |
Severity: | Normal | Keywords: | Lookups, caching, ORM |
Cc: | Carlton Gibson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
In current source code, in the _unregister_lookup method, https://github.com/django/django/blame/main/django/db/models/query_utils.py#L212, the cache is not cleared, which should be done, as it is done in register_lookup, https://github.com/django/django/blame/main/django/db/models/query_utils.py#L202. Corresponding to this change, minor changes need to be brought in the schema.tests.SchemaTests.test_func_unique_constraint_lookups test.
The PR generated is https://github.com/django/django/pull/15569
Change History (10)
comment:2 by , 3 years ago
Description: | modified (diff) |
---|
comment:3 by , 3 years ago
Replying to Carlton Gibson:
Hi Himanshu. This may be right, yes — I need to have a sit-down and play with it.
Main question: Are you able to put together an example case where the wrong result arrises?
Hi Carlton.
I have opened the PR https://github.com/django/django/pull/15569, and have also modified the test that was supposed to throw error( schema.tests.SchemaTests.test_func_unique_constraint_lookups ).
There is no test that checks if the lookup stays in cache after unregistering it. In my PR, I have added an assert statement to check it in custom_lookups.tests.LookupTests.test_lookups_caching test. Running the test without clearing cache from _unregister_lookup will fail.
- I was looking at PR #6906 which added the cache clearing.
- Also noting the
For use in tests only...
in the_unregister_lookup
docstring. So this would show up in a test inter-dependency...? 🤔
The cache stays between tests, so you are likely to get different results running tests independently and running all tests in a module. (PS: I faced this problem earlier running tests individually and together gave different results.)
comment:4 by , 3 years ago
Description: | modified (diff) |
---|
Replying to Carlton Gibson:
Hi Himanshu. This may be right, yes — I need to have a sit-down and play with it.
Main question: Are you able to put together an example case where the wrong result arrises?
- I was looking at PR #6906 which added the cache clearing.
- Also noting the
For use in tests only...
in the_unregister_lookup
docstring. So this would show up in a test inter-dependency...? 🤔
comment:5 by , 3 years ago
Description: | modified (diff) |
---|
comment:6 by , 3 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Hi Himanshu. Thanks for updating with the PR. I'll accept to review.
comment:7 by , 3 years ago
Summary: | django.db.models.query_utils.RegisterLookupMixin._unregister_lookup method also should clear the cache stored for django.db.models.query_utils.RegisterLookupMixin.get_lookups → RegisterLookupMixin._unregister_lookup() should clear the lookup cache. |
---|
comment:8 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Type: | Bug → Cleanup/optimization |
comment:9 by , 3 years ago
Component: | Error reporting → Database layer (models, ORM) |
---|
Hi Himanshu. This may be right, yes — I need to have a sit-down and play with it.
Main question: Are you able to put together an example case where the wrong result arrises?
For use in tests only...
in the_unregister_lookup
docstring. So this would show up in a test inter-dependency...? 🤔