From 6a68d2464e0707d92b1be8b7c642d372116d3b3b Mon Sep 17 00:00:00 2001 From: allocsys <225476909+allocsys@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:17:56 +0330 Subject: [PATCH] fix(any): rewrite ? placeholders to $N for Postgres backend (#3000) --- sqlx-core/src/any/arguments.rs | 18 ++ sqlx-core/src/arguments.rs | 15 ++ sqlx-core/src/query_builder.rs | 1 + sqlx-postgres/src/any.rs | 295 ++++++++++++++++++++++++++++++++- 4 files changed, 328 insertions(+), 1 deletion(-) diff --git a/sqlx-core/src/any/arguments.rs b/sqlx-core/src/any/arguments.rs index 59d6f4d6e0..34a7eb91df 100644 --- a/sqlx-core/src/any/arguments.rs +++ b/sqlx-core/src/any/arguments.rs @@ -10,6 +10,19 @@ use std::sync::Arc; pub struct AnyArguments { #[doc(hidden)] pub values: AnyArgumentBuffer, + + /// Byte offsets, into the query string being built by + /// [`QueryBuilder`][crate::query_builder::QueryBuilder], of each `?` placeholder written by + /// [`push_bind()`][crate::query_builder::QueryBuilder::push_bind], in the order they were + /// added. + /// + /// This is empty unless the query was built with `QueryBuilder`; e.g. a plain + /// `sqlx::query()` call with a hand-written `?` in the SQL string has no way to report + /// where that `?` is, since the string is opaque to us. Backends that can't use `?` + /// natively (namely Postgres) use this, when available, to rewrite placeholders precisely + /// instead of re-parsing the query string. + #[doc(hidden)] + pub placeholder_offsets: Vec, } impl Arguments for AnyArguments { @@ -17,6 +30,7 @@ impl Arguments for AnyArguments { fn reserve(&mut self, additional: usize, _size: usize) { self.values.0.reserve(additional); + self.placeholder_offsets.reserve(additional); } fn add<'t, T>(&mut self, value: T) -> Result<(), BoxDynError> @@ -30,6 +44,10 @@ impl Arguments for AnyArguments { fn len(&self) -> usize { self.values.0.len() } + + fn note_placeholder_offset(&mut self, offset: usize) { + self.placeholder_offsets.push(offset); + } } #[derive(Default)] diff --git a/sqlx-core/src/arguments.rs b/sqlx-core/src/arguments.rs index 7b9da60b9c..d1695d01cf 100644 --- a/sqlx-core/src/arguments.rs +++ b/sqlx-core/src/arguments.rs @@ -27,6 +27,21 @@ pub trait Arguments: Send + Sized + Default { fn format_placeholder(&self, writer: &mut W) -> fmt::Result { writer.write_str("?") } + + /// Called by [`QueryBuilder::push_bind()`][crate::query_builder::QueryBuilder::push_bind] + /// with the byte offset, into the query string being built, at which the placeholder for + /// this argument is about to be written by [`format_placeholder()`][Self::format_placeholder]. + /// + /// Most backends write an unambiguous placeholder immediately (Postgres writes `$1`, `$2`, + /// ...) and so have no need to remember where it ended up; the default implementation does + /// nothing. + /// + /// The `Any` driver overrides this: it always writes a plain `?`, since the real backend + /// isn't known yet when `QueryBuilder` is being built. Recording the exact offset of + /// each `?` lets the backend-specific driver (e.g. Postgres) rewrite them precisely once the + /// backend *is* known, instead of re-parsing the finished SQL string to guess which `?` + /// characters are placeholders as opposed to, say, part of a string literal or comment. + fn note_placeholder_offset(&mut self, _offset: usize) {} } pub trait IntoArguments: Sized + Send { diff --git a/sqlx-core/src/query_builder.rs b/sqlx-core/src/query_builder.rs index 25aa500aea..4ede8e6d1d 100644 --- a/sqlx-core/src/query_builder.rs +++ b/sqlx-core/src/query_builder.rs @@ -165,6 +165,7 @@ where arguments.add(value).expect("Failed to add argument"); let query: &mut String = Arc::get_mut(&mut self.query).expect(ERROR); + arguments.note_placeholder_offset(query.len()); arguments .format_placeholder(query) .expect("error in format_placeholder"); diff --git a/sqlx-postgres/src/any.rs b/sqlx-postgres/src/any.rs index 62b3dedbac..29fd7fd6f9 100644 --- a/sqlx-postgres/src/any.rs +++ b/sqlx-postgres/src/any.rs @@ -5,7 +5,8 @@ use crate::{ use futures_core::future::BoxFuture; use futures_core::stream::BoxStream; use futures_util::{stream, FutureExt, StreamExt, TryFutureExt, TryStreamExt}; -use sqlx_core::sql_str::SqlStr; +use sqlx_core::sql_str::{AssertSqlSafe, SqlSafeStr, SqlStr}; +use std::borrow::Cow; use std::{future, pin::pin}; use sqlx_core::any::{ @@ -22,6 +23,177 @@ use sqlx_core::transaction::TransactionManager; sqlx_core::declare_driver_with_optional_migrate!(DRIVER = Postgres); +/// Rewrite `?`-style placeholders (as produced by the `Any` driver, e.g. via `QueryBuilder`) +/// into Postgres-style positional placeholders (`$1`, `$2`, ...). +/// +/// `AnyArguments` binds its values in the same left-to-right order that `?` placeholders were +/// written, so numbering them `1..=N` in order of appearance preserves the correct binding. +/// +/// This is SQL-aware to avoid rewriting a literal `?` character that appears inside: +/// - a single-quoted string literal (`'...'`, with `''` as an escaped quote) +/// - a double-quoted identifier (`"..."`, with `""` as an escaped quote) +/// - a dollar-quoted string (`$$...$$` or `$tag$...$tag$`) +/// - a `--` line comment or a `/* */` block comment (not nested) +/// +/// Only MySQL and SQLite use `?` natively, so this rewrite is only needed on the Postgres leg +/// of the `Any` driver; see . +fn rewrite_any_placeholders(sql: &str) -> Cow<'_, str> { + if !sql.contains('?') { + return Cow::Borrowed(sql); + } + + let chars: Vec = sql.chars().collect(); + let mut out = String::with_capacity(sql.len() + 8); + let mut i = 0usize; + let mut placeholder_num = 0usize; + + while i < chars.len() { + let c = chars[i]; + match c { + '?' => { + placeholder_num += 1; + out.push('$'); + out.push_str(&placeholder_num.to_string()); + i += 1; + } + '\'' | '"' => { + // String literal or quoted identifier; the doubled-quote is the escape for both. + let quote = c; + out.push(c); + i += 1; + while i < chars.len() { + let ch = chars[i]; + out.push(ch); + i += 1; + if ch == quote { + if chars.get(i) == Some("e) { + out.push(quote); + i += 1; + continue; + } + break; + } + } + } + '-' if chars.get(i + 1) == Some(&'-') => { + // Line comment: copy through to (but not including) the newline. + while i < chars.len() && chars[i] != '\n' { + out.push(chars[i]); + i += 1; + } + } + '/' if chars.get(i + 1) == Some(&'*') => { + // Block comment (not handling nesting; Postgres nested comments are a rare + // edge case and out of scope for this fix). + out.push(chars[i]); + out.push(chars[i + 1]); + i += 2; + while i < chars.len() { + if chars[i] == '*' && chars.get(i + 1) == Some(&'/') { + out.push('*'); + out.push('/'); + i += 2; + break; + } + out.push(chars[i]); + i += 1; + } + } + '$' => { + // Possible dollar-quote opening tag: `$tag$` where `tag` is `[A-Za-z0-9_]*` + // (empty tag is the common `$$...$$` form). + let mut j = i + 1; + while j < chars.len() && (chars[j].is_alphanumeric() || chars[j] == '_') { + j += 1; + } + if j < chars.len() && chars[j] == '$' { + let tag: String = chars[i + 1..j].iter().collect(); + let open: String = chars[i..=j].iter().collect(); + out.push_str(&open); + i = j + 1; + + let close: Vec = format!("${tag}$").chars().collect(); + let mut found = false; + while i < chars.len() { + if chars[i..].starts_with(&close[..]) { + out.extend(&close); + i += close.len(); + found = true; + break; + } + out.push(chars[i]); + i += 1; + } + // If unterminated, we've already copied through to the end of input; + // nothing further to do (matches Postgres's own eventual parse error). + let _ = found; + } else { + out.push('$'); + i += 1; + } + } + _ => { + out.push(c); + i += 1; + } + } + } + + Cow::Owned(out) +} + +/// Rewrite `?`-style placeholders into `$1, $2, ...` using exact byte offsets recorded by +/// `QueryBuilder::push_bind()` (see `Arguments::note_placeholder_offset`), rather than +/// re-parsing the SQL string to find them. +/// +/// `offsets` must be given in ascending order and each one must point at the first (and only) +/// byte of a `?` placeholder in `sql`. This holds by construction, since `QueryBuilder` records +/// them in order as it writes the query left-to-right, so no quote/comment-awareness is needed +/// here: we already know unambiguously where every placeholder is. +fn rewrite_any_placeholders_by_offset(sql: &str, offsets: &[usize]) -> String { + let mut out = String::with_capacity(sql.len() + offsets.len() * 2); + let mut last = 0; + + for (i, &offset) in offsets.iter().enumerate() { + out.push_str(&sql[last..offset]); + out.push('$'); + out.push_str(&(i + 1).to_string()); + // `?` is a single ASCII byte, so this is always a valid char boundary to resume at. + last = offset + 1; + } + + out.push_str(&sql[last..]); + out +} + +/// Rewrite `?`-style placeholders (as produced by the `Any` driver, e.g. via `QueryBuilder`) +/// into Postgres-style positional placeholders (`$1`, `$2`, ...), as a [`SqlStr`], only +/// allocating a new one if a rewrite was actually needed. +/// +/// When `arguments` carries offsets recorded by `QueryBuilder::push_bind()` (see +/// [`rewrite_any_placeholders_by_offset`]), those are used directly since they're unambiguous. +/// Otherwise (e.g. a plain `sqlx::query()` call with a hand-written `?` in the SQL), we fall +/// back to [`rewrite_any_placeholders`], which parses the query to skip `?` that isn't really a +/// placeholder. +fn maybe_rewrite_any_placeholders(sql: SqlStr, arguments: Option<&AnyArguments>) -> SqlStr { + if let Some(arguments) = arguments { + let offsets = &arguments.placeholder_offsets; + + // Only trust the recorded offsets if there's exactly one per bound value; if they don't + // line up (e.g. `?` was hand-written into `QueryBuilder::push()` SQL rather than added + // via `push_bind()`), fall through to the parser instead of risking a bad rewrite. + if !offsets.is_empty() && offsets.len() == arguments.values.0.len() { + let rewritten = rewrite_any_placeholders_by_offset(sql.as_str(), offsets); + return AssertSqlSafe(rewritten).into_sql_str(); + } + } + + match rewrite_any_placeholders(sql.as_str()) { + Cow::Borrowed(_) => sql, + Cow::Owned(rewritten) => AssertSqlSafe(rewritten).into_sql_str(), + } +} + impl AnyConnectionBackend for PgConnection { fn name(&self) -> &str { ::NAME @@ -84,6 +256,7 @@ impl AnyConnectionBackend for PgConnection { persistent: bool, arguments: Option, ) -> BoxStream<'_, sqlx_core::Result>> { + let query = maybe_rewrite_any_placeholders(query, arguments.as_ref()); let persistent = persistent && arguments.is_some(); let arguments = match arguments.map(AnyArguments::convert_into).transpose() { Ok(arguments) => arguments, @@ -110,6 +283,7 @@ impl AnyConnectionBackend for PgConnection { persistent: bool, arguments: Option, ) -> BoxFuture<'_, sqlx_core::Result>> { + let query = maybe_rewrite_any_placeholders(query, arguments.as_ref()); let persistent = persistent && arguments.is_some(); let arguments = arguments .map(AnyArguments::convert_into) @@ -133,6 +307,9 @@ impl AnyConnectionBackend for PgConnection { sql: SqlStr, _parameters: &[AnyTypeInfo], ) -> BoxFuture<'c, sqlx_core::Result> { + // No bound arguments are available here (this is a prepare-only call), so we can't use + // the offset-based rewrite; fall back to parsing the SQL for hand-written `?`. + let sql = maybe_rewrite_any_placeholders(sql, None); Box::pin(async move { let statement = Executor::prepare_with(self, sql, &[]).await?; let column_names = statement.metadata.column_names.clone(); @@ -145,6 +322,8 @@ impl AnyConnectionBackend for PgConnection { &mut self, sql: SqlStr, ) -> BoxFuture<'_, sqlx_core::Result>> { + // Same reasoning as `prepare_with`: no arguments in scope, so fall back to parsing. + let sql = maybe_rewrite_any_placeholders(sql, None); Box::pin(async move { let describe = Executor::describe(self, sql).await?; @@ -252,3 +431,117 @@ fn map_result(res: PgQueryResult) -> AnyQueryResult { last_insert_id: None, } } + +#[cfg(test)] +mod tests { + use super::{rewrite_any_placeholders, rewrite_any_placeholders_by_offset}; + + #[test] + fn no_placeholders_is_borrowed_unchanged() { + let sql = "SELECT * FROM foo"; + assert!(matches!( + rewrite_any_placeholders(sql), + std::borrow::Cow::Borrowed(s) if s == sql + )); + } + + #[test] + fn simple_placeholders_are_numbered_in_order() { + assert_eq!( + rewrite_any_placeholders("SELECT * FROM foo WHERE a = ? AND b = ?"), + "SELECT * FROM foo WHERE a = $1 AND b = $2" + ); + } + + #[test] + fn limit_offset_placeholders() { + assert_eq!( + rewrite_any_placeholders("SELECT * FROM foo LIMIT ? OFFSET ?"), + "SELECT * FROM foo LIMIT $1 OFFSET $2" + ); + } + + #[test] + fn question_mark_in_string_literal_is_untouched() { + assert_eq!( + rewrite_any_placeholders("SELECT * FROM foo WHERE q = 'is this ok?' AND a = ?"), + "SELECT * FROM foo WHERE q = 'is this ok?' AND a = $1" + ); + } + + #[test] + fn escaped_quote_in_string_literal() { + assert_eq!( + rewrite_any_placeholders("SELECT 'it''s a ? test' WHERE a = ?"), + "SELECT 'it''s a ? test' WHERE a = $1" + ); + } + + #[test] + fn question_mark_in_quoted_identifier_is_untouched() { + assert_eq!( + rewrite_any_placeholders(r#"SELECT "weird?col" FROM foo WHERE a = ?"#), + r#"SELECT "weird?col" FROM foo WHERE a = $1"# + ); + } + + #[test] + fn question_mark_in_dollar_quoted_string_is_untouched() { + assert_eq!( + rewrite_any_placeholders("SELECT $$has a ? in it$$ WHERE a = ?"), + "SELECT $$has a ? in it$$ WHERE a = $1" + ); + } + + #[test] + fn question_mark_in_tagged_dollar_quoted_string_is_untouched() { + assert_eq!( + rewrite_any_placeholders("SELECT $tag$has a ? in it$tag$ WHERE a = ?"), + "SELECT $tag$has a ? in it$tag$ WHERE a = $1" + ); + } + + #[test] + fn question_mark_in_line_comment_is_untouched() { + assert_eq!( + rewrite_any_placeholders("SELECT a -- is this ok?\nWHERE a = ?"), + "SELECT a -- is this ok?\nWHERE a = $1" + ); + } + + #[test] + fn question_mark_in_block_comment_is_untouched() { + assert_eq!( + rewrite_any_placeholders("SELECT a /* ok? */ WHERE a = ?"), + "SELECT a /* ok? */ WHERE a = $1" + ); + } + + #[test] + fn offset_based_rewrite_orders_by_position() { + let sql = "SELECT * FROM foo WHERE a = ? AND b = ?"; + let offsets: Vec = sql.match_indices('?').map(|(i, _)| i).collect(); + assert_eq!( + rewrite_any_placeholders_by_offset(sql, &offsets), + "SELECT * FROM foo WHERE a = $1 AND b = $2" + ); + } + + #[test] + fn offset_based_rewrite_only_touches_given_offsets() { + // There's a `?` inside the string literal too, but since we trust the caller's + // offsets completely (no re-parsing), only the offset we pass in gets rewritten. + let sql = "SELECT * FROM foo WHERE q = 'is this ok?' AND a = ?"; + let last_offset = sql.rfind('?').unwrap(); + assert_eq!( + rewrite_any_placeholders_by_offset(sql, &[last_offset]), + "SELECT * FROM foo WHERE q = 'is this ok?' AND a = $1" + ); + } + + #[test] + fn offset_based_rewrite_no_offsets_is_unchanged() { + let sql = "SELECT * FROM foo"; + assert_eq!(rewrite_any_placeholders_by_offset(sql, &[]), sql); + } +}