From b4cab6a8547dbb12211efc0b086a80712df3278d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 29 Dec 2020 03:56:54 +0000 Subject: [PATCH] Make sure the :if options of callbacks is not mutated In some cases, the framework was mutating the :if option of callbacks. Since #38323, those options are frozen, so the framework could raise exception when trying to mutate those options if they were being resued with method like `with_options`. --- activemodel/lib/active_model/callbacks.rb | 2 +- .../lib/active_model/validations/callbacks.rb | 29 +++++++++---------- activemodel/test/cases/callbacks_test.rb | 10 +++++++ .../test/cases/validations/callbacks_test.rb | 20 +++++++++++++ .../lib/active_record/transactions.rb | 6 ++-- .../test/cases/transaction_callbacks_test.rb | 12 ++++++++ 6 files changed, 61 insertions(+), 18 deletions(-) diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index 7a544395cb..dc8f798f6a 100644 --- a/activemodel/lib/active_model/callbacks.rb +++ b/activemodel/lib/active_model/callbacks.rb @@ -147,7 +147,7 @@ module ActiveModel conditional = ActiveSupport::Callbacks::Conditionals::Value.new { |v| v != false } - options[:if] = Array(options[:if]) << conditional + options[:if] = Array(options[:if]) + [conditional] set_callback(:"#{callback}", :after, *args, options, &block) end end diff --git a/activemodel/lib/active_model/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb index 7178ba99e9..ca371bcc7f 100644 --- a/activemodel/lib/active_model/validations/callbacks.rb +++ b/activemodel/lib/active_model/validations/callbacks.rb @@ -56,14 +56,7 @@ module ActiveModel def before_validation(*args, &block) options = args.extract_options! - if options.key?(:on) - options = options.dup - options[:on] = Array(options[:on]) - options[:if] = Array(options[:if]) - options[:if].unshift ->(o) { - !(options[:on] & Array(o.validation_context)).empty? - } - end + set_options_for_callback(options) set_callback(:validation, :before, *args, options, &block) end @@ -99,13 +92,7 @@ module ActiveModel options = options.dup options[:prepend] = true - if options.key?(:on) - options[:on] = Array(options[:on]) - options[:if] = Array(options[:if]) - options[:if].unshift ->(o) { - !(options[:on] & Array(o.validation_context)).empty? - } - end + set_options_for_callback(options) set_callback(:validation, :after, *args, options, &block) end @@ -116,6 +103,18 @@ module ActiveModel def run_validations! _run_validation_callbacks { super } end + + def set_options_for_callback(options) + if options.key?(:on) + options[:on] = Array(options[:on]) + options[:if] = [ + ->(o) { + !(options[:on] & Array(o.validation_context)).empty? + }, + *options[:if] + ] + end + end end end end diff --git a/activemodel/test/cases/callbacks_test.rb b/activemodel/test/cases/callbacks_test.rb index 0711dc56ca..803101a46f 100644 --- a/activemodel/test/cases/callbacks_test.rb +++ b/activemodel/test/cases/callbacks_test.rb @@ -102,6 +102,16 @@ class CallbacksTest < ActiveModel::TestCase assert_not_respond_to ModelCallbacks, :after_empty end + test "the :if option array should not be mutated by an after callback" do + opts = [] + + Class.new(ModelCallbacks) do + after_create(if: opts) { } + end + + assert_empty opts + end + class Violin attr_reader :history def initialize diff --git a/activemodel/test/cases/validations/callbacks_test.rb b/activemodel/test/cases/validations/callbacks_test.rb index ff3cf61746..9d97c56b19 100644 --- a/activemodel/test/cases/validations/callbacks_test.rb +++ b/activemodel/test/cases/validations/callbacks_test.rb @@ -185,4 +185,24 @@ class CallbacksWithMethodNamesShouldBeCalled < ActiveModel::TestCase assert_equal ["before_validation_marker"], d.history assert_equal false, output end + + def test_before_validation_does_not_mutate_the_if_options_array + opts = [] + + Class.new(Dog) do + before_validation(if: opts, on: :create) { } + end + + assert_empty opts + end + + def test_after_validation_does_not_mutate_the_if_options_array + opts = [] + + Class.new(Dog) do + after_validation(if: opts, on: :create) { } + end + + assert_empty opts + end end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index cf0eeb6a88..9eb08b198d 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -271,8 +271,10 @@ module ActiveRecord if options[:on] fire_on = Array(options[:on]) assert_valid_transaction_action(fire_on) - options[:if] = Array(options[:if]) - options[:if].unshift(-> { transaction_include_any_action?(fire_on) }) + options[:if] = [ + -> { transaction_include_any_action?(fire_on) }, + *options[:if] + ] end end diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index dbc450b94f..a1dfa6f3f3 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -538,6 +538,18 @@ class TransactionCallbacksTest < ActiveRecord::TestCase assert_equal [:after_rollback], records.second.history end + def test_after_commit_does_not_mutate_the_if_options_array + opts = [] + + Class.new(ActiveRecord::Base) do + self.table_name = "topics" + + after_commit(if: opts, on: :create) { } + end + + assert_empty opts + end + private def add_transaction_execution_blocks(record) record.after_commit_block(:create) { |r| r.history << :commit_on_create }