From af1cb58357591149e9c9204beb5e2d5ffba8cd8d Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 23 May 2023 16:51:52 +0100 Subject: [PATCH 1/3] Fix tests (#1055) --- .github/workflows/ci.yml | 4 +- Gemfile | 3 +- appveyor.yml | 12 +++--- test/cases/coerced_tests.rb | 81 ++++++++++++++++++++++++++++++++++++- 4 files changed, 90 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c08390700..b8b658081 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,7 +13,9 @@ jobs: strategy: fail-fast: false matrix: - ruby: [2.5.9, 2.6.7, 2.7.3, 3.0.1] + ruby: + - 2.7.5 + - 3.0.3 steps: - name: Checkout code diff --git a/Gemfile b/Gemfile index e81c9033f..7e034f4ff 100644 --- a/Gemfile +++ b/Gemfile @@ -7,9 +7,10 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" } gemspec gem "bcrypt" -gem "pg", ">= 0.18.0" +gem "pg", "~> 1.3" gem "sqlite3", "~> 1.4" gem "tzinfo-data", platforms: [:mingw, :mswin, :x64_mingw, :jruby] +gem "minitest", ">= 5.15.0", "< 5.16" if ENV["RAILS_SOURCE"] gemspec path: ENV["RAILS_SOURCE"] diff --git a/appveyor.yml b/appveyor.yml index 7b4d943de..ce47d4d00 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -5,10 +5,10 @@ build: off matrix: fast_finish: true allow_failures: - - ruby_version: "25" - - ruby_version: "26" - - ruby_version: "27" - ruby_version: "27-x64" + - ruby_version: "27" + - ruby_version: "30" + - ruby_version: "30-x64" services: - mssql2014 @@ -38,9 +38,7 @@ environment: CI_AZURE_PASS: secure: cSQp8sk4urJYvq0utpsK+r7J+snJ2wpcdp8RdXJfB+w= matrix: - - ruby_version: "25-x64" - - ruby_version: "25" - - ruby_version: "26-x64" - - ruby_version: "26" - ruby_version: "27-x64" - ruby_version: "27" + - ruby_version: "30" + - ruby_version: "30-x64" diff --git a/test/cases/coerced_tests.rb b/test/cases/coerced_tests.rb index 48d886053..d51ce3cee 100644 --- a/test/cases/coerced_tests.rb +++ b/test/cases/coerced_tests.rb @@ -1088,7 +1088,8 @@ class YamlSerializationTest < ActiveRecord::TestCase coerce_tests! :test_types_of_virtual_columns_are_not_changed_on_round_trip def test_types_of_virtual_columns_are_not_changed_on_round_trip_coerced author = Author.select("authors.*, 5 as posts_count").first - dumped = YAML.load(YAML.dump(author)) + dumped_author = YAML.dump(author) + dumped = YAML.respond_to?(:unsafe_load) ? YAML.unsafe_load(dumped_author) : YAML.load(dumped_author) assert_equal 5, author.posts_count assert_equal 5, dumped.posts_count end @@ -1207,6 +1208,7 @@ def test_statement_cache_values_differ_coerced original_test_statement_cache_values_differ ensure + Book.where(author_id: nil, name: 'my book').delete_all Book.connection.add_index(:books, [:author_id, :name], unique: true) end end @@ -1399,6 +1401,7 @@ class EnumTest < ActiveRecord::TestCase send(:'original_enums are distinct per class') ensure + Book.where(author_id: nil, name: nil).delete_all Book.connection.add_index(:books, [:author_id, :name], unique: true) end @@ -1409,6 +1412,7 @@ class EnumTest < ActiveRecord::TestCase send(:'original_creating new objects with enum scopes') ensure + Book.where(author_id: nil, name: nil).delete_all Book.connection.add_index(:books, [:author_id, :name], unique: true) end @@ -1419,6 +1423,7 @@ class EnumTest < ActiveRecord::TestCase send(:'original_enums are inheritable') ensure + Book.where(author_id: nil, name: nil).delete_all Book.connection.add_index(:books, [:author_id, :name], unique: true) end @@ -1429,6 +1434,7 @@ class EnumTest < ActiveRecord::TestCase send(:'original_declare multiple enums at a time') ensure + Book.where(author_id: nil, name: nil).delete_all Book.connection.add_index(:books, [:author_id, :name], unique: true) end end @@ -1522,3 +1528,76 @@ class ReloadModelsTest < ActiveRecord::TestCase # `activesupport/lib/active_support/testing/isolation.rb` exceeds what Windows can handle. coerce_tests! :test_has_one_with_reload if RbConfig::CONFIG["host_os"] =~ /mswin|mingw/ end + +require "models/post" +class AnnotateTest < ActiveRecord::TestCase + # Same as original coerced test except our SQL starts with `EXEC sp_executesql`. + # TODO: Remove coerce after Rails 7 (see https://github.com/rails/rails/pull/42027) + coerce_tests! :test_annotate_wraps_content_in_an_inline_comment + def test_annotate_wraps_content_in_an_inline_comment_coerced + quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts") + + assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do + posts = Post.select(:id).annotate("foo") + assert posts.first + end + end + + # Same as original coerced test except our SQL starts with `EXEC sp_executesql`. + # TODO: Remove coerce after Rails 7 (see https://github.com/rails/rails/pull/42027) + coerce_tests! :test_annotate_is_sanitized + def test_annotate_is_sanitized_coerced + quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts") + + assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* /foo/ \* \*/}i) do + posts = Post.select(:id).annotate("*/foo/*") + assert posts.first + end + + assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \*\* //foo// \*\* \*/}i) do + posts = Post.select(:id).annotate("**//foo//**") + assert posts.first + end + + assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* \* //foo// \* \* \*/}i) do + posts = Post.select(:id).annotate("* *//foo//* *") + assert posts.first + end + + assert_sql(%r{SELECT #{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{SELECT #{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 +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 From 7f5245507ab70c4c999f5a5b0199f2097aa23e03 Mon Sep 17 00:00:00 2001 From: Evgeniy Demin Date: Tue, 23 May 2023 18:07:40 +0200 Subject: [PATCH 2/3] Apply monkey patches for SQL Server connections only (#933) (#1054) --- CHANGELOG.md | 6 ++++++ .../sqlserver/core_ext/attribute_methods.rb | 2 ++ .../connection_adapters/sqlserver/core_ext/calculations.rb | 4 ++++ .../connection_adapters/sqlserver/core_ext/explain.rb | 2 ++ .../sqlserver/core_ext/finder_methods.rb | 2 ++ .../connection_adapters/sqlserver/core_ext/preloader.rb | 2 ++ 6 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96913e3cc..658b1176b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## Unreleased + +#### Fixed + +[#1054](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1054) Conditionally apply SQL Server monkey patches to ActiveRecord so that it is safe to use this gem alongside other database adapters (e.g. PostgreSQL) in a multi-database Rails app + ## v6.0.2 #### Fixed diff --git a/lib/active_record/connection_adapters/sqlserver/core_ext/attribute_methods.rb b/lib/active_record/connection_adapters/sqlserver/core_ext/attribute_methods.rb index 1becf129b..af0e6a4e7 100644 --- a/lib/active_record/connection_adapters/sqlserver/core_ext/attribute_methods.rb +++ b/lib/active_record/connection_adapters/sqlserver/core_ext/attribute_methods.rb @@ -10,6 +10,8 @@ module AttributeMethods private def attributes_for_update(attribute_names) + return super unless self.class.connection.adapter_name == "SQLServer" + super.reject do |name| column = self.class.columns_hash[name] column && column.respond_to?(:is_identity?) && column.is_identity? diff --git a/lib/active_record/connection_adapters/sqlserver/core_ext/calculations.rb b/lib/active_record/connection_adapters/sqlserver/core_ext/calculations.rb index d02e6a302..254fa3c99 100644 --- a/lib/active_record/connection_adapters/sqlserver/core_ext/calculations.rb +++ b/lib/active_record/connection_adapters/sqlserver/core_ext/calculations.rb @@ -10,6 +10,8 @@ module CoreExt module Calculations # Same as original except we don't perform PostgreSQL hack that removes ordering. def calculate(operation, column_name) + return super unless klass.connection.adapter_name == "SQLServer" + if has_include?(column_name) relation = apply_join_dependency @@ -29,6 +31,8 @@ def calculate(operation, column_name) private def build_count_subquery(relation, column_name, distinct) + return super unless klass.connection.adapter_name == "SQLServer" + super(relation.unscope(:order), column_name, distinct) end diff --git a/lib/active_record/connection_adapters/sqlserver/core_ext/explain.rb b/lib/active_record/connection_adapters/sqlserver/core_ext/explain.rb index 89e1d6bb1..9a6a9a775 100644 --- a/lib/active_record/connection_adapters/sqlserver/core_ext/explain.rb +++ b/lib/active_record/connection_adapters/sqlserver/core_ext/explain.rb @@ -9,6 +9,8 @@ module Explain SQLSERVER_STATEMENT_REGEXP = /N'(.+)', N'(.+)', (.+)/ def exec_explain(queries) + return super unless connection.adapter_name == "SQLServer" + unprepared_queries = queries.map do |(sql, binds)| [unprepare_sqlserver_statement(sql, binds), binds] end diff --git a/lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb b/lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb index ccfa32620..03932d492 100644 --- a/lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb +++ b/lib/active_record/connection_adapters/sqlserver/core_ext/finder_methods.rb @@ -12,6 +12,8 @@ module FinderMethods # Same as original except we order by values in distinct select if present. def construct_relation_for_exists(conditions) + return super unless klass.connection.adapter_name == "SQLServer" + conditions = sanitize_forbidden_attributes(conditions) if distinct_value && offset_value diff --git a/lib/active_record/connection_adapters/sqlserver/core_ext/preloader.rb b/lib/active_record/connection_adapters/sqlserver/core_ext/preloader.rb index 0171f4c65..9d64c99e8 100644 --- a/lib/active_record/connection_adapters/sqlserver/core_ext/preloader.rb +++ b/lib/active_record/connection_adapters/sqlserver/core_ext/preloader.rb @@ -10,6 +10,8 @@ module Preloader private def records_for(ids) + return super unless klass.connection.adapter_name == "SQLServer" + ids.each_slice(in_clause_length).flat_map do |slice| scope.where(association_key_name => slice).load do |record| # Processing only the first owner From c52fce3f9a13a1a367198bada22149cc6d80ab0a Mon Sep 17 00:00:00 2001 From: Aidan Haran Date: Tue, 23 May 2023 17:12:03 +0100 Subject: [PATCH 3/3] Prepare for release 6.0.3 --- CHANGELOG.md | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 658b1176b..241e86ed6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## Unreleased +## v6.0.3 #### Fixed diff --git a/VERSION b/VERSION index 9b9a24420..090ea9dad 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.2 +6.0.3