-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Support function table names in Arel Table #46864
Conversation
ffd4072
to
a37945f
Compare
@name = name.to_s | ||
@name = | ||
case name | ||
when Symbol then name.to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To preserve current behavior (covered with tests). Not sure it leaked outside of tests though, but prefer to be defensive here.
activerecord/lib/arel/table.rb
Outdated
@name = | ||
case name | ||
when Symbol then name.to_s | ||
when Nodes::Node then Arel.sql(name.to_sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to not carry an ast node around and convert it into a SQL literal right here.
Alternatively, I considered updated the SQL visitor (to_sql.rb
) and handle nodes in the #quote_table_name
method. But that would affect some other nodes, too, so I hesitated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of calling .to_sql
and wrapping it in a Arel::Nodes::SqlLiteral
is that this looses track of potential bind params inside the expression. This might be undesirable, especially because the examples you bring up are function calls with parameters.
By not being able to use bind params, the resulting query will get a different signature for every set of params and can't be as easily reused as a prepared statement.
That being said, I agree that #quote_table_name
probably isn't the best place to implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the resulting query will get a different signature for every set of params and can't be as easily reused as a prepared statement.
That's a good point (though in real life I prefer to avoid prepared statements).
Refactored to perform complication within a visitor (to_sql.rb#visit_Arel_Table
).
Make it possible to select from computed/function table using Arel, or pass arbitrary string as a table name This makes it possible to use Arel to query against functions, e.g., generate_series in PostgreSQL, or json_each, or whatever
333d739
to
6a69a29
Compare
@palkan Looks good to me! |
Looking for some feedback for this tiny but useful change /cc @rafaelfranca @matthewd |
The `table_name` method was defined in `Arel::Nodes::Table` and `Arel::Nodes::TableAlias` to get the table name, but `Arel::Nodes::Table#table_name` was removed rails/rails#46864. Therefore, it is no longer possible to simply call `#table_name` to get `table_name`, so a `TableNode.table_name` has been added to get table_name from node.
The `table_name` method was defined in `Arel::Nodes::Table` and `Arel::Nodes::TableAlias` to get the table name, but `Arel::Nodes::Table#table_name` was removed rails/rails#46864. Therefore, it is no longer possible to simply call `#table_name` to get `table_name`, so a `TableNode.table_name` has been added to get table_name from node.
The `table_name` method was defined in `Arel::Nodes::Table` and `Arel::Nodes::TableAlias` to get the table name, but `Arel::Nodes::Table#table_name` was removed rails/rails#46864. Therefore, it is no longer possible to simply call `#table_name` to get `table_name`, so a `TableNode.table_name` has been added to get table_name from node.
* Add rails 7.1.0.beta1 * Support Rails7.1 The `table_name` method was defined in `Arel::Nodes::Table` and `Arel::Nodes::TableAlias` to get the table name, but `Arel::Nodes::Table#table_name` was removed rails/rails#46864. Therefore, it is no longer possible to simply call `#table_name` to get `table_name`, so a `TableNode.table_name` has been added to get table_name from node.
The table_name method was removed in rails/rails#46864 It was an alias of `name` which is still supported, so use `name` instead.
This patch is not required in Rails >= 7.1 Close #311 Ref: - rails/rails#46864 - rails/rails@1d98bc5 [ci skip]
Motivation / Background
Make it possible to select from computed/function table using Arel, or pass arbitrary string as a table name
This makes it possible to use Arel to query against functions, e.g., generate_series in PostgreSQL, or json_each, or whatever.
Detail
My particular use-case was in transforming the following SQL to Arel (for SQLite 3):
Currently, it's not possible to use computed table sources with
Arel::Table
due to the@name = name.to_s
call.The workaround is:
With this patch, we can provide a SqlLiteral (or a custom node) right to the Table constructor:
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]