diff options
Diffstat (limited to 'activerecord')
16 files changed, 133 insertions, 75 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8c5342edb2..6f08b1b8fe 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,13 @@ +* Fix invalid schema when primary key column has a comment + + Fixes #29966 + + *Guilherme Goettems Schneider* + +* Fix table comment also being applied to the primary key column + + *Guilherme Goettems Schneider* + * Allow generated `create_table` migrations to include or skip timestamps. *Michael Duchemin* diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 220043c061..fd32eaaf3a 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -185,12 +185,14 @@ module ActiveRecord /ix def disallow_raw_sql!(args, permit: COLUMN_NAME) # :nodoc: - unexpected = args.reject do |arg| - Arel.arel_node?(arg) || + unexpected = nil + args.each do |arg| + next if arg.is_a?(Symbol) || Arel.arel_node?(arg) || arg.to_s.split(/\s*,\s*/).all? { |part| permit.match?(part) } + (unexpected ||= []) << arg end - return if unexpected.none? + return unless unexpected if allow_unsafe_raw_sql == :deprecated ActiveSupport::Deprecation.warn( @@ -437,7 +439,7 @@ module ActiveRecord def attributes_for_update(attribute_names) attribute_names &= self.class.column_names attribute_names.delete_if do |name| - readonly_attribute?(name) + self.class.readonly_attribute?(name) end end @@ -460,10 +462,6 @@ module ActiveRecord end end - def readonly_attribute?(name) - self.class.readonly_attributes.include?(name) - end - def pk_attribute?(name) name == @primary_key end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 4ff3cb0071..7628ef5537 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -3,6 +3,7 @@ require "thread" require "concurrent/map" require "monitor" +require "weakref" module ActiveRecord # Raised when a connection could not be obtained within the connection @@ -294,28 +295,37 @@ module ActiveRecord @frequency = frequency end - @@mutex = Mutex.new - @@pools = {} + @mutex = Mutex.new + @pools = {} - def self.register_pool(pool, frequency) # :nodoc: - @@mutex.synchronize do - if @@pools.key?(frequency) - @@pools[frequency] << pool - else - @@pools[frequency] = [pool] + class << self + def register_pool(pool, frequency) # :nodoc: + @mutex.synchronize do + unless @pools.key?(frequency) + @pools[frequency] = [] + spawn_thread(frequency) + end + @pools[frequency] << WeakRef.new(pool) + end + end + + private + + def spawn_thread(frequency) Thread.new(frequency) do |t| loop do sleep t - @@mutex.synchronize do - @@pools[frequency].each do |p| + @mutex.synchronize do + @pools[frequency].select!(&:weakref_alive?) + @pools[frequency].each do |p| p.reap p.flush + rescue WeakRef::RefError end end end end end - end end def run 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 688eea75e8..dbd533b4b3 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -264,8 +264,7 @@ module ActiveRecord if_not_exists: false, options: nil, as: nil, - comment: nil, - ** + comment: nil ) @conn = conn @columns_hash = {} diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb index 622e00fffb..fb56e712be 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb @@ -15,7 +15,7 @@ module ActiveRecord def column_spec_for_primary_key(column) return {} if default_primary_key?(column) spec = { id: schema_type(column).inspect } - spec.merge!(prepare_column_options(column).except!(:null)) + spec.merge!(prepare_column_options(column).except!(:null, :comment)) spec[:default] ||= "nil" if explicit_primary_key_default?(column) spec end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index f97842b3f5..cf57af5473 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -291,25 +291,25 @@ module ActiveRecord # SELECT * FROM orders INNER JOIN line_items ON order_id=orders.id # # See also TableDefinition#column for details on how to create columns. - def create_table(table_name, **options) - td = create_table_definition(table_name, options) + def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **options) + td = create_table_definition( + table_name, options.extract!(:temporary, :if_not_exists, :options, :as, :comment) + ) - if options[:id] != false && !options[:as] - pk = options.fetch(:primary_key) do - Base.get_primary_key table_name.to_s.singularize - end + if id && !td.as + pk = primary_key || Base.get_primary_key(table_name.to_s.singularize) if pk.is_a?(Array) td.primary_keys pk else - td.primary_key pk, options.fetch(:id, :primary_key), options + td.primary_key pk, id, options end end yield td if block_given? - if options[:force] - drop_table(table_name, options.merge(if_exists: true)) + if force + drop_table(table_name, force: force, if_exists: true) end result = execute schema_creation.accept td @@ -321,7 +321,7 @@ module ActiveRecord end if supports_comments? && !supports_comments_in_create? - if table_comment = options[:comment].presence + if table_comment = td.comment.presence change_table_comment(table_name, table_comment) end @@ -518,14 +518,15 @@ module ActiveRecord # Available options are (none of these exists by default): # * <tt>:limit</tt> - # Requests a maximum column length. This is the number of characters for a <tt>:string</tt> column - # and number of bytes for <tt>:text</tt>, <tt>:binary</tt> and <tt>:integer</tt> columns. + # and number of bytes for <tt>:text</tt>, <tt>:binary</tt>, and <tt>:integer</tt> columns. # This option is ignored by some backends. # * <tt>:default</tt> - # The column's default value. Use +nil+ for +NULL+. # * <tt>:null</tt> - # Allows or disallows +NULL+ values in the column. # * <tt>:precision</tt> - - # Specifies the precision for the <tt>:decimal</tt> and <tt>:numeric</tt> columns. + # Specifies the precision for the <tt>:decimal</tt>, <tt>:numeric</tt>, + # <tt>:datetime</tt>, and <tt>:time</tt> columns. # * <tt>:scale</tt> - # Specifies the scale for the <tt>:decimal</tt> and <tt>:numeric</tt> columns. # * <tt>:collation</tt> - diff --git a/activerecord/lib/active_record/integration.rb b/activerecord/lib/active_record/integration.rb index 573a823dbc..4a97061731 100644 --- a/activerecord/lib/active_record/integration.rb +++ b/activerecord/lib/active_record/integration.rb @@ -93,7 +93,7 @@ module ActiveRecord # cache_version, but this method can be overwritten to return something else. # # Note, this method will return nil if ActiveRecord::Base.cache_versioning is set to - # +false+ (which it is by default until Rails 6.0). + # +false+. def cache_version return unless cache_versioning diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index a58c5fd48a..b7bc4b3b8e 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -939,7 +939,7 @@ module ActiveRecord end def verify_readonly_attribute(name) - raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name) + raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attribute?(name) end def _raise_record_not_destroyed diff --git a/activerecord/lib/active_record/readonly_attributes.rb b/activerecord/lib/active_record/readonly_attributes.rb index 7bc26993d5..c851ed52c3 100644 --- a/activerecord/lib/active_record/readonly_attributes.rb +++ b/activerecord/lib/active_record/readonly_attributes.rb @@ -19,6 +19,10 @@ module ActiveRecord def readonly_attributes _attr_readonly end + + def readonly_attribute?(name) # :nodoc: + _attr_readonly.include?(name) + end end end end diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb index 04a1c03474..a5862ae06b 100644 --- a/activerecord/lib/active_record/timestamp.rb +++ b/activerecord/lib/active_record/timestamp.rb @@ -59,19 +59,26 @@ module ActiveRecord attribute_names.index_with(time || current_time_from_proper_timezone) end - private - def timestamp_attributes_for_create_in_model - timestamp_attributes_for_create.select { |c| column_names.include?(c) } - end + def timestamp_attributes_for_create_in_model + @timestamp_attributes_for_create_in_model ||= + (timestamp_attributes_for_create & column_names).freeze + end - def timestamp_attributes_for_update_in_model - timestamp_attributes_for_update.select { |c| column_names.include?(c) } - end + def timestamp_attributes_for_update_in_model + @timestamp_attributes_for_update_in_model ||= + (timestamp_attributes_for_update & column_names).freeze + end - def all_timestamp_attributes_in_model - timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model - end + def all_timestamp_attributes_in_model + @all_timestamp_attributes_in_model ||= + (timestamp_attributes_for_create_in_model + timestamp_attributes_for_update_in_model).freeze + end + def current_time_from_proper_timezone + default_timezone == :utc ? Time.now.utc : Time.now + end + + private def timestamp_attributes_for_create ["created_at", "created_on"] end @@ -80,8 +87,11 @@ module ActiveRecord ["updated_at", "updated_on"] end - def current_time_from_proper_timezone - default_timezone == :utc ? Time.now.utc : Time.now + def reload_schema_from_cache + @timestamp_attributes_for_create_in_model = nil + @timestamp_attributes_for_update_in_model = nil + @all_timestamp_attributes_in_model = nil + super end end @@ -124,19 +134,19 @@ module ActiveRecord end def timestamp_attributes_for_create_in_model - self.class.send(:timestamp_attributes_for_create_in_model) + self.class.timestamp_attributes_for_create_in_model end def timestamp_attributes_for_update_in_model - self.class.send(:timestamp_attributes_for_update_in_model) + self.class.timestamp_attributes_for_update_in_model end def all_timestamp_attributes_in_model - self.class.send(:all_timestamp_attributes_in_model) + self.class.all_timestamp_attributes_in_model end def current_time_from_proper_timezone - self.class.send(:current_time_from_proper_timezone) + self.class.current_time_from_proper_timezone end def max_updated_column_timestamp diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb index 49f754be63..cbe48a374f 100644 --- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb +++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb @@ -37,8 +37,8 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_hmt_does_not_table_name_collide_when_joining_associations authors = Author.joins(:posts).eager_load(:comments).where(posts: { tags_count: 1 }).order(:id).to_a - assert_equal 3, assert_no_queries { authors.size } - assert_equal 10, assert_no_queries { authors[0].comments.size } + assert_equal 3, assert_queries(0) { authors.size } + assert_equal 10, assert_queries(0) { authors[0].comments.size } end def test_eager_association_loading_grafts_stashed_associations_to_correct_parent @@ -103,14 +103,14 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase firms = Firm.all.merge!(includes: { account: { firm: :account } }, order: "companies.id").to_a assert_equal 2, firms.size assert_equal firms.first.account, firms.first.account.firm.account - assert_equal companies(:first_firm).account, assert_no_queries { firms.first.account.firm.account } - assert_equal companies(:first_firm).account.firm.account, assert_no_queries { firms.first.account.firm.account } + assert_equal companies(:first_firm).account, assert_queries(0) { firms.first.account.firm.account } + assert_equal companies(:first_firm).account.firm.account, assert_queries(0) { firms.first.account.firm.account } end def test_eager_association_loading_with_has_many_sti topics = Topic.all.merge!(includes: :replies, order: "topics.id").to_a first, second, = topics(:first).replies.size, topics(:second).replies.size - assert_no_queries do + assert_queries(0) do assert_equal first, topics[0].replies.size assert_equal second, topics[1].replies.size end @@ -131,13 +131,13 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase replies = Reply.all.merge!(includes: :topic, order: "topics.id").to_a assert_includes replies, topics(:second) assert_not_includes replies, topics(:first) - assert_equal topics(:first), assert_no_queries { replies.first.topic } + assert_equal topics(:first), assert_queries(0) { replies.first.topic } end def test_eager_association_loading_with_multiple_stis_and_order author = Author.all.merge!(includes: { posts: [ :special_comments, :very_special_comment ] }, order: ["authors.name", "comments.body", "very_special_comments_posts.body"], where: "posts.id = 4").first assert_equal authors(:david), author - assert_no_queries do + assert_queries(0) do author.posts.first.special_comments author.posts.first.very_special_comment end @@ -146,7 +146,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_of_stis_with_multiple_references authors = Author.all.merge!(includes: { posts: { special_comments: { post: [ :special_comments, :very_special_comment ] } } }, order: "comments.body, very_special_comments_posts.body", where: "posts.id = 4").to_a assert_equal [authors(:david)], authors - assert_no_queries do + assert_queries(0) do authors.first.posts.first.special_comments.first.post.special_comments authors.first.posts.first.special_comments.first.post.very_special_comment end @@ -155,14 +155,14 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_where_first_level_returns_nil authors = Author.all.merge!(includes: { post_about_thinking: :comments }, order: "authors.id DESC").to_a assert_equal [authors(:bob), authors(:mary), authors(:david)], authors - assert_no_queries do + assert_queries(0) do authors[2].post_about_thinking.comments.first end end def test_preload_through_missing_records post = Post.where.not(author_id: Author.select(:id)).preload(author: { comments: :post }).first! - assert_no_queries { assert_nil post.author } + assert_queries(0) { assert_nil post.author } end def test_eager_association_loading_with_missing_first_record @@ -172,12 +172,12 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_recursive_cascading_four_levels_has_many_through source = Vertex.all.merge!(includes: { sinks: { sinks: { sinks: :sinks } } }, order: "vertices.id").first - assert_equal vertices(:vertex_4), assert_no_queries { source.sinks.first.sinks.first.sinks.first } + assert_equal vertices(:vertex_4), assert_queries(0) { source.sinks.first.sinks.first.sinks.first } end def test_eager_association_loading_with_recursive_cascading_four_levels_has_and_belongs_to_many sink = Vertex.all.merge!(includes: { sources: { sources: { sources: :sources } } }, order: "vertices.id DESC").first - assert_equal vertices(:vertex_1), assert_no_queries { sink.sources.first.sources.first.sources.first.sources.first } + assert_equal vertices(:vertex_1), assert_queries(0) { sink.sources.first.sources.first.sources.first.sources.first } end def test_eager_association_loading_with_cascaded_interdependent_one_level_and_two_levels diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 9ed25ca7c2..f7aad9d775 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -1245,7 +1245,7 @@ class EagerAssociationTest < ActiveRecord::TestCase Post.all.merge!(select: "posts.*, authors.name as author_name", includes: :comments, joins: :author, order: "posts.id").to_a end assert_equal "David", posts[0].author_name - assert_equal posts(:welcome).comments, assert_no_queries { posts[0].comments } + assert_equal posts(:welcome).comments.sort_by(&:id), assert_no_queries { posts[0].comments.sort_by(&:id) } end def test_eager_loading_with_conditions_on_join_model_preloads @@ -1257,8 +1257,8 @@ class EagerAssociationTest < ActiveRecord::TestCase end def test_preload_belongs_to_uses_exclusive_scope - people = Person.males.merge(includes: :primary_contact).to_a - assert_not_equal people.length, 0 + people = Person.males.includes(:primary_contact).to_a + assert_equal 2, people.length people.each do |person| assert_no_queries { assert_not_nil person.primary_contact } assert_equal Person.find(person.id).primary_contact, person.primary_contact @@ -1267,16 +1267,17 @@ class EagerAssociationTest < ActiveRecord::TestCase def test_preload_has_many_uses_exclusive_scope people = Person.males.includes(:agents).to_a + assert_equal 2, people.length people.each do |person| - assert_equal Person.find(person.id).agents, person.agents + assert_equal Person.find(person.id).agents.sort_by(&:id), person.agents.sort_by(&:id) end end def test_preload_has_many_using_primary_key - expected = Firm.first.clients_using_primary_key.to_a + expected = Firm.first.clients_using_primary_key.sort_by(&:id) firm = Firm.includes(:clients_using_primary_key).first assert_no_queries do - assert_equal expected, firm.clients_using_primary_key + assert_equal expected, firm.clients_using_primary_key.sort_by(&:id) end end 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 0e9dafeee6..25cfa0a723 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 @@ -550,7 +550,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase developer = project.developers.first - assert_no_queries do + assert_queries(0) do assert_predicate project.developers, :loaded? assert_includes project.developers, developer end @@ -745,7 +745,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase def test_get_ids_for_loaded_associations developer = developers(:david) developer.projects.reload - assert_no_queries do + assert_queries(0) do developer.project_ids developer.project_ids end @@ -873,7 +873,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase def test_has_and_belongs_to_many_associations_on_new_records_use_null_relations projects = Developer.new.projects - assert_no_queries do + assert_queries(0) do assert_equal [], projects assert_equal [], projects.where(title: "omg") assert_equal [], projects.pluck(:title) diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 0d13174e12..e42af3686e 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -837,7 +837,7 @@ class CalculationsTest < ActiveRecord::TestCase def test_pluck_columns_with_same_name expected = [["The First Topic", "The Second Topic of the day"], ["The Third Topic of the day", "The Fourth Topic of the day"]] - actual = Topic.joins(:replies) + actual = Topic.joins(:replies).order(:id) .pluck("topics.title", "replies_topics.title") assert_equal expected, actual end diff --git a/activerecord/test/cases/comment_test.rb b/activerecord/test/cases/comment_test.rb index 584e03d196..25e2f20676 100644 --- a/activerecord/test/cases/comment_test.rb +++ b/activerecord/test/cases/comment_test.rb @@ -14,6 +14,9 @@ if ActiveRecord::Base.connection.supports_comments? class BlankComment < ActiveRecord::Base end + class PkCommented < ActiveRecord::Base + end + setup do @connection = ActiveRecord::Base.connection @@ -35,8 +38,13 @@ if ActiveRecord::Base.connection.supports_comments? t.index :absent_comment end + @connection.create_table("pk_commenteds", comment: "Table comment", id: false, force: true) do |t| + t.integer :id, comment: "Primary key comment", primary_key: true + end + Commented.reset_column_information BlankComment.reset_column_information + PkCommented.reset_column_information end teardown do @@ -44,6 +52,11 @@ if ActiveRecord::Base.connection.supports_comments? @connection.drop_table "blank_comments", if_exists: true end + def test_default_primary_key_comment + column = Commented.columns_hash["id"] + assert_nil column.comment + end + def test_column_created_in_block column = Commented.columns_hash["name"] assert_equal :string, column.type @@ -164,5 +177,17 @@ if ActiveRecord::Base.connection.supports_comments? column = Commented.columns_hash["name"] assert_nil column.comment end + + def test_comment_on_primary_key + column = PkCommented.columns_hash["id"] + assert_equal "Primary key comment", column.comment + assert_equal "Table comment", @connection.table_comment("pk_commenteds") + end + + def test_schema_dump_with_primary_key_comment + output = dump_table_schema "pk_commenteds" + assert_match %r[create_table "pk_commenteds",.*\s+comment: "Table comment"], output + assert_no_match %r[create_table "pk_commenteds",.*\s+comment: "Primary key comment"], output + end end end diff --git a/activerecord/test/models/face.rb b/activerecord/test/models/face.rb index e900fd40fb..45ccc442ba 100644 --- a/activerecord/test/models/face.rb +++ b/activerecord/test/models/face.rb @@ -6,7 +6,7 @@ class Face < ActiveRecord::Base belongs_to :polymorphic_man, polymorphic: true, inverse_of: :polymorphic_face # Oracle identifier length is limited to 30 bytes or less, `polymorphic` renamed `poly` belongs_to :poly_man_without_inverse, polymorphic: true - # These is a "broken" inverse_of for the purposes of testing + # These are "broken" inverse_of associations for the purposes of testing belongs_to :horrible_man, class_name: "Man", inverse_of: :horrible_face belongs_to :horrible_polymorphic_man, polymorphic: true, inverse_of: :horrible_polymorphic_face |