https://github.com/django/django/blob/418263c457636d3301f2068c47f09a0f42e15c52/django/middleware/cache.py#L127
I have an idea how to improve readability of this method. I've already
implemented
it but before creating an issue and PR I decided to discuss it with
community. Maybe
all of this doesn't really have any sense.
I propose to change this code:
136 # try and get the cached GET response
137 cache_key = get_cache_key(request, self.key_prefix, 'GET', cache=
self.cache)
138 if cache_key is None:
139 request._cache_update_cache = True
140 return None # No cache information available, need to rebuild.
141 response = self.cache.get(cache_key)
142 # if it wasn't found and we are looking for a HEAD, try looking
just for that
143 if response is None and request.method == 'HEAD':
144 cache_key = get_cache_key(request, self.key_prefix, 'HEAD',
cache=self.cache)
145 response = self.cache.get(cache_key)
146
147 if response is None:
148 request._cache_update_cache = True
149 return None # No cache information available, need to rebuild.
On this code:
136 vary_headers = get_cached_vary_headers(request)
137 if vary_headers is None:
138 # Vary headers for this request was not cached,
139 # so there is no cache for both GET and HEAD requests
140 # and cache should be updated
141 request._cache_update_cache = True
142 return None
143
144 cached_response = None
145
146 if request.method == 'GET':
147 cached_response = get_cached_response(request, vary_headers,
'GET', self.key_prefix, self.cache)
148
149 if request.method == 'HEAD':
150 # HEAD request can use GET and HEAD responses both
151 # because web servers should delete body when sending HEAD
response
152 cached_response = get_cached_response(request, vary_headers,
'GET', self.key_prefix, self.cache) or \
153 get_cached_response(request, vary_headers,
'HEAD', self.key_prefix, self.cache)
154
155 if cached_response is None:
156 request._cache_update_cache = True
157 return None # No cache information available, need to rebuild.
Why? I will try to explain it by examples of questions that I had while
reading
this code.
Let's look at the line 140. What does get_cache_key() do? Let's suppose
it generates
key for given request and also takes into account the http method.
If so, what does it mean when cache_key is None? In according to
comment on 140 line,
None means that cache doesn't have the response for this request and
method.
Then why we don't check if cache_key is None after 144 line?
If None value of cache_key means that cache doesn't have the response
for given request,
therefore not-None value means that cache has response. If so, why do
we check
if response is None on 143 line?
I suspect the problem of this code is that it tries to hide one
implementation
detail inside get_cache_key(). This detail is caching vary headers for the
request.
get_cache_key(), at first, tries to get these headers and if there are none
of them
it returns None that means there is no cache satisfying given request. But
if
there are these headers get_cache_key() just generates key taking into
account request, method and headers. But generated key doesn't mean that
cache actually has something for this key.
I suspect that refactored code can answer all of these questions above, and
looks
more readable, clear and explicit.
So what do you think about it?
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/056cecfe-979f-4662-988c-0378669c498c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
