From 69d3cb0809c24da345468767dd1b63cb1b39ac8b Mon Sep 17 00:00:00 2001 From: Juan Carlos Garcia Date: Sun, 10 Dec 2023 23:08:28 +0100 Subject: [PATCH] fix(#1975): Allow to use `before/after/rescue_from` methods in any order when using `mount` --- .rubocop_todo.yml | 9 + CHANGELOG.md | 1 + README.md | 14 +- lib/grape/api.rb | 13 ++ lib/grape/dsl/routing.rb | 20 +- .../grape/api/mount_and_helpers_order_spec.rb | 171 ++++++++++++++++++ spec/grape/api/mount_and_rescue_from_spec.rb | 80 ++++++++ 7 files changed, 305 insertions(+), 3 deletions(-) create mode 100644 spec/grape/api/mount_and_helpers_order_spec.rb create mode 100644 spec/grape/api/mount_and_rescue_from_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 57e7a83bd8..cffa616810 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -27,6 +27,8 @@ Lint/ConstantDefinitionInBlock: Exclude: - 'spec/grape/api/defines_boolean_in_params_spec.rb' - 'spec/grape/api/inherited_helpers_spec.rb' + - 'spec/grape/api/mount_and_helpers_order_spec.rb' + - 'spec/grape/api/mount_and_rescue_from_spec.rb' - 'spec/grape/api/nested_helpers_spec.rb' - 'spec/grape/api/patch_method_helpers_spec.rb' - 'spec/grape/api_spec.rb' @@ -235,6 +237,8 @@ RSpec/FilePath: - 'spec/grape/api/documentation_spec.rb' - 'spec/grape/api/inherited_helpers_spec.rb' - 'spec/grape/api/invalid_format_spec.rb' + - 'spec/grape/api/mount_and_helpers_order_spec.rb' + - 'spec/grape/api/mount_and_rescue_from_spec.rb' - 'spec/grape/api/namespace_parameters_in_route_spec.rb' - 'spec/grape/api/nested_helpers_spec.rb' - 'spec/grape/api/optional_parameters_in_route_spec.rb' @@ -289,6 +293,7 @@ RSpec/IndexedLet: # Configuration parameters: AssignmentOnly. RSpec/InstanceVariable: Exclude: + - 'spec/grape/api/mount_and_helpers_order_spec.rb' - 'spec/grape/api_spec.rb' - 'spec/grape/endpoint_spec.rb' - 'spec/grape/middleware/base_spec.rb' @@ -301,6 +306,8 @@ RSpec/LeakyConstantDeclaration: Exclude: - 'spec/grape/api/defines_boolean_in_params_spec.rb' - 'spec/grape/api/inherited_helpers_spec.rb' + - 'spec/grape/api/mount_and_helpers_order_spec.rb' + - 'spec/grape/api/mount_and_rescue_from_spec.rb' - 'spec/grape/api/nested_helpers_spec.rb' - 'spec/grape/api/patch_method_helpers_spec.rb' - 'spec/grape/api_spec.rb' @@ -343,6 +350,8 @@ RSpec/MultipleExpectations: - 'spec/grape/api/deeply_included_options_spec.rb' - 'spec/grape/api/defines_boolean_in_params_spec.rb' - 'spec/grape/api/invalid_format_spec.rb' + - 'spec/grape/api/mount_and_helpers_order_spec.rb' + - 'spec/grape/api/mount_and_rescue_from_spec.rb' - 'spec/grape/api/namespace_parameters_in_route_spec.rb' - 'spec/grape/api/optional_parameters_in_route_spec.rb' - 'spec/grape/api/parameters_modification_spec.rb' diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ab7a42b35..5d734454d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,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). +* [](): Allow to use `before/after/rescue_from` methods in any order when using `mount` - [@jcagarcia](https://github.com/jcagarcia). * Your contribution here. #### Fixes diff --git a/README.md b/README.md index 3c3105c8ae..bc2509cf57 100644 --- a/README.md +++ b/README.md @@ -408,7 +408,7 @@ class Twitter::API < Grape::API end ``` -Keep in mind such declarations as `before/after/rescue_from` must be placed before `mount` in a case where they should be inherited. +Declarations as `before/after/rescue_from` can be placed before or after `mount`. In any case they will be inherited. ```ruby class Twitter::API < Grape::API @@ -416,8 +416,20 @@ class Twitter::API < Grape::API header 'X-Base-Header', 'will be defined for all APIs that are mounted below' end + rescue_from :all do + error!({ "error" => "Internal Server Error" }, 500) + end + mount Twitter::Users mount Twitter::Search + + after do + clean_cache! + end + + rescue_from ZeroDivisionError do + error!({ "error" => "Not found" }, 404) + end end ``` diff --git a/lib/grape/api.rb b/lib/grape/api.rb index 1fc057bfb3..628f9ddca9 100644 --- a/lib/grape/api.rb +++ b/lib/grape/api.rb @@ -150,6 +150,19 @@ def add_setup(method, *args, &block) @instances.each do |instance| last_response = replay_step_on(instance, setup_step) end + + # Updating all previously mounted classes in the case that new methods have been executed. + if method != :mount && @setup.size > 1 + previous_mount_steps = @setup.select { |step| step[:method] == :mount } + previous_mount_steps.each do |mount_step| + refresh_mount_step = mount_step.merge(method: :refresh_mounted_api) + @setup += [refresh_mount_step] + @instances.each do |instance| + replay_step_on(instance, refresh_mount_step) + end + end + end + last_response end diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 158db99f5a..a422c34d04 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -85,8 +85,8 @@ def mount(mounts, *opts) mounts = { mounts => '/' } unless mounts.respond_to?(:each_pair) mounts.each_pair do |app, path| if app.respond_to?(:mount_instance) - opts_with = opts.any? ? opts.shift[:with] : {} - mount({ app.mount_instance(configuration: opts_with) => path }) + opts_with = opts.any? ? opts.first[:with] : {} + mount({ app.mount_instance(configuration: opts_with) => path }, *opts) next end in_setting = inheritable_setting @@ -103,6 +103,15 @@ def mount(mounts, *opts) change! end + # When trying to mount multiple times the same endpoint, remove the previous ones + # from the list of endpoints if refresh_already_mounted parameter is true + refresh_already_mounted = opts.any? ? opts.first[:refresh_already_mounted] : false + if refresh_already_mounted && !endpoints.empty? + endpoints.delete_if do |endpoint| + endpoint.options[:app].to_s == app.to_s + end + end + endpoints << Grape::Endpoint.new( in_setting, method: :any, @@ -225,6 +234,13 @@ def route_param(param, options = {}, &block) def versions @versions ||= [] end + + private + + def refresh_mounted_api(mounts, *opts) + opts << { refresh_already_mounted: true } + mount(mounts, *opts) + end end end end diff --git a/spec/grape/api/mount_and_helpers_order_spec.rb b/spec/grape/api/mount_and_helpers_order_spec.rb new file mode 100644 index 0000000000..a00423a14f --- /dev/null +++ b/spec/grape/api/mount_and_helpers_order_spec.rb @@ -0,0 +1,171 @@ +# frozen_string_literal: true + +describe Grape::API do + def app + subject + end + + describe 'rescue_from' do + context 'when the API is mounted AFTER defining the class rescue_from handler' do + class APIRescueFrom < Grape::API + rescue_from :all do + error!({ type: 'all' }, 404) + end + + get do + { count: 1 / 0 } + end + end + + class MainRescueFromAfter < Grape::API + rescue_from ZeroDivisionError do + error!({ type: 'zero' }, 500) + end + + mount APIRescueFrom + end + + subject { MainRescueFromAfter } + + it 'is rescued by the rescue_from ZeroDivisionError handler from Main class' do + get '/' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq({ type: 'zero' }.to_json) + end + end + + context 'when the API is mounted BEFORE defining the class rescue_from handler' do + class APIRescueFrom < Grape::API + rescue_from :all do + error!({ type: 'all' }, 404) + end + + get do + { count: 1 / 0 } + end + end + + class MainRescueFromBefore < Grape::API + mount APIRescueFrom + + rescue_from ZeroDivisionError do + error!({ type: 'zero' }, 500) + end + end + + subject { MainRescueFromBefore } + + it 'is rescued by the rescue_from ZeroDivisionError handler from Main class' do + get '/' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq({ type: 'zero' }.to_json) + end + end + end + + describe 'before' do + context 'when the API is mounted AFTER defining the before helper' do + class APIBeforeHandler < Grape::API + get do + { count: @count }.to_json + end + end + + class MainBeforeHandlerAfter < Grape::API + before do + @count = 1 + end + + mount APIBeforeHandler + end + + subject { MainBeforeHandlerAfter } + + it 'is able to access the variables defined in the before helper' do + get '/' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq({ count: 1 }.to_json) + end + end + + context 'when the API is mounted BEFORE defining the before helper' do + class APIBeforeHandler < Grape::API + get do + { count: @count }.to_json + end + end + + class MainBeforeHandlerBefore < Grape::API + mount APIBeforeHandler + + before do + @count = 1 + end + end + + subject { MainBeforeHandlerBefore } + + it 'is able to access the variables defined in the before helper' do + get '/' + + expect(last_response.status).to eq(200) + expect(last_response.body).to eq({ count: 1 }.to_json) + end + end + end + + describe 'after' do + context 'when the API is mounted AFTER defining the after handler' do + class APIAfterHandler < Grape::API + get do + { count: 1 }.to_json + end + end + + class MainAfterHandlerAfter < Grape::API + after do + error!({ type: 'after' }, 500) + end + + mount APIAfterHandler + end + + subject { MainAfterHandlerAfter } + + it 'is able to access the variables defined in the after helper' do + get '/' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq({ type: 'after' }.to_json) + end + end + + context 'when the API is mounted BEFORE defining the after helper' do + class APIAfterHandler < Grape::API + get do + { count: 1 }.to_json + end + end + + class MainAfterHandlerBefore < Grape::API + mount APIAfterHandler + + after do + error!({ type: 'after' }, 500) + end + end + + subject { MainAfterHandlerBefore } + + it 'is able to access the variables defined in the after helper' do + get '/' + + expect(last_response.status).to eq(500) + expect(last_response.body).to eq({ type: 'after' }.to_json) + end + end + end +end diff --git a/spec/grape/api/mount_and_rescue_from_spec.rb b/spec/grape/api/mount_and_rescue_from_spec.rb new file mode 100644 index 0000000000..0dfc4a35d7 --- /dev/null +++ b/spec/grape/api/mount_and_rescue_from_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +describe Grape::API do + def app + subject + end + + context 'when multiple classes defines the same rescue_from' do + class AnAPI < Grape::API + rescue_from ZeroDivisionError do + error!({ type: 'an-api-zero' }, 404) + end + + get '/an-api' do + { count: 1 / 0 } + end + end + + class AnotherAPI < Grape::API + rescue_from ZeroDivisionError do + error!({ type: 'another-api-zero' }, 322) + end + + get '/another-api' do + { count: 1 / 0 } + end + end + + class OtherMain < Grape::API + mount AnAPI + mount AnotherAPI + end + + subject { OtherMain } + + it 'is rescued by the rescue_from ZeroDivisionError handler defined inside each of the classes' do + get '/an-api' + + expect(last_response.status).to eq(404) + expect(last_response.body).to eq({ type: 'an-api-zero' }.to_json) + + get '/another-api' + + expect(last_response.status).to eq(322) + expect(last_response.body).to eq({ type: 'another-api-zero' }.to_json) + end + + context 'when some class does not define a rescue_from but it was defined in a previous mounted endpoint' do + class AnAPIWithoutDefinedRescueFrom < Grape::API + get '/another-api-without-defined-rescue-from' do + { count: 1 / 0 } + end + end + + class OtherMainWithNotDefinedRescueFrom < Grape::API + mount AnAPI + mount AnotherAPI + mount AnAPIWithoutDefinedRescueFrom + end + + subject { OtherMainWithNotDefinedRescueFrom } + + it 'is not rescued by any of the previous defined rescue_from ZeroDivisionError handlers' do + get '/an-api' + + expect(last_response.status).to eq(404) + expect(last_response.body).to eq({ type: 'an-api-zero' }.to_json) + + get '/another-api' + + expect(last_response.status).to eq(322) + expect(last_response.body).to eq({ type: 'another-api-zero' }.to_json) + + expect do + get '/another-api-without-defined-rescue-from' + end.to raise_error(ZeroDivisionError) + end + end + end +end