Switch to using FormEncodedPairParser for parsing request parameters.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4866 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
2026-04-04 03:00:58 -04:00 · 2006-08-30 05:50:02 +00:00
parent 9ece976a99
commit d34a346d9d
4 changed files with 76 additions and 132 deletions
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*

+* Switch to using FormEncodedPairParser for parsing request parameters. [Nicholas Seckar, DHH]
+
 * respond_to .html now always renders #{action_name}.rhtml so that registered custom template handlers do not override it in priority. Custom mime types require a block and throw proper error now. [Tobias Luetke]

 * Deprecation: test deprecated instance vars in partials. [Jeremy Kemper]
--- a/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb
+++ b/actionpack/lib/action_controller/cgi_ext/cgi_methods.rb
@@ -8,31 +8,39 @@ class CGIMethods #:nodoc:
  class << self
    # DEPRECATED: Use parse_form_encoded_parameters
    def parse_query_parameters(query_string)
-      parse_form_encoded_parameters(query_string)
+      pairs = query_string.split('&').collect do |chunk|
+        next if chunk.empty?
+        key, value = chunk.split('=', 2)
+        value = (value.nil? || value.empty?) ? nil : CGI.unescape(value)
+        [ key, value ]
+      end.compact
+
+      FormEncodedPairParser.new(pairs).result
    end

    # DEPRECATED: Use parse_form_encoded_parameters
    def parse_request_parameters(params)
-      parsed_params = {}
+      parser = FormEncodedPairParser.new

-      for key, value in params
-        next unless key
-        value = [value] if key =~ /.*\[\]$/
-        unless key.include?('[')
-          # much faster to test for the most common case first (GET)
-          # and avoid the call to build_deep_hash
-          parsed_params[key] = get_typed_value(value[0])
-        else
-          build_deep_hash(get_typed_value(value[0]), parsed_params, get_levels(key))
+      finished = false
+      until finished
+        finished = true
+        for key, value in params
+          next unless key
+          if !key.include?('[')
+            # much faster to test for the most common case first (GET)
+            # and avoid the call to build_deep_hash
+            parser.result[key] = get_typed_value(value[0])
+          elsif value.is_a?(Array)
+            parser.parse(key, get_typed_value(value.shift))
+            finished = false unless value.empty?
+          else
+            raise TypeError, "Expected array, found #{value.inspect}"
+          end
        end
      end
    
-      parsed_params
-    end
-    
-    # TODO: Docs
-    def parse_form_encoded_parameters(form_encoded_string)
-      FormEncodedStringScanner.decode(form_encoded_string)
+      parser.result
    end

    def parse_formatted_request_parameters(mime_type, raw_post_data)
@@ -93,99 +101,40 @@ class CGIMethods #:nodoc:
          value.to_s
        end
      end
-  
-      PARAMS_HASH_RE = /^([^\[]+)(\[.*\])?(.)?.*$/
-      def get_levels(key)
-        all, main, bracketed, trailing = PARAMS_HASH_RE.match(key).to_a
-        if main.nil?
-          []
-        elsif trailing
-          [key]
-        elsif bracketed
-          [main] + bracketed.slice(1...-1).split('][')
-        else
-          [main]
-        end
-      end
-
-      def build_deep_hash(value, hash, levels)
-        if levels.length == 0
-          value
-        elsif hash.nil?
-          { levels.first => build_deep_hash(value, nil, levels[1..-1]) }
-        else
-          hash.update({ levels.first => build_deep_hash(value, hash[levels.first], levels[1..-1]) })
-        end
-      end
  end

-  class FormEncodedStringScanner < StringScanner
+  class FormEncodedPairParser < StringScanner
    attr_reader :top, :parent, :result

-    def self.decode(form_encoded_string)
-      new(form_encoded_string).parse
+    def initialize(pairs = [])
+      super('')
+      @result = {}
+      pairs.each { |key, value| parse(key, value) }
    end
-
-    def initialize(form_encoded_string)
-      super(unescape_keys(form_encoded_string))
-    end
-    
+     
    KEY_REGEXP = %r{([^\[\]=&]+)}
    BRACKETED_KEY_REGEXP = %r{\[([^\[\]=&]+)\]}
    
-    
-    def parse
-      @result = {}
-
-      until eos?
-        # Parse each & delimited chunk
-        @parent, @top = nil, result
-        
-        # First scan the bare key
-        key = scan(KEY_REGEXP) or (skip_term and next)
-        key = post_key_check(key)
-        
-        # Then scan as many nestings as present
-        until check(/\=/) || eos? 
-          r = scan(BRACKETED_KEY_REGEXP) or (skip_term and break)
-          key = self[1]
-          key = post_key_check(key)
-        end
-        
-        # Scan the value if we see an =
-        if scan %r{=}
-          value = scan(/[^\&]+/) # scan_until doesn't handle \Z
-          value = CGI.unescape(value) if value # May be nil when eos?
-          bind(key, value)
-        end
-
-        scan(%r/\&+/) # Ignore multiple adjacent &'s
-      end
+    # Parse the query string
+    def parse(key, value)
+      self.string = key
+      @top, @parent = result, nil
      
-      return result
+      # First scan the bare key
+      key = scan(KEY_REGEXP) or return
+      key = post_key_check(key)
+            
+      # Then scan as many nestings as present
+      until eos? 
+        r = scan(BRACKETED_KEY_REGEXP) or return
+        key = self[1]
+        key = post_key_check(key)
+      end
+ 
+      bind(key, value)
    end

    private
-      # Turn keys like person%5Bname%5D into person[name], so they can be processed as hashes
-      def unescape_keys(query_string)
-        query_string.split('&').collect do |fragment|
-          key, value = fragment.split('=', 2)
-          
-          if key
-            key = key.gsub(/%5D/, ']').gsub(/%5B/, '[')
-            [ key, value ].join("=")
-          else
-            fragment
-          end
-        end.join('&')
-      end
-
-      # Skip over the current term by scanning past the next &, or to
-      # then end of the string if there is no next &
-      def skip_term
-        scan_until(%r/\&+/) || scan(/.+/)
-      end
-    
      # After we see a key, we must look ahead to determine our next action. Cases:
      # 
      #   [] follows the key. Then the value must be an array.
@@ -193,16 +142,13 @@ class CGIMethods #:nodoc:
      #   & or the end of string follows the key. Then the key is a flag.
      #   otherwise, a hash follows the key. 
      def post_key_check(key)
-        if eos? || check(/\&/) # a& or a\Z indicates a is a flag.
-          bind key, nil # Curiously enough, the flag's value is nil
-          nil
-        elsif scan(/\[\]/) # a[b][] indicates that b is an array
-          container key, Array
+        if scan(/\[\]/) # a[b][] indicates that b is an array
+          container(key, Array)
          nil
        elsif check(/\[[^\]]/) # a[b] indicates that a is a hash
-          container key, Hash
+          container(key, Hash)
          nil
-        else # Presumably an = sign is next.
+        else # End of key? We do nothing.
          key
        end
      end
@@ -212,8 +158,8 @@ class CGIMethods #:nodoc:
      def container(key, klass)
        raise TypeError if top.is_a?(Hash) && top.key?(key) && ! top[key].is_a?(klass)
        value = bind(key, klass.new)
-        raise TypeError unless value.is_a? klass
-        push value
+        raise TypeError unless value.is_a?(klass)
+        push(value)
      end
    
      # Push a value onto the 'stack', which is actually only the top 2 items.
@@ -236,13 +182,12 @@ class CGIMethods #:nodoc:
          end
        elsif top.is_a? Hash
          key = CGI.unescape(key)
-          if top.key?(key) && parent.is_a?(Array)
-            parent << (@top = {})
-          end
+          parent << (@top = {}) if top.key?(key) && parent.is_a?(Array)
          return top[key] ||= value
        else
-          # Do nothing?
+          raise ArgumentError, "Don't know what to do: top is #{top.inspect}"
        end
+
        return value
      end
    end
--- a/actionpack/lib/action_controller/cgi_process.rb
+++ b/actionpack/lib/action_controller/cgi_process.rb
@@ -145,7 +145,7 @@ module ActionController #:nodoc:
          begin
            Module.const_missing($1)
          rescue LoadError, NameError => const_error
-            raise ActionController::SessionRestoreError, <<end_msg
+            raise ActionController::SessionRestoreError, <<-end_msg
 Session contains objects whose class definition isn\'t available.
 Remember to require the classes for all objects kept in the session.
 (Original exception: #{const_error.message} [#{const_error.class}])
@@ -207,4 +207,4 @@ end_msg
        end
      end
  end
-end
+end
--- a/actionpack/test/controller/cgi_test.rb
+++ b/actionpack/test/controller/cgi_test.rb
@@ -16,12 +16,7 @@ class CGITest < Test::Unit::TestCase
    @query_string_with_many_equal = "action=create_customer&full_name=abc=def=ghi"
    @query_string_without_equal = "action"
    @query_string_with_many_ampersands =
-      "&action=create_customer&&&full_name=David%20Heinemeier%20Hansson"
-    @query_string_with_escaped_brackets = "subscriber%5Bfirst_name%5D=Jan5&subscriber%5B\
-last_name%5D=Waterman&subscriber%5Bemail%5D=drakn%40domain.tld&subscriber%5Bpassword\
-%5D=893ea&subscriber%5Bstreet%5D=&subscriber%5Bzip%5D=&subscriber%5Bcity%5D\
-=&commit=Update+info"
-    
+      "&action=create_customer&&&full_name=David%20Heinemeier%20Hansson"    
  end

  def test_query_string
@@ -34,15 +29,6 @@ last_name%5D=Waterman&subscriber%5Bemail%5D=drakn%40domain.tld&subscriber%5Bpass
  def test_deep_query_string
    expected = {'x' => {'y' => {'z' => '10'}}}
    assert_equal(expected, CGIMethods.parse_query_parameters('x[y][z]=10'))
-    expected = {"commit"=>"Update info", 
-      "subscriber" => {
-          "city" => nil, "zip"=>nil,
-          "last_name" => "Waterman", "street" => nil,
-          "password" =>"893ea", "first_name" =>  "Jan5" ,
-          "email" => "drakn@domain.tld"
-      }
-    }
-    assert_equal(expected, CGIMethods.parse_query_parameters(@query_string_with_escaped_brackets))
  end
  
  def test_deep_query_string_with_array
@@ -61,6 +47,17 @@ last_name%5D=Waterman&subscriber%5Bemail%5D=drakn%40domain.tld&subscriber%5Bpass
    assert_equal("10", CGIMethods.parse_query_parameters('x[y][][z]=10&x[y][][z]=20').with_indifferent_access[:x][:y].first[:z])
  end
  
+  def test_request_hash_parsing
+    query = {
+      "note[viewers][viewer][][type]" => ["User", "Group"],
+      "note[viewers][viewer][][id]"   => ["1", "2"]
+    }
+
+    expected = { "note" => { "viewers"=>{"viewer"=>[{ "id"=>"1", "type"=>"User"}, {"type"=>"Group", "id"=>"2"} ]} } }
+
+    assert_equal(expected, CGIMethods.parse_request_parameters(query))
+  end
+  
  def test_deep_query_string_with_array_of_hashes_with_multiple_pairs
    assert_equal(
      {'x' => {'y' => [{'z' => '10', 'w' => 'a'}, {'z' => '20', 'w' => 'b'}]}}, 
@@ -239,25 +236,25 @@ last_name%5D=Waterman&subscriber%5Bemail%5D=drakn%40domain.tld&subscriber%5Bpass

  def test_parse_params_with_single_brackets_in_middle
    input     = { "a/b[c]d" =>  %w(e) }
-    expected  = { "a/b[c]d" => "e" }
+    expected  = { "a/b" => {} }
    assert_equal expected, CGIMethods.parse_request_parameters(input)
  end

  def test_parse_params_with_separated_brackets
    input     = { "a/b@[c]d[e]" =>  %w(f) }
-    expected  = { "a/b@" => { "c]d[e" => "f" }}
+    expected  = { "a/b@" => { }}
    assert_equal expected, CGIMethods.parse_request_parameters(input)
  end

  def test_parse_params_with_separated_brackets_and_array
    input     = { "a/b@[c]d[e][]" =>  %w(f) }
-    expected  = { "a/b@" => { "c]d[e" => ["f"] }}
+    expected  = { "a/b@" => { }}
    assert_equal expected , CGIMethods.parse_request_parameters(input)
  end

  def test_parse_params_with_unmatched_brackets_and_array
    input     = { "a/b@[c][d[e][]" =>  %w(f) }
-    expected  = { "a/b@" => { "c" => { "d[e" => ["f"] }}}
+    expected  = { "a/b@" => { "c" => { }}}
    assert_equal expected, CGIMethods.parse_request_parameters(input)
  end