coerce_with should be called for params with nil value

[braktar]

Regarding the expected behavior pointed by @dblock

So should we adjust the default validator behavior ?

Yes, I believe so. A nil value is a nil value, not the absence of a parameter.

Originally posted by @dblock in #2040 (comment)

Here is a first attemp to fix the behavior applied to the CustomTypeCoercer

[senhalil]

It looks good to me

[dblock]

This needs a README update to document behavior, CHANGELOG entry, and an UPGRADING note as the behavior has changed, even though it was undocumented.

Do we think version should be bumped?

[senhalil]

For me the current behaviour was surprising -- i.e., coerce not being called for a declared value -- and it immediately registered as a bug.

This new behaviour got introduced silently somewhere between 1.4.1 and 1.5.0 so from my point of view, it is more of a bug fix (to go back to the usual behaviour) then a behaviour change per se.

If you see this as a backwards (original behaviour) compatible bug fix, semver says patch level shall be bumped.

If you see it as correcting the backwards compatibility broken by a previous release then it says minor version.

After saying all that, I don't think I am really at the position to suggest one way or another vis-a-vis version change so you have been warned.

[dblock]

This new behaviour got introduced silently somewhere between 1.4.1 and 1.5.0 so from my point of view, it is more of a bug fix (to go back to the usual behaviour) then a behaviour change per se.

Makes sense, bug fix it is. Looking forward to all the README/UPGRADING/CHANGELOG parts. Thank you.

[braktar]

The .md files are now updated. + I've added spec in case of an array

[dblock]

Good. I still would like clearer UPGRADING help.

[dm1try]

aside note
there is no specification for passing a nil value using HTTP query string, so in the examples above we are relying on the fact that rack interprets foo without = sign as a key with a nil value, while foo= is interpreted as a key with an empty string. but not all client implementations follow the same rules.

[dm1try]

another aside note

I feel like coerce_with is something redundant. it looks like all requirements can be solved by providing a custom abstraction for a type(just a class) and allowing using lambda in type

`type`: ->(val) { }

I think providing different parsing rules for something should lead to creating a new type as using the same rules will lead to inconsistency from the client perspective, not sure though :)

type: `Integer` # default rules
type: `Integer`, coerce_with: MyInt #another 
type: `Integer`, corce_with: ->() #another

[dblock]

Any idea what the truffleruby problem is here? @gogainda @eregon

[dblock]

I'm good with this, but need to figure out what's up with CI.

[eregon]

https://github.com/ruby-grape/grape/pull/2164/checks?check_run_id=2039817421

     ArgumentError:
       wrong number of arguments (given 0, expected 1)
     # ./lib/grape/exceptions/method_not_allowed.rb:6:in `initialize'
     # <internal:core> core/truffle/exception_operations.rb:18:in `new'
     # <internal:core> core/truffle/exception_operations.rb:18:in `build_exception_for_raise'
     # <internal:core> core/kernel.rb:694:in `raise'
     # ./lib/grape/endpoint.rb:258:in `block in run'
     # ./vendor/bundle/truffleruby/21.1.0-dev-ffeea561/gems/activesupport-6.1.3/lib/active_support/notifications.rb:205:in `instrument'
     # ./lib/grape/endpoint.rb:247:in `run'
     # ./lib/grape/endpoint.rb:322:in `block in build_stack'

So

grape/lib/grape/endpoint.rb

Line 258 in a43fea9

raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) unless options?

calls raise and it does not give a message String. It does pass a positional Hash argument though.
And

grape/lib/grape/exceptions/method_not_allowed.rb

Line 6 in a43fea9

def initialize(headers)

expects to receive that Hash.
It started failing most likely because of oracle/truffleruby@8348283, cc @bjfish

The code in grape seems fine, I think we need to find another way to extract the cause: keyword argument of raise (e.g., kwargs = undefined or maybe **kwargs) so we don't lose other keys in the Hash (in TruffleRuby).
In Ruby 3 this should no longer be a problem and we could restore the current code, because then the positional Hash would not be considered for keyword arguments.

[dblock]

CI should be skipping truffleruby failures (we have this experimental=true in there, don't we?). Feel free to "fix" this by removing it if needed, and someone else can deal with it, I'll merge on green. I don't want to hold up a release either.

[eregon]

@dblock That's due to the confusing continue-on-error of GitHub, see actions/runner#2347.
The latest build as a whole is green, see https://github.com/ruby-grape/grape/actions/runs/624485357
But the PR UI is really bad at showing that.

I think you could switch to truffleruby instead of truffleruby-head to avoid this issue, and it should also be more stable.
OTOH it's very useful that we see this error, so thank you for reporting it.

[bjfish]

I currently see an issue with:

grape/lib/grape/endpoint.rb

Line 258 in a43fea9

raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) unless options?

The second to parameter to Kernel#raise should be a string as indicated by the documentation here:
https://ruby-doc.org/core-2.7.2/Kernel.html#method-i-raise

Here is a good example on how to pass custom data to an exception:
https://www.honeybadger.io/blog/ruby-custom-exceptions/

I will also work on fixing the TruffleRuby failure here as it is an incompatibility.

[dblock]

Good call @bjfish. Want to help fix the Grape implementation?

[bjfish]

@dblock Yes, I've created a PR here with all of these types of usages that I found:
#2165