feat!: render through a QueryRenderer, and stop calling it a rewrite - #9
Open
iamralch wants to merge 2 commits into
Open
feat!: render through a QueryRenderer, and stop calling it a rewrite#9iamralch wants to merge 2 commits into
iamralch wants to merge 2 commits into
Conversation
`rewrite` numbers its fragments from `$1`, which fits a caller that binds the fragment first and appends its own `LIMIT` after. The other arrangement is at least as common: a generated query with parameters of its own and a sentinel comment somewhere in the middle of it, where the fragment has to start at `$3` or `$4` instead. Without an offset the caller renumbers the fragment itself, which means scanning SQL for `$N` while stepping over the string literals a `LIKE` fragment carries and the quoted identifiers every column arrives as. That scanner is the largest and least pleasant part of pgxquery, and it exists there only because pgxaip's `Rewrite` has no offset to pass. Nothing forces the same shape here. `rewrite_with` takes the `Options` sqlx-cel already defines, so the knob is the same one, spelled the same way, at both layers -- and it is re-exported for the same reason `Columns` and `Value` are. The offset moves the filter and the key-set predicate together and the cursor still follows the filter's literals, so `values` stays in bind order and a caller that hands it to `bind_all` notices nothing. `param_offset` is read as `max(1)` here as well as in sqlx-cel: the cursor's own offset is computed from it rather than passed through, so a zero would otherwise reach the predicate as `$0` after sqlx-cel had already corrected it for the filter. A positional dialect ignores the offset, since a `?` carries no number to shift -- but bind order there follows the text rather than the numbering, so a fragment spliced into the middle of such a query needs its values bound in the middle. That is a real constraint numbered placeholders do not impose, and the docs on `rewrite_with` say so rather than leaving it to be discovered.
Two problems with `Query::rewrite(dialect)`, and they have one fix.
Nothing was being rewritten. A rewrite takes a statement and returns a
changed statement; this takes typed values -- a compiled filter, a parsed
ordering, a decoded token -- and produces SQL text plus the values to
bind. That is rendering. The word came from pgxaip, which took it from
pgx's `QueryRewriter` hook, where something genuinely is rewritten. The
inheritance stops here: `render` is the verb, in the public API and in
the `cursor` and `order` modules behind it.
And the arguments were the request and the schema mixed together. Six
values reached a call site -- filter, order_by, page_token, columns,
dialect, param_offset -- split evenly between what changes per request
and what is fixed for a table in a deployment. `QueryRenderer` holds the
fixed half, so it is stated once and the call passes one argument:
const VOLUMES: QueryRenderer<'static, dialect::Postgres> =
QueryRenderer::new(dialect::Postgres).columns(VOLUME_COLUMNS);
let fragment = VOLUMES.render(&query)?; // whole request
let fragment = VOLUMES.at(3).render(&query)?; // ... spliced after $1, $2
`new` is `const` and the renderer is `Copy`, so it costs an entry in the
binary and nothing at runtime: the dialects are zero-sized and `Columns`
borrows its entries.
The dialect is the one argument `new` insists on, and the column map is
not, which looks inconsistent until you compare the failure modes. A
renderer with no column map rejects every path -- fail-closed, loud, on
the first request. A renderer with a guessed dialect is not an error at
all: it is `$1` placeholders sent to MySQL, or the wrong quoting, valid
SQL for the wrong database. So the one that cannot fail safely is
required, and the one that can is a builder method.
`Query` loses `columns` and its lifetime with it, which makes it exactly
the three fields `protoc-gen-rust-aip` generates onto a request. Copying
one into the other is now field-for-field with nothing left over, which
was always the intent.
`rewrite_with` and the `Options` re-export are gone; `at` replaces them.
Options remains the right shape for sqlx-cel, which has one knob and no
required argument to pair it with -- but here it would have had to carry
the dialect to be worth keeping, and a defaultable struct is the wrong
home for a value with no correct default.
Breaking, and deliberately so while nothing is published.
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.
Started as "let a caller say where placeholder numbering starts" and grew, in review, into the API change that was actually needed. Two problems, one fix.
Nothing was being rewritten. A rewrite takes a statement and returns a changed statement. This takes typed values — a compiled filter, a parsed ordering, a decoded token — and produces SQL text plus binds. That is rendering. The word came from pgxaip, which took it from pgx's
QueryRewriterhook, where something genuinely is rewritten; the inheritance stops here.renderis now the verb in the public API and in thecursorandordermodules behind it.The arguments mixed the request with the schema. Six values reached a call site —
filter,order_by,page_token,columns,dialect,param_offset— split evenly between what changes per request and what is fixed for a table in a deployment.newisconstand the renderer isCopy, so it costs an entry in the binary and nothing at runtime — the dialects are zero-sized andColumnsborrows.The asymmetry is deliberate
The dialect is required by
new; the column map is a builder method. That looks inconsistent until you compare failure modes. A renderer with no column map rejects every path — fail-closed, loud, on the first request. A renderer with a guessed dialect is not an error at all: it's$1placeholders sent to MySQL, valid SQL for the wrong database. The one that cannot fail safely is required; the one that can is optional.Knock-on
Querylosescolumnsand its lifetime, leaving exactly the three fieldsprotoc-gen-rust-aipgenerates onto a request — so copying one into the other is field-for-field with nothing left over, which was always the intent.rewrite,rewrite_withand theOptionsre-export are gone;atreplaces the last of them.Optionsis still right for sqlx-cel, which has one knob and no required argument to pair it with.Breaking, deliberately, while nothing is published.
Testing
Against a real Postgres this time — not the CI-only path — plus the full feature sweep CI runs:
cargo test --features sqlite,mysqlwithDATABASE_URLset: 59 passed, including the 6 end-to-end Postgres tests and the 6 SQLite ones--no-default-features --features postgres) still correctly refuses to buildcargo fmt --check,cargo clippy --all-targets,cargo docwith-D warnings: clean