Allow ActiveRecord::QueryMethods#select to accept a hash

[alextrueman]

Add ability to use hash with columns and aliases inside #select method.

    Post
      .joins(:comments)
      .select(
        posts: { id: :post_id, title: :post_title },
        comments: { id: :comment_id, body: :comment_body}
      )

    # instead of:

    Post
      .joins(:comments)
      .select(
        "posts.id as post_id, posts.title as post_title,
        comments.id as comment_id, comments.body as comment_body"
      )

Summary

Adds processing of #select arguments to convert hash args into strings to avoid deeper changes of #select_values

Other Information

We have very similar interface for #where method, when, for example, we using it with #joins:

Post.joins(:comments).where(comments: { author_id: 1 })

So I think this feature will be pretty obvious and understandable.

[simi]

I did one take on this already at https://github.com/rails/rails/pull/45612, but there was no interest :(.

[casperisfine]

I did one take on this already at #45612, but there was no interest :(.

This is pointing to the current PR.

[casperisfine]

I'm gonna assume you meant https://github.com/rails/rails/pull/39874

[alextrueman]

Thank you @casperisfine ! I can prepare another PR with Arel implementation ( including your comments ) as well so we can compare both, what do you think?

[casperisfine]

Sure. It might also be a good idea to add @simi as co-author on that new PR, as his original one wasn't noticed and got stale for no valid reason.

Also don't hesitate to reduce the scope of your PR, e.g. if you do a PR that only accept symbol arrays and symbol hashes, it will be simple for me to merge it. The SQL fragment feature, I won't feel comfortable making the decision alone. So it might be better to submit it as a follow up PR, and worst case a large part of the feature that cover the most common use cases would already have made it in.

[alextrueman]

@casperisfine,

Sure. It might also be a good idea to add @simi as co-author on that new PR, as his original one wasn't noticed and got stale for no valid reason.

For sure, this is what I wanted to do 😄

Also don't hesitate to reduce the scope of your PR, e.g. if you do a PR that only accept symbol arrays and symbol hashes, it will be simple for me to merge it. The SQL fragment feature, I won't feel comfortable making the decision alone. So it might be better to submit it as a follow up PR, and worst case a large part of the feature that cover the most common use cases would already have made it in.

Funny thing, but this "SQL fragment feature" was more like a "side-effect" of implementation rather then something I planned to do, I'm on refactoring with Arel so I think this side-effect will gone with this 😅

P.S. I also found that refactoring with Arel will not take a lot of changes, so I can just push it in this, what do you think?

[casperisfine]

so I can just push it in this, what do you think?

Sure, go ahead.

While we're at it, policy is only one commit per PR unless very special reasons, so you might want to squash/rebase.

[alextrueman]

@casperisfine , final question 🙇🏻

After I refactored with Arel, test for this select({ "UPPER(title)" => :title }) started failing with error [ on screenshot ] Should I left it as it is, or it's better to add some validation of field and raise custom message, like: "Use SQL functions with regular SQL strings" ? 👀

[casperisfine]

Well, if we no longer add the feature the test should be removed.

You screenshot is a bit too truncated for me to really answer your question though. I suggest you remove the test and push what you have once you are happy with it, and then I can look on my side how invalid / unexpected input is handled.

[alextrueman]

@casperisfine so I created another PR https://github.com/rails/rails/pull/45921 😅 Too much commits for rebase 🙃

[simi]

@casperisfine so I created another PR #45921 sweat_smile Too much commits for rebase upside_down_face

Why? It was enough to force push in here.

[alextrueman]

@casperisfine so I created another PR #45921 sweat_smile Too much commits for rebase upside_down_face

Why? It was enough to force push in here.

Ouch, yeah, I just forget about such option, should I push it here instead and close the new one? 👀

[simi]

@casperisfine so I created another PR #45921 sweat_smile Too much commits for rebase upside_down_face

Why? It was enough to force push in here.

Ouch, yeah, I just forget about such option, should I push it here instead and close the new one? eyes

I think it would be nice to keep it all together in here with the discussion and code together. :pray:

[casperisfine]

should I push it here instead and close the new one?

It would be best yes, this way we have the full PR discussion in case we git blame these changed lines.

started failing with error

Ok, so I looked at it, and I think we must address this. arel_column do expect a block, because if it can't map the arguments with an existing column it knows about, it yields back to the caller to provide a SQL fragment.

You can look at how other callers do it and do more or less the same.

This also suggest we should add some test that use a non-existing column so that we cover this branch.

Other than this your patch looks good to me, and I'll merge once this is done.

[alextrueman]

@casperisfine , I hope I get your idea correctly 🙇🏻

[casperisfine]

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index a0d9c3d878f..e1bc3b6e6ee 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1854,11 +1854,13 @@ def transform_select_hash_values(fields)
           case columns_aliases
           when Hash
             columns_aliases.map do |column, column_alias|
-              arel_column("#{key}.#{column}", &:itself).as(column_alias.to_s)
+              arel_column("#{key}.#{column}") do
+                predicate_builder.resolve_arel_attribute(key.to_s, column)
+              end.as(column_alias.to_s)
             end
           when Array
             columns_aliases.map do |column|
-              arel_column("#{key}.#{column}", &:itselft)
+              arel_column("#{key}.#{column}", &:itself)
             end
           when String, Symbol
             arel_column(key.to_s) do |attribute_name|
diff --git a/activerecord/test/cases/relation/select_test.rb b/activerecord/test/cases/relation/select_test.rb
index 874d4146bb7..0ca53280760 100644
--- a/activerecord/test/cases/relation/select_test.rb
+++ b/activerecord/test/cases/relation/select_test.rb
@@ -33,6 +33,15 @@ def test_select_with_not_exists_field
       end
     end
 
+    def test_select_with_invalid_nested_field
+      assert_raises(ActiveRecord::StatementInvalid) do
+        Post.select(posts: { "UPPER(title)" => :post_title }).take
+      end
+      assert_raises(ActiveRecord::StatementInvalid) do
+        Post.select(posts: ["UPPER(title)"]).take
+      end
+    end
+
     def test_select_with_hash_with_not_exists_field
       assert_raises(ActiveRecord::StatementInvalid) do
         Post.select(posts: { boo: :post_title }).take

I fixed this locally, if you apply the above change it's good for me and I'll merge.

[alextrueman]

@casperisfine, I'm done, thank you so much for your help on it 🙏🏻

[casperisfine]

Welcome. Thanks for the feature.

[simi]

Seems my original use-case is not supported. Would be additional PR of supporting raw SQL (Arel.sql) accepted?

Post.joins(:comments).select('posts.*', Arel.sql('COUNT(comments.id)') => :comment_count).load
/home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `initialize': SQLite3::SQLException: no such column: posts.COUNT(comments.id) (ActiveRecord::StatementInvalid)
        from /home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `new'
        from /home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `prepare'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:48:in `block (2 levels) in exec_query'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:953:in `block in with_raw_connection'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:926:in `with_raw_connection'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:45:in `block in exec_query'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/notifications/instrumenter.rb:58:in `instrument'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:1057:in `log'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:44:in `exec_query'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:589:in `select'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:66:in `select_all'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:110:in `select_all'
        ... 32 levels...
/home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `initialize': no such column: posts.COUNT(comments.id) (SQLite3::SQLException)
        from /home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `new'
        from /home/retro/.gem/ruby/3.1.2/gems/sqlite3-1.4.4/lib/sqlite3/database.rb:152:in `prepare'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:48:in `block (2 levels) in exec_query'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:953:in `block in with_raw_connection'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:926:in `with_raw_connection'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:45:in `block in exec_query'
        from /home/retro/code/work/oss/rails/activesupport/lib/active_support/notifications/instrumenter.rb:58:in `instrument'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:1057:in `log'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb:44:in `exec_query'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:589:in `select'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:66:in `select_all'
        from /home/retro/code/work/oss/rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:110:in `select_all'
        ... 32 levels...

[byroot]

Seems my original use-case is not supported.

Yeah, I asked to remove it because:

One part I'm a bit on the fence with is select({ "UPPER(title)" => :title }), as I fear that accepting raw SQL as hash key may lead to encourage unsafe practice with String interpolation etc.

Would be additional PR of supporting raw SQL (Arel.sql) accepted?

I think having to use Arel.sql would cover my reservation about safety, so 👍

[alextrueman]

@byroot @simi, I think I have idea how to handle it, do you mind if I will prepare PR for this as well?

[simi]

@byroot @simi, I think I have idea how to handle it, do you mind if I will prepare PR for this as well?

Sure, feel free to.

[leejarvis]

Forgive my ignorance, but shouldn't this work for aliases across multiple joins (specifically those that might point to the same table name). For example:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "pg"
end

require "active_record"
require "logger"

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "select_hash_test")
# ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title
  end

  create_table :authors, force: true do |t|
    t.string :name
  end

  create_table :post_authors, force: true do |t|
    t.belongs_to :post
    t.belongs_to :author
  end

  create_table :comments, force: true do |t|
    t.belongs_to :post
    t.belongs_to :author
    t.string :text
  end
end

class Post < ActiveRecord::Base
  has_many :post_authors
  has_many :authors, through: :post_authors

  has_many :comments
  has_many :comment_authors, through: :comments, source: :author
end

class PostAuthor < ActiveRecord::Base
  belongs_to :post
  belongs_to :author
end

class Author < ActiveRecord::Base
  has_many :post_authors
  has_many :posts, through: :post_authors
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
  belongs_to :author
end

a1 = Author.create!(name: "Author 1")
a2 = Author.create!(name: "Author 2")

post = a1.posts.create!(title: "Post 1")
post.comments.create! text: "Comment 1", author: a2
post.comments.create! text: "Reply", author: a1

p Post
  .select(authors: { name: :author_name }, comment_authors: { name: :commentor_name })
  .joins(:authors, :comment_authors).to_a

This code blows up because Rails generates the alias for comment_authors as comment_authors_posts which of course isn't obvious from the code. It works if I manually set that as the alias:

p Post
  .select(authors: { name: :author_name }, comment_authors_posts: { name: :commentor_name })
  .joins(:authors, :comment_authors).to_a

Maybe I'm missing something though? I think based on this, I would still need to generate raw SQL in the select and/or join statement in order to be sure the correct alias is used?