[PATCH] fix parameter parsing bug affecting hashes nested in arrays

Attaching a patch for a test to reproduce this issue: apply with "patch -p1 <1079-001.patch"

Investigating UriEncodedPairParser, I would expect to see the following for the input "a[][b][c]=d":

0. "a"                 DATA {}
1. "[]"                DATA {"a" => []}
2. "[b]"               DATA {"a" => [{}]}
3. "[c]"               DATA {"a" => [{"b" => {}}]}
4. EOS                 DATA {"a" => [{"b" => {"c" => "d"}}]}

Instead, I see something more like:

0. "a"                 DATA {}
1. "[]"                DATA {"a" => []}
2. "[b]"               DATA {"a" => [{}]}
3. "[c]"               DATA {"a" => [{"b" => {}}]}
4. EOS                 DATA {"a" => [{"b" => {}}]}

Weird stuff.

After a bit of debugging I discovered an easy-to-overlook bug in UrlEncodedPairParser#bind.

It's not immediately obvious at a glance, but when adding the first hash following the array for the earlier example of a[][b][c]:

top << {key => value}.with_indifferent_access

This actually stores a copy of value as a HashWithIndifferentAccess in the array. value is not updated to reflect the real object that is now in the array, and gets passed back to container and pushed to the top of the stack - everything added to this hash will be lost.

I'll attach a fix for the issue shortly. Can I get somebody to verify/review this?

Updated patch with a working fix and unit test: apply with "patch -p1 <10797-002.patch"

Simplified the fix ever so slightly: apply with "patch -p1 <10797-003.patch"

+1

See #9030 (a duplicate of this issue) for further discussion of this issue, as well as some rationale for rejecting changes to existing semantics outside of the UriEncodedPairParser.

Added an extra unit test for complex parsing of nested form parameters (apply with patch -p1 <10797-004.patch)

I committed patch #3 in [9010]. I'm leaving this open since the last test case in patch #4 fails, highlighting another complex parsing issue.

Fix failing unit test in patch 4: more weirdness due to #with_indifferent_access

It seems the real culprit here is the #with_indifferent_access call. One can safely remove this call, remove all other fixes I've provided here and all unit tests will pass. I have a feeling that doing this will actually break other code unless there's another pass performing the Hash -> HashWithIndifferentAccess conversion higher up.

For now I'm not going to investigate further, since the immediate bug is fixed.

As an interesting aside, the following line can be removed without breaking any unit tests. Does anybody know what it's supposed to be doing? I initially thought the ordinary Hash semantics may have been at play here, but it turns out the unit tests never seem to execute the #<<:

parent << (@top = {}) if top.key?(key) && parent.is_a?(Array)

(In [9020]) Fix more obscure nested parameter hash parsing bug. Closes #10797 [thomas.lee]