diff options
24 files changed, 121 insertions, 98 deletions
diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index 12f9e1cc17..585ade7229 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -1,5 +1,9 @@ ## Rails 5.1.0.alpha ## +* Yield the job instance so you have access to things like `job.arguments` on the custom logic after retries fail. + + *DHH* + * Added declarative exception handling via `ActiveJob::Base.retry_on` and `ActiveJob::Base.discard_on`. Examples: diff --git a/activejob/lib/active_job/exceptions.rb b/activejob/lib/active_job/exceptions.rb index a8ff55fbe4..f8d76587ad 100644 --- a/activejob/lib/active_job/exceptions.rb +++ b/activejob/lib/active_job/exceptions.rb @@ -12,7 +12,7 @@ module ActiveJob # holding queue for inspection. # # You can also pass a block that'll be invoked if the retry attempts fail for custom logic rather than letting - # the exception bubble up. + # the exception bubble up. This block is yielded with the job instance as the first and the error instance as the second parameter. # # ==== Options # * <tt>:wait</tt> - Re-enqueues the job with a delay specified either in seconds (default: 3 seconds), @@ -28,7 +28,7 @@ module ActiveJob # class RemoteServiceJob < ActiveJob::Base # retry_on CustomAppException # defaults to 3s wait, 5 attempts # retry_on AnotherCustomAppException, wait: ->(executions) { executions * 2 } - # retry_on(YetAnotherCustomAppException) do |exception| + # retry_on(YetAnotherCustomAppException) do |job, exception| # ExceptionNotifier.caught(exception) # end # retry_on ActiveRecord::StatementInvalid, wait: 5.seconds, attempts: 3 @@ -47,7 +47,7 @@ module ActiveJob retry_job wait: determine_delay(wait), queue: queue, priority: priority else if block_given? - yield exception + yield self, exception else logger.error "Stopped retrying #{self.class} due to a #{exception}, which reoccurred on #{executions} attempts. The original exception was #{error.cause.inspect}." raise error diff --git a/activejob/test/cases/exceptions_test.rb b/activejob/test/cases/exceptions_test.rb index 30e43c99cb..9ee1dbfa0a 100644 --- a/activejob/test/cases/exceptions_test.rb +++ b/activejob/test/cases/exceptions_test.rb @@ -58,8 +58,8 @@ class ExceptionsTest < ActiveJob::TestCase test "custom handling of job that exceeds retry attempts" do perform_enqueued_jobs do - RetryJob.perform_later "CustomCatchError", 6 - assert_equal "Dealt with a job that failed to retry in a custom way", JobBuffer.last_value + RetryJob.perform_later 'CustomCatchError', 6 + assert_equal "Dealt with a job that failed to retry in a custom way after 6 attempts", JobBuffer.last_value end end diff --git a/activejob/test/jobs/retry_job.rb b/activejob/test/jobs/retry_job.rb index be20156984..c02febc50c 100644 --- a/activejob/test/jobs/retry_job.rb +++ b/activejob/test/jobs/retry_job.rb @@ -15,7 +15,7 @@ class RetryJob < ActiveJob::Base 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) { |exception| JobBuffer.add("Dealt with a job that failed to retry in a custom way") } + retry_on(CustomCatchError) { |job, exception| JobBuffer.add("Dealt with a job that failed to retry in a custom way after #{job.arguments.second} attempts") } discard_on DiscardableError def perform(raising, attempts) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index bf81c23036..6b996fd6bd 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Doing count on relations that contain LEFT OUTER JOIN Arel node no longer + force a DISTINCT. This solves issues when using count after a left_joins. + + *Maxime Handfield Lapointe* + * RecordNotFound raised by association.find exposes `id`, `primary_key` and `model` methods to be consistent with RecordNotFound raised by Record.find. diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 01d7886406..1e7e939097 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -312,8 +312,8 @@ module ActiveRecord #:nodoc: include NestedAttributes include Aggregations include Transactions - include NoTouching include TouchLater + include NoTouching include Reflection include Serialization include Store diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 0c7197a002..4dde525ebc 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -433,16 +433,16 @@ module ActiveRecord @connection end - def case_sensitive_comparison(table, attribute, column, value) - table[attribute].eq(Arel::Nodes::BindParam.new) + def case_sensitive_comparison(attribute, column, value) # :nodoc: + attribute.eq(value) end - def case_insensitive_comparison(table, attribute, column, value) + def case_insensitive_comparison(attribute, column, value) # :nodoc: if can_perform_case_insensitive_comparison_for?(column) - table[attribute].lower.eq(table.lower(Arel::Nodes::BindParam.new)) - else - table[attribute].eq(Arel::Nodes::BindParam.new) + value = attribute.relation.lower(value) + attribute = attribute.lower end + attribute.eq(value) end def can_perform_case_insensitive_comparison_for?(column) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 5900919e88..e7efc4caf7 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -613,12 +613,11 @@ module ActiveRecord SQL end - def case_sensitive_comparison(table, attribute, column, value) + def case_sensitive_comparison(attribute, column, value) # :nodoc: if column.collation && !column.case_sensitive? - table[attribute].eq(Arel::Nodes::Bin.new(Arel::Nodes::BindParam.new)) - else - super + value = Arel::Nodes::Bin.new(value) end + attribute.eq(value) end def can_perform_case_insensitive_comparison_for?(column) diff --git a/activerecord/lib/active_record/no_touching.rb b/activerecord/lib/active_record/no_touching.rb index edb5066fa0..4059020e25 100644 --- a/activerecord/lib/active_record/no_touching.rb +++ b/activerecord/lib/active_record/no_touching.rb @@ -45,6 +45,10 @@ module ActiveRecord NoTouching.applied_to?(self.class) end + def touch_later(*) # :nodoc: + super unless no_touching? + end + def touch(*) # :nodoc: super unless no_touching? end diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index ecf3700aab..a4962879ab 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -117,7 +117,10 @@ module ActiveRecord end if has_include?(column_name) - construct_relation_for_association_calculations.calculate(operation, column_name) + relation = construct_relation_for_association_calculations + relation = relation.distinct if operation.to_s.downcase == "count" + + relation.calculate(operation, column_name) else perform_calculation(operation, column_name) end @@ -198,11 +201,6 @@ module ActiveRecord if operation == "count" column_name ||= select_for_count - - unless arel.ast.grep(Arel::Nodes::OuterJoin).empty? - distinct = true - end - column_name = primary_key if column_name == :all && distinct distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i end diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 0c8282de8e..29422bf131 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -4,6 +4,7 @@ module ActiveRecord require "active_record/relation/predicate_builder/association_query_handler" require "active_record/relation/predicate_builder/base_handler" require "active_record/relation/predicate_builder/basic_object_handler" + require "active_record/relation/predicate_builder/case_sensitive_handler" require "active_record/relation/predicate_builder/class_handler" require "active_record/relation/predicate_builder/polymorphic_array_handler" require "active_record/relation/predicate_builder/range_handler" @@ -16,6 +17,7 @@ module ActiveRecord @handlers = [] register_handler(BasicObject, BasicObjectHandler.new) + register_handler(CaseSensitiveHandler::Value, CaseSensitiveHandler.new) register_handler(Class, ClassHandler.new(self)) register_handler(Base, BaseHandler.new(self)) register_handler(Range, RangeHandler.new) @@ -31,9 +33,9 @@ module ActiveRecord expand_from_hash(attributes) end - def create_binds(attributes) + def create_binds(attributes, options) attributes = convert_dot_notation_to_hash(attributes) - create_binds_for_hash(attributes) + create_binds_for_hash(attributes, options) end def self.references(attributes) @@ -74,7 +76,7 @@ module ActiveRecord return ["1=0"] if attributes.empty? attributes.flat_map do |key, value| - if value.is_a?(Hash) + if value.is_a?(Hash) && !table.has_column?(key) associated_predicate_builder(key).expand_from_hash(value) else build(table.arel_attribute(key), value) @@ -82,14 +84,14 @@ module ActiveRecord end end - def create_binds_for_hash(attributes) + def create_binds_for_hash(attributes, options) result = attributes.dup binds = [] attributes.each do |column_name, value| case - when value.is_a?(Hash) - attrs, bvs = associated_predicate_builder(column_name).create_binds_for_hash(value) + when value.is_a?(Hash) && !table.has_column?(column_name) + attrs, bvs = associated_predicate_builder(column_name).create_binds_for_hash(value, options) result[column_name] = attrs binds += bvs next @@ -108,11 +110,15 @@ module ActiveRecord end result[column_name] = RangeHandler::RangeWithBinds.new(first, last, value.exclude_end?) - else - if can_be_bound?(column_name, value) - result[column_name] = Arel::Nodes::BindParam.new - binds << build_bind_param(column_name, value) - end + when can_be_bound?(column_name, value) + result[column_name] = + if perform_case_sensitive?(options) + CaseSensitiveHandler::Value.new( + Arel::Nodes::BindParam.new, table, options[:case_sensitive]) + else + Arel::Nodes::BindParam.new + end + binds << build_bind_param(column_name, value) end # Find the foreign key when using queries such as: @@ -164,6 +170,10 @@ module ActiveRecord end end + def perform_case_sensitive?(options) + options.key?(:case_sensitive) + end + def build_bind_param(column_name, value) Relation::QueryAttribute.new(column_name.to_s, value, table.type(column_name)) end diff --git a/activerecord/lib/active_record/relation/predicate_builder/case_sensitive_handler.rb b/activerecord/lib/active_record/relation/predicate_builder/case_sensitive_handler.rb new file mode 100644 index 0000000000..acf0bbd829 --- /dev/null +++ b/activerecord/lib/active_record/relation/predicate_builder/case_sensitive_handler.rb @@ -0,0 +1,21 @@ +module ActiveRecord + class PredicateBuilder + class CaseSensitiveHandler # :nodoc: + def call(attribute, value) + value.call(attribute) + end + + class Value < Struct.new(:value, :table, :case_sensitive?) # :nodoc: + def call(attribute) + klass = table.send(:klass) + column = klass.column_for_attribute(attribute.name) + if case_sensitive? + klass.connection.case_sensitive_comparison(attribute, column, value) + else + klass.connection.case_insensitive_comparison(attribute, column, value) + end + end + end + end + end +end diff --git a/activerecord/lib/active_record/relation/where_clause_factory.rb b/activerecord/lib/active_record/relation/where_clause_factory.rb index dc00149130..122ab04c00 100644 --- a/activerecord/lib/active_record/relation/where_clause_factory.rb +++ b/activerecord/lib/active_record/relation/where_clause_factory.rb @@ -17,7 +17,7 @@ module ActiveRecord attributes = klass.send(:expand_hash_conditions_for_aggregates, attributes) attributes.stringify_keys! - attributes, binds = predicate_builder.create_binds(attributes) + attributes, binds = predicate_builder.create_binds(attributes, other.last || {}) parts = predicate_builder.build_from_hash(attributes) when Arel::Nodes::Node diff --git a/activerecord/lib/active_record/sanitization.rb b/activerecord/lib/active_record/sanitization.rb index 6617008344..c40e98715e 100644 --- a/activerecord/lib/active_record/sanitization.rb +++ b/activerecord/lib/active_record/sanitization.rb @@ -5,13 +5,6 @@ module ActiveRecord extend ActiveSupport::Concern module ClassMethods - # Used to sanitize objects before they're used in an SQL SELECT statement. - # Delegates to {connection.quote}[rdoc-ref:ConnectionAdapters::Quoting#quote]. - def sanitize(object) # :nodoc: - connection.quote(object) - end - alias_method :quote_value, :sanitize - protected # Accepts an array or string of SQL conditions and sanitizes @@ -216,7 +209,7 @@ module ActiveRecord # TODO: Deprecate this def quoted_id # :nodoc: - self.class.quote_value(@attributes[self.class.primary_key].value_for_database) + self.class.connection.quote(@attributes[self.class.primary_key].value_for_database) end end end diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb index 46ee3540bd..094c0e9c6f 100644 --- a/activerecord/lib/active_record/scoping/named.rb +++ b/activerecord/lib/active_record/scoping/named.rb @@ -174,7 +174,7 @@ module ActiveRecord protected def valid_scope_name?(name) - if respond_to?(name, true) + if respond_to?(name, true) && logger logger.warn "Creating scope :#{name}. " \ "Overwriting existing method #{self.name}.#{name}." end diff --git a/activerecord/lib/active_record/table_metadata.rb b/activerecord/lib/active_record/table_metadata.rb index e8d6a144f9..0ca880e635 100644 --- a/activerecord/lib/active_record/table_metadata.rb +++ b/activerecord/lib/active_record/table_metadata.rb @@ -37,6 +37,10 @@ module ActiveRecord end end + def has_column?(column_name) + klass && klass.columns_hash.key?(column_name.to_s) + end + def associated_with?(association_name) klass && klass._reflect_on_association(association_name) end diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index 8c4930a81d..08c4b01439 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -50,37 +50,7 @@ module ActiveRecord end def build_relation(klass, attribute, value) # :nodoc: - if reflection = klass._reflect_on_association(attribute) - attribute = reflection.foreign_key - value = value.attributes[reflection.klass.primary_key] unless value.nil? - end - - if value.nil? - return klass.unscoped.where!(attribute => value) - end - - # the attribute may be an aliased attribute - if klass.attribute_alias?(attribute) - attribute = klass.attribute_alias(attribute) - end - - attribute_name = attribute.to_s - - table = klass.arel_table - column = klass.columns_hash[attribute_name] - cast_type = klass.type_for_attribute(attribute_name) - - comparison = if !options[:case_sensitive] - # will use SQL LOWER function before comparison, unless it detects a case insensitive collation - klass.connection.case_insensitive_comparison(table, attribute, column, value) - else - klass.connection.case_sensitive_comparison(table, attribute, column, value) - end - klass.unscoped.tap do |scope| - parts = [comparison] - binds = [Relation::QueryAttribute.new(attribute_name, value, cast_type)] - scope.where_clause += Relation::WhereClause.new(parts, binds) - end + klass.unscoped.where!({ attribute => value }, options) end def scope_relation(record, relation) diff --git a/activerecord/test/cases/associations/left_outer_join_association_test.rb b/activerecord/test/cases/associations/left_outer_join_association_test.rb index d814229f64..2cc6468827 100644 --- a/activerecord/test/cases/associations/left_outer_join_association_test.rb +++ b/activerecord/test/cases/associations/left_outer_join_association_test.rb @@ -25,23 +25,32 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase end end - def test_construct_finder_sql_executes_a_left_outer_join - assert_not_equal Author.count, Author.joins(:posts).count - assert_equal Author.count, Author.left_outer_joins(:posts).count + def test_left_outer_joins_count_is_same_as_size_of_loaded_results + assert_equal 17, Post.left_outer_joins(:comments).to_a.size + assert_equal 17, Post.left_outer_joins(:comments).count end - def test_left_outer_join_by_left_joins - assert_not_equal Author.count, Author.joins(:posts).count - assert_equal Author.count, Author.left_joins(:posts).count + def test_left_joins_aliases_left_outer_joins + assert_equal Post.left_outer_joins(:comments).to_sql, Post.left_joins(:comments).to_sql + end + + def test_left_outer_joins_return_has_value_for_every_comment + all_post_ids = Post.pluck(:id) + assert_equal all_post_ids, all_post_ids & Post.left_outer_joins(:comments).pluck(:id) + end + + def test_left_outer_joins_actually_does_a_left_outer_join + queries = capture_sql { Author.left_outer_joins(:posts).to_a } + assert queries.any? { |sql| /LEFT OUTER JOIN/i.match?(sql) } end def test_construct_finder_sql_ignores_empty_left_outer_joins_hash - queries = capture_sql { Author.left_outer_joins({}) } + queries = capture_sql { Author.left_outer_joins({}).to_a } assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) } end def test_construct_finder_sql_ignores_empty_left_outer_joins_array - queries = capture_sql { Author.left_outer_joins([]) } + queries = capture_sql { Author.left_outer_joins([]).to_a } assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) } end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index f8f9f2d383..31bc4fa1f2 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -22,13 +22,13 @@ class FinderTest < ActiveRecord::TestCase fixtures :companies, :topics, :entrants, :developers, :developers_projects, :posts, :comments, :accounts, :authors, :author_addresses, :customers, :categories, :categorizations, :cars def test_find_by_id_with_hash - assert_raises(ActiveRecord::StatementInvalid) do + assert_nothing_raised do Post.find_by_id(limit: 1) end end def test_find_by_title_and_id_with_hash - assert_raises(ActiveRecord::StatementInvalid) do + assert_nothing_raised do Post.find_by_title_and_id("foo", limit: 1) end end @@ -874,11 +874,6 @@ class FinderTest < ActiveRecord::TestCase assert_kind_of Time, Topic.where(["id = :id", { id: 1 }]).first.written_on end - def test_string_sanitation - assert_not_equal "'something ' 1=1'", ActiveRecord::Base.sanitize("something ' 1=1") - assert_equal "'something; select table'", ActiveRecord::Base.sanitize("something; select table") - end - def test_count_by_sql assert_equal(0, Entrant.count_by_sql("SELECT COUNT(*) FROM entrants WHERE id > 3")) assert_equal(1, Entrant.count_by_sql(["SELECT COUNT(*) FROM entrants WHERE id > ?", 2])) diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb index ad9008ea4d..ce4e041793 100644 --- a/activerecord/test/cases/relation/where_test.rb +++ b/activerecord/test/cases/relation/where_test.rb @@ -196,7 +196,7 @@ module ActiveRecord end def test_where_error - assert_raises(ActiveRecord::StatementInvalid) do + assert_nothing_raised do Post.where(id: { "posts.author_id" => 10 }).first end end diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index 951b83e87b..103075d09c 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -223,7 +223,7 @@ module ActiveRecord def test_relation_merging_with_merged_joins_as_symbols special_comments_with_ratings = SpecialComment.joins(:ratings) posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings) - assert_equal 3, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count.length + assert_equal({ 2=>1, 4=>3, 5=>1 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count) end def test_relation_merging_with_joins_as_join_dependency_pick_proper_parent @@ -273,7 +273,7 @@ module ActiveRecord join_string = "LEFT OUTER JOIN #{Rating.quoted_table_name} ON #{SpecialComment.quoted_table_name}.id = #{Rating.quoted_table_name}.comment_id" special_comments_with_ratings = SpecialComment.joins join_string posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings) - assert_equal 3, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count.length + assert_equal({ 2=>1, 4=>3, 5=>1 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count) end class EnsureRoundTripTypeCasting < ActiveRecord::Type::Value diff --git a/activerecord/test/cases/serialized_attribute_test.rb b/activerecord/test/cases/serialized_attribute_test.rb index ceb0d5441b..bebd856faf 100644 --- a/activerecord/test/cases/serialized_attribute_test.rb +++ b/activerecord/test/cases/serialized_attribute_test.rb @@ -157,6 +157,13 @@ class SerializedAttributeTest < ActiveRecord::TestCase assert_equal(settings, Topic.find(topic.id).content) end + def test_where_by_serialized_attribute_with_hash + settings = { "color" => "green" } + Topic.serialize(:content, Hash) + topic = Topic.create!(content: settings) + assert_equal topic, Topic.where(content: settings).take + end + def test_serialized_default_class Topic.serialize(:content, Hash) topic = Topic.new diff --git a/activerecord/test/cases/touch_later_test.rb b/activerecord/test/cases/touch_later_test.rb index 697447a4f3..d1e8c649d9 100644 --- a/activerecord/test/cases/touch_later_test.rb +++ b/activerecord/test/cases/touch_later_test.rb @@ -24,6 +24,15 @@ class TouchLaterTest < ActiveRecord::TestCase assert_not invoice.changed? end + def test_touch_later_respects_no_touching_policy + time = Time.now.utc - 25.days + topic = Topic.create!(updated_at: time, created_at: time) + Topic.no_touching do + topic.touch_later + end + assert_equal time.to_i, topic.updated_at.to_i + end + def test_touch_later_update_the_attributes time = Time.now.utc - 25.days topic = Topic.create!(updated_at: time, created_at: time) diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb index 4e47465726..a669d666be 100644 --- a/activesupport/test/caching_test.rb +++ b/activesupport/test/caching_test.rb @@ -831,11 +831,6 @@ class FileStoreTest < ActiveSupport::TestCase @cache.clear end - def test_long_keys - @cache.write("a"*10000, 1) - assert_equal 1, @cache.read("a"*10000) - end - def test_long_uri_encoded_keys @cache.write("%"*870, 1) assert_equal 1, @cache.read("%"*870) |