Parameterize N1QL queries for prepared-statement caching#32
Conversation
Replace string-interpolated values with positional parameter placeholders ($1, $2, ...) across n1ql, relation, has_many, and query_helper modules. This enables Couchbase prepared-statement caching by making query strings stable regardless of parameter values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Default adhoc to true in N1ql.config to preserve current behavior (queries are not cached). When set to false, Couchbase will cache the parameterized query plan, improving performance for repeated queries. The option is forwarded to Couchbase::Options::Query in both n1ql and relation query paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces parameterized queries to the Couchbase ORM library to support prepared statements, replacing inline string interpolation with positional parameters across several query-building components. The review feedback highlights a critical N1QL injection vulnerability in nested attribute updates where values are still directly interpolated. Additionally, the reviewer suggests optimizing array parameterization to enable prepared-statement caching for IN queries and adding support for ActiveSupport::TimeWithZone to ensure robust serialization of timezone-aware times.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| end.join(", ") + for_clause | ||
| else | ||
| "#{key} = #{@model.quote(value)}#{for_clause}" | ||
| "#{key} = #{@model.bind(value, params)}#{for_clause}" |
There was a problem hiding this comment.
In build_update_with_params, when value is a Hash (used for updating nested attributes), the values are directly interpolated into the query string as #{v} without being parameterized or quoted:
if value.is_a?(Hash)
value.map do |k, v|
"#{key}.#{k} = #{v}"
end.join(", ") + for_clauseThis not only defeats prepared-statement caching for nested attribute updates but also introduces a N1QL injection vulnerability if user-controlled input is passed to nested attributes.
To fix this, we should parameterize these values using @model.bind(v, params) as well:
if value.is_a?(Hash)
value.map do |k, v|
"#{key}.#{k} = #{@model.bind(v, params)}"
end.join(", ") + for_clause| def serialize_for_binding(value) | ||
| if [DateTime, Time].any? { |clazz| value.is_a?(clazz) } | ||
| value.iso8601(@precision || 0) | ||
| elsif value.is_a?(Date) | ||
| value.to_s | ||
| else | ||
| value | ||
| end | ||
| end | ||
|
|
||
| def bind(value, params) | ||
| if value.is_a?(Array) | ||
| "[#{value.map { |v| bind(v, params) }.join(', ')}]" | ||
| elsif value.nil? | ||
| nil | ||
| else | ||
| params << serialize_for_binding(value) | ||
| "$#{params.length}" | ||
| end | ||
| end |
There was a problem hiding this comment.
There are two important improvement opportunities in serialize_for_binding and bind:
-
Array Parameterization (Prepared-Statement Caching): N1QL natively supports passing arrays as a single positional parameter (e.g.,
name IN $1). The current implementation converts arrays into individual positional parameters (e.g.,name IN [$1, $2]), which produces a different query string for every different array length. This completely defeats prepared-statement caching forINqueries (which is one of the main goals of this PR). Passing the array itself as a single parameter keeps the query string identical regardless of array length, enabling prepared-statement caching. -
ActiveSupport::TimeWithZone Support:
ActiveSupport::TimeWithZone(the default time class in Rails) does not inherit fromTimeorDateTime, so timezone-aware times will not be serialized to ISO8601 strings and will fail or serialize incorrectly. Addingvalue.acts_like?(:time)ensures robust support for Rails applications.
def serialize_for_binding(value)
if value.is_a?(Array)
value.map { |v| serialize_for_binding(v) }
elsif [DateTime, Time].any? { |clazz| value.is_a?(clazz) } || (value.respond_to?(:acts_like?) && value.acts_like?(:time))
value.iso8601(@precision || 0)
elsif value.is_a?(Date)
value.to_s
else
value
end
end
def bind(value, params)
if value.nil?
nil
else
params << serialize_for_binding(value)
"$#{params.length}"
end
endParameterize Hash values in build_update_with_params using bind() instead of direct string interpolation, which was both a security vulnerability and defeated prepared-statement caching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hing Instead of expanding arrays into individual parameters (e.g. IN [$1, $2]), pass the whole array as one parameter (e.g. IN $1). This keeps the query string stable regardless of array length, enabling Couchbase to cache the prepared statement for IN queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TimeWithZone does not inherit from Time or DateTime, so it was not being serialized to ISO8601. Use acts_like?(:time) duck-typing to handle all time-like objects from Rails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
$1,$2, ...) in all N1QL query paths (n1ql,relation,has_many,query_helper), enabling Couchbase prepared-statement caching by making query strings stable regardless of parameter values.adhocoption (defaulttrue) to control whether queries use prepared-statement caching. Set tofalseto enable caching for repeated queries.Test plan
n1ql_spec,relation_spec)adhoc: falseagainst a live Couchbase cluster🤖 Generated with Claude Code