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.

[ericproulx]

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.

That's a valid point. Should we keep the current behavior for application/json and add some conditions if application/x-www-form-urlencoded or should both act the same way ?

Since Rails 5, testing an API in a Rails app will automatically use application/x-www-form-urlencoded if you don't specify as: :json in your spec. For instance,

# will be application/x-www-form-urlencoded
post '/my_endpoint', params { my_params: 'test' }

# will be application/json
post '/my_endpoint', params { my_params: 'test' }, as: :json 

If we choose to have a different behavior, it might gives some headaches.

[dblock]

I don't believe Rack parses JSON, or does it? We use our own parser. I expect no changes for JSON data, that would be a massive regression.

[ericproulx]

I don't believe Rack parses JSON, or does it? We use our own parser. I expect no changes for JSON data, that would be a massive regression.

I dont think it does, its just the way the request is built under the hood. Anyway, I believe we should have the same behavior no matter if it's application/x-www-form-urlencoded or application/json

[ericproulx]

Also, #2294 will definitely bring some changes on Rack 3.1. We might want to make a stable release for rack < 3 and add changes for rack >= 3

[ioquatix]

If we can find justification from an RFC to handle ?foo and ?foo= differently (or even just examples from other programming environments), then I would be happy to support such a feature back into Rack. My understanding was that the RFC implied they are the same, but I didn't personally read it.

Alternatively, we could either:

1. Add support as an optional feature, or
2. Re-implementing the old parser in a gem.

[dblock]

@ericproulx Feel free to PR what you think is right, including making a release. I agree that locking Rack down < 3 because of known issues is the right thing to do now.

@ioquatix I like the idea of extracting parsers out of Rack. Grape would like Rack not to parse data, really, and bring its own parser next to the JSON one or any other parser. I can see some issues with other Rack middleware and the assumptions being made that Rack always parses application/x-www-form-urlencoded . Either way those issues belong in Rack, should we open some? Maybe @ericproulx wants to help? :)

[ioquatix]

I went looking for specifications today and the closest thing I found was https://url.spec.whatwg.org/#application/x-www-form-urlencoded which states that:

If bytes contains a 0x3D (=), then let name be the bytes from the start of bytes up to but excluding its first 0x3D (=), and let value be the bytes, if any, after the first 0x3D (=) up to the end of bytes. If 0x3D (=) is the first byte, then name will be the empty byte sequence. If it is the last, then value will be the empty byte sequence.
Otherwise, let name have the value of bytes and let value be the empty byte sequence.
...
Append (nameString, valueString) to output.

So it seems to me there is no valid representation of "null".

That being said, this is a retrospective specification, the full extent of the query part of a URL is left un(der) specified.

[ioquatix]

The correct place to fix this (if anything can be done) is in the standard. I've made an issue and linked it. I suggest if you feel passionate about it, to go and comment on it.

[kbarrette]

There is a vulnerability in rack < 2 that some security monitoring software is currently bugging me about: rack/rack#1732
Any advice on how we should move forward with grape and rack?

[dblock]

@kbarrette I'd start by making a PR that has passing specs with Rack 3. We can consider a major version upgrade that only supports v3, but I would prefer not to.

[kbarrette]

@kbarrette I'd start by making a PR that has passing specs with Rack 3. We can consider a major version upgrade that only supports v3, but I would prefer not to.

#2346

[dblock]

@kbarrette So now we pass specs with Rack 3. Should we close this?

[kbarrette]

@dblock I don't know if fixing the tests addressed all of @ericproulx's concerns.

As for me, I'd like to see a new version released so I can resolve my snyk alert.

[dblock]

Add a +1 to #2349. I'll do a release this week if another maintainer doesn't beat me to it (please do!).

[ericproulx]

@dblock this has been fixed in rack 3.0.7. Should we add some information in the readme to specify that if you're using rack 3.X you need at least to be >= 3.0.7 ?

@ioquatix thanks for all the work.

[dblock]

@dblock this has been fixed in rack 3.0.7. Should we add some information in the readme to specify that if you're using rack 3.X you need at least to be >= 3.0.7 ?

I would update/edit tests that were modified in #2346 in a way that fails with rack < 3.0.7 and passes with rack >= 3.0.7, and then just close this. If you think a note is helpful, why not.

[ericproulx]

GH worflows always run the latest 3.* so its ain't an issue for testing. I'll leave it like this.