From eb7843cc2fabb62c66174f89f951412c0f6ffc9f Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 18 Dec 2024 12:13:15 -0800 Subject: [PATCH] Make json_parser plugin handle case where Rack::Request#POST is called previously for env on Rack 3 (Fixes #372) The Rack 1-2 behavior did not cache rack.request.form_hash values for data it didn't understand. That changed in Rack 3, to fix a bug in Rack that was open since 2014. This updates the code to work with all versions of Rack. It now never checks rack.request.form_hash or sets rack.request.form_input. This approach is slightly slower for r.POST when given non-JSON input, because it now checks the content_type when previously it did not. Unfortunately, I don't see a way to avoid that. I did make the check faster by switching from a regexp to using String#include?. In most cases, r.POST performance doesn't matter, as users typically use r.params, and that is cached via an instance variable, so r.POST is generally only called once per request. --- CHANGELOG | 2 ++ lib/roda/plugins/json_parser.rb | 39 +++++++++++++++++---------------- spec/plugin/json_parser_spec.rb | 7 ++++++ spec/spec_helper.rb | 10 ++++++--- 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index cf18bb0c..bfd88bfd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,7 @@ === master +* Make json_parser plugin handle case where Rack::Request#POST is called previously for env on Rack 3 (jeremyevans) (#372) + * Fix strict_unused_block warnings when running specs on Ruby 3.4 (jeremyevans) = 3.87.0 (2024-12-17) diff --git a/lib/roda/plugins/json_parser.rb b/lib/roda/plugins/json_parser.rb index b9a3be99..00b724e4 100644 --- a/lib/roda/plugins/json_parser.rb +++ b/lib/roda/plugins/json_parser.rb @@ -53,27 +53,28 @@ module RequestMethods # parse the request body as JSON. Ignore an empty request body. def POST env = @env - if post_params = (env["roda.json_params"] || env["rack.request.form_hash"]) - post_params - elsif (input = env["rack.input"]) && content_type =~ /json/ - str = _read_json_input(input) - return super if str.empty? - begin - json_params = parse_json(str) - rescue - roda_class.opts[:json_parser_error_handler].call(self) - end + if post_params = env["roda.json_params"] + return post_params + end + + unless (input = env["rack.input"]) && (content_type = self.content_type) && content_type.include?('json') + return super + end + + str = _read_json_input(input) + return super if str.empty? + begin + json_params = parse_json(str) + rescue + roda_class.opts[:json_parser_error_handler].call(self) + end - wrap = roda_class.opts[:json_parser_wrap] - if wrap == :always || (wrap == :unless_hash && !json_params.is_a?(Hash)) - json_params = {"_json"=>json_params} - end - env["roda.json_params"] = json_params - env["rack.request.form_input"] = input - json_params - else - super + wrap = roda_class.opts[:json_parser_wrap] + if wrap == :always || (wrap == :unless_hash && !json_params.is_a?(Hash)) + json_params = {"_json"=>json_params} end + env["roda.json_params"] = json_params + json_params end private diff --git a/spec/plugin/json_parser_spec.rb b/spec/plugin/json_parser_spec.rb index 3c6a4371..c06c605a 100644 --- a/spec/plugin/json_parser_spec.rb +++ b/spec/plugin/json_parser_spec.rb @@ -11,6 +11,13 @@ body('rack.input'=>rack_input('{"a":{"b":1}}'), 'CONTENT_TYPE'=>'text/json', 'REQUEST_METHOD'=>'POST').must_equal '1' end + it "Handles Rack::Request#POST being called in advance" do + env = req_env('rack.input'=>rack_input('{"a":{"b":1}}'), 'CONTENT_TYPE'=>'text/json', 'REQUEST_METHOD'=>'POST') + r = Rack::Request.new(env) + r.POST + body(env).must_equal '1' + end + it "doesn't affect parsing of non-json content type" do body('rack.input'=>rack_input('a[b]=1'), 'REQUEST_METHOD'=>'POST').must_equal '1' end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 08fe0f6f..0e49ceb0 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -170,8 +170,8 @@ def req(path='/', env={}) _req(@app, env) end - def _req(app, env) - env = {"REQUEST_METHOD" => "GET", "PATH_INFO" => "/", "SCRIPT_NAME" => ""}.merge(env) + def req_env(env) + env = {"REQUEST_METHOD" => "GET", "PATH_INFO" => "/", "SCRIPT_NAME" => ""}.merge!(env) if ENV['LINT'] env['SERVER_NAME'] ||= 'example.com' env['SERVER_PROTOCOL'] ||= env['HTTP_VERSION'] || 'HTTP/1.0' @@ -187,7 +187,11 @@ def _req(app, env) env['rack.multiprocess'] = env['rack.multithread'] = env['rack.run_once'] = false end end - a = @app.call(env) + env + end + + def _req(app, env) + a = @app.call(req_env(env)) if ENV['LINT'] orig = a[2]