diff options
Diffstat (limited to 'activerecord')
25 files changed, 186 insertions, 34 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 6bdb53ac5a..b12d048169 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* Fix regression causing `after_create` callbacks to run before associated + records are autosaved. + + Fixes #17209. + + *Agis Anastasopoulos* + * Honor overridden `rack.test` in Rack environment for the connection management middleware. diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 12ca3a48a9..8911506694 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -46,6 +46,12 @@ module ActiveRecord end end + class HasOneAssociationPolymorphicThroughError < ActiveRecordError #:nodoc: + def initialize(owner_class_name, reflection) + super("Cannot have a has_one :through association '#{owner_class_name}##{reflection.name}' which goes through the polymorphic association '#{owner_class_name}##{reflection.through_reflection.name}'.") + end + end + class HasManyThroughSourceAssociationNotFoundError < ActiveRecordError #:nodoc: def initialize(reflection) through_reflection = reflection.through_reflection diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 1836ff0910..bdfd569be2 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -407,7 +407,12 @@ module ActiveRecord private def get_records - return scope.to_a if reflection.scope_chain.any?(&:any?) || scope.eager_loading? + if reflection.scope_chain.any?(&:any?) || + scope.eager_loading? || + klass.current_scope + + return scope.to_a + end conn = klass.connection sc = reflection.association_scope_cache(conn, owner) do diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index ec5c189cd3..c5c4edd090 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -142,11 +142,20 @@ module ActiveRecord parents = model_cache[join_root] column_aliases = aliases.column_aliases join_root - result_set.each { |row_hash| - parent = parents[row_hash[primary_key]] ||= join_root.instantiate(row_hash, column_aliases) - construct(parent, join_root, row_hash, result_set, seen, model_cache, aliases) + message_bus = ActiveSupport::Notifications.instrumenter + + payload = { + record_count: result_set.length, + class_name: join_root.base_klass.name } + message_bus.instrument('instantiation.active_record', payload) do + result_set.each { |row_hash| + parent = parents[row_hash[primary_key]] ||= join_root.instantiate(row_hash, column_aliases) + construct(parent, join_root, row_hash, result_set, seen, model_cache, aliases) + } + end + parents.values end diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index b9326b9683..c360ef1b2c 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -39,7 +39,12 @@ module ActiveRecord end def get_records - return scope.limit(1).to_a if reflection.scope_chain.any?(&:any?) || scope.eager_loading? + if reflection.scope_chain.any?(&:any?) || + scope.eager_loading? || + klass.current_scope + + return scope.limit(1).to_a + end conn = klass.connection sc = reflection.association_scope_cache(conn, owner) do diff --git a/activerecord/lib/active_record/attribute_methods/serialization.rb b/activerecord/lib/active_record/attribute_methods/serialization.rb index 100d6d4229..04a1df37c6 100644 --- a/activerecord/lib/active_record/attribute_methods/serialization.rb +++ b/activerecord/lib/active_record/attribute_methods/serialization.rb @@ -11,9 +11,6 @@ module ActiveRecord # serialized object must be of that class on retrieval or # <tt>SerializationTypeMismatch</tt> will be raised. # - # A notable side effect of serialized attributes is that the model will - # be updated on every save, even if it is not dirty. - # # ==== Parameters # # * +attr_name+ - The field name that should be serialized. diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index c384e8c413..a0d70435fa 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -184,7 +184,9 @@ module ActiveRecord before_save :before_save_collection_association define_non_cyclic_method(save_method) { save_collection_association(reflection) } - after_save save_method + # Doesn't use after_save as that would save associations added in after_create/after_update twice + after_create save_method + after_update save_method elsif reflection.has_one? define_method(save_method) { save_has_one_association(reflection) } unless method_defined?(save_method) # Configures two callbacks instead of a single after_save so that diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index fe00f9d750..adb9fcaeb8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -256,7 +256,7 @@ module ActiveRecord name = name.to_s type = type.to_sym - if primary_key_column_name == name + if @columns_hash[name] && @columns_hash[name].primary_key? raise ArgumentError, "you can't redefine the primary key column '#{name}'. To define a custom primary key, pass { id: false } to create_table." end @@ -270,7 +270,7 @@ module ActiveRecord @columns_hash.delete name.to_s end - [:string, :text, :integer, :float, :decimal, :datetime, :timestamp, :time, :date, :binary, :boolean].each do |column_type| + [:string, :text, :integer, :bigint, :float, :decimal, :datetime, :timestamp, :time, :date, :binary, :boolean].each do |column_type| define_method column_type do |*args| options = args.extract_options! column_names = args @@ -318,7 +318,7 @@ module ActiveRecord alias :belongs_to :references def new_column_definition(name, type, options) # :nodoc: - type = aliased_types[type] || type + type = aliased_types(type.to_s, type) column = create_column_definition name, type limit = options.fetch(:limit) do native[type][:limit] if native[type].is_a?(Hash) @@ -340,19 +340,12 @@ module ActiveRecord ColumnDefinition.new name, type end - def primary_key_column_name - primary_key_column = columns.detect { |c| c.primary_key? } - primary_key_column && primary_key_column.name - end - def native @native end - def aliased_types - HashWithIndifferentAccess.new( - timestamp: :datetime, - ) + def aliased_types(name, fallback) + 'timestamp' == name ? :datetime : fallback end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 0d74cb6707..fe7648291d 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -94,6 +94,7 @@ module ActiveRecord int8range: { name: "int8range" }, binary: { name: "bytea" }, boolean: { name: "boolean" }, + bigint: { name: "bigint" }, xml: { name: "xml" }, tsvector: { name: "tsvector" }, hstore: { name: "hstore" }, diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 45b6b1c925..e8de4db3a7 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -39,7 +39,16 @@ module ActiveRecord result_set = connection.select_all(sanitize_sql(sql), "#{name} Load", binds) column_types = result_set.column_types.dup columns_hash.each_key { |k| column_types.delete k } - result_set.map { |record| instantiate(record, column_types) } + message_bus = ActiveSupport::Notifications.instrumenter + + payload = { + record_count: result_set.length, + class_name: name + } + + message_bus.instrument('instantiation.active_record', payload) do + result_set.map { |record| instantiate(record, column_types) } + end end # Returns the result of an SQL statement that should only include a COUNT(*) in the SELECT part. diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 6b5a592ee5..22aa175ce2 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -821,7 +821,11 @@ module ActiveRecord end if through_reflection.polymorphic? - raise HasManyThroughAssociationPolymorphicThroughError.new(active_record.name, self) + if has_one? + raise HasOneAssociationPolymorphicThroughError.new(active_record.name, self) + else + raise HasManyThroughAssociationPolymorphicThroughError.new(active_record.name, self) + end end if source_reflection.nil? diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index ad54d84665..7f51e4134d 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -4,8 +4,6 @@ require 'arel/collectors/bind' module ActiveRecord # = Active Record Relation class Relation - JoinOperation = Struct.new(:relation, :join_class, :on) - MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group, :order, :joins, :where, :having, :bind, :references, :extending, :unscope] @@ -305,7 +303,8 @@ module ActiveRecord # Updates all records with details given if they match a set of conditions supplied, limits and order can # also be supplied. This method constructs a single SQL UPDATE statement and sends it straight to the # database. It does not instantiate the involved models and it does not trigger Active Record callbacks - # or validations. + # or validations. Values passed to `update_all` will not go through ActiveRecord's type-casting behavior. + # It should receive only values that can be passed as-is to the SQL database. # # ==== Parameters # diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index ed56369f86..c95ec2522b 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -138,7 +138,7 @@ module ActiveRecord # Same as +first+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. Note that <tt>first!</tt> accepts no arguments. def first! - first or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 0 end # Find the last record (or last N records if a parameter is supplied). @@ -187,7 +187,7 @@ module ActiveRecord # Same as +second+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. def second! - second or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 1 end # Find the third record. @@ -203,7 +203,7 @@ module ActiveRecord # Same as +third+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. def third! - third or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 2 end # Find the fourth record. @@ -219,7 +219,7 @@ module ActiveRecord # Same as +fourth+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. def fourth! - fourth or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 3 end # Find the fifth record. @@ -235,7 +235,7 @@ module ActiveRecord # Same as +fifth+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. def fifth! - fifth or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 4 end # Find the forty-second record. Also known as accessing "the reddit". @@ -251,7 +251,7 @@ module ActiveRecord # Same as +forty_two+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record # is found. def forty_two! - forty_two or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + find_nth! 41 end # Returns +true+ if a record exists in the table that matches the +id+ or @@ -489,6 +489,10 @@ module ActiveRecord end end + def find_nth!(index) + find_nth(index, offset_index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql}]") + end + def find_nth_with_limit(offset, limit) relation = if order_values.empty? && primary_key order(arel_table[primary_key].asc) diff --git a/activerecord/lib/active_record/result.rb b/activerecord/lib/active_record/result.rb index 8405fdaeb9..3a3e65ef32 100644 --- a/activerecord/lib/active_record/result.rb +++ b/activerecord/lib/active_record/result.rb @@ -42,6 +42,10 @@ module ActiveRecord @column_types = column_types end + def length + @rows.length + end + def each if block_given? hash_rows.each { |row| yield row } diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index e53297d0ab..bb06d0304b 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -11,7 +11,7 @@ module ActiveRecord "\n" \ "You can opt into the new behavior and remove this warning by setting:\n" \ "\n" \ - " config.active_record.raise_in_transactional_callbacks = true" + " config.active_record.raise_in_transactional_callbacks = true\n\n" included do define_callbacks :commit, :rollback, diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index b852bd3536..8234ee95be 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -935,6 +935,42 @@ class EagerAssociationTest < ActiveRecord::TestCase assert_equal 3, authors(:david).posts_with_comments.where("length(comments.body) > 15").references(:comments).count end + def test_association_loading_notification + notifications = messages_for('instantiation.active_record') do + Developer.all.merge!(:includes => 'projects', :where => { 'developers_projects.access_level' => 1 }, :limit => 5).to_a.size + end + + message = notifications.first + payload = message.last + count = Developer.all.merge!(:includes => 'projects', :where => { 'developers_projects.access_level' => 1 }, :limit => 5).to_a.size + + # eagerloaded row count should be greater than just developer count + assert_operator payload[:record_count], :>, count + assert_equal Developer.name, payload[:class_name] + end + + def test_base_messages + notifications = messages_for('instantiation.active_record') do + Developer.all.to_a + end + message = notifications.first + payload = message.last + + assert_equal Developer.all.to_a.count, payload[:record_count] + assert_equal Developer.name, payload[:class_name] + end + + def messages_for(name) + notifications = [] + ActiveSupport::Notifications.subscribe(name) do |*args| + notifications << args + end + yield + notifications + ensure + ActiveSupport::Notifications.unsubscribe(name) + end + def test_load_with_sti_sharing_association assert_queries(2) do #should not do 1 query per subclass Comment.includes(:post).to_a diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 089cb0a3a2..19d1aa87a8 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -289,6 +289,12 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase end end + def test_has_one_through_polymorphic_association + assert_raise(ActiveRecord::HasOneAssociationPolymorphicThroughError) do + @member.premium_club + end + end + def test_has_one_through_belongs_to_should_update_when_the_through_foreign_key_changes minivan = minivans(:cool_first) diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index b2a7d3956d..734fd5fe18 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -1,5 +1,6 @@ require 'cases/helper' require 'models/bird' +require 'models/comment' require 'models/company' require 'models/customer' require 'models/developer' @@ -616,6 +617,14 @@ class TestDefaultAutosaveAssociationOnNewRecord < ActiveRecord::TestCase firm.save! assert !account.persisted? end + + def test_autosave_new_record_with_after_create_callback + post = PostWithAfterCreateCallback.new(title: 'Captain Murphy', body: 'is back') + post.comments.build(body: 'foo') + post.save! + + assert_not_nil post.author_id + end end class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase diff --git a/activerecord/test/cases/migration/change_schema_test.rb b/activerecord/test/cases/migration/change_schema_test.rb index bd3dd29f4d..d91e7142b3 100644 --- a/activerecord/test/cases/migration/change_schema_test.rb +++ b/activerecord/test/cases/migration/change_schema_test.rb @@ -97,6 +97,25 @@ module ActiveRecord end end + def test_create_table_with_bigint + connection.create_table :testings do |t| + t.bigint :eight_int + end + columns = connection.columns(:testings) + eight = columns.detect { |c| c.name == "eight_int" } + + if current_adapter?(:OracleAdapter) + assert_equal 'NUMBER(8)', eight.sql_type + elsif current_adapter?(:SQLite3Adapter) + assert_equal 'bigint', eight.sql_type + else + assert_equal :integer, eight.type + assert_equal 8, eight.limit + end + ensure + connection.drop_table :testings + end + def test_create_table_with_limits connection.create_table :testings do |t| t.column :foo, :string, :limit => 255 diff --git a/activerecord/test/cases/result_test.rb b/activerecord/test/cases/result_test.rb index d6decafad9..dec01dfa76 100644 --- a/activerecord/test/cases/result_test.rb +++ b/activerecord/test/cases/result_test.rb @@ -10,6 +10,10 @@ module ActiveRecord ]) end + test "length" do + assert_equal 3, result.length + end + test "to_hash returns row_hashes" do assert_equal [ {'col_1' => 'row 1 col 1', 'col_2' => 'row 1 col 2'}, diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb index 93fb502410..2241c41f36 100644 --- a/activerecord/test/cases/schema_dumper_test.rb +++ b/activerecord/test/cases/schema_dumper_test.rb @@ -19,7 +19,7 @@ class SchemaDumperTest < ActiveRecord::TestCase def test_dump_schema_information_outputs_lexically_ordered_versions versions = %w{ 20100101010101 20100201010101 20100301010101 } - versions.reverse.each do |v| + versions.reverse_each do |v| ActiveRecord::SchemaMigration.create!(:version => v) end diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb index 8e512e118a..73835c85a8 100644 --- a/activerecord/test/cases/scoping/relation_scoping_test.rb +++ b/activerecord/test/cases/scoping/relation_scoping_test.rb @@ -15,6 +15,26 @@ class RelationScopingTest < ActiveRecord::TestCase developers(:david) end + def test_unscoped_breaks_caching + author = authors :mary + assert_nil author.first_post + post = FirstPost.unscoped do + author.reload.first_post + end + assert post + end + + def test_scope_breaks_caching_on_collections + author = authors :david + ids = author.reload.special_posts_with_default_scope.map(&:id) + assert_equal [1,5,6], ids.sort + scoped_posts = SpecialPostWithDefaultScope.unscoped do + author = authors :david + author.reload.special_posts_with_default_scope.to_a + end + assert_equal author.posts.map(&:id).sort, scoped_posts.map(&:id).sort + end + def test_reverse_order assert_equal Developer.order("id DESC").to_a.reverse, Developer.order("id DESC").reverse_order end diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 8949cf5826..3f34d09a04 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -44,6 +44,7 @@ class Author < ActiveRecord::Base has_many :special_posts has_many :special_post_comments, :through => :special_posts, :source => :comments + has_many :special_posts_with_default_scope, :class_name => 'SpecialPostWithDefaultScope' has_many :sti_posts, :class_name => 'StiPost' has_many :sti_post_comments, :through => :sti_posts, :source => :comments diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index 72095f9236..dc0566d8a7 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -27,6 +27,9 @@ class Member < ActiveRecord::Base has_many :clubs, :through => :current_memberships has_one :club_through_many, :through => :current_memberships, :source => :club + + belongs_to :admittable, polymorphic: true + has_one :premium_club, through: :admittable end class SelfMember < ActiveRecord::Base diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 256b720c9a..67027cbc22 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -219,6 +219,15 @@ class PostThatLoadsCommentsInAnAfterSaveHook < ActiveRecord::Base end end +class PostWithAfterCreateCallback < ActiveRecord::Base + self.table_name = 'posts' + has_many :comments, foreign_key: :post_id + + after_create do |post| + update_attribute(:author_id, comments.first.id) + end +end + class PostWithCommentWithDefaultScopeReferencesAssociation < ActiveRecord::Base self.table_name = 'posts' has_many :comment_with_default_scope_references_associations, foreign_key: :post_id |