Merge pull request #21950 from rafaelsales/issues/21922-fix-ar-group-by-attribute-lookup

Fix generated projection fields in group by query

4 files changed, 17 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 258b2be899..e1f543c3a1 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Queries such as `Computer.joins(:monitor).group(:status).count` will now be
+    interpreted as  `Computer.joins(:monitor).group('computers.status').count`
+    so that when `Computer` and `Monitor` have both `status` columns we don't
+    have conflicts in projection.
+
+    *Rafael Sales*
+
 *   Add ability to default to `uuid` as primary key when generating database migrations
 
     Set `Rails.application.config.active_record.primary_key = :uuid`
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 8b9367acc7..f45844a9ea 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -275,15 +275,10 @@ module ActiveRecord
       else
         group_fields = group_attrs
       end
+      group_fields = arel_columns(group_fields)
 
-      group_aliases = group_fields.map { |field|
-        column_alias_for(field)
-      }
-      group_columns = group_aliases.zip(group_fields).map { |aliaz,field|
-        [aliaz, field]
-      }
-
-      group = group_fields
+      group_aliases = group_fields.map { |field| column_alias_for(field) }
+      group_columns = group_aliases.zip(group_fields)
 
       if operation == 'count' && column_name == :all
         aggregate_alias = 'count_all'
@@ -299,7 +294,7 @@ module ActiveRecord
       ]
       select_values += select_values unless having_clause.empty?
 
-      select_values.concat arel_columns(group_fields).zip(group_aliases).map { |field,aliaz|
+      select_values.concat group_columns.map { |aliaz, field|
         if field.respond_to?(:as)
           field.as(aliaz)
         else
@@ -308,7 +303,7 @@ module ActiveRecord
       }
 
       relation = except(:group)
-      relation.group_values  = group
+      relation.group_values  = group_fields
       relation.select_values = select_values
 
       calculated_data = @klass.connection.select_all(relation, nil, relation.bound_attributes)
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index a0e4b4971b..4a0e6f497f 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -455,6 +455,11 @@ class CalculationsTest < ActiveRecord::TestCase
     [1,6,2,9].each { |firm_id| assert c.keys.include?(firm_id) }
   end
 
+  def test_should_count_field_of_root_table_with_conflicting_group_by_column
+    assert_equal({ 1 => 1 }, Firm.joins(:accounts).group(:firm_id).count)
+    assert_equal({ 1 => 1 }, Firm.joins(:accounts).group('accounts.firm_id').count)
+  end
+
   def test_count_with_no_parameters_isnt_deprecated
     assert_not_deprecated { Account.count }
   end
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index a96b8ef0f2..1dcd9fc21e 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -10,7 +10,6 @@ class Company < AbstractCompany
   has_one :dummy_account, :foreign_key => "firm_id", :class_name => "Account"
   has_many :contracts
   has_many :developers, :through => :contracts
-  has_many :accounts
 
   scope :of_first_firm, lambda {
     joins(:account => :firm).