diff options
20 files changed, 299 insertions, 137 deletions
diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 0f2fa5fb08..572d1770f7 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -30,9 +30,9 @@ module ActionController :_recall => request.path_parameters }.merge!(super).freeze - if (same_origin = _routes.equal?(env["action_dispatch.routes".freeze])) || - (script_name = env["ROUTES_#{_routes.object_id}_SCRIPT_NAME"]) || - (original_script_name = env['ORIGINAL_SCRIPT_NAME'.freeze]) + if (same_origin = _routes.equal?(request.routes)) || + (script_name = request.engine_script_name(_routes)) || + (original_script_name = request.original_script_name) options = @_url_options.dup if original_script_name diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 2a7bb374a5..d211ea2b77 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -105,6 +105,18 @@ module ActionDispatch @request_method ||= check_method(env["REQUEST_METHOD"]) end + def routes # :nodoc: + env["action_dispatch.routes".freeze] + end + + def original_script_name # :nodoc: + env['ORIGINAL_SCRIPT_NAME'.freeze] + end + + def engine_script_name(_routes) # :nodoc: + env["ROUTES_#{_routes.object_id}_SCRIPT_NAME"] + end + def request_method=(request_method) #:nodoc: if check_method(request_method) @request_method = env["REQUEST_METHOD"] = request_method diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index f9c481998e..c9d9484518 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1 +1,26 @@ +* `ActiveJob::Base.deserialize` delegates to the job class + + Since `ActiveJob::Base#deserialize` can be overriden by subclasses (like `ActiveJob::Base#serialize`) + this allows jobs to attach arbitrary metadata when they get serialized and read it back when they get + performed. E.g. + + class DeliverWebhookJob < ActiveJob::Base + def serialize + super.merge('attempt_number' => (@attempt_number || 0) + 1) + end + + def deserialize(job_data) + super(job_data) + @attempt_number = job_data['attempt_number'] + end + + rescue_from(TimeoutError) do |ex| + raise ex if @attempt_number > 5 + retry_job(wait: 10) + end + end + + *Isaac Seymour* + + Please check [4-2-stable](https://github.com/rails/rails/blob/4-2-stable/activejob/CHANGELOG.md) for previous changes. diff --git a/activejob/Rakefile b/activejob/Rakefile index 7e66860b36..b215ba65fa 100644 --- a/activejob/Rakefile +++ b/activejob/Rakefile @@ -1,7 +1,7 @@ require 'rake/testtask' require 'rubygems/package_task' -ACTIVEJOB_ADAPTERS = %w(inline delayed_job qu que queue_classic resque sidekiq sneakers sucker_punch backburner) +ACTIVEJOB_ADAPTERS = %w(inline delayed_job qu que queue_classic resque sidekiq sneakers sucker_punch backburner test) ACTIVEJOB_ADAPTERS -= %w(queue_classic) if defined?(JRUBY_VERSION) task default: :test diff --git a/activejob/lib/active_job/core.rb b/activejob/lib/active_job/core.rb index a0e55a0028..450c56bfdc 100644 --- a/activejob/lib/active_job/core.rb +++ b/activejob/lib/active_job/core.rb @@ -22,10 +22,8 @@ module ActiveJob module ClassMethods # Creates a new job instance from a hash created with +serialize+ def deserialize(job_data) - job = job_data['job_class'].constantize.new - job.job_id = job_data['job_id'] - job.queue_name = job_data['queue_name'] - job.serialized_arguments = job_data['arguments'] + job = job_data['job_class'].constantize.new + job.deserialize(job_data) job end @@ -69,6 +67,32 @@ module ActiveJob } end + # Attaches the stored job data to the current instance. Receives a hash + # returned from +serialize+ + # + # ==== Examples + # + # class DeliverWebhookJob < ActiveJob::Base + # def serialize + # super.merge('attempt_number' => (@attempt_number || 0) + 1) + # end + # + # def deserialize(job_data) + # super(job_data) + # @attempt_number = job_data['attempt_number'] + # end + # + # rescue_from(TimeoutError) do |ex| + # raise ex if @attempt_number > 5 + # retry_job(wait: 10) + # end + # end + def deserialize(job_data) + self.job_id = job_data['job_id'] + self.queue_name = job_data['queue_name'] + self.serialized_arguments = job_data['arguments'] + end + private def deserialize_arguments_if_needed if defined?(@serialized_arguments) && @serialized_arguments.present? diff --git a/activejob/lib/active_job/queue_adapter.rb b/activejob/lib/active_job/queue_adapter.rb index 85d7c44bb8..d48d56e1da 100644 --- a/activejob/lib/active_job/queue_adapter.rb +++ b/activejob/lib/active_job/queue_adapter.rb @@ -2,7 +2,7 @@ require 'active_job/queue_adapters/inline_adapter' require 'active_support/core_ext/string/inflections' module ActiveJob - # The <tt>ActionJob::QueueAdapter</tt> module is used to load the + # The <tt>ActionJob::QueueAdapter</tt> module is used to load the # correct adapter. The default queue adapter is the :inline queue. module QueueAdapter #:nodoc: extend ActiveSupport::Concern @@ -21,8 +21,8 @@ module ActiveJob ActiveJob::QueueAdapters::TestAdapter.new when Symbol, String load_adapter(name_or_adapter) - when Class - name_or_adapter + else + name_or_adapter if name_or_adapter.respond_to?(:enqueue) end end diff --git a/activejob/lib/active_job/queue_adapters/test_adapter.rb b/activejob/lib/active_job/queue_adapters/test_adapter.rb index e4fdf60008..ea9df9a063 100644 --- a/activejob/lib/active_job/queue_adapters/test_adapter.rb +++ b/activejob/lib/active_job/queue_adapters/test_adapter.rb @@ -14,6 +14,11 @@ module ActiveJob attr_accessor(:perform_enqueued_jobs, :perform_enqueued_at_jobs) attr_writer(:enqueued_jobs, :performed_jobs) + def initialize + self.perform_enqueued_jobs = false + self.perform_enqueued_at_jobs = false + end + # Provides a store of all the enqueued jobs with the TestAdapter so you can check them. def enqueued_jobs @enqueued_jobs ||= [] @@ -26,19 +31,19 @@ module ActiveJob def enqueue(job) #:nodoc: if perform_enqueued_jobs - performed_jobs << {job: job.class, args: job.arguments, queue: job.queue_name} - job.perform_now + performed_jobs << {job: job.class, args: job.serialize['arguments'], queue: job.queue_name} + Base.execute job.serialize else - enqueued_jobs << {job: job.class, args: job.arguments, queue: job.queue_name} + enqueued_jobs << {job: job.class, args: job.serialize['arguments'], queue: job.queue_name} end end def enqueue_at(job, timestamp) #:nodoc: if perform_enqueued_at_jobs - performed_jobs << {job: job.class, args: job.arguments, queue: job.queue_name, at: timestamp} - job.perform_now + performed_jobs << {job: job.class, args: job.serialize['arguments'], queue: job.queue_name, at: timestamp} + Base.execute job.serialize else - enqueued_jobs << {job: job.class, args: job.arguments, queue: job.queue_name, at: timestamp} + enqueued_jobs << {job: job.class, args: job.serialize['arguments'], queue: job.queue_name, at: timestamp} end end end diff --git a/activejob/lib/active_job/test_helper.rb b/activejob/lib/active_job/test_helper.rb index 1720b140e5..2efcea7f2e 100644 --- a/activejob/lib/active_job/test_helper.rb +++ b/activejob/lib/active_job/test_helper.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/hash/keys' + module ActiveJob # Provides helper methods for testing Active Job module TestHelper diff --git a/activejob/test/adapters/test.rb b/activejob/test/adapters/test.rb new file mode 100644 index 0000000000..7180b38a57 --- /dev/null +++ b/activejob/test/adapters/test.rb @@ -0,0 +1,3 @@ +ActiveJob::Base.queue_adapter = :test +ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true +ActiveJob::Base.queue_adapter.perform_enqueued_at_jobs = true diff --git a/activejob/test/cases/adapter_test.rb b/activejob/test/cases/adapter_test.rb index 61a7cae5bf..6570c55a83 100644 --- a/activejob/test/cases/adapter_test.rb +++ b/activejob/test/cases/adapter_test.rb @@ -2,7 +2,6 @@ require 'helper' class AdapterTest < ActiveSupport::TestCase test "should load #{ENV['AJADAPTER']} adapter" do - ActiveJob::Base.queue_adapter = ENV['AJADAPTER'].to_sym - assert_equal "active_job/queue_adapters/#{ENV['AJADAPTER']}_adapter".classify.constantize, ActiveJob::Base.queue_adapter + assert_equal "active_job/queue_adapters/#{ENV['AJADAPTER']}_adapter".classify, ActiveJob::Base.queue_adapter.name end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index c78ddd5fe6..859ba38588 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,16 @@ +* Fix error message when trying to create an associated record and the foreign + key is missing. + + Before this fix the following exception was being raised: + + NoMethodError: undefined method `val' for #<Arel::Nodes::BindParam:0x007fc64d19c218> + + Now the message is: + + ActiveRecord::UnknownAttributeError: unknown attribute 'foreign_key' for Model. + + *Rafael Mendonça França* + * When a table has a composite primary key, the `primary_key` method for SQLite3 and PostgreSQL adapters was only returning the first field of the key. Ensures that it will return nil instead, as Active Record doesn't support diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index cd5fdd5964..14af55f327 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -116,6 +116,7 @@ module ActiveRecord autoload :Association, 'active_record/associations/association' autoload :SingularAssociation, 'active_record/associations/singular_association' autoload :CollectionAssociation, 'active_record/associations/collection_association' + autoload :ForeignAssociation, 'active_record/associations/foreign_association' autoload :CollectionProxy, 'active_record/associations/collection_proxy' autoload :BelongsToAssociation, 'active_record/associations/belongs_to_association' diff --git a/activerecord/lib/active_record/associations/foreign_association.rb b/activerecord/lib/active_record/associations/foreign_association.rb new file mode 100644 index 0000000000..fe48ecec29 --- /dev/null +++ b/activerecord/lib/active_record/associations/foreign_association.rb @@ -0,0 +1,11 @@ +module ActiveRecord::Associations + module ForeignAssociation + def foreign_key_present? + if reflection.klass.primary_key + owner.attribute_present?(reflection.active_record_primary_key) + else + false + end + end + end +end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 93084e0dcf..d7f655d00c 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -6,6 +6,7 @@ module ActiveRecord # If the association has a <tt>:through</tt> option further specialization # is provided by its child HasManyThroughAssociation. class HasManyAssociation < CollectionAssociation #:nodoc: + include ForeignAssociation def handle_dependency case options[:dependent] @@ -153,14 +154,6 @@ module ActiveRecord end end - def foreign_key_present? - if reflection.klass.primary_key - owner.attribute_present?(reflection.association_primary_key) - else - false - end - end - def concat_records(records, *) update_counter_if_success(super, records.length) end diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index e6095d84dc..74b8c53758 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -2,6 +2,7 @@ module ActiveRecord # = Active Record Belongs To Has One Association module Associations class HasOneAssociation < SingularAssociation #:nodoc: + include ForeignAssociation def handle_dependency case options[:dependent] diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 2f067d867c..cdafa6a50a 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -569,7 +569,7 @@ module ActiveRecord [name, binds.fetch(name.to_s) { case where.right when Array then where.right.map(&:val) - else + when Arel::Nodes::Casted, Arel::Nodes::Quoted where.right.val end }] diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index d3b74aa616..21a45042fa 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -31,6 +31,8 @@ require 'models/student' require 'models/pirate' require 'models/ship' require 'models/tyre' +require 'models/subscriber' +require 'models/subscription' class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCase fixtures :authors, :posts, :comments @@ -43,12 +45,59 @@ class HasManyAssociationsTestForReorderWithJoinDependency < ActiveRecord::TestCa end end +class HasManyAssociationsTestPrimaryKeys < ActiveRecord::TestCase + fixtures :authors, :essays, :subscribers, :subscriptions, :people + + def test_custom_primary_key_on_new_record_should_fetch_with_query + subscriber = Subscriber.new(nick: 'webster132') + assert !subscriber.subscriptions.loaded? + + assert_queries 1 do + assert_equal 2, subscriber.subscriptions.size + end + + assert_equal subscriber.subscriptions, Subscription.where(subscriber_id: 'webster132') + end + + def test_association_primary_key_on_new_record_should_fetch_with_query + author = Author.new(:name => "David") + assert !author.essays.loaded? + + assert_queries 1 do + assert_equal 1, author.essays.size + end + + assert_equal author.essays, Essay.where(writer_id: "David") + end + + def test_has_many_custom_primary_key + david = authors(:david) + assert_equal david.essays, Essay.where(writer_id: "David") + end + + def test_has_many_assignment_with_custom_primary_key + david = people(:david) + + assert_equal ["A Modest Proposal"], david.essays.map(&:name) + david.essays = [Essay.create!(name: "Remote Work" )] + assert_equal ["Remote Work"], david.essays.map(&:name) + end + + def test_blank_custom_primary_key_on_new_record_should_not_run_queries + author = Author.new + assert !author.essays.loaded? + + assert_queries 0 do + assert_equal 0, author.essays.size + end + end +end class HasManyAssociationsTest < ActiveRecord::TestCase fixtures :accounts, :categories, :companies, :developers, :projects, :developers_projects, :topics, :authors, :comments, - :people, :posts, :readers, :taggings, :cars, :essays, - :categorizations, :jobs, :tags + :posts, :readers, :taggings, :cars, :jobs, :tags, + :categorizations def setup Client.destroyed_client_ids.clear @@ -1578,39 +1627,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase end end - def test_custom_primary_key_on_new_record_should_fetch_with_query - author = Author.new(:name => "David") - assert !author.essays.loaded? - - assert_queries 1 do - assert_equal 1, author.essays.size - end - - assert_equal author.essays, Essay.where(writer_id: "David") - end - - def test_has_many_custom_primary_key - david = authors(:david) - assert_equal david.essays, Essay.where(writer_id: "David") - end - - def test_has_many_assignment_with_custom_primary_key - david = people(:david) - - assert_equal ["A Modest Proposal"], david.essays.map(&:name) - david.essays = [Essay.create!(name: "Remote Work" )] - assert_equal ["Remote Work"], david.essays.map(&:name) - end - - def test_blank_custom_primary_key_on_new_record_should_not_run_queries - author = Author.new - assert !author.essays.loaded? - - assert_queries 0 do - assert_equal 0, author.essays.size - end - end - def test_calling_first_or_last_with_integer_on_association_should_not_load_association firm = companies(:first_firm) firm.clients.create(:name => 'Foo') diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index a69f7a5262..15b37ad709 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -273,6 +273,14 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal account, firm.reload.account end + def test_create_with_inexistent_foreign_key_failing + firm = Firm.create(name: 'GlobalMegaCorp') + + assert_raises(ActiveRecord::UnknownAttributeError) do + firm.create_account_with_inexistent_foreign_key + end + end + def test_build firm = Firm.new("name" => "GlobalMegaCorp") firm.save @@ -566,6 +574,12 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal author.post, post end + def test_has_one_loading_for_new_record + post = Post.create!(author_id: 42, title: 'foo', body: 'bar') + author = Author.new(id: 42) + assert author.post + end + def test_has_one_relationship_cannot_have_a_counter_cache assert_raise(ArgumentError) do Class.new(ActiveRecord::Base) do diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 42f7fb4680..5a56616eb9 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -72,6 +72,7 @@ class Firm < Company # Oracle tests were failing because of that as the second fixture was selected has_one :account_using_primary_key, -> { order('id') }, :primary_key => "firm_id", :class_name => "Account" has_one :account_using_foreign_and_primary_keys, :foreign_key => "firm_name", :primary_key => "name", :class_name => "Account" + has_one :account_with_inexistent_foreign_key, class_name: 'Account', foreign_key: "inexistent" has_one :deletable_account, :foreign_key => "firm_id", :class_name => "Account", :dependent => :delete has_one :account_limit_500_with_hash_conditions, -> { where :credit_limit => 500 }, :foreign_key => "firm_id", :class_name => "Account" diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 95dbc9a0cb..d2911a254c 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -121,22 +121,22 @@ module ActiveSupport ENDING = End.new class Before - def self.build(next_callback, user_callback, user_conditions, chain_config, filter) + def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter) halted_lambda = chain_config[:terminator] if chain_config.key?(:terminator) && user_conditions.any? - halting_and_conditional(next_callback, user_callback, user_conditions, halted_lambda, filter) + halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter) elsif chain_config.key? :terminator - halting(next_callback, user_callback, halted_lambda, filter) + halting(callback_sequence, user_callback, halted_lambda, filter) elsif user_conditions.any? - conditional(next_callback, user_callback, user_conditions) + conditional(callback_sequence, user_callback, user_conditions) else - simple next_callback, user_callback + simple callback_sequence, user_callback end end - def self.halting_and_conditional(next_callback, user_callback, user_conditions, halted_lambda, filter) - lambda { |env| + def self.halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter) + callback_sequence.before do |env| target = env.target value = env.value halted = env.halted @@ -148,13 +148,14 @@ module ActiveSupport target.send :halted_callback_hook, filter end end - next_callback.call env - } + + env + end end private_class_method :halting_and_conditional - def self.halting(next_callback, user_callback, halted_lambda, filter) - lambda { |env| + def self.halting(callback_sequence, user_callback, halted_lambda, filter) + callback_sequence.before do |env| target = env.target value = env.value halted = env.halted @@ -166,57 +167,59 @@ module ActiveSupport target.send :halted_callback_hook, filter end end - next_callback.call env - } + + env + end end private_class_method :halting - def self.conditional(next_callback, user_callback, user_conditions) - lambda { |env| + def self.conditional(callback_sequence, user_callback, user_conditions) + callback_sequence.before do |env| target = env.target value = env.value if user_conditions.all? { |c| c.call(target, value) } user_callback.call target, value end - next_callback.call env - } + + env + end end private_class_method :conditional - def self.simple(next_callback, user_callback) - lambda { |env| + def self.simple(callback_sequence, user_callback) + callback_sequence.before do |env| user_callback.call env.target, env.value - next_callback.call env - } + + env + end end private_class_method :simple end class After - def self.build(next_callback, user_callback, user_conditions, chain_config) + def self.build(callback_sequence, user_callback, user_conditions, chain_config) if chain_config[:skip_after_callbacks_if_terminated] if chain_config.key?(:terminator) && user_conditions.any? - halting_and_conditional(next_callback, user_callback, user_conditions) + halting_and_conditional(callback_sequence, user_callback, user_conditions) elsif chain_config.key?(:terminator) - halting(next_callback, user_callback) + halting(callback_sequence, user_callback) elsif user_conditions.any? - conditional next_callback, user_callback, user_conditions + conditional callback_sequence, user_callback, user_conditions else - simple next_callback, user_callback + simple callback_sequence, user_callback end else if user_conditions.any? - conditional next_callback, user_callback, user_conditions + conditional callback_sequence, user_callback, user_conditions else - simple next_callback, user_callback + simple callback_sequence, user_callback end end end - def self.halting_and_conditional(next_callback, user_callback, user_conditions) - lambda { |env| - env = next_callback.call env + def self.halting_and_conditional(callback_sequence, user_callback, user_conditions) + callback_sequence.after do |env| target = env.target value = env.value halted = env.halted @@ -224,122 +227,124 @@ module ActiveSupport if !halted && user_conditions.all? { |c| c.call(target, value) } user_callback.call target, value end + env - } + end end private_class_method :halting_and_conditional - def self.halting(next_callback, user_callback) - lambda { |env| - env = next_callback.call env + def self.halting(callback_sequence, user_callback) + callback_sequence.after do |env| unless env.halted user_callback.call env.target, env.value end + env - } + end end private_class_method :halting - def self.conditional(next_callback, user_callback, user_conditions) - lambda { |env| - env = next_callback.call env + def self.conditional(callback_sequence, user_callback, user_conditions) + callback_sequence.after do |env| target = env.target value = env.value if user_conditions.all? { |c| c.call(target, value) } user_callback.call target, value end + env - } + end end private_class_method :conditional - def self.simple(next_callback, user_callback) - lambda { |env| - env = next_callback.call env + def self.simple(callback_sequence, user_callback) + callback_sequence.after do |env| user_callback.call env.target, env.value + env - } + end end private_class_method :simple end class Around - def self.build(next_callback, user_callback, user_conditions, chain_config) + def self.build(callback_sequence, user_callback, user_conditions, chain_config) if chain_config.key?(:terminator) && user_conditions.any? - halting_and_conditional(next_callback, user_callback, user_conditions) + halting_and_conditional(callback_sequence, user_callback, user_conditions) elsif chain_config.key? :terminator - halting(next_callback, user_callback) + halting(callback_sequence, user_callback) elsif user_conditions.any? - conditional(next_callback, user_callback, user_conditions) + conditional(callback_sequence, user_callback, user_conditions) else - simple(next_callback, user_callback) + simple(callback_sequence, user_callback) end end - def self.halting_and_conditional(next_callback, user_callback, user_conditions) - lambda { |env| + def self.halting_and_conditional(callback_sequence, user_callback, user_conditions) + callback_sequence.around do |env, &run| target = env.target value = env.value halted = env.halted if !halted && user_conditions.all? { |c| c.call(target, value) } user_callback.call(target, value) { - env = next_callback.call env + env = run.call env env.value } + env else - next_callback.call env + run.call env end - } + end end private_class_method :halting_and_conditional - def self.halting(next_callback, user_callback) - lambda { |env| + def self.halting(callback_sequence, user_callback) + callback_sequence.around do |env, &run| target = env.target value = env.value if env.halted - next_callback.call env + run.call env else user_callback.call(target, value) { - env = next_callback.call env + env = run.call env env.value } env end - } + end end private_class_method :halting - def self.conditional(next_callback, user_callback, user_conditions) - lambda { |env| + def self.conditional(callback_sequence, user_callback, user_conditions) + callback_sequence.around do |env, &run| target = env.target value = env.value if user_conditions.all? { |c| c.call(target, value) } user_callback.call(target, value) { - env = next_callback.call env + env = run.call env env.value } env else - next_callback.call env + run.call env end - } + end end private_class_method :conditional - def self.simple(next_callback, user_callback) - lambda { |env| + def self.simple(callback_sequence, user_callback) + callback_sequence.around do |env, &run| user_callback.call(env.target, env.value) { - env = next_callback.call env + env = run.call env env.value } env - } + end end private_class_method :simple end @@ -392,17 +397,17 @@ module ActiveSupport end # Wraps code with filter - def apply(next_callback) + def apply(callback_sequence) user_conditions = conditions_lambdas user_callback = make_lambda @filter case kind when :before - Filters::Before.build(next_callback, user_callback, user_conditions, chain_config, @filter) + Filters::Before.build(callback_sequence, user_callback, user_conditions, chain_config, @filter) when :after - Filters::After.build(next_callback, user_callback, user_conditions, chain_config) + Filters::After.build(callback_sequence, user_callback, user_conditions, chain_config) when :around - Filters::Around.build(next_callback, user_callback, user_conditions, chain_config) + Filters::Around.build(callback_sequence, user_callback, user_conditions, chain_config) end end @@ -467,6 +472,42 @@ module ActiveSupport end end + # Execute before and after filters in a sequence instead of + # chaining them with nested lambda calls, see: + # https://github.com/rails/rails/issues/18011 + class CallbackSequence + def initialize(&call) + @call = call + @before = [] + @after = [] + end + + def before(&before) + @before.unshift(before) + self + end + + def after(&after) + @after.push(after) + self + end + + def around(&around) + CallbackSequence.new do |*args| + around.call(*args) { + self.call(*args) + } + end + end + + def call(*args) + @before.each { |b| b.call(*args) } + value = @call.call(*args) + @after.each { |a| a.call(*args) } + value + end + end + # An Array with a compile method. class CallbackChain #:nodoc:# include Enumerable @@ -511,8 +552,9 @@ module ActiveSupport def compile @callbacks || @mutex.synchronize do - @callbacks ||= @chain.reverse.inject(Filters::ENDING) do |chain, callback| - callback.apply chain + final_sequence = CallbackSequence.new { |env| Filters::ENDING.call(env) } + @callbacks ||= @chain.reverse.inject(final_sequence) do |callback_sequence, callback| + callback.apply callback_sequence end end end |