diff options
-rw-r--r-- | actionview/lib/action_view/helpers/form_helper.rb | 2 | ||||
-rw-r--r-- | activemodel/CHANGELOG.md | 4 | ||||
-rw-r--r-- | activemodel/lib/active_model/serialization.rb | 2 | ||||
-rw-r--r-- | activemodel/test/cases/serialization_test.rb | 7 | ||||
-rw-r--r-- | activerecord/CHANGELOG.md | 8 | ||||
-rw-r--r-- | activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb | 2 | ||||
-rw-r--r-- | activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb | 2 | ||||
-rw-r--r-- | activerecord/lib/active_record/migration.rb | 10 | ||||
-rw-r--r-- | activerecord/lib/active_record/relation/calculations.rb | 2 | ||||
-rw-r--r-- | activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb | 10 | ||||
-rw-r--r-- | activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb | 20 | ||||
-rw-r--r-- | activerecord/test/cases/calculations_test.rb | 6 | ||||
-rw-r--r-- | activerecord/test/cases/finder_test.rb | 5 | ||||
-rw-r--r-- | activerecord/test/cases/migration_test.rb | 19 | ||||
-rw-r--r-- | activerecord/test/cases/relations_test.rb | 6 | ||||
-rw-r--r-- | guides/source/initialization.md | 2 |
16 files changed, 84 insertions, 23 deletions
diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb index 65143efabb..6fd66066c6 100644 --- a/actionview/lib/action_view/helpers/form_helper.rb +++ b/actionview/lib/action_view/helpers/form_helper.rb @@ -2246,7 +2246,7 @@ module ActionView @template.button_tag(value, options, &block) end - def emitted_hidden_id? + def emitted_hidden_id? # :nodoc: @emitted_hidden_id ||= nil end diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md index b28c83e4ed..8a3578229a 100644 --- a/activemodel/CHANGELOG.md +++ b/activemodel/CHANGELOG.md @@ -1,5 +1,9 @@ ## Rails 6.0.0.alpha (Unreleased) ## +* Do not lose all multiple `:includes` with options in serialization. + + *Mike Mangino* + * Rails 6 requires Ruby 2.4.1 or newer. *Jeremy Daer* diff --git a/activemodel/lib/active_model/serialization.rb b/activemodel/lib/active_model/serialization.rb index 47cb81bee5..c4b7b32291 100644 --- a/activemodel/lib/active_model/serialization.rb +++ b/activemodel/lib/active_model/serialization.rb @@ -179,7 +179,7 @@ module ActiveModel return unless includes = options[:include] unless includes.is_a?(Hash) - includes = Hash[Array(includes).map { |n| n.is_a?(Hash) ? n.to_a.first : [n, {}] }] + includes = Hash[Array(includes).flat_map { |n| n.is_a?(Hash) ? n.to_a : [[n, {}]] }] end includes.each do |association, opts| diff --git a/activemodel/test/cases/serialization_test.rb b/activemodel/test/cases/serialization_test.rb index 9002982e7f..6826d2bbd1 100644 --- a/activemodel/test/cases/serialization_test.rb +++ b/activemodel/test/cases/serialization_test.rb @@ -174,4 +174,11 @@ class SerializationTest < ActiveModel::TestCase { "name" => "Sue", "email" => "sue@example.com", "gender" => "female" }] } assert_equal expected, @user.serializable_hash(include: [{ address: { only: "street" } }, :friends]) end + + def test_all_includes_with_options + expected = { "email" => "david@example.com", "gender" => "male", "name" => "David", + "address" => { "street" => "123 Lane" }, + "friends" => [{ "name" => "Joe" }, { "name" => "Sue" }] } + assert_equal expected, @user.serializable_hash(include: [address: { only: "street" }, friends: { only: "name" }]) + end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 36bb5e1daf..92c20ec79f 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,13 @@ ## Rails 6.0.0.alpha (Unreleased) ## +* Fix `#columsn_for_distinct` of MySQL and PostgreSQL to make + `ActiveRecord::FinderMethods#limited_ids_for` use correct primary key values + even if `ORDER BY` columns include other table's primary key. + + Fixes #28364. + + *Takumi Kagiyama* + * Make `reflection.klass` raise if `polymorphic?` not to be misused. Fixes #31876. 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 608258d05c..fbedddd7f9 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -513,7 +513,7 @@ module ActiveRecord s.gsub(/\s+(?:ASC|DESC)\b/i, "") }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" } - [super, *order_columns].join(", ") + (order_columns << super).join(", ") end def strict_mode? diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 6f3db772cd..45b230f0f9 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -583,7 +583,7 @@ module ActiveRecord .gsub(/\s+NULLS\s+(?:FIRST|LAST)\b/i, "") }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" } - [super, *order_columns].join(", ") + (order_columns << super).join(", ") end def update_table_definition(table_name, base) # :nodoc: diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 5322571820..663b3c590a 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -140,6 +140,7 @@ module ActiveRecord class ConcurrentMigrationError < MigrationError #:nodoc: DEFAULT_MESSAGE = "Cannot run migrations because another migration process is currently running.".freeze + RELEASE_LOCK_FAILED_MESSAGE = "Failed to release advisory lock".freeze def initialize(message = DEFAULT_MESSAGE) super @@ -1355,12 +1356,17 @@ module ActiveRecord def with_advisory_lock lock_id = generate_migrator_advisory_lock_id - got_lock = Base.connection.get_advisory_lock(lock_id) + connection = Base.connection + got_lock = connection.get_advisory_lock(lock_id) raise ConcurrentMigrationError unless got_lock load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock yield ensure - Base.connection.release_advisory_lock(lock_id) if got_lock + if got_lock && !connection.release_advisory_lock(lock_id) + raise ConcurrentMigrationError.new( + ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE + ) + end end MIGRATOR_SALT = 2053462845 diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 6a63eb70b1..f215c95f51 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -132,7 +132,7 @@ module ActiveRecord if has_include?(column_name) relation = apply_join_dependency - if operation.to_s.downcase == "count" && !distinct_value + if operation.to_s.downcase == "count" relation.distinct! # PostgreSQL: ORDER BY expressions must appear in SELECT list when using DISTINCT if (column_name == :all || column_name.nil?) && select_values.empty? diff --git a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb index d18fb97e05..0719baaa23 100644 --- a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb +++ b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb @@ -25,25 +25,25 @@ class Mysql2AdapterTest < ActiveRecord::Mysql2TestCase end def test_columns_for_distinct_one_order - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @conn.columns_for_distinct("posts.id", ["posts.created_at desc"]) end def test_columns_for_distinct_few_orders - assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1", + assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id", @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"]) end def test_columns_for_distinct_with_case assert_equal( - "posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0", + "CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id", @conn.columns_for_distinct("posts.id", ["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"]) ) end def test_columns_for_distinct_blank_not_nil_orders - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "", " "]) end @@ -52,7 +52,7 @@ class Mysql2AdapterTest < ActiveRecord::Mysql2TestCase def order.to_sql "posts.created_at desc" end - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @conn.columns_for_distinct("posts.id", [order]) end diff --git a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb index 1951230c8a..cbb6cd42b5 100644 --- a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb +++ b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb @@ -263,25 +263,25 @@ module ActiveRecord end def test_columns_for_distinct_one_order - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @connection.columns_for_distinct("posts.id", ["posts.created_at desc"]) end def test_columns_for_distinct_few_orders - assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1", + assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id", @connection.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"]) end def test_columns_for_distinct_with_case assert_equal( - "posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0", + "CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id", @connection.columns_for_distinct("posts.id", ["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"]) ) end def test_columns_for_distinct_blank_not_nil_orders - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @connection.columns_for_distinct("posts.id", ["posts.created_at desc", "", " "]) end @@ -290,23 +290,23 @@ module ActiveRecord def order.to_sql "posts.created_at desc" end - assert_equal "posts.id, posts.created_at AS alias_0", + assert_equal "posts.created_at AS alias_0, posts.id", @connection.columns_for_distinct("posts.id", [order]) end def test_columns_for_distinct_with_nulls - assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"]) - assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"]) + assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"]) + assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"]) end def test_columns_for_distinct_without_order_specifiers - assert_equal "posts.title, posts.updater_id AS alias_0", + assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id"]) - assert_equal "posts.title, posts.updater_id AS alias_0", + assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id nulls last"]) - assert_equal "posts.title, posts.updater_id AS alias_0", + assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id nulls first"]) end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 0828d6b58f..74228b2796 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -242,6 +242,12 @@ class CalculationsTest < ActiveRecord::TestCase assert_queries(1) { assert_equal 11, posts.count(:all) } end + def test_count_with_eager_loading_and_custom_order_and_distinct + posts = Post.includes(:comments).order("comments.id").distinct + assert_queries(1) { assert_equal 11, posts.count } + assert_queries(1) { assert_equal 11, posts.count(:all) } + end + def test_distinct_count_all_with_custom_select_and_order accounts = Account.distinct.select("credit_limit % 10").order(Arel.sql("credit_limit % 10")) assert_queries(1) { assert_equal 3, accounts.count(:all) } diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index ebddf81449..d85910d9c6 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1189,6 +1189,11 @@ class FinderTest < ActiveRecord::TestCase order("author_addresses_authors.id DESC").limit(3).to_a.size end + def test_find_with_eager_loading_collection_and_ordering_by_collection_primary_key + assert_equal Post.first, Post.eager_load(comments: :ratings). + order("posts.id, ratings.id, comments.id").first + end + def test_find_with_nil_inside_set_passed_for_one_attribute client_of = Company. where(client_of: [2, 1, nil], diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index a4b8664a85..c241ddc807 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -693,6 +693,25 @@ class MigrationTest < ActiveRecord::TestCase assert_no_column Person, :last_name, "without an advisory lock, the Migrator should not make any changes, but it did." end + + def test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock + migration = Class.new(ActiveRecord::Migration::Current).new + migrator = ActiveRecord::Migrator.new(:up, [migration], 100) + lock_id = migrator.send(:generate_migrator_advisory_lock_id) + + e = assert_raises(ActiveRecord::ConcurrentMigrationError) do + silence_stream($stderr) do + migrator.send(:with_advisory_lock) do + ActiveRecord::Base.connection.release_advisory_lock(lock_id) + end + end + end + + assert_match( + /#{ActiveRecord::ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE}/, + e.message + ) + end end private diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 18e0bed5b6..cf65789b97 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -976,6 +976,12 @@ class RelationTest < ActiveRecord::TestCase assert_queries(1) { assert_equal 11, posts.load.size } end + def test_size_with_eager_loading_and_custom_order_and_distinct + posts = Post.includes(:comments).order("comments.id").distinct + assert_queries(1) { assert_equal 11, posts.size } + assert_queries(1) { assert_equal 11, posts.load.size } + end + def test_update_all_with_scope tag = Tag.first Post.tagged_with(tag.id).update_all title: "rofl" diff --git a/guides/source/initialization.md b/guides/source/initialization.md index c4f1df487b..d3b122c7fe 100644 --- a/guides/source/initialization.md +++ b/guides/source/initialization.md @@ -532,12 +532,12 @@ require "rails" %w( active_record/railtie + active_storage/engine action_controller/railtie action_view/railtie action_mailer/railtie active_job/railtie action_cable/engine - active_storage/engine rails/test_unit/railtie sprockets/railtie ).each do |railtie| |