rails icon indicating copy to clipboard operation
rails copied to clipboard

Add Relation#select_append method to append to default selection.

Open simi opened this issue 1 year ago • 10 comments

:information_source: reopenning https://github.com/rails/rails/pull/39873 per @rafaelfranca request

How select works

  • when no select is used for query all columns are selected
  • when select is used only selected columns are selected

Why select is not enough?

Often I would like to just append to current selection. Currently I need to check if anything was selected before or select Arel.star on my own. But I don't actually care what was selected before, I would like to add new field everytime.

# before
User.joins(:account).select(User.arel_table[Arel.star], 'accounts.name AS account_name')
#=> SELECT "users".*, accounts.name AS account_name FROM "users" INNER JOIN "accounts" ON "accounts"."id" = "users"."account_id" LIMIT $1 


# after
User.joins(:account).select_append('accounts.name AS account_name')
#=> SELECT "users".*, accounts.name AS account_name FROM "users" INNER JOIN "accounts" ON "accounts"."id" = "users"."account_id" LIMIT $1

TODO

  • [ ] changelog entry
  • [ ] documentation

:information_source: We are using following monkey-patch in our app for some time already.

module ActiveRecord
  module SelectAppend
    extend ActiveSupport::Concern

    included do
      ActiveRecord::Querying.delegate :select_append, to: :all
    end

    def select_append(*fields)
      raise ArgumentError, "Call `select_append' with at least one field" if fields.empty?

      self.select_values = [table[Arel.star]] unless self.select_values.present?
      select(*fields)
    end
  end
end

simi avatar Aug 20 '22 20:08 simi

It looks like Sequel uses select_append or select_more and ROM also uses select_append. It may be worth using select_append just for consistency

skipkayhil avatar Aug 24 '22 22:08 skipkayhil

We also put the verb first for reverse_order, invert_where, etc. So maybe append_select, similar to append_around_action?

If we look at existing methods that start with select_, We have methods like select_all and select_rows. In all these cases select is the verb instead of the subject.

So the naming convention seems to beverb_subject

p8 avatar Aug 25 '22 16:08 p8

I'm also wondering if it would be more inline with where/rewhere and order/reorder if select would append by default (just like where and order append) and if you want to override the existing select you should use reselect. But I guess that ship has sailed 🙂

p8 avatar Aug 25 '22 17:08 p8

I'm also wondering if it would be more inline with where/rewhere and order/reorder if select would append by default (just like where and order append) and if you want to override the existing select you should use reselect. But I guess that ship has sailed 🙂

Yup, I would expect this, but IMHO select is impossible to get changed now even with proper deprecation cycle.

simi avatar Aug 25 '22 17:08 simi

IMHO select is impossible to get changed now even with proper deprecation cycle.

Not exactly impossible, but yeah it would be a mess.

I'd very much be in favor of renaming it to reselect for consistency, and make select a soft-deprecated alias, as for newcommers it's really not evident that select replaces everything.

byroot avatar Aug 25 '22 17:08 byroot

IMHO select is impossible to get changed now even with proper deprecation cycle.

Not exactly impossible, but yeah it would be a mess.

I'd very much be in favor of renaming it to reselect for consistency, and make select a soft-deprecated alias, as for newcommers it's really not evident that select replaces everything.

select actually works as expected

https://github.com/rails/rails/blob/d1a9a9928d62e550c16df6c9b7e91e0bae7f8887/activerecord/lib/active_record/relation/query_methods.rb#L322-L325

It appends to the select_values array.

The problem is that [table[Arel.star]] is not default value and thus select doesn't append to that. Instead there is check for empty select_values falling back to table[Arel.star] in build_select.

https://github.com/rails/rails/blob/d1a9a9928d62e550c16df6c9b7e91e0bae7f8887/activerecord/lib/active_record/relation/query_methods.rb#L1575-L1583

If it will be possible to make [table[Arel.star]] default value for select_values both select and reselect will work as expected. But I have no idea how to do that in backwards compatible (or somehow friendly deprecated) manner.

simi avatar Aug 25 '22 19:08 simi

Nice write up, I wasn't aware of that subtelity.

But then that select_append only "works" if called first, e.g. Post.select('something').select_append('something_else'), won't include *.

So maybe another possible API would be something like select(:*)

So that:

User.joins(:account).select_append('accounts.name AS account_name')

Becomes:

User.joins(:account).select(:*, 'accounts.name AS account_name')

Just a quick thought though.

byroot avatar Aug 25 '22 19:08 byroot

There also the somewhat related PR for implicit default_order: https://github.com/rails/rails/pull/39525 This order will be used unless someone overrides it by calling order.

select also has this "implicit default" but it's hardcoded by Rails to table[Arel.star].

p8 avatar Aug 25 '22 19:08 p8

select(:*) doesn't work since the star is getting qoted.

Anyway my usecase is simple. Somehow deep in the codebase I get relation and I have no idea what's selected and I just want to append to that selection. Using select is dangerous, since if not called before on relation, I'll get rid of * selection by mistake.

simi avatar Aug 25 '22 20:08 simi

select(:*) doesn't work since the star is getting qoted.

I'm suggesting to handle it differently in the future. AFAIK * is not a valid column name, or at least not a reasonable one, so it shouldn't have backward compatibility implications to change its behavior.

Anyway my usecase is simple

I'm not debating the merits of your use case. I even started my intervention by stating I'd like to have this feature and I wanted it many times.

I'm just not personally convinced select_append is the proper API for the various reasons I stated before (inconsistent naming, order dependent, etc).

byroot avatar Aug 25 '22 20:08 byroot