Skip to content

refactor: ask the dialect whether it is positional #6

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.

Sign up for GitHub

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

Merged
iamralch merged 1 commit into from
Sep 5, 2026
Merged

refactor: ask the dialect whether it is positional #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 2 additions & 21 deletions src/cursor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use crate::error::{Dimension, Error};
/// in each of them and is bound once — but a positional `?` has no way to point
/// backwards, so each one consumes its own bind and the value list repeats.
/// Three ordering fields bind three values on Postgres and six on SQLite, for
/// the same predicate.
/// the same predicate. [`Dialect::is_positional`] is which.
///
/// Getting this wrong does not raise an error. The binds shift by one and the
/// page silently resumes from the wrong row.
Expand Down Expand Up @@ -67,7 +67,7 @@ pub(crate) fn rewrite(
.map(|(field, value)| bind(field, value))
.collect::<Result<_, Error>>()?;

let positional = is_positional(dialect);
let positional = dialect.is_positional();
let mut repeated: Vec<Value> = Vec::new();

let mut sql = String::from("(");
Expand Down Expand Up @@ -115,25 +115,6 @@ pub(crate) fn rewrite(
Ok((Some(sql), if positional { repeated } else { keys }))
}

/// Whether `dialect` renders every placeholder alike, so that a bind cannot be
/// referenced twice.
///
/// Decided by asking the dialect rather than by naming the three built in, so a
/// caller's own [`Dialect`] is classified correctly too. It handles the awkward
/// middle case for free: a dialect emitting SQLite's numbered `?1` / `?2` form
/// is *positional in syntax but addressable*, renders the two differently, and
/// is correctly treated as numbered.
///
/// This infers a behavioural property from rendered text, which is a smell. The
/// honest fix is a `Dialect::is_positional` in sqlx-cel, defaulting to exactly
/// this comparison; until that exists, this is the only signal the trait
/// offers.
fn is_positional(dialect: &impl Dialect) -> bool {
// Two arbitrary adjacent indices. Any dialect that distinguishes parameters
// at all distinguishes these, so rendering them alike means it does not.
dialect.placeholder(1) == dialect.placeholder(2)
}

/// Converts one cursor value into the bind value it compares against.
///
/// Mechanical apart from the null: [`CursorValue`] widens sized integers to 64
Expand Down