Fix handling of cached values in Rack::Request.
#2054
Conversation
888ac32 to
337c70d
Compare
Rack::Request.
|
We will also want to back port this to |
d1424e8 to
4896eab
Compare
|
See rails/rails#47652 for the Rails PR. |
jeremyevans
left a comment
There was a problem hiding this comment.
This class-based caching seems like unnecessary complexity to me. The underlying issue seems to be that you don't want Rails using the same key as Rack uses, so why not just allow for easily using a separate key?:
diff --git a/lib/rack/request.rb b/lib/rack/request.rb
index e6969645..c3f8caa1 100644
--- a/lib/rack/request.rb
+++ b/lib/rack/request.rb
@@ -483,12 +483,12 @@ module Rack
# Returns the data received in the query string.
def GET
if get_header(RACK_REQUEST_QUERY_STRING) == query_string
- if query_hash = get_header(RACK_REQUEST_QUERY_HASH)
+ if query_hash = get_header(query_hash_key)
return query_hash
end
end
- set_header(RACK_REQUEST_QUERY_HASH, expand_params(query_param_list))
+ set_header(query_hash_key, expand_params(query_param_list))
end
def query_param_list
@@ -508,12 +508,12 @@ module Rack
# multipart/form-data.
def POST
if get_header(RACK_REQUEST_FORM_INPUT).equal?(get_header(RACK_INPUT))
- if form_hash = get_header(RACK_REQUEST_FORM_HASH)
+ if form_hash = get_header(form_hash_key)
return form_hash
end
end
- set_header(RACK_REQUEST_FORM_HASH, expand_params(body_param_list))
+ set_header(form_hash_key, expand_params(body_param_list))
end
def body_param_list
@@ -649,6 +649,14 @@ module Rack
private
+ def query_hash_key
+ RACK_REQUEST_QUERY_HASH
+ end
+
+ def form_hash_key
+ RACK_REQUEST_FORM_HASH
+ end
+
def default_session; {}; end
# Assist with compatibility when processing `X-Forwarded-For`.Rails (and other frameworks who want to differ from Rack's default behavior) would then override #query_hash_key and #form_hash_key to use a framework-specific key.
|
The There is an existing known issue in Rack, in that Rack::Request is designed to allow subclasses to customize how the parameters are parsed, but then all instances, regardless of their implementation, will use the same env-cached value that is stored by whoever happens to run first. Rails in particular can avoid the problem by setting a different key (plus a bit more -- either using or implementing |
|
@jeremyevans We tried your proposed approach already, but it doesn't work because we don't have the full set of keys to invalidate, if say, |
This cache key defaults to `:rack`. Sub-classes need to explicitly change this if they alter the way cached values are generated.
5671a1e to
c295b99
Compare
jeremyevans
left a comment
There was a problem hiding this comment.
Would be nice to have more documentation for the added methods explaining why the complexity is needed, but that doesn't need to block this.
I personally would like some comments before merging this. The added complexity isn't really self-evident from reading the code. Also if we're doing this to support subclasses, shouldn't we be adding tests that use subclasses? There is only one test changed in this PR and it's not very clear to me how/why that drives the rest of the changes in the PR. |
* Per-class cache keys for cached query/body parameters. * Use the query parser class as the default cache key.
* Per-class cache keys for cached query/body parameters. * Use the query parser class as the default cache key.
* Per-class cache keys for cached query/body parameters. * Use the query parser class as the default cache key.
* Per-class cache keys for cached query/body parameters. * Use the query parser class as the default cache key.
This reverts commit d25fedd.
* Revert "Prefer to use `query_parser` itself as the cache key. (#2058)" This reverts commit 5f90c33. * Revert "Fix handling of cached values in `Rack::Request`. (#2054)" This reverts commit d25fedd. * Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)" This reverts commit 59d9ba9. * Revert "Split form/query parsing into two steps (#2038)" This reverts commit 9f059d1. * Make query parameters without = have nil values This was Rack's historical behavior. While it doesn't match URL spec section 5.1.3.3, keeping the historical behavior avoids all of the complexity required to support the URL spec standard by default, but also support frameworks that want to be backwards compatible. This keeps as much of the specs added by the recently reverted commits that make sense.
* Revert "Prefer to use `query_parser` itself as the cache key. (rack#2058)" This reverts commit 5f90c33. * Revert "Fix handling of cached values in `Rack::Request`. (rack#2054)" This reverts commit d25fedd. * Revert "Add `QueryParser#missing_value` for handling missing values + tests. (rack#2052)" This reverts commit 59d9ba9. * Revert "Split form/query parsing into two steps (rack#2038)" This reverts commit 9f059d1. * Make query parameters without = have nil values This was Rack's historical behavior. While it doesn't match URL spec section 5.1.3.3, keeping the historical behavior avoids all of the complexity required to support the URL spec standard by default, but also support frameworks that want to be backwards compatible. This keeps as much of the specs added by the recently reverted commits that make sense. # Conflicts: # lib/rack/multipart.rb # lib/rack/request.rb # test/spec_request.rb
* Revert "Prefer to use `query_parser` itself as the cache key. (#2058)" This reverts commit 5f90c33. * Revert "Fix handling of cached values in `Rack::Request`. (#2054)" This reverts commit d25fedd. * Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)" This reverts commit 59d9ba9. * Revert "Split form/query parsing into two steps (#2038)" This reverts commit 9f059d1. * Make query parameters without = have nil values This was Rack's historical behavior. While it doesn't match URL spec section 5.1.3.3, keeping the historical behavior avoids all of the complexity required to support the URL spec standard by default, but also support frameworks that want to be backwards compatible. This keeps as much of the specs added by the recently reverted commits that make sense. # Conflicts: # lib/rack/multipart.rb # lib/rack/request.rb # test/spec_request.rb
This PR introduces two private cache helpers for
Rack::Request, based on a proposal from @matthewd:cache_for: Things which are expected to be per-request, and are cached inenv, and depend on a specific key matching a current input value.class_cache_for: Things which are expected to be per-request-class, and are cached inenvusing a per-class cache key, and depend on a specific key matching a current input value.It changes the structure of the
envcache keys slightly whenclass_cache_foris used, as this usesself.cache_keyas a key in the cache lookup.With this change, the bespoke cache logic is now centralised in these two methods, which also unify error handling.
While I'm slightly against this level of complexity, it is required for sub-classes of
requestwhich change the behaviour ofdef GETanddef POSTto behave correctly. I have confirmed this works with Rails which essentially sub-classesRack::Requestwith the following addition:I'm mostly happy that this makes sense and is a reasonable implementation.
Some thoughts, in no particular order:
envmakes caching harder, e.g. since at any timeQUERY_STRINGcan change. I feel like this is a poor design, and might be a strong signal that we need to rethink how this cache works so that it can be more efficient and/or predictable. For example, I don't understand what the use case is for mutatingQUERY_STRING. It's also true that we key onrack.input- and this also doesn't make a huge amount of sense (why is this being updated).Rack::Requestfor the sameenvis inefficient, and it would be good to avoid doing this. Since some things should be cached per-env, e.g. query parameter list/body parameter list, but other things should be cached per request instance, e.g. the interpretation of these fields and potentially other fields. IIRC, there was some level of memoization ofRack::Requestinenv, I don't recall if this existed or why it was removed.Rack::Requestbut it seems like we don't have much choice given the circumstances. I would like to think that Rails could adopt a more standards based approach to query parsing and/or we can simplify the cache model by some of the other proposed changes, e.g. an additional interface which uses passes a singleRack::Requestinstance through the middleware or perhaps some changes to howenvmutability is considered and/or instances ofRack::Requestare cached.Potential solution to #2053.