Rack3 breaking change with query param nil

[ericproulx]

While updating rack-test locally to the latest version, rack got updated also to it's latest version 3.0.3

Some tests are failing due to the following breaking change :

BREAKING CHANGE: Query parsing now treats parameters without = as having the empty string value instead of nil value, to conform to the URL spec.

What does it means ? Passing a query params like ?name will be considered by Rack (>=3) as { "name" => "" } instead { "name" => nil }.

Moslty, it affects all the optional parameters that you can pass param_name: nil.

Question? Since it's a breaking change, should we update the gemspec to keep rack < 3. That's one breaking change, but there might be others since other specs are falling.

Tests failing:

rspec ./spec/grape/validations/validators/regexp_spec.rb:114 # Grape::Validations::Validators::RegexpValidator accepts nil
rspec ./spec/grape/validations/validators/regexp_spec.rb:59 # Grape::Validations::Validators::RegexpValidator custom validation message accepts nil
rspec ./spec/grape/validations/validators/regexp_spec.rb:82 # Grape::Validations::Validators::RegexpValidator custom validation message regexp with array refuses nil items
rspec ./spec/grape/validations_spec.rb:36 # Grape::Validations params optional doesn't validate when param not present
rspec './spec/grape/validations/validators/default_spec.rb[1:12:2:1]' # Grape::Validations::Validators::DefaultValidator optional with nil as value structures types respects the default value
rspec './spec/grape/validations/validators/default_spec.rb[1:12:2:2]' # Grape::Validations::Validators::DefaultValidator optional with nil as value structures types respects the default value
rspec './spec/grape/validations/validators/default_spec.rb[1:12:1:15]' # Grape::Validations::Validators::DefaultValidator optional with nil as value primitive types respects the default value
rspec ./spec/grape/validations/validators/coerce_spec.rb:212 # Grape::Validations::Validators::CoerceValidator coerce coerces String
rspec './spec/grape/validations/validators/coerce_spec.rb[1:1:5:11:2:1]' # Grape::Validations::Validators::CoerceValidator coerce coerces nil values structures types respects the nil value
rspec './spec/grape/validations/validators/coerce_spec.rb[1:1:5:11:1:9]' # Grape::Validations::Validators::CoerceValidator coerce coerces nil values primitive types respects the nil value
rspec ./spec/grape/validations/validators/coerce_spec.rb:724 # Grape::Validations::Validators::CoerceValidator coerce using coerce_with Array type and coerce_with should coerce nil value to array

Othere tests failing with Rack > 3

rspec ./spec/grape/middleware/formatter_spec.rb:403 # Grape::Middleware::Formatter send file returns a file response
rspec ./spec/grape/endpoint_spec.rb:139 # Grape::Endpoint#headers includes request headers
rspec ./spec/grape/endpoint_spec.rb:195 # Grape::Endpoint#cookies sets and update browser cookies
rspec ./spec/grape/endpoint_spec.rb:206 # Grape::Endpoint#cookies deletes cookie
rspec ./spec/grape/endpoint_spec.rb:162 # Grape::Endpoint#cookies is callable from within a block
rspec ./spec/grape/endpoint_spec.rb:230 # Grape::Endpoint#cookies deletes cookies with path
rspec ./spec/grape/endpoint_spec.rb:427 # Grape::Endpoint#params from body parameters returns a 400 if given an invalid multipart body

[dblock]

I think this (requiring Rack 3.0+) is a breaking 2.0 change, and should probably release 1.7.1 before merging it.

The other alternative is to support both, not do a major version increment, and have clearly separate specs for Rack 2.x and Rack 3.x and document the difference in behavior. This is a bit more involved, would potentially hide the behavior change of someone doing a bundle update, not sure whether it's worth it. I believe this is already a problem today since we require rack >= 1.3.

WDYT?

[ioquatix]

I don't personally have a strong opinion about this. Is this change semantically breaking something that was previously working?

[dblock]

@ioquatix yes, I believe this is a semantically breaking change. Per above, passing a query params like ?name will be considered by Rack (>=3) as { "name" => "" } instead { "name" => nil }, aka the parameter now has a default value of empty string instead of nil, which is quite different at least as far as code that is evaluating the parameter is concerned. In Rack 2.x the parameter value is not present, while in 3.x it is.

[ericproulx]

I think it would be easier to it in two steps starting with upgrading rake-test and limit rack < 3. It would give more time to think about empty vs nil behavior. I can create a PR and add a rack3 gemfile for distinguish from the edge.

[ioquatix]

Are there any HTML controls which generate query strings like this e.g. that differentiate between ?foo and foo=?

[ericproulx]

I don't know but when looking at rack's tests, it's now "" instead of nil not matter how its written

[ioquatix]

What I'm asking is whether browsers actually utilise the difference.

[dblock]

@ioquatix Most of users of Grape aren't browsers. We're an API framework.

[ioquatix]

I see, so is there any difference between api?foo=bar&baz vs api?foo=bar?

[ericproulx]

I see, so is there any difference between api?foo=bar&baz vs api?foo=bar?

Rack3

foo=bar&baz => {"foo"=>"bar", "baz"=>""}
foo=bar => {"foo"=>"bar"}

Rack2

foo=bar&baz => {"foo"=>"bar", "baz"=>nil}
foo=bar => {"foo"=>"bar"}

The tricky part is this change only affect query parameters. A body payload would still have nil values but not the query params.

[ioquatix]

I understand that, but is there any difference, if I write query["baz"] in both cases I get nil. I know one is because it's explicitly nil and the other is because it defaults to nil. Just wondering if the fact that query.key?("baz") being true matters.

[dblock]

Isn't the Rack 3 case of foo=bar&baz => {"foo"=>"bar", "baz"=>""} query["baz"] == "" (not nil)?

[ioquatix]

Yes, IIUC, that's the case. And it's done that way to match the RFCs. However, my question is, why specify baz at all if you just want it to be nil?

[dblock]

Yes, IIUC, that's the case. And it's done that way to match the RFCs. However, my question is, why specify baz at all if you just want it to be nil?

Because I want to pass nil as opposed to the absence of parameter. Consider an API that stores a middle name as a string. To set my middle name I pass name=Middle. To unset it, I pass name. If I don't pass any value I don't want it changed. In no case I wanted it to be a blank string.

If I am not mistaken, with this change an application/json POST now behaves differently from an application/x-www-form-urlencoded POST.