Make query parameters without = have nil values (#2059)

* Revert "Prefer to use `query_parser` itself as the cache key. (#2058)"

This reverts commit 5f90c33e4ccee827cb5df3d8854dc72791345c51.

* Revert "Fix handling of cached values in `Rack::Request`. (#2054)"

This reverts commit d25feddcbe634d95ec693bfbd710167a11c74069.

* Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)"

This reverts commit 59d9ba903fdb50cf8db708c8263a7b2a79de83fb.

* Revert "Split form/query parsing into two steps (#2038)"

This reverts commit 9f059d19647aeaef5c2cc683a333c06120caf939.

* 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.

7 files changed, 64 insertions, 297 deletions

diff --git a/lib/rack/constants.rb b/lib/rack/constants.rb
index 5b0c181e..13365935 100644
--- a/lib/rack/constants.rb
+++ b/lib/rack/constants.rb
@@ -54,13 +54,11 @@ module Rack
   RACK_RESPONSE_FINISHED              = 'rack.response_finished'
   RACK_REQUEST_FORM_INPUT             = 'rack.request.form_input'
   RACK_REQUEST_FORM_HASH              = 'rack.request.form_hash'
-  RACK_REQUEST_FORM_PAIRS             = 'rack.request.form_pairs'
   RACK_REQUEST_FORM_VARS              = 'rack.request.form_vars'
   RACK_REQUEST_FORM_ERROR             = 'rack.request.form_error'
   RACK_REQUEST_COOKIE_HASH            = 'rack.request.cookie_hash'
   RACK_REQUEST_COOKIE_STRING          = 'rack.request.cookie_string'
   RACK_REQUEST_QUERY_HASH             = 'rack.request.query_hash'
-  RACK_REQUEST_QUERY_PAIRS            = 'rack.request.query_pairs'
   RACK_REQUEST_QUERY_STRING           = 'rack.request.query_string'
   RACK_METHODOVERRIDE_ORIGINAL_METHOD = 'rack.methodoverride.original_method'
 end
diff --git a/lib/rack/multipart.rb b/lib/rack/multipart.rb
index 4b02fb3e..165b4db3 100644
--- a/lib/rack/multipart.rb
+++ b/lib/rack/multipart.rb
@@ -19,31 +19,6 @@ module Rack
       include BadRequest
     end
 
-    # Accumulator for multipart form data, conforming to the QueryParser API.
-    # In future, the Parser could return the pair list directly, but that would
-    # change its API.
-    class ParamList # :nodoc:
-      def self.make_params
-        new
-      end
-
-      def self.normalize_params(params, key, value)
-        params << [key, value]
-      end
-
-      def initialize
-        @pairs = []
-      end
-
-      def <<(pair)
-        @pairs << pair
-      end
-
-      def to_params_hash
-        @pairs
-      end
-    end
-
     class << self
       def parse_multipart(env, params = Rack::Utils.default_query_parser)
         unless io = env[RACK_INPUT]
diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb
index 525900f7..28cbce18 100644
--- a/lib/rack/query_parser.rb
+++ b/lib/rack/query_parser.rb
@@ -38,42 +38,19 @@ module Rack
       @param_depth_limit = param_depth_limit
     end
 
-    # Originally stolen from Mongrel, now with some modifications:
+    # Stolen from Mongrel, with some small modifications:
     # Parses a query string by breaking it up at the '&'.  You can also use this
     # to parse cookies by changing the characters used in the second parameter
     # (which defaults to '&').
-    #
-    # Returns an array of 2-element arrays, where the first element is the
-    # key and the second element is the value.
-    def split_query(qs, separator = nil, &unescaper)
-      pairs = []
-
-      if qs && !qs.empty?
-        unescaper ||= method(:unescape)
-
-        qs.split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p|
-          next if p.empty?
-          pair = p.split('=', 2).map!(&unescaper)
-          pair << nil if pair.length == 1
-          pairs << pair
-        end
-      end
-
-      pairs
-    rescue ArgumentError => e
-      raise InvalidParameterError, e.message, e.backtrace
-    end
-
-    # Parses a query string by breaking it up at the '&'.  You can also use this
-    # to parse cookies by changing the characters used in the second parameter
-    # (which defaults to '&').
-    #
-    # Returns a hash where each value is a string (when a key only appears once)
-    # or an array of strings (when a key appears more than once).
     def parse_query(qs, separator = nil, &unescaper)
+      unescaper ||= method(:unescape)
+
       params = make_params
 
-      split_query(qs, separator, &unescaper).each do |k, v|
+      (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p|
+        next if p.empty?
+        k, v = p.split('=', 2).map!(&unescaper)
+
         if cur = params[k]
           if cur.class == Array
             params[k] << v
@@ -85,7 +62,7 @@ module Rack
         end
       end
 
-      params.to_h
+      return params.to_h
     end
 
     # parse_nested_query expands a query string into structural types. Supported
@@ -96,11 +73,17 @@ module Rack
     def parse_nested_query(qs, separator = nil)
       params = make_params
 
-      split_query(qs, separator).each do |k, v|
-        _normalize_params(params, k, v, 0)
+      unless qs.nil? || qs.empty?
+        (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p|
+          k, v = p.split('=', 2).map! { |s| unescape(s) }
+
+          _normalize_params(params, k, v, 0)
+        end
       end
 
-      params.to_h
+      return params.to_h
+    rescue ArgumentError => e
+      raise InvalidParameterError, e.message, e.backtrace
     end
 
     # normalize_params recursively expands parameters into structural types. If
@@ -112,14 +95,6 @@ module Rack
       _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
 
@@ -154,8 +129,6 @@ module Rack
 
       return if k.empty?
 
-      v ||= missing_value
-
       if after == ''
         if k == '[]' && depth != 0
           return [v]
diff --git a/lib/rack/request.rb b/lib/rack/request.rb
index fbdead8a..a3eb9926 100644
--- a/lib/rack/request.rb
+++ b/lib/rack/request.rb
@@ -480,115 +480,14 @@ module Rack
         PARSEABLE_DATA_MEDIA_TYPES.include?(media_type)
       end
 
-      # Given a current input value, and a validity key, check if the cache
-      # is valid, and if so, return the cached value. If not, yield the
-      # current value to the block, and set the cache to the result.
-      #
-      # This method does not use cache_key, so it is shared between all
-      # instance of Rack::Request and it's sub-classes.
-      private def cache_for(key, validity_key, current_value)
-        # Get the current value of the validity key and compare it with the input value:
-        if get_header(validity_key).equal?(current_value)
-          # If the values are the same, then the cache is valid, so return the cached value.
-          if has_header?(key)
-            value = get_header(key)
-            # If the cached value is an exception, then re-raise it.
-            if value.is_a?(Exception)
-              raise value.class, value.message, cause: value.cause
-            else
-              # Otherwise, return the cached value.
-              return value
-            end
-          end
-        end
-
-        # If the cache is not valid, then yield the current value to the block:
-        value = yield(current_value)
-
-        # Set the validity key to the current value so that we can detect changes:
-        set_header(validity_key, current_value)
-
-        # Set the cache to the result of the block, and return the result:
-        set_header(key, value)
-      rescue => error
-        # If an exception is raised, then set the cache to the exception, and re-raise it:
-        set_header(validity_key, current_value)
-        set_header(key, error)
-        raise
-      end
-
-      # This cache key is used by cached values generated by class_cache_for,
-      # specfically GET and POST. This is to ensure that the cache is not
-      # shared between instances of different classes which have different
-      # behaviour. This includes sub-classes that override query_parser or
-      # expand_params.
-      def cache_key
-        query_parser
-      end
-
-      # Given a current input value, and a validity key, check if the cache
-      # is valid, and if so, return the cached value. If not, yield the
-      # current value to the block, and set the cache to the result.
-      #
-      # This method uses cache_key to ensure that the cache is not shared
-      # between instances of different classes which have different
-      # behaviour of the cached operations.
-      private def class_cache_for(key, validity_key, current_value)
-        # The cache is organised in the env as:
-        # env[key][cache_key] = value
-        # and is valid as long as env[validity_key].equal?(current_value)
-
-        cache_key = self.cache_key
-
-        # Get the current value of the validity key and compare it with the input value:
-        if get_header(validity_key).equal?(current_value)
-          # Lookup the cache for the current cache key:
-          if cache = get_header(key)
-            if cache.key?(cache_key)
-              # If the cache is valid, then return the cached value.
-              value = cache[cache_key]
-              if value.is_a?(Exception)
-                # If the cached value is an exception, then re-raise it.
-                raise value.class, value.message, cause: value.cause
-              else
-                # Otherwise, return the cached value.
-                return value
-              end
-            end
-          end
-        end
-
-        # If the cache was not defined for this cache key, then create a new cache:
-        unless cache
-          cache = Hash.new.compare_by_identity
-          set_header(key, cache)
-        end
-
-        begin
-          # Yield the current value to the block to generate an updated value:
-          value = yield(current_value)
-
-          # Only set this after generating the value, so that if an error or other cache depending on the same key, it will be invalidated correctly:
-          set_header(validity_key, current_value)
-          return cache[cache_key] = value
-        rescue => error
-          set_header(validity_key, current_value)
-          cache[cache_key] = error
-          raise
-        end
-      end
-
       # Returns the data received in the query string.
       def GET
-        class_cache_for(RACK_REQUEST_QUERY_HASH, RACK_REQUEST_QUERY_STRING, query_string) do
-          expand_params(query_param_list)
-        end
-      end
-
-      def query_param_list
-        cache_for(RACK_REQUEST_QUERY_PAIRS, RACK_REQUEST_QUERY_STRING, query_string) do
-          set_header(RACK_REQUEST_QUERY_HASH, nil)
-          split_query(query_string, '&')
+        if get_header(RACK_REQUEST_QUERY_STRING) == query_string
+          get_header(RACK_REQUEST_QUERY_HASH)
+        else
+          query_hash = parse_query(query_string, '&')
+          set_header(RACK_REQUEST_QUERY_STRING, query_string)
+          set_header(RACK_REQUEST_QUERY_HASH, query_hash)
         end
       end
 
@@ -597,33 +496,46 @@ module Rack
       # This method support both application/x-www-form-urlencoded and
       # multipart/form-data.
       def POST
-        class_cache_for(RACK_REQUEST_FORM_HASH, RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)) do
-          expand_params(body_param_list)
+        if error = get_header(RACK_REQUEST_FORM_ERROR)
+          raise error.class, error.message, cause: error.cause
         end
-      end
 
-      def body_param_list
-        cache_for(RACK_REQUEST_FORM_PAIRS, RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)) do |rack_input|
+        begin
+          rack_input = get_header(RACK_INPUT)
+
+          # If the form hash was already memoized:
+          if form_hash = get_header(RACK_REQUEST_FORM_HASH)
+            # And it was memoized from the same input:
+            if get_header(RACK_REQUEST_FORM_INPUT).equal?(rack_input)
+              return form_hash
+            end
+          end
+
+          # Otherwise, figure out how to parse the input:
           if rack_input.nil?
-            form_pairs = []
+            set_header RACK_REQUEST_FORM_INPUT, nil
+            set_header(RACK_REQUEST_FORM_HASH, {})
           elsif form_data? || parseable_data?
-            unless form_pairs = Rack::Multipart.extract_multipart(self, Rack::Multipart::ParamList)
-              form_vars = rack_input.read
+            unless set_header(RACK_REQUEST_FORM_HASH, parse_multipart)
+              form_vars = get_header(RACK_INPUT).read
 
               # Fix for Safari Ajax postings that always append \0
               # form_vars.sub!(/\0\z/, '') # performance replacement:
               form_vars.slice!(-1) if form_vars.end_with?("\0")
 
-              # Removing this line breaks Rail test "test_filters_rack_request_form_vars"!
-              set_header(RACK_REQUEST_FORM_VARS, form_vars)
-
-              form_pairs = split_query(form_vars, '&')
+              set_header RACK_REQUEST_FORM_VARS, form_vars
+              set_header RACK_REQUEST_FORM_HASH, parse_query(form_vars, '&')
             end
+
+            set_header RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)
+            get_header RACK_REQUEST_FORM_HASH
           else
-            form_pairs = []
+            set_header RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)
+            set_header(RACK_REQUEST_FORM_HASH, {})
           end
-          
-          form_pairs
+        rescue => error
+          set_header(RACK_REQUEST_FORM_ERROR, error)
+          raise
         end
       end
 
@@ -760,28 +672,6 @@ module Rack
         Rack::Multipart.extract_multipart(self, query_parser)
       end
 
-      def split_query(query, d = '&')
-        query_parser = query_parser()
-        unless query_parser.respond_to?(:split_query)
-          query_parser = Utils.default_query_parser
-          unless query_parser.respond_to?(:split_query)
-            query_parser = QueryParser.make_default(0)
-          end
-        end
-
-        query_parser.split_query(query, d)
-      end
-
-      def expand_params(pairs, query_parser = query_parser())
-        params = query_parser.make_params
-
-        pairs.each do |key, value|
-          query_parser.normalize_params(params, key, value)
-        end
-
-        params.to_params_hash
-      end
-
       def split_header(value)
         value ? value.strip.split(/[,\s]+/) : []
       end
diff --git a/test/spec_query_parser.rb b/test/spec_query_parser.rb
index 5f470818..dbb8b14e 100644
--- a/test/spec_query_parser.rb
+++ b/test/spec_query_parser.rb
@@ -7,27 +7,11 @@ separate_testing do
 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
+  query_parser ||= Rack::QueryParser.make_default(8)
 
   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
diff --git a/test/spec_request.rb b/test/spec_request.rb
index e525621a..9966762e 100644
--- a/test/spec_request.rb
+++ b/test/spec_request.rb
@@ -574,10 +574,9 @@ class RackRequestTest < Minitest::Spec
   it "parse the query string" do
     request = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=bla&nothing&empty="))
     request.query_string.must_equal "foo=bar&quux=bla&nothing&empty="
-    request.GET.must_equal "foo" => "bar", "quux" => "bla", "nothing" => "", "empty" => ""
+    request.GET.must_equal "foo" => "bar", "quux" => "bla", "nothing" => nil, "empty" => ""
     request.POST.must_be :empty?
-    request.params.must_equal "foo" => "bar", "quux" => "bla", "nothing" => "", "empty" => ""
-    request.query_param_list.must_equal [["foo", "bar"], ["quux", "bla"], ["nothing", nil], ["empty", ""]]
+    request.params.must_equal "foo" => "bar", "quux" => "bla", "nothing" => nil, "empty" => ""
   end
 
   it "handles invalid unicode in query string value" do
@@ -1554,19 +1553,16 @@ EOF
     rack_input.write(input)
     rack_input.rewind
 
-    form_hash_cache = {}
+    form_hash = {}
 
     req = make_request Rack::MockRequest.env_for(
       "/",
-      "rack.request.form_hash" => form_hash_cache,
+      "rack.request.form_hash" => form_hash,
       "rack.request.form_input" => rack_input,
       :input => rack_input
     )
 
-    form_hash = {'foo' => 'bar'}.freeze
-    form_hash_cache[req.cache_key] = form_hash
-
-    req.POST.must_equal form_hash
+    req.POST.must_be_same_as form_hash
   end
 
   it "conform to the Rack spec" do
@@ -1964,53 +1960,4 @@ EOF
       DelegateRequest.new super(env)
     end
   end
-
-  class UpperRequest < Rack::Request
-    def expand_params(parameters)
-      parameters.map do |(key, value)|
-        [key.upcase, value]
-      end.to_h
-    end
-
-    # If this is not specified, the behaviour becomes order dependent.
-    def cache_key
-      :my_request
-    end
-  end
-
-  it "correctly expands parameters" do
-    env = {"QUERY_STRING" => "foo=bar"}
-
-    request = Rack::Request.new(env)
-    request.query_param_list.must_equal [["foo", "bar"]]
-    request.GET.must_equal "foo" => "bar"
-
-    upper_request = UpperRequest.new(env)
-    upper_request.query_param_list.must_equal [["foo", "bar"]]
-    upper_request.GET.must_equal "FOO" => "bar"
-
-    env['QUERY_STRING'] = "foo=bar&bar=baz"
-
-    request.GET.must_equal "foo" => "bar", "bar" => "baz"
-    upper_request.GET.must_equal "FOO" => "bar", "BAR" => "baz"
-  end
-
-  class BrokenRequest < Rack::Request
-    def expand_params(parameters)
-      raise "boom"
-    end
-  end
-
-  it "raises an error if expand_params raises an error" do
-    env = {"QUERY_STRING" => "foo=bar"}
-
-    request = Rack::Request.new(env)
-    request.GET.must_equal "foo" => "bar"
-
-    broken_request = BrokenRequest.new(env)
-    lambda { broken_request.GET }.must_raise RuntimeError
-
-    # Subsequnt calls also raise an error:
-    lambda { broken_request.GET }.must_raise RuntimeError
-  end
 end
diff --git a/test/spec_utils.rb b/test/spec_utils.rb
index b771eafa..64e070ed 100644
--- a/test/spec_utils.rb
+++ b/test/spec_utils.rb
@@ -141,7 +141,7 @@ describe Rack::Utils do
 
   it "parse nested query strings correctly" do
     Rack::Utils.parse_nested_query("foo").
-      must_equal "foo" => ""
+      must_equal "foo" => nil
     Rack::Utils.parse_nested_query("foo=").
       must_equal "foo" => ""
     Rack::Utils.parse_nested_query("foo=bar").
@@ -158,7 +158,7 @@ describe Rack::Utils do
     Rack::Utils.parse_nested_query("&foo=1&&bar=2").
       must_equal "foo" => "1", "bar" => "2"
     Rack::Utils.parse_nested_query("foo&bar=").
-      must_equal "foo" => "", "bar" => ""
+      must_equal "foo" => nil, "bar" => ""
     Rack::Utils.parse_nested_query("foo=bar&baz=").
       must_equal "foo" => "bar", "baz" => ""
     Rack::Utils.parse_nested_query("my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F").
@@ -168,19 +168,19 @@ describe Rack::Utils do
       must_equal "pid=1234" => "1023", "a" => "b"
 
     Rack::Utils.parse_nested_query("foo[]").
-      must_equal "foo" => [""]
+      must_equal "foo" => [nil]
     Rack::Utils.parse_nested_query("foo[]=").
       must_equal "foo" => [""]
     Rack::Utils.parse_nested_query("foo[]=bar").
       must_equal "foo" => ["bar"]
     Rack::Utils.parse_nested_query("foo[]=bar&foo").
-      must_equal "foo" => ""
+      must_equal "foo" => nil
     Rack::Utils.parse_nested_query("foo[]=bar&foo[").
-      must_equal "foo" => ["bar"], "foo[" => ""
+      must_equal "foo" => ["bar"], "foo[" => nil
     Rack::Utils.parse_nested_query("foo[]=bar&foo[=baz").
       must_equal "foo" => ["bar"], "foo[" => "baz"
     Rack::Utils.parse_nested_query("foo[]=bar&foo[]").
-      must_equal "foo" => ["bar", ""]
+      must_equal "foo" => ["bar", nil]
     Rack::Utils.parse_nested_query("foo[]=bar&foo[]=").
       must_equal "foo" => ["bar", ""]
 
@@ -192,7 +192,7 @@ describe Rack::Utils do
       must_equal "foo" => ["bar"], "baz" => ["1", "2", "3"]
 
     Rack::Utils.parse_nested_query("x[y][z]").
-      must_equal "x" => { "y" => { "z" => "" } }
+      must_equal "x" => { "y" => { "z" => nil } }
     Rack::Utils.parse_nested_query("x[y][z]=1").
       must_equal "x" => { "y" => { "z" => "1" } }
     Rack::Utils.parse_nested_query("x[y][z][]=1").