From b16ae87a863849aa801770d377494f14de5ef1fb Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Thu, 10 Sep 2026 07:49:56 +0400 Subject: [PATCH 1/2] feat: splice SQL fragments into a statement's sentinel comments 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.`, with the connective inside it. `/* query.where AND */` substitutes to ` 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. --- .devcontainer/devcontainer.json | 50 +++ .devcontainer/docker-compose.yml | 27 ++ .github/config/release-please-config.json | 11 + .github/config/release-please-manifest.json | 3 + .github/dependabot.yml | 30 ++ .github/workflows/ci.yml | 99 +++++ .github/workflows/merge.yml | 26 ++ .github/workflows/update.yml | 37 ++ .gitignore | 10 + Cargo.toml | 40 ++ LICENSE | 21 + README.md | 162 +++++++- clippy.toml | 4 + flake.lock | 173 ++++++++ flake.nix | 77 ++++ rust-toolchain.toml | 8 + src/lib.rs | 421 ++++++++++++++++++++ src/scan.rs | 375 +++++++++++++++++ tests/postgres.rs | 190 +++++++++ 19 files changed, 1763 insertions(+), 1 deletion(-) create mode 100644 .devcontainer/devcontainer.json create mode 100644 .devcontainer/docker-compose.yml create mode 100644 .github/config/release-please-config.json create mode 100644 .github/config/release-please-manifest.json create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/ci.yml create mode 100644 .github/workflows/merge.yml create mode 100644 .github/workflows/update.yml create mode 100644 .gitignore create mode 100644 Cargo.toml create mode 100644 LICENSE create mode 100644 clippy.toml create mode 100644 flake.lock create mode 100644 flake.nix create mode 100644 rust-toolchain.toml create mode 100644 src/lib.rs create mode 100644 src/scan.rs create mode 100644 tests/postgres.rs diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json new file mode 100644 index 0000000..f09ad20 --- /dev/null +++ b/.devcontainer/devcontainer.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://raw.githubusercontent.com/devcontainers/spec/refs/heads/main/schemas/devContainer.base.schema.json", + "name": "sqlx-query", + "service": "workspace", + "dockerComposeFile": "docker-compose.yml", + "workspaceFolder": "/home/vscode/workspace", + "features": { + "ghcr.io/devcontainers/features/nix:1": { + // Single-user install. The feature defaults to multiUser, which runs + // post-install steps through the nix-daemon — but a Docker build layer + // has no init system to start one, so those steps fail with + // "opening lock file /nix/var/nix/db/big-lock: Permission denied". + // A single-user store owned by vscode needs no daemon, and is what the + // /nix volume and the postStartCommand chown below already assume. + "multiUser": false, + "extraNixConfig": "experimental-features = nix-command flakes" + } + }, + // The single definition of the test database. Inside the container this + // resolves over the compose network; on the host, `devcontainer-env export` + // rewrites it to the port Docker assigned. `containerEnv` rather than + // `remoteEnv` because only the former applies to every process, which is + // what devcontainer-env reads. + "containerEnv": { + "DATABASE_URL": "postgres://vscode@postgres:5432/sqlx_query_test?sslmode=disable" + }, + "mounts": [ + "source=${localWorkspaceFolderBasename}-nix,target=/nix,type=volume", + "source=${localWorkspaceFolderBasename}-cache,target=/home/vscode/.cache,type=volume", + "source=${localWorkspaceFolderBasename}-cargo,target=/home/vscode/.cargo,type=volume" + ], + // Every volume stays *outside* the workspace. Mounting one at + // workspace/target would create that directory inside the bind mount, owned + // by the container's uid -- and CI runs cargo on the runner itself, where + // that is a different uid and `target/` becomes unwritable. + "postStartCommand": "sudo chown vscode:vscode /nix /home/vscode/.cache /home/vscode/.cargo", + "customizations": { + "vscode": { + "settings": { + "terminal.integrated.defaultProfile.linux": "default", + "terminal.integrated.profiles.linux": { + "default": { + "path": "nix", + "args": ["develop"] + } + } + } + } + } +} diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml new file mode 100644 index 0000000..5c37451 --- /dev/null +++ b/.devcontainer/docker-compose.yml @@ -0,0 +1,27 @@ +services: + workspace: + image: mcr.microsoft.com/devcontainers/base:bookworm + command: sleep infinity + volumes: + - ..:/home/vscode/workspace:cached + + # What tests/postgres.rs runs against. `trust` auth is safe only because this + # is never reachable off the machine -- see the port note below. + postgres: + image: postgres:18-bookworm + restart: unless-stopped + environment: + POSTGRES_USER: vscode + POSTGRES_DB: sqlx_query_test + POSTGRES_HOST_AUTH_METHOD: trust + healthcheck: + test: ["CMD-SHELL", "pg_isready"] + interval: 1s + timeout: 5s + retries: 10 + # Bare `5432`, not "5432:5432": Docker assigns a random host port, so two + # projects can run at once without colliding. `devcontainer-env export` + # finds the assigned port and rewrites DATABASE_URL to match, which is what + # lets the flake and CI share one definition of it. + ports: + - 5432 diff --git a/.github/config/release-please-config.json b/.github/config/release-please-config.json new file mode 100644 index 0000000..18d60f0 --- /dev/null +++ b/.github/config/release-please-config.json @@ -0,0 +1,11 @@ +{ + "$schema": "https://raw.githubusercontent.com/googleapis/release-please/main/schemas/config.json", + "packages": { + ".": { + "release-type": "rust", + "include-v-in-tag": true, + "include-v-in-release-name": true, + "include-component-in-tag": false + } + } +} diff --git a/.github/config/release-please-manifest.json b/.github/config/release-please-manifest.json new file mode 100644 index 0000000..e18ee07 --- /dev/null +++ b/.github/config/release-please-manifest.json @@ -0,0 +1,3 @@ +{ + ".": "0.0.0" +} diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..523d50f --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,30 @@ +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + open-pull-requests-limit: 10 + groups: + all-github-actions: + patterns: ["*"] + labels: + - "dependencies" + - "auto-merge" + commit-message: + prefix: "chore" + include: "scope" + - package-ecosystem: "cargo" + directory: "/" + schedule: + interval: "weekly" + open-pull-requests-limit: 10 + groups: + all-cargo: + patterns: ["*"] + labels: + - "dependencies" + - "auto-merge" + commit-message: + prefix: "chore" + include: "scope" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..21b6f3f --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,99 @@ +name: CI +on: + push: + branches: [main] + pull_request: + branches: [main] +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} +jobs: + test: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + timeout-minutes: 30 + steps: + - name: Checkout + uses: actions/checkout@v7 + # Brings up .devcontainer/docker-compose.yml -- including the Postgres + # tests/postgres.rs runs against -- and tears it down in a post-step even + # if the job is cancelled. DATABASE_URL is not set here: the shell hook + # gets it from `devcontainer-env export`, so it is defined once, in + # devcontainer.json. + # + # Nix then runs on the runner itself rather than inside the container. + # Building the devcontainer anyway is what keeps its definition honest -- + # a broken one fails CI instead of only failing the next contributor. + - name: Setup Devcontainer + uses: devcontainer-env/devcontainer-ci@v1 + - name: Install Nix + uses: DeterminateSystems/nix-installer-action@v22 + - name: Setup Nix Cache + uses: DeterminateSystems/magic-nix-cache-action@v14 + with: + use-flakehub: disabled + use-gha-cache: enabled + - name: Check Formatting + run: nix develop --command cargo fmt --check + - name: Lint + run: nix develop --command cargo clippy --all-targets + - name: Run Tests + run: nix develop --command cargo test + - name: Check Documentation + run: nix develop --command cargo doc --no-deps + env: + RUSTDOCFLAGS: -D warnings + release: + needs: test + if: github.event_name == 'push' + runs-on: ubuntu-latest + permissions: + contents: write + issues: write + pull-requests: write + timeout-minutes: 15 + outputs: + release_created: ${{ steps.release.outputs.release_created }} + tag_name: ${{ steps.release.outputs.tag_name }} + steps: + - name: Checkout + uses: actions/checkout@v7 + with: + fetch-depth: 0 + - name: Create Release PR + id: release + uses: googleapis/release-please-action@v5 + with: + manifest-file: .github/config/release-please-manifest.json + config-file: .github/config/release-please-config.json + target-branch: main + # No `build` job: this is a library crate with no binary, so there is nothing + # to `nix build` and no asset to attach to the release. The crate itself is + # the artifact, and it goes to crates.io below. + publish: + needs: release + if: ${{ needs.release.outputs.release_created }} + runs-on: ubuntu-latest + permissions: + contents: read + id-token: write + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@v7 + - name: Install Nix + uses: DeterminateSystems/nix-installer-action@v22 + - name: Setup Nix Cache + uses: DeterminateSystems/magic-nix-cache-action@v14 + with: + use-flakehub: disabled + use-gha-cache: enabled + - name: Authenticate with Crates.io + uses: rust-lang/crates-io-auth-action@v1 + id: auth + - name: Publish to Crates.io + run: nix develop --command cargo publish + env: + CARGO_REGISTRY_TOKEN: ${{ steps.auth.outputs.token }} diff --git a/.github/workflows/merge.yml b/.github/workflows/merge.yml new file mode 100644 index 0000000..286790b --- /dev/null +++ b/.github/workflows/merge.yml @@ -0,0 +1,26 @@ +name: Auto Merge + +on: + pull_request: + types: [opened, reopened, labeled, synchronize] + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }} + cancel-in-progress: true + +jobs: + merge: + if: contains(github.event.pull_request.labels.*.name, 'auto-merge') + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + timeout-minutes: 5 + steps: + - name: Enable auto-merge + env: + GH_TOKEN: ${{ github.token }} + PR_URL: ${{ github.event.pull_request.html_url }} + run: | + gh pr review --approve "$PR_URL" || echo "PR approval skipped or failed." + gh pr merge --auto --squash "$PR_URL" diff --git a/.github/workflows/update.yml b/.github/workflows/update.yml new file mode 100644 index 0000000..c1f9d18 --- /dev/null +++ b/.github/workflows/update.yml @@ -0,0 +1,37 @@ +name: Update Flake Locks +on: + schedule: + - cron: "0 0 * * 1" + workflow_dispatch: +jobs: + update: + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + timeout-minutes: 30 + steps: + - name: Checkout + uses: actions/checkout@v6 + - name: Generate App token + id: app-token + uses: actions/create-github-app-token@v3 + with: + client-id: ${{ vars.APP_CLIENT_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + - name: Install Nix + uses: DeterminateSystems/nix-installer-action@v22 + - name: Setup Nix Cache + uses: DeterminateSystems/magic-nix-cache-action@v13 + with: + use-flakehub: disabled + use-gha-cache: enabled + - name: Update root flake.lock + uses: DeterminateSystems/update-flake-lock@v28 + with: + token: ${{ steps.app-token.outputs.token }} + pr-title: "chore(flake): update flake.lock" + pr-labels: | + dependencies + auto-merge + commit-msg: "chore(flake): update flake.lock" \ No newline at end of file diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..8563548 --- /dev/null +++ b/.gitignore @@ -0,0 +1,10 @@ +/target +Cargo.lock + +# Nix build outputs and direnv's cached shell. flake.lock IS committed. +result +result-* +.direnv/ + +# macOS +.DS_Store diff --git a/Cargo.toml b/Cargo.toml new file mode 100644 index 0000000..5680987 --- /dev/null +++ b/Cargo.toml @@ -0,0 +1,40 @@ +[package] +name = "sqlx-query" +version = "0.0.0" +edition = "2024" +# Not a choice: sqlx 0.9 declares it, and the tests build against it. The +# library itself would compile on far older, but a crate in this set that +# claimed a lower MSRV than its siblings would be a promise nothing checks. +rust-version = "1.94" +license = "MIT" +description = "Splices SQL fragments into the sentinel comments of a query, for sqlx." +repository = "https://github.com/sqlx-contrib/sqlx-query" +keywords = ["sql", "sqlx", "postgres", "query", "filter"] +categories = ["database"] + +# Denied rather than warned, because several consumers in this ecosystem deny +# pedantic at the workspace level: a lint this crate tolerates is a lint they +# cannot. +[lints.clippy] +all = { level = "deny", priority = -1 } +pedantic = { level = "deny", priority = -1 } + +# No dependencies, deliberately. This crate takes a `&str` and returns a +# `String`; it does not need sqlx to do that, and a caller splicing into a +# hand-written query should not acquire a database driver for the privilege. +# The name says where it belongs, not what it links against. +[dependencies] + +[dev-dependencies] +# For tests/postgres.rs alone -- the round trip is the only thing here that +# needs a database, and it is the only thing text assertions cannot stand in +# for. +sqlx = { version = "0.9", default-features = false, features = [ + "postgres", + "runtime-tokio", +] } +tokio = { version = "1", features = ["macros", "rt-multi-thread"] } + +[package.metadata.docs.rs] +all-features = true +rustdoc-args = ["--cfg", "docsrs"] diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..c43d20f --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 sqlx-contrib + +Permission is hereby granted, free of charge, to any person obtaining a copy of +this software and associated documentation files (the "Software"), to deal in +the Software without restriction, including without limitation the rights to +use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +of the Software, and to permit persons to whom the Software is furnished to do +so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md index 687bb52..7ff479b 100644 --- a/README.md +++ b/README.md @@ -1,2 +1,162 @@ # sqlx-query -Splices SQL fragments into the sentinel comments of a query you already wrote, for sqlx. + +> Splice a SQL fragment into the sentinel comment of a query you already wrote — +> the connective stays with the statement, and unspliced it still runs. + +[![CI](https://github.com/sqlx-contrib/sqlx-query/actions/workflows/ci.yml/badge.svg)](https://github.com/sqlx-contrib/sqlx-query/actions/workflows/ci.yml) +[![Crate](https://img.shields.io/crates/v/sqlx-query)](https://crates.io/crates/sqlx-query) +[![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](LICENSE) + +Substitutes SQL fragments into the comments of a statement, for +[sqlx](https://github.com/launchbadge/sqlx). + +The Rust counterpart of [pgxquery](https://github.com/pgx-contrib/pgxquery), and +the third of the three: [sqlx-cel](https://github.com/sqlx-contrib/sqlx-cel) +makes a fragment out of a CEL expression, +[sqlx-aip](https://github.com/sqlx-contrib/sqlx-aip) makes two out of an AIP +`List` request, and this puts one in a query. It depends on neither — a +`format!` produces a fragment just as well. + +```rust +use sqlx_query::splice; + +const LIST_VOLUMES: &str = "\ +SELECT * FROM volumes +WHERE /* query.where AND */ TRUE +ORDER BY /* query.order_by , */ id +LIMIT $1 OFFSET $2"; + +let sql = splice(LIST_VOLUMES, &[ + ("where", Some(r#""title" = $3"#)), + ("order_by", Some(r#""created_at" DESC"#)), +])?; + +// SELECT * FROM volumes +// WHERE "title" = $3 AND TRUE +// ORDER BY "created_at" DESC , id +// LIMIT $1 OFFSET $2 +``` + +## The connective belongs to the statement + +Whatever else is inside the sentinel is kept, on the side it was written: +`/* query.where AND */` substitutes to ` AND`, and +`/* query.order_by , */` to ` ,`. + +That is the whole trick. The author of the statement decides how a fragment +joins to what surrounds it, so a fragment never has to know — the same one +splices into a `WHERE` that is `AND`ed and one that is `OR`ed, and into an +`ORDER BY` ahead of a primary-key tiebreaker. + +```rust +splice("WHERE archived /* OR query.where */", &[("where", Some("a = $1"))])?; +// WHERE archived OR a = $1 +``` + +## Unspliced, it is a comment + +A sentinel whose fragment is `None` is removed, connective and all, which leaves +`WHERE TRUE` on an unfiltered list. And a statement nobody splices at all runs +exactly as written, sentinels included — they are comments. + +That is what makes the convention safe to put in checked-in or generated SQL. +The file stays valid for `psql`, for `sqlc`, and for whatever else reads it. + +## What is an error, and what is not + +A sentinel this call says nothing about is **left alone**. Substituting what you +were not given would be deciding the statement is wrong. + +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 — the kind of bug that returns plausible rows and reaches +production. `Error::MissingSentinel` names the fragment instead. + +## Placeholders + +A spliced fragment lands among the statement's own parameters, and the two have +to agree. Two ways, and the good one costs nothing: + +**Ask the producer to start where the statement stops.** sqlx-cel's `Options` +and sqlx-aip's `rewrite_with` both take a `param_offset`, and +`placeholder_count` is what you pass them: + +```rust +let offset = sqlx_query::placeholder_count(LIST_VOLUMES) + 1; // 3 +``` + +**Or renumber afterwards** with `shift`, for a fragment that arrived numbered +from `$1` and cannot be asked to start elsewhere: + +```rust +assert_eq!(sqlx_query::shift(r#""title" = $1"#, 2), r#""title" = $3"#); +``` + +Both read the SQL properly rather than reaching for a regex. A `$1` inside a +string literal, a quoted identifier, a comment or a dollar-quoted body is text, +not a parameter, and renumbering it produces SQL that still parses and binds the +wrong value: + +```rust +// The literal is left alone; only the parameter moves. +assert_eq!( + sqlx_query::shift("note = 'costs $9' AND id = $1", 4), + "note = 'costs $9' AND id = $5", +); +``` + +## Positional dialects + +All of the above assumes numbered placeholders. With SQLite's or MySQL's `?`, +binds match the *text* rather than a number, so a fragment spliced into the +middle of a statement needs its values bound in the middle of the list too. +`shift` has nothing to do there and `placeholder_count` returns zero. + +Splicing still works; the bookkeeping moves to you. Splice at the end, use a +numbered dialect, or count the placeholders either side of the sentinel +yourself. + +## Is this safe? + +It concatenates strings into SQL, so: exactly as safe as what you hand it. A +fragment from sqlx-cel or sqlx-aip carries literals as placeholders and column +names from a fail-closed allow-list, and is safe to splice. A fragment built by +interpolating a request field is an injection, and nothing here changes that. +sqlx says the same by making you write `AssertSqlSafe` around the result, which +is a sentence you are asserting rather than a cast. + +## Scope + +**In.** Substituting named fragments into sentinel comments. Counting a +statement's placeholders. Renumbering a fragment's. + +**Out.** Building SQL, knowing what a `WHERE` clause is, binding values +(sqlx-cel's `BindAll` does that), talking to a database, and parsing SQL beyond +knowing where text ends. + +The crate has no dependencies — not even sqlx. It takes a `&str` and returns a +`String`. + +## Development + +sqlx 0.9 declares `rust-version = "1.94"`, so this crate does too. +`rust-toolchain.toml` pins the dev toolchain to 1.95.0, so plain `cargo` picks +the right one even when the machine's default stable is older than the MSRV. + +```sh +cargo test +cargo clippy --all-targets +``` + +`tests/postgres.rs` needs a database and skips without `DATABASE_URL`. It is +where the claim that a spliced statement *runs* is checked — text assertions +cannot tell a query bound one slot out from a correct one, because both return +rows. + +There is a Nix flake and a devcontainer for a batteries-included shell — the +pinned toolchain, `psql`, and a Postgres to run against: + +```sh +devcontainer up --workspace-folder . # brings up Postgres +nix develop +``` diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..c212702 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +# `..` keeps clippy's defaults and adds the product names this crate's docs +# name in prose. Without these, `doc_markdown` wants backticks around every +# mention of a database. +doc-valid-idents = ["SQLite", "PostgreSQL", "MySQL", "MariaDB", ".."] diff --git a/flake.lock b/flake.lock new file mode 100644 index 0000000..a27e98b --- /dev/null +++ b/flake.lock @@ -0,0 +1,173 @@ +{ + "nodes": { + "devcontainer-env": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs", + "rust-overlay": "rust-overlay" + }, + "locked": { + "lastModified": 1788752194, + "narHash": "sha256-2374dcsCZ/ghyZvVGDFefDh57RfTPB5k3l9v0lwWHok=", + "owner": "devcontainer-env", + "repo": "devcontainer-env", + "rev": "14f4ddc6df1b3b5ad690f10d0082fe574a53fa78", + "type": "github" + }, + "original": { + "owner": "devcontainer-env", + "repo": "devcontainer-env", + "type": "github" + } + }, + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "flake-utils_2": { + "inputs": { + "systems": "systems_2" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1788549839, + "narHash": "sha256-kOrCcSIA6w9J1hX5DqHy2k9pDTJymExTsbV74U9UtCA=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "17de0b976395537756f30a3e78f2f06e5cec89ed", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixpkgs-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "nixpkgs_2": { + "locked": { + "lastModified": 1788549839, + "narHash": "sha256-kOrCcSIA6w9J1hX5DqHy2k9pDTJymExTsbV74U9UtCA=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "17de0b976395537756f30a3e78f2f06e5cec89ed", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixpkgs-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "devcontainer-env": "devcontainer-env", + "flake-utils": "flake-utils_2", + "nixpkgs": "nixpkgs_2", + "rust-overlay": "rust-overlay_2" + } + }, + "rust-overlay": { + "inputs": { + "nixpkgs": [ + "devcontainer-env", + "nixpkgs" + ] + }, + "locked": { + "lastModified": 1788678114, + "narHash": "sha256-pcqbpV4ZI79al6KAlr+WJjaEo/PYfgctRlG43D5BSJM=", + "owner": "oxalica", + "repo": "rust-overlay", + "rev": "4748ec2f5ed4a881474ed4c98aa71a5308cdac8d", + "type": "github" + }, + "original": { + "owner": "oxalica", + "repo": "rust-overlay", + "type": "github" + } + }, + "rust-overlay_2": { + "inputs": { + "nixpkgs": [ + "nixpkgs" + ] + }, + "locked": { + "lastModified": 1788678114, + "narHash": "sha256-pcqbpV4ZI79al6KAlr+WJjaEo/PYfgctRlG43D5BSJM=", + "owner": "oxalica", + "repo": "rust-overlay", + "rev": "4748ec2f5ed4a881474ed4c98aa71a5308cdac8d", + "type": "github" + }, + "original": { + "owner": "oxalica", + "repo": "rust-overlay", + "type": "github" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + }, + "systems_2": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 0000000..8a47c13 --- /dev/null +++ b/flake.nix @@ -0,0 +1,77 @@ +{ + description = "sqlx-query — splices SQL fragments into the sentinel comments of a query, for sqlx"; + + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable"; + flake-utils.url = "github:numtide/flake-utils"; + rust-overlay = { + url = "github:oxalica/rust-overlay"; + inputs.nixpkgs.follows = "nixpkgs"; + }; + devcontainer-env.url = "github:devcontainer-env/devcontainer-env"; + }; + + outputs = + { + nixpkgs, + flake-utils, + rust-overlay, + devcontainer-env, + ... + }: + flake-utils.lib.eachDefaultSystem ( + system: + let + pkgs = import nixpkgs { + inherit system; + overlays = [ (import rust-overlay) ]; + }; + manifest = (pkgs.lib.importTOML ./Cargo.toml).package; + rust-toolchain = pkgs.rust-bin.fromRustupToolchainFile ./rust-toolchain.toml; + in + { + # No `packages.default`. This is a library crate with no binary, and + # `buildRustPackage` would want a committed Cargo.lock -- which a library + # deliberately does not have. The dev shell is the whole point here. + devShells.default = pkgs.mkShell { + inherit (manifest) name; + + packages = with pkgs; [ + rust-toolchain + pkg-config + # A CLI only, for poking at what the tests leave behind. The + # Postgres tests/postgres.rs runs against is the compose service in + # .devcontainer, not a server started here. + postgresql + devcontainer-env.packages.${system}.default + ]; + + # DATABASE_URL is defined once, in .devcontainer/devcontainer.json. + # `export` reads it from `containerEnv` and rewrites the compose + # hostname to the port Docker assigned, so the same definition is + # correct inside the container, on the host, and on a CI runner. + # + # Tolerating failure is deliberate: with no devcontainer running -- + # a contributor without Docker, or someone only touching the unit + # tests -- DATABASE_URL stays unset and tests/postgres.rs skips, + # which is its documented behaviour. + shellHook = '' + eval "$(devcontainer-env export 2>/dev/null)" 2>/dev/null || true + + # Only greet a human. CI drives this shell with + # `nix develop --command ...`, where a banner is just noise in front + # of the output someone is actually reading. + if [[ $- == *i* ]]; then + echo "${manifest.name} ${manifest.version} — $(cargo --version)" + if [[ -n "''${DATABASE_URL:-}" ]]; then + echo " cargo test # incl. the end-to-end Postgres round trip" + else + echo " cargo test # Postgres tests skip; start the devcontainer for them" + fi + echo " cargo clippy --all-targets" + fi + ''; + }; + } + ); +} diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 0000000..8138e8f --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,8 @@ +[toolchain] +# The MSRV in Cargo.toml is 1.94, which sqlx 0.9 forces. This is the *dev* +# toolchain and is deliberately newer: pinning it means `cargo` picks the right +# one here even when the machine's default stable is older than the MSRV, which +# otherwise fails with a resolver error naming five packages at once. +channel = "1.95.0" +components = ["rustfmt", "clippy", "rust-src", "rust-analyzer"] +profile = "minimal" diff --git a/src/lib.rs b/src/lib.rs new file mode 100644 index 0000000..2609338 --- /dev/null +++ b/src/lib.rs @@ -0,0 +1,421 @@ +//! Splices SQL fragments into the sentinel comments of a query you already +//! wrote, for [sqlx](https://github.com/launchbadge/sqlx). +//! +//! ``` +//! use sqlx_query::splice; +//! +//! const LIST_VOLUMES: &str = "\ +//! SELECT * FROM volumes +//! WHERE /* query.where AND */ TRUE +//! ORDER BY /* query.order_by , */ id +//! LIMIT $1 OFFSET $2"; +//! +//! let sql = splice(LIST_VOLUMES, &[ +//! ("where", Some(r#""title" = $3"#)), +//! ("order_by", Some(r#""created_at" DESC"#)), +//! ])?; +//! +//! assert!(sql.contains(r#"WHERE "title" = $3 AND TRUE"#)); +//! assert!(sql.contains(r#"ORDER BY "created_at" DESC , id"#)); +//! # Ok::<_, sqlx_query::Error>(()) +//! ``` +//! +//! # What this crate is +//! +//! Text substitution, and a convention. A statement carries a comment where a +//! fragment may go; this puts one there. It does not build SQL, does not know +//! what a `WHERE` clause is, does not talk to a database, and has no +//! dependencies — not even sqlx. +//! +//! It is the Rust counterpart of +//! [pgxquery](https://github.com/pgx-contrib/pgxquery), which does the same 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 the +//! substitution has to happen where the string is built, and it is a function +//! rather than an interface. +//! +//! Where the fragments come from is not this crate's business. +//! [sqlx-cel](https://github.com/sqlx-contrib/sqlx-cel) transpiles a CEL filter +//! into one, [sqlx-aip](https://github.com/sqlx-contrib/sqlx-aip) turns a whole +//! AIP `List` request into two, and a `format!` will do. All this needs is a +//! string. +//! +//! # The sentinel +//! +//! A sentinel is a block comment naming `query.`: +//! +//! ```sql +//! WHERE +//! /* query.where AND */ TRUE +//! ORDER BY +//! /* query.order_by , */ id -- primary key, so the order is total +//! ``` +//! +//! Whatever else is inside the comment is kept, on the side it was written: +//! `/* query.where AND */` substitutes to ` AND`, and +//! `/* query.order_by , */` to ` ,`. **The connective belongs to the +//! SQL, not to the fragment.** That is the whole trick — the author of the +//! statement decides how a fragment joins to what surrounds it, so a fragment +//! never has to know, and the same fragment can be spliced into a `WHERE` that +//! is `AND`ed and one that is `OR`ed. +//! +//! A sentinel whose fragment is [`None`] is removed, comment and connective +//! together, which is what leaves `WHERE TRUE` on an unfiltered list. A +//! statement with no fragments at all therefore runs exactly as written, which +//! is what makes the convention safe to put in generated SQL: unspliced, it is +//! a comment. +//! +//! A comment naming something not in the list — `/* query.limit */` when only +//! `where` was supplied — is left alone. This crate substitutes what it was +//! given and does not decide that a sentinel is stale. +//! +//! The reverse is an error. A fragment supplied for a sentinel the statement +//! does not carry means the predicate silently would not apply, and a dropped +//! predicate widens a result set rather than emptying it, so it fails loudly as +//! [`Error::MissingSentinel`] instead. +//! +//! # Placeholders +//! +//! A spliced fragment lands in a statement that usually binds parameters of its +//! own, and the two sets have to agree. There are two ways to arrange that, and +//! the good one costs nothing: +//! +//! **Ask the producer to start where the statement stops.** Both sqlx-cel and +//! sqlx-aip take a `param_offset`, and [`placeholder_count`] is how you know +//! what to pass: +//! +//! ``` +//! # use sqlx_query::placeholder_count; +//! # const LIST_VOLUMES: &str = "SELECT * FROM volumes LIMIT $1 OFFSET $2"; +//! let offset = placeholder_count(LIST_VOLUMES) + 1; // 3 +//! # assert_eq!(offset, 3); +//! ``` +//! +//! **Or renumber the fragment afterwards**, with [`shift`], for a fragment that +//! arrived numbered from `$1` and cannot be asked to start elsewhere. It is +//! correct, and it re-reads the SQL to do a job that need not have existed. +//! +//! Either way, the values are bound in the order the numbers say: the +//! statement's own first, the fragment's after them. +//! +//! # Positional dialects +//! +//! Everything above assumes numbered placeholders. With SQLite's or MySQL's +//! `?`, binds are matched to the *text* rather than to a number, so a fragment +//! spliced into the middle of a statement must have its values bound in the +//! middle of the list too — after the values of the placeholders before it, and +//! before those after it. [`shift`] has nothing to do there and +//! [`placeholder_count`] returns zero. +//! +//! Splicing still works; the bookkeeping moves to the caller. Splice at the end +//! of a statement, or use a numbered dialect, or count the placeholders on each +//! side of the sentinel yourself. +//! +//! # Is this safe? +//! +//! It concatenates strings into SQL, so the honest answer is: exactly as safe as +//! what you hand it. A fragment from sqlx-cel or sqlx-aip contains literals as +//! placeholders and column names from a fail-closed allow-list, and is safe to +//! splice. A fragment built by interpolating a request field is an injection, +//! and no amount of care here changes that. +//! +//! sqlx says the same thing by making you write `AssertSqlSafe` around the +//! result, which is a sentence you are asserting rather than a cast. + +#![deny(missing_docs)] +#![cfg_attr(docsrs, feature(doc_cfg))] + +mod scan; + +pub use scan::{placeholder_count, shift}; + +use core::fmt; + +/// The prefix that marks a comment as a sentinel. +/// +/// Deliberately not configurable. The point of a convention is that a statement +/// written for one project splices in another, and pgxquery has already spelled +/// it this way. +const PREFIX: &str = "query."; + +/// Substitutes `fragments` into the sentinel comments of `sql`. +/// +/// Each entry is a sentinel name — the part after `query.` — and the text to +/// put there, or [`None`] to remove the sentinel. See the crate docs for the +/// convention. +/// +/// ``` +/// # use sqlx_query::splice; +/// let sql = splice( +/// "SELECT * FROM t WHERE /* query.where AND */ TRUE", +/// &[("where", Some("a = $1"))], +/// )?; +/// +/// assert_eq!(sql, "SELECT * FROM t WHERE a = $1 AND TRUE"); +/// # Ok::<_, sqlx_query::Error>(()) +/// ``` +/// +/// # Errors +/// +/// [`Error::MissingSentinel`] when a fragment is [`Some`] and `sql` has no +/// sentinel to put it in — which would otherwise drop a predicate and widen the +/// result set, silently. +pub fn splice(sql: &str, fragments: &[(&str, Option<&str>)]) -> Result { + let mut spliced = String::with_capacity(sql.len()); + let mut rest = sql; + // Which sentinels were actually found, so the check below can tell a + // fragment that was used from one that had nowhere to go. + let mut substituted = vec![false; fragments.len()]; + + while let Some(open) = rest.find("/*") { + let Some(length) = rest[open..].find("*/") else { + // Unterminated, so there is no comment here to substitute into and + // nothing further to find. The database can have its opinion. + break; + }; + let close = open + length + "*/".len(); + + spliced.push_str(&rest[..open]); + + match sentinel(&rest[open + "/*".len()..close - "*/".len()]) { + Some((prefix, name, suffix)) => match position(fragments, name) { + Some(index) => { + substituted[index] = true; + if let Some(fragment) = fragments[index].1 { + spliced.push_str(prefix); + spliced.push_str(fragment); + spliced.push_str(suffix); + } + } + // A sentinel this call says nothing about. Left as it was: + // it is a comment, and the statement runs with it. + None => spliced.push_str(&rest[open..close]), + }, + None => spliced.push_str(&rest[open..close]), + } + + rest = &rest[close..]; + } + + spliced.push_str(rest); + + for (index, (name, fragment)) in fragments.iter().enumerate() { + if fragment.is_some() && !substituted[index] { + return Err(Error::MissingSentinel { + name: (*name).to_owned(), + }); + } + } + + Ok(spliced) +} + +/// Splits a comment body around the `query.` it holds. +/// +/// Returns the text either side, with the whitespace that abutted the comment +/// markers trimmed off, so `" query.where AND "` yields `("", " AND")` and the +/// substitution reads ` AND`. +/// +/// The name runs to the first character that cannot be part of one, so +/// `query.where` and `query.order_by` are both found without the caller +/// declaring which names exist. +fn sentinel(body: &str) -> Option<(&str, &str, &str)> { + let at = body.find(PREFIX)?; + let after = at + PREFIX.len(); + + let length = body[after..] + .find(|character: char| !character.is_ascii_alphanumeric() && character != '_') + .unwrap_or(body.len() - after); + if length == 0 { + return None; + } + + Some(( + body[..at].trim_start(), + &body[after..after + length], + body[after + length..].trim_end(), + )) +} + +/// Finds `name` among the supplied fragments. +/// +/// A linear scan: the list is two entries long in every real call, and this +/// keeps the parameter an ordinary slice rather than something the caller has +/// to build. +fn position(fragments: &[(&str, Option<&str>)], name: &str) -> Option { + fragments + .iter() + .position(|(candidate, _)| *candidate == name) +} + +/// Why a [`splice`] failed. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub enum Error { + /// A fragment was supplied for a sentinel the statement does not carry. + /// + /// Almost always a typo in one of the two — `/* query.wehre AND */`, or a + /// name that was renamed on one side only. It is an error rather than a + /// no-op because the alternative is a filter that quietly does not apply, + /// and a query that returns *more* rows than it should is the kind of bug + /// that reaches production. + MissingSentinel { + /// The name that had nowhere to go. + name: String, + }, +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::MissingSentinel { name } => write!( + f, + "no /* query.{name} … */ sentinel in the statement to splice the {name} fragment into", + ), + } + } +} + +impl core::error::Error for Error {} + +#[cfg(test)] +mod tests { + use super::{Error, splice}; + + /// The shape sqlc emits, sentinels and all. + const LIST: &str = "SELECT *\nFROM volumes\nWHERE\n /* query.where AND */ TRUE\nORDER BY\n /* query.order_by , */ id\nLIMIT $1 OFFSET $2"; + + fn list(where_sql: Option<&str>, order_sql: Option<&str>) -> String { + splice(LIST, &[("where", where_sql), ("order_by", order_sql)]).unwrap() + } + + #[test] + fn substitutes_both_and_keeps_the_connectives_where_they_were_written() { + let sql = list(Some(r#""title" = $3"#), Some(r#""created_at" DESC"#)); + + assert!(sql.contains("WHERE\n \"title\" = $3 AND TRUE"), "{sql}"); + assert!( + sql.contains("ORDER BY\n \"created_at\" DESC , id"), + "{sql}" + ); + } + + /// The property that makes the convention safe to generate: unspliced, the + /// statement is the statement. + #[test] + fn no_fragments_leaves_the_statement_running_as_written() { + let sql = list(None, None); + + assert!(!sql.contains("query."), "{sql}"); + assert!(sql.contains("WHERE\n TRUE"), "{sql}"); + assert!(sql.contains("ORDER BY\n id"), "{sql}"); + } + + #[test] + fn one_fragment_does_not_disturb_the_others_sentinel() { + let sql = list(Some("a = $3"), None); + + assert!(sql.contains("WHERE\n a = $3 AND TRUE"), "{sql}"); + assert!(sql.contains("ORDER BY\n id"), "{sql}"); + } + + /// The connective is the statement's, so the same fragment reads correctly + /// in a query that joins it differently. + #[test] + fn the_statement_decides_how_a_fragment_joins() { + let sql = splice( + "WHERE archived /* OR query.where */", + &[("where", Some("a = $1"))], + ) + .unwrap(); + + assert_eq!(sql, "WHERE archived OR a = $1"); + } + + #[test] + fn a_comment_that_is_not_a_sentinel_is_left_alone() { + let sql = splice( + "SELECT 1 /* an ordinary comment */ WHERE /* query.where AND */ TRUE", + &[("where", Some("a = $1"))], + ) + .unwrap(); + + assert_eq!( + sql, + "SELECT 1 /* an ordinary comment */ WHERE a = $1 AND TRUE" + ); + } + + /// A sentinel the call says nothing about survives: substituting what you + /// were not given would be deciding the statement is wrong. + #[test] + fn an_unmentioned_sentinel_survives() { + let sql = splice( + "WHERE /* query.where AND */ TRUE LIMIT /* query.limit */ 10", + &[("where", Some("a = $1"))], + ) + .unwrap(); + + assert_eq!(sql, "WHERE a = $1 AND TRUE LIMIT /* query.limit */ 10"); + } + + /// The reverse, which is not survivable: the filter would silently not + /// apply and the page would be wider than the caller asked for. + #[test] + fn a_fragment_with_nowhere_to_go_is_an_error() { + let error = splice("SELECT * FROM t", &[("where", Some("a = $1"))]).unwrap_err(); + + assert_eq!( + error, + Error::MissingSentinel { + name: "where".to_owned() + }, + ); + assert!(error.to_string().contains("query.where"), "{error}"); + } + + /// Nothing to splice, nothing to warn about. + #[test] + fn an_absent_fragment_with_nowhere_to_go_is_fine() { + let sql = splice("SELECT * FROM t", &[("where", None)]).unwrap(); + + assert_eq!(sql, "SELECT * FROM t"); + } + + #[test] + fn a_sentinel_may_appear_more_than_once() { + let sql = splice( + "WHERE /* query.where AND */ TRUE UNION SELECT * FROM u WHERE /* query.where AND */ TRUE", + &[("where", Some("a = $1"))], + ) + .unwrap(); + + assert_eq!(sql.matches("a = $1").count(), 2, "{sql}"); + } + + #[test] + fn an_unterminated_comment_is_left_where_it_is() { + let sql = splice( + "WHERE /* query.where AND */ TRUE /* unterminated", + &[("where", Some("a = $1"))], + ) + .unwrap(); + + assert_eq!(sql, "WHERE a = $1 AND TRUE /* unterminated"); + } + + #[test] + fn a_bare_prefix_is_not_a_sentinel() { + let sql = splice("SELECT 1 /* query. */", &[("where", None)]).unwrap(); + + assert_eq!(sql, "SELECT 1 /* query. */"); + } + + #[test] + fn a_statement_with_no_comments_at_all_is_returned_whole() { + let sql = splice("SELECT 1", &[]).unwrap(); + + assert_eq!(sql, "SELECT 1"); + } +} diff --git a/src/scan.rs b/src/scan.rs new file mode 100644 index 0000000..f080ee7 --- /dev/null +++ b/src/scan.rs @@ -0,0 +1,375 @@ +//! A scanner over SQL text, and the two things this crate reads off it. +//! +//! Both [`placeholder_count`] and [`shift`] have to answer the same question — +//! *is this `$1` a placeholder, or is it text?* — so both walk the statement the +//! same way, stepping over the five constructs where a `$` means nothing: +//! +//! | | | +//! | --- | --- | +//! | `'…'` | a string literal, `''` escaping a quote | +//! | `"…"` | a quoted identifier, `""` escaping a quote | +//! | `-- …` | a line comment | +//! | `/* … */` | a block comment, which PostgreSQL allows to nest | +//! | `$tag$…$tag$` | a dollar-quoted string | +//! +//! Every one of those can contain a `$1`, and none of them binds anything. A +//! naive scan over `WHERE note = 'costs $1' AND id = $1` sees two placeholders +//! and renumbers the wrong one, which is a bug that survives review because the +//! SQL still parses. +//! +//! This is not a SQL parser and does not try to be. It knows where text ends, +//! which is the whole of what these two functions need. + +/// Returns the highest `$N` in `sql`, or `0` when it binds nothing. +/// +/// This is how many parameters a statement already has, which is what a caller +/// needs to know before splicing a fragment into it: the fragment's first +/// placeholder is this plus one. +/// +/// The *highest*, not the count of occurrences. `$1` may be referenced from +/// several places and bound once, so counting occurrences would over-report; +/// and a statement is free to skip a number, in which case the driver still +/// expects that many values. The highest is the only answer that is right for +/// both. +/// +/// ``` +/// assert_eq!(sqlx_query::placeholder_count("SELECT * FROM t WHERE a = $1 AND b = $2"), 2); +/// assert_eq!(sqlx_query::placeholder_count("SELECT * FROM t"), 0); +/// // Text is not a placeholder, however much it looks like one. +/// assert_eq!(sqlx_query::placeholder_count("SELECT '$9' FROM t WHERE a = $1"), 1); +/// ``` +#[must_use] +pub fn placeholder_count(sql: &str) -> usize { + let mut highest = 0; + + scan(sql, |token| { + if let Token::Placeholder(number) = token { + highest = highest.max(number); + } + }); + + highest +} + +/// Renumbers every placeholder in `sql` by `offset`, so `$1` becomes +/// `$(1 + offset)`. +/// +/// For a fragment that arrived numbered from `$1` and has to be spliced into a +/// statement that already binds parameters. **Prefer not needing it**: a +/// producer that accepts a starting offset — [`sqlx-cel`]'s `Options` and +/// [`sqlx-aip`]'s `rewrite_with`, both of which take one — emits the right +/// numbers to begin with, and then nothing has to re-read the SQL at all. Use +/// this for a fragment you were handed and cannot ask to renumber. +/// +/// ``` +/// // The statement binds $1 and $2 already, so the fragment starts at $3. +/// assert_eq!(sqlx_query::shift(r#""title" = $1"#, 2), r#""title" = $3"#); +/// ``` +/// +/// Only numbered placeholders move. A positional `?` has no number and is +/// returned untouched — see the crate docs on what that costs. +/// +/// [`sqlx-cel`]: https://github.com/sqlx-contrib/sqlx-cel +/// [`sqlx-aip`]: https://github.com/sqlx-contrib/sqlx-aip +#[must_use] +pub fn shift(sql: &str, offset: usize) -> String { + if offset == 0 { + return sql.to_owned(); + } + + let mut shifted = String::with_capacity(sql.len()); + + scan(sql, |token| match token { + Token::Placeholder(number) => { + shifted.push('$'); + shifted.push_str(&(number + offset).to_string()); + } + Token::Text(text) => shifted.push_str(text), + }); + + shifted +} + +/// What [`scan`] hands its visitor. +enum Token<'a> { + /// A `$N`, already parsed. The text it came from is not passed, because + /// every caller that wants it wants it renumbered. + Placeholder(usize), + /// Everything else, in runs as long as the scanner can make them. + Text(&'a str), +} + +/// Walks `sql`, calling `visit` for each placeholder and each run of text +/// between them. +/// +/// The concatenation of every `Token::Text` and the source of every +/// `Token::Placeholder` is exactly `sql`, which is what lets [`shift`] rebuild +/// the statement by appending as it goes. +fn scan<'a, F>(sql: &'a str, mut visit: F) +where + F: FnMut(Token<'a>), +{ + let bytes = sql.as_bytes(); + // The start of the current run of ordinary text, flushed whenever the + // scanner reaches something it has to treat specially. + let mut text = 0; + let mut at = 0; + + while at < bytes.len() { + // Each arm returns where the construct ends. `None` means "ordinary + // text", and the byte is simply consumed. + let skipped = match bytes[at] { + b'\'' => Some(quoted(bytes, at, b'\'')), + b'"' => Some(quoted(bytes, at, b'"')), + b'-' if bytes.get(at + 1) == Some(&b'-') => Some(line_comment(bytes, at)), + b'/' if bytes.get(at + 1) == Some(&b'*') => Some(block_comment(bytes, at)), + b'$' => { + if let Some((number, end)) = placeholder(bytes, at) { + if text < at { + visit(Token::Text(&sql[text..at])); + } + visit(Token::Placeholder(number)); + text = end; + at = end; + continue; + } + // Not `$N`, so either dollar-quoting or a lone `$`. + dollar_quoted(bytes, at) + } + _ => None, + }; + + at = match skipped { + Some(end) => end, + None => at + 1, + }; + } + + if text < bytes.len() { + visit(Token::Text(&sql[text..])); + } +} + +/// The end of a `'…'` or `"…"` beginning at `at`, doubled quotes included. +/// +/// An unterminated literal ends at the end of the input rather than being an +/// error: this scanner reports what it can see, and a statement that does not +/// parse is the database's to complain about. +fn quoted(bytes: &[u8], at: usize, quote: u8) -> usize { + let mut index = at + 1; + + while index < bytes.len() { + if bytes[index] == quote { + // A doubled quote is an escaped one, and the literal continues. + if bytes.get(index + 1) == Some("e) { + index += 2; + continue; + } + return index + 1; + } + index += 1; + } + + bytes.len() +} + +/// The end of a `-- …` comment, including its newline. +fn line_comment(bytes: &[u8], at: usize) -> usize { + let mut index = at + 2; + + while index < bytes.len() && bytes[index] != b'\n' { + index += 1; + } + + (index + 1).min(bytes.len()) +} + +/// The end of a `/* … */` comment, honouring PostgreSQL's nesting. +/// +/// Nesting is why this cannot be a search for the next `*/`: in +/// `/* a /* b */ c */` that would stop in the middle and leave `c */` to be +/// scanned as SQL. +fn block_comment(bytes: &[u8], at: usize) -> usize { + let mut index = at + 2; + let mut depth = 1usize; + + while index < bytes.len() && depth > 0 { + if bytes[index] == b'/' && bytes.get(index + 1) == Some(&b'*') { + depth += 1; + index += 2; + } else if bytes[index] == b'*' && bytes.get(index + 1) == Some(&b'/') { + depth -= 1; + index += 2; + } else { + index += 1; + } + } + + index +} + +/// The number and end of a `$N` beginning at `at`, if that is what it is. +fn placeholder(bytes: &[u8], at: usize) -> Option<(usize, usize)> { + let mut index = at + 1; + + while index < bytes.len() && bytes[index].is_ascii_digit() { + index += 1; + } + + if index == at + 1 { + return None; + } + + // Digits only, and bounded by the length of the statement, so the parse + // cannot fail for any reason but overflow -- at which point the statement + // has bigger problems than this crate. + let number = core::str::from_utf8(&bytes[at + 1..index]) + .ok()? + .parse() + .ok()?; + + Some((number, index)) +} + +/// The end of a `$tag$…$tag$` string beginning at `at`, if that is what it is. +/// +/// The tag is empty (`$$…$$`) or an identifier, and the closing tag must match +/// it exactly. An unclosed one runs to the end of the input, on the same +/// reasoning as [`quoted`]. +fn dollar_quoted(bytes: &[u8], at: usize) -> Option { + let mut index = at + 1; + + while index < bytes.len() && bytes[index] != b'$' { + let character = bytes[index]; + let valid = character.is_ascii_alphabetic() + || character == b'_' + // A digit is allowed inside a tag but not as its first character, + // which is also what stops `$1` reaching here as a tag. + || (index > at + 1 && character.is_ascii_digit()); + if !valid { + return None; + } + index += 1; + } + + if bytes.get(index) != Some(&b'$') { + return None; + } + + let tag = &bytes[at..=index]; + let body = index + 1; + + let close = bytes[body..] + .windows(tag.len()) + .position(|window| window == tag); + + Some(match close { + Some(offset) => body + offset + tag.len(), + None => bytes.len(), + }) +} + +#[cfg(test)] +mod tests { + use super::{placeholder_count, shift}; + + #[test] + fn counts_the_highest_placeholder_rather_than_the_occurrences() { + // $1 twice and $3 once: three values are expected, not three + // occurrences and not two distinct numbers. + assert_eq!(placeholder_count("a = $1 OR (b = $1 AND c = $3)"), 3); + } + + #[test] + fn a_statement_with_no_placeholders_binds_nothing() { + assert_eq!(placeholder_count("SELECT 1"), 0); + assert_eq!(placeholder_count(""), 0); + } + + #[test] + fn shifts_every_placeholder_and_leaves_the_rest_alone() { + assert_eq!( + shift(r#"("a" = $1 OR "a" = $2) AND "b" > $10"#, 2), + r#"("a" = $3 OR "a" = $4) AND "b" > $12"#, + ); + } + + #[test] + fn a_zero_shift_is_the_statement_unchanged() { + assert_eq!(shift("a = $1", 0), "a = $1"); + } + + /// The reason this is a scanner and not a regex. A `LIKE` fragment carries + /// `'%'` literals, and a literal can carry anything at all. + #[test] + fn text_inside_a_string_literal_is_not_a_placeholder() { + assert_eq!(placeholder_count("note = 'costs $9' AND id = $1"), 1); + assert_eq!( + shift("note = 'costs $9' AND id = $1", 4), + "note = 'costs $9' AND id = $5", + ); + assert_eq!( + shift(r#""a" LIKE '%' || $1 || '%'"#, 1), + r#""a" LIKE '%' || $2 || '%'"#, + ); + } + + #[test] + fn a_doubled_quote_does_not_end_the_literal() { + // The literal is `it's $9`, so the $9 is still text. + assert_eq!(placeholder_count("a = 'it''s $9' AND b = $1"), 1); + assert_eq!(placeholder_count(r#""odd""name $9" = $1"#), 1); + } + + #[test] + fn text_inside_a_comment_is_not_a_placeholder() { + assert_eq!(placeholder_count("a = $1 -- was $9\nAND b = $2"), 2); + assert_eq!(placeholder_count("a = $1 /* was $9 */"), 1); + // Nested, which is where a search for the next `*/` would stop early + // and then scan `$9 */` as SQL. + assert_eq!(placeholder_count("a = $1 /* /* $9 */ $9 */"), 1); + } + + #[test] + fn text_inside_a_dollar_quoted_string_is_not_a_placeholder() { + assert_eq!(placeholder_count("a = $body$ $9 $body$ AND b = $1"), 1); + assert_eq!(placeholder_count("a = $$ $9 $$ AND b = $1"), 1); + assert_eq!( + shift("a = $body$ $9 $body$ AND b = $1", 3), + "a = $body$ $9 $body$ AND b = $4", + ); + } + + /// A tag cannot start with a digit, which is what keeps `$1` a placeholder + /// rather than the opening of a dollar-quoted string. + #[test] + fn a_placeholder_is_not_read_as_a_dollar_quote_tag() { + assert_eq!(placeholder_count("a = $1$ AND b = $2"), 2); + } + + /// Unterminated text runs to the end rather than raising: this is a + /// scanner, and the statement is the database's to reject. + #[test] + fn unterminated_text_swallows_the_rest() { + assert_eq!(placeholder_count("a = 'open $9"), 0); + assert_eq!(placeholder_count("a = $tag$ open $9"), 0); + assert_eq!(placeholder_count("a = $1 /* open $9"), 1); + } + + #[test] + fn a_lone_dollar_is_left_where_it_is() { + assert_eq!(shift("cost = '$' || $1", 1), "cost = '$' || $2"); + assert_eq!(placeholder_count("a = $ AND b = $1"), 1); + } + + /// Multi-byte text is copied whole rather than byte by byte, so an index + /// landing inside a character would panic on the slice. It must not. + #[test] + fn text_outside_ascii_survives() { + assert_eq!( + shift("titel = 'Grüße $9' AND id = $1", 1), + "titel = 'Grüße $9' AND id = $2" + ); + assert_eq!(placeholder_count("titel = '日本語 $9' AND id = $2"), 2); + } +} diff --git a/tests/postgres.rs b/tests/postgres.rs new file mode 100644 index 0000000..68c37b9 --- /dev/null +++ b/tests/postgres.rs @@ -0,0 +1,190 @@ +//! End-to-end tests against a real Postgres. +//! +//! The unit tests assert what the spliced statement *says*. These assert the +//! part text cannot: that it parses, and that the placeholders line up with the +//! values across the boundary between the statement's own parameters and the +//! fragment's. Both failures produce SQL that reads correctly — the first is +//! caught by the server, the second by nothing at all, since a query bound one +//! slot out still runs and still returns rows. +//! +//! Set `DATABASE_URL` to run them. Without it each test skips, because a +//! missing database is a missing environment rather than a failure: +//! +//! ```sh +//! DATABASE_URL=postgres://localhost/sqlx_query_test cargo test --test postgres +//! ``` + +use sqlx::{AssertSqlSafe, PgPool, Row}; +use sqlx_query::{placeholder_count, shift, splice}; + +/// The shape sqlc generates: two parameters of its own, and a sentinel before +/// either of them. +const LIST_VOLUMES: &str = "\ +SELECT title +FROM volumes +WHERE + /* query.where AND */ TRUE +ORDER BY + /* query.order_by , */ id +LIMIT $1 OFFSET $2"; + +/// Creates `schema`, seeds `volumes` inside it, and returns a pool whose +/// `search_path` points there. Returns `None` when `DATABASE_URL` is unset. +/// +/// A schema per test, because `cargo test` runs them concurrently against one +/// database and they would otherwise be seeding the same table. +async fn pool(schema: &str) -> Option { + let Ok(url) = std::env::var("DATABASE_URL") else { + // Skipping is right on a machine with no Docker, but in CI it would + // mean the round trip quietly stopped being tested -- and a skipped + // test looks exactly like a passing one. The devcontainer is there + // precisely so this cannot happen, so assert it rather than trust it. + assert!( + std::env::var_os("CI").is_none(), + "DATABASE_URL is unset in CI: the devcontainer's Postgres never \ + reached the shell, so these tests would have silently skipped", + ); + eprintln!("skipped: DATABASE_URL is unset"); + return None; + }; + + let pool = PgPool::connect(&url) + .await + .expect("DATABASE_URL must connect"); + + // The schema name is this file's, never a caller's, so the format! is not + // an injection -- and an identifier cannot be a bind parameter anyway. + for statement in [ + format!("DROP SCHEMA IF EXISTS {schema} CASCADE"), + format!("CREATE SCHEMA {schema}"), + format!("SET search_path TO {schema}"), + "CREATE TABLE volumes (id BIGSERIAL PRIMARY KEY, title TEXT NOT NULL, read_count INT NOT NULL)".to_owned(), + "INSERT INTO volumes (title, read_count) VALUES ('Dune', 9), ('Emma', 3), ('Ulysses', 12)" + .to_owned(), + ] { + sqlx::query(AssertSqlSafe(statement)) + .execute(&pool) + .await + .expect("seed the schema"); + } + + // `SET` above applies to one pooled connection; this applies to every + // connection the pool hands out for the rest of the test. + sqlx::query(AssertSqlSafe(format!( + "ALTER ROLE CURRENT_USER IN DATABASE {} SET search_path TO {schema}", + database(&url) + ))) + .execute(&pool) + .await + .ok(); + + Some(pool) +} + +/// The database name in `url`, for the `ALTER ROLE … IN DATABASE` above. +fn database(url: &str) -> String { + url.rsplit('/') + .next() + .and_then(|tail| tail.split('?').next()) + .unwrap_or("postgres") + .to_owned() +} + +#[tokio::test] +async fn a_spliced_filter_runs_and_selects_what_it_says() { + let Some(pool) = pool("splice_filter").await else { + return; + }; + + // The statement binds $1 and $2, so the fragment starts at $3 -- which is + // the number `placeholder_count` exists to produce. + let offset = placeholder_count(LIST_VOLUMES) + 1; + assert_eq!(offset, 3); + + let sql = splice( + LIST_VOLUMES, + &[ + ("where", Some(&format!("read_count > ${offset}"))), + ("order_by", Some("title DESC")), + ], + ) + .expect("splice"); + + let rows = sqlx::query(AssertSqlSafe(sql)) + .bind(10_i64) // $1, LIMIT + .bind(0_i64) // $2, OFFSET + .bind(5_i32) // $3, the fragment's + .fetch_all(&pool) + .await + .expect("the spliced statement must run"); + + let titles: Vec = rows.iter().map(|row| row.get("title")).collect(); + assert_eq!(titles, ["Ulysses", "Dune"]); +} + +/// The other route to the same statement: a fragment numbered from `$1` and +/// renumbered afterwards. Both paths have to produce the same rows, or one of +/// them is binding a slot out. +#[tokio::test] +async fn a_shifted_fragment_agrees_with_a_pre_numbered_one() { + let Some(pool) = pool("splice_shift").await else { + return; + }; + + let shifted = shift("read_count > $1", placeholder_count(LIST_VOLUMES)); + assert_eq!(shifted, "read_count > $3"); + + let sql = splice(LIST_VOLUMES, &[("where", Some(&shifted))]).expect("splice"); + + let rows = sqlx::query(AssertSqlSafe(sql)) + .bind(10_i64) + .bind(0_i64) + .bind(5_i32) + .fetch_all(&pool) + .await + .expect("the spliced statement must run"); + + let titles: Vec = rows.iter().map(|row| row.get("title")).collect(); + // Ordered by id, since no order_by fragment was supplied. + assert_eq!(titles, ["Dune", "Ulysses"]); +} + +/// The property the whole convention rests on: unspliced, the statement is the +/// statement. If the sentinels did not survive as comments, generated SQL could +/// not carry them. +#[tokio::test] +async fn an_unspliced_statement_runs_as_written() { + let Some(pool) = pool("splice_none").await else { + return; + }; + + let sql = splice(LIST_VOLUMES, &[("where", None), ("order_by", None)]).expect("splice"); + + let rows = sqlx::query(AssertSqlSafe(sql)) + .bind(10_i64) + .bind(1_i64) + .fetch_all(&pool) + .await + .expect("the unspliced statement must run"); + + let titles: Vec = rows.iter().map(|row| row.get("title")).collect(); + assert_eq!(titles, ["Emma", "Ulysses"]); +} + +/// The sentinel is a comment, so the *original* has to run too — that is what +/// makes it safe to put in checked-in SQL that other tools also read. +#[tokio::test] +async fn the_statement_with_its_sentinels_intact_runs() { + let Some(pool) = pool("splice_intact").await else { + return; + }; + + let rows = sqlx::query(AssertSqlSafe(LIST_VOLUMES)) + .bind(10_i64) + .bind(0_i64) + .fetch_all(&pool) + .await + .expect("the statement must run with its sentinels in place"); + + assert_eq!(rows.len(), 3); +} From 6646f678f2cbdb0fe967b95fc03500bc1e7c8d84 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Thu, 10 Sep 2026 07:57:55 +0400 Subject: [PATCH 2/2] fix(tests): put search_path on the connection, not on a statement `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. --- tests/postgres.rs | 51 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/tests/postgres.rs b/tests/postgres.rs index 68c37b9..5f6144f 100644 --- a/tests/postgres.rs +++ b/tests/postgres.rs @@ -14,6 +14,9 @@ //! DATABASE_URL=postgres://localhost/sqlx_query_test cargo test --test postgres //! ``` +use std::str::FromStr as _; + +use sqlx::postgres::PgConnectOptions; use sqlx::{AssertSqlSafe, PgPool, Row}; use sqlx_query::{placeholder_count, shift, splice}; @@ -48,7 +51,7 @@ async fn pool(schema: &str) -> Option { return None; }; - let pool = PgPool::connect(&url) + let admin = PgPool::connect(&url) .await .expect("DATABASE_URL must connect"); @@ -57,39 +60,39 @@ async fn pool(schema: &str) -> Option { for statement in [ format!("DROP SCHEMA IF EXISTS {schema} CASCADE"), format!("CREATE SCHEMA {schema}"), - format!("SET search_path TO {schema}"), - "CREATE TABLE volumes (id BIGSERIAL PRIMARY KEY, title TEXT NOT NULL, read_count INT NOT NULL)".to_owned(), - "INSERT INTO volumes (title, read_count) VALUES ('Dune', 9), ('Emma', 3), ('Ulysses', 12)" - .to_owned(), ] { sqlx::query(AssertSqlSafe(statement)) + .execute(&admin) + .await + .expect("create the schema"); + } + admin.close().await; + + // `search_path` goes on the *connection options*, so every connection the + // pool opens has it. A `SET search_path` statement would apply to whichever + // pooled connection happened to run it, and the next statement -- taken + // from a different connection -- would create its table in `public`, where + // it collides with every other test doing the same. + let options = PgConnectOptions::from_str(&url) + .expect("DATABASE_URL must parse") + .options([("search_path", schema)]); + let pool = PgPool::connect_with(options) + .await + .expect("connect to the schema"); + + for statement in [ + "CREATE TABLE volumes (id BIGSERIAL PRIMARY KEY, title TEXT NOT NULL, read_count INT NOT NULL)", + "INSERT INTO volumes (title, read_count) VALUES ('Dune', 9), ('Emma', 3), ('Ulysses', 12)", + ] { + sqlx::query(statement) .execute(&pool) .await .expect("seed the schema"); } - // `SET` above applies to one pooled connection; this applies to every - // connection the pool hands out for the rest of the test. - sqlx::query(AssertSqlSafe(format!( - "ALTER ROLE CURRENT_USER IN DATABASE {} SET search_path TO {schema}", - database(&url) - ))) - .execute(&pool) - .await - .ok(); - Some(pool) } -/// The database name in `url`, for the `ALTER ROLE … IN DATABASE` above. -fn database(url: &str) -> String { - url.rsplit('/') - .next() - .and_then(|tail| tail.split('?').next()) - .unwrap_or("postgres") - .to_owned() -} - #[tokio::test] async fn a_spliced_filter_runs_and_selects_what_it_says() { let Some(pool) = pool("splice_filter").await else {