Fix #41417 Handle Relations with having referencing aliased selects in #include?

[smartygus]

Skips optimised #exists? query in #include? for relations that have a having clause.

Summary

Relations that have aliased select values AND a having clause that references an aliased select value would generate an error when #include? was called, due to an optimisation that would generate call #exists? on the relation instead, which effectively alters the select values of the query (and thus removes the aliased select values), but leaves the having clause intact. Because the having clause is then referencing an aliased column that is no longer present in the simplified query, an ActiveRecord::InvalidStatement error was raised.

An sample query affected by this problem:

Author.select('COUNT(*) as total_posts', 'authors.*')
      .joins(:posts)
      .group(:id)
      .having('total_posts > 2')
      .include?(Author.first)

This change adds an additional condition that skips the simplified #exists? query, which simply checks for the presence of a having clause.

Other Information

Not all Relations that have a having clause actually trigger this problem. It's specifically the ones that have aliased select values AND where the having clause references an aliased select value.

The change does however skip the optimised #exists? query for all relations that have a having clause. I attempted to find a decent way to detect when aliased select values where included in the relation, and when the having clause was referencing these aliases, and while I could actually do it for the specific example of query above, it involved effectively parsing SQL, and based on what I've read in other discussion in different issues, this is strongly discouraged. So I figured in this case maybe the pragmatic path is just to skip the optimisation introduced in 166b63e for all relations that use a having condition. This way at least the problem for this very specific type of query is resolved, even if it means that a slightly broader group of queries are exempted from the optimisation as a result.

Fixes #41417

[rafaelfranca]

Tests are broken on postgresql. Can you take a look?

[smartygus]

@rafaelfranca yeah I just saw this too and had a look into it. Based on what I can tell, using an alias in a HAVING statement is actually technically outside the SQL spec. Mysql and sqlite seem to allow it no problems, but postgresql does not. I guess it just seems to actually follow the spec more closely (info).

I'm not sure what the best course forward here is. I came across this problem because I use MySQL in one of my Rails apps that has one of these queries, and it worked fine until I updated to Rails 6.1.1. So I guess technically it's a 'regression' of sorts, but it does seem clear that this query probably never would've worked in postgresql in the first place.

[rafaelfranca]

I think we only need to skip the new test you added in the postgresql adapter.

[smartygus]

So should I just add something like:

skip if current_adapter?(:PostgreSQLAdapter)

at the beginning of the test?

Or is there a different way of doing it that is preferred?

Seems to work (2 assertions less and 1 skip for postgresql):

/usr/bin/ruby2.7 -w -I"lib:test" /var/lib/gems/2.7.0/gems/rake-13.0.3/lib/rake/rake_test_loader.rb "test/cases/finder_test.rb"
Using mysql2
Run options: --seed 20793

# Running:

............................................................................................................................................................................................................................................

Finished in 1.621330s, 145.5595 runs/s, 337.9941 assertions/s.
236 runs, 548 assertions, 0 failures, 0 errors, 0 skips
/usr/bin/ruby2.7 -w -I"lib:test" /var/lib/gems/2.7.0/gems/rake-13.0.3/lib/rake/rake_test_loader.rb "test/cases/finder_test.rb"
Using sqlite3
Run options: --seed 40604

# Running:

............................................................................................................................................................................................................................................

Finished in 2.545469s, 92.7138 runs/s, 215.2845 assertions/s.
236 runs, 548 assertions, 0 failures, 0 errors, 0 skips
/usr/bin/ruby2.7 -w -I"lib:test" /var/lib/gems/2.7.0/gems/rake-13.0.3/lib/rake/rake_test_loader.rb "test/cases/finder_test.rb"
Using postgresql
Run options: --seed 51520

# Running:

.........................................................................................................................................................................................................S..................................

Finished in 1.711387s, 137.8999 runs/s, 319.0395 assertions/s.
236 runs, 546 assertions, 0 failures, 0 errors, 1 skips

[rafaelfranca]

That is good enough.

[smartygus]

Rightio, just pushed again with the skip for postgres, hopefully the tests should pass now.

[smartygus]

Thanks heaps for the quick review and feedback! 😊