From 0b4a0d95a05be20bd48ffcf0135676359cf42d94 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Mon, 24 Apr 2023 20:48:53 -0700 Subject: [PATCH 1/2] Deprecate automatic cache invalidation in Request#{GET,POST} Add Request#clear_{GET,POST} for users to perform manual cache invalidation when replacing env['QUERY_STRING'] or env['rack.input']. With this invalidation, env[RACK_REQUEST_QUERY_STRING] and env[RACK_REQUEST_FORM_INPUT] are unnecessary. It appears as though env[RACK_REQUEST_FORM_VARS] is already unnecessary, as the value is set but never accessed, dating back to its introduction in 6c80c6cf86f1f897e08882cb6c9ae731fab1b9c1. However, even though it is never used by Rack, it apparently is used by Rails. However, Rails usage appears to be limited to parameter filtering, and if the RACK_REQUEST_FORM_VARS key wasn't set, there would be nothing to filter. So it's possible Rails could be changed so that if the key was missing, there are no problems (maybe it works like that already, and only the Rails tests need updates). --- CHANGELOG.md | 3 ++- lib/rack/request.rb | 30 ++++++++++++++++++++++++++++-- test/spec_request.rb | 25 ++++++++++++++++++++++++- 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7383a2411..7cb53ff2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,8 @@ All notable changes to this project will be documented in this file. For info on - Add fallback lookup and deprecation warning for obsolete status symbols. ([#2137](https://github.com/rack/rack/pull/2137), [@wtn]) - In `Rack::Files`, ignore the `Range` header if served file is 0 bytes. ([#2159](https://github.com/rack/rack/pull/2159), [@zarqman]) - rack.early_hints is now officially supported as an optional feature (already implemented by Unicorn, Puma, and Falcon). ([#1831](https://github.com/rack/rack/pull/1831), [@casperisfine, @jeremyevans]) -- - Add `Rack::SetXForwardedProtoHeader` middleware ([#2089](https://github.com/rack/rack/pull/2089), [@tomharvey]) +- Add `Rack::SetXForwardedProtoHeader` middleware ([#2089](https://github.com/rack/rack/pull/2089), [@tomharvey]) +- Deprecate automatic cache invalidation in `Request#{GET,POST}` and add `Request#clear_{GET,POST}` for manual cache invalidation ([@jeremyevans]) ## [3.0.11] - 2024-05-10 diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 30d9abdb1..68edff9c5 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -480,11 +480,34 @@ def parseable_data? PARSEABLE_DATA_MEDIA_TYPES.include?(media_type) end + # Clear cached data related to GET/query string parameters. + # Should be called if modifying env["QUERY_STRING"]. + def clear_GET + env.delete(RACK_REQUEST_QUERY_STRING) # Remove in Rack 3.2 + env.delete(RACK_REQUEST_QUERY_HASH) + @params = nil + end + + # Clear cached data related to POST/request body parameters. + # Should be called if modifying env["rack.input"]. + def clear_POST + env.delete(RACK_REQUEST_FORM_INPUT) # Remove in Rack 3.2 + env.delete(RACK_REQUEST_FORM_VARS) + env.delete(RACK_REQUEST_FORM_HASH) + env.delete(RACK_REQUEST_FORM_ERROR) + @params = nil + end + # Returns the data received in the query string. def GET - if get_header(RACK_REQUEST_QUERY_STRING) == query_string + rr_query_string = get_header(RACK_REQUEST_QUERY_STRING) + query_string = self.query_string + if rr_query_string == query_string get_header(RACK_REQUEST_QUERY_HASH) else + if rr_query_string + warn "Cached query string different from current query string. Starting in Rack 3.2, you must call clear_GET when modifying env[\"QUERY_STRING\"]", uplevel: 1 + end query_hash = parse_query(query_string, '&') set_header(RACK_REQUEST_QUERY_STRING, query_string) set_header(RACK_REQUEST_QUERY_HASH, query_hash) @@ -505,9 +528,12 @@ def POST # If the form hash was already memoized: if form_hash = get_header(RACK_REQUEST_FORM_HASH) + form_input = get_header(RACK_REQUEST_FORM_INPUT) # And it was memoized from the same input: - if get_header(RACK_REQUEST_FORM_INPUT).equal?(rack_input) + if form_input.equal?(rack_input) return form_hash + elsif form_input + warn "Cached input stream value different from current input stream. Starting in Rack 3.2, you must call clear_POST when modifying env[\"#{RACK_INPUT}\"]", uplevel: 1 end end diff --git a/test/spec_request.rb b/test/spec_request.rb index 661c45708..d04d33b6b 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -911,7 +911,7 @@ def initialize(*) req.media_type.must_be_nil end - it "cache, but invalidates the cache" do + deprecated "cache, but invalidates the cache" do req = make_request \ Rack::MockRequest.env_for("/?foo=quux", "CONTENT_TYPE" => "application/x-www-form-urlencoded", @@ -929,6 +929,29 @@ def initialize(*) req.POST.must_equal "foo" => "bla", "quux" => "bar" end + it "support manual cache invalidation via clear_GET and clear_POST" do + req = make_request \ + Rack::MockRequest.env_for("/?foo=quux", + "CONTENT_TYPE" => "application/x-www-form-urlencoded", + :input => "foo=bar&quux=bla") + req.GET.must_equal "foo" => "quux" + req.GET.must_equal "foo" => "quux" + req.POST.must_equal "foo" => "bar", "quux" => "bla" + req.POST.must_equal "foo" => "bar", "quux" => "bla" + + req.set_header("QUERY_STRING", "bla=foo") + req.clear_GET + req.GET.must_equal "bla" => "foo" + req.GET.must_equal "bla" => "foo" + req.params.must_equal "bla" => "foo", "foo" => "bar", "quux" => "bla" + + req.set_header("rack.input", StringIO.new("foo=bla&quux=bar")) + req.clear_POST + req.POST.must_equal "foo" => "bla", "quux" => "bar" + req.POST.must_equal "foo" => "bla", "quux" => "bar" + req.params.must_equal "bla" => "foo", "foo" => "bla", "quux" => "bar" + end + it "figure out if called via XHR" do req = make_request(Rack::MockRequest.env_for("")) req.wont_be :xhr? From 3136917c0589ddacf700c5df2fa8e9e3e60043ca Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 31 May 2024 08:27:32 -0700 Subject: [PATCH 2/2] Remove manual cache invalidation via clear_{GET,POST} methods Fix the warning messages now that manual cache invalidation is no longer possible via those methods. --- lib/rack/request.rb | 22 ++-------------------- test/spec_request.rb | 23 ----------------------- 2 files changed, 2 insertions(+), 43 deletions(-) diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 68edff9c5..0a0a21b71 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -480,24 +480,6 @@ def parseable_data? PARSEABLE_DATA_MEDIA_TYPES.include?(media_type) end - # Clear cached data related to GET/query string parameters. - # Should be called if modifying env["QUERY_STRING"]. - def clear_GET - env.delete(RACK_REQUEST_QUERY_STRING) # Remove in Rack 3.2 - env.delete(RACK_REQUEST_QUERY_HASH) - @params = nil - end - - # Clear cached data related to POST/request body parameters. - # Should be called if modifying env["rack.input"]. - def clear_POST - env.delete(RACK_REQUEST_FORM_INPUT) # Remove in Rack 3.2 - env.delete(RACK_REQUEST_FORM_VARS) - env.delete(RACK_REQUEST_FORM_HASH) - env.delete(RACK_REQUEST_FORM_ERROR) - @params = nil - end - # Returns the data received in the query string. def GET rr_query_string = get_header(RACK_REQUEST_QUERY_STRING) @@ -506,7 +488,7 @@ def GET get_header(RACK_REQUEST_QUERY_HASH) else if rr_query_string - warn "Cached query string different from current query string. Starting in Rack 3.2, you must call clear_GET when modifying env[\"QUERY_STRING\"]", uplevel: 1 + warn "query string used for GET parsing different from current query string. Starting in Rack 3.2, Rack will used the cached GET value instead of parsing the current query string.", uplevel: 1 end query_hash = parse_query(query_string, '&') set_header(RACK_REQUEST_QUERY_STRING, query_string) @@ -533,7 +515,7 @@ def POST if form_input.equal?(rack_input) return form_hash elsif form_input - warn "Cached input stream value different from current input stream. Starting in Rack 3.2, you must call clear_POST when modifying env[\"#{RACK_INPUT}\"]", uplevel: 1 + warn "input stream used for POST parsing different from current input stream. Starting in Rack 3.2, Rack will used the cached POST value instead of parsing the current input stream.", uplevel: 1 end end diff --git a/test/spec_request.rb b/test/spec_request.rb index d04d33b6b..3c26d8c65 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -929,29 +929,6 @@ def initialize(*) req.POST.must_equal "foo" => "bla", "quux" => "bar" end - it "support manual cache invalidation via clear_GET and clear_POST" do - req = make_request \ - Rack::MockRequest.env_for("/?foo=quux", - "CONTENT_TYPE" => "application/x-www-form-urlencoded", - :input => "foo=bar&quux=bla") - req.GET.must_equal "foo" => "quux" - req.GET.must_equal "foo" => "quux" - req.POST.must_equal "foo" => "bar", "quux" => "bla" - req.POST.must_equal "foo" => "bar", "quux" => "bla" - - req.set_header("QUERY_STRING", "bla=foo") - req.clear_GET - req.GET.must_equal "bla" => "foo" - req.GET.must_equal "bla" => "foo" - req.params.must_equal "bla" => "foo", "foo" => "bar", "quux" => "bla" - - req.set_header("rack.input", StringIO.new("foo=bla&quux=bar")) - req.clear_POST - req.POST.must_equal "foo" => "bla", "quux" => "bar" - req.POST.must_equal "foo" => "bla", "quux" => "bar" - req.params.must_equal "bla" => "foo", "foo" => "bla", "quux" => "bar" - end - it "figure out if called via XHR" do req = make_request(Rack::MockRequest.env_for("")) req.wont_be :xhr?