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

arg · arg · commit 8cdcf0614a4f · 2024-04-01T17:07:59.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,6 +28,7 @@
 * [#2387](https://github.com/ruby-grape/grape/pull/2387): Fix rubygems version within workflows - [@ericproulx](https://github.com/ericproulx).
 * [#2405](https://github.com/ruby-grape/grape/pull/2405): Fix edge workflow - [@ericproulx](https://github.com/ericproulx).
 * [#2414](https://github.com/ruby-grape/grape/pull/2414): Fix Rack::Lint missing content-type - [@ericproulx](https://github.com/ericproulx).
+* [#2378](https://github.com/ruby-grape/grape/pull/2378): Do not overwrite `route_param` with a regular one if they share same name - [@arg](https://github.com/arg).
 * Your contribution here.
 
 ### 2.0.0 (2023/11/11)
diff --git a/README.md b/README.md
@@ -39,6 +39,7 @@
   - [Include Parent Namespaces](#include-parent-namespaces)
   - [Include Missing](#include-missing)
   - [Evaluate Given](#evaluate-given)
+  - [Parameter Precedence](#parameter-precedence)
 - [Parameter Validation and Coercion](#parameter-validation-and-coercion)
   - [Supported Parameter Types](#supported-parameter-types)
   - [Integer/Fixnum and Coercions](#integerfixnum-and-coercions)
@@ -1198,6 +1199,35 @@ curl -X POST -H "Content-Type: application/json" localhost:9292/child -d '{"chil
 }
 ````
 
+### Parameter Precedence
+
+Using `route_param` takes higher precedence over a regular parameter defined with same name:
+
+```ruby
+params do
+  requires :foo, type: String
+end
+route_param :foo do
+  get do
+    { value: params[:foo] }
+  end
+end
+```
+
+**Request**
+
+```bash
+curl -X POST -H "Content-Type: application/json" localhost:9292/bar -d '{"foo": "baz"}'
+```
+
+**Response**
+
+```json
+{
+  "value": "bar"
+}
+```
+
 ## Parameter Validation and Coercion
 
 You can define validations and coercion options for your parameters using a `params` block.
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -9,6 +9,59 @@ The `rack_response` method has been deprecated and the `error_response` method h
 
 See [#2414](https://github.com/ruby-grape/grape/pull/2414) for more information.
 
+#### Change in parameters precedence
+
+When using together with `Grape::Extensions::Hash::ParamBuilder`, `route_param` takes higher precedence over a regular parameter defined with same name, which now matches the default param builder behavior.
+
+This was a regression introduced by [#2326](https://github.com/ruby-grape/grape/pull/2326) in Grape v1.8.0.
+
+```ruby
+grape.configure do |config|
+  config.param_builder = Grape::Extensions::Hash::ParamBuilder
+end
+
+params do
+  requires :foo, type: String
+end
+route_param :foo do
+  get do
+    { value: params[:foo] }
+  end
+end
+```
+
+Request:
+
+```bash
+curl -X POST -H "Content-Type: application/json" localhost:9292/bar -d '{"foo": "baz"}'
+```
+
+Response prior to v1.8.0:
+
+```json
+{
+  "value": "bar"
+}
+```
+
+v1.8.0..v2.0.0:
+
+```json
+{
+  "value": "baz"
+}
+```
+
+v2.1.0+:
+
+```json
+{
+  "value": "bar"
+}
+```
+
+See [#2378](https://github.com/ruby-grape/grape/pull/2378) for details.
+
 #### Grape::Router::Route.route_xxx methods have been removed
 
 - `route_method` is accessible through `request_method`
diff --git a/lib/grape/extensions/hash.rb b/lib/grape/extensions/hash.rb
@@ -12,8 +12,12 @@ module ParamBuilder
 
         def build_params
           rack_params.deep_dup.tap do |params|
-            params.deep_merge!(grape_routing_args) if env.key?(Grape::Env::GRAPE_ROUTING_ARGS)
             params.deep_symbolize_keys!
+
+            if env.key?(Grape::Env::GRAPE_ROUTING_ARGS)
+              grape_routing_args.deep_symbolize_keys!
+              params.deep_merge!(grape_routing_args)
+            end
           end
         end
       end
diff --git a/spec/grape/extensions/param_builders/hash_spec.rb b/spec/grape/extensions/param_builders/hash_spec.rb
@@ -79,5 +79,34 @@ def app
       expect(last_response.status).to eq(200)
       expect(last_response.body).to eq('["bar", nil]')
     end
+
+    it 'does not overwrite route_param with a regular param if they have same name' do
+      subject.namespace :route_param do
+        route_param :foo do
+          get { params.to_json }
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('{"foo":"bar"}')
+    end
+
+    it 'does not overwrite route_param with a defined regular param if they have same name' do
+      subject.namespace :route_param do
+        params do
+          requires :foo, type: String
+        end
+        route_param :foo do
+          get do
+            params[:foo]
+          end
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('bar')
+    end
   end
 end
diff --git a/spec/grape/extensions/param_builders/hash_with_indifferent_access_spec.rb b/spec/grape/extensions/param_builders/hash_with_indifferent_access_spec.rb
@@ -101,5 +101,34 @@ def app
         expect(last_response.body).to eq('["bar", "bar"]')
       end
     end
+
+    it 'does not overwrite route_param with a regular param if they have same name' do
+      subject.namespace :route_param do
+        route_param :foo do
+          get { params.to_json }
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('{"foo":"bar"}')
+    end
+
+    it 'does not overwrite route_param with a defined regular param if they have same name' do
+      subject.namespace :route_param do
+        params do
+          requires :foo, type: String
+        end
+        route_param :foo do
+          get do
+            [params[:foo], params['foo']]
+          end
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('["bar", "bar"]')
+    end
   end
 end
diff --git a/spec/grape/extensions/param_builders/hashie/mash_spec.rb b/spec/grape/extensions/param_builders/hashie/mash_spec.rb
@@ -75,5 +75,35 @@ def app
       expect(last_response.status).to eq(200)
       expect(last_response.body).to eq('["bar", "bar"]')
     end
+
+    it 'does not overwrite route_param with a regular param if they have same name' do
+      subject.namespace :route_param do
+        route_param :foo do
+          get { params.to_json }
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('{"foo":"bar"}')
+    end
+
+    it 'does not overwrite route_param with a defined regular param if they have same name' do
+      subject.namespace :route_param do
+        params do
+          build_with Grape::Extensions::Hashie::Mash::ParamBuilder # rubocop:disable RSpec/DescribedClass
+          requires :foo, type: String
+        end
+        route_param :foo do
+          get do
+            [params[:foo], params['foo']]
+          end
+        end
+      end
+
+      get '/route_param/bar', foo: 'baz'
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('["bar", "bar"]')
+    end
   end
 end
