Make a helper reach the routes declared above it in its scope

CHANGELOG.md

@@ -23,6 +23,7 @@
 * [#3051](https://github.com/ruby-grape/grape/pull/3051): Look up the validation error format once per response instead of once per message - [@ericproulx](https://github.com/ericproulx).
 * [#3055](https://github.com/ruby-grape/grape/pull/3055): Stop a 405 behind `Rack::Cascade` from making the next app answer 405 too - [@ericproulx](https://github.com/ericproulx).
 * [#3058](https://github.com/ruby-grape/grape/pull/3058): Declare `rexml` in the test bundle instead of relying on Danger to bring it in - [@ericproulx](https://github.com/ericproulx).
+* [#3061](https://github.com/ruby-grape/grape/pull/3061): Make a helper reach the routes declared above it in its scope - [@ericproulx](https://github.com/ericproulx).
 * [#3064](https://github.com/ruby-grape/grape/pull/3064): Cascade past a mounted Rack app that declines with a lowercase `x-cascade` - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 

README.md

@@ -2940,7 +2940,7 @@ class Twitter::API < Grape::API
 end
 ```
 
-The same goes for `use`, `helpers` and the `before`/`after` callbacks.
+The same goes for `use` and the `before`/`after` callbacks. `helpers` are the exception: they reach every route of their scope, wherever they are declared in it.
 
 Notice that you could combine these two approaches (rescuing custom errors takes precedence). For example, it's useful for handling all exceptions except Grape validation errors.
 

UPGRADING.md

@@ -33,6 +33,27 @@ Grape.config.allow_undeclared_params = false
 
 To keep accepting undeclared params from 5.0, set it to `true`. See [Rejecting Undeclared Parameters](README.md#rejecting-undeclared-parameters).
 
+#### A helper reaches every route of its scope, wherever it is declared
+
+A `helpers` block or module used to reach only the routes declared below it in the same scope. A route declared above it raised `NameError` when it called the helper ([#34](https://github.com/ruby-grape/grape/issues/34)). Since [#3061](https://github.com/ruby-grape/grape/pull/3061) it reaches every route of its scope, as it already did from an enclosing scope and through `mount`.
+
+So a helper method defined twice in one scope now answers with its last definition in every route of that scope, including the routes declared between the two definitions:
+
+```ruby
+helpers { def greeting = 'first' }
+get('/before') { greeting } # was 'first', now 'second'
+helpers { def greeting = 'second' }
+get('/after') { greeting }  # 'second'
+```
+
+Until 5.0, Grape warns about each such route when the API compiles:
+
+```
+Grape: helper `greeting` is defined again below GET /before in the same scope, so since 4.2.0 that route answers with the later definition (see UPGRADING.md). Rename one of them, or declare the route in its own namespace.
+```
+
+To keep a different method per route, give the methods different names, or declare the routes in separate namespaces, each with its own `helpers`. `use`, `rescue_from` and the `before`/`after` callbacks still apply only to the routes declared below them.
+
 #### A request every route declines answers 404, however many routes declined
 
 A route declines a request by answering with `X-Cascade: pass`, as a version declared with `cascade: true` does for a version it does not serve. As the [README](README.md#header) documents, the router then answers its own `404` if no other route takes the request. It only did so when one route had declined: with two or more routes on the path, the last one's response came back instead, and so did that of an ANY route (since 4.0.0). So a request for an unknown version answered `404` from an API with one version and `406` from an API with several.

lib/grape/endpoint.rb

@@ -113,6 +113,10 @@ def initialize(new_settings, http_methods:, path:, api:, app: nil, params: {}, r
         source: self.class.block_to_unbound_method(block)
       )
       @endpoints = app.endpoints if app.respond_to?(:endpoints)
+      # The live settings of the scope the route is declared in. The copy in
+      # the State holds that scope's helpers as they were at this line; these
+      # also hold the ones declared further down, read once the API compiles.
+      @scope_setting = new_settings
     end
 
     # Update our settings from a given parent settings instance. Used when
@@ -372,7 +376,7 @@ def build_undeclared_params
     # sets, which fits an 80-byte slot until a route block or a helper sets one
     # of its own; carrying the route-building state as well, it took 320. The
     # endpoint itself counts too when there are no helpers, both being a
-    # Grape::Endpoint, and it holds at most five.
+    # Grape::Endpoint, and it holds at most six.
     #
     # Helpers reach an endpoint by module inclusion, and +Object#dup+ does not
     # carry a singleton class over — so including them on the per-request copy
@@ -536,11 +540,15 @@ def error_middleware_options(format, content_types)
       }
     end
 
+    # Every helper of the route's scope, wherever it was declared in it, and
+    # of the scopes around it.
     def build_helpers
-      helpers = inheritable_setting.helpers
+      helpers = @scope_setting.helpers
       return if helpers.empty?
 
-      Module.new { helpers.each { |mod_to_include| include mod_to_include } }
+      composed = Module.new { helpers.each { |mod_to_include| include mod_to_include } }
+      RedefinedHelpersWarning.call(routes:, above: inheritable_setting.helpers, helpers:, composed:)
+      composed
     end
 
     # A bare Rack app mounted with +mount+ is called directly (see +compile!+):

lib/grape/endpoint/redefined_helpers_warning.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module Grape
+  class Endpoint
+    # Remove in 5.0. Before 4.2.0 a route reached only the helpers declared
+    # above it in its scope, the ones its settings copy holds. Where one
+    # declared below redefines a method of those, the route now answers with
+    # the later definition, and nothing else would tell.
+    module RedefinedHelpersWarning
+      module_function
+
+      # +above+ are the helpers the route reached before 4.2.0, +helpers+ all
+      # those it reaches now, and +composed+ the module they make up.
+      def call(routes:, above:, helpers:, composed:)
+        below = helpers - above
+        return if above.empty? || below.empty?
+
+        before = Module.new { above.each { |mod_to_include| include mod_to_include } }
+        names = redefined_names(below, before, composed)
+        return if names.empty?
+
+        names.each do |name|
+          warn(
+            "Grape: helper `#{name}` is defined again below #{route_names(routes)} in the same scope, so since 4.2.0 " \
+            'that route answers with the later definition (see UPGRADING.md). ' \
+            'Rename one of them, or declare the route in its own namespace.'
+          )
+        end
+      end
+
+      def route_names(routes)
+        routes.map { |route| "#{route.request_method} #{route.origin}" }.uniq.join(', ')
+      end
+
+      # The methods of +below+ that change what +before+ answered.
+      def redefined_names(below, before, composed)
+        below.flat_map { |mod| mod.instance_methods + mod.private_instance_methods }.uniq.select do |name|
+          (before.method_defined?(name) || before.private_method_defined?(name)) &&
+            before.instance_method(name).owner != composed.instance_method(name).owner
+        end
+      end
+    end
+  end
+end

spec/grape/api_spec.rb

@@ -2518,6 +2518,114 @@ def three
       end
       expect { get '/howdy' }.not_to raise_error
     end
+
+    context 'when declared after a route of the same scope' do
+      it 'is accessible from that route' do
+        subject.get '/howdy' do
+          hello
+        end
+
+        subject.helpers do
+          def hello
+            'Hello, world.'
+          end
+        end
+
+        get '/howdy'
+        expect(last_response.body).to eql 'Hello, world.'
+      end
+
+      it 'is accessible from that route in a namespace' do
+        subject.namespace :foo do
+          get :howdy do
+            hello
+          end
+
+          helpers do
+            def hello
+              'Hello, world.'
+            end
+          end
+        end
+
+        get '/foo/howdy'
+        expect(last_response.body).to eql 'Hello, world.'
+      end
+    end
+
+    it 'answers every route of its scope with the last definition of a helper' do
+      subject.helpers do
+        def greeting
+          'first'
+        end
+      end
+
+      subject.get '/before' do
+        greeting
+      end
+
+      subject.helpers do
+        def greeting
+          'second'
+        end
+      end
+
+      subject.get '/after' do
+        greeting
+      end
+
+      expect { get '/before' }.to output(
+        'Grape: helper `greeting` is defined again below GET /before in the same scope, so since 4.2.0 ' \
+        "that route answers with the later definition (see UPGRADING.md). Rename one of them, or declare the route in its own namespace.\n"
+      ).to_stderr
+      expect(last_response.body).to eql 'second'
+      get '/after'
+      expect(last_response.body).to eql 'second'
+    end
+
+    it 'does not warn for a helper declared below a route that only adds methods' do
+      subject.helpers do
+        def greeting
+          'Hello'
+        end
+      end
+
+      subject.get '/howdy' do
+        [greeting, audience].join(', ')
+      end
+
+      subject.helpers do
+        def audience
+          'world.'
+        end
+      end
+
+      expect { get '/howdy' }.not_to output.to_stderr
+      expect(last_response.body).to eql 'Hello, world.'
+    end
+
+    it 'does not warn for a helper redefined in an enclosing scope, which already reached the route' do
+      subject.helpers do
+        def greeting
+          'first'
+        end
+      end
+
+      subject.namespace :nested do
+        get '/howdy' do
+          greeting
+        end
+      end
+
+      subject.helpers do
+        def greeting
+          'second'
+        end
+      end
+
+      expect { get '/nested/howdy' }.not_to output.to_stderr
+      expect(last_response.body).to eql 'second'
+    end
   end
 
   describe '.scope' do