diff --git a/.coderabbit.yaml b/.coderabbit.yaml index fc200b1..0591de4 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,39 @@ 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. 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 + 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 +166,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..ffa8e07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,54 @@ 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. +- **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 + +- `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 +105,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..0f8c9ac 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,47 @@ 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 + } + // 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 { + 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 +305,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..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/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..88eddf0 --- /dev/null +++ b/test/integration/go.mod @@ -0,0 +1,24 @@ +// 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.10.0 + github.com/jackc/pgx/v5 v5.10.0 +) + +require ( + 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/sync v0.21.0 // indirect + golang.org/x/text v0.29.0 // indirect +) diff --git a/test/integration/go.sum b/test/integration/go.sum new file mode 100644 index 0000000..1cc868e --- /dev/null +++ b/test/integration/go.sum @@ -0,0 +1,32 @@ +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.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.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= +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.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.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= +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..cd4f9e3 --- /dev/null +++ b/test/integration/integration_test.go @@ -0,0 +1,116 @@ +//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. 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 { + 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) + } + } +} + +// 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) + } +}