From e922b1b1b10fd2cc7e74a864a77f309e096304f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Carlos=20Garc=C3=ADa=20del=20Canto?= Date: Sun, 26 Nov 2023 16:34:18 +0100 Subject: [PATCH] fix(#2350): Update `recognize_paths` taking into account the `route_param` type (#2379) * fix(#2350): Use musterman-grape 1.1.0 for recognize_paths taking into account the route_param type * fix(#2350): Updating wording and passing rubocop --- CHANGELOG.md | 1 + README.md | 27 +++++++++++++ UPGRADING.md | 44 +++++++++++++++++++++ grape.gemspec | 2 +- lib/grape/router/pattern.rb | 2 + spec/grape/api/recognize_path_spec.rb | 55 +++++++++++++++++++++++++++ spec/grape/api_spec.rb | 23 +++++++++-- 7 files changed, 150 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1fa986c24..bdda7ab3be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [#2371](https://github.com/ruby-grape/grape/pull/2371): Use a param value as the `default` value of other param - [@jcagarcia](https://github.com/jcagarcia). * [#2377](https://github.com/ruby-grape/grape/pull/2377): Allow to use instance variables values inside `rescue_from` - [@jcagarcia](https://github.com/jcagarcia). +* [#2379](https://github.com/ruby-grape/grape/pull/2379): `recognize_path` now takes into account the `route_param` type - [@jcagarcia](https://github.com/jcagarcia). * Your contribution here. #### Fixes diff --git a/README.md b/README.md index f599d5bc44..4116daf7d9 100644 --- a/README.md +++ b/README.md @@ -2408,6 +2408,33 @@ end API.recognize_path '/statuses' ``` +Since version `2.1.0`, the `recognize_path` method takes into account the parameters type to determine which endpoint should match with given path. + +```ruby +class Books < Grape::API + resource :books do + route_param :id, type: Integer do + # GET /books/:id + get do + #... + end + end + + resource :share do + # POST /books/share + post do + # .... + end + end + end +end + +API.recognize_path '/books/1' # => /books/:id +API.recognize_path '/books/share' # => /books/share +API.recognize_path '/books/other' # => nil +``` + + ## Allowed Methods When you add a `GET` route for a resource, a route for the `HEAD` method will also be added automatically. You can disable this behavior with `do_not_route_head!`. diff --git a/UPGRADING.md b/UPGRADING.md index fb53565d07..f8cdb93b37 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -51,6 +51,50 @@ class TwitterAPI < Grape::API end ``` +#### Recognizing Path + +Grape now considers the types of the configured `route_params` in order to determine the endpoint that matches with the performed request. + +So taking into account this `Grape::API` class + +```ruby +class Books < Grape::API + resource :books do + route_param :id, type: Integer do + # GET /books/:id + get do + #... + end + end + + resource :share do + # POST /books/share + post do + # .... + end + end + end +end +``` + +Before: +```ruby +API.recognize_path '/books/1' # => /books/:id +API.recognize_path '/books/share' # => /books/:id +API.recognize_path '/books/other' # => /books/:id +``` + +After: +```ruby +API.recognize_path '/books/1' # => /books/:id +API.recognize_path '/books/share' # => /books/share +API.recognize_path '/books/other' # => nil +``` + +This implies that before this changes, when you performed `/books/other` and it matched with the `/books/:id` endpoint, you get a `400 Bad Request` response because the type of the provided `:id` param was not an `Integer`. However, after upgrading to version `2.1.0` you will get a `404 Not Found` response, because there is not a defined endpoint that matches with `/books/other`. + +See [#2379](https://github.com/ruby-grape/grape/pull/2379) for more information. + ### Upgrading to >= 2.0.0 #### Headers diff --git a/grape.gemspec b/grape.gemspec index 9e53ddea75..7dbc25a0e7 100644 --- a/grape.gemspec +++ b/grape.gemspec @@ -23,7 +23,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'activesupport', '>= 5' s.add_runtime_dependency 'builder' s.add_runtime_dependency 'dry-types', '>= 1.1' - s.add_runtime_dependency 'mustermann-grape', '~> 1.0.0' + s.add_runtime_dependency 'mustermann-grape', '~> 1.1.0' s.add_runtime_dependency 'rack', '>= 1.3.0' s.add_runtime_dependency 'rack-accept' diff --git a/lib/grape/router/pattern.rb b/lib/grape/router/pattern.rb index a239980484..6d7047773c 100644 --- a/lib/grape/router/pattern.rb +++ b/lib/grape/router/pattern.rb @@ -28,8 +28,10 @@ def initialize(pattern, **options) def pattern_options(options) capture = extract_capture(**options) + params = options[:params] options = DEFAULT_PATTERN_OPTIONS.dup options[:capture] = capture if capture.present? + options[:params] = params if params.present? options end diff --git a/spec/grape/api/recognize_path_spec.rb b/spec/grape/api/recognize_path_spec.rb index b3e9afa969..04b0b48b8a 100644 --- a/spec/grape/api/recognize_path_spec.rb +++ b/spec/grape/api/recognize_path_spec.rb @@ -17,5 +17,60 @@ subject.get {} expect(subject.recognize_path('/bar/1234')).to be_nil end + + context 'when parametrized route with type specified together with a static route' do + subject do + Class.new(described_class) do + resource :books do + route_param :id, type: Integer do + get do + end + + resource :loans do + route_param :loan_id, type: Integer do + get do + end + end + + resource :print do + post do + end + end + end + end + + resource :share do + post do + end + end + end + end + end + + it 'recognizes the static route when the parameter does not match with the specified type' do + actual = subject.recognize_path('/books/share').routes[0].origin + expect(actual).to eq('/books/share') + end + + it 'does not recognize any endpoint when there is not other endpoint that matches with the requested path' do + actual = subject.recognize_path('/books/other') + expect(actual).to be_nil + end + + it 'recognizes the parametrized route when the parameter matches with the specified type' do + actual = subject.recognize_path('/books/1').routes[0].origin + expect(actual).to eq('/books/:id') + end + + it 'recognizes the static nested route when the parameter does not match with the specified type' do + actual = subject.recognize_path('/books/1/loans/print').routes[0].origin + expect(actual).to eq('/books/:id/loans/print') + end + + it 'recognizes the nested parametrized route when the parameter matches with the specified type' do + actual = subject.recognize_path('/books/1/loans/33').routes[0].origin + expect(actual).to eq('/books/:id/loans/:loan_id') + end + end end end diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index f2ca6d55d7..9826dd8674 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -1132,7 +1132,7 @@ class DummyFormatClass d = double('after mock') subject.params do - requires :id, type: Integer + requires :id, type: Integer, values: [1, 2, 3] end subject.resource ':id' do before { a.do_something! } @@ -1151,9 +1151,9 @@ class DummyFormatClass expect(c).to receive(:do_something!).exactly(0).times expect(d).to receive(:do_something!).exactly(0).times - get '/abc' + get '/4' expect(last_response.status).to be 400 - expect(last_response.body).to eql 'id is invalid' + expect(last_response.body).to eql 'id does not have a valid value' end it 'calls filters in the correct order' do @@ -4408,5 +4408,22 @@ def uniqe_id_route expect(last_response.body).to eq({ my_var: nil }.to_json) end end + + context 'when set type to a route_param' do + context 'and the param does not match' do + it 'returns a 404 response' do + subject.namespace :books do + route_param :id, type: Integer do + get do + params[:id] + end + end + end + + get '/books/other' + expect(last_response.status).to be 404 + end + end + end end end