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.
This commit is contained in:
parent
d12eca1f40
commit
60f7d49033
|
@ -33,14 +33,26 @@ module AbstractController
|
||||||
define_callbacks :process_action,
|
define_callbacks :process_action,
|
||||||
terminator: ->(controller, result_lambda) { result_lambda.call; controller.performed? },
|
terminator: ->(controller, result_lambda) { result_lambda.call; controller.performed? },
|
||||||
skip_after_callbacks_if_terminated: true
|
skip_after_callbacks_if_terminated: true
|
||||||
|
mattr_accessor :raise_on_missing_callback_actions, default: false
|
||||||
end
|
end
|
||||||
|
|
||||||
class ActionFilter # :nodoc:
|
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
|
@actions = Array(actions).map(&:to_s).to_set
|
||||||
end
|
end
|
||||||
|
|
||||||
def match?(controller)
|
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)
|
@actions.include?(controller.action_name)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -75,9 +87,10 @@ module AbstractController
|
||||||
end
|
end
|
||||||
|
|
||||||
def _normalize_callback_option(options, from, to) # :nodoc:
|
def _normalize_callback_option(options, from, to) # :nodoc:
|
||||||
if from = options.delete(from)
|
if from_value = options.delete(from)
|
||||||
from = ActionFilter.new(from)
|
filters = options[:filters]
|
||||||
options[to] = Array(options[to]).unshift(from)
|
from_value = ActionFilter.new(filters, from, from_value)
|
||||||
|
options[to] = Array(options[to]).unshift(from_value)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -95,8 +108,10 @@ module AbstractController
|
||||||
# * <tt>options</tt> - A hash of options to be used when adding the callback.
|
# * <tt>options</tt> - A hash of options to be used when adding the callback.
|
||||||
def _insert_callbacks(callbacks, block = nil)
|
def _insert_callbacks(callbacks, block = nil)
|
||||||
options = callbacks.extract_options!
|
options = callbacks.extract_options!
|
||||||
_normalize_callback_options(options)
|
|
||||||
callbacks.push(block) if block
|
callbacks.push(block) if block
|
||||||
|
options[:filters] = callbacks
|
||||||
|
_normalize_callback_options(options)
|
||||||
|
options.delete(:filters)
|
||||||
callbacks.each do |callback|
|
callbacks.each do |callback|
|
||||||
yield callback, options
|
yield callback, options
|
||||||
end
|
end
|
||||||
|
|
|
@ -308,5 +308,190 @@ module AbstractController
|
||||||
assert_equal "Hello world Howdy!", controller.response_body
|
assert_equal "Hello world Howdy!", controller.response_body
|
||||||
end
|
end
|
||||||
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, "#<Proc:"
|
||||||
|
assert_includes error.message, "only"
|
||||||
|
assert_includes error.message, "showw"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
class BlockCallbackWithMissingOnly < ControllerWithCallbacks
|
||||||
|
before_action only: :showw do
|
||||||
|
# Callback body
|
||||||
|
end
|
||||||
|
|
||||||
|
def index
|
||||||
|
end
|
||||||
|
|
||||||
|
def show
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
test "raised exception message includes a block callback" do
|
||||||
|
with_raise_on_missing_callback_actions do
|
||||||
|
controller = BlockCallbackWithMissingOnly.new
|
||||||
|
error = assert_raises(AbstractController::ActionNotFound) do
|
||||||
|
controller.process(:index)
|
||||||
|
end
|
||||||
|
|
||||||
|
assert_includes error.message, "#<Proc:"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
class CallbacksWithBothOnlyAndExcept < ControllerWithCallbacks
|
||||||
|
before_action :callback, only: [:index, :show], except: :showw
|
||||||
|
|
||||||
|
def index
|
||||||
|
end
|
||||||
|
|
||||||
|
def show
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
def callback
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
test "callbacks with both :only and :except options raise an exception with the correct message" do
|
||||||
|
with_raise_on_missing_callback_actions do
|
||||||
|
controller = CallbacksWithBothOnlyAndExcept.new
|
||||||
|
error = assert_raises(AbstractController::ActionNotFound) do
|
||||||
|
controller.process(:index)
|
||||||
|
end
|
||||||
|
|
||||||
|
assert_includes error.message, ":callback"
|
||||||
|
assert_includes error.message, "except"
|
||||||
|
assert_includes error.message, "showw"
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
def with_raise_on_missing_callback_actions
|
||||||
|
old_raise_on_missing_callback_actions = ControllerWithCallbacks.raise_on_missing_callback_actions
|
||||||
|
ControllerWithCallbacks.raise_on_missing_callback_actions = true
|
||||||
|
yield
|
||||||
|
ensure
|
||||||
|
ControllerWithCallbacks.raise_on_missing_callback_actions = old_raise_on_missing_callback_actions
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -245,8 +245,8 @@ module Rails
|
||||||
end
|
end
|
||||||
|
|
||||||
if respond_to?(:action_controller)
|
if respond_to?(:action_controller)
|
||||||
|
action_controller.raise_on_missing_callback_actions = false
|
||||||
action_controller.raise_on_open_redirects = true
|
action_controller.raise_on_open_redirects = true
|
||||||
|
|
||||||
action_controller.wrap_parameters_by_default = true
|
action_controller.wrap_parameters_by_default = true
|
||||||
end
|
end
|
||||||
when "7.1"
|
when "7.1"
|
||||||
|
|
|
@ -77,4 +77,7 @@ Rails.application.configure do
|
||||||
|
|
||||||
# Uncomment if you wish to allow Action Cable access from any origin.
|
# Uncomment if you wish to allow Action Cable access from any origin.
|
||||||
# config.action_cable.disable_request_forgery_protection = true
|
# config.action_cable.disable_request_forgery_protection = true
|
||||||
|
|
||||||
|
# Raise error when a before_action's only/except options reference missing actions
|
||||||
|
config.action_controller.raise_on_missing_callback_actions = true
|
||||||
end
|
end
|
||||||
|
|
|
@ -61,4 +61,7 @@ Rails.application.configure do
|
||||||
|
|
||||||
# Annotate rendered view with file names.
|
# Annotate rendered view with file names.
|
||||||
# config.action_view.annotate_rendered_view_with_filenames = true
|
# config.action_view.annotate_rendered_view_with_filenames = true
|
||||||
|
|
||||||
|
# Raise error when a before_action's only/except options reference missing actions
|
||||||
|
config.action_controller.raise_on_missing_callback_actions = true
|
||||||
end
|
end
|
||||||
|
|
|
@ -3505,6 +3505,32 @@ module ApplicationTests
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
test "ActionController::Base.raise_on_missing_callback_actions is false by default for production" do
|
||||||
|
app "production"
|
||||||
|
|
||||||
|
assert_equal false, ActionController::Base.raise_on_missing_callback_actions
|
||||||
|
end
|
||||||
|
|
||||||
|
test "ActionController::Base.raise_on_missing_callback_actions is false by default for upgraded apps" do
|
||||||
|
remove_from_config '.*config\.load_defaults.*\n'
|
||||||
|
|
||||||
|
app "development"
|
||||||
|
|
||||||
|
assert_equal false, ActionController::Base.raise_on_missing_callback_actions
|
||||||
|
end
|
||||||
|
|
||||||
|
test "ActionController::Base.raise_on_missing_callback_actions can be configured in the new framework defaults" do
|
||||||
|
remove_from_config '.*config\.load_defaults.*\n'
|
||||||
|
|
||||||
|
app_file "config/initializers/new_framework_defaults_6_2.rb", <<-RUBY
|
||||||
|
Rails.application.config.action_controller.raise_on_missing_callback_actions = true
|
||||||
|
RUBY
|
||||||
|
|
||||||
|
app "production"
|
||||||
|
|
||||||
|
assert_equal true, ActionController::Base.raise_on_missing_callback_actions
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
def set_custom_config(contents, config_source = "custom".inspect)
|
def set_custom_config(contents, config_source = "custom".inspect)
|
||||||
app_file "config/custom.yml", contents
|
app_file "config/custom.yml", contents
|
||||||
|
|
Loading…
Reference in New Issue