feat: splice SQL fragments into a statement's sentinel comments - #1
Open
iamralch wants to merge 2 commits into
Open
feat: splice SQL fragments into a statement's sentinel comments#1iamralch wants to merge 2 commits into
iamralch wants to merge 2 commits into
Conversation
The third of the three. sqlx-cel makes a fragment out of a CEL expression, sqlx-aip makes two out of an AIP `List` request, and nothing put one in a query -- so every caller ended up with its own copy of the substitution, which is how the same off-by-one gets made twice. This is pgxquery's job, at a different moment. pgx exposes a `QueryRewriter` hook, so there the substitution happens as the query is sent and the caller never sees it. sqlx has no such hook -- `query_as` takes a string and binds positionally -- so it has to happen where the string is built, and it is a function rather than an interface. The convention is pgxquery's, unchanged, because the point of a convention is that a statement written for one project splices in the other: a block comment naming `query.<name>`, with the connective inside it. `/* query.where AND */` substitutes to `<fragment> AND`, so the author of the statement decides how a fragment joins to what surrounds it and the fragment never has to know. A sentinel whose fragment is absent is removed entirely, and a statement nobody splices runs as written, sentinels and all -- they are comments. That last property is what makes the convention safe to put in generated SQL. Two departures from pgxquery, both deliberate. A fragment with no sentinel to go into is an error rather than a no-op. The predicate would silently not apply, and a dropped predicate widens a result set rather than emptying it -- plausible rows, no error, and a bug that reaches production. The reverse is not an error: a sentinel this call says nothing about is left alone, because substituting what you were not given would be deciding the statement is wrong. And renumbering is the fallback rather than the path. `placeholder_count` tells a caller where a statement stops binding, so a producer that takes a starting offset -- sqlx-cel's `Options`, sqlx-aip's `rewrite_with` -- emits the right numbers to begin with and nothing re-reads the SQL at all. `shift` is still here for a fragment that arrived numbered from `$1` and cannot be asked to start elsewhere. Both of those read the statement with a scanner rather than a regex, which is most of the code here. A `$1` inside a string literal, a quoted identifier, a line or block comment, or a dollar-quoted body is text and not a parameter; renumbering one produces SQL that still parses and binds the wrong value. Block comments nest in PostgreSQL, so finding the next `*/` is not enough either. No dependencies, not even sqlx: this takes a `&str` and returns a `String`, and a caller splicing into a hand-written query should not acquire a driver for the privilege. sqlx is a dev-dependency, for the round trip in tests/postgres.rs -- text assertions cannot tell a query bound one slot out from a correct one, because both return rows.
`SET search_path` applies to the connection that runs it, and a pool hands out whichever connection is free -- so the `CREATE TABLE` that followed landed in `public`, where all four tests collided with each other. CI caught it; the local run could not, having no database. `PgConnectOptions::options` sets it for every connection the pool opens, which is what the sibling crate already does and what this should have copied in the first place. The `ALTER ROLE ... IN DATABASE` that was papering over it is gone with the rest: it was global, swallowed its own error, and would have leaked the setting into any other database user on the same server.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The third of the three. sqlx-cel makes a fragment out of a CEL expression, sqlx-aip makes two out of an AIP
Listrequest, and nothing put one in a query — so every caller grew its own copy of the substitution.This is pgxquery's job at a different moment: pgx has a
QueryRewriterhook, so there it happens as the query is sent. sqlx has no such hook, so it happens where the string is built, and it is a function rather than an interface.The convention is pgxquery's, unchanged — the connective lives inside the comment, so the statement decides how a fragment joins to what surrounds it, and an unspliced statement runs as written because sentinels are comments.
Two departures from pgxquery, both deliberate
A fragment with no sentinel to go into is an error. The predicate would silently not apply, and a dropped predicate widens a result set rather than emptying it — plausible rows, no error, production. The reverse is not an error: a sentinel this call says nothing about is left alone, since substituting what you weren't given is deciding the statement is wrong.
Renumbering is the fallback, not the path.
placeholder_countsays where a statement stops binding, so a producer that takes a starting offset — sqlx-cel'sOptions, sqlx-aip'srewrite_with(sqlx-contrib/sqlx-aip#9) — emits the right numbers to begin with and nothing re-reads the SQL.shiftremains for a fragment that arrived numbered from$1.What to look at
src/scan.rsis most of the code. A$1inside a string literal, a quoted identifier, a line or block comment, or a dollar-quoted body is text, not a parameter — renumbering one yields SQL that still parses and binds the wrong value. Block comments nest in PostgreSQL, so "find the next*/" is also not enough. This is the part worth reviewing closely; it's ported from pgxquery'sshiftPlaceholdersand shares its cases.&strin,Stringout; a caller splicing into a hand-written query shouldn't acquire a driver for it. sqlx is a dev-dependency for the round trip only.splicetakes a slice of(name, Option<&str>)rather than hardcodingwhere/order_byas pgxquery does, soquery.limitor anything else works without a change here.Testing
cargo fmt --check,cargo clippy --all-targets,cargo test,cargo doc --no-depsandnix flake checkare all clean: 33 tests.The 4 in
tests/postgres.rsskipped — noDATABASE_URLwhere this was written. They're the ones that check a spliced statement actually runs, which text assertions can't stand in for, so CI's devcontainer Postgres is where that claim gets tested. The harness assertsDATABASE_URLis set whenCIis, so a skip there fails rather than passing quietly.