Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add QueryParser#missing_value for handling missing values + tests. #2052

Merged
merged 1 commit into from Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rack.rb
Expand Up @@ -41,6 +41,7 @@ module Rack
autoload :MethodOverride, "rack/method_override"
autoload :Mime, "rack/mime"
autoload :NullLogger, "rack/null_logger"
autoload :QueryParser, "rack/query_parser"
autoload :Recursive, "rack/recursive"
autoload :Reloader, "rack/reloader"
autoload :RewindableInput, "rack/rewindable_input"
Expand Down
15 changes: 12 additions & 3 deletions lib/rack/query_parser.rb
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require_relative 'bad_request'
require 'uri'

module Rack
class QueryParser
Expand Down Expand Up @@ -111,6 +112,14 @@ def normalize_params(params, name, v, _depth=nil)
_normalize_params(params, name, v, 0)
end

# This value is used by default when a parameter is missing (nil). This
# usually happens when a parameter is specified without an `=value` part.
# The default value is an empty string, but this can be overridden by
# subclasses.
def missing_value
String.new
end

private def _normalize_params(params, name, v, depth)
raise ParamsTooDeepError if depth >= param_depth_limit

Expand Down Expand Up @@ -145,7 +154,7 @@ def normalize_params(params, name, v, _depth=nil)

return if k.empty?

v ||= String.new
v ||= missing_value

if after == ''
if k == '[]' && depth != 0
Expand Down Expand Up @@ -207,8 +216,8 @@ def params_hash_has_key?(hash, key)
true
end

def unescape(s)
Utils.unescape(s)
def unescape(string, encoding = Encoding::UTF_8)
URI.decode_www_form_component(string, encoding)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated to the change being made. I'm not necessary opposed to this, but it shouldn't be part of this PR. It can be a separate PR with a reason given for the change. Ditto for the uri require at the top.

Copy link
Member Author

@ioquatix ioquatix Mar 13, 2023

Choose a reason for hiding this comment

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

I couldn't make the separate tests pass without it, otherwise it introduces a circular dependency. I don't want to make a separate PR as we plan to backport this AFAIK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix for this is simple, submitted in #2055. However, considering this was backported without any discussion, I'm not sure what we want to do here.

end

class Params < Hash
Expand Down
33 changes: 33 additions & 0 deletions test/spec_query_parser.rb
@@ -0,0 +1,33 @@
# frozen_string_literal: true

require_relative 'helper'

separate_testing do
require_relative '../lib/rack/query_parser'
end

describe Rack::QueryParser do
def query_parser
@query_parser ||= Rack::QueryParser.new(Rack::QueryParser::Params, 8)
end

it "has a default value" do
assert_equal "", query_parser.missing_value
end

it "can normalize values with missing values" do
query_parser.parse_nested_query("a=a").must_equal({"a" => "a"})
query_parser.parse_nested_query("a=").must_equal({"a" => ""})
query_parser.parse_nested_query("a").must_equal({"a" => ""})
end

it "can override default missing value" do
def query_parser.missing_value
nil
end

query_parser.parse_nested_query("a=a").must_equal({"a" => "a"})
query_parser.parse_nested_query("a=").must_equal({"a" => ""})
query_parser.parse_nested_query("a").must_equal({"a" => nil})
end
end