Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions lib/hanami/router.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1040,18 +1040,36 @@ def _params(env, params)
params ||= {}
env[PARAMS] ||= {}

if !env.key?(ROUTER_PARSED_BODY) && (input = env[::Rack::RACK_INPUT]) and input.rewind
env[PARAMS].merge!(::Rack::Utils.parse_nested_query(input.read))
input.rewind
end

_merge_form_urlencoded_body!(env)
env[PARAMS].merge!(::Rack::Utils.parse_nested_query(env[::Rack::QUERY_STRING]))
env[PARAMS].merge!(params)
env[PARAMS] = Params.deep_symbolize(env[PARAMS])

env
end

# @since x.x.x
# @api private
def _merge_form_urlencoded_body!(env)
return if env.key?(ROUTER_PARSED_BODY)
return unless _form_urlencoded?(env)
return unless (input = env[::Rack::RACK_INPUT])

input.rewind
env[PARAMS].merge!(::Rack::Utils.parse_nested_query(input.read))
input.rewind # leave the stream readable for downstream consumers
end

# @since x.x.x
# @api private
def _form_urlencoded?(env)
content_type = env[CONTENT_TYPE]
return false unless content_type

content_type == FORM_URLENCODED_MEDIA_TYPE ||
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
content_type == FORM_URLENCODED_MEDIA_TYPE ||
content_type.downcase! == FORM_URLENCODED_MEDIA_TYPE ||

Feels like we should do this for maximum compatibility, and preservation of our existing behaviour. Rack::Request#media_type returns a downcased string courtesy of its internal MediaType.type. And if we're aiming to follow the spec strictly, it does say that these values are case insensitive.

content_type.start_with?("#{FORM_URLENCODED_MEDIA_TYPE};")
Comment on lines +1069 to +1070
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
content_type == FORM_URLENCODED_MEDIA_TYPE ||
content_type.start_with?("#{FORM_URLENCODED_MEDIA_TYPE};")
content_type.downcase!.start_with?(FORM_URLENCODED_MEDIA_TYPE)

In fact, I wonder whether we could actually just simplify it to this?

It'd work fine for both type/subtype as well as type/subtype;parameter=value.

end

# @since 2.0.0
# @api private
def _not_allowed_fixed(env)
Expand Down
8 changes: 8 additions & 0 deletions lib/hanami/router/constants.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,13 @@ class Router
# @api private
# @since 2.0.0
ROUTER_PARSED_BODY = "router.parsed_body"

# @api private
# @since x.x.x
CONTENT_TYPE = "CONTENT_TYPE"

# @api private
# @since x.x.x
FORM_URLENCODED_MEDIA_TYPE = "application/x-www-form-urlencoded"
end
end
22 changes: 22 additions & 0 deletions spec/integration/hanami/router/params_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,28 @@
end
end

context "JSON payload without body parser" do
# See: https://github.com/hanami/router/issues/237
it "doesn't merge a JSON body into params" do
input = JSON.generate("foo" => "bar")
env = Rack::MockRequest.env_for("/submit?from=ui", method: "POST", params: input)
env["CONTENT_TYPE"] = "application/json"
subject.call(env)

expect(env["router.params"]).to eq(from: "ui")
end
end

context "multipart payload without body parser" do
# See: https://github.com/hanami/router/issues/296
it "doesn't merge a multipart body into params" do
env, _contents = multipart_fixture("foo.xml")
subject.call(env)

expect(env["router.params"]).to eq({})
end
end

context "priority" do
it "gives first level priority to path variables" do
expected = "23"
Expand Down