From bbcd707aefe5da137137a8deb13908ec9a7db77d Mon Sep 17 00:00:00 2001 From: Vishal Telangre Date: Wed, 8 May 2019 18:11:43 +0530 Subject: Fix: ActiveRecord::RecordInvalid is not raised when an associated record fails to #save! due to uniqueness validation failure Add tests Fix tests failing due to introduction of uniquness rule added to Book model --- .../lib/active_record/autosave_association.rb | 2 +- .../test/cases/autosave_association_test.rb | 41 ++++++++++++++++++++++ activerecord/test/models/author.rb | 1 + activerecord/test/models/book.rb | 6 ++++ 4 files changed, 49 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 8d89e7d84a..2d4f0ad77e 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -411,7 +411,7 @@ module ActiveRecord saved = record.save(validate: false) end - raise ActiveRecord::Rollback unless saved + raise(RecordInvalid.new(association.owner)) unless saved end end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 1a0732c14b..45bb58eb06 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -2,6 +2,7 @@ require "cases/helper" require "models/author" +require "models/book" require "models/bird" require "models/post" require "models/comment" @@ -1685,6 +1686,10 @@ class TestAutosaveAssociationValidationsOnAHasManyAssociation < ActiveRecord::Te super @pirate = Pirate.create(catchphrase: "Don' botharrr talkin' like one, savvy?") @pirate.birds.create(name: "cookoo") + + @author = Author.new(name: "DHH") + @author.published_books.build(name: "Rework", isbn: "1234") + @author.published_books.build(name: "Remote", isbn: "1234") end test "should automatically validate associations" do @@ -1693,6 +1698,42 @@ class TestAutosaveAssociationValidationsOnAHasManyAssociation < ActiveRecord::Te assert_not_predicate @pirate, :valid? end + + test "rollbacks whole transaction and raises ActiveRecord::RecordInvalid when associations fail to #save! due to uniqueness validation failure" do + author_count_before_save = Author.count + book_count_before_save = Book.count + + assert_no_difference "Author.count" do + assert_no_difference "Book.count" do + exception = assert_raises(ActiveRecord::RecordInvalid) do + @author.save! + end + + assert_equal("Validation failed: Published books is invalid", exception.message) + end + end + + assert_equal(author_count_before_save, Author.count) + assert_equal(book_count_before_save, Book.count) + end + + test "rollbacks whole transaction when associations fail to #save due to uniqueness validation failure" do + author_count_before_save = Author.count + book_count_before_save = Book.count + + assert_no_difference "Author.count" do + assert_no_difference "Book.count" do + assert_nothing_raised do + result = @author.save + + assert_not(result) + end + end + end + + assert_equal(author_count_before_save, Author.count) + assert_equal(book_count_before_save, Book.count) + end end class TestAutosaveAssociationValidationsOnAHasOneAssociation < ActiveRecord::TestCase diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index b52b643ad7..da7e4139b1 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -116,6 +116,7 @@ class Author < ActiveRecord::Base has_many :tags_with_primary_key, through: :posts has_many :books + has_many :published_books, class_name: "PublishedBook" has_many :unpublished_books, -> { where(status: [:proposed, :written]) }, class_name: "Book" has_many :subscriptions, through: :books has_many :subscribers, -> { order("subscribers.nick") }, through: :subscriptions diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index afdda1a81e..43b82e6047 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -24,3 +24,9 @@ class Book < ActiveRecord::Base "do publish work..." end end + +class PublishedBook < ActiveRecord::Base + self.table_name = "books" + + validates_uniqueness_of :isbn +end -- cgit v1.2.3 From 17acb771d815f030ac1cf36b1af4050f02816c39 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 8 Apr 2019 13:10:15 +0200 Subject: Deduplicate various Active Record schema cache structures Real world database schemas contain a lot of duplicated data. Some column names like `id`, `created_at` etc can easily be repeated hundreds of times. Same for SqlTypeMetada, most database will contain only a limited number of possible combinations. This result in a lot of wasted memory. The idea here is to make these data sctructures immutable, use a registry to substitute similar instances with pre-existing ones. --- .../active_record/connection_adapters/column.rb | 14 +++++++++++ .../connection_adapters/deduplicable.rb | 29 ++++++++++++++++++++++ .../connection_adapters/mysql/type_metadata.rb | 11 +++++++- .../connection_adapters/postgresql/column.rb | 13 ++++++++++ .../postgresql/type_metadata.rb | 8 ++++++ .../connection_adapters/schema_cache.rb | 21 +++++++++++++++- .../connection_adapters/sql_type_metadata.rb | 10 ++++++++ .../connection_adapters/sqlite3_adapter.rb | 1 + .../connection_adapters/fake_adapter.rb | 3 ++- activerecord/test/cases/base_test.rb | 9 ++++--- activerecord/test/cases/json_serialization_test.rb | 2 +- activerecord/test/models/contact.rb | 18 +++++++------- 12 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 activerecord/lib/active_record/connection_adapters/deduplicable.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/column.rb b/activerecord/lib/active_record/connection_adapters/column.rb index 279d0b9e84..2708d2756b 100644 --- a/activerecord/lib/active_record/connection_adapters/column.rb +++ b/activerecord/lib/active_record/connection_adapters/column.rb @@ -5,6 +5,8 @@ module ActiveRecord module ConnectionAdapters # An abstract definition of a column in a table. class Column + include Deduplicable + attr_reader :name, :default, :sql_type_metadata, :null, :default_function, :collation, :comment delegate :precision, :scale, :limit, :type, :sql_type, to: :sql_type_metadata, allow_nil: true @@ -76,6 +78,7 @@ module ActiveRecord def hash Column.hash ^ name.hash ^ + name.encoding.hash ^ default.hash ^ sql_type_metadata.hash ^ null.hash ^ @@ -83,6 +86,17 @@ module ActiveRecord collation.hash ^ comment.hash end + + private + def deduplicated + @name = -name + @sql_type_metadata = sql_type_metadata.deduplicate if sql_type_metadata + @default = -default if default + @default_function = -default_function if default_function + @collation = -collation if collation + @comment = -comment if comment + super + end end class NullColumn < Column diff --git a/activerecord/lib/active_record/connection_adapters/deduplicable.rb b/activerecord/lib/active_record/connection_adapters/deduplicable.rb new file mode 100644 index 0000000000..fb2fd60bbc --- /dev/null +++ b/activerecord/lib/active_record/connection_adapters/deduplicable.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module ActiveRecord + module ConnectionAdapters # :nodoc: + module Deduplicable + extend ActiveSupport::Concern + + module ClassMethods + def registry + @registry ||= {} + end + + def new(*) + super.deduplicate + end + end + + def deduplicate + self.class.registry[self] ||= deduplicated + end + alias :-@ :deduplicate + + private + def deduplicated + freeze + end + end + end +end diff --git a/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb b/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb index 9167593064..a7232fa249 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/type_metadata.rb @@ -6,9 +6,11 @@ module ActiveRecord class TypeMetadata < DelegateClass(SqlTypeMetadata) # :nodoc: undef to_yaml if method_defined?(:to_yaml) + include Deduplicable + attr_reader :extra - def initialize(type_metadata, extra: "") + def initialize(type_metadata, extra: nil) super(type_metadata) @extra = extra end @@ -25,6 +27,13 @@ module ActiveRecord __getobj__.hash ^ extra.hash end + + private + def deduplicated + __setobj__(__getobj__.deduplicate) + @extra = -extra if extra + super + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb index ec25bb1e19..b4ea6c42d3 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb @@ -23,6 +23,19 @@ module ActiveRecord def sql_type super.sub(/\[\]\z/, "") end + + def ==(other) + other.is_a?(Column) && + super && + serial? == other.serial? + end + alias :eql? :== + + def hash + Column.hash ^ + super.hash ^ + serial?.hash + end end end PostgreSQLColumn = PostgreSQL::Column # :nodoc: diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb b/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb index 8bdec623af..b7f6479357 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb @@ -7,6 +7,8 @@ module ActiveRecord class TypeMetadata < DelegateClass(SqlTypeMetadata) undef to_yaml if method_defined?(:to_yaml) + include Deduplicable + attr_reader :oid, :fmod def initialize(type_metadata, oid: nil, fmod: nil) @@ -29,6 +31,12 @@ module ActiveRecord oid.hash ^ fmod.hash end + + private + def deduplicated + __setobj__(__getobj__.deduplicate) + super + end end end PostgreSQLTypeMetadata = PostgreSQL::TypeMetadata diff --git a/activerecord/lib/active_record/connection_adapters/schema_cache.rb b/activerecord/lib/active_record/connection_adapters/schema_cache.rb index dbfe1e4a34..218d6c168a 100644 --- a/activerecord/lib/active_record/connection_adapters/schema_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/schema_cache.rb @@ -129,10 +129,29 @@ module ActiveRecord def marshal_load(array) @version, @columns, @columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array - @indexes = @indexes || {} + @indexes ||= {} + + @columns = deep_deduplicate(@columns) + @columns_hash = deep_deduplicate(@columns_hash) + @primary_keys = deep_deduplicate(@primary_keys) + @data_sources = deep_deduplicate(@data_sources) + @indexes = deep_deduplicate(@indexes) end private + def deep_deduplicate(value) + case value + when Hash + value.transform_keys { |k| deep_deduplicate(k) }.transform_values { |v| deep_deduplicate(v) } + when Array + value.map { |i| deep_deduplicate(i) } + when String, Deduplicable + -value + else + value + end + end + def prepare_data_sources connection.data_sources.each { |source| @data_sources[source] = true } end diff --git a/activerecord/lib/active_record/connection_adapters/sql_type_metadata.rb b/activerecord/lib/active_record/connection_adapters/sql_type_metadata.rb index df28df7a7c..969867e70f 100644 --- a/activerecord/lib/active_record/connection_adapters/sql_type_metadata.rb +++ b/activerecord/lib/active_record/connection_adapters/sql_type_metadata.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true +require "active_record/connection_adapters/deduplicable" + module ActiveRecord # :stopdoc: module ConnectionAdapters class SqlTypeMetadata + include Deduplicable + attr_reader :sql_type, :type, :limit, :precision, :scale def initialize(sql_type: nil, type: nil, limit: nil, precision: nil, scale: nil) @@ -32,6 +36,12 @@ module ActiveRecord precision.hash >> 1 ^ scale.hash >> 2 end + + private + def deduplicated + @sql_type = -sql_type + super + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 7f3f32162e..733a8f1c86 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -381,6 +381,7 @@ module ActiveRecord if from_primary_key.is_a?(Array) @definition.primary_keys from_primary_key end + columns(from).each do |column| column_name = options[:rename] ? (options[:rename][column.name] || diff --git a/activerecord/test/active_record/connection_adapters/fake_adapter.rb b/activerecord/test/active_record/connection_adapters/fake_adapter.rb index f977b2997b..f1f457aedd 100644 --- a/activerecord/test/active_record/connection_adapters/fake_adapter.rb +++ b/activerecord/test/active_record/connection_adapters/fake_adapter.rb @@ -32,7 +32,8 @@ module ActiveRecord name.to_s, options[:default], fetch_type_metadata(sql_type), - options[:null]) + options[:null], + ) end def columns(table_name) diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index ddafa468ed..1c83100a75 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1141,11 +1141,14 @@ class BasicsTest < ActiveRecord::TestCase def test_clear_cache! # preheat cache c1 = Post.connection.schema_cache.columns("posts") + assert_not_equal 0, Post.connection.schema_cache.size + ActiveRecord::Base.clear_cache! + assert_equal 0, Post.connection.schema_cache.size + c2 = Post.connection.schema_cache.columns("posts") - c1.each_with_index do |v, i| - assert_not_same v, c2[i] - end + assert_not_equal 0, Post.connection.schema_cache.size + assert_equal c1, c2 end diff --git a/activerecord/test/cases/json_serialization_test.rb b/activerecord/test/cases/json_serialization_test.rb index 82cf281cff..8f9b1e2ec1 100644 --- a/activerecord/test/cases/json_serialization_test.rb +++ b/activerecord/test/cases/json_serialization_test.rb @@ -24,7 +24,7 @@ class JsonSerializationTest < ActiveRecord::TestCase include JsonSerializationHelpers class NamespacedContact < Contact - column :name, :string + column :name, "string" end def setup diff --git a/activerecord/test/models/contact.rb b/activerecord/test/models/contact.rb index 6e02ff199b..d5f6f00691 100644 --- a/activerecord/test/models/contact.rb +++ b/activerecord/test/models/contact.rb @@ -10,14 +10,14 @@ module ContactFakeColumns table_name => "id" } - column :id, :integer - column :name, :string - column :age, :integer - column :avatar, :binary - column :created_at, :datetime - column :awesome, :boolean - column :preferences, :string - column :alternative_id, :integer + column :id, "integer" + column :name, "string" + column :age, "integer" + column :avatar, "binary" + column :created_at, "datetime" + column :awesome, "boolean" + column :preferences, "string" + column :alternative_id, "integer" serialize :preferences @@ -37,7 +37,7 @@ end class ContactSti < ActiveRecord::Base extend ContactFakeColumns - column :type, :string + column :type, "string" def type; "ContactSti" end end -- cgit v1.2.3 From d29d4598972e85df7ee42f5aa969b51d1b14d615 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 14 Jun 2019 08:50:24 +0900 Subject: Avoid redundant `time.getutc` call if it is already utc time object Currently `type.serialize` and `connection.{quote|type_cast}` for a time object always does `time.getutc` call regardless of whether it is already utc time object or not, that duplicated proccess (`connection.type_cast(type.serialize(time))`) allocates extra/useless time objects for each type casting. This avoids that redundant `time.getutc` call if it is already utc time object. In the case of a model has timestamps (`created_at` and `updated_at`), it avoids 6,000 time objects allocation for 1,000 times `model.save`. ```ruby ObjectSpace::AllocationTracer.setup(%i{path line type}) pp ObjectSpace::AllocationTracer.trace { 1_000.times { User.create } }.select { |k, _| k[0].end_with?("quoting.rb", "time_value.rb") } ``` Before (c104bfe424e6cebe9c8e85a38515327a6c88b1f8): ``` {["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 203, :T_ARRAY]=>[1004, 0, 778, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 220, :T_STRING]=>[2, 0, 2, 1, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 209, :T_ARRAY]=>[8, 0, 8, 1, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 57, :T_ARRAY]=>[4, 0, 4, 1, 1, 0], ["~/rails/activemodel/lib/active_model/type/helpers/time_value.rb", 17, :T_DATA]=>[4000, 0, 3096, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 120, :T_DATA]=>[2000, 0, 1548, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 126, :T_STRING]=>[4000, 0, 3096, 0, 1, 0]} ``` After (this change): ``` {["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 203, :T_ARRAY]=>[1004, 0, 823, 0, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 220, :T_STRING]=>[2, 0, 2, 1, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 209, :T_ARRAY]=>[8, 0, 8, 1, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 57, :T_ARRAY]=>[4, 0, 4, 1, 1, 0], ["~/rails/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb", 126, :T_STRING]=>[2000, 0, 1638, 0, 1, 0]} ``` --- .../lib/active_record/connection_adapters/abstract/quoting.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb index 1b6ba8ce97..93273f6cf6 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb @@ -114,16 +114,16 @@ module ActiveRecord # if the value is a Time responding to usec. def quoted_date(value) if value.acts_like?(:time) - zone_conversion_method = ActiveRecord::Base.default_timezone == :utc ? :getutc : :getlocal - - if value.respond_to?(zone_conversion_method) - value = value.send(zone_conversion_method) + if ActiveRecord::Base.default_timezone == :utc + value = value.getutc if value.respond_to?(:getutc) && !value.utc? + else + value = value.getlocal if value.respond_to?(:getlocal) end end result = value.to_s(:db) if value.respond_to?(:usec) && value.usec > 0 - "#{result}.#{sprintf("%06d", value.usec)}" + result << "." << sprintf("%06d", value.usec) else result end -- cgit v1.2.3 From 2c96b046ec50e496ccdb6a196efcbbba5818a79c Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 19 Jun 2019 23:18:22 +0900 Subject: Add test cases to ensure deterministic order for ordinal methods Before 1340498d2, `order` with no-op value (e.g. `nil`, `""`) had broken the contract of ordinal methods, which returns a result deterministic ordered. --- activerecord/test/cases/finder_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'activerecord') diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index ca114d468e..3aa610f86b 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -517,6 +517,7 @@ class FinderTest < ActiveRecord::TestCase expected.touch # PostgreSQL changes the default order if no order clause is used assert_equal expected, Topic.first assert_equal expected, Topic.limit(5).first + assert_equal expected, Topic.order(nil).first end def test_model_class_responds_to_first_bang @@ -540,6 +541,7 @@ class FinderTest < ActiveRecord::TestCase expected.touch # PostgreSQL changes the default order if no order clause is used assert_equal expected, Topic.second assert_equal expected, Topic.limit(5).second + assert_equal expected, Topic.order(nil).second end def test_model_class_responds_to_second_bang @@ -563,6 +565,7 @@ class FinderTest < ActiveRecord::TestCase expected.touch # PostgreSQL changes the default order if no order clause is used assert_equal expected, Topic.third assert_equal expected, Topic.limit(5).third + assert_equal expected, Topic.order(nil).third end def test_model_class_responds_to_third_bang @@ -586,6 +589,7 @@ class FinderTest < ActiveRecord::TestCase expected.touch # PostgreSQL changes the default order if no order clause is used assert_equal expected, Topic.fourth assert_equal expected, Topic.limit(5).fourth + assert_equal expected, Topic.order(nil).fourth end def test_model_class_responds_to_fourth_bang @@ -609,6 +613,7 @@ class FinderTest < ActiveRecord::TestCase expected.touch # PostgreSQL changes the default order if no order clause is used assert_equal expected, Topic.fifth assert_equal expected, Topic.limit(5).fifth + assert_equal expected, Topic.order(nil).fifth end def test_model_class_responds_to_fifth_bang @@ -777,6 +782,7 @@ class FinderTest < ActiveRecord::TestCase assert_equal expected, clients.first(2) assert_equal expected, clients.limit(5).first(2) + assert_equal expected, clients.order(nil).first(2) end def test_implicit_order_column_is_configurable -- cgit v1.2.3 From dc99e5fe6529f3702b0490349cbc1f45c031a5c6 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 19 Jun 2019 14:21:44 +0200 Subject: Stop serializing and parsing columns_hash in Active Record schema caches --- .../lib/active_record/connection_adapters/schema_cache.rb | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/schema_cache.rb b/activerecord/lib/active_record/connection_adapters/schema_cache.rb index 218d6c168a..5da26171bb 100644 --- a/activerecord/lib/active_record/connection_adapters/schema_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/schema_cache.rb @@ -27,7 +27,6 @@ module ActiveRecord def encode_with(coder) coder["columns"] = @columns - coder["columns_hash"] = @columns_hash coder["primary_keys"] = @primary_keys coder["data_sources"] = @data_sources coder["indexes"] = @indexes @@ -37,7 +36,7 @@ module ActiveRecord def init_with(coder) @columns = coder["columns"] - @columns_hash = coder["columns_hash"] + @columns_hash = {} @primary_keys = coder["primary_keys"] @data_sources = coder["data_sources"] @indexes = coder["indexes"] || {} @@ -79,9 +78,7 @@ module ActiveRecord # Get the columns for a table as a hash, key is the column name # value is the column object. def columns_hash(table_name) - @columns_hash[table_name] ||= Hash[columns(table_name).map { |col| - [col.name, col] - }] + @columns_hash[table_name] ||= columns(table_name).index_by(&:name) end # Checks whether the columns hash is already cached for a table. @@ -124,15 +121,15 @@ module ActiveRecord def marshal_dump # if we get current version during initialization, it happens stack over flow. @version = connection.migration_context.current_version - [@version, @columns, @columns_hash, @primary_keys, @data_sources, @indexes, database_version] + [@version, @columns, {}, @primary_keys, @data_sources, @indexes, database_version] end def marshal_load(array) - @version, @columns, @columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array + @version, @columns, _columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array @indexes ||= {} + @columns_hash = {} @columns = deep_deduplicate(@columns) - @columns_hash = deep_deduplicate(@columns_hash) @primary_keys = deep_deduplicate(@primary_keys) @data_sources = deep_deduplicate(@data_sources) @indexes = deep_deduplicate(@indexes) -- cgit v1.2.3 From fd3532204c7302ec80a245c72852a11288ec38b5 Mon Sep 17 00:00:00 2001 From: Guilherme Mansur Date: Tue, 14 May 2019 15:13:29 -0400 Subject: Better error message for calling columns_hash When a record does not have a table name, as in the case for a record with `self.abstract_class = true` and no `self.table_name` set the error message raises a cryptic: "ActiveRecord::StatementInvalid: Could not find table ''" this patch now raises a new `TableNotSpecified Error` Fixes: #36274 Co-Authored-By: Eugene Kenny --- activerecord/CHANGELOG.md | 4 ++++ activerecord/lib/active_record/errors.rb | 4 ++++ activerecord/lib/active_record/model_schema.rb | 3 +++ activerecord/test/cases/base_test.rb | 8 ++++++++ 4 files changed, 19 insertions(+) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8d4b01e995..8642227a2b 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Loading the schema for a model that has no `table_name` raises a `TableNotSpecified` error. + + *Guilherme Mansur*, *Eugene Kenny* + * PostgreSQL: Fix GROUP BY with ORDER BY virtual count attribute. Fixes #36022. diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index 60cf9818c1..c8c06375a3 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -38,6 +38,10 @@ module ActiveRecord class AdapterNotSpecified < ActiveRecordError end + # Raised when a model makes a query but it has not specified an associated table. + class TableNotSpecified < ActiveRecordError + end + # Raised when Active Record cannot find database adapter specified in # +config/database.yml+ or programmatically. class AdapterNotFound < ActiveRecordError diff --git a/activerecord/lib/active_record/model_schema.rb b/activerecord/lib/active_record/model_schema.rb index 2a45f63d64..18f19af6be 100644 --- a/activerecord/lib/active_record/model_schema.rb +++ b/activerecord/lib/active_record/model_schema.rb @@ -482,6 +482,9 @@ module ActiveRecord end def load_schema! + unless table_name + raise ActiveRecord::TableNotSpecified, "#{self} has no table configured. Set one with #{self}.table_name=" + end @columns_hash = connection.schema_cache.columns_hash(table_name).except(*ignored_columns) @columns_hash.each do |name, column| define_attribute( diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 983a46a2d0..1324bdf9b8 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1415,6 +1415,14 @@ class BasicsTest < ActiveRecord::TestCase assert_not_includes SymbolIgnoredDeveloper.columns_hash.keys, "first_name" end + test ".columns_hash raises an error if the record has an empty table name" do + expected_message = "FirstAbstractClass has no table configured. Set one with FirstAbstractClass.table_name=" + exception = assert_raises(ActiveRecord::TableNotSpecified) do + FirstAbstractClass.columns_hash + end + assert_equal expected_message, exception.message + end + test "ignored columns have no attribute methods" do assert_not_respond_to Developer.new, :first_name assert_not_respond_to Developer.new, :first_name= -- cgit v1.2.3 From 4feeee2abeaf61eed2f9ee8463d3ac9661ec8cc6 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Thu, 20 Jun 2019 14:00:42 +0200 Subject: Revert schema dumper to use strings rather than integers I think we should change this, but not in 6-0-stable since that's already in RC and I was trying to only make changes that won't require any app changes. This reverts a portion of https://github.com/rails/rails/pull/36439 that made all schema migration version numbers get dumped as an integer. While it doesn't _really_ matter it did change behavior. We should bring this back in 6.1 with a deprecation. --- activerecord/lib/active_record/migration.rb | 4 ++-- activerecord/lib/active_record/schema_migration.rb | 2 +- activerecord/test/cases/schema_dumper_test.rb | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 9db017cded..7edfec9903 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -1071,7 +1071,7 @@ module ActiveRecord def get_all_versions if schema_migration.table_exists? - schema_migration.all_versions + schema_migration.all_versions.map(&:to_i) else [] end @@ -1247,7 +1247,7 @@ module ActiveRecord end def load_migrated - @migrated_versions = Set.new(@schema_migration.all_versions) + @migrated_versions = Set.new(@schema_migration.all_versions.map(&:to_i)) end private diff --git a/activerecord/lib/active_record/schema_migration.rb b/activerecord/lib/active_record/schema_migration.rb index 58b21d2cc8..dec7fee986 100644 --- a/activerecord/lib/active_record/schema_migration.rb +++ b/activerecord/lib/active_record/schema_migration.rb @@ -45,7 +45,7 @@ module ActiveRecord end def all_versions - order(:version).pluck(:version).map(&:to_i) + order(:version).pluck(:version) end end diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb index 49e9be9565..bb7184c5fc 100644 --- a/activerecord/test/cases/schema_dumper_test.rb +++ b/activerecord/test/cases/schema_dumper_test.rb @@ -33,6 +33,7 @@ class SchemaDumperTest < ActiveRecord::TestCase schema_info = ActiveRecord::Base.connection.dump_schema_information assert_match(/20100201010101.*20100301010101/m, schema_info) + assert_includes schema_info, "20100101010101" ensure ActiveRecord::SchemaMigration.delete_all end -- cgit v1.2.3 From 688da62a4c43438374fa58b3aa028082a15b5e9e Mon Sep 17 00:00:00 2001 From: Yasuo Honda Date: Thu, 20 Jun 2019 14:12:58 +0000 Subject: Address test_statement_cache_with_in_clause failure due to nondeterministic sort order This failure is occasional, does not always reproduce. ```ruby $ cd activerecord $ bundle exec rake test_postgresql ... snip ... ....F Failure: ActiveRecord::BindParameterTest#test_statement_cache_with_in_clause [/home/yahonda/git/rails/activerecord/test/cases/bind_parameter_test.rb:97]: Expected: [1, 3] Actual: [3, 1] rails test home/yahonda/git/rails/activerecord/test/cases/bind_parameter_test.rb:93 ``` --- activerecord/test/cases/bind_parameter_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/bind_parameter_test.rb b/activerecord/test/cases/bind_parameter_test.rb index 85685d1d00..720446b39d 100644 --- a/activerecord/test/cases/bind_parameter_test.rb +++ b/activerecord/test/cases/bind_parameter_test.rb @@ -93,7 +93,7 @@ if ActiveRecord::Base.connection.prepared_statements def test_statement_cache_with_in_clause @connection.clear_cache! - topics = Topic.where(id: [1, 3]) + topics = Topic.where(id: [1, 3]).order(:id) assert_equal [1, 3], topics.map(&:id) assert_not_includes statement_cache, to_sql_key(topics.arel) end -- cgit v1.2.3 From bba7c63a663b073034f4c73f0d59655751694e5a Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 21 Jun 2019 12:11:16 +0200 Subject: Also deduplicate schema cache data when using the init_with interface --- .../lib/active_record/connection_adapters/schema_cache.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/schema_cache.rb b/activerecord/lib/active_record/connection_adapters/schema_cache.rb index 5da26171bb..d01327290d 100644 --- a/activerecord/lib/active_record/connection_adapters/schema_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/schema_cache.rb @@ -35,11 +35,11 @@ module ActiveRecord end def init_with(coder) - @columns = coder["columns"] - @columns_hash = {} - @primary_keys = coder["primary_keys"] - @data_sources = coder["data_sources"] - @indexes = coder["indexes"] || {} + @columns = deep_deduplicate(coder["columns"]) + @columns_hash = @columns.transform_values { |columns| columns.index_by(&:name) } + @primary_keys = deep_deduplicate(coder["primary_keys"]) + @data_sources = deep_deduplicate(coder["data_sources"]) + @indexes = deep_deduplicate(coder["indexes"] || {}) @version = coder["version"] @database_version = coder["database_version"] end @@ -127,9 +127,9 @@ module ActiveRecord def marshal_load(array) @version, @columns, _columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array @indexes ||= {} - @columns_hash = {} @columns = deep_deduplicate(@columns) + @columns_hash = @columns.transform_values { |columns| columns.index_by(&:name) } @primary_keys = deep_deduplicate(@primary_keys) @data_sources = deep_deduplicate(@data_sources) @indexes = deep_deduplicate(@indexes) -- cgit v1.2.3 From ef1dbd8cc725f3df9080870ce58afd7d822df98a Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Fri, 21 Jun 2019 18:20:16 +0200 Subject: Schema Cache: extract deduplication commonality --- .../connection_adapters/schema_cache.rb | 25 +++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/schema_cache.rb b/activerecord/lib/active_record/connection_adapters/schema_cache.rb index d01327290d..7d54fcf9a0 100644 --- a/activerecord/lib/active_record/connection_adapters/schema_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/schema_cache.rb @@ -35,13 +35,14 @@ module ActiveRecord end def init_with(coder) - @columns = deep_deduplicate(coder["columns"]) - @columns_hash = @columns.transform_values { |columns| columns.index_by(&:name) } - @primary_keys = deep_deduplicate(coder["primary_keys"]) - @data_sources = deep_deduplicate(coder["data_sources"]) - @indexes = deep_deduplicate(coder["indexes"] || {}) + @columns = coder["columns"] + @primary_keys = coder["primary_keys"] + @data_sources = coder["data_sources"] + @indexes = coder["indexes"] || {} @version = coder["version"] @database_version = coder["database_version"] + + derive_columns_hash_and_deduplicate_values end def primary_keys(table_name) @@ -128,14 +129,18 @@ module ActiveRecord @version, @columns, _columns_hash, @primary_keys, @data_sources, @indexes, @database_version = array @indexes ||= {} - @columns = deep_deduplicate(@columns) - @columns_hash = @columns.transform_values { |columns| columns.index_by(&:name) } - @primary_keys = deep_deduplicate(@primary_keys) - @data_sources = deep_deduplicate(@data_sources) - @indexes = deep_deduplicate(@indexes) + derive_columns_hash_and_deduplicate_values end private + def derive_columns_hash_and_deduplicate_values + @columns = deep_deduplicate(@columns) + @columns_hash = @columns.transform_values { |columns| columns.index_by(&:name) } + @primary_keys = deep_deduplicate(@primary_keys) + @data_sources = deep_deduplicate(@data_sources) + @indexes = deep_deduplicate(@indexes) + end + def deep_deduplicate(value) case value when Hash -- cgit v1.2.3 From f6db8b8d8286730757570a8b0f92e6b6be779fa8 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 26 Jun 2019 08:56:00 +0900 Subject: `length(title)` is a safe SQL string since #36448 --- activerecord/test/cases/relations_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 5df1e3ccf9..1a20fe5dc2 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -308,9 +308,9 @@ class RelationTest < ActiveRecord::TestCase end def test_reverse_order_with_function_other_predicates - topics = Topic.order(Arel.sql("author_name, length(title), id")).reverse_order + topics = Topic.order("author_name, length(title), id").reverse_order assert_equal topics(:second).title, topics.first.title - topics = Topic.order(Arel.sql("length(author_name), id, length(title)")).reverse_order + topics = Topic.order("length(author_name), id, length(title)").reverse_order assert_equal topics(:fifth).title, topics.first.title end -- cgit v1.2.3 From df6b0de7d9522c46c140c556ee61e09df14ef97a Mon Sep 17 00:00:00 2001 From: eileencodes Date: Wed, 26 Jun 2019 14:25:11 -0400 Subject: Load initial database.yml once, and warn if we can't create tasks For multiple databases we attempt to generate the tasks by reading the database.yml before the Rails application is booted. This means that we need to strip out ERB since it could be reading Rails configs. In some cases like https://github.com/rails/rails/issues/36540 the ERB is too complex and we can't overwrite with the DummyCompilier we used in https://github.com/rails/rails/pull/35497. For the complex causes we simply issue a warning that says we couldn't infer the database tasks from the database.yml. While working on this I decided to update the code to only load the database.yml once initially so that we avoid having to issue the same warning multiple times. Note that this had no performance impact in my testing and is merely for not having to save the error off somewhere. Also this feels cleaner. Note that this will not break running tasks that exist, it will just mean that tasks for multi-db like `db:create:other_db` will not be generated. If the database.yml is actually unreadable it will blow up during normal rake task calls. Fixes #36540 --- activerecord/lib/active_record/railties/databases.rake | 16 +++++++++------- activerecord/lib/active_record/tasks/database_tasks.rb | 13 +++++++++++-- 2 files changed, 20 insertions(+), 9 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake index d17acc408c..648fdd0dc4 100644 --- a/activerecord/lib/active_record/railties/databases.rake +++ b/activerecord/lib/active_record/railties/databases.rake @@ -2,6 +2,8 @@ require "active_record" +databases = ActiveRecord::Tasks::DatabaseTasks.setup_initial_database_yaml + db_namespace = namespace :db do desc "Set the environment value for the database" task "environment:set" => :load_config do @@ -23,7 +25,7 @@ db_namespace = namespace :db do ActiveRecord::Tasks::DatabaseTasks.create_all end - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| desc "Create #{spec_name} database for current environment" task spec_name => :load_config do db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name) @@ -42,7 +44,7 @@ db_namespace = namespace :db do ActiveRecord::Tasks::DatabaseTasks.drop_all end - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| desc "Drop #{spec_name} database for current environment" task spec_name => [:load_config, :check_protected_environments] do db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name) @@ -101,7 +103,7 @@ db_namespace = namespace :db do end namespace :migrate do - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| desc "Migrate #{spec_name} database for current environment" task spec_name => :load_config do db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name) @@ -142,7 +144,7 @@ db_namespace = namespace :db do end namespace :up do - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| task spec_name => :load_config do raise "VERSION is required" if !ENV["VERSION"] || ENV["VERSION"].empty? @@ -176,7 +178,7 @@ db_namespace = namespace :db do end namespace :down do - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| task spec_name => :load_config do raise "VERSION is required" if !ENV["VERSION"] || ENV["VERSION"].empty? @@ -203,7 +205,7 @@ db_namespace = namespace :db do end namespace :status do - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| desc "Display status of migrations for #{spec_name} database" task spec_name => :load_config do db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name) @@ -266,7 +268,7 @@ db_namespace = namespace :db do end namespace :abort_if_pending_migrations do - ActiveRecord::Tasks::DatabaseTasks.for_each do |spec_name| + ActiveRecord::Tasks::DatabaseTasks.for_each(databases) do |spec_name| # desc "Raises an error if there are pending migrations for #{spec_name} database" task spec_name => :load_config do db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, spec_name: spec_name) diff --git a/activerecord/lib/active_record/tasks/database_tasks.rb b/activerecord/lib/active_record/tasks/database_tasks.rb index aecc9350e8..a78bebf764 100644 --- a/activerecord/lib/active_record/tasks/database_tasks.rb +++ b/activerecord/lib/active_record/tasks/database_tasks.rb @@ -141,10 +141,19 @@ module ActiveRecord end end - def for_each + def setup_initial_database_yaml return {} unless defined?(Rails) - databases = Rails.application.config.load_database_yaml + begin + Rails.application.config.load_database_yaml + rescue + $stderr.puts "Rails couldn't infer whether you are using multiple databases from your database.yml and can't generate the tasks for the non-primary databases. If you'd like to use this feature, please simplify your ERB." + + {} + end + end + + def for_each(databases) database_configs = ActiveRecord::DatabaseConfigurations.new(databases).configs_for(env_name: Rails.env) # if this is a single database application we don't want tasks for each primary database -- cgit v1.2.3 From 7e08b6d2b2b67edbd0314b3a15c9ed5633f1c753 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 28 Jun 2019 00:26:22 +0900 Subject: Address to "DEPRECATION WARNING: Uniqueness validator will no longer enforce case sensitive comparison in Rails 6.1" Caused by #36210. --- activerecord/test/schema/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index eed18a7b89..b6c0ae0de2 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -115,7 +115,7 @@ ActiveRecord::Schema.define do t.column :font_size, :integer, **default_zero t.column :difficulty, :integer, **default_zero t.column :cover, :string, default: "hard" - t.string :isbn + t.string :isbn, **case_sensitive_options t.datetime :published_on t.index [:author_id, :name], unique: true t.index :isbn, where: "published_on IS NOT NULL", unique: true -- cgit v1.2.3 From f2ad69fe7a605b01bb7c37eeac6a9b4e7deb488e Mon Sep 17 00:00:00 2001 From: eileencodes Date: Thu, 27 Jun 2019 12:56:30 -0400 Subject: Fix broken url configs This PR is to fix #36559 but I also found other issues that haven't been reported. The check for `(config.size == 1 && config.values.all? { |v| v.is_a? String })` was naive. The only reason this passed was because we had tests that had single hash size configs, but that doesn't mean we don't want to create a hash config in other cases. So this now checks for `config["database"] || config["adapter"] || ENV["DATABASE_URL"]`. In the end for url configs we still get a UrlConfig but we need to pass through the HashConfig to create the right kind of UrlConfig. The UrlConfig's are really complex and I don't necessarily understand everything that's needed in order to act the same as Rails 5.2. I edited the connection handler test to demonstrate how the previous implementation was broken when checking config size. Now old and new tests pass so I think this is closer to 5.2. Fixes #36559 --- .../lib/active_record/database_configurations.rb | 2 +- .../connection_adapters/connection_handler_test.rb | 2 +- .../merge_and_resolve_default_url_config_test.rb | 31 ++++++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/database_configurations.rb b/activerecord/lib/active_record/database_configurations.rb index 44b5cfc738..b917f4e6b7 100644 --- a/activerecord/lib/active_record/database_configurations.rb +++ b/activerecord/lib/active_record/database_configurations.rb @@ -141,7 +141,7 @@ module ActiveRecord config_without_url.delete "url" ActiveRecord::DatabaseConfigurations::UrlConfig.new(env_name, spec_name, url, config_without_url) - elsif config["database"] || (config.size == 1 && config.values.all? { |v| v.is_a? String }) + elsif config["database"] || config["adapter"] || ENV["DATABASE_URL"] ActiveRecord::DatabaseConfigurations::HashConfig.new(env_name, spec_name, config) else config.each_pair.map do |sub_spec_name, sub_config| diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb index 27589966af..843242a897 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -29,7 +29,7 @@ module ActiveRecord def test_establish_connection_uses_spec_name old_config = ActiveRecord::Base.configurations - config = { "readonly" => { "adapter" => "sqlite3" } } + config = { "readonly" => { "adapter" => "sqlite3", "pool" => "5" } } ActiveRecord::Base.configurations = config resolver = ConnectionAdapters::ConnectionSpecification::Resolver.new(ActiveRecord::Base.configurations) spec = resolver.spec(:readonly) diff --git a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb index 515bf5df06..c0a9f8f9ca 100644 --- a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb +++ b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb @@ -273,6 +273,37 @@ module ActiveRecord } assert_equal expected, actual end + + def test_merge_no_conflicts_with_database_url_and_adapter + ENV["DATABASE_URL"] = "postgres://localhost/foo" + + config = { "default_env" => { "adapter" => "postgresql", "pool" => "5" } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost", + "pool" => "5" + } + } + assert_equal expected, actual + end + + def test_merge_no_conflicts_with_database_url_and_numeric_pool + ENV["DATABASE_URL"] = "postgres://localhost/foo" + + config = { "default_env" => { "pool" => 5 } } + actual = resolve_config(config) + expected = { "default_env" => + { "adapter" => "postgresql", + "database" => "foo", + "host" => "localhost", + "pool" => 5 + } + } + + assert_equal expected, actual + end end end end -- cgit v1.2.3 From 12bc75932562e0ab94da46f46bf719f76702d6d1 Mon Sep 17 00:00:00 2001 From: utilum Date: Thu, 27 Jun 2019 22:02:38 +0000 Subject: warning: instance variable @serial not initialized (#36556) Introduced in bba7c63a663b073034f4c73f0d59655751694e5a Before: ``` $ TESTOPTS="-n=/test_yaml_dump_and_load/" bundle exec rake test:postgresql :scisors: ... :scisors: Using postgresql Run options: -n=/test_yaml_dump_and_load/ --seed 36896 /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized /home/u/code/rails/activerecord/lib/active_record/connection_adapters/postgresql/column.rb:15: warning: instance variable @serial not initialized . Finished in 0.195325s, 5.1197 runs/s, 35.8376 assertions/s. 1 runs, 7 assertions, 0 failures, 0 errors, 0 skips ``` Co-authored-by: Ryuta Kamizono --- .../lib/active_record/connection_adapters/postgresql/column.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb index b4ea6c42d3..f1ecf6df30 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb @@ -24,6 +24,16 @@ module ActiveRecord super.sub(/\[\]\z/, "") end + def init_with(coder) + @serial = coder["serial"] + super + end + + def encode_with(coder) + coder["serial"] = @serial + super + end + def ==(other) other.is_a?(Column) && super && -- cgit v1.2.3 From 1eb5cc13a2ed8922b47df4ae47faf5f23faf3d35 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 28 Jun 2019 13:07:39 +0900 Subject: Add "SCHEMA" to the query in `configure_connection` like as other adapters This makes to be able to ignore the query in `assert_queries` even if accidentally reconnected a connection. https://buildkite.com/rails/rails/builds/61917#4c49187a-3173-4d5c-8a8d-d65768f5bfc9/1000-1799 --- .../lib/active_record/connection_adapters/abstract_mysql_adapter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord') 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 70292e6d63..e9ae8d159e 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -737,7 +737,7 @@ module ActiveRecord end.compact.join(", ") # ...and send them all in one query - execute "SET #{encoding} #{sql_mode_assignment} #{variable_assignments}" + execute("SET #{encoding} #{sql_mode_assignment} #{variable_assignments}", "SCHEMA") end def column_definitions(table_name) # :nodoc: -- cgit v1.2.3 From 52729fb5f24ff96c812327ff045b9a1921d37a7c Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 1 Jul 2019 12:07:31 +0900 Subject: MySQL: Fix schema dumping `enum` and `set` columns correctly `enum` and `set` are typed cast as `:string`, but currently the `:string` type is incorrectly reused for schema dumping. A cast type on columns is not always the same with `sql_type`, this fixes schema dumping `enum` and `set` columns to use `sql_type` instead of `type` correctly. --- .../connection_adapters/mysql/schema_dumper.rb | 4 ++- activerecord/lib/active_record/schema_dumper.rb | 6 +++- .../test/cases/adapters/mysql2/enum_test.rb | 14 ++++++++++ .../test/cases/adapters/mysql2/set_test.rb | 32 ++++++++++++++++++++++ .../connection_adapters/mysql_type_lookup_test.rb | 2 +- activerecord/test/schema/mysql2_specific_schema.rb | 4 --- 6 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 activerecord/test/cases/adapters/mysql2/set_test.rb (limited to 'activerecord') diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb index 234fb25fdf..bcd300f3db 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb @@ -41,13 +41,15 @@ module ActiveRecord case column.sql_type when /\Atimestamp\b/ :timestamp + when /\A(?:enum|set)\b/ + column.sql_type else super end end def schema_limit(column) - super unless /\A(?:tiny|medium|long)?(?:text|blob)/.match?(column.sql_type) + super unless /\A(?:enum|set|(?:tiny|medium|long)?(?:text|blob))\b/.match?(column.sql_type) end def schema_precision(column) diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb index 2f7cc07221..f4b1f536b3 100644 --- a/activerecord/lib/active_record/schema_dumper.rb +++ b/activerecord/lib/active_record/schema_dumper.rb @@ -146,7 +146,11 @@ HEADER raise StandardError, "Unknown type '#{column.sql_type}' for column '#{column.name}'" unless @connection.valid_type?(column.type) next if column.name == pk type, colspec = column_spec(column) - tbl.print " t.#{type} #{column.name.inspect}" + if type.is_a?(Symbol) + tbl.print " t.#{type} #{column.name.inspect}" + else + tbl.print " t.column #{column.name.inspect}, #{type.inspect}" + end tbl.print ", #{format_colspec(colspec)}" if colspec.present? tbl.puts end diff --git a/activerecord/test/cases/adapters/mysql2/enum_test.rb b/activerecord/test/cases/adapters/mysql2/enum_test.rb index 832f5d61d1..1168b3677e 100644 --- a/activerecord/test/cases/adapters/mysql2/enum_test.rb +++ b/activerecord/test/cases/adapters/mysql2/enum_test.rb @@ -1,11 +1,20 @@ # frozen_string_literal: true require "cases/helper" +require "support/schema_dumping_helper" class Mysql2EnumTest < ActiveRecord::Mysql2TestCase + include SchemaDumpingHelper + class EnumTest < ActiveRecord::Base end + def setup + EnumTest.connection.create_table :enum_tests, id: false, force: true do |t| + t.column :enum_column, "enum('text','blob','tiny','medium','long','unsigned','bigint')" + end + end + def test_enum_limit column = EnumTest.columns_hash["enum_column"] assert_equal 8, column.limit @@ -20,4 +29,9 @@ class Mysql2EnumTest < ActiveRecord::Mysql2TestCase column = EnumTest.columns_hash["enum_column"] assert_not_predicate column, :bigint? end + + def test_schema_dumping + schema = dump_table_schema "enum_tests" + assert_match %r{t\.column "enum_column", "enum\('text','blob','tiny','medium','long','unsigned','bigint'\)"$}, schema + end end diff --git a/activerecord/test/cases/adapters/mysql2/set_test.rb b/activerecord/test/cases/adapters/mysql2/set_test.rb new file mode 100644 index 0000000000..89107e142f --- /dev/null +++ b/activerecord/test/cases/adapters/mysql2/set_test.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "cases/helper" +require "support/schema_dumping_helper" + +class Mysql2SetTest < ActiveRecord::Mysql2TestCase + include SchemaDumpingHelper + + class SetTest < ActiveRecord::Base + end + + def setup + SetTest.connection.create_table :set_tests, id: false, force: true do |t| + t.column :set_column, "set('text','blob','tiny','medium','long','unsigned','bigint')" + end + end + + def test_should_not_be_unsigned + column = SetTest.columns_hash["set_column"] + assert_not_predicate column, :unsigned? + end + + def test_should_not_be_bigint + column = SetTest.columns_hash["set_column"] + assert_not_predicate column, :bigint? + end + + def test_schema_dumping + schema = dump_table_schema "set_tests" + assert_match %r{t\.column "set_column", "set\('text','blob','tiny','medium','long','unsigned','bigint'\)"$}, schema + end +end diff --git a/activerecord/test/cases/connection_adapters/mysql_type_lookup_test.rb b/activerecord/test/cases/connection_adapters/mysql_type_lookup_test.rb index bc823fd072..774380d7e0 100644 --- a/activerecord/test/cases/connection_adapters/mysql_type_lookup_test.rb +++ b/activerecord/test/cases/connection_adapters/mysql_type_lookup_test.rb @@ -40,7 +40,7 @@ if current_adapter?(:Mysql2Adapter) end def test_enum_type_with_value_matching_other_type - assert_lookup_type :string, "ENUM('unicode', '8bit', 'none')" + assert_lookup_type :string, "ENUM('unicode', '8bit', 'none', 'time')" end def test_binary_types diff --git a/activerecord/test/schema/mysql2_specific_schema.rb b/activerecord/test/schema/mysql2_specific_schema.rb index b143035213..911ac808c6 100644 --- a/activerecord/test/schema/mysql2_specific_schema.rb +++ b/activerecord/test/schema/mysql2_specific_schema.rb @@ -62,10 +62,6 @@ ActiveRecord::Schema.define do t.binary :binary_column, limit: 1 end - create_table :enum_tests, id: false, force: true do |t| - t.column :enum_column, "ENUM('text','blob','tiny','medium','long','unsigned','bigint')" - end - execute "DROP PROCEDURE IF EXISTS ten" execute <<~SQL -- cgit v1.2.3 From dddb331bd24ab163ac61b4af7abbdb920264bf9b Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 8 Jul 2019 08:46:07 +0900 Subject: Do not use aliases in GROUP BY clause It appears that Oracle does not allow using aliases in GROUP BY clause unlike ORDER BY clause. Fixes #36613. --- activerecord/lib/active_record/relation/calculations.rb | 2 +- activerecord/test/cases/calculations_test.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 0be9ba7d7b..0a14a33c1d 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -340,7 +340,7 @@ module ActiveRecord } relation = except(:group).distinct!(false) - relation.group_values = group_aliases + relation.group_values = group_fields relation.select_values = select_values calculated_data = skip_query_cache_if_necessary { @klass.connection.select_all(relation.arel, nil) } diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 525085bb28..dbd1d03c4c 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -139,6 +139,13 @@ class CalculationsTest < ActiveRecord::TestCase end end + def test_should_not_use_alias_for_grouped_field + assert_sql(/GROUP BY #{Regexp.escape(Account.connection.quote_table_name("accounts.firm_id"))}/i) do + c = Account.group(:firm_id).order("accounts_firm_id").sum(:credit_limit) + assert_equal [1, 2, 6, 9], c.keys.compact + end + end + def test_should_order_by_grouped_field c = Account.group(:firm_id).order("firm_id").sum(:credit_limit) assert_equal [1, 2, 6, 9], c.keys.compact -- cgit v1.2.3 From 7d699dad334996838dd529e4b75e1648692d56f8 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 8 Jul 2019 09:26:03 +0900 Subject: Should `Regexp.escape` quoted table name in regex It is for agnostic test case, since quoted table name may include `.` for all adapters, and `[` / `]` for sqlserver adapter. --- .../test/cases/adapters/mysql2/annotate_test.rb | 37 ----------------- .../cases/adapters/postgresql/annotate_test.rb | 37 ----------------- .../test/cases/adapters/sqlite3/annotate_test.rb | 37 ----------------- activerecord/test/cases/annotate_test.rb | 46 ++++++++++++++++++++++ activerecord/test/cases/batches_test.rb | 2 +- activerecord/test/cases/finder_test.rb | 3 +- activerecord/test/cases/inheritance_test.rb | 4 +- 7 files changed, 51 insertions(+), 115 deletions(-) delete mode 100644 activerecord/test/cases/adapters/mysql2/annotate_test.rb delete mode 100644 activerecord/test/cases/adapters/postgresql/annotate_test.rb delete mode 100644 activerecord/test/cases/adapters/sqlite3/annotate_test.rb create mode 100644 activerecord/test/cases/annotate_test.rb (limited to 'activerecord') diff --git a/activerecord/test/cases/adapters/mysql2/annotate_test.rb b/activerecord/test/cases/adapters/mysql2/annotate_test.rb deleted file mode 100644 index b512540073..0000000000 --- a/activerecord/test/cases/adapters/mysql2/annotate_test.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require "cases/helper" -require "models/post" - -class Mysql2AnnotateTest < ActiveRecord::Mysql2TestCase - fixtures :posts - - def test_annotate_wraps_content_in_an_inline_comment - assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do - posts = Post.select(:id).annotate("foo") - assert posts.first - end - end - - def test_annotate_is_sanitized - assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do - posts = Post.select(:id).annotate("*/foo/*") - assert posts.first - end - - assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/}) do - posts = Post.select(:id).annotate("**//foo//**") - assert posts.first - end - - assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* foo \*/ /\* bar \*/}) do - posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") - assert posts.first - end - - assert_sql(%r{\ASELECT `posts`\.`id` FROM `posts` /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do - posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") - assert posts.first - end - end -end diff --git a/activerecord/test/cases/adapters/postgresql/annotate_test.rb b/activerecord/test/cases/adapters/postgresql/annotate_test.rb deleted file mode 100644 index 42a2861511..0000000000 --- a/activerecord/test/cases/adapters/postgresql/annotate_test.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require "cases/helper" -require "models/post" - -class PostgresqlAnnotateTest < ActiveRecord::PostgreSQLTestCase - fixtures :posts - - def test_annotate_wraps_content_in_an_inline_comment - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("foo") - assert posts.first - end - end - - def test_annotate_is_sanitized - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("*/foo/*") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("**//foo//**") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/ /\* bar \*/}) do - posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do - posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") - assert posts.first - end - end -end diff --git a/activerecord/test/cases/adapters/sqlite3/annotate_test.rb b/activerecord/test/cases/adapters/sqlite3/annotate_test.rb deleted file mode 100644 index 6567a5eca3..0000000000 --- a/activerecord/test/cases/adapters/sqlite3/annotate_test.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require "cases/helper" -require "models/post" - -class SQLite3AnnotateTest < ActiveRecord::SQLite3TestCase - fixtures :posts - - def test_annotate_wraps_content_in_an_inline_comment - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("foo") - assert posts.first - end - end - - def test_annotate_is_sanitized - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("*/foo/*") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/}) do - posts = Post.select(:id).annotate("**//foo//**") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* foo \*/ /\* bar \*/}) do - posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") - assert posts.first - end - - assert_sql(%r{\ASELECT "posts"\."id" FROM "posts" /\* \+ MAX_EXECUTION_TIME\(1\) \*/}) do - posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") - assert posts.first - end - end -end diff --git a/activerecord/test/cases/annotate_test.rb b/activerecord/test/cases/annotate_test.rb new file mode 100644 index 0000000000..4d71d28f83 --- /dev/null +++ b/activerecord/test/cases/annotate_test.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require "cases/helper" +require "models/post" + +class AnnotateTest < ActiveRecord::TestCase + fixtures :posts + + def test_annotate_wraps_content_in_an_inline_comment + quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts") + + assert_sql(%r{\ASELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do + posts = Post.select(:id).annotate("foo") + assert posts.first + end + end + + def test_annotate_is_sanitized + quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts") + + assert_sql(%r{\ASELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do + posts = Post.select(:id).annotate("*/foo/*") + assert posts.first + end + + assert_sql(%r{\ASELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do + posts = Post.select(:id).annotate("**//foo//**") + assert posts.first + end + + assert_sql(%r{\ASELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/ /\* bar \*/}i) do + posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar") + assert posts.first + end + + assert_sql(%r{\ASELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \+ MAX_EXECUTION_TIME\(1\) \*/}i) do + posts = Post.select(:id).annotate("+ MAX_EXECUTION_TIME(1)") + assert posts.first + end + end + + private + def regexp_escape_table_name(name) + Regexp.escape(Post.connection.quote_table_name(name)) + end +end diff --git a/activerecord/test/cases/batches_test.rb b/activerecord/test/cases/batches_test.rb index cf6e280898..0d0bf39f79 100644 --- a/activerecord/test/cases/batches_test.rb +++ b/activerecord/test/cases/batches_test.rb @@ -146,7 +146,7 @@ class EachTest < ActiveRecord::TestCase def test_find_in_batches_should_quote_batch_order c = Post.connection - assert_sql(/ORDER BY #{c.quote_table_name('posts')}\.#{c.quote_column_name('id')}/) do + assert_sql(/ORDER BY #{Regexp.escape(c.quote_table_name("posts.id"))}/i) do Post.find_in_batches(batch_size: 1) do |batch| assert_kind_of Array, batch assert_kind_of Post, batch.first diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 3aa610f86b..3752fd42e3 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -245,7 +245,8 @@ class FinderTest < ActiveRecord::TestCase end def test_exists_does_not_select_columns_without_alias - assert_sql(/SELECT\W+1 AS one FROM ["`]topics["`]/i) do + c = Topic.connection + assert_sql(/SELECT 1 AS one FROM #{Regexp.escape(c.quote_table_name("topics"))}/i) do Topic.exists? end end diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index 629167e9ed..01e4878c3f 100644 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -471,9 +471,9 @@ class InheritanceTest < ActiveRecord::TestCase end def test_eager_load_belongs_to_primary_key_quoting - con = Account.connection + c = Account.connection bind_param = Arel::Nodes::BindParam.new(nil) - assert_sql(/#{con.quote_table_name('companies')}\.#{con.quote_column_name('id')} = (?:#{Regexp.quote(bind_param.to_sql)}|1)/) do + assert_sql(/#{Regexp.escape(c.quote_table_name("companies.id"))} = (?:#{Regexp.escape(bind_param.to_sql)}|1)/i) do Account.all.merge!(includes: :firm).find(1) end end -- cgit v1.2.3 From ff42b21915c08cf44f2c263dfe13c249dd780528 Mon Sep 17 00:00:00 2001 From: Will Jessop Date: Mon, 8 Jul 2019 05:49:58 +0100 Subject: When DATABASE_URL is specified don't trample envs that use a url: key fixes #36610 --- .../lib/active_record/database_configurations.rb | 10 +++++----- .../merge_and_resolve_default_url_config_test.rb | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/database_configurations.rb b/activerecord/lib/active_record/database_configurations.rb index b917f4e6b7..bf31bb7c22 100644 --- a/activerecord/lib/active_record/database_configurations.rb +++ b/activerecord/lib/active_record/database_configurations.rb @@ -153,11 +153,11 @@ module ActiveRecord def build_url_config(url, configs) env = ActiveRecord::ConnectionHandling::DEFAULT_ENV.call.to_s - if original_config = configs.find(&:for_current_env?) - if original_config.url_config? - configs - else - configs.map do |config| + if configs.find(&:for_current_env?) + configs.map do |config| + if config.url_config? + config + else ActiveRecord::DatabaseConfigurations::UrlConfig.new(config.env_name, config.spec_name, url, config.config) end end diff --git a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb index c0a9f8f9ca..6372abbf3f 100644 --- a/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb +++ b/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb @@ -244,6 +244,25 @@ module ActiveRecord assert_equal expected, actual end + def test_no_url_sub_key_with_database_url_doesnt_trample_other_envs + ENV["DATABASE_URL"] = "postgres://localhost/baz" + + config = { "default_env" => { "database" => "foo" }, "other_env" => { "url" => "postgres://foohost/bardb" } } + actual = resolve_config(config) + expected = { "default_env" => + { "database" => "baz", + "adapter" => "postgresql", + "host" => "localhost" + }, + "other_env" => + { "adapter" => "postgresql", + "database" => "bardb", + "host" => "foohost" + } + } + assert_equal expected, actual + end + def test_merge_no_conflicts_with_database_url ENV["DATABASE_URL"] = "postgres://localhost/foo" -- cgit v1.2.3