Add Grape.config.warn_on_helper_overrides (opt-in dev-time signal)

CHANGELOG.md

@@ -22,6 +22,7 @@
 * [#2693](https://github.com/ruby-grape/grape/pull/2693): Introduce `Grape::Exceptions::ErrorResponse` value object to replace the implicit-schema Hash thrown via `throw` - [@ericproulx](https://github.com/ericproulx).
 * [#2701](https://github.com/ruby-grape/grape/pull/2701): Replace `.tap` usages in `lib/` with explicit local variables - [@ericproulx](https://github.com/ericproulx).
 * [#2704](https://github.com/ruby-grape/grape/pull/2704): Add `Grape::Endpoint#logger` so the API's configured logger is reachable inside route handlers, filters, and `rescue_from` blocks without a helper - [@ericproulx](https://github.com/ericproulx).
+* [#2705](https://github.com/ruby-grape/grape/pull/2705): Add `Grape.config.warn_on_helper_overrides` (off by default) to emit a warning when a helper method masks a `Grape::Endpoint` instance method - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

lib/grape.rb

@@ -52,6 +52,7 @@ module Grape
 
   setting :param_builder, default: :hash_with_indifferent_access
   setting :lint, default: false
+  setting :warn_on_helper_overrides, default: false
 
   HTTP_SUPPORTED_METHODS = [
     Rack::GET,

lib/grape/dsl/helpers.rb

@@ -71,9 +71,30 @@ def define_boolean_in_mod(mod)
       def inject_api_helpers_to_mod(mod, &block)
         mod.extend(BaseHelper) unless mod.is_a?(BaseHelper)
         yield if block
+        warn_on_endpoint_overrides(mod) if Grape.config.warn_on_helper_overrides
         mod.api_changed(self)
       end
 
+      # When +Grape.config.warn_on_helper_overrides+ is enabled, emit a
+      # warning to +$stderr+ for any helper method that masks an instance
+      # method on +Grape::Endpoint+. Helpers are mixed into the endpoint's
+      # singleton class and therefore take precedence over +Endpoint+
+      # instance methods — usually intentional, but a common source of
+      # surprise when the framework gains a method that already collides
+      # with an existing helper name.
+      def warn_on_endpoint_overrides(mod)
+        overridden = mod.instance_methods(false).select { |m| Grape::Endpoint.method_defined?(m) }
+        return if overridden.empty?
+
+        overridden.each do |name|
+          warn(
+            "Grape: helper method `#{name}` overrides Grape::Endpoint##{name}. " \
+            'The helper takes precedence. To use the framework implementation, remove the helper. ' \
+            'Silence this warning by setting Grape.config.warn_on_helper_overrides = false.'
+          )
+        end
+      end
+
       # This module extends user defined helpers
       # to provide some API-specific functionality.
       module BaseHelper

spec/grape/dsl/helpers_spec.rb

@@ -93,5 +93,93 @@ def test
         expect(Class.new { extend Grape::DSL::Helpers }.singleton_methods - Class.methods).to contain_exactly(:helpers)
       end
     end
+
+    context 'with Grape.config.warn_on_helper_overrides enabled' do
+      it 'warns when a helper masks a Grape::Endpoint instance method' do
+        expect do
+          Class.new(Grape::API) do
+            helpers do
+              def params
+                'overridden'
+              end
+            end
+          end
+        end.to output(/helper method `params` overrides Grape::Endpoint#params/).to_stderr
+      end
+
+      it 'warns for each masking method in the same helpers block' do
+        output = capture_stderr do
+          Class.new(Grape::API) do
+            helpers do
+              def params
+                :p
+              end
+
+              def headers
+                :h
+              end
+
+              def my_unique_helper
+                :ok
+              end
+            end
+          end
+        end
+
+        expect(output).to include('overrides Grape::Endpoint#params')
+        expect(output).to include('overrides Grape::Endpoint#headers')
+        expect(output).not_to include('my_unique_helper')
+      end
+
+      it 'warns for an external helper module too' do
+        external = Module.new do
+          def params
+            :overridden
+          end
+        end
+
+        expect do
+          Class.new(Grape::API) { helpers external }
+        end.to output(/helper method `params` overrides Grape::Endpoint#params/).to_stderr
+      end
+
+      it 'does not warn for helpers that do not mask any Endpoint method' do
+        expect do
+          Class.new(Grape::API) do
+            helpers do
+              def my_unique_helper
+                :ok
+              end
+            end
+          end
+        end.not_to output.to_stderr
+      end
+    end
+
+    context 'with Grape.config.warn_on_helper_overrides disabled' do
+      before { Grape.config.warn_on_helper_overrides = false }
+      after  { Grape.config.warn_on_helper_overrides = true }
+
+      it 'does not warn even when a helper masks an Endpoint method' do
+        expect do
+          Class.new(Grape::API) do
+            helpers do
+              def params
+                'overridden'
+              end
+            end
+          end
+        end.not_to output.to_stderr
+      end
+    end
+  end
+
+  def capture_stderr
+    original = $stderr
+    $stderr = StringIO.new
+    yield
+    $stderr.string
+  ensure
+    $stderr = original
   end
 end

spec/spec_helper.rb

@@ -14,6 +14,10 @@
 end
 
 Grape.config.lint = true # lint all apis by default
+# Enabled in CI so that future test additions which accidentally mask
+# Grape::Endpoint instance methods via helpers surface as warnings.
+# Specs that need to toggle it explicitly should restore it to +true+ in their +after+.
+Grape.config.warn_on_helper_overrides = true
 Grape::Util::Registry.include(Deregister)
 
 # The default value for this setting is true in a standard Rails app,