Enable passing retryable SqlLiterals to #where
#54951
Conversation
| elsif rest.first.is_a?(Hash) && /:\w+/.match?(opts) | ||
| parts = [build_named_bound_sql_literal(opts, rest.first)] | ||
| elsif opts.include?("?") | ||
| parts = [build_bound_sql_literal(opts, rest)] | ||
| else | ||
| parts = [model.sanitize_sql(rest.empty? ? opts : [opts, *rest])] | ||
| parts = [Arel.sql(model.sanitize_sql([opts, *rest]))] |
There was a problem hiding this comment.
The rest.empty? check appears to have been dead code since 8e6a5de
| if Arel.arel_node?(opts) | ||
| parts = [opts] | ||
| else | ||
| parts = [Arel.sql(opts)] | ||
| end |
There was a problem hiding this comment.
Shouldn't we just do that in Arel.sql? Right now it assumes it always receives a String, hence always wrap it, but we could make it return arel nodes unchanged.
There was a problem hiding this comment.
I tried searching for usages of Arel.sql that would pass a SqlLiteral, but it seems like there aren't many (any?) others than this one (I used ❯ rg '[^rf]\.sql\([^"%]').
I think it could make sense to do if the pattern was more common, but it seems like 90+% of the time a string is passed anyways.
There was a problem hiding this comment.
For a more definitive answer, I ran ARCONN=sqlite3 bin/test with this patch:
diff --git a/activerecord/lib/arel.rb b/activerecord/lib/arel.rb
index 738e80df35..cf82e60656 100644
--- a/activerecord/lib/arel.rb
+++ b/activerecord/lib/arel.rb
@@ -50,6 +50,7 @@ module Arel
# Use this option only if the SQL is idempotent, as it could be executed
# more than once.
def self.sql(sql_string, *positional_binds, retryable: false, **named_binds)
+ raise if Nodes::SqlLiteral === sql_string
if positional_binds.empty? && named_binds.empty?
Arel::Nodes::SqlLiteral.new(sql_string, retryable: retryable)
else
It fails on main but passes on this branch, so the two places touched by this PR are the only places we pass a SqlLiteral to Arel.sql.
There was a problem hiding this comment.
I guess my point was more for the future, and for general API design. e.g. Array([]) # => [].
There was a problem hiding this comment.
I feel a bit non-specifically wary of defining Arel.sql to pass-through all Nodes (while conceding that's functionally what we do in several call-sites already)... but regardless of that, I do agree it seems reasonable/sensible for it to pass-through its own results rather than re-wrapping them.
There was a problem hiding this comment.
Updated to pass through SqlLiterals 👍 I agree passing through other Nodes probably doesn't make sense (and I think Arel.sql doesn't even accept other nodes at the moment since they can't be coerced to String).
Previously, retryable SqlLiterals passed to `#where` would lose their retryability because both `#build_where_clause` and `WhereClause` would wrap them in non-retryable `SqlLiteral`s. To fix this problem, this commit updates `#build_where_clause` to check for `SqlLiterals` and updates `WhereClause` to assume that any predicate passed in will already be wrapped. There were only a few places that passed Strings to `WhereClause` (`"1=0"` in `PredicateBuilder` and sanitized sql strings in `#build_where_clause`) that needed to be updated to use `Arel.sql`. The `WhereClause` tests also had Strings to wrap but they are really unit tests and aren't representative of what can be done with the public API
Previously, passing a SqlLiteral to Arel.sql would wrap the SqlLiteral in a new SqlLiteral (which is generally unnecessary since it allocates a new SqlLiteral/String). This commit updates Arel.sql to no longer perform the wrapping of the given "sql_string" is already a SqlLiteral.
skipkayhil
force-pushed
the
hm-where-sql-literal
branch
from
204c3f9 to
f3763e6
Compare
This builds upon the retryable SqlLiteral support introduced in rails#54951 by extending Arel.sql to accept a retryable option with bind parameters. Previously, only plain SqlLiterals could be marked as retryable, but now developers can create retryable bound SQL literals for parameterized queries. The retryable option works with both positional (?) and named (:name) bind parameters and integrates with ActiveRecord's existing retry infrastructure. Example usage: ```rb User.where(Arel.sql('name LIKE ?', search_term, retryable: true)) Post.where(Arel.sql('id = :id', retryable: true), { id: 1 }) ```
Motivation / Background
Previously, retryable SqlLiterals passed to
#wherewould lose their retryability because both#build_where_clauseandWhereClausewould wrap them in non-retryableSqlLiterals.Detail
To fix this problem, this commit updates
#build_where_clauseto check forSqlLiteralsand updatesWhereClauseto assume that any predicate passed in will already be wrapped.Additional information
There were only a few places that passed Strings to
WhereClause("1=0"inPredicateBuilderand sanitized sql strings in#build_where_clause) that needed to be updated to useArel.sql. TheWhereClausetests also had Strings to wrap but they are really unit tests and aren't representative of what can be done with the public APIChecklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]