From 60f7d490333f8b802fa327caaa4b0683d2e29f0e Mon Sep 17 00:00:00 2001 From: Jess Bees Date: Wed, 13 Oct 2021 16:02:39 -0400 Subject: [PATCH] Raise error when callback's only/unless symbols aren't methods When `before_action :callback, only: :action_name` is declared on a controller that doesn't respond to `action_name`, raise an exception at request time. This is a safety measure to ensure that typos or forgetfulness don't prevent a crucial callback from being run when it should. Include names of filters for more useful error messages The error message of the raised exception will be more useful if it indicates the names of the callbacks that have the missing conditinoal action. The way the callbacks get shoehorned into `_normalize_callback_options` options parameter is a little awkward, but done this way to avoid changing the method's signature, since it is a publicly documented method. --- .../lib/abstract_controller/callbacks.rb | 25 ++- actionpack/test/abstract/callbacks_test.rb | 185 ++++++++++++++++++ .../lib/rails/application/configuration.rb | 2 +- .../config/environments/development.rb.tt | 3 + .../templates/config/environments/test.rb.tt | 3 + .../test/application/configuration_test.rb | 26 +++ 6 files changed, 238 insertions(+), 6 deletions(-) diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb index 6ada987bdc..e16e5cded3 100644 --- a/actionpack/lib/abstract_controller/callbacks.rb +++ b/actionpack/lib/abstract_controller/callbacks.rb @@ -33,14 +33,26 @@ module AbstractController define_callbacks :process_action, terminator: ->(controller, result_lambda) { result_lambda.call; controller.performed? }, skip_after_callbacks_if_terminated: true + mattr_accessor :raise_on_missing_callback_actions, default: false end class ActionFilter # :nodoc: - def initialize(actions) + def initialize(filters, conditional_key, actions) + @filters = filters.to_a + @conditional_key = conditional_key @actions = Array(actions).map(&:to_s).to_set end def match?(controller) + if controller.raise_on_missing_callback_actions + missing_action = @actions.find { |action| !controller.available_action?(action) } + if missing_action + filter_names = @filters.length == 1 ? @filters.first.inspect : @filters.inspect + message = "The #{missing_action} action could not be found for the #{filter_names} callback on #{controller.class.name}, but it is listed in its #{@conditional_key.inspect} option" + raise ActionNotFound.new(message, controller, missing_action) + end + end + @actions.include?(controller.action_name) end @@ -75,9 +87,10 @@ module AbstractController end def _normalize_callback_option(options, from, to) # :nodoc: - if from = options.delete(from) - from = ActionFilter.new(from) - options[to] = Array(options[to]).unshift(from) + if from_value = options.delete(from) + filters = options[:filters] + from_value = ActionFilter.new(filters, from, from_value) + options[to] = Array(options[to]).unshift(from_value) end end @@ -95,8 +108,10 @@ module AbstractController # * options - A hash of options to be used when adding the callback. def _insert_callbacks(callbacks, block = nil) options = callbacks.extract_options! - _normalize_callback_options(options) callbacks.push(block) if block + options[:filters] = callbacks + _normalize_callback_options(options) + options.delete(:filters) callbacks.each do |callback| yield callback, options end diff --git a/actionpack/test/abstract/callbacks_test.rb b/actionpack/test/abstract/callbacks_test.rb index e64f580fb2..88f333cf8a 100644 --- a/actionpack/test/abstract/callbacks_test.rb +++ b/actionpack/test/abstract/callbacks_test.rb @@ -308,5 +308,190 @@ module AbstractController assert_equal "Hello world Howdy!", controller.response_body end end + + class TestCallbacksWithMissingConditions < ActiveSupport::TestCase + class CallbacksWithMissingOnly < ControllerWithCallbacks + before_action :callback, only: :showw + + def index + end + + def show + end + + private + def callback + end + end + + test "callbacks raise exception when their 'only' condition is a missing action" do + with_raise_on_missing_callback_actions do + controller = CallbacksWithMissingOnly.new + assert_raises(AbstractController::ActionNotFound) do + controller.process(:index) + end + end + end + + class CallbacksWithMissingOnlyInArray < ControllerWithCallbacks + before_action :callback, only: [:index, :showw] + + def index + end + + def show + end + + private + def callback + end + end + + test "callbacks raise exception when their 'only' array condition contains a missing action" do + with_raise_on_missing_callback_actions do + controller = CallbacksWithMissingOnlyInArray.new + assert_raises(AbstractController::ActionNotFound) do + controller.process(:index) + end + end + end + + class CallbacksWithMissingExcept < ControllerWithCallbacks + before_action :callback, except: :showw + + def index + end + + def show + end + + private + def callback + end + end + + test "callbacks raise exception when their 'except' condition is a missing action" do + with_raise_on_missing_callback_actions do + controller = CallbacksWithMissingExcept.new + assert_raises(AbstractController::ActionNotFound) do + controller.process(:index) + end + end + end + + class CallbacksWithMissingExceptInArray < ControllerWithCallbacks + before_action :callback, except: [:index, :showw] + + def index + end + + def show + end + + private + def callback + end + end + + test "callbacks raise exception when their 'except' array condition contains a missing action" do + with_raise_on_missing_callback_actions do + controller = CallbacksWithMissingExceptInArray.new + assert_raises(AbstractController::ActionNotFound) do + controller.process(:index) + end + end + end + + class MultipleCallbacksWithMissingOnly < ControllerWithCallbacks + before_action :callback1, :callback2, ->() { }, only: :showw + + def index + end + + def show + end + + private + def callback1 + end + + def callback2 + end + end + + test "raised exception message includes the names of callback actions and missing conditional action" do + with_raise_on_missing_callback_actions do + controller = MultipleCallbacksWithMissingOnly.new + error = assert_raises(AbstractController::ActionNotFound) do + controller.process(:index) + end + + assert_includes error.message, ":callback1" + assert_includes error.message, ":callback2" + assert_includes error.message, "#