Merge branch 'main' into issues/yellowspot/rails-6-1/coerce-test-when-we-produce-more-or-less-queries

Author: mgrunberg

lib/active_record/tasks/sqlserver_database_tasks.rb

@@ -24,7 +24,7 @@ def initialize(configuration)
 
       def create(master_established = false)
         establish_master_connection unless master_established
-        connection.create_database configuration.database, configuration_hash.merge("collation" => default_collation)
+        connection.create_database configuration.database, configuration_hash.merge(collation: default_collation)
         establish_connection configuration
       rescue ActiveRecord::StatementInvalid => e
         if /database .* already exists/i === e.message
@@ -54,14 +54,14 @@ def purge
       end
 
       def structure_dump(filename, extra_flags)
-        server_arg = "-S #{Shellwords.escape(configuration_hash['host'])}"
-        server_arg += ":#{Shellwords.escape(configuration_hash['port'])}" if configuration_hash["port"]
+        server_arg = "-S #{Shellwords.escape(configuration_hash[:host])}"
+        server_arg += ":#{Shellwords.escape(configuration_hash[:port])}" if configuration_hash[:port]
         command = [
           "defncopy-ttds",
           server_arg,
-          "-D #{Shellwords.escape(configuration_hash['database'])}",
-          "-U #{Shellwords.escape(configuration_hash['username'])}",
-          "-P #{Shellwords.escape(configuration_hash['password'])}",
+          "-D #{Shellwords.escape(configuration_hash[:database])}",
+          "-U #{Shellwords.escape(configuration_hash[:username])}",
+          "-P #{Shellwords.escape(configuration_hash[:password])}",
           "-o #{Shellwords.escape(filename)}",
         ]
         table_args = connection.tables.map { |t| Shellwords.escape(t) }
@@ -88,11 +88,11 @@ def structure_load(filename, extra_flags)
       attr_reader :configuration, :configuration_hash
 
       def default_collation
-        configuration_hash["collation"] || DEFAULT_COLLATION
+        configuration_hash[:collation] || DEFAULT_COLLATION
       end
 
       def establish_master_connection
-        establish_connection configuration_hash.merge("database" => "master")
+        establish_connection configuration_hash.merge(database: "master")
       end
     end
 

test/cases/coerced_tests.rb

@@ -1675,8 +1675,34 @@ def marshal_fixture_path(file_name)
   end
 end
 
+class NestedThroughAssociationsTest < ActiveRecord::TestCase
+  # Same as original but replace order with "order(:id)" to ensure that assert_includes_and_joins_equal doesn't raise
+  # "A column has been specified more than once in the order by list"
+  # Example: original test generate queries like "ORDER BY authors.id, [authors].[id]". We don't support duplicate columns in the order list
+  coerce_tests! :test_has_many_through_has_many_with_has_many_through_habtm_source_reflection_preload_via_joins, :test_has_many_through_has_and_belongs_to_many_with_has_many_source_reflection_preload_via_joins
+  def test_has_many_through_has_many_with_has_many_through_habtm_source_reflection_preload_via_joins_coerced
+    # preload table schemas
+    Author.joins(:category_post_comments).first
+
+    assert_includes_and_joins_equal(
+      Author.where("comments.id" => comments(:does_it_hurt).id).order(:id),
+      [authors(:david), authors(:mary)], :category_post_comments
+    )
+  end
+
+  def test_has_many_through_has_and_belongs_to_many_with_has_many_source_reflection_preload_via_joins_coerced
+    # preload table schemas
+    Category.joins(:post_comments).first
+
+    assert_includes_and_joins_equal(
+      Category.where("comments.id" => comments(:more_greetings).id).order(:id),
+      [categories(:general), categories(:technology)], :post_comments
+    )
+  end
+end
+
 class BasePreventWritesTest < ActiveRecord::TestCase
-  # We open one transaction, not two. Same as original but checking one query
+  # SQL Server does not have query for release_savepoint
   coerce_tests! %r{an empty transaction does not raise if preventing writes}
   test "an empty transaction does not raise if preventing writes coerced" do
     ActiveRecord::Base.while_preventing_writes do
@@ -1690,7 +1716,7 @@ class BasePreventWritesTest < ActiveRecord::TestCase
 end
 
 class BasePreventWritesLegacyTest < ActiveRecord::TestCase
-  # We open one transaction, not two. Same as original but checking one query
+  # SQL Server does not have query for release_savepoint
   coerce_tests! %r{an empty transaction does not raise if preventing writes}
   test "an empty transaction does not raise if preventing writes coerced" do
     ActiveRecord::Base.connection_handler.while_preventing_writes do

test/cases/order_test_sqlserver.rb

@@ -143,4 +143,11 @@ class OrderTestSQLServer < ActiveRecord::TestCase
     assert_equal post1, Post.order(Arel.sql(order)).first
     assert_equal post2, Post.order(Arel.sql(order)).second
   end
+
+  # Executing this kind of queries will raise "A column has been specified more than once in the order by list"
+  # This test shows that we don't do anything to prevent this
+  it "doesn't deduplicate semantically equal orders" do
+    sql = Post.order(:id).order("posts.id ASC").to_sql
+    assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC, posts.id ASC", sql
+  end
 end

test/cases/rake_test_sqlserver.rb

@@ -12,6 +12,7 @@ class SQLServerRakeTest < ActiveRecord::TestCase
   let(:new_database)          { "activerecord_unittest_tasks" }
   let(:default_configuration) { ARTest.test_configuration_hashes["arunit"] }
   let(:configuration)         { default_configuration.merge("database" => new_database) }
+  let(:db_config)             { ActiveRecord::Base.configurations.resolve(configuration) }
 
   before { skip "on azure" if azure_skip }
   before { disconnect! unless azure_skip }
@@ -151,7 +152,7 @@ class SQLServerRakeStructureDumpLoadTest < SQLServerRakeTest
     _(filedata).must_match %r{CREATE TABLE dbo\.users}
     db_tasks.purge(configuration)
     _(connection.tables).wont_include "users"
-    db_tasks.load_schema configuration, :sql, filename
+    db_tasks.load_schema db_config, :sql, filename
     _(connection.tables).must_include "users"
   end
 end