improved rack-compatibility for our "pure" servlet-env request env parsing impl

... we'll now raise a TypeError just like rack does when it detects invalid parameters

Author: kares

src/main/ruby/rack/handler/servlet/servlet_env.rb

@@ -50,6 +50,12 @@ def load_env_key(env, key)
         # @private
         POST_PARAM_METHODS = [ 'POST', 'PUT', 'DELETE' ].freeze
 
+        if defined? Rack::Utils::ParameterTypeError
+          ParameterTypeError = Rack::Utils::ParameterTypeError
+        else
+          ParameterTypeError = TypeError
+        end
+
         # Load parameters into the (Rack) env from the Servlet API.
         # using javax.servlet.http.HttpServletRequest#getParameterMap
         def load_parameters
@@ -65,7 +71,7 @@ def load_parameters
           @servlet_env.getParameterMap.each do |key, val| # String, String[]
             val = [''] if val.nil? # e.g. buggy Jetty 6
             val = [''] if val.length == 1 && val[0].nil?
-            
+
             if ( q_vals = query_values(key) ) || get_only
               if q_vals.length != val.length
                 # some are GET params some POST params
@@ -93,6 +99,20 @@ def load_parameters
           @env[ FORM_HASH ] = form_hash
         end
 
+        def [](key)
+          value = super(key)
+          if key.eql? QUERY_HASH
+            if @parameter_error ||= nil
+              raise @parameter_error
+            end
+          end
+          value
+        end
+        public :[]
+
+        # @private
+        KEY_SEP = /([^\[\]]*)(?:\[(.*?)\])?/
+
         # Store the parameter into the given Hash.
         # By default this is performed in a Rack compatible way and thus
         # some parameter values might get "lost" - it only accepts multiple
@@ -102,12 +122,33 @@ def load_parameters
         # @param val the value(s) in a array-like structure
         # @param hash the Hash to store the name, value pair
         def store_parameter(key, val, hash)
-          # Rack::Utils.parse_nested_query behaviour
-          # for 'foo=bad&foo=bar' does { 'foo' => 'bar' }
-          if key[-2, 2] == '[]' # foo[]=f1&foo[]=f2
-            hash[ key[0...-2] ] = val.to_a # String[]
+          # emulating Rack::Utils.parse_nested_query behavior
+
+          if match = key.match(KEY_SEP)
+            n_key = match[1]; sub = match[2]
           else
-            hash[ key ] = val[ val.length - 1 ] # last
+            n_key = key; sub = nil # normalized-key[ sub-key ]
+          end
+
+          if sub
+            if sub.empty? # e.g. foo[]=1&foo[]=2
+              if arr = hash[ n_key ]
+                return mark_parameter_error "expected Array (got #{arr.class}) for param `#{n_key}'" unless arr.is_a?(Array)
+                hash[ n_key ] = arr + val.to_a; return
+              end
+              hash[ n_key ] = val.to_a # String[]
+            else # foo[bar]=rrr&foo[baz]=zzz
+              v = val[ val.length - 1 ] # last
+              if hsh = hash[ n_key ]
+                return mark_parameter_error "expected Hash (got #{hsh.class}) for param `#{n_key}'" unless hsh.is_a?(Hash)
+                hsh[ sub ] = v
+              else
+                hash[ n_key ] = { sub => v }
+              end
+            end
+          else
+            # for 'foo=bad&foo=bar' does { 'foo' => 'bar' }
+            hash[ n_key ] = val[ val.length - 1 ] # last
           end
         end
 
@@ -149,6 +190,12 @@ def parse_query_string
           Java::JavaxServletHttp::HttpUtils.parseQueryString(query_string)
         end
 
+        def mark_parameter_error(msg)
+          raise ParameterTypeError, msg
+        rescue ParameterTypeError => e
+          @parameter_error = e
+        end
+
       end
     end
   end

src/spec/ruby/rack/handler/servlet_spec.rb

@@ -368,6 +368,33 @@ def getAttributeNames
       expect( env['org.apache.internal'] ).to be true
     end
 
+    it "raises if nested request parameters are broken (Rack-compat)" do
+      servlet_request = @servlet_request
+      servlet_request.setMethod 'GET'
+      servlet_request.setContextPath '/'
+      servlet_request.setPathInfo '/path'
+      servlet_request.setRequestURI '/home/path'
+      servlet_request.setQueryString 'foo[]=0&foo[bar]=1'
+      servlet_request.addParameter('foo[]', '0')
+      servlet_request.addParameter('foo[bar]', '1')
+
+      env = servlet.create_env(@servlet_env)
+      rack_request = Rack::Request.new(env)
+
+      # Rack::Utils::ParameterTypeError (< TypeError) since 1.6.0
+      if Rack::Utils.const_defined? :ParameterTypeError
+        error = Rack::Utils::ParameterTypeError
+      else
+        error = TypeError
+      end
+
+      expect { rack_request.GET }.to raise_error(error, "expected Hash (got Array) for param `foo'")
+      rack_request.POST.should == {}
+      expect { rack_request.params }.to raise_error(error, "expected Hash (got Array) for param `foo'") if rack_release('1.6')
+
+      rack_request.query_string.should == 'foo[]=0&foo[bar]=1'
+    end
+
   end
 
   shared_examples "(eager)rack-env" do