Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 37 additions & 12 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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: >-
Expand All @@ -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: >-
Expand All @@ -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:
Expand Down
59 changes: 59 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
51 changes: 50 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
(`<union1,2>`) 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.
Expand Down Expand Up @@ -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
56 changes: 54 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Loading