diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c0a92b65..a774d2d6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - [#1318](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1318) Reverse order of values when upserting - [#1321](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1321) Fix SQL statement to calculate `updated_at` when upserting +- [1322](https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1322) When using `FETCH` without ordering try to order using projection rather than the primary key. ## v8.0.5 diff --git a/lib/arel/visitors/sqlserver.rb b/lib/arel/visitors/sqlserver.rb index 5890b7fc3..8fcf88aa1 100644 --- a/lib/arel/visitors/sqlserver.rb +++ b/lib/arel/visitors/sqlserver.rb @@ -9,6 +9,8 @@ class SQLServer < Arel::Visitors::ToSql FETCH0 = " FETCH FIRST (SELECT 0) " ROWS_ONLY = " ROWS ONLY" + ONE_AS_ONE = ActiveRecord::FinderMethods::ONE_AS_ONE + private # SQLServer ToSql/Visitor (Overrides) @@ -300,24 +302,68 @@ def select_statement_lock? @select_statement && @select_statement.lock end + # FETCH cannot be used without an order. If no order is given, then try to use the projections for the ordering. + # If no suitable projection is present, then fallback to using the primary key of the table. def make_Fetch_Possible_And_Deterministic(o) return if o.limit.nil? && o.offset.nil? return if o.orders.any? - t = table_From_Statement o - pk = primary_Key_From_Table t - return unless pk + if any_groupings?(o) || has_non_table_join_sources?(o) + if (projection = projection_to_order_by_for_fetch(o)) + o.orders = [projection.asc] + return + end + end + + if (pk = primary_Key_From_Table(table_From_Statement(o))) + o.orders = [pk.asc] + end + end + + def any_groupings?(o) + o.cores.any? { |core| core.groups.present? } + end + + def has_non_table_join_sources?(o) + o.cores.none? { |core| core.source.left.is_a?(Arel::Table) } + end + + # Find the first projection or part of projection that can be used for ordering. Cannot use + # projections with '*' or '1 AS one' in them. + def projection_to_order_by_for_fetch(o) + o.cores.first.projections.each do |projection| + case projection + when Arel::Attributes::Attribute + return projection unless projection.name.include?("*") + when Arel::Nodes::SqlLiteral + projection.split(",").each do |p| + next if p.match?(/#{Regexp.escape(ONE_AS_ONE)}/i) || p.include?("*") - # Prefer deterministic vs a simple `(SELECT NULL)` expr. - o.orders = [pk.asc] + return Arel::Nodes::SqlLiteral.new(remove_last_AS_from_projection(p)) + end + end + end + + nil + end + + # Remove last AS from projection. Example projections: + # - 'name' + # - 'name AS first_name' + # - 'AVG(accounts.credit_limit AS DECIMAL) AS avg_credit_limit)' + def remove_last_AS_from_projection(projection) + parts = projection.split(/\sAS\s/i) + parts.pop if parts.length > 1 + parts.join(" AS ") end def distinct_One_As_One_Is_So_Not_Fetch(o) core = o.cores.first distinct = Nodes::Distinct === core.set_quantifier - oneasone = core.projections.all? { |x| x == ActiveRecord::FinderMethods::ONE_AS_ONE } - limitone = [nil, 0, 1].include? node_value(o.limit) - if distinct && oneasone && limitone && !o.offset + one_as_one = core.projections.all? { |x| x == ONE_AS_ONE } + limit_one = [nil, 0, 1].include? node_value(o.limit) + + if distinct && one_as_one && limit_one && !o.offset core.projections = [Arel.sql("TOP(1) 1 AS [one]")] o.limit = nil end diff --git a/test/cases/fetch_test_sqlserver.rb b/test/cases/fetch_test_sqlserver.rb index 2b55bd25a..51214ffe2 100755 --- a/test/cases/fetch_test_sqlserver.rb +++ b/test/cases/fetch_test_sqlserver.rb @@ -2,8 +2,11 @@ require "cases/helper_sqlserver" require "models/book" +require "models/post" class FetchTestSqlserver < ActiveRecord::TestCase + fixtures :posts + let(:books) { @books } before { create_10_books } @@ -59,6 +62,74 @@ class FetchTestSqlserver < ActiveRecord::TestCase query.to_sql end end + + it "order by the provided orderings" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[legacy_comments_count] DESC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.order(legacy_comments_count: :desc).limit(5).select(:legacy_comments_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [11, 5, 1] + end + end + + it "in subquery order by first projection if no order provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].[legacy_comments_count], [posts].[tags_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5).select(:legacy_comments_count, :tags_count)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + + it "in subquery order by primary key if no projections and order provided" do + sql = "SELECT sum(legacy_comments_count), count(*), min(legacy_comments_count) FROM (SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT @0 ROWS ONLY) subquery ORDER BY sum(legacy_comments_count) ASC OFFSET 0 ROWS FETCH NEXT @1 ROWS ONLY" + + assert_queries_match(/#{Regexp.escape(sql)}/) do + result = Post.from(Post.limit(5)).pick(Arel.sql("sum(legacy_comments_count), count(*), min(legacy_comments_count)")) + assert_equal result, [10, 5, 0] + end + end + end + + describe "GROUP query" do + it "order by primary key if not a GROUP query" do + assert_queries_match(/#{Regexp.escape("ORDER BY [posts].[id] ASC")}/i) do + Post.pick(:title) + end + end + + it "invalid SQL generated by `find_nth_with_limit` adding primary key ordering" do + error = assert_raises(ActiveRecord::StatementInvalid) do + Post.select(:title, "count(*)").group(:title).first(2) + end + assert_match(/Column "posts\.id" is invalid in the ORDER BY clause/, error.message) + end + + it "valid SQL generated if order specified" do + assert_queries_match(/#{Regexp.escape("SELECT [posts].[title], count(*) FROM [posts] GROUP BY [posts].[title] ORDER BY [posts].[title]")}/i) do + Post.select(:title, "count(*)").group(:title).order(:title).first(2) + end + end + end + + describe "LIMIT query" do + it "order by primary key if no projections" do + sql = Post.limit(5).to_sql + + assert_equal "SELECT [posts].* FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + it "order by primary key if projections and no order provided" do + sql = Post.select(:legacy_comments_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[id] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end + + it "use order provided" do + sql = Post.select(:legacy_comments_count).order(:tags_count).limit(5).to_sql + + assert_equal "SELECT [posts].[legacy_comments_count] FROM [posts] ORDER BY [posts].[tags_count] ASC OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY", sql + end end protected