diff options
25 files changed, 122 insertions, 71 deletions
diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index dcb9d27f45..f9cb0e204d 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,3 +1,7 @@ +* Allow passing multiple exceptions to `retry_on`, and `discard_on`. + + *George Claghorn* + * Pass the error instance as the second parameter of block executed by `discard_on`. Fixes #32853. diff --git a/activejob/lib/active_job.rb b/activejob/lib/active_job.rb index 01fab4d918..69c02098a0 100644 --- a/activejob/lib/active_job.rb +++ b/activejob/lib/active_job.rb @@ -31,6 +31,7 @@ require "global_id" module ActiveJob extend ActiveSupport::Autoload + autoload :Arguments autoload :Base autoload :QueueAdapters autoload :Serializers diff --git a/activejob/lib/active_job/enqueuing.rb b/activejob/lib/active_job/enqueuing.rb index 53cb98fc71..66a7919aad 100644 --- a/activejob/lib/active_job/enqueuing.rb +++ b/activejob/lib/active_job/enqueuing.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require "active_job/arguments" - module ActiveJob # Provides behavior for enqueuing jobs. module Enqueuing diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb index 96b1e262f2..1e57dbcb1c 100644 --- a/activejob/lib/active_job/exceptions.rb +++ b/activejob/lib/active_job/exceptions.rb @@ -42,16 +42,16 @@ module ActiveJob # # Might raise Net::OpenTimeout when the remote service is down # end # end - def retry_on(exception, wait: 3.seconds, attempts: 5, queue: nil, priority: nil) - rescue_from exception do |error| + def retry_on(*exceptions, wait: 3.seconds, attempts: 5, queue: nil, priority: nil) + rescue_from(*exceptions) do |error| if executions < attempts - logger.error "Retrying #{self.class} in #{wait} seconds, due to a #{exception}. The original exception was #{error.cause.inspect}." + logger.error "Retrying #{self.class} in #{wait} seconds, due to a #{error.class}. The original exception was #{error.cause.inspect}." retry_job wait: determine_delay(wait), queue: queue, priority: priority else if block_given? yield self, error else - logger.error "Stopped retrying #{self.class} due to a #{exception}, which reoccurred on #{executions} attempts. The original exception was #{error.cause.inspect}." + logger.error "Stopped retrying #{self.class} due to a #{error.class}, which reoccurred on #{executions} attempts. The original exception was #{error.cause.inspect}." raise error end end @@ -76,12 +76,12 @@ module ActiveJob # # Might raise CustomAppException for something domain specific # end # end - def discard_on(exception) - rescue_from exception do |error| + def discard_on(*exceptions) + rescue_from(*exceptions) do |error| if block_given? yield self, error else - logger.error "Discarded #{self.class} due to a #{exception}. The original exception was #{error.cause.inspect}." + logger.error "Discarded #{self.class} due to a #{error.class}. The original exception was #{error.cause.inspect}." end end end diff --git a/activejob/lib/active_job/execution.rb b/activejob/lib/active_job/execution.rb index d75be376ec..8833e798d2 100644 --- a/activejob/lib/active_job/execution.rb +++ b/activejob/lib/active_job/execution.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require "active_support/rescuable" -require "active_job/arguments" module ActiveJob module Execution diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb index 15938e3fc7..47d4e3c0c2 100644 --- a/activejob/test/cases/exceptions_test.rb +++ b/activejob/test/cases/exceptions_test.rb @@ -113,4 +113,22 @@ class ExceptionsTest < ActiveJob::TestCase end end end + + test "successfully retry job throwing one of two retryable exceptions" do + perform_enqueued_jobs do + RetryJob.perform_later "SecondRetryableErrorOfTwo", 3 + + assert_equal [ + "Raised SecondRetryableErrorOfTwo for the 1st time", + "Raised SecondRetryableErrorOfTwo for the 2nd time", + "Successfully completed job" ], JobBuffer.values + end + end + + test "discard job throwing one of two discardable exceptions" do + perform_enqueued_jobs do + RetryJob.perform_later "SecondDiscardableErrorOfTwo", 2 + assert_equal [ "Raised SecondDiscardableErrorOfTwo for the 1st time" ], JobBuffer.values + end + end end diff --git a/activejob/test/jobs/retry_job.rb b/activejob/test/jobs/retry_job.rb index a12c65015b..1383fffd7d 100644 --- a/activejob/test/jobs/retry_job.rb +++ b/activejob/test/jobs/retry_job.rb @@ -4,22 +4,29 @@ require_relative "../support/job_buffer" require "active_support/core_ext/integer/inflections" class DefaultsError < StandardError; end +class FirstRetryableErrorOfTwo < StandardError; end +class SecondRetryableErrorOfTwo < StandardError; end class LongWaitError < StandardError; end class ShortWaitTenAttemptsError < StandardError; end class ExponentialWaitTenAttemptsError < StandardError; end class CustomWaitTenAttemptsError < StandardError; end class CustomCatchError < StandardError; end class DiscardableError < StandardError; end +class FirstDiscardableErrorOfTwo < StandardError; end +class SecondDiscardableErrorOfTwo < StandardError; end class CustomDiscardableError < StandardError; end class RetryJob < ActiveJob::Base retry_on DefaultsError + retry_on FirstRetryableErrorOfTwo, SecondRetryableErrorOfTwo retry_on LongWaitError, wait: 1.hour, attempts: 10 retry_on ShortWaitTenAttemptsError, wait: 1.second, attempts: 10 retry_on ExponentialWaitTenAttemptsError, wait: :exponentially_longer, attempts: 10 retry_on CustomWaitTenAttemptsError, wait: ->(executions) { executions * 2 }, attempts: 10 retry_on(CustomCatchError) { |job, error| JobBuffer.add("Dealt with a job that failed to retry in a custom way after #{job.arguments.second} attempts. Message: #{error.message}") } + discard_on DiscardableError + discard_on FirstDiscardableErrorOfTwo, SecondDiscardableErrorOfTwo discard_on(CustomDiscardableError) { |job, error| JobBuffer.add("Dealt with a job that was discarded in a custom way. Message: #{error.message}") } def perform(raising, attempts) diff --git a/activerecord/lib/active_record/aggregations.rb b/activerecord/lib/active_record/aggregations.rb index 27a641f05b..3250e29b82 100644 --- a/activerecord/lib/active_record/aggregations.rb +++ b/activerecord/lib/active_record/aggregations.rb @@ -3,8 +3,6 @@ module ActiveRecord # See ActiveRecord::Aggregations::ClassMethods for documentation module Aggregations - extend ActiveSupport::Concern - def initialize_dup(*) # :nodoc: @aggregation_cache = {} super @@ -225,6 +223,10 @@ module ActiveRecord def composed_of(part_id, options = {}) options.assert_valid_keys(:class_name, :mapping, :allow_nil, :constructor, :converter) + unless self < Aggregations + include Aggregations + end + name = part_id.id2name class_name = options[:class_name] || name.camelize mapping = options[:mapping] || [ name, name ] diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index 0835d2cc9b..8cf1b5d25a 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -172,7 +172,7 @@ module ActiveRecord end def table_alias_for(reflection, parent, join) - name = "#{reflection.plural_name}_#{parent.table_name}" + name = reflection.alias_candidate(parent.table_name) join ? "#{name}_join" : name end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 7ab9160265..5169f312f5 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -288,6 +288,7 @@ module ActiveRecord #:nodoc: extend Enum extend Delegation::DelegateCache extend CollectionCacheKey + extend Aggregations::ClassMethods include Core include DatabaseConfigurations @@ -314,7 +315,6 @@ module ActiveRecord #:nodoc: include ActiveModel::SecurePassword include AutosaveAssociation include NestedAttributes - include Aggregations include Transactions include TouchLater include NoTouching diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index e1a0b2ecf8..df795df52e 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -139,11 +139,6 @@ module ActiveRecord end module ClassMethods # :nodoc: - def allocate - define_attribute_methods - super - end - def initialize_find_by_cache # :nodoc: @find_by_statement_cache = { true => Concurrent::Map.new, false => Concurrent::Map.new } end @@ -350,6 +345,28 @@ module ActiveRecord end ## + # Initializer used for instantiating objects that have been read from the + # database. +attributes+ should be an attributes object, and unlike the + # `initialize` method, no assignment calls are made per attribute. + # + # :nodoc: + def init_from_db(attributes) + init_internals + + @new_record = false + @attributes = attributes + + self.class.define_attribute_methods + + yield self if block_given? + + _run_find_callbacks + _run_initialize_callbacks + + self + end + + ## # :method: clone # Identical to Ruby's clone method. This is a "shallow" copy. Be warned that your attributes are not copied. # That means that modifying attributes of the clone will modify the original, since they will both point to the diff --git a/activerecord/lib/active_record/no_touching.rb b/activerecord/lib/active_record/no_touching.rb index 754c891884..697076bdae 100644 --- a/activerecord/lib/active_record/no_touching.rb +++ b/activerecord/lib/active_record/no_touching.rb @@ -43,6 +43,13 @@ module ActiveRecord end end + # Returns +true+ if the class has +no_touching+ set, +false+ otherwise. + # + # Project.no_touching do + # Project.first.no_touching? # true + # Message.first.no_touching? # false + # end + # def no_touching? NoTouching.applied_to?(self.class) end diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index a0d5f1ee9f..155d67fd8f 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -67,8 +67,18 @@ module ActiveRecord # how this "single-table" inheritance mapping is implemented. def instantiate(attributes, column_types = {}, &block) klass = discriminate_class_for_record(attributes) + instantiate_instance_of(klass, attributes, column_types, &block) + end + + # Given a class, an attributes hash, +instantiate_instance_of+ returns a + # new instance of the class. Accepts only keys as strings. + # + # This is private, don't call it. :) + # + # :nodoc: + def instantiate_instance_of(klass, attributes, column_types = {}, &block) attributes = klass.attributes_builder.build_from_database(attributes, column_types) - klass.allocate.init_with("attributes" => attributes, "new_record" => false, &block) + klass.allocate.init_from_db(attributes, &block) end # Updates an object (or multiple objects) and saves it to the database, if validations pass. diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index d33d36ac02..c84f3d0fbb 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -49,7 +49,12 @@ module ActiveRecord } message_bus.instrument("instantiation.active_record", payload) do - result_set.map { |record| instantiate(record, column_types, &block) } + if result_set.includes_column?(inheritance_column) + result_set.map { |record| instantiate(record, column_types, &block) } + else + # Instantiate a homogeneous set + result_set.map { |record| instantiate_instance_of(self, record, column_types, &block) } + end end end diff --git a/activerecord/lib/active_record/result.rb b/activerecord/lib/active_record/result.rb index ffef229be2..7f1c2fd7eb 100644 --- a/activerecord/lib/active_record/result.rb +++ b/activerecord/lib/active_record/result.rb @@ -43,6 +43,11 @@ module ActiveRecord @column_types = column_types end + # Returns true if this result set includes the column named +name+ + def includes_column?(name) + @columns.include? name + end + # Returns the number of elements in the rows array. def length @rows.length diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 5d9735d98a..f414fbf64b 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -697,24 +697,13 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase end def test_join_table_alias - # FIXME: `references` has no impact on the aliases generated for the join - # query. The fact that we pass `:developers_projects_join` to `references` - # and that the SQL string contains `developers_projects_join` is merely a - # coincidence. assert_equal( 3, - Developer.references(:developers_projects_join).merge( - includes: { projects: :developers }, - where: "projects_developers_projects_join.joined_on IS NOT NULL" - ).to_a.size + Developer.includes(projects: :developers).where.not("projects_developers_projects_join.joined_on": nil).to_a.size ) end def test_join_with_group - # FIXME: `references` has no impact on the aliases generated for the join - # query. The fact that we pass `:developers_projects_join` to `references` - # and that the SQL string contains `developers_projects_join` is merely a - # coincidence. group = Developer.columns.inject([]) do |g, c| g << "developers.#{c.name}" g << "developers_projects_2.#{c.name}" @@ -723,10 +712,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal( 3, - Developer.references(:developers_projects_join).merge( - includes: { projects: :developers }, where: "projects_developers_projects_join.joined_on IS NOT NULL", - group: group.join(",") - ).to_a.size + Developer.includes(projects: :developers).where.not("projects_developers_projects_join.joined_on": nil).group(group.join(",")).to_a.size ) end diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index 434d32846c..0bfd46a522 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -163,19 +163,6 @@ class AttributeMethodsTest < ActiveRecord::TestCase assert_equal "10", keyboard.read_attribute_before_type_cast(:key_number) end - # Syck calls respond_to? before actually calling initialize. - test "respond_to? with an allocated object" do - klass = Class.new(ActiveRecord::Base) do - self.table_name = "topics" - end - - topic = klass.allocate - assert_not_respond_to topic, "nothingness" - assert_not_respond_to topic, :nothingness - assert_respond_to topic, "title" - assert_respond_to topic, :title - end - # IRB inspects the return value of MyModel.allocate. test "allocated objects can be inspected" do topic = Topic.allocate diff --git a/activerecord/test/cases/result_test.rb b/activerecord/test/cases/result_test.rb index db52c108ac..68fcafb682 100644 --- a/activerecord/test/cases/result_test.rb +++ b/activerecord/test/cases/result_test.rb @@ -12,6 +12,11 @@ module ActiveRecord ]) end + test "includes_column?" do + assert result.includes_column?("col_1") + assert_not result.includes_column?("foo") + end + test "length" do assert_equal 3, result.length end diff --git a/activestorage/test/service/azure_storage_service_test.rb b/activestorage/test/service/azure_storage_service_test.rb index be31bbe858..76920850d1 100644 --- a/activestorage/test/service/azure_storage_service_test.rb +++ b/activestorage/test/service/azure_storage_service_test.rb @@ -10,7 +10,7 @@ if SERVICE_CONFIGURATIONS[:azure] include ActiveStorage::Service::SharedServiceTests test "signed URL generation" do - url = @service.url(FIXTURE_KEY, expires_in: 5.minutes, + url = @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png") assert_match(/(\S+)&rscd=inline%3B\+filename%3D%22avatar\.png%22%3B\+filename\*%3DUTF-8%27%27avatar\.png&rsct=image%2Fpng/, url) diff --git a/activestorage/test/service/disk_service_test.rb b/activestorage/test/service/disk_service_test.rb index a4f2f4765f..a0218bff1c 100644 --- a/activestorage/test/service/disk_service_test.rb +++ b/activestorage/test/service/disk_service_test.rb @@ -9,10 +9,10 @@ class ActiveStorage::Service::DiskServiceTest < ActiveSupport::TestCase test "url generation" do assert_match(/^https:\/\/example.com\/rails\/active_storage\/disk\/.*\/avatar\.png\?content_type=image%2Fpng&disposition=inline/, - @service.url(FIXTURE_KEY, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png")) + @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png")) end test "headers_for_direct_upload generation" do - assert_equal({ "Content-Type" => "application/json" }, @service.headers_for_direct_upload(FIXTURE_KEY, content_type: "application/json")) + assert_equal({ "Content-Type" => "application/json" }, @service.headers_for_direct_upload(@key, content_type: "application/json")) end end diff --git a/activestorage/test/service/gcs_service_test.rb b/activestorage/test/service/gcs_service_test.rb index 2f212b5811..2ba2f8b346 100644 --- a/activestorage/test/service/gcs_service_test.rb +++ b/activestorage/test/service/gcs_service_test.rb @@ -33,7 +33,7 @@ if SERVICE_CONFIGURATIONS[:gcs] test "signed URL generation" do assert_match(/storage\.googleapis\.com\/.*response-content-disposition=inline.*test\.txt.*response-content-type=text%2Fplain/, - @service.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) + @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("test.txt"), content_type: "text/plain")) end test "signed URL response headers" do diff --git a/activestorage/test/service/mirror_service_test.rb b/activestorage/test/service/mirror_service_test.rb index 87306644c5..bb502dde60 100644 --- a/activestorage/test/service/mirror_service_test.rb +++ b/activestorage/test/service/mirror_service_test.rb @@ -47,11 +47,11 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase end test "deleting from all services" do - @service.delete FIXTURE_KEY + @service.delete @key - assert_not SERVICE.primary.exist?(FIXTURE_KEY) + assert_not SERVICE.primary.exist?(@key) SERVICE.mirrors.each do |mirror| - assert_not mirror.exist?(FIXTURE_KEY) + assert_not mirror.exist?(@key) end end @@ -59,8 +59,8 @@ class ActiveStorage::Service::MirrorServiceTest < ActiveSupport::TestCase filename = ActiveStorage::Filename.new("test.txt") freeze_time do - assert_equal @service.primary.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: filename, content_type: "text/plain"), - @service.url(FIXTURE_KEY, expires_in: 2.minutes, disposition: :inline, filename: filename, content_type: "text/plain") + assert_equal @service.primary.url(@key, expires_in: 2.minutes, disposition: :inline, filename: filename, content_type: "text/plain"), + @service.url(@key, expires_in: 2.minutes, disposition: :inline, filename: filename, content_type: "text/plain") end end end diff --git a/activestorage/test/service/s3_service_test.rb b/activestorage/test/service/s3_service_test.rb index 7833e51122..4bfcda017f 100644 --- a/activestorage/test/service/s3_service_test.rb +++ b/activestorage/test/service/s3_service_test.rb @@ -32,7 +32,7 @@ if SERVICE_CONFIGURATIONS[:s3] end test "signed URL generation" do - url = @service.url(FIXTURE_KEY, expires_in: 5.minutes, + url = @service.url(@key, expires_in: 5.minutes, disposition: :inline, filename: ActiveStorage::Filename.new("avatar.png"), content_type: "image/png") assert_match(/s3(-[-a-z0-9]+)?\.(\S+)?amazonaws.com.*response-content-disposition=inline.*avatar\.png.*response-content-type=image%2Fpng/, url) diff --git a/activestorage/test/service/shared_service_tests.rb b/activestorage/test/service/shared_service_tests.rb index b9f352e460..30cfca4e36 100644 --- a/activestorage/test/service/shared_service_tests.rb +++ b/activestorage/test/service/shared_service_tests.rb @@ -6,17 +6,17 @@ require "active_support/core_ext/securerandom" module ActiveStorage::Service::SharedServiceTests extend ActiveSupport::Concern - FIXTURE_KEY = SecureRandom.base58(24) FIXTURE_DATA = "\211PNG\r\n\032\n\000\000\000\rIHDR\000\000\000\020\000\000\000\020\001\003\000\000\000%=m\"\000\000\000\006PLTE\000\000\000\377\377\377\245\331\237\335\000\000\0003IDATx\234c\370\377\237\341\377_\206\377\237\031\016\2603\334?\314p\1772\303\315\315\f7\215\031\356\024\203\320\275\317\f\367\201R\314\f\017\300\350\377\177\000Q\206\027(\316]\233P\000\000\000\000IEND\256B`\202".dup.force_encoding(Encoding::BINARY) included do setup do + @key = SecureRandom.base58(24) @service = self.class.const_get(:SERVICE) - @service.upload FIXTURE_KEY, StringIO.new(FIXTURE_DATA) + @service.upload @key, StringIO.new(FIXTURE_DATA) end teardown do - @service.delete FIXTURE_KEY + @service.delete @key end test "uploading with integrity" do @@ -47,7 +47,7 @@ module ActiveStorage::Service::SharedServiceTests end test "downloading" do - assert_equal FIXTURE_DATA, @service.download(FIXTURE_KEY) + assert_equal FIXTURE_DATA, @service.download(@key) end test "downloading in chunks" do @@ -69,18 +69,18 @@ module ActiveStorage::Service::SharedServiceTests end test "downloading partially" do - assert_equal "\x10\x00\x00", @service.download_chunk(FIXTURE_KEY, 19..21) - assert_equal "\x10\x00\x00", @service.download_chunk(FIXTURE_KEY, 19...22) + assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19..21) + assert_equal "\x10\x00\x00", @service.download_chunk(@key, 19...22) end test "existing" do - assert @service.exist?(FIXTURE_KEY) - assert_not @service.exist?(FIXTURE_KEY + "nonsense") + assert @service.exist?(@key) + assert_not @service.exist?(@key + "nonsense") end test "deleting" do - @service.delete FIXTURE_KEY - assert_not @service.exist?(FIXTURE_KEY) + @service.delete @key + assert_not @service.exist?(@key) end test "deleting nonexistent key" do diff --git a/railties/Rakefile b/railties/Rakefile index 8251b2bb32..f918329df3 100644 --- a/railties/Rakefile +++ b/railties/Rakefile @@ -46,7 +46,7 @@ namespace :test do # We could run these in parallel, but pretty much all of the # railties tests already run in parallel, so ¯\_(⊙︿⊙)_/¯ Process.waitpid fork { - ARGV.clear + ARGV.clear.push ENV["TESTOPTS"] Rake.application = nil load file |