Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix search cache #57

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Conversation

ottusp
Copy link
Contributor

@ottusp ottusp commented Jul 1, 2024

Context

Cache policy does not take into account parameters in the request body. It should not, as this is an anti-pattern. However, search requests use a proxy to list method, which, contrary to search, supports cache.

The issue

The problem is that the proxified request still use the arguments passed in the body, just like search request received it. And as cache does not look at body arguments, a search request that looked like this:

// example.com?potato=true
{
    "id": [10, 11, 12]
}

Will have a cache hit with the following request (which is completely different from the previous):

// example.com?potato=true
{
    "title": "Despicable me"
    "related_titles": ["A quiet place"]
}

Solution

The solution has two layers:

  1. Add bypass cache to subsequent cached methods in a View call stack. Only the view entry point (i.e. the method that maps to the endpoint hit by the client) will be able to both consume from cache and set values to it. The subsequent method calls ignore caching.
  2. Create a new cache key_func for search requests. This key_func takes into account the body passed to the request, so calls for the same url, but with different bodies will now map to different keys, avoiding unexpected cache hits.

@ottusp ottusp force-pushed the of-fix-search-cache branch from 8dd4fbb to 0340c3d Compare July 1, 2024 19:31
drf_kit/views/viewsets.py Outdated Show resolved Hide resolved
@ottusp ottusp force-pushed the of-fix-search-cache branch from 0340c3d to 05b662d Compare July 3, 2024 00:23
Copy link
Contributor

@joaodaher joaodaher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the strategy to prevent double-caching works, creating s new custom directive can become complex because you'll have one more moving part to handle from the client-side POV.
Also, I think problem lies deeper in the architecture: re-usage of list. So we should tackle it directly.

I can see 2 options:

(A) list-no-cache
A new list method with no cache (eg. def _list_no_cache) will be reused by the cached list (def list) and the search (def search) and each one of them have their own dedicated cache.

(B) don't reuse list
Simply don't reuse list at all. Copy its logic to the search.
At first glance it might seem against DRY, but search might have it's own custom logic that differs from the default list, which is actually good.
The code itself is quite simple.

@ottusp ottusp force-pushed the of-fix-search-cache branch 3 times, most recently from 8bc8e35 to 973f5bb Compare July 3, 2024 19:20
drf_kit/views/viewsets.py Outdated Show resolved Hide resolved
@ottusp ottusp force-pushed the of-fix-search-cache branch from 973f5bb to dddd0e2 Compare July 3, 2024 19:49
drf_kit/cache.py Outdated
Comment on lines 63 to 65
elif request.META.get("X-Bypass-Cache"):
valid_cache_control = False
response_dict = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this too from the previous approach.

@ottusp ottusp force-pushed the of-fix-search-cache branch from dddd0e2 to 6c42ab7 Compare July 3, 2024 20:08
ottusp added 3 commits July 4, 2024 11:51
Having a view method calling another adds complexity to the viewset. For example, it is harder to handle cache if two different methods have their own cache rules. This change aims to make it easier for upcoming changes to deal with view methods gracefully.
In addition to new cache strategy, new ViewSets were created to support
specific cache-searching funcionality
@ottusp ottusp force-pushed the of-fix-search-cache branch from 6c42ab7 to 0ed89b2 Compare July 4, 2024 14:52
@joaodaher joaodaher merged commit 51c64fb into flamingo-run:main Jul 4, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants