Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix generated projection fields in group by query #21950

Merged
merged 2 commits into from
Oct 22, 2015
Merged

Fix generated projection fields in group by query #21950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2d33c4
Fix generated projection fields in group by query
rafaelsales Oct 13, 2015
4f21d42
Refactor Calculations#execute_grouped_calculation and clean AR test case
rafaelsales Oct 13, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions activerecord/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`
Expand Down
15 changes: 5 additions & 10 deletions activerecord/lib/active_record/relation/calculations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,15 +275,10 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
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'
Expand All @@ -299,7 +294,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
]
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
Expand All @@ -308,7 +303,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
}

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)
Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/calculations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,11 @@ def test_should_count_field_in_joined_table_with_group_by
[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
Expand Down
1 change: 0 additions & 1 deletion activerecord/test/models/company.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being deleted because it was also defined in Firm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixeltrix
Not only that, but also because Account is defined as:

  create_table :accounts, force: true do |t|
    t.integer :firm_id
    t.string  :firm_name
    t.integer :credit_limit
  end

So, company.accounts fails because accounts doesn't have company_id. ITOH, firm.accounts works because accounts has firm_id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that definition originated in this commit: 99307b9 - where I don't think it was actually used. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... probably just small oversight of a pair eyes tired of hard work

scope :of_first_firm, lambda {
joins(:account => :firm).
Expand Down