Opened 3 years ago

Closed 2 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 Himanshu Balasamanta)

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:1 by Carlton Gibson, 3 years ago

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 interdependency...? 🤔
Version 0, edited 3 years ago by Carlton Gibson (next)

comment:2 by Himanshu Balasamanta, 3 years ago

Description: modified (diff)

in reply to:  1 comment:3 by Himanshu Balasamanta, 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.)

Last edited 3 years ago by Himanshu Balasamanta (previous) (diff)

in reply to:  1 comment:4 by Himanshu Balasamanta, 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 Himanshu Balasamanta, 3 years ago

Description: modified (diff)

comment:6 by Carlton Gibson, 3 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

Hi Himanshu. Thanks for updating with the PR. I'll accept to review.

comment:7 by Carlton Gibson, 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_lookupsRegisterLookupMixin._unregister_lookup() should clear the lookup cache.

comment:8 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin
Type: BugCleanup/optimization

comment:9 by Mariusz Felisiak, 2 years ago

Component: Error reportingDatabase layer (models, ORM)

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 06ebaa9e:

Fixed #33626 -- Cleared cache when unregistering a lookup.

Note: See TracTickets for help on using tickets.
Back to Top