fix(#1975): Allow to use before/after/rescue_from methods in any order when using mount (#2384)

* fix(#1975): Allow to use `before/after/rescue_from` methods in any order when using `mount`

* fix(#1975): Apply suggestions

Author: jcagarcia

.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'

CHANGELOG.md

@@ -6,6 +6,7 @@
 * [#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): Take into account the `route_param` type in `recognize_path` - [@jcagarcia](https://github.com/jcagarcia).
 * [#2383](https://github.com/ruby-grape/grape/pull/2383): Use regex block instead of if - [@ericproulx](https://github.com/ericproulx).
+* [#2384](https://github.com/ruby-grape/grape/pull/2384): Allow to use `before/after/rescue_from` methods in any order when using `mount` - [@jcagarcia](https://github.com/jcagarcia).
 * Your contribution here.
 
 #### Fixes

README.md

@@ -408,16 +408,28 @@ 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
   before do
     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
 ```
 

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.any?
+          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
 

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

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

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