diff --git a/CHANGELOG.md b/CHANGELOG.md index c0584c57c..5f2ca373b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/grape.rb b/lib/grape.rb index fa8e70833..cab0b054e 100644 --- a/lib/grape.rb +++ b/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, diff --git a/lib/grape/dsl/helpers.rb b/lib/grape/dsl/helpers.rb index a4626c4a3..03d7a524e 100644 --- a/lib/grape/dsl/helpers.rb +++ b/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 diff --git a/spec/grape/dsl/helpers_spec.rb b/spec/grape/dsl/helpers_spec.rb index f2d0c2177..f457c51a5 100644 --- a/spec/grape/dsl/helpers_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index abcaa2146..b237ecd56 100644 --- a/spec/spec_helper.rb +++ b/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,