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.

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

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

Conversation

@smartygus
Copy link
Contributor

@smartygus smartygus commented Feb 11, 2021

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

@smartygus smartygus force-pushed the activerecord-findermethod-include-with-having branch from 04ae412 to 1f4e053 Feb 11, 2021
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 11, 2021

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

@smartygus
Copy link
Contributor Author

@smartygus smartygus commented Feb 11, 2021

@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
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 11, 2021

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

@smartygus
Copy link
Contributor Author

@smartygus smartygus commented Feb 12, 2021

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

- skips optimised exists? query for relations that have a
  having clause

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 addition check to the condition that skips the
simplified #exists? query, which simply checks for the presence of
a having clause.
@smartygus smartygus force-pushed the activerecord-findermethod-include-with-having branch from 1f4e053 to 644d694 Feb 12, 2021
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 12, 2021

That is good enough.

@smartygus
Copy link
Contributor Author

@smartygus smartygus commented Feb 12, 2021

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

@rafaelfranca rafaelfranca merged commit afa58dc into rails:main Feb 12, 2021
2 checks passed
rafaelfranca added a commit that referenced this issue Feb 12, 2021
…clude-with-having

Fix #41417 Handle Relations with having referencing aliased selects in #include?
@smartygus
Copy link
Contributor Author

@smartygus smartygus commented Feb 12, 2021

Thanks heaps for the quick review and feedback! 😊

@smartygus smartygus deleted the activerecord-findermethod-include-with-having branch Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants