Make `.select()` query method accept a hash option for stating field selection on joined tables. #10169

Open
wants to merge 1 commit into
from

4 participants

@Nerian

Hi,

The .select query method accepts multiple arguments. I added that when one of those arguments is a Hash, the key would be understood as the name of a table and each of its values as fields of said table.

Example:

User.joins(:posts).select(:name, posts: [:id, :title, :body])
#   => SELECT name, posts.id, posts.title, posts.body FROM "users" INNER JOIN "posts" ON "posts"."user_id" = "users"."id"

Let me know how can I help you to get this merged.

Thanks.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 10, 2013
activerecord/test/cases/relations_test.rb
@@ -237,6 +237,11 @@ def test_select_with_block
assert_equal [2, 4, 6, 8, 10], even_ids.sort
end
+ def test_select_with_hash
+ ids = authors(:david).posts.select(posts: [:id]).map(&:id)
@carlosantoniodasilva
Ruby on Rails member

I think you could use joins here instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Apr 10, 2013
activerecord/lib/active_record/relation/query_methods.rb
@@ -215,9 +220,17 @@ def references!(*args) # :nodoc:
def select(*fields)
if block_given?
to_a.select { |*block_args| yield(*block_args) }
- else
- raise ArgumentError, 'Call this with at least one field' if fields.empty?
+ elsif fields.present?
+ fields.each_index do |index|
+ if fields[index].is_a?(Hash)
+ fields[index] = fields[index].each.collect do |namespace, field_names|
+ field_names.map{|field_name| "#{namespace}.#{field_name}"} unless field_names.is_a?(String)
@carlosantoniodasilva
Ruby on Rails member

Why the string check? It doesn't seem to be covered by a test.

@Nerian
Nerian added a line comment Apr 11, 2013

If I remove the check, this test would fail:

https://github.com/rails/rails/blob/master/activerecord/test/cases/scoping/default_scoping_test.rb#L169

With this error:

DefaultScopingTest#test_unscope_select:
NoMethodError: undefined method `map' for "Jamis":String
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:233:in `block (2 levels) in select!'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:232:in `each'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:232:in `map'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:232:in `block in select!'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:230:in `each_index'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:230:in `select!'
    /vagrant/rails/activerecord/lib/active_record/relation/query_methods.rb:225:in `select'
    test/cases/scoping/default_scoping_test.rb:169:in `test_unscope_select'

Although, perhaps the check should be verifying that we have an Array, instead that it is not a String. I will change that too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 10, 2013
activerecord/lib/active_record/relation/query_methods.rb
@@ -215,9 +220,17 @@ def references!(*args) # :nodoc:
def select(*fields)
if block_given?
to_a.select { |*block_args| yield(*block_args) }
- else
- raise ArgumentError, 'Call this with at least one field' if fields.empty?
+ elsif fields.present?
+ fields.each_index do |index|
+ if fields[index].is_a?(Hash)
+ fields[index] = fields[index].each.collect do |namespace, field_names|
+ field_names.map{|field_name| "#{namespace}.#{field_name}"} unless field_names.is_a?(String)
+ end
+ end
+ end
@carlosantoniodasilva
Ruby on Rails member

I'd say this logic should be moved to select! instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Apr 10, 2013
activerecord/lib/active_record/relation/query_methods.rb
@@ -215,9 +220,17 @@ def references!(*args) # :nodoc:
def select(*fields)
if block_given?
to_a.select { |*block_args| yield(*block_args) }
- else
- raise ArgumentError, 'Call this with at least one field' if fields.empty?
+ elsif fields.present?
+ fields.each_index do |index|
+ if fields[index].is_a?(Hash)
+ fields[index] = fields[index].each.collect do |namespace, field_names|
@carlosantoniodasilva
Ruby on Rails member

Just use map, no need for each.collect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

I kinda buy this idea, but lets wait for some more feedback. Meanwhile I made a few comments, it's also gonna need a changelog entry. Thanks!

@Nerian

Thanks for the feedback 😄 . I implemented your ideas. Let me know if anything else needs to be changed.

Thanks!

@dasch dasch and 1 other commented on an outdated diff Apr 15, 2013
activerecord/lib/active_record/relation/query_methods.rb
spawn.select!(*fields)
end
end
def select!(*fields) # :nodoc:
+ fields.each_index do |index|
+ if fields[index].is_a?(Hash)
+ fields[index] = fields[index].map do |namespace, field_names|
+ field_names.map{ |field_name| "#{namespace}.#{field_name}" } if field_names.is_a?(Array)
@dasch
dasch added a line comment Apr 15, 2013

If field_names is not an Array, this will replace the entries with nil values – perhaps fail loudly instead?

@Nerian
Nerian added a line comment Apr 15, 2013

Agree.

Fixed at: Nerian@6da0db8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Nerian

Is there anything else I need to do?

@crodjer

This will fail for projects which specify a table prefix. To take care of that, this should work:

namespace.to_s.singularize.camelize.constantize.table_name
@Nerian Nerian The `.select` query method accepts multiple arguments. When one of th…
…ose arguments is a Hash, the key would be understood as the name of a table and each of its values as fields of said table.

Example:

User.joins(:posts).select(:name, posts: [:id, :title, :body])
=> SELECT name, posts.id, posts.title, posts.body FROM "users" INNER JOIN "posts" ON "posts"."user_id" = "users"."id"
e3d7dab
@Nerian

@crodjer Thanks for catching that! It has been added to the code.

@ccutrer ccutrer commented on the diff Jan 22, 2014
activerecord/lib/active_record/relation/query_methods.rb
spawn.select!(*fields)
end
end
def select!(*fields) # :nodoc:
+ fields.each_index do |index|
+ if fields[index].is_a?(Hash)
+ fields[index] = fields[index].map do |namespace, field_names|
+ raise ArgumentError.new("The value of #{namespace} must be an Array, but was #{field_names.class}") unless field_names.is_a?(Array)
+ field_names.map{ |field_name| "#{namespace.to_s.singularize.camelize.constantize.table_name}.#{field_name}" }
@ccutrer
ccutrer added a line comment Jan 22, 2014

I'd suggest looking at PredicateBuilder.build_from_hash and making the logic the same as there. That way the behavior of column resolution is the same between #select and #where

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

Related with #13760

@rafaelfranca rafaelfranca self-assigned this Feb 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment