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

Already on GitHub? Sign in to your account

handle includes/join correctly for calculation #6007

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

aderyabin commented Apr 27, 2012

This problem very detailed described at this issue.

Also in progress of work with this issue I find that something like this returns error

Company.create!(:name => "test", :contracts => [Contract.new(:developer_id => 7)])
Company.includes(:contracts).maximum(:developer_id)

This bug fixed at second commit in this pull request

@jonleighton jonleighton commented on the diff May 5, 2012

activerecord/lib/active_record/relation/calculations.rb
@@ -125,7 +126,12 @@ def pluck(column_name)
column_name = "#{table_name}.#{column_name}"
end
- result = klass.connection.select_all(select(column_name).arel, nil, bind_values)
+ if eager_loading? || (includes_values.present? && (column_name || references_eager_loaded_tables?))
+ return construct_relation_for_association_calculations.pluck(column_name)
+ end
+
+ result = @klass.connection.select_all(select(column_name).arel, nil, bind_values)
@jonleighton

jonleighton May 5, 2012

Member

why have you change klass to @klass?

Member

jonleighton commented May 5, 2012

I think the code in the association #ids_reader method is now unnecessary after this. Can you look at removing it please?

Member

jonleighton commented May 5, 2012

Also, please squash the 'merge' and 'refactoring' commits and rebase.

Member

jonleighton commented May 5, 2012

Please see #6086

ahawkins commented May 7, 2012

@aderyabin I've closed #6086. Do you need help with this?

@ghost ghost assigned rafaelfranca Jun 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment