Add Rack::Test::Utils.override_build_nested_query

[jdufresne]

The new Rack::Test::Utils.override_build_nested_query allows library users to make rack-test reuse Rack::Utils#build_nested_query instead of replacing it.

The rack package already provides a Rack::Utils#build_nested_query. So the rack-test doesn't need to override or re-implement it. By reusing the one from rack, this package will receive improvements as they are made there.

For example, the upstream method has improved URL encoding since commit:
rack/rack@10864b6

[mpalmer]

This seems like it'll end really, really badly at some point in the future, when Rack gets to major version 10:

>> '3.2.1' < '3.1.0'
=> false
>> "10.0.0" < '3.1.0'
=> true

[jdufresne]

Will fix. FWIW, I copied this pattern from elsewhere in the code:

lib/rack/test.rb
11     if Rack.release >= '2.3'
285        unless Rack.release >= '2.3'
376        Rack.release >= '1.6'

[jdufresne]

@mpalmer Thanks for the review. This is now using Gem::Version in commit:

• fixup! Reuse Rack::Utils#build_nested_query

[mpalmer]

This (and the other changes to the test suite) suggest that the Rack::Utils#build_nested_query does not produce the same results as the existing method, which means, in theory at least, that adopting this change requires a major version bump to rack-test. Is that your understanding also, and if not, why not?

[jdufresne]

Yes that is right. This is backward incompatible change and I agree with bumping the major version. A very similar discussion came up when this was fixed in rack: rack/rack#1989

[jeremyevans]

As this would be a breaking change, it can't be considered until rack-test 3. However, there are no plans to release rack-test 3 in the near future. Is there evidence that this is causing issues for rack-test users? Unlike in Rack, usage of build_nested_query in rack-test is not designed for consumption by browsers, but directly by rack applications. Are there any frameworks/middleware where the current behavior is problematic?

If we are going to reuse Rack::Utils#build_nested_query, we should do so in all cases, we shouldn't force the newer Rack::Utils#build_nested_query behavior on applications using an older Rack version.

[jdufresne]

If we are going to reuse Rack::Utils#build_nested_query, we should do so in all cases, we shouldn't force the newer Rack::Utils#build_nested_query behavior on applications using an older Rack version.

That is good with me. I've made this change in commit:

• Remove fallback for older Rack versions

[jdufresne]

Thanks for the review and considering my change.

Is there evidence that this is causing issues for rack-test users?

Are there any frameworks/middleware where the current behavior is problematic?

Yes. The old behavior is causing direct issues in my projects, which motivated me to address this first in Rack and now in rack-test.

In particular, my projects serialize HTTP requests and responses generated by Rails (using Rack and rack-test) so they can be consumed by other tools. The gem I use to generate these serialized responses is:

rails-response-dumper: https://github.com/Pioneer-Valley-Books/rails-response-dumper

I've observed that URLs generated by rails-response-dumper do not encode GET query strings according to specification. For example, before this PR, rack-test does not percent-encode square brackets in nested hash parameters.

As a result, tools consuming these URLs require workarounds or extra code to align behavior with expectations. While these workarounds aren't catastrophic by any means, they are unnecessary friction -- fixing the issue at its source would be a more reliable solution. So, I am a direct user who would benefit from this change.

As this would be a breaking change, it can't be considered until rack-test 3. However, there are no plans to release rack-test 3 in the near future.

I understand and that's fine with me. If nothing else, this PR could help start the discussion around what a rack-test 3 release might look like. The current release has been very stable -- there hasn't been a new release in nearly a year -- which gives users flexibility. Those affected by this change can upgrade and adapt their code at their own pace once rack-test 3 is released, while continuing to use rack-test 2 in the meantime.

If rack-test 3 requires additional changes to move ahead (for example, removing some old Ruby and Rack supported versions) I can volunteer some time.

[jeremyevans]

I don't expect to release rack-test 3 until there are significant enough changes that the benefits of a major version bump are worth the cost. That seems unlikely to happen in the near future. I can't predict the future, but it could be many years away.

I'm OK to accept these changes in a new rack-test-3 branch. Just be aware that the odds of it getting released anytime soon are low.

It seems like with the current implementation, you can get the behavior you want in one line (Rack::Test::Utils.send(:remove_method, :build_nested_query)). So the workaround to get what you want is trivial.

Note that I think we can remove all but one of the build_nested_query-specific tests if we don't have a custom implementation. A single test with {"a" => "b"} resulting in a=b is sufficient. The purpose of the tests is not to show how Rack's behavior for the method has changed over time.

[jeremyevans]

Actually, I thought of a better way to handle this. Make the behavior opt-in. Add a top-level accessor (Rack::Test::Utils.override_build_nested_query), defaulting to false. At the top of Rack::Test::Utils#build_nested_query, do return super if Rack::Test::Utils.override_build_nested_query. At one test with the accessor set to true, with {"a" => "b"} resulting in a=b. That can be accepted and released. Then you get the behavior you want in a supported way, and when we do release rack-test 3, we can default the accessor to true instead of false.

[jdufresne]

Actually, I thought of a better way to handle this. Make the behavior opt-in. Add a top-level accessor (Rack::Test::Utils.override_build_nested_query), defaulting to false. At the top of Rack::Test::Utils#build_nested_query, do return super if Rack::Test::Utils.override_build_nested_query. At one test with the accessor set to true, with {"a" => "b"} resulting in a=b. That can be accepted and released. Then you get the behavior you want in a supported way, and when we do release rack-test 3, we can default the accessor to true instead of false.

I implemented this suggestion in:

• Add Rack::Test::Utils.override_build_nested_query

And squashed the commits.

[jeremyevans]

Looks good, thank you for working on this!