Make QueryParser::Params a Hash subclass

Because of the to_params_hash method, this cannot be Hash itself.
We should consider deprecating that method and maybe the Params
constant as well.

Most of the spec changes are just changing overrides of
Params#initialize.

This tweaks the "not create infinite loops with cycle structures"
spec, because it wasn't actually testing for loops, it was
checking the string representation was the same between Hash
and Params.  This spec could probably be eliminated, but the
tweak allows it to pass by checking that the level 1 hash string
representation is the same as the level 2 hash string representation.

4 files changed, 7 insertions, 65 deletions

diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb
index f71bd765..1592a01e 100644
--- a/lib/rack/query_parser.rb
+++ b/lib/rack/query_parser.rb
@@ -194,59 +194,7 @@ module Rack
       Utils.unescape(s)
     end
 
-    class Params
-      def initialize
-        @size   = 0
-        @params = {}
-      end
-
-      def [](key)
-        @params[key]
-      end
-
-      def []=(key, value)
-        @params[key] = value
-      end
-
-      def key?(key)
-        @params.key?(key)
-      end
-
-      # Recursively unwraps nested `Params` objects and constructs an object
-      # of the same shape, but using the objects' internal representations
-      # (Ruby hashes) in place of the objects. The result is a hash consisting
-      # purely of Ruby primitives.
-      #
-      #   Mutation warning!
-      #
-      #   1. This method mutates the internal representation of the `Params`
-      #      objects in order to save object allocations.
-      #
-      #   2. The value you get back is a reference to the internal hash
-      #      representation, not a copy.
-      #
-      #   3. Because the `Params` object's internal representation is mutable
-      #      through the `#[]=` method, it is not thread safe. The result of
-      #      getting the hash representation while another thread is adding a
-      #      key to it is non-deterministic.
-      #
-      def to_h
-        @params.each do |key, value|
-          case value
-          when self
-            # Handle circular references gracefully.
-            @params[key] = @params
-          when Params
-            @params[key] = value.to_h
-          when Array
-            value.map! { |v| v.kind_of?(Params) ? v.to_h : v }
-          else
-            # Ignore anything that is not a `Params` object or
-            # a collection that can contain one.
-          end
-        end
-        @params
-      end
+    class Params < Hash
       alias_method :to_params_hash, :to_h
     end
   end
diff --git a/test/spec_multipart.rb b/test/spec_multipart.rb
index 3149dd6b..14d0f6a5 100644
--- a/test/spec_multipart.rb
+++ b/test/spec_multipart.rb
@@ -256,8 +256,7 @@ describe Rack::Multipart do
   it "accept the params hash class to use for multipart parsing" do
     c = Class.new(Rack::QueryParser::Params) do
       def initialize(*)
-        super
-        @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)}
+        super(){|h, k| h[k.to_s] if k.is_a?(Symbol)}
       end
     end
     query_parser = Rack::QueryParser.new c, 100
diff --git a/test/spec_request.rb b/test/spec_request.rb
index 295abf95..f4e940ce 100644
--- a/test/spec_request.rb
+++ b/test/spec_request.rb
@@ -609,8 +609,7 @@ class RackRequestTest < Minitest::Spec
   it "should use the query_parser for query parsing" do
     c = Class.new(Rack::QueryParser::Params) do
       def initialize(*)
-        super
-        @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)}
+        super(){|h, k| h[k.to_s] if k.is_a?(Symbol)}
       end
     end
     parser = Rack::QueryParser.new(c, 100)
@@ -676,8 +675,7 @@ class RackRequestTest < Minitest::Spec
   it "use the query_parser's params_class for multipart params" do
     c = Class.new(Rack::QueryParser::Params) do
       def initialize(*)
-        super
-        @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)}
+        super(){|h, k| h[k.to_s] if k.is_a?(Symbol)}
       end
     end
     parser = Rack::QueryParser.new(c, 100)
diff --git a/test/spec_utils.rb b/test/spec_utils.rb
index e88311aa..b771eafa 100644
--- a/test/spec_utils.rb
+++ b/test/spec_utils.rb
@@ -119,12 +119,10 @@ describe Rack::Utils do
   end
 
   it "not create infinite loops with cycle structures" do
-    ex = { "foo" => nil }
-    ex["foo"] = ex
-
     params = Rack::Utils::KeySpaceConstrainedParams.new
     params['foo'] = params
-    params.to_params_hash.to_s.must_equal ex.to_s
+    h = params.to_params_hash
+    h['foo'].to_s.must_equal h['foo']['foo'].to_s
   end
 
   it "parse nil as an empty query string" do
@@ -277,8 +275,7 @@ describe Rack::Utils do
       default_parser = Rack::Utils.default_query_parser
       param_parser_class = Class.new(Rack::QueryParser::Params) do
         def initialize(*)
-          super
-          @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)}
+          super(){|h, k| h[k.to_s] if k.is_a?(Symbol)}
         end
       end
       Rack::Utils.default_query_parser = Rack::QueryParser.new(param_parser_class, 100)