diff options
-rw-r--r-- | Gemfile | 1 | ||||
-rw-r--r-- | actionpack/actionpack.gemspec | 2 | ||||
-rw-r--r-- | actionview/test/template/sanitize_helper_test.rb | 2 | ||||
-rw-r--r-- | activerecord/CHANGELOG.md | 14 | ||||
-rw-r--r-- | activerecord/lib/active_record/connection_adapters/abstract/transaction.rb | 30 | ||||
-rw-r--r-- | activerecord/lib/active_record/transactions.rb | 26 | ||||
-rw-r--r-- | activerecord/test/cases/helper.rb | 3 | ||||
-rw-r--r-- | activerecord/test/cases/transaction_callbacks_test.rb | 76 | ||||
-rw-r--r-- | activesupport/activesupport.gemspec | 2 | ||||
-rw-r--r-- | railties/lib/rails/generators/app_base.rb | 10 |
10 files changed, 143 insertions, 23 deletions
@@ -16,7 +16,6 @@ gem 'rails-html-sanitizer', github: 'rails/rails-html-sanitizer' #temporary gem until a new version of loofah is released gem 'loofah', github: 'kaspth/loofah', branch: 'single-scrub' gem 'sprockets-rails', github: 'rails/sprockets-rails', branch: 'master' -gem 'i18n', github: 'svenfuchs/i18n', branch: 'master' # require: false so bcrypt is loaded only when has_secure_password is used. # This is to avoid ActiveModel (and by extension the entire framework) diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec index 5834e79668..722e874c7e 100644 --- a/actionpack/actionpack.gemspec +++ b/actionpack/actionpack.gemspec @@ -21,7 +21,7 @@ Gem::Specification.new do |s| s.add_dependency 'activesupport', version - s.add_dependency 'rack', '~> 1.6.0.alpha' + s.add_dependency 'rack', '~> 1.6.0.beta' s.add_dependency 'rack-test', '~> 0.6.2' s.add_dependency 'rails-deprecated_sanitizer' s.add_dependency 'actionview', version diff --git a/actionview/test/template/sanitize_helper_test.rb b/actionview/test/template/sanitize_helper_test.rb index e4be21be2c..a27258a870 100644 --- a/actionview/test/template/sanitize_helper_test.rb +++ b/actionview/test/template/sanitize_helper_test.rb @@ -18,7 +18,7 @@ class SanitizeHelperTest < ActionView::TestCase def test_should_sanitize_illegal_style_properties raw = %(display:block; position:absolute; left:0; top:0; width:100%; height:100%; z-index:1; background-color:black; background-image:url(http://www.ragingplatypus.com/i/cam-full.jpg); background-x:center; background-y:center; background-repeat:repeat;) - expected = %(display: block; width: 100%; height: 100%; background-color: black; background-x: center; background-y: center;) + expected = %(display: block; width: 100%; height: 100%; background-color: black; background-image: ; background-x: center; background-y: center;) assert_equal expected, sanitize_css(raw) end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b09d9e336b..0fbe5bfaed 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,17 @@ +* Currently, Active Record will rescue any errors raised within + after_rollback/after_create callbacks and print them to the logs. Next versions of rails + will not rescue those errors anymore, and just bubble them up, as the other callbacks. + + This adds a opt-in flag to enable that behaviour, of not rescuing the errors. + Example: + + # For not swallow errors in after_commit/after_rollback callbacks. + config.active_record.errors_in_transactional_callbacks = true + + Fixes #13460. + + *arthurnn* + * Fixed an issue where custom accessor methods (such as those generated by `enum`) with the same name as a global method are incorrectly overridden when subclassing. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 8f06cf3a1f..90be835d8a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -22,6 +22,10 @@ module ActiveRecord @state == :rolledback end + def completed? + committed? || rolledback? + end + def set_state(state) if !VALID_STATES.include?(state) raise ArgumentError, "Invalid transaction state: #{state}" @@ -63,13 +67,19 @@ module ActiveRecord end def rollback_records - records.uniq.each do |record| + ite = records.uniq + while record = ite.shift begin record.rolledback! full_rollback? rescue => e + raise if ActiveRecord::Base.raise_in_transactional_callbacks record.logger.error(e) if record.respond_to?(:logger) && record.logger end end + ensure + ite.each do |i| + i.rolledback!(full_rollback?, false) + end end def commit @@ -77,13 +87,19 @@ module ActiveRecord end def commit_records - records.uniq.each do |record| + ite = records.uniq + while record = ite.shift begin record.committed! rescue => e + raise if ActiveRecord::Base.raise_in_transactional_callbacks record.logger.error(e) if record.respond_to?(:logger) && record.logger end end + ensure + ite.each do |i| + i.committed!(false) + end end def full_rollback?; true; end @@ -103,14 +119,14 @@ module ActiveRecord end def rollback - super connection.rollback_to_savepoint(savepoint_name) + super rollback_records end def commit - super connection.release_savepoint(savepoint_name) + super parent = connection.transaction_manager.current_transaction records.each { |r| parent.add_record(r) } end @@ -130,14 +146,14 @@ module ActiveRecord end def rollback - super connection.rollback_db_transaction + super rollback_records end def commit - super connection.commit_db_transaction + super commit_records end end @@ -177,7 +193,7 @@ module ActiveRecord begin commit_transaction unless error rescue Exception - transaction.rollback + transaction.rollback unless transaction.state.completed? raise end end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 7e4dc4c895..9a56291b35 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -3,11 +3,23 @@ module ActiveRecord module Transactions extend ActiveSupport::Concern ACTIONS = [:create, :destroy, :update] + CALLBACK_WARN_MESSAGE = <<-EOF +Currently, Active Record will rescue any errors raised within +after_rollback/after_create callbacks and print them to the logs. In the next +version, these errors will no longer be rescued. Instead, they will simply +bubble just like other Active Record callbacks. + +You can opt into the new behavior and remove this warning by setting +config.active_record.raise_in_transactional_callbacks to true. +EOF included do define_callbacks :commit, :rollback, terminator: ->(_, result) { result == false }, scope: [:kind, :name] + + mattr_accessor :raise_in_transactional_callbacks, instance_writer: false + self.raise_in_transactional_callbacks = false end # = Active Record Transactions @@ -223,6 +235,9 @@ module ActiveRecord def after_commit(*args, &block) set_options_for_callbacks!(args) set_callback(:commit, :after, *args, &block) + unless ActiveRecord::Base.raise_in_transactional_callbacks + ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE) + end end # This callback is called after a create, update, or destroy are rolled back. @@ -231,6 +246,9 @@ module ActiveRecord def after_rollback(*args, &block) set_options_for_callbacks!(args) set_callback(:rollback, :after, *args, &block) + unless ActiveRecord::Base.raise_in_transactional_callbacks + ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE) + end end private @@ -290,16 +308,16 @@ module ActiveRecord # # Ensure that it is not called if the object was never persisted (failed create), # but call it after the commit of a destroyed object. - def committed! #:nodoc: - run_callbacks :commit if destroyed? || persisted? + def committed!(should_run_callbacks = true) #:nodoc: + run_callbacks :commit if should_run_callbacks && destroyed? || persisted? ensure force_clear_transaction_record_state end # Call the +after_rollback+ callbacks. The +force_restore_state+ argument indicates if the record # state should be rolled back to the beginning or just to the last savepoint. - def rolledback!(force_restore_state = false) #:nodoc: - run_callbacks :rollback + def rolledback!(force_restore_state = false, should_run_callbacks = true) #:nodoc: + run_callbacks :rollback if should_run_callbacks ensure restore_transaction_record_state(force_restore_state) clear_transaction_record_state diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 6a8aff4b69..e43b796237 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -24,6 +24,9 @@ ActiveSupport::Deprecation.debug = true # Disable available locale checks to avoid warnings running the test suite. I18n.enforce_available_locales = false +# Enable raise errors in after_commit and after_rollback. +ActiveRecord::Base.raise_in_transactional_callbacks = true + # Connect to the database ARTest.connect diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index a3f39804b7..b5ac1bdaf9 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -267,6 +267,9 @@ class TransactionCallbacksTest < ActiveRecord::TestCase end def test_after_transaction_callbacks_should_prevent_callbacks_from_being_called + old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks + ActiveRecord::Base.raise_in_transactional_callbacks = false + def @first.last_after_transaction_error=(e); @last_transaction_error = e; end def @first.last_after_transaction_error; @last_transaction_error; end @first.after_commit_block{|r| r.last_after_transaction_error = :commit; raise "fail!";} @@ -291,6 +294,79 @@ class TransactionCallbacksTest < ActiveRecord::TestCase end assert_equal :rollback, @first.last_after_transaction_error assert_equal [:after_rollback], second.history + ensure + ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config + end + + def test_after_commit_should_not_raise_when_raise_in_transactional_callbacks_false + old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks + ActiveRecord::Base.raise_in_transactional_callbacks = false + @first.after_commit_block{ fail "boom" } + Topic.transaction { @first.save! } + ensure + ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config + end + + def test_after_commit_callback_should_not_swallow_errors + @first.after_commit_block{ fail "boom" } + assert_raises(RuntimeError) do + Topic.transaction do + @first.save! + end + end + end + + def test_after_commit_callback_when_raise_should_not_restore_state + first = TopicWithCallbacks.new + second = TopicWithCallbacks.new + first.after_commit_block{ fail "boom" } + second.after_commit_block{ fail "boom" } + + begin + Topic.transaction do + first.save! + assert_not_nil first.id + second.save! + assert_not_nil second.id + end + rescue + end + assert_not_nil first.id + assert_not_nil second.id + assert first.reload + end + + def test_after_rollback_callback_should_not_swallow_errors_when_set_to_raise + error_class = Class.new(StandardError) + @first.after_rollback_block{ raise error_class } + assert_raises(error_class) do + Topic.transaction do + @first.save! + raise ActiveRecord::Rollback + end + end + end + + def test_after_rollback_callback_when_raise_should_restore_state + error_class = Class.new(StandardError) + + first = TopicWithCallbacks.new + second = TopicWithCallbacks.new + first.after_rollback_block{ raise error_class } + second.after_rollback_block{ raise error_class } + + begin + Topic.transaction do + first.save! + assert_not_nil first.id + second.save! + assert_not_nil second.id + raise ActiveRecord::Rollback + end + rescue error_class + end + assert_nil first.id + assert_nil second.id end def test_after_rollback_callbacks_should_validate_on_condition diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec index c0b457c341..80d1d35888 100644 --- a/activesupport/activesupport.gemspec +++ b/activesupport/activesupport.gemspec @@ -20,7 +20,7 @@ Gem::Specification.new do |s| s.rdoc_options.concat ['--encoding', 'UTF-8'] - s.add_dependency 'i18n', '>= 0.7.0.dev', '< 0.8' + s.add_dependency 'i18n', '>= 0.7.0.beta1', '< 0.8' s.add_dependency 'json', '~> 1.7', '>= 1.7.7' s.add_dependency 'tzinfo', '~> 1.1' s.add_dependency 'minitest', '~> 5.1' diff --git a/railties/lib/rails/generators/app_base.rb b/railties/lib/rails/generators/app_base.rb index caaaae09e6..2f6f760438 100644 --- a/railties/lib/rails/generators/app_base.rb +++ b/railties/lib/rails/generators/app_base.rb @@ -198,15 +198,9 @@ module Rails def rails_gemfile_entry if options.dev? - [GemfileEntry.path('rails', Rails::Generators::RAILS_DEV_PATH), - GemfileEntry.github('arel', 'rails/arel'), - GemfileEntry.github('rack', 'rack/rack'), - GemfileEntry.github('i18n', 'svenfuchs/i18n')] + [GemfileEntry.path('rails', Rails::Generators::RAILS_DEV_PATH)] elsif options.edge? - [GemfileEntry.github('rails', 'rails/rails'), - GemfileEntry.github('arel', 'rails/arel'), - GemfileEntry.github('rack', 'rack/rack'), - GemfileEntry.github('i18n', 'svenfuchs/i18n')] + [GemfileEntry.github('rails', 'rails/rails')] else [GemfileEntry.version('rails', Rails::VERSION::STRING, |