sum fail on includes

[lysenko]

Sum and other calculations generate wrong sql when used with includes and called on included table.
The following line will raise sql exception.

Category.includes(:authors).sum('authours.id')

The problem was in Relation#references_eager_loaded_tables?, it checks that Relation#to_sql references eager loaded tables, but to_sql and and sql generated by Relation#sum is two different things.

[jonleighton]

Hi, thanks for the patch.

Rather than adding a column_name param to references_eager_loaded_table?, I would prefer that we pass through the exact SQL used by count. E.g., I would change the method signature to this:

def references_eager_loaded_table?(sql = to_sql)

And then change count to pass through the appropriate sql. Does that sound workable?

[jonleighton]

There's no need for assert_nothing_raised here, the test will fail if an exception is raised.

[lysenko]

And then change count to pass through the appropriate sql.
Then I should move to_sql.to_s + column_name.to_s from references_eager_loaded_table? to its caller.
I think that we should not calculate an argument for a method if we can do it inside that method.

I'll rebase and change commit according to your other comments .

[NZKoz]

I recall discussion at one point about deprecating this behavior. Parsing SQL strings is a little mad when you can just use Category.joins(:authors) and get the same result no? Perhaps we could deprecate this for 3.2 and remove in 3.3?

[lysenko]

Sure, this is mad.
But.
I have scopes with includes, if I want to sum on the scope then I should copy&paste its code and change includes to joins?
Then, how I can keep my application, not Rails Core, DRY?
Maybe I miss something?

[jonleighton]

@lysenko you said originally "to_sql and and sql generated by Relation#sum is two different things.", so I am suggesting that you pass "sql generated by Relation#sum" to references_eager_loaded_tables?.

@NZKoz I agree with you. Though we should still accept a patch for this as it is (apparently) a regression. But I am up for deprecating.

[NZKoz]

@jonleighton totally, apply the patch and all that, let's just think about how/if to clear this up for the future.

@lysenko include and joins are two different things. include means "join and also load objects from the results from the join table", that makes no sense when you're issuing a count query.

Having said that you'd probably nuke some pagination gems, but this ambiguity is pretty harmful at present.

[lysenko]

@jonleighton,
I have lack of ActiveRecord internals knowledge, seems like we should check references_eager_loaded_tables? in order to create sql for Relation#sum. So I can't pass final sql for Relation#sum as the argument for references_eager_loaded_tables?. Maybe you can find better solution.

@NZKoz,
include with sum makes a lot of sense for me, because I like to reuse my code.
For example:

def Category.cool_relation
includes(:authors) # plus many where, order, limit and other cool things.
end

Then in controller A:
@categories= Category.useful_relation.all

And in controller B:
@sum= Category.useful_relation.sum("authors.id")

Is it something bad?

[isaacsanders]

@lysenko Is this still needed?

[lysenko]

It is not needed anymore, according to @jonleighton : a2dab46