From c9bb54ffd1fa43eaa92e90d79f00b03e3cc269ba Mon Sep 17 00:00:00 2001 From: kartik Date: Thu, 9 Jul 2026 21:12:17 +0530 Subject: [PATCH 1/2] Fix EXPLAIN on MySQL 9 and MariaDB, add live-database integration tests The explain package failed on every query against MySQL 9 and MariaDB. Three independent defects, all in analyzeMySQL, all invisible to unit tests because they concern what a real server actually returns: - MySQL 9 defaults @@explain_format to TREE, so a plain EXPLAIN returns one free-text column instead of the twelve-column plan the code scanned. Request EXPLAIN FORMAT=TRADITIONAL, which MySQL 5.7/8/9 and MariaDB all accept, so no server version detection is needed. - MariaDB emits ten plan columns where MySQL emits twelve (no partitions, no filtered), breaking the positional Scan. Address columns by name. - MySQL and MariaDB reject every statement inside a READ ONLY transaction with error 1792, including an EXPLAIN that only plans it, so WithAllowDML never worked there. Use a regular transaction for DML; safety still rests on validate(), on plain EXPLAIN never executing the statement, and on the unconditional rollback. Postgres allows it and keeps its read-only tx. Also drops a false positive: a UNION RESULT row names a temporary table () with type=ALL and no key, and was reported as an unindexed full table scan. validate() now returns the statement kind so the MySQL path can choose its transaction mode, and the per-row rules move into mysqlRowIssues to keep analyzeMySQL under the gocyclo limit. test/integration is a new unpublished module that runs explain against live PostgreSQL 18.3, MySQL 9.7.1 and MariaDB 12.3.2 via docker compose, behind an `integration` build tag so `go test ./...` stays Docker-free. Verified these are real guards: with the fix reverted, every MySQL and MariaDB case fails and every Postgres case passes. Separately, the satellite modules were compiling against the *published* core rather than the working tree, because `make release-prep` had stripped their replace directives and nothing restored them. A breaking change to analyzer/ or middleware/ would have passed CI green. A committed go.work now handles local resolution for all ten modules; no go.mod carries a replace, and release-prep is removed in favour of the manual steps in CONTRIBUTING. Consumers are unaffected (GOWORK=off reproduces their build). --- .coderabbit.yaml | 47 +++++-- .github/workflows/ci.yml | 59 ++++++++ .gitignore | 6 +- AGENTS.md | 3 +- CHANGELOG.md | 47 ++++++- CONTRIBUTING.md | 56 +++++++- Makefile | 72 +++++----- README.md | 26 ++-- explain/explain.go | 174 ++++++++++++++--------- explain/explain_test.go | 2 +- go.work | 32 +++++ go.work.sum | 58 ++++++++ integrations/gormguard/go.mod | 2 +- integrations/gormguard/go.sum | 4 +- integrations/pgxguard/go.mod | 2 +- integrations/pgxguard/go.sum | 4 +- parsers/pgparser/go.mod | 2 +- parsers/pgparser/go.sum | 4 +- test/integration/docker-compose.yml | 49 +++++++ test/integration/go.mod | 25 ++++ test/integration/go.sum | 34 +++++ test/integration/integration_test.go | 110 +++++++++++++++ test/integration/mysql_test.go | 199 +++++++++++++++++++++++++++ test/integration/postgres_test.go | 190 +++++++++++++++++++++++++ 24 files changed, 1075 insertions(+), 132 deletions(-) create mode 100644 go.work create mode 100644 go.work.sum create mode 100644 test/integration/docker-compose.yml create mode 100644 test/integration/go.mod create mode 100644 test/integration/go.sum create mode 100644 test/integration/integration_test.go create mode 100644 test/integration/mysql_test.go create mode 100644 test/integration/postgres_test.go diff --git a/.coderabbit.yaml b/.coderabbit.yaml index fc200b1..31a7891 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -95,7 +95,9 @@ reviews: Every integration must build on the exported middleware.Guard (pgxguard is the reference). Flag any integration that re-implements analysis, redaction or N+1 by hand. Each should expose ResetN1() for - per-request scoping. + per-request scoping. Each `require`s a published core version and is + pointed at the working tree by the root go.work — do NOT suggest adding + a `replace` directive; no go.mod in this repo has one. - path: "parsers/**" instructions: >- @@ -116,16 +118,37 @@ reviews: - path: "explain/**" instructions: >- - EXPLAIN must never execute the statement: keep the read-only - BeginTx + always-Rollback, never use ANALYZE, and keep validate()'s - comment/string-aware multi-statement rejection and SELECT/WITH-only (DML - behind WithAllowDML) policy. EXPLAIN takes no bind params, so - concatenation is by design — the defense is validate() + the rolled-back - read-only tx; do not "fix" it with parameterization. Deliberate - carve-out: explain keeps Result.Query (and the inner analyzer.Result - .Query of its findings) RAW — the user typed the query on their own CLI, - it never reaches a log/telemetry sink, and Fingerprint is still set. Do - NOT flag explain findings for not redacting Query; that is intended. + EXPLAIN must never execute the statement: keep the BeginTx + + always-Rollback, never use ANALYZE, and keep validate()'s comment/string- + aware multi-statement rejection and SELECT/WITH-only (DML behind + WithAllowDML) policy. The tx is read-only EXCEPT on MySQL/MariaDB for DML + (BeginTx ReadOnly: !dml): those servers reject every statement in a READ + ONLY tx with error 1792, including a planning-only EXPLAIN — do NOT + "restore" ReadOnly:true there, it breaks WithAllowDML outright. EXPLAIN + takes no bind params, so concatenation is by design — the defense is + validate() + the rolled-back tx; do not "fix" it with parameterization. + The MySQL plan is requested as `EXPLAIN FORMAT=TRADITIONAL` because + MySQL 9 defaults @@explain_format to TREE (one free-text column); do not + drop the clause. Plan columns are addressed BY NAME, never by position: + MariaDB emits 10 columns where MySQL emits 12. Rows whose `table` starts + with `<` are UNION RESULT / derived temporary tables and are skipped on + purpose. Deliberate carve-out: explain keeps Result.Query (and the inner + analyzer.Result.Query of its findings) RAW — the user typed the query on + their own CLI, it never reaches a log/telemetry sink, and Fingerprint is + still set. Do NOT flag explain findings for not redacting Query; that is + intended. + + - path: "test/integration/**" + instructions: >- + Integration tests for explain/ against live PostgreSQL, MySQL and + MariaDB, in their own unpublished module so the DB drivers stay out of + the core import graph. Everything is behind the `integration` build tag, + so `go test ./...` stays Docker-free — do not suggest removing the tag or + folding this into the core module. The tabular EXPLAIN output these pin + is version-dependent, which is precisely why they cannot be unit tests; + do not suggest replacing them with fixtures or mocks. Tests skip when + their DSN env var is unset. MySQL and MariaDB cases must run against both + servers (they share the "mysql" dialect but differ in EXPLAIN output). - path: "**/*_test.go" instructions: >- @@ -141,7 +164,7 @@ reviews: gitleaks: enabled: true # secret scanning — important for a security-adjacent tool yamllint: - enabled: true + enabled: true # also lints test/integration/docker-compose.yml actionlint: enabled: true # lints .github/workflows/ci.yml markdownlint: diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index de79eaa..cc14095 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,6 +28,9 @@ jobs: with: go-version: ${{ matrix.go-version }} + # The committed go.work makes the satellite steps below compile against + # this commit's core, not the published one, so a breaking change here + # fails them rather than passing CI silently. - name: Run tests run: go test ./... -count=1 -race @@ -55,6 +58,62 @@ jobs: - name: Test parsers (mysqlparser) run: cd parsers/mysqlparser && go test ./... -count=1 -race + # Exercises explain/ against live servers. The tabular EXPLAIN output it + # parses is version-dependent (MySQL 9 defaults @@explain_format to TREE; + # MariaDB emits 10 columns where MySQL emits 12), so this cannot be covered + # by unit tests. Service containers mirror test/integration/docker-compose.yml. + integration: + runs-on: ubuntu-latest + services: + postgres: + image: postgres:18.3-alpine + env: + POSTGRES_USER: sqlguard + POSTGRES_PASSWORD: sqlguard + POSTGRES_DB: sqlguard + ports: + - 55432:5432 + options: >- + --health-cmd "pg_isready -U sqlguard -d sqlguard" + --health-interval 2s + --health-timeout 5s + --health-retries 30 + mysql: + image: mysql:9.7.1 + env: + MYSQL_ROOT_PASSWORD: sqlguard + MYSQL_DATABASE: sqlguard + ports: + - 53306:3306 + options: >- + --health-cmd "mysqladmin ping -h 127.0.0.1 -uroot -psqlguard" + --health-interval 2s + --health-timeout 5s + --health-retries 30 + mariadb: + image: mariadb:12.3.2 + env: + MARIADB_ROOT_PASSWORD: sqlguard + MARIADB_DATABASE: sqlguard + ports: + - 53307:3306 + options: >- + --health-cmd "mariadb-admin ping -h 127.0.0.1 -uroot -psqlguard" + --health-interval 2s + --health-timeout 5s + --health-retries 30 + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + + - uses: actions/setup-go@v6 + with: + go-version: "1.26" + + - name: Run integration tests + run: make test-integration + lint: runs-on: ubuntu-latest strategy: diff --git a/.gitignore b/.gitignore index 1151f4d..9a2e4e9 100644 --- a/.gitignore +++ b/.gitignore @@ -24,9 +24,9 @@ sqlguard-website # Dependency directories (remove the comment below to include it) # vendor/ -# Go workspace file -go.work -go.work.sum +# go.work and go.work.sum are deliberately committed: every module in this repo +# lives here, and the workspace is what makes the satellites compile against +# this tree instead of the published core. Consumers never see it. # env file .env diff --git a/AGENTS.md b/AGENTS.md index 6e3eb10..879a84a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,8 @@ loop so a target can't silently skip a satellite. - `make lint` — `golangci-lint run` in every module (config in `.golangci.yml`, v2 schema). `make fmt` / `make fmt-check` run `gofmt -s` + `goimports`. - `make tidy` — `go mod tidy` across all nine modules. Run after any dependency change; tidying only the root leaves the others stale. - `make setup` — installs pinned `golangci-lint` / `goimports` if missing (a prereq of `lint`/`fmt`). -- `make release-prep VERSION=vX.Y.Z` — drops the local `replace` and pins each sub-module to the tagged parent version, then prints the per-module tag commands. +- The committed `go.work` makes every satellite compile against this tree, not the published core it `require`s — so a breaking change to `analyzer/`/`middleware/` fails their tests. No `go.mod` here has a `replace`. Use `GOWORK=off` to see a consumer's build. Releasing is manual (see CONTRIBUTING.md). +- `make db-up` / `make test-integration` / `make db-down` — run `explain/` against live Postgres, MySQL and MariaDB (`test/integration/`, behind the `integration` build tag). CI (`.github/workflows/ci.yml`) runs the root and each satellite module individually too. diff --git a/CHANGELOG.md b/CHANGELOG.md index 31cfd92..85c6851 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,50 @@ the same version in lockstep. ## [Unreleased] +## [0.1.1] - 2026-07-09 + +Fixes `explain` against current MySQL and MariaDB servers, where it previously +failed on every query. No public API changed. + +### Fixed + +- **`explain` on MySQL 9**: the plan is now requested as + `EXPLAIN FORMAT=TRADITIONAL`. MySQL 9 defaults `@@explain_format` to `TREE`, + which returns a single free-text column instead of the tabular plan, so every + `Analyze` call failed to scan. The clause is also accepted by MySQL 5.7/8 and + MariaDB, so no server version detection is needed. +- **`explain` on MariaDB**: plan columns are read by name rather than by + position. MariaDB emits ten columns where MySQL emits twelve (no + `partitions`, no `filtered`), which made the positional scan fail. +- **`WithAllowDML` on MySQL and MariaDB**: both reject *every* statement inside + a `READ ONLY` transaction (error 1792), including an `EXPLAIN` that only plans + it. The MySQL path now uses a regular transaction; safety still comes from + input validation, plain `EXPLAIN` never executing the statement, and the + transaction always being rolled back. PostgreSQL is unaffected and keeps its + read-only transaction. +- **False positive on `UNION`**: a `UNION RESULT` row names a temporary table + (``) with `type=ALL` and no key, and was reported as an unindexed + full table scan. Rows naming a derived or temporary table are now skipped. + +### Added + +- `test/integration`: an unpublished module that runs `explain` against live + PostgreSQL, MySQL and MariaDB (`make db-up && make test-integration`). The + tabular `EXPLAIN` output it parses is version-dependent, so no unit test can + guard these regressions. Kept as its own module so the database drivers stay + out of the core import graph, and behind an `integration` build tag so the + default `go test ./...` needs no Docker. + +### Changed + +- Local development now uses a committed `go.work` instead of per-module + `replace` directives. Without it the satellite modules compiled against the + *published* core even in CI, so a breaking change to `analyzer/` or + `middleware/` could pass a green build. Consumers are unaffected; use + `GOWORK=off` to reproduce a consumer's build. `make release-prep` and its + `replace`-restoring dance are gone — releasing is documented in + `CONTRIBUTING.md`. + ## [0.1.0] - 2026-06-08 Initial public release. @@ -57,5 +101,6 @@ Initial public release. `integrations/sqlxguard`, `integrations/pgxguard` (native pgx / pgxpool), `integrations/bunguard`, `integrations/xormguard`, `integrations/entguard`. -[Unreleased]: https://github.com/KARTIKrocks/sqlguard/compare/v0.1.0...HEAD +[Unreleased]: https://github.com/KARTIKrocks/sqlguard/compare/v0.1.1...HEAD +[0.1.1]: https://github.com/KARTIKrocks/sqlguard/compare/v0.1.0...v0.1.1 [0.1.0]: https://github.com/KARTIKrocks/sqlguard/releases/tag/v0.1.0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6d8f201..9f9025b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,8 +15,15 @@ lockstep: - **`integrations/{gormguard,sqlxguard,pgxguard,bunguard,xormguard,entguard}`** — ORM/driver adapters, each a separate module so its deps stay opt-in. -The satellite modules use a local `replace` directive pointing at the root, so -you can develop across modules without publishing. +Every satellite `require`s a published version of the core. The committed +`go.work` at the repo root overrides that with the working tree, so you can +develop across modules without publishing — and so a breaking change to +`analyzer/` or `middleware/` fails the satellite tests instead of passing CI +against a stale published core. + +No `go.mod` in this repo carries a `replace` directive; `go.work` is the only +place local resolution is configured. It is ignored entirely when someone +depends on these modules. To reproduce a consumer's build, set `GOWORK=off`. > **Important:** `go test ./...` (and `go build` / `go vet` / `go mod tidy`) > from the root does **not** reach the satellite modules. Always use the @@ -40,6 +47,51 @@ Before opening a PR, run `make ci` and make sure it's green. - After any dependency change, run `make tidy` (tidies all nine modules — tidying only the root leaves the others stale). +## Integration tests + +`explain/` parses real `EXPLAIN` output, whose shape varies by server and +version — MySQL 9 defaults `@@explain_format` to `TREE`, and MariaDB emits ten +plan columns where MySQL emits twelve. No unit test can catch a regression +there, so `test/integration/` runs the analyzer against live databases: + +```bash +make db-up # postgres 18, mysql 9.7, mariadb 12.3 via docker compose +make test-integration +make db-down # stop and drop the volumes +``` + +The tests sit behind the `integration` build tag, so `make test` and `make ci` +never need Docker. Each database's tests skip when its DSN environment variable +is unset (`SQLGUARD_TEST_PG_DSN`, `SQLGUARD_TEST_MYSQL_DSN`, +`SQLGUARD_TEST_MARIADB_DSN`), so a partial stack still runs what it can — and +you can point any of them at your own server by overriding the variable. + +`test/integration/` is a tenth module that is never published: it exists to keep +the Postgres and MySQL drivers out of the core module's import graph. + +## Releasing + +Tag the root module first, then point each satellite at that tag. Everything +here is safe to commit to `main` — `go.work` keeps local builds on the working +tree no matter which core version the satellites require. + +```bash +git tag vX.Y.Z && git push origin vX.Y.Z # root module first + +for mod in integrations/* parsers/*; do + (cd "$mod" && go mod edit -require github.com/KARTIKrocks/sqlguard@vX.Y.Z) +done +make tidy && make test + +git commit -am 'Pin sub-modules to vX.Y.Z' +for mod in integrations/* parsers/*; do git tag "$mod/vX.Y.Z"; done +git push origin --tags +``` + +Skip `test/integration/` — it is not published. Before tagging, it is worth +running `GOWORK=off make test` once: that is what a consumer's build sees, and +it will catch a satellite whose required core version predates an API it uses. + ## Conventions - **Pre-1.0, no backward-compatibility burden.** Prefer the clean design over diff --git a/Makefile b/Makefile index d205223..6c618da 100644 --- a/Makefile +++ b/Makefile @@ -1,11 +1,10 @@ GOLANGCI_LINT_VERSION := v2.12.2 GOIMPORTS_VERSION := v0.45.0 -MODULE_PATH := github.com/KARTIKrocks/sqlguard - # Sub-modules carry their own go.mod (heavy/opt-in deps kept out of the core # import graph). `go test ./...` from root does NOT reach them, so every -# all-modules target loops over MODULES. +# all-modules target loops over MODULES. They compile against this tree rather +# than the published core because go.work lists them all. SUB_MODULES = \ ./integrations/gormguard \ ./integrations/sqlxguard \ @@ -17,7 +16,19 @@ SUB_MODULES = \ ./parsers/mysqlparser MODULES = . $(SUB_MODULES) -.PHONY: all help setup deps ci test test-v test-race coverage lint lint-fix fix fmt fmt-check vet tidy build cli install bench clean release-prep +# The integration module is never released, so it is not in SUB_MODULES. Its +# tests sit behind the `integration` build tag and need the compose stack +# below, so it is also excluded from the plain vet/test loops. +INTEGRATION_MODULE := ./test/integration +COMPOSE_FILE := test/integration/docker-compose.yml + +# Override to point the integration tests at your own servers. An unset DSN +# skips that database's tests rather than failing them. +SQLGUARD_TEST_PG_DSN ?= postgres://sqlguard:sqlguard@localhost:55432/sqlguard?sslmode=disable +SQLGUARD_TEST_MYSQL_DSN ?= root:sqlguard@tcp(localhost:53306)/sqlguard +SQLGUARD_TEST_MARIADB_DSN ?= root:sqlguard@tcp(localhost:53307)/sqlguard + +.PHONY: all help setup deps ci test test-v test-race coverage lint lint-fix fix fmt fmt-check vet tidy build cli install bench clean db-up db-down test-integration vet-integration all: tidy fmt vet lint build test @@ -31,6 +42,9 @@ help: @echo " test - Run tests across all modules" @echo " test-v - Run tests with verbose output (all modules)" @echo " test-race - Run tests with race detector (all modules)" + @echo " db-up - Start the integration-test databases (docker compose)" + @echo " db-down - Stop the integration-test databases and drop volumes" + @echo " test-integration - Run explain/ tests against the live databases" @echo " coverage - Run tests with merged coverage report (all modules)" @echo " vet - Run go vet (all modules)" @echo " lint - Run golangci-lint (all modules)" @@ -44,7 +58,6 @@ help: @echo " install - Install the CLI to \$$GOPATH/bin" @echo " bench - Run benchmarks (all modules)" @echo " clean - Remove build/coverage artifacts" - @echo " release-prep - Pin sub-modules to a release version (VERSION=vX.Y.Z)" ## Install development tools (skips if already present) setup: @@ -104,6 +117,29 @@ test-race: (cd $$mod && go test -race -count=1 ./...) || exit 1; \ done +## Start the integration-test databases and wait for them to become healthy +db-up: + docker compose -f $(COMPOSE_FILE) up -d --wait + +## Stop the integration-test databases and remove their volumes +db-down: + docker compose -f $(COMPOSE_FILE) down -v + +## Run the explain/ integration tests against live Postgres, MySQL and MariaDB. +## Requires `make db-up` first. Each database's tests skip if its DSN is unset. +test-integration: + @echo "==> Integration testing $(INTEGRATION_MODULE)" + @cd $(INTEGRATION_MODULE) && \ + SQLGUARD_TEST_PG_DSN='$(SQLGUARD_TEST_PG_DSN)' \ + SQLGUARD_TEST_MYSQL_DSN='$(SQLGUARD_TEST_MYSQL_DSN)' \ + SQLGUARD_TEST_MARIADB_DSN='$(SQLGUARD_TEST_MARIADB_DSN)' \ + go test -tags=integration -count=1 -race ./... + +## Vet the integration module (needs the build tag to see any files) +vet-integration: + @echo "==> Vetting $(INTEGRATION_MODULE)" + @cd $(INTEGRATION_MODULE) && go vet -tags=integration ./... + ## Run tests with coverage and generate a merged report across all modules coverage: @echo "mode: atomic" > coverage.out @@ -166,29 +202,3 @@ clean: @find . -name cover.tmp -delete 2>/dev/null || true @rm -rf dist/ build/ bin/ -## Prepare sub-modules for release: drop the local replace and pin the parent -## version. Usage: make release-prep VERSION=v0.1.0 -## Run this AFTER the root module tag for VERSION exists and is pushed, then -## commit and tag the sub-modules. Restore replaces afterwards for local dev -## (git checkout -- '**/go.mod') or develop against the published version. -release-prep: -ifndef VERSION - $(error VERSION is required. Usage: make release-prep VERSION=v0.1.0) -endif - @for mod in $(SUB_MODULES); do \ - echo "==> release-prep $$mod"; \ - (cd $$mod && go mod edit -dropreplace $(MODULE_PATH) -require $(MODULE_PATH)@$(VERSION)) || exit 1; \ - done - @echo "" - @echo "Done! Sub-modules now require $(MODULE_PATH)@$(VERSION) (replace dropped)." - @echo "Next steps (root tag $(VERSION) must already be pushed):" - @echo " git add -A && git commit -m 'Prepare release $(VERSION)'" - @echo " git tag integrations/gormguard/$(VERSION)" - @echo " git tag integrations/sqlxguard/$(VERSION)" - @echo " git tag integrations/pgxguard/$(VERSION)" - @echo " git tag integrations/bunguard/$(VERSION)" - @echo " git tag integrations/xormguard/$(VERSION)" - @echo " git tag integrations/entguard/$(VERSION)" - @echo " git tag parsers/pgparser/$(VERSION)" - @echo " git tag parsers/mysqlparser/$(VERSION)" - @echo " git push origin main --tags" diff --git a/README.md b/README.md index d4f2b00..744ed65 100644 --- a/README.md +++ b/README.md @@ -255,13 +255,23 @@ sqlguard explain --db "..." --format json "SELECT * FROM orders" Detects sequential scans, missing indexes, filesort, and high-cost operations. -For safety the EXPLAIN runs inside a **read-only transaction that is always -rolled back** (Postgres and MySQL), and `ANALYZE` is never used — the -statement is planned, never executed. Input is validated with a -comment- and string-literal-aware multi-statement check (a `;` hidden in a -comment or string can't smuggle a second statement). Only `SELECT`/`WITH` is -allowed by default; pass `--allow-dml` to EXPLAIN an `INSERT/UPDATE/DELETE` -(still rolled back). DDL/`SET`/transaction-control is always refused. +For safety the EXPLAIN runs inside a **transaction that is always rolled back**, +and `ANALYZE` is never used — the statement is planned, never executed. Input +is validated with a comment- and string-literal-aware multi-statement check (a +`;` hidden in a comment or string can't smuggle a second statement). Only +`SELECT`/`WITH` is allowed by default; pass `--allow-dml` to EXPLAIN an +`INSERT/UPDATE/DELETE` (still rolled back). DDL/`SET`/transaction-control is +always refused. + +The transaction is additionally **read-only** everywhere except MySQL/MariaDB +under `--allow-dml`: those servers reject *every* statement in a read-only +transaction (error 1792), including an EXPLAIN that only plans it. There the +guarantee rests on the validation, on plain `EXPLAIN` never executing the +statement, and on the unconditional rollback. + +MariaDB works through `--dialect mysql`. The MySQL plan is requested as +`EXPLAIN FORMAT=TRADITIONAL`, since MySQL 9 defaults `@@explain_format` to +`TREE`. ### GORM Integration @@ -509,7 +519,7 @@ per-request scoping. constant concatenation, and `fmt.Sprintf` with a constant format string (via `go/types`); it cannot resolve values only known at runtime. - The default fallback parser is best-effort; for exact structural analysis use a real parser module (see _SQL Parsers_ above) -- EXPLAIN analyzer requires a live database connection; only Postgres and MySQL dialects are supported +- EXPLAIN analyzer requires a live database connection; only the `postgres` and `mysql` dialects are supported (`mysql` also covers MariaDB) ## License diff --git a/explain/explain.go b/explain/explain.go index fa178d0..c9af9f5 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -8,6 +8,7 @@ import ( "database/sql" "encoding/json" "fmt" + "strconv" "strings" "github.com/KARTIKrocks/sqlguard/analyzer" @@ -62,7 +63,7 @@ type Result struct { // always-rolled-back transaction, so a query passed here cannot mutate the // target database. func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, error) { - safe, err := p.validate(query) + safe, kind, err := p.validate(query) if err != nil { return nil, err } @@ -72,7 +73,7 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro case "postgres": res, err = p.analyzePostgres(ctx, safe) case "mysql": - res, err = p.analyzeMySQL(ctx, safe) + res, err = p.analyzeMySQL(ctx, safe, isDML(kind)) default: return nil, fmt.Errorf("explain: unsupported dialect %q", p.dialect) } @@ -102,27 +103,37 @@ func (p *PlanAnalyzer) Analyze(ctx context.Context, query string) (*Result, erro // - Classify the statement via the same parser the analyzer uses. Only // SELECT/WITH are allowed by default; INSERT/UPDATE/DELETE require // WithAllowDML; DDL/SET/other is always refused. -func (p *PlanAnalyzer) validate(query string) (string, error) { +func (p *PlanAnalyzer) validate(query string) (string, analyzer.StmtKind, error) { q := strings.TrimSpace(query) if q == "" { - return "", fmt.Errorf("explain: refusing to explain an empty query") + return "", analyzer.StmtUnknown, fmt.Errorf("explain: refusing to explain an empty query") } if analyzer.IsMultiStatement(q) { - return "", fmt.Errorf("explain: refusing to explain multi-statement input") + return "", analyzer.StmtUnknown, fmt.Errorf("explain: refusing to explain multi-statement input") } q = strings.TrimRight(q, "; \t\r\n") st, _ := analyzer.NewFallbackParser().Parse(q) switch st.Kind { case analyzer.StmtSelect: - return q, nil + return q, st.Kind, nil case analyzer.StmtInsert, analyzer.StmtUpdate, analyzer.StmtDelete: if !p.allowDML { - return "", fmt.Errorf("explain: refusing to EXPLAIN a data-modifying statement by default; construct the analyzer with explain.WithAllowDML to opt in") + return "", st.Kind, fmt.Errorf("explain: refusing to EXPLAIN a data-modifying statement by default; construct the analyzer with explain.WithAllowDML to opt in") } - return q, nil + return q, st.Kind, nil default: - return "", fmt.Errorf("explain: refusing to explain a non-SELECT/WITH/DML statement (DDL, SET, transaction control, or unrecognized)") + return "", st.Kind, fmt.Errorf("explain: refusing to explain a non-SELECT/WITH/DML statement (DDL, SET, transaction control, or unrecognized)") + } +} + +// isDML reports whether kind is a data-modifying statement. +func isDML(kind analyzer.StmtKind) bool { + switch kind { + case analyzer.StmtInsert, analyzer.StmtUpdate, analyzer.StmtDelete: + return true + default: + return false } } @@ -215,17 +226,27 @@ func (p *PlanAnalyzer) walkPgPlan(node *pgPlanNode, query string, issues *[]anal } } -func (p *PlanAnalyzer) analyzeMySQL(ctx context.Context, query string) (*Result, error) { +func (p *PlanAnalyzer) analyzeMySQL(ctx context.Context, query string, dml bool) (*Result, error) { // See analyzePostgres: validated single statement, no ANALYZE, run in an - // always-rolled-back read-only transaction so EXPLAIN cannot mutate data. + // always-rolled-back transaction so EXPLAIN cannot mutate data. + // + // FORMAT=TRADITIONAL pins the tabular plan. MySQL 9 defaults + // @@explain_format to TREE, which yields one free-text column and no + // per-table rows to inspect; the clause restores the classic output and is + // also accepted by MySQL 5.7/8 and MariaDB. + // //nolint:gosec // G202: EXPLAIN takes no bind params, so concatenation is by - // design; the defense is validate() + the rolled-back read-only tx, not + // design; the defense is validate() + the rolled-back tx, not // parameterization. - explainQuery := "EXPLAIN " + query + explainQuery := "EXPLAIN FORMAT=TRADITIONAL " + query - tx, err := p.db.BeginTx(ctx, &sql.TxOptions{ReadOnly: true}) + // MySQL and MariaDB reject *any* statement inside a READ ONLY transaction + // (error 1792), including an EXPLAIN that only plans it. Fall back to a + // regular transaction for DML: plain EXPLAIN never executes the statement, + // and the rollback below is what guarantees nothing is committed. + tx, err := p.db.BeginTx(ctx, &sql.TxOptions{ReadOnly: !dml}) if err != nil { - return nil, fmt.Errorf("explain: failed to begin read-only transaction: %w", err) + return nil, fmt.Errorf("explain: failed to begin transaction: %w", err) } defer func() { _ = tx.Rollback() }() @@ -235,62 +256,37 @@ func (p *PlanAnalyzer) analyzeMySQL(ctx context.Context, query string) (*Result, } defer func() { _ = rows.Close() }() + // MySQL emits 12 columns, MariaDB 10 (no `partitions`, no `filtered`). + // Address them by name so a differing column set is not a scan error. + cols, err := rows.Columns() + if err != nil { + return nil, fmt.Errorf("explain: failed to read EXPLAIN columns: %w", err) + } + index := make(map[string]int, len(cols)) + for i, c := range cols { + index[strings.ToLower(c)] = i + } + cells := make([]sql.NullString, len(cols)) + scanArgs := make([]any, len(cols)) + for i := range cells { + scanArgs[i] = &cells[i] + } + col := func(name string) string { + if i, ok := index[name]; ok { + return cells[i].String + } + return "" + } + result := &Result{ Query: query, } for rows.Next() { - var ( - id int - selectType string - table sql.NullString - partitions sql.NullString - accessType sql.NullString - possibleKeys sql.NullString - key sql.NullString - keyLen sql.NullString - ref sql.NullString - rowCount sql.NullInt64 - filtered sql.NullFloat64 - extra sql.NullString - ) - - if err := rows.Scan(&id, &selectType, &table, &partitions, &accessType, &possibleKeys, &key, &keyLen, &ref, &rowCount, &filtered, &extra); err != nil { + if err := rows.Scan(scanArgs...); err != nil { return result, fmt.Errorf("explain: failed to scan row: %w", err) } - - // Detect full table scans (type = ALL) - if accessType.Valid && accessType.String == "ALL" { - result.Issues = append(result.Issues, analyzer.Result{ - RuleName: "full-table-scan", - Severity: analyzer.SeverityWarning, - Query: query, - Message: fmt.Sprintf("Full table scan on %s (estimated %d rows)", table.String, rowCount.Int64), - Suggestion: "Consider adding an index to avoid full table scan.", - }) - } - - // Detect missing indexes - if (!key.Valid || key.String == "") && (!possibleKeys.Valid || possibleKeys.String == "") && table.Valid && table.String != "" { - result.Issues = append(result.Issues, analyzer.Result{ - RuleName: "no-index-used", - Severity: analyzer.SeverityWarning, - Query: query, - Message: fmt.Sprintf("No index used on table %s", table.String), - Suggestion: "Consider adding an index on the filtered/joined columns.", - }) - } - - // Detect filesort - if strings.Contains(extra.String, "Using filesort") { - result.Issues = append(result.Issues, analyzer.Result{ - RuleName: "filesort", - Severity: analyzer.SeverityInfo, - Query: query, - Message: fmt.Sprintf("Filesort detected on table %s", table.String), - Suggestion: "Consider adding an index that covers the ORDER BY columns.", - }) - } + result.Issues = append(result.Issues, mysqlRowIssues(query, col)...) } if err := rows.Err(); err != nil { @@ -299,3 +295,53 @@ func (p *PlanAnalyzer) analyzeMySQL(ctx context.Context, query string) (*Result, return result, nil } + +// mysqlRowIssues applies the plan rules to a single EXPLAIN row. col resolves a +// lower-cased column name to its value, or "" when the server does not emit it. +func mysqlRowIssues(query string, col func(string) string) []analyzer.Result { + // A UNION RESULT or derived-table row names a temporary table such as + // ``, never a real one. It has no index by construction, so the + // rules below would only produce noise. + table := col("table") + if table == "" || strings.HasPrefix(table, "<") { + return nil + } + + var issues []analyzer.Result + + // Detect full table scans (type = ALL) + if col("type") == "ALL" { + planRows, _ := strconv.ParseInt(col("rows"), 10, 64) + issues = append(issues, analyzer.Result{ + RuleName: "full-table-scan", + Severity: analyzer.SeverityWarning, + Query: query, + Message: fmt.Sprintf("Full table scan on %s (estimated %d rows)", table, planRows), + Suggestion: "Consider adding an index to avoid full table scan.", + }) + } + + // Detect missing indexes + if col("key") == "" && col("possible_keys") == "" { + issues = append(issues, analyzer.Result{ + RuleName: "no-index-used", + Severity: analyzer.SeverityWarning, + Query: query, + Message: fmt.Sprintf("No index used on table %s", table), + Suggestion: "Consider adding an index on the filtered/joined columns.", + }) + } + + // Detect filesort + if strings.Contains(col("extra"), "Using filesort") { + issues = append(issues, analyzer.Result{ + RuleName: "filesort", + Severity: analyzer.SeverityInfo, + Query: query, + Message: fmt.Sprintf("Filesort detected on table %s", table), + Suggestion: "Consider adding an index that covers the ORDER BY columns.", + }) + } + + return issues +} diff --git a/explain/explain_test.go b/explain/explain_test.go index d251bcb..84bc776 100644 --- a/explain/explain_test.go +++ b/explain/explain_test.go @@ -31,7 +31,7 @@ func TestValidate(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { p := &PlanAnalyzer{dialect: "postgres", allowDML: c.allowDML} - safe, err := p.validate(c.query) + safe, _, err := p.validate(c.query) if c.wantErr == "" { if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/go.work b/go.work new file mode 100644 index 0000000..6169f1f --- /dev/null +++ b/go.work @@ -0,0 +1,32 @@ +// Every module in this repository, wired together for local development. +// +// The satellites (integrations/*, parsers/*) each `require` a published +// version of the core. Without this workspace they would compile against that +// published version even here, so a breaking change to analyzer/ or +// middleware/ would pass CI while silently breaking every adapter. The +// workspace overrides that requirement with the working tree. +// +// No module in this repo carries a `replace` directive; this file is the only +// place local resolution is configured. It is committed so CI uses it too, and +// it is ignored entirely when someone depends on any of these modules. +// +// Releasing bumps each satellite's `require` to the new core tag (see +// CONTRIBUTING.md). That stays on main safely, precisely because the workspace +// still points local builds at the working tree. +// +// To reproduce a consumer's build instead, set GOWORK=off. + +go 1.26 + +use ( + . + ./integrations/bunguard + ./integrations/entguard + ./integrations/gormguard + ./integrations/pgxguard + ./integrations/sqlxguard + ./integrations/xormguard + ./parsers/mysqlparser + ./parsers/pgparser + ./test/integration +) diff --git a/go.work.sum b/go.work.sum new file mode 100644 index 0000000..8a042cc --- /dev/null +++ b/go.work.sum @@ -0,0 +1,58 @@ +ariga.io/atlas v0.36.2-0.20250730182955-2c6300d0a3e1/go.mod h1:Ex5l1xHsnWQUc3wYnrJ9gD7RUEzG76P7ZRQp8wNr0wc= +gitee.com/travelliu/dm v1.8.11192/go.mod h1:DHTzyhCrM843x9VdKVbZ+GKXGRbKM2sJ4LxihRxShkE= +github.com/agext/levenshtein v1.2.3/go.mod h1:JEDfjyjHDjOF/1e4FlBE/PkbqA9OfWu2ki2W0IB5558= +github.com/apparentlymart/go-textseg/v15 v15.0.0/go.mod h1:K8XmNZdhEBkdlyDdvbmmsvpAG721bKi0joRfFdHIWJ4= +github.com/bmatcuk/doublestar v1.3.4/go.mod h1:wiQtGV+rzVYxB7WIlirSN++5HPtPlXEo9MEoZQC/PmE= +github.com/clipperhouse/displaywidth v0.6.2/go.mod h1:R+kHuzaYWFkTm7xoMmK1lFydbci4X2CicfbGstSGg0o= +github.com/clipperhouse/stringish v0.1.1/go.mod h1:v/WhFtE1q0ovMta2+m+UbpZ+2/HEXNWYXQgCt4hdOzA= +github.com/clipperhouse/uax29/v2 v2.3.0/go.mod h1:Wn1g7MK6OoeDT0vL+Q0SQLDz/KpfsVRgg6W7ihQeh4g= +github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU= +github.com/go-openapi/inflect v0.19.0/go.mod h1:lHpZVlpIQqLyKwJ4N+YSc9hchQy/i12fJykb83CRBH4= +github.com/golang-sql/civil v0.0.0-20220223132316-b832511892a9/go.mod h1:8vg3r2VgvsThLBIFL93Qb5yWzgyZWhEmBwUJWevAkK0= +github.com/golang-sql/sqlexp v0.1.0/go.mod h1:J4ad9Vo8ZCWQ2GMrC4UCQy1JpCbwU9m3EOqtpKwwwHI= +github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= +github.com/gorilla/websocket v1.5.0/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= +github.com/hashicorp/hcl/v2 v2.18.1/go.mod h1:ThLC89FV4p9MPW804KVbe/cEXoQ8NZEh+JtMeeGErHE= +github.com/jessevdk/go-flags v1.5.0/go.mod h1:Fw0T6WPc1dYxT4mKEZRfG5kJhaTDP9pj1c2EWnYs/m4= +github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= +github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= +github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= +github.com/mattn/go-colorable v0.1.14/go.mod h1:6LmQG8QLFO4G5z1gPvYEzlUgJ2wF+stgPZH1UqBm1s8= +github.com/mattn/go-runewidth v0.0.19/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= +github.com/microsoft/go-mssqldb v1.8.2/go.mod h1:vp38dT33FGfVotRiTmDo3bFyaHq+p3LektQrjTULowo= +github.com/mitchellh/go-wordwrap v1.0.1/go.mod h1:R62XHJLzvMFRBbcrT7m7WgmE1eOyTSsCt+hzestvNj0= +github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= +github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/olekukonko/cat v0.0.0-20250911104152-50322a0618f6/go.mod h1:rEKTHC9roVVicUIfZK7DYrdIoM0EOr8mK1Hj5s3JjH0= +github.com/olekukonko/errors v1.1.0/go.mod h1:ppzxA5jBKcO1vIpCXQ9ZqgDh8iwODz6OXIGKU8r5m4Y= +github.com/olekukonko/ll v0.1.4-0.20260115111900-9e59c2286df0/go.mod h1:b52bVQRRPObe+yyBl0TxNfhesL0nedD4Cht0/zx55Ew= +github.com/olekukonko/tablewriter v1.1.3/go.mod h1:9VU0knjhmMkXjnMKrZ3+L2JhhtsQ/L38BbL3CRNE8tM= +github.com/rogpeppe/go-internal v1.6.1 h1:/FiVV8dS/e+YqF2JvO3yXRFbBLTIuSDkuC7aBOAvL+k= +github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= +github.com/rs/zerolog v1.34.0/go.mod h1:bJsvje4Z08ROH4Nhs5iH600c3IkWhwp44iRc54W6wYQ= +github.com/sergi/go-diff v1.3.1/go.mod h1:aMJSSKb2lpPvRNec0+w3fl7LP9IOFzdc9Pa4NFbPK1I= +github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +github.com/zclconf/go-cty v1.14.4/go.mod h1:VvMs5i0vgZdhYawQNq5kePSpLAoz8u1xvZgrPIxfnZE= +github.com/zclconf/go-cty-yaml v1.1.0/go.mod h1:9YLUH4g7lOhVWqUbctnVlZ5KLpg7JAprQNgxSZ1Gyxs= +github.com/ziutek/mymysql v1.5.4/go.mod h1:LMSpPZ6DbqWFxNCHW77HeMg9I646SAhApZ/wKdgO/C0= +go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo= +golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= +golang.org/x/mod v0.27.0/go.mod h1:rWI627Fq0DEoudcK+MBkNkCe0EetEaDSwJJkCcjpazc= +golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= +golang.org/x/sync v0.17.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/telemetry v0.0.0-20260625142307-59b4966ccb57/go.mod h1:3AWMyWHS+caVoiEXpiq6+tzKA40J4vQT3MYr80ZtQpc= +golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= +golang.org/x/text v0.38.0 h1:sXmwo9DwP3OK9EZ7PqAdaooSGozfl/3a6/xJcbzPRhE= +golang.org/x/text v0.38.0/go.mod h1:YXZt3QhHUKYT53r2lLKFIVi6Ao1jdzrTR/KQ09qyxF4= +golang.org/x/tools v0.36.0/go.mod h1:WBDiHKJK8YgLHlcQPYQzNCkUxUypCaa5ZegCVutKm+s= +golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= +gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= +gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= diff --git a/integrations/gormguard/go.mod b/integrations/gormguard/go.mod index e4be0db..84a9264 100644 --- a/integrations/gormguard/go.mod +++ b/integrations/gormguard/go.mod @@ -11,6 +11,6 @@ require ( require ( github.com/jinzhu/inflection v1.0.0 // indirect github.com/jinzhu/now v1.1.5 // indirect - github.com/mattn/go-sqlite3 v1.14.45 // indirect + github.com/mattn/go-sqlite3 v1.14.47 // indirect golang.org/x/text v0.20.0 // indirect ) diff --git a/integrations/gormguard/go.sum b/integrations/gormguard/go.sum index 85a0d98..80232ac 100644 --- a/integrations/gormguard/go.sum +++ b/integrations/gormguard/go.sum @@ -4,8 +4,8 @@ github.com/jinzhu/inflection v1.0.0 h1:K317FqzuhWc8YvSVlFMCCUb36O/S9MCKRDI7QkRKD github.com/jinzhu/inflection v1.0.0/go.mod h1:h+uFLlag+Qp1Va5pdKtLDYj+kHp5pxUVkryuEj+Srlc= github.com/jinzhu/now v1.1.5 h1:/o9tlHleP7gOFmsnYNz3RGnqzefHA47wQpKrrdTIwXQ= github.com/jinzhu/now v1.1.5/go.mod h1:d3SSVoowX0Lcu0IBviAWJpolVfI5UJVZZ7cO71lE/z8= -github.com/mattn/go-sqlite3 v1.14.45 h1:6KA/spDguL3KV8rnybG7ezSaE4SeMR3KC9VbUoAQaIk= -github.com/mattn/go-sqlite3 v1.14.45/go.mod h1:pjEuOr8IwzLJP2MfGeTb0A35jauH+C2kbHKBr7yXKVQ= +github.com/mattn/go-sqlite3 v1.14.47 h1:jOBI62gS7nKeZv+as1oGEy0+1qISgXwH/QBlR6KbfIo= +github.com/mattn/go-sqlite3 v1.14.47/go.mod h1:6JTjA44L93a0QCyJef5YvlPoKXntQPjzWv5gtm9sB6w= golang.org/x/text v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug= golang.org/x/text v0.20.0/go.mod h1:D4IsuqiFMhST5bX19pQ9ikHC2GsaKyk/oF+pn3ducp4= gorm.io/driver/sqlite v1.6.0 h1:WHRRrIiulaPiPFmDcod6prc4l2VGVWHz80KspNsxSfQ= diff --git a/integrations/pgxguard/go.mod b/integrations/pgxguard/go.mod index 7477400..fe6e49e 100644 --- a/integrations/pgxguard/go.mod +++ b/integrations/pgxguard/go.mod @@ -11,6 +11,6 @@ require ( github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect - golang.org/x/sync v0.20.0 // indirect + golang.org/x/sync v0.21.0 // indirect golang.org/x/text v0.29.0 // indirect ) diff --git a/integrations/pgxguard/go.sum b/integrations/pgxguard/go.sum index 7dbec39..ed42667 100644 --- a/integrations/pgxguard/go.sum +++ b/integrations/pgxguard/go.sum @@ -20,8 +20,8 @@ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UV github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/text v0.29.0 h1:1neNs90w9YzJ9BocxfsQNHKuAT4pkghyXc4nhZ6sJvk= golang.org/x/text v0.29.0/go.mod h1:7MhJOA9CD2qZyOKYazxdYMF85OwPdEr9jTtBpO7ydH4= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/parsers/pgparser/go.mod b/parsers/pgparser/go.mod index b8c68b1..90b8e88 100644 --- a/parsers/pgparser/go.mod +++ b/parsers/pgparser/go.mod @@ -24,7 +24,7 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/sirupsen/logrus v1.6.0 // indirect github.com/spf13/pflag v1.0.10 // indirect - golang.org/x/sync v0.20.0 // indirect + golang.org/x/sync v0.21.0 // indirect golang.org/x/sys v0.0.0-20201214210602-f9fddec55a1e // indirect golang.org/x/text v0.3.4 // indirect google.golang.org/genproto v0.0.0-20200911024640-645f7a48b24f // indirect diff --git a/parsers/pgparser/go.sum b/parsers/pgparser/go.sum index 8708db8..2505f95 100644 --- a/parsers/pgparser/go.sum +++ b/parsers/pgparser/go.sum @@ -260,8 +260,8 @@ golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20181205085412-a5c9d58dba9a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/test/integration/docker-compose.yml b/test/integration/docker-compose.yml new file mode 100644 index 0000000..7d69aff --- /dev/null +++ b/test/integration/docker-compose.yml @@ -0,0 +1,49 @@ +# Databases for the explain/ integration tests. +# +# docker compose -f test/integration/docker-compose.yml up -d --wait +# make test-integration +# docker compose -f test/integration/docker-compose.yml down -v +# +# Host ports are deliberately non-default so this never collides with a +# database you already run locally. + +services: + postgres: + image: postgres:18.3-alpine + environment: + POSTGRES_USER: sqlguard + POSTGRES_PASSWORD: sqlguard + POSTGRES_DB: sqlguard + ports: + - "55432:5432" + healthcheck: + test: ["CMD-SHELL", "pg_isready -U sqlguard -d sqlguard"] + interval: 2s + timeout: 5s + retries: 30 + + mysql: + image: mysql:9.7.1 + environment: + MYSQL_ROOT_PASSWORD: sqlguard + MYSQL_DATABASE: sqlguard + ports: + - "53306:3306" + healthcheck: + test: ["CMD", "mysqladmin", "ping", "-h", "127.0.0.1", "-uroot", "-psqlguard"] + interval: 2s + timeout: 5s + retries: 30 + + mariadb: + image: mariadb:12.3.2 + environment: + MARIADB_ROOT_PASSWORD: sqlguard + MARIADB_DATABASE: sqlguard + ports: + - "53307:3306" + healthcheck: + test: ["CMD", "mariadb-admin", "ping", "-h", "127.0.0.1", "-uroot", "-psqlguard"] + interval: 2s + timeout: 5s + retries: 30 diff --git a/test/integration/go.mod b/test/integration/go.mod new file mode 100644 index 0000000..5168412 --- /dev/null +++ b/test/integration/go.mod @@ -0,0 +1,25 @@ +// Integration tests for the explain/ package against live databases. +// +// This module is never published or tagged: it exists so the Postgres and +// MySQL drivers stay out of the core module's import graph. Like every module +// here it carries no `replace` directive — the root go.work points it at the +// working tree. +module github.com/KARTIKrocks/sqlguard/test/integration + +go 1.26 + +require ( + github.com/KARTIKrocks/sqlguard v0.1.0 + github.com/go-sql-driver/mysql v1.9.3 + github.com/jackc/pgx/v5 v5.7.6 +) + +require ( + filippo.io/edwards25519 v1.1.0 // indirect + github.com/jackc/pgpassfile v1.0.0 // indirect + github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect + github.com/jackc/puddle/v2 v2.2.2 // indirect + golang.org/x/crypto v0.37.0 // indirect + golang.org/x/sync v0.21.0 // indirect + golang.org/x/text v0.24.0 // indirect +) diff --git a/test/integration/go.sum b/test/integration/go.sum new file mode 100644 index 0000000..baba15c --- /dev/null +++ b/test/integration/go.sum @@ -0,0 +1,34 @@ +filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA= +filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4= +github.com/KARTIKrocks/sqlguard v0.1.0 h1:1rELGKkh30y2UvkmyKCK0veiFe3Jr66+RGbEsAIOofk= +github.com/KARTIKrocks/sqlguard v0.1.0/go.mod h1:Y9zX2L9OSqMXa1gx9c4mMKPTDEsArvbtYmzKpAwWPv8= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-sql-driver/mysql v1.9.3 h1:U/N249h2WzJ3Ukj8SowVFjdtZKfu9vlLZxjPXV1aweo= +github.com/go-sql-driver/mysql v1.9.3/go.mod h1:qn46aNg1333BRMNU69Lq93t8du/dwxI64Gl8i5p1WMU= +github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= +github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= +github.com/jackc/pgx/v5 v5.7.6 h1:rWQc5FwZSPX58r1OQmkuaNicxdmExaEz5A2DO2hUuTk= +github.com/jackc/pgx/v5 v5.7.6/go.mod h1:aruU7o91Tc2q2cFp5h4uP3f6ztExVpyVv88Xl/8Vl8M= +github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= +github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= +github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= +golang.org/x/crypto v0.37.0 h1:kJNSjF/Xp7kU0iB2Z+9viTPMW4EqqsrywMXLJOOsXSE= +golang.org/x/crypto v0.37.0/go.mod h1:vg+k43peMZ0pUMhYmVAWysMK35e6ioLh3wB8ZCAfbVc= +golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= +golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/text v0.24.0 h1:dd5Bzh4yt5KYA8f9CJHCP4FB4D51c2c6JvN37xJJkJ0= +golang.org/x/text v0.24.0/go.mod h1:L8rBsPeo2pSS+xqN0d5u2ikmjtmoJbDBT1b7nHvFCdU= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go new file mode 100644 index 0000000..59ff029 --- /dev/null +++ b/test/integration/integration_test.go @@ -0,0 +1,110 @@ +//go:build integration + +// Package integration exercises explain.PlanAnalyzer against live databases. +// +// These tests are excluded from a normal `go test ./...` by the build tag. +// Bring the servers up and run them with: +// +// docker compose -f test/integration/docker-compose.yml up -d --wait +// make test-integration +// +// Each test skips when its DSN environment variable is unset, so a partial +// stack (say, Postgres only) still runs the tests it can. +package integration + +import ( + "context" + "database/sql" + "os" + "testing" + "time" + + "github.com/KARTIKrocks/sqlguard/analyzer" + "github.com/KARTIKrocks/sqlguard/explain" + + _ "github.com/go-sql-driver/mysql" + _ "github.com/jackc/pgx/v5/stdlib" +) + +// DSN environment variables. The defaults in docker-compose.yml are: +// +// SQLGUARD_TEST_PG_DSN postgres://sqlguard:sqlguard@localhost:55432/sqlguard?sslmode=disable +// SQLGUARD_TEST_MYSQL_DSN root:sqlguard@tcp(localhost:53306)/sqlguard +// SQLGUARD_TEST_MARIADB_DSN root:sqlguard@tcp(localhost:53307)/sqlguard +const ( + envPostgres = "SQLGUARD_TEST_PG_DSN" + envMySQL = "SQLGUARD_TEST_MYSQL_DSN" + envMariaDB = "SQLGUARD_TEST_MARIADB_DSN" +) + +// connect opens the database named by env, skipping the test when it is unset. +// driver is the database/sql driver name ("pgx" or "mysql"). +func connect(t *testing.T, env, driver string) *sql.DB { + t.Helper() + + dsn := os.Getenv(env) + if dsn == "" { + t.Skipf("%s not set; start the stack with `docker compose -f test/integration/docker-compose.yml up -d --wait`", env) + } + + db, err := sql.Open(driver, dsn) + if err != nil { + t.Fatalf("open %s: %v", driver, err) + } + t.Cleanup(func() { _ = db.Close() }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := db.PingContext(ctx); err != nil { + t.Fatalf("ping %s: %v", driver, err) + } + return db +} + +// exec runs each statement in order, failing the test on the first error. +func exec(t *testing.T, db *sql.DB, stmts ...string) { + t.Helper() + for _, s := range stmts { + if _, err := db.ExecContext(context.Background(), s); err != nil { + t.Fatalf("exec %q: %v", s, err) + } + } +} + +// testCtx returns a context that expires well before the Go test timeout, so a +// hung server surfaces as a test failure rather than a panic. +func testCtx(t *testing.T) context.Context { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + return ctx +} + +// ruleNames returns the RuleName of every issue, in order. +func ruleNames(issues []analyzer.Result) []string { + names := make([]string, len(issues)) + for i, r := range issues { + names[i] = r.RuleName + } + return names +} + +// hasRule reports whether any issue carries the given rule name. +func hasRule(issues []analyzer.Result, name string) bool { + for _, r := range issues { + if r.RuleName == name { + return true + } + } + return false +} + +// analyzerFor builds a PlanAnalyzer, failing the test if construction errors. +func analyzerFor(t *testing.T, db *sql.DB, dialect string, opts ...explain.Option) *explain.PlanAnalyzer { + t.Helper() + p, err := explain.New(db, dialect, opts...) + if err != nil { + t.Fatalf("explain.New(%q): %v", dialect, err) + } + return p +} diff --git a/test/integration/mysql_test.go b/test/integration/mysql_test.go new file mode 100644 index 0000000..c9a969c --- /dev/null +++ b/test/integration/mysql_test.go @@ -0,0 +1,199 @@ +//go:build integration + +package integration + +import ( + "database/sql" + "strings" + "testing" + + "github.com/KARTIKrocks/sqlguard/explain" +) + +// server is one MySQL-dialect database under test. MySQL and MariaDB share the +// "mysql" dialect but disagree on EXPLAIN's column set, so every case runs +// against both. +type server struct { + name string + env string +} + +var mysqlServers = []server{ + {"mysql", envMySQL}, + {"mariadb", envMariaDB}, +} + +// setupMySQL creates a table whose `name` column has no index. 500 rows keeps +// the seeding CTE under MySQL's default cte_max_recursion_depth of 1000 while +// still being large enough for the optimizer to choose a full scan. +func setupMySQL(t *testing.T, env string) *sql.DB { + t.Helper() + db := connect(t, env, "mysql") + exec(t, db, + `DROP TABLE IF EXISTS users`, + `CREATE TABLE users (id int AUTO_INCREMENT PRIMARY KEY, email varchar(64), name varchar(64))`, + `INSERT INTO users (email, name) + WITH RECURSIVE seq(n) AS ( + SELECT 1 UNION ALL SELECT n + 1 FROM seq WHERE n < 500 + ) + SELECT CONCAT('e', n), CONCAT('n', n) FROM seq`, + `ANALYZE TABLE users`, + ) + return db +} + +// eachServer runs fn against every configured MySQL-dialect server. +func eachServer(t *testing.T, fn func(t *testing.T, db *sql.DB)) { + t.Helper() + for _, s := range mysqlServers { + t.Run(s.name, func(t *testing.T) { + fn(t, setupMySQL(t, s.env)) + }) + } +} + +// Regression: MySQL 9 defaults @@explain_format to TREE, which returns a +// single free-text column instead of the tabular plan. analyzeMySQL must pin +// FORMAT=TRADITIONAL, or the column lookup finds nothing and every rule goes +// silent. MariaDB emits 10 columns to MySQL's 12, so the lookup must also be +// by name rather than by position. +func TestMySQL_FullTableScanAndMissingIndex(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users WHERE name = 'n1'`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if !hasRule(res.Issues, "full-table-scan") { + t.Errorf("expected full-table-scan, got %v", ruleNames(res.Issues)) + } + if !hasRule(res.Issues, "no-index-used") { + t.Errorf("expected no-index-used, got %v", ruleNames(res.Issues)) + } + }) +} + +// The negative control: a primary-key lookup uses an index and scans nothing. +func TestMySQL_IndexedLookupIsClean(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users WHERE id = 1`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if hasRule(res.Issues, "full-table-scan") || hasRule(res.Issues, "no-index-used") { + t.Errorf("primary-key lookup should be clean, got %v", ruleNames(res.Issues)) + } + }) +} + +// ORDER BY on an unindexed column sorts outside the index. +func TestMySQL_Filesort(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users ORDER BY name`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if !hasRule(res.Issues, "filesort") { + t.Errorf("expected filesort, got %v", ruleNames(res.Issues)) + } + }) +} + +// A UNION adds a UNION RESULT row naming the temporary table ``, +// which has type=ALL and no key. It is not a real table, so it must not be +// reported as an unindexed full scan. +func TestMySQL_UnionPseudoTableNotReported(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql") + + res, err := p.Analyze(testCtx(t), `SELECT id FROM users UNION SELECT id FROM users`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + for _, r := range res.Issues { + if strings.Contains(r.Message, "<") { + t.Errorf("issue names a temporary table, not a real one: %q", r.Message) + } + } + }) +} + +// Without WithAllowDML, a DELETE never reaches the server. +func TestMySQL_DMLRefusedByDefault(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql") + + _, err := p.Analyze(testCtx(t), `DELETE FROM users WHERE id = 1`) + if err == nil { + t.Fatal("expected an error without WithAllowDML") + } + if !strings.Contains(err.Error(), "data-modifying") { + t.Errorf("error = %v, want it to mention a data-modifying statement", err) + } + }) +} + +// Regression: MySQL and MariaDB reject every statement inside a READ ONLY +// transaction with error 1792 — including an EXPLAIN that only plans it. So +// WithAllowDML must open a regular transaction and rely on the rollback, which +// this test also verifies by counting rows. +func TestMySQL_ExplainDMLDoesNotMutate(t *testing.T) { + eachServer(t, func(t *testing.T, db *sql.DB) { + p := analyzerFor(t, db, "mysql", explain.WithAllowDML()) + + var before int + if err := db.QueryRow(`SELECT count(*) FROM users`).Scan(&before); err != nil { + t.Fatalf("count before: %v", err) + } + + if _, err := p.Analyze(testCtx(t), `DELETE FROM users WHERE id < 100`); err != nil { + t.Fatalf("Analyze(DELETE) with WithAllowDML: %v", err) + } + + var after int + if err := db.QueryRow(`SELECT count(*) FROM users`).Scan(&after); err != nil { + t.Fatalf("count after: %v", err) + } + if before != after { + t.Errorf("row count changed from %d to %d; EXPLAIN must never mutate", before, after) + } + }) +} + +// Forcing @@explain_format=TREE reproduces the MySQL 9 default on any MySQL +// version, proving the FORMAT=TRADITIONAL clause overrides the session default +// rather than merely happening to agree with it. MariaDB has no such variable. +func TestMySQL_TraditionalFormatOverridesSessionDefault(t *testing.T) { + db := setupMySQL(t, envMySQL) + + var original string + if err := db.QueryRow(`SELECT @@explain_format`).Scan(&original); err != nil { + t.Skipf("server has no @@explain_format: %v", err) + } + + exec(t, db, `SET GLOBAL explain_format = 'TREE'`) + t.Cleanup(func() { exec(t, db, `SET GLOBAL explain_format = '`+original+`'`) }) + + // A fresh pool picks up the new global default on every new connection. + fresh := connect(t, envMySQL, "mysql") + var got string + if err := fresh.QueryRow(`SELECT @@explain_format`).Scan(&got); err != nil { + t.Fatalf("read @@explain_format: %v", err) + } + if got != "TREE" { + t.Fatalf("@@explain_format = %q, want TREE; the premise of this test does not hold", got) + } + + res, err := analyzerFor(t, fresh, "mysql").Analyze(testCtx(t), `SELECT * FROM users WHERE name = 'n1'`) + if err != nil { + t.Fatalf("Analyze under explain_format=TREE: %v", err) + } + if !hasRule(res.Issues, "full-table-scan") { + t.Errorf("expected full-table-scan under explain_format=TREE, got %v", ruleNames(res.Issues)) + } +} diff --git a/test/integration/postgres_test.go b/test/integration/postgres_test.go new file mode 100644 index 0000000..2c4cf14 --- /dev/null +++ b/test/integration/postgres_test.go @@ -0,0 +1,190 @@ +//go:build integration + +package integration + +import ( + "database/sql" + "encoding/json" + "strings" + "testing" + + "github.com/KARTIKrocks/sqlguard/analyzer" + "github.com/KARTIKrocks/sqlguard/explain" +) + +// setupPostgres creates two unindexed-by-name tables with enough rows that the +// planner's estimates cross the seq-scan and high-cost thresholds. +func setupPostgres(t *testing.T) *sql.DB { + t.Helper() + db := connect(t, envPostgres, "pgx") + exec(t, db, + `DROP TABLE IF EXISTS orders, users`, + `CREATE TABLE users (id serial PRIMARY KEY, email text, name text)`, + `CREATE TABLE orders (id serial PRIMARY KEY, user_id int, total numeric)`, + `INSERT INTO users (email, name) SELECT 'e' || g, 'n' || g FROM generate_series(1, 5000) g`, + `INSERT INTO orders (user_id, total) SELECT g % 5000 + 1, g FROM generate_series(1, 5000) g`, + `ANALYZE users`, + `ANALYZE orders`, + ) + return db +} + +// A predicate on an unindexed column must plan as a Seq Scan, and an estimate +// above 1000 rows must escalate the severity from INFO to WARNING. +func TestPostgres_SeqScanOnUnindexedColumn(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users WHERE name LIKE 'n1%'`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if !hasRule(res.Issues, "seq-scan") { + t.Fatalf("expected seq-scan, got %v", ruleNames(res.Issues)) + } + for _, r := range res.Issues { + if r.RuleName != "seq-scan" { + continue + } + if r.Severity != analyzer.SeverityWarning { + t.Errorf("severity = %v, want WARNING (estimate exceeds 1000 rows)", r.Severity) + } + } +} + +// The primary-key lookup plans as an Index Scan, so no seq-scan is reported. +// This is the negative control for the test above. +func TestPostgres_IndexedLookupHasNoSeqScan(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users WHERE id = 1`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if hasRule(res.Issues, "seq-scan") { + t.Errorf("unexpected seq-scan on a primary-key lookup: %v", ruleNames(res.Issues)) + } +} + +// A join over two unindexed columns nests one Seq Scan under another inside +// "Plans", so more than one issue proves walkPgPlan actually recurses. +func TestPostgres_WalkRecursesIntoChildPlans(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users u JOIN orders o ON o.user_id = u.id`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + var seqScans int + for _, r := range res.Issues { + if r.RuleName == "seq-scan" { + seqScans++ + } + } + if seqScans < 2 { + t.Errorf("seq-scan count = %d, want >= 2 (one per joined table); issues %v", seqScans, ruleNames(res.Issues)) + } +} + +// A cartesian product pushes Total Cost past the 10000 threshold. +func TestPostgres_HighCost(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users u, orders o`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if !hasRule(res.Issues, "high-cost") { + t.Errorf("expected high-cost on a 5000x5000 cartesian product, got %v", ruleNames(res.Issues)) + } +} + +// RawPlan must be the verbatim server response: a JSON array whose first +// element carries a "Plan" object with the keys pgPlanNode unmarshals. This is +// the contract that would silently break on a Postgres major upgrade. +func TestPostgres_RawPlanJSONContract(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + res, err := p.Analyze(testCtx(t), `SELECT * FROM users WHERE name = 'n1'`) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + + var plans []struct { + Plan map[string]any `json:"Plan"` + } + if err := json.Unmarshal([]byte(res.RawPlan), &plans); err != nil { + t.Fatalf("RawPlan is not a JSON array: %v\n%s", err, res.RawPlan) + } + if len(plans) != 1 { + t.Fatalf("len(plans) = %d, want 1", len(plans)) + } + for _, key := range []string{"Node Type", "Total Cost", "Plan Rows"} { + if _, ok := plans[0].Plan[key]; !ok { + t.Errorf("plan is missing key %q; pgPlanNode would decode a zero value", key) + } + } +} + +// Every issue carries the fingerprint of the original query text. +func TestPostgres_IssuesCarryFingerprint(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + const q = `SELECT * FROM users WHERE name LIKE 'n1%'` + res, err := p.Analyze(testCtx(t), q) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if len(res.Issues) == 0 { + t.Fatal("expected at least one issue") + } + want := analyzer.Fingerprint(q) + for _, r := range res.Issues { + if r.Fingerprint != want { + t.Errorf("Fingerprint = %q, want %q", r.Fingerprint, want) + } + } +} + +// Without WithAllowDML, a DELETE never reaches the server. +func TestPostgres_DMLRefusedByDefault(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres") + + _, err := p.Analyze(testCtx(t), `DELETE FROM users WHERE id = 1`) + if err == nil { + t.Fatal("expected an error without WithAllowDML") + } + if !strings.Contains(err.Error(), "data-modifying") { + t.Errorf("error = %v, want it to mention a data-modifying statement", err) + } +} + +// Postgres permits EXPLAIN of a DELETE inside a READ ONLY transaction (it +// plans without executing), and the rollback leaves every row in place. +func TestPostgres_ExplainDMLDoesNotMutate(t *testing.T) { + db := setupPostgres(t) + p := analyzerFor(t, db, "postgres", explain.WithAllowDML()) + + var before int + if err := db.QueryRow(`SELECT count(*) FROM users`).Scan(&before); err != nil { + t.Fatalf("count before: %v", err) + } + + if _, err := p.Analyze(testCtx(t), `DELETE FROM users WHERE id < 100`); err != nil { + t.Fatalf("Analyze(DELETE) with WithAllowDML: %v", err) + } + + var after int + if err := db.QueryRow(`SELECT count(*) FROM users`).Scan(&after); err != nil { + t.Fatalf("count after: %v", err) + } + if before != after { + t.Errorf("row count changed from %d to %d; EXPLAIN must never mutate", before, after) + } +} From f1daa3657cd88e0b6523421db22bc38e66a37459 Mon Sep 17 00:00:00 2001 From: kartik Date: Thu, 9 Jul 2026 21:37:09 +0530 Subject: [PATCH 2/2] Address CodeRabbit review: fail closed on unknown plan shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies four of six review findings; the other two rest on premises that do not hold against the current code. Applied: - explain: fail closed when the MySQL EXPLAIN plan lacks an expected column. Previously a plan of the wrong shape made every col() lookup return "" and Analyze reported zero issues. Verified by pointing the analyzer at a plain EXPLAIN on MySQL 9, which now errors with `unrecognized MySQL EXPLAIN plan (missing "table" column; got [EXPLAIN])` rather than silently passing. - explain: assert the statement kind in TestValidate, including CTE-wrapped DML. The kind drives isDML, which picks the transaction mode, so a classification regression would quietly reintroduce MySQL error 1792. - test/integration: give each statement in exec its own deadline, so a wedged server surfaces as a context error instead of hanging to the suite timeout. Uses a standalone context rather than testCtx so exec stays safe in cleanups. - test/integration: pgx v5.7.6 -> v5.10.0, mysql v1.9.3 -> v1.10.0. Not a security fix — govulncheck reports no advisory against v5.7.6, called or otherwise. pgxguard already requires pgx v5.10.0, so the workspace resolved it there while go.mod said v5.7.6; the module now resolves identically with and without GOWORK. Not applied: - "BeginTx ReadOnly: !dml breaks the EXPLAIN safety invariant; error out instead." Doing so removes WithAllowDML on MySQL and MariaDB entirely: both reject every statement in a READ ONLY transaction with error 1792, including a planning-only EXPLAIN. That was the bug this branch fixes. The invariant the code actually guarantees — the statement is never executed — is upheld by validate(), by never using EXPLAIN ANALYZE, and by the unconditional rollback, and is covered by TestMySQL_ExplainDMLDoesNotMutate. - "t.Fatalf in t.Cleanup can panic on Go 1.21+." It does not. Verified on Go 1.26: a Fatalf inside a cleanup fails the test normally and does not skip the other registered cleanups. Failing loudly is also right here, since a failed restore leaves @@explain_format mutated on a shared server. --- .coderabbit.yaml | 6 ++- CHANGELOG.md | 4 ++ explain/explain.go | 10 +++++ explain/explain_test.go | 56 ++++++++++++++++++++-------- test/integration/go.mod | 9 ++--- test/integration/go.sum | 22 +++++------ test/integration/integration_test.go | 10 ++++- 7 files changed, 81 insertions(+), 36 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 31a7891..0591de4 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -130,8 +130,10 @@ reviews: The MySQL plan is requested as `EXPLAIN FORMAT=TRADITIONAL` because MySQL 9 defaults @@explain_format to TREE (one free-text column); do not drop the clause. Plan columns are addressed BY NAME, never by position: - MariaDB emits 10 columns where MySQL emits 12. Rows whose `table` starts - with `<` are UNION RESULT / derived temporary tables and are skipped on + MariaDB emits 10 columns where MySQL emits 12. The required-column check + after rows.Columns() makes an unrecognized plan shape a hard error rather + than a silent zero-issue result; keep it. Rows whose `table` starts with + `<` are UNION RESULT / derived temporary tables and are skipped on purpose. Deliberate carve-out: explain keeps Result.Query (and the inner analyzer.Result.Query of its findings) RAW — the user typed the query on their own CLI, it never reaches a log/telemetry sink, and Fingerprint is diff --git a/CHANGELOG.md b/CHANGELOG.md index 85c6851..ffa8e07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,10 @@ failed on every query. No public API changed. - **False positive on `UNION`**: a `UNION RESULT` row names a temporary table (``) with `type=ALL` and no key, and was reported as an unindexed full table scan. Rows naming a derived or temporary table are now skipped. +- **Fails closed on an unrecognized MySQL plan**: if the server returns a plan + without the expected columns, `Analyze` now returns an error naming the + missing column instead of silently reporting zero issues. A false negative in + a query linter is worse than a hard failure. ### Added diff --git a/explain/explain.go b/explain/explain.go index c9af9f5..0f8c9ac 100644 --- a/explain/explain.go +++ b/explain/explain.go @@ -266,6 +266,16 @@ func (p *PlanAnalyzer) analyzeMySQL(ctx context.Context, query string, dml bool) for i, c := range cols { index[strings.ToLower(c)] = i } + // Fail closed on an unrecognized plan shape. If a future server ignores + // FORMAT=TRADITIONAL the way MySQL 9 ignores a bare EXPLAIN, every lookup + // below would return "" and Analyze would report zero issues — a silent + // false negative is worse than an error. `rows` is not required: it only + // decorates a message. + for _, want := range []string{"table", "type", "key", "possible_keys", "extra"} { + if _, ok := index[want]; !ok { + return nil, fmt.Errorf("explain: unrecognized MySQL EXPLAIN plan (missing %q column; got %v)", want, cols) + } + } cells := make([]sql.NullString, len(cols)) scanArgs := make([]any, len(cols)) for i := range cells { diff --git a/explain/explain_test.go b/explain/explain_test.go index 84bc776..9ed2aab 100644 --- a/explain/explain_test.go +++ b/explain/explain_test.go @@ -3,35 +3,61 @@ package explain import ( "strings" "testing" + + "github.com/KARTIKrocks/sqlguard/analyzer" ) func TestValidate(t *testing.T) { + // wantKind is asserted on every case, including the rejected ones: the kind + // drives isDML, which picks the transaction mode in analyzeMySQL. A + // classification regression there would silently reintroduce MySQL error + // 1792 (a DML EXPLAIN inside a READ ONLY transaction), which no unit test + // can observe. cases := []struct { name string query string allowDML bool wantErr string // substring; "" means no error wantSafe string // expected returned statement when no error + wantKind analyzer.StmtKind }{ - {"select ok", `SELECT * FROM t WHERE id = 1`, false, "", `SELECT * FROM t WHERE id = 1`}, - {"with ok", `WITH c AS (SELECT 1) SELECT * FROM c`, false, "", `WITH c AS (SELECT 1) SELECT * FROM c`}, - {"trailing semicolon trimmed", `SELECT 1;`, false, "", `SELECT 1`}, - {"empty", ` `, false, "empty", ""}, - {"stacked statements", `SELECT 1; DROP TABLE users`, false, "multi-statement", ""}, - {"semicolon in comment is fine", "SELECT 1 -- ; DROP\n", false, "", "SELECT 1 -- ; DROP"}, - {"semicolon in string is fine", `SELECT * FROM t WHERE s = 'a;b'`, false, "", `SELECT * FROM t WHERE s = 'a;b'`}, - {"stack hidden after string", `SELECT 'a;b'; DELETE FROM t`, false, "multi-statement", ""}, - {"dml refused by default", `DELETE FROM t WHERE id = 1`, false, "data-modifying", ""}, - {"update refused by default", `UPDATE t SET a = 1`, false, "data-modifying", ""}, - {"dml allowed with opt-in", `DELETE FROM t WHERE id = 1`, true, "", `DELETE FROM t WHERE id = 1`}, - {"ddl always refused", `DROP TABLE users`, true, "non-SELECT", ""}, - {"set always refused", `SET search_path = x`, true, "non-SELECT", ""}, - {"truncate refused", `TRUNCATE t`, true, "non-SELECT", ""}, + {"select ok", `SELECT * FROM t WHERE id = 1`, false, "", `SELECT * FROM t WHERE id = 1`, analyzer.StmtSelect}, + {"with ok", `WITH c AS (SELECT 1) SELECT * FROM c`, false, "", `WITH c AS (SELECT 1) SELECT * FROM c`, analyzer.StmtSelect}, + {"trailing semicolon trimmed", `SELECT 1;`, false, "", `SELECT 1`, analyzer.StmtSelect}, + {"empty", ` `, false, "empty", "", analyzer.StmtUnknown}, + {"stacked statements", `SELECT 1; DROP TABLE users`, false, "multi-statement", "", analyzer.StmtUnknown}, + {"semicolon in comment is fine", "SELECT 1 -- ; DROP\n", false, "", "SELECT 1 -- ; DROP", analyzer.StmtSelect}, + {"semicolon in string is fine", `SELECT * FROM t WHERE s = 'a;b'`, false, "", `SELECT * FROM t WHERE s = 'a;b'`, analyzer.StmtSelect}, + {"stack hidden after string", `SELECT 'a;b'; DELETE FROM t`, false, "multi-statement", "", analyzer.StmtUnknown}, + {"dml refused by default", `DELETE FROM t WHERE id = 1`, false, "data-modifying", "", analyzer.StmtDelete}, + {"update refused by default", `UPDATE t SET a = 1`, false, "data-modifying", "", analyzer.StmtUpdate}, + {"insert refused by default", `INSERT INTO t (a) VALUES (1)`, false, "data-modifying", "", analyzer.StmtInsert}, + {"dml allowed with opt-in", `DELETE FROM t WHERE id = 1`, true, "", `DELETE FROM t WHERE id = 1`, analyzer.StmtDelete}, + {"update allowed with opt-in", `UPDATE t SET a = 1`, true, "", `UPDATE t SET a = 1`, analyzer.StmtUpdate}, + // A CTE-wrapped DML statement starts with WITH but must not classify as + // SELECT: on MySQL that would leave it in a READ ONLY transaction. + {"cte-wrapped delete is dml", `WITH c AS (SELECT 1) DELETE FROM t WHERE id = 1`, true, "", `WITH c AS (SELECT 1) DELETE FROM t WHERE id = 1`, analyzer.StmtDelete}, + {"cte-wrapped update is dml", `WITH c AS (SELECT 1) UPDATE t SET a = 1`, true, "", `WITH c AS (SELECT 1) UPDATE t SET a = 1`, analyzer.StmtUpdate}, + {"cte-wrapped insert is dml", `WITH c AS (SELECT 1) INSERT INTO t SELECT * FROM c`, true, "", `WITH c AS (SELECT 1) INSERT INTO t SELECT * FROM c`, analyzer.StmtInsert}, + {"cte-wrapped delete refused by default", `WITH c AS (SELECT 1) DELETE FROM t WHERE id = 1`, false, "data-modifying", "", analyzer.StmtDelete}, + {"ddl always refused", `DROP TABLE users`, true, "non-SELECT", "", analyzer.StmtOther}, + {"set always refused", `SET search_path = x`, true, "non-SELECT", "", analyzer.StmtOther}, + {"truncate refused", `TRUNCATE t`, true, "non-SELECT", "", analyzer.StmtOther}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { p := &PlanAnalyzer{dialect: "postgres", allowDML: c.allowDML} - safe, _, err := p.validate(c.query) + safe, kind, err := p.validate(c.query) + + // Asserted on both paths: a rejected DML statement must still be + // classified as DML, not folded into StmtUnknown. + if kind != c.wantKind { + t.Errorf("kind = %v, want %v", kind, c.wantKind) + } + if isDML(kind) != isDML(c.wantKind) { + t.Errorf("isDML(%v) = %v, want %v", kind, isDML(kind), isDML(c.wantKind)) + } + if c.wantErr == "" { if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/test/integration/go.mod b/test/integration/go.mod index 5168412..88eddf0 100644 --- a/test/integration/go.mod +++ b/test/integration/go.mod @@ -10,16 +10,15 @@ go 1.26 require ( github.com/KARTIKrocks/sqlguard v0.1.0 - github.com/go-sql-driver/mysql v1.9.3 - github.com/jackc/pgx/v5 v5.7.6 + github.com/go-sql-driver/mysql v1.10.0 + github.com/jackc/pgx/v5 v5.10.0 ) require ( - filippo.io/edwards25519 v1.1.0 // indirect + filippo.io/edwards25519 v1.2.0 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect - golang.org/x/crypto v0.37.0 // indirect golang.org/x/sync v0.21.0 // indirect - golang.org/x/text v0.24.0 // indirect + golang.org/x/text v0.29.0 // indirect ) diff --git a/test/integration/go.sum b/test/integration/go.sum index baba15c..1cc868e 100644 --- a/test/integration/go.sum +++ b/test/integration/go.sum @@ -1,18 +1,18 @@ -filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA= -filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4= +filippo.io/edwards25519 v1.2.0 h1:crnVqOiS4jqYleHd9vaKZ+HKtHfllngJIiOpNpoJsjo= +filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q77mc= github.com/KARTIKrocks/sqlguard v0.1.0 h1:1rELGKkh30y2UvkmyKCK0veiFe3Jr66+RGbEsAIOofk= github.com/KARTIKrocks/sqlguard v0.1.0/go.mod h1:Y9zX2L9OSqMXa1gx9c4mMKPTDEsArvbtYmzKpAwWPv8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/go-sql-driver/mysql v1.9.3 h1:U/N249h2WzJ3Ukj8SowVFjdtZKfu9vlLZxjPXV1aweo= -github.com/go-sql-driver/mysql v1.9.3/go.mod h1:qn46aNg1333BRMNU69Lq93t8du/dwxI64Gl8i5p1WMU= +github.com/go-sql-driver/mysql v1.10.0 h1:Q+1LV8DkHJvSYAdR83XzuhDaTykuDx0l6fkXxoWCWfw= +github.com/go-sql-driver/mysql v1.10.0/go.mod h1:M+cqaI7+xxXGG9swrdeUIoPG3Y3KCkF0pZej+SK+nWk= github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= -github.com/jackc/pgx/v5 v5.7.6 h1:rWQc5FwZSPX58r1OQmkuaNicxdmExaEz5A2DO2hUuTk= -github.com/jackc/pgx/v5 v5.7.6/go.mod h1:aruU7o91Tc2q2cFp5h4uP3f6ztExVpyVv88Xl/8Vl8M= +github.com/jackc/pgx/v5 v5.10.0 h1:VhSvgU2jSli8o3AqIEOTJr7rZwAEUVo4E4XhR94Zfr0= +github.com/jackc/pgx/v5 v5.10.0/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= @@ -20,14 +20,12 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= -github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -golang.org/x/crypto v0.37.0 h1:kJNSjF/Xp7kU0iB2Z+9viTPMW4EqqsrywMXLJOOsXSE= -golang.org/x/crypto v0.37.0/go.mod h1:vg+k43peMZ0pUMhYmVAWysMK35e6ioLh3wB8ZCAfbVc= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= -golang.org/x/text v0.24.0 h1:dd5Bzh4yt5KYA8f9CJHCP4FB4D51c2c6JvN37xJJkJ0= -golang.org/x/text v0.24.0/go.mod h1:L8rBsPeo2pSS+xqN0d5u2ikmjtmoJbDBT1b7nHvFCdU= +golang.org/x/text v0.29.0 h1:1neNs90w9YzJ9BocxfsQNHKuAT4pkghyXc4nhZ6sJvk= +golang.org/x/text v0.29.0/go.mod h1:7MhJOA9CD2qZyOKYazxdYMF85OwPdEr9jTtBpO7ydH4= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index 59ff029..cd4f9e3 100644 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -61,11 +61,17 @@ func connect(t *testing.T, env, driver string) *sql.DB { return db } -// exec runs each statement in order, failing the test on the first error. +// exec runs each statement in order, failing the test on the first error. Each +// statement carries its own deadline, so a wedged server surfaces as a context +// error rather than hanging until the suite-wide test timeout. The context is +// not tied to t via testCtx, so exec is also safe to call from a t.Cleanup. func exec(t *testing.T, db *sql.DB, stmts ...string) { t.Helper() for _, s := range stmts { - if _, err := db.ExecContext(context.Background(), s); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + _, err := db.ExecContext(ctx, s) + cancel() + if err != nil { t.Fatalf("exec %q: %v", s, err) } }