Fix Collection cache key with limit and custom select

[fedxgibson]

Fix Collection cache key with limit and custom select (PG:AmbigousColumn: Error)

Change query to use alias name for timestamp_column to avoid ambiguity problems when using timestamp from subquery.

It fixes #33056

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[rafaelfranca]

Can you add a test to make sure the issue is fixed?

[fedxgibson]

Sure! I'll work on a test

[fedxgibson]

@rafaelfranca done!

[dillonwelch]

👍

[schneems]

Your test passes even without your patch. I pulled your patch locally, copied the test file and then checked out the commit before your patch, I then pasted in your test file. It passes

⛄  2.5.1 🚀  ~/Documents/projects/rails/activerecord ((6aa9fa482a...))
$ bundle exec rake test:sqlite3_mem TEST=test/cases/collection_cache_key_test.rb
/Users/rschneeman/.rubies/ruby-2.5.1/bin/ruby -w -I"lib:test" -I"/Users/rschneeman/.gem/ruby/2.5.1/gems/rake-12.3.1/lib" "/Users/rschneeman/.gem/ruby/2.5.1/gems/rake-12.3.1/lib/rake/rake_test_loader.rb" "test/cases/collection_cache_key_test.rb"
Using sqlite3_mem
Run options: --seed 20581

# Running:

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

Finished in 0.177709s, 112.5435 runs/s, 292.6132 assertions/s.
20 runs, 52 assertions, 0 failures, 0 errors, 0 skips

[fedxgibson]

@schneems This happens only with postgres, can you run it with postgres adapter please ?

[kamipo]

This not only happen on postgres but also on mysql2, so preferable more generic changelog entry, and please add new changelog entry to top of the file.

% ARCONN=mysql2 be ruby -w -Itest test/cases/collection_cache_key_test.rb -n test_cache_key_for_relation_with_custom_select_and_limit
Using mysql2
Run options: -n test_cache_key_for_relation_with_custom_select_and_limit --seed 6336
# Running:
E

Error:ActiveRecord::CollectionCacheKeyTest#test_cache_key_for_relation_with_custom_select_and_limit:
ActiveRecord::StatementInvalid: Mysql2::Error: Duplicate column name 'updated_at': SELECT COUNT(*) AS `size`, MAX(subquery_for_cache_key.updated_at) AS timestamp FROM (SELECT developers.*, `developers`.`updated_at` FROM `developers` WHERE `developers`.`salary` = ? ORDER BY `devel
opers`.`updated_at` DESC LIMIT ?) subquery_for_cache_key
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:116:in `prepare
'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:116:in `block i
n exec_stmt_and_free'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:601:in `block (2 levels)
 in log'
    /Users/kamipo/.rbenv/versions/2.5.1/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:600:in `block in log'
    /Users/kamipo/src/github.com/rails/rails/activesupport/lib/active_support/notifications/instrumenter.rb:23:in `instrument'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:591:in `log'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:112:in `exec_st
mt_and_free'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:39:in `exec_query'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:455:in `select'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:62:in `select_all'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:101:in `select_all'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb:12:in `select_all'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:69:in `select_one'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/collection_cache_key.rb:35:in `collection_cache_key'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation.rb:297:in `cache_key'
    test/cases/collection_cache_key_test.rb:50:in `block in <class:CollectionCacheKeyTest>'


rails test test/cases/collection_cache_key_test.rb:45

[kamipo]

Keep a blank line.

[schneems]

@fedxgibson confirm this test fails on postgres. Looks good. Please address the changes by @kamipo and I think this is good to go.

[fedxgibson]

@schneems @kamipo Everything done gents!

[kamipo]

Thanks!