Do not overwrite route_param with a regular one if they share same name

[arg]

#2326 changed the behavior of params merging. Before v1.8.0 the following route:

resources :users do
  route_param :id do
    get do
      user = User.find(params[:id])
      { id: user.id }
    end
  end
end

would return { id: 12345 } if called as

GET /users/12345?id=1

Now, since v1.8.0, it returns { id: 1 } as id passed in params overwrites route_param.

This PR restores the previous behavior.

[dblock]

Thanks! See below.

[arg]

@dblock could you please take a look, I made all the requested changes but PR is still in "changes requested" state for some reason.

[dblock]

Code looks good.

Looking at this deeper, it sounds like we're changing the undocumented/undefined behavior of a regular param overriding a route param before?

1. Is that the right thing to do? Why wouldn't a regular parameter specified by the caller override a route parameter?
2. Does anything change if I declare a regular parameter over a route parameter for the get API in your example? Add a spec for that.
3. Document this behavior in README.
4. This is a behavior change, but not breaking since it wasn't documented. Either way add a section to UPGRADING.

[dblock]

@arg want to finish this?

[arg]

@arg want to finish this?

Yes, I will. A bit busy now but will try to find some time in the next few days.

[DmytroVasin]

@dblock
I encountered the same issue, and if you don't mind, here's what I've found:

Why wouldn't a regular parameter specified by the caller override a route parameter?

Hashie::Mash, HashWithIndifferentAccess and Hash (prior 1.8.0) follow a similar logic: rack_params are taken as a base and overwritten with grape_routing_args (thus, grape_routing_args is strongly preferred).

NOTE:
Old deep symbolize behavior,
is functionally equivalent to deep_symbolize_keys

deep_symbolize_keys_in({"id" => nil}.merge(id: '123'))
#=> {:id=>"123"}

{"id" => nil}.merge(id: '123').deep_symbolize_keys
#=> {:id=>"123"}

However
In Use Active support functions we replaced deep_symbolize_keys_in with deep_symbolize_keys! (with a bang), that has a bit different behavior

{"id" => nil}.merge(id: '123').deep_symbolize_keys!
#=> {:id=>nil}

The priority works for Hashie::Mash and HashWithIndifferentAccess perfectly, when Hash relies on a sequence of arguments.
I agree that it was not documented, but the idea of priority of params was broken by our changes

[dblock]

@DmytroVasin thanks for the deep dive, makes sense, let's get this PR finished and make sure we have the right specs - is there anything else missing here?

[arg]

@DmytroVasin thanks for the deep dive, makes sense, let's get this PR finished and make sure we have the right specs - is there anything else missing here?

@dblock so what are the changes to make? Just the API test and README note?

[dblock]

I think so, basically what I said in #2378 (review). Thanks for picking this up!

[DmytroVasin]

@arg Any updates on this?

Sorry for disturbing you, but I can proceed with completing this MR if you don't mind

[arg]

@dblock @DmytroVasin sorry, have been quite busy the last couple of months. So anyways, I added test to cover scenario when both route_param and regular param are defined with same nave, also added README and UPGRADING entries. Please review

[dblock]

Thanks! I have some more asks, mostly in README/UPGRADING.

[DmytroVasin]

@arg Any updates on this?

Sorry for annoying you :)

[dblock]

@DmytroVasin want to finish this PR for @arg?

[arg]

Hey, thanks for reminding and sorry for the delay. I’m coming back tomorrow and will try to find some time to finish the PR.

[arg]

It looks like all requested changes are applied, please review

[dblock]

Spotted one typo, otherwise good to go!

[arg]

@dblock ready to merge

[dblock]

Thanks for the good work and for hanging in here with my OCD!