diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index bfb4e3fd54af1e5cfdd84d781a4532bb2655fcbd..7d329ef9d95b99867f1e9fc6c74b71d4a7169a1e 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix `COUNT(DISTINCT ...)` with `ORDER BY` and `LIMIT` to keep the existing select list. + + *Ryuta Kamizono* + * Fix `unscoped(where: [columns])` removing the wrong bind values When the `where` is called on a relation after a `or`, unscoping the column of that later `where`, it removed diff --git a/activerecord/lib/active_record/collection_cache_key.rb b/activerecord/lib/active_record/collection_cache_key.rb index 1e1de1863ac062805b47df290589fc827ca9373b..c1594ca5e393b59c80482d9316ff56feed31c0a8 100644 --- a/activerecord/lib/active_record/collection_cache_key.rb +++ b/activerecord/lib/active_record/collection_cache_key.rb @@ -14,7 +14,7 @@ def collection_cache_key(collection = all, timestamp_column = :updated_at) # :no column = "#{connection.quote_table_name(collection.table_name)}.#{connection.quote_column_name(timestamp_column)}" select_values = "COUNT(*) AS #{connection.quote_column_name("size")}, MAX(%s) AS timestamp" - if collection.limit_value || collection.offset_value + if collection.has_limit_or_offset? query = collection.spawn query.select_values = [column] subquery_alias = "subquery_for_cache_key" diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 56a3b18fbd388384f0e5b978b5a54f0014263bf2..af40b3ac66b8cf9251898536af555512cef28b3d 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -646,6 +646,10 @@ def empty_scope? # :nodoc: @values == klass.unscoped.values end + def has_limit_or_offset? # :nodoc: + limit_value || offset_value + end + protected def load_records(records) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 9cabd1af13667bbb6482e42e24a58c4c6357b329..782ea7c13ab84362286601a316803baa803811df 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -111,7 +111,7 @@ def sum(column_name = nil) def calculate(operation, column_name) if has_include?(column_name) relation = construct_relation_for_association_calculations - relation = relation.distinct if operation.to_s.downcase == "count" + relation.distinct! if operation.to_s.downcase == "count" relation.calculate(operation, column_name) else @@ -194,8 +194,13 @@ def perform_calculation(operation, column_name) if operation == "count" column_name ||= select_for_count - column_name = primary_key if column_name == :all && distinct - distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i + if column_name == :all + if distinct && !(has_limit_or_offset? && order_values.any?) + column_name = primary_key + end + elsif column_name =~ /\s*DISTINCT[\s(]+/i + distinct = nil + end end if group_values.any? @@ -222,7 +227,7 @@ def operation_over_aggregate_column(column, operation, distinct) def execute_simple_calculation(operation, column_name, distinct) #:nodoc: column_alias = column_name - if operation == "count" && (limit_value || offset_value) + if operation == "count" && has_limit_or_offset? # Shortcut when limit is zero. return 0 if limit_value == 0 @@ -361,16 +366,19 @@ def select_for_count end def build_count_subquery(relation, column_name, distinct) - column_alias = Arel.sql("count_column") - subquery_alias = Arel.sql("subquery_for_count") + relation.select_values = [ + if column_name == :all + distinct ? table[Arel.star] : Arel.sql("1") + else + column_alias = Arel.sql("count_column") + aggregate_column(column_name).as(column_alias) + end + ] - aliased_column = aggregate_column(column_name == :all ? 1 : column_name).as(column_alias) - relation.select_values = [aliased_column] - subquery = relation.arel.as(subquery_alias) + subquery = relation.arel.as(Arel.sql("subquery_for_count")) + select_value = operation_over_aggregate_column(column_alias || Arel.star, "count", false) - sm = Arel::SelectManager.new relation.engine - select_value = operation_over_aggregate_column(column_alias, "count", distinct) - sm.project(select_value).from(subquery) + Arel::SelectManager.new(subquery).project(select_value) end end end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 3214d778d4bb1a8bbbf2a6015f6c5cb0dd8611ff..99b99d05149a5c9bf9773c4561ac99b54b108894 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -241,6 +241,30 @@ def test_apply_distinct_in_count end end + def test_distinct_count_with_order_and_limit + assert_equal 4, Account.distinct.order(:firm_id).limit(4).count + end + + def test_distinct_count_with_order_and_offset + assert_equal 4, Account.distinct.order(:firm_id).offset(2).count + end + + def test_distinct_count_with_order_and_limit_and_offset + assert_equal 4, Account.distinct.order(:firm_id).limit(4).offset(2).count + end + + def test_distinct_joins_count_with_order_and_limit + assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).limit(3).count + end + + def test_distinct_joins_count_with_order_and_offset + assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).offset(2).count + end + + def test_distinct_joins_count_with_order_and_limit_and_offset + assert_equal 3, Account.joins(:firm).distinct.order(:firm_id).limit(3).offset(2).count + end + def test_should_group_by_summed_field_having_condition c = Account.group(:firm_id).having("sum(credit_limit) > 50").sum(:credit_limit) assert_nil c[1] diff --git a/activerecord/test/models/account.rb b/activerecord/test/models/account.rb new file mode 100644 index 0000000000000000000000000000000000000000..0c3cd45a81abc32e230da076c09e57502a54d903 --- /dev/null +++ b/activerecord/test/models/account.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class Account < ActiveRecord::Base + belongs_to :firm, class_name: "Company" + belongs_to :unautosaved_firm, foreign_key: "firm_id", class_name: "Firm", autosave: false + + alias_attribute :available_credit, :credit_limit + + def self.destroyed_account_ids + @destroyed_account_ids ||= Hash.new { |h, k| h[k] = [] } + end + + # Test private kernel method through collection proxy using has_many. + def self.open + where("firm_name = ?", "37signals") + end + + before_destroy do |account| + if account.firm + Account.destroyed_account_ids[account.firm.id] << account.id + end + end + + validate :check_empty_credit_limit + + private + def check_empty_credit_limit + errors.add("credit_limit", :blank) if credit_limit.blank? + end + + def private_method + "Sir, yes sir!" + end +end diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 20e37710e7790a8b833033392d80946bea86e013..72038871d6c48ec1b6cf45cc4057183b720d4801 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -192,37 +192,4 @@ class SpecialClient < Client class VerySpecialClient < SpecialClient end -class Account < ActiveRecord::Base - belongs_to :firm, class_name: "Company" - belongs_to :unautosaved_firm, foreign_key: "firm_id", class_name: "Firm", autosave: false - - alias_attribute :available_credit, :credit_limit - - def self.destroyed_account_ids - @destroyed_account_ids ||= Hash.new { |h, k| h[k] = [] } - end - - # Test private kernel method through collection proxy using has_many. - def self.open - where("firm_name = ?", "37signals") - end - - before_destroy do |account| - if account.firm - Account.destroyed_account_ids[account.firm.id] << account.id - end - true - end - - validate :check_empty_credit_limit - - private - - def check_empty_credit_limit - errors.add("credit_limit", :blank) if credit_limit.blank? - end - - def private_method - "Sir, yes sir!" - end -end +require "models/account"