From 8a95cc5d8fd5912a4cbf08b0d4457298d62101f2 Mon Sep 17 00:00:00 2001 From: kartik Date: Mon, 5 Oct 2026 16:26:07 +0530 Subject: [PATCH 1/5] fix(analyzer): count WHERE/LIMIT only where it bounds the statement The fallback counted a WHERE or LIMIT anywhere in the text while the dialect parsers counted only the top level, so a parser could report a finding the fallback did not. Both now count the top level and, for a SELECT, a derived table, CTE body or set-operation operand; one inside IN (...), a scalar subquery or a function argument does not. The parsers OR their AST answer with the fallback's, replacing the set-operation special case. Fixes #91 --- .codeant/review.json | 2 +- .coderabbit.yaml | 4 + .greptile/config.json | 2 +- AGENTS.md | 2 +- CHANGELOG.md | 13 ++ analyzer/fallback.go | 157 ++++++++++++++++++++++-- analyzer/fallback_test.go | 41 +++++++ analyzer/statement.go | 15 ++- parsers/mysqlparser/mysqlparser.go | 15 ++- parsers/mysqlparser/mysqlparser_test.go | 23 ++++ parsers/pgparser/pgparser.go | 37 ++---- parsers/pgparser/pgparser_test.go | 23 ++++ website/docs/parsers.md | 12 +- 13 files changed, 283 insertions(+), 63 deletions(-) diff --git a/.codeant/review.json b/.codeant/review.json index 90c0c60..9f697ef 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -62,7 +62,7 @@ }, { "id": "parser-never-breaks-query-path", - "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency fallback has to learn it too - detectKind and insertColumnsListed are the pair to check - and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node - CREATE VIEW ... AS SELECT, EXPLAIN, DDL - must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.", + "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency fallback has to learn it too - detectKind and insertColumnsListed are the pair to check - and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node - CREATE VIEW ... AS SELECT, EXPLAIN, DDL - must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). HasWhere/HasLimit count a clause only where it bounds the statement's rows: the top level, and for a SELECT a derived table, CTE body or parenthesised set-operation operand, never IN (...), a scalar subquery or a function argument; the parsers OR their top-level AST answer with the fallback's (keepFallbackBounds) for every SELECT (#91). The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.", "files": ["parsers/**/*.go", "analyzer/analyzer.go", "analyzer/fallback.go", "analyzer/parser.go", "analyzer/statement.go"], "scope": ["pr", "ide"] }, diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 7c2bc98..dc04ea1 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -166,6 +166,10 @@ reviews: with Exact=false, because blanking fields nothing was derived for drops findings silently (#81). HasLimit means a row count, not a limit node (#82). + HasWhere/HasLimit count only a clause that bounds the statement's rows: + top level, or for a SELECT a derived table, CTE body or parenthesised + set-operation operand — never IN (...) or a scalar subquery. Parsers OR their top-level AST + answer with the fallback's for every SELECT (#91). - path: "config/**" instructions: >- diff --git a/.greptile/config.json b/.greptile/config.json index 7bb26f4..2dab999 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -68,7 +68,7 @@ }, { "id": "parser-never-breaks-query-path", - "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead — that degradation lives in analyzer.Analyze. A dialect parser may also only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind the zero-dependency fallback has to learn it too (detectKind and insertColumnsListed are the pair to check), and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node (CREATE VIEW ... AS SELECT, EXPLAIN, DDL) must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.", + "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead — that degradation lives in analyzer.Analyze. A dialect parser may also only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind the zero-dependency fallback has to learn it too (detectKind and insertColumnsListed are the pair to check), and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. A dialect parser resets the structural fields only for an AST node it models; any other node (CREATE VIEW ... AS SELECT, EXPLAIN, DDL) must return the fallback's Statement untouched with Exact=false, since blanking fields nothing was derived for silently drops findings (#81), pinned by TestParser_KeepsFallbackFactsForUnmodelledStatements. A field refilled from the AST must mean what its name says: HasLimit is a row count, not the presence of a limit node (#82). HasWhere/HasLimit count a clause only where it bounds the statement's rows: the top level, and for a SELECT a derived table, CTE body or parenthesised set-operation operand, never IN (...), a scalar subquery or a function argument; the parsers OR their top-level AST answer with the fallback's (keepFallbackBounds) for every SELECT (#91). The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.", "scope": ["parsers/**/*.go", "analyzer/**/*.go"], "severity": "high" }, diff --git a/AGENTS.md b/AGENTS.md index 0135f7a..4747514 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **The analyzer is parser-pluggable.** `analyzer.Analyzer` runs `Rule`s against a normalized, dialect-agnostic `Statement` produced by an `analyzer.Parser` (`analyzer/parser.go`, `statement.go`). The default `FallbackParser` (`fallback.go`) is zero-dependency, strips comments/string literals, and never errors. `analyzer.Analyze` degrades to the FallbackParser if a configured parser errors, so analysis never breaks the caller's query path. Real grammars are supplied via `middleware.WithParser(...)` / `analyzer.Default().WithParser(...)` using the `parsers/*` modules. Rules read the `Statement`, never raw SQL. -**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. The converse holds too: **a dialect parser resets the structural fields only inside a `case` for an AST node it models, and any other node returns the fallback's `Statement` untouched with `Exact = false`.** Blanking them up front dropped every finding on `CREATE VIEW v AS SELECT * FROM t`, `EXPLAIN SELECT *` and the like (#81), because nothing had been derived to replace them — a false negative the "never adds" test cannot see, so `TestParser_KeepsFallbackFactsForUnmodelledStatements` pins it. A field refilled from the AST must mean what its name says: `HasLimit` is a row count, not the presence of a limit node, which pgparser also builds for a bare `OFFSET` (#82). +**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. The converse holds too: **a dialect parser resets the structural fields only inside a `case` for an AST node it models, and any other node returns the fallback's `Statement` untouched with `Exact = false`.** Blanking them up front dropped every finding on `CREATE VIEW v AS SELECT * FROM t`, `EXPLAIN SELECT *` and the like (#81), because nothing had been derived to replace them — a false negative the "never adds" test cannot see, so `TestParser_KeepsFallbackFactsForUnmodelledStatements` pins it. A field refilled from the AST must mean what its name says: `HasLimit` is a row count, not the presence of a limit node, which pgparser also builds for a bare `OFFSET` (#82). `HasWhere`/`HasLimit` count a clause only where it bounds the statement's rows: the top level, and for a `SELECT` a derived table, CTE body or parenthesised set-operation operand (`scopedBounds` / `rowSourceSpans`), never `IN (...)`, a scalar subquery or a function argument. The parsers OR their top-level AST answer with the fallback's (`keepFallbackBounds`) for every `SELECT`, so the two agree by construction (#91). **Rules self-register; config is resolved once, never per query.** Built-in rules call `analyzer.Register(RuleSpec{...})` from `init()` in `analyzer/rules.go` — a stable name, default severity, and a settings-aware `Factory`. To add a rule, write it and add one `Register` call; **do not** hand-maintain a rule list in `Default()`. Being addressable by name is what makes enable/disable, severity overrides, per-rule `Settings`, and suppressions work uniformly. `analyzer.Profile` (disabled set, `only` whitelist, severity map, per-rule settings) is applied in `DefaultWithProfile` at construction; the per-query `Analyze` path does no config work (it runs on every query through the driver — keep it allocation-light). `analyzer` must stay free of `config`/YAML imports. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4420ad1..b3c1dca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,18 @@ the same version in lockstep. ### Fixed +- **A `WHERE` or `LIMIT` inside a subquery counts only where it bounds the + statement** ([#91]). The default parser counted one anywhere and the + dialect parsers only at the top level, so the parsers reported findings the + default parser did not. Both now count the top level, and for a `SELECT` a + derived table, CTE body or parenthesised set-operation operand; one inside + `IN (...)`, a scalar subquery or a function argument does not. The default + parser therefore reports some findings it used to miss: + `UPDATE t SET a = (SELECT b FROM u WHERE …)` gets `update-without-where` + (it updates every row), and `… WHERE id IN (SELECT … LIMIT 1) ORDER BY a` + gets `orderby-without-limit`. The parsers take the derived-table, CTE and + set-operand part from the default parser, which replaces the set-operation + special case. - **A `sqlguard:ignore` inside a string literal no longer suppresses anything** ([#66]). The in-SQL directive only had to follow a comment marker somewhere earlier in the text, and the marker could itself be inside @@ -66,6 +78,7 @@ the same version in lockstep. [#66]: https://github.com/KARTIKrocks/sqlguard/issues/66 [#81]: https://github.com/KARTIKrocks/sqlguard/issues/81 +[#91]: https://github.com/KARTIKrocks/sqlguard/issues/91 [#82]: https://github.com/KARTIKrocks/sqlguard/issues/82 ## [0.5.0] - 2026-09-25 diff --git a/analyzer/fallback.go b/analyzer/fallback.go index e9f11c6..eec96a4 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -136,8 +136,7 @@ func (p *FallbackParser) Parse(sql string) (*Statement, error) { sanitized := blankStringLiterals(noComments) st.Kind = detectKind(sanitized) - st.HasWhere = fbWhereRe.MatchString(sanitized) - st.HasLimit = fbLimitRe.MatchString(sanitized) + st.HasWhere, st.HasLimit = scopedBounds(sanitized, st.Kind) st.HasOrderBy = hasTopLevelOrderBy(sanitized) st.HasFrom = fbFromRe.MatchString(sanitized) st.SelectStar = fbSelectStarRe.MatchString(sanitized) @@ -468,23 +467,155 @@ func unwrapStatementParens(sanitized string) string { // parenthesis depth zero, so subquery and function-argument keywords are // ignored. func fromRegion(sanitized string) string { - fromEnd := -1 - for _, loc := range fbFromRe.FindAllStringIndex(sanitized, -1) { + lo, hi := fromRegionBounds(sanitized) + return sanitized[lo:hi] +} + +// fromRegionBounds is fromRegion as a byte range; lo == hi when there is no +// top-level FROM. +func fromRegionBounds(sanitized string) (lo, hi int) { + if r := fromRegions(sanitized); len(r) > 0 { + return r[0].lo, r[0].hi + } + return 0, 0 +} + +// fromRegions returns every top-level FROM clause, one per SELECT arm of a set +// operation, each running to the next top-level clause keyword. +func fromRegions(sanitized string) []span { + var ends []int + for _, loc := range fbFromRegionEndRe.FindAllStringIndex(sanitized, -1) { if parenDepthBefore(sanitized, loc[0]) == 0 { - fromEnd = loc[1] - break + ends = append(ends, loc[0]) } } - if fromEnd == -1 { - return "" + var out []span + for _, loc := range fbFromRe.FindAllStringIndex(sanitized, -1) { + if parenDepthBefore(sanitized, loc[0]) != 0 { + continue + } + hi := len(sanitized) + for _, e := range ends { + if e >= loc[1] { + hi = e + break + } + } + out = append(out, span{loc[1], hi}) } - region := sanitized[fromEnd:] - for _, loc := range fbFromRegionEndRe.FindAllStringIndex(region, -1) { - if parenDepthBefore(region, loc[0]) == 0 { - return region[:loc[0]] + return out +} + +// scopedBounds reports whether a WHERE and a LIMIT bound the statement's rows: +// at the top level, or for a SELECT also inside a FROM-clause derived table, a +// CTE body or a set-operation operand. One inside IN (...), a scalar subquery +// or a function argument bounds only that subquery (#91). +func scopedBounds(sanitized string, kind StmtKind) (where, limit bool) { + s := unwrapStatementParens(sanitized) + var sources []span + if kind == StmtSelect { + sources = rowSourceSpans(s) + } + scoped := func(re *regexp.Regexp) bool { + for _, loc := range re.FindAllStringIndex(s, -1) { + if parenDepthBefore(s, loc[0]) == 0 || inSpan(sources, loc[0]) { + return true + } } + return false } - return region + return scoped(fbWhereRe), scoped(fbLimitRe) +} + +// rowSourceSpans returns the top-level parenthesised groups that feed rows to +// the statement: derived tables, CTE bodies and set-operation operands. An ON +// condition or a function argument in FROM is not one. +func rowSourceSpans(s string) []span { + froms := fromRegions(s) + var out []span + depth := 0 + for i := range len(s) { + switch s[i] { + case '(': + if depth == 0 && opensRowSource(s[:i], inSpan(froms, i)) { + out = append(out, span{i, closingParen(s, i)}) + } + depth++ + case ')': + if depth > 0 { + depth-- + } + } + } + return out +} + +// opensRowSource reports whether a "(" after prefix opens a row source. +func opensRowSource(prefix string, inFrom bool) bool { + p := strings.TrimRight(prefix, " \t\r\n") + if p == "" || (inFrom && strings.HasSuffix(p, ",")) { + return true + } + for _, w := range []string{"FROM", "JOIN", "LATERAL"} { + if hasTrailingWord(p, w) { + return true + } + } + q := trimTrailingWord(trimTrailingWord(p, "ALL"), "DISTINCT") + for _, w := range []string{"UNION", "INTERSECT", "EXCEPT"} { + if hasTrailingWord(q, w) { + return true + } + } + return opensCTEBody(p) +} + +// opensCTEBody reports whether prefix ends in AS [NOT] [MATERIALIZED], the +// words before a CTE body's "(". +func opensCTEBody(prefix string) bool { + p := trimTrailingWord(strings.TrimRight(prefix, " \t\r\n"), "MATERIALIZED") + p = trimTrailingWord(p, "NOT") + return hasTrailingWord(p, "AS") +} + +func trimTrailingWord(p, w string) string { + if hasTrailingWord(p, w) { + return strings.TrimRight(p[:len(p)-len(w)], " \t\r\n") + } + return p +} + +func hasTrailingWord(p, w string) bool { + if len(p) < len(w) || !strings.EqualFold(p[len(p)-len(w):], w) { + return false + } + return len(p) == len(w) || !isIdentByte(p[len(p)-len(w)-1]) +} + +// closingParen returns the index just past the ')' matching s[open], or len(s). +func closingParen(s string, open int) int { + depth := 0 + for i := open; i < len(s); i++ { + switch s[i] { + case '(': + depth++ + case ')': + depth-- + if depth == 0 { + return i + 1 + } + } + } + return len(s) +} + +func inSpan(spans []span, pos int) bool { + for _, sp := range spans { + if pos >= sp.lo && pos < sp.hi { + return true + } + } + return false } // parenDepthBefore returns the net parenthesis nesting depth at index idx diff --git a/analyzer/fallback_test.go b/analyzer/fallback_test.go index f455e94..4b870f8 100644 --- a/analyzer/fallback_test.go +++ b/analyzer/fallback_test.go @@ -145,3 +145,44 @@ func TestFallbackParserDollarQuotedBody(t *testing.T) { }) } } + +// TestFallbackScopedWhereLimit pins which WHERE / LIMIT bound a statement's +// rows (#91): top level, and for a SELECT a derived table, CTE body or +// parenthesised set-operation operand; never one inside IN (...), a scalar +// subquery or a function argument. +func TestFallbackScopedWhereLimit(t *testing.T) { + tests := []struct { + sql string + wantWhere, wantLimit bool + }{ + {"SELECT a FROM t WHERE x = 1 LIMIT 5", true, true}, + {"SELECT a FROM (SELECT a FROM t LIMIT 3) s", false, true}, + {"SELECT a FROM (SELECT a FROM t WHERE x = 1) s", true, false}, + {"SELECT a FROM t JOIN (SELECT id FROM u LIMIT 3) s ON s.id = t.id", false, true}, + {"WITH c AS (SELECT a FROM t WHERE x = 1 LIMIT 3) SELECT a FROM c", true, true}, + {"WITH c AS MATERIALIZED (SELECT a FROM t LIMIT 3) SELECT a FROM c", false, true}, + {"SELECT a FROM t WHERE id IN (SELECT id FROM u LIMIT 1)", true, false}, + {"SELECT (SELECT max(b) FROM u WHERE u.id = t.id) FROM t", false, false}, + {"SELECT ARRAY(SELECT b FROM u LIMIT 5) FROM t", false, false}, + {"(SELECT a FROM t WHERE x = 1 LIMIT 5)", true, true}, + {"UPDATE t SET a = (SELECT b FROM u WHERE u.id = t.id)", false, false}, + {"WITH c AS (SELECT id FROM u WHERE x = 1) DELETE FROM t", false, false}, + {"DELETE FROM t WHERE id IN (SELECT id FROM u)", true, false}, + {"(SELECT a FROM t WHERE id = 1) UNION ALL (SELECT b FROM u WHERE id = 2)", true, false}, + {"(SELECT a FROM t ORDER BY a LIMIT 10) UNION ALL (SELECT b FROM u ORDER BY b LIMIT 10)", false, true}, + {"SELECT * FROM t JOIN u ON u.id IN (SELECT id FROM v WHERE flag = 1)", false, false}, + {"SELECT * FROM t, (SELECT id FROM v LIMIT 3) s", false, true}, + {"SELECT a FROM t WHERE x = 1 UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", true, true}, + {"SELECT * FROM t CROSS JOIN LATERAL (SELECT id FROM v WHERE v.a = t.a LIMIT 1) s", true, true}, + {"SELECT a, (SELECT b FROM u LIMIT 1) FROM t", false, false}, + {"SELECT * FROM generate_series(1, (SELECT max(n) FROM u WHERE x = 1))", false, false}, + } + for _, tt := range tests { + t.Run(tt.sql, func(t *testing.T) { + st, _ := NewFallbackParser().Parse(tt.sql) + if st.HasWhere != tt.wantWhere || st.HasLimit != tt.wantLimit { + t.Errorf("HasWhere=%v HasLimit=%v, want %v %v", st.HasWhere, st.HasLimit, tt.wantWhere, tt.wantLimit) + } + }) + } +} diff --git a/analyzer/statement.go b/analyzer/statement.go index 325da16..b97546b 100644 --- a/analyzer/statement.go +++ b/analyzer/statement.go @@ -40,10 +40,14 @@ type Statement struct { // Kind is the statement's top-level kind. Kind StmtKind - // HasWhere reports whether the statement has a WHERE clause. + // HasWhere reports whether a WHERE filters the statement's rows: at the + // top level, or for a SELECT inside a derived table, CTE body or + // parenthesised set-operation operand. One inside IN (...) or a scalar + // subquery does not count. HasWhere bool - // HasLimit reports whether the statement has a LIMIT clause. + // HasLimit reports whether a LIMIT bounds the statement, scoped like + // HasWhere. The fallback counts the bare keyword, so LIMIT ALL counts. HasLimit bool // HasOrderBy reports whether the statement has an ORDER BY clause. @@ -146,9 +150,8 @@ type Statement struct { // ImplicitCommaJoin, CartesianJoin, and the literal/text-level fields // (LeadingWildcard*, NonSargablePredicate, AddNotNullNoDefault) — because // they read literal values the AST discards or are intentionally text-level. - // Each such field documents this. For a set operation (UNION / INTERSECT / - // EXCEPT) HasWhere and HasLimit are taken from the fallback too, which - // counts a WHERE or LIMIT anywhere in the text, so the grammar can never - // report select-without-limit where the default parser does not. + // Each such field documents this. For a SELECT, the dialect parsers read + // HasWhere and HasLimit from the AST at the top level and take the + // derived-table, CTE and set-operand part from the fallback. Exact bool } diff --git a/parsers/mysqlparser/mysqlparser.go b/parsers/mysqlparser/mysqlparser.go index fdc6689..e457dc6 100644 --- a/parsers/mysqlparser/mysqlparser.go +++ b/parsers/mysqlparser/mysqlparser.go @@ -58,9 +58,11 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { switch n := ast.(type) { case *sqlparser.Select: + fb := *st resetStructural(st) st.Kind = analyzer.StmtSelect fillSelect(st, n, true) + keepFallbackBounds(st, &fb) case *sqlparser.Union: fb := *st resetStructural(st) @@ -132,7 +134,7 @@ func hasRowLimit(lim *sqlparser.Limit) bool { // st. top is false for the operands of a set operation: a FROM, a star, a // DISTINCT or a literal OFFSET in either operand is one in the statement, but // an operand's ORDER BY orders that operand rather than the result, and its -// WHERE and LIMIT are left to keepFallbackBounds. +// WHERE and LIMIT come from keepFallbackBounds. func fillSelect(st *analyzer.Statement, sel sqlparser.SelectStatement, top bool) { switch s := sel.(type) { case *sqlparser.Select: @@ -158,14 +160,11 @@ func fillSelect(st *analyzer.Statement, sel sqlparser.SelectStatement, top bool) } } -// keepFallbackBounds takes a set operation's WHERE and LIMIT presence from the -// fallback, which counts them anywhere in the text — inside an operand's -// subquery too. Reading them from the operands' top level instead reports -// select-without-limit on SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u -// LIMIT 3) s, which the fallback does not, and a parser may only remove -// findings. A LIMIT on the whole result is still read from the AST. +// keepFallbackBounds ORs in the fallback's WHERE / LIMIT, which also counts +// one inside a derived table, a CTE body or a set-operation +// operand. The AST's own answer covers only the top level (#91). func keepFallbackBounds(st, fb *analyzer.Statement) { - st.HasWhere = fb.HasWhere + st.HasWhere = st.HasWhere || fb.HasWhere st.HasLimit = st.HasLimit || fb.HasLimit } diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index 0f2d74c..5ab7527 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -315,17 +315,33 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "CREATE VIEW v AS SELECT * FROM t", "EXPLAIN SELECT * FROM t", "SHOW TABLES", + // A WHERE / LIMIT only some subqueries bound (#91). + "(SELECT a FROM t WHERE id = 1) UNION ALL (SELECT b FROM u WHERE id = 2)", + "(SELECT a FROM t ORDER BY a LIMIT 10) UNION ALL (SELECT b FROM u ORDER BY b LIMIT 10)", + "SELECT * FROM t JOIN u ON u.id IN (SELECT id FROM v WHERE flag = 1)", + "SELECT a FROM (SELECT a FROM t LIMIT 3) s", + "SELECT a FROM t UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", + "SELECT a FROM (SELECT a FROM t WHERE x = 1) s", + "WITH c AS (SELECT a FROM t WHERE x = 1 LIMIT 3) SELECT a FROM c", + "SELECT a FROM t WHERE id IN (SELECT id FROM u LIMIT 1) ORDER BY a", + "SELECT (SELECT max(b) FROM u WHERE u.id = t.id) FROM t", + "UPDATE t SET a = (SELECT b FROM u WHERE u.id = t.id)", + "DELETE t FROM t JOIN (SELECT id FROM u WHERE x = 1) s ON s.id = t.id", } fallback := analyzer.Default() exact := analyzer.Default().WithParser(New()) coreKnows := fallbackKnowsInsertLikeKeywords() + coreScopes := fallbackScopesWhereLimit() for _, sql := range corpus { t.Run(sql, func(t *testing.T) { if !coreKnows && needsCoreKeywordSupport(sql) { t.Skip("linked core predates the fallback's row-inserting keyword support") } + if !coreScopes && strings.Contains(sql, "(SELECT") { + t.Skip("linked core predates the fallback's scoped WHERE / LIMIT (#91)") + } base := ruleSet(fallback.Analyze(sql)) for name := range ruleSet(exact.Analyze(sql)) { if _, ok := base[name]; !ok { @@ -371,3 +387,10 @@ func fallbackKnowsInsertLikeKeywords() bool { st, _ := analyzer.NewFallbackParser().Parse("REPLACE INTO t VALUES (1)") return st.Kind == analyzer.StmtInsert } + +// fallbackScopesWhereLimit reports whether the linked core ignores a WHERE +// inside a scalar subquery when deciding whether a statement is filtered. +func fallbackScopesWhereLimit() bool { + st, _ := analyzer.NewFallbackParser().Parse("SELECT (SELECT b FROM u WHERE x = 1) FROM t") + return !st.HasWhere +} diff --git a/parsers/pgparser/pgparser.go b/parsers/pgparser/pgparser.go index 86a2f04..14b1fd6 100644 --- a/parsers/pgparser/pgparser.go +++ b/parsers/pgparser/pgparser.go @@ -62,13 +62,13 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { st.HasLimit = hasRowLimit(n.Limit) st.OffsetValue = offsetValue(n.Limit) fillSelectBody(st, n.Select) - if isSetOperation(n.Select) { - keepFallbackBounds(st, &fb) - } + keepFallbackBounds(st, &fb) case *tree.SelectClause: + fb := *st resetStructural(st) st.Kind = analyzer.StmtSelect fillSelectClause(st, n) + keepFallbackBounds(st, &fb) case *tree.Delete: resetStructural(st) st.Kind = analyzer.StmtDelete @@ -162,7 +162,7 @@ func fillSelectBody(st *analyzer.Statement, sel tree.SelectStatement) { // mergeArm folds one operand of a set operation (UNION / INTERSECT / EXCEPT) // into st: a FROM, a star, a DISTINCT or a literal OFFSET in either operand is -// one in the statement. Its WHERE and LIMIT are left to keepFallbackBounds, and +// one in the statement. Its WHERE and LIMIT come from keepFallbackBounds, and // its ORDER BY orders that operand, not the result, so it is not merged. func mergeArm(st *analyzer.Statement, arm *tree.Select) { if arm == nil { @@ -177,32 +177,11 @@ func mergeArm(st *analyzer.Statement, arm *tree.Select) { st.OffsetValue = max(st.OffsetValue, a.OffsetValue) } -// isSetOperation reports whether a select body is a UNION / INTERSECT / -// EXCEPT, looking through parentheses around the whole of it. -func isSetOperation(sel tree.SelectStatement) bool { - for { - switch c := sel.(type) { - case *tree.UnionClause: - return true - case *tree.ParenSelect: - if c.Select == nil { - return false - } - sel = c.Select.Select - default: - return false - } - } -} - -// keepFallbackBounds takes a set operation's WHERE and LIMIT presence from the -// fallback, which counts them anywhere in the text — inside an operand's -// subquery too. Reading them from the operands' top level instead reports -// select-without-limit on SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u -// LIMIT 3) s, which the fallback does not, and a parser may only remove -// findings. A LIMIT on the whole result is still read from the AST. +// keepFallbackBounds ORs in the fallback's WHERE / LIMIT, which also counts +// one inside a derived table, a CTE body or a set-operation +// operand. The AST's own answer covers only the top level (#91). func keepFallbackBounds(st, fb *analyzer.Statement) { - st.HasWhere = fb.HasWhere + st.HasWhere = st.HasWhere || fb.HasWhere st.HasLimit = st.HasLimit || fb.HasLimit } diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index bc8f590..b7c4153 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -317,11 +317,24 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "CREATE VIEW v AS SELECT * FROM t", "CREATE TABLE c AS SELECT a FROM t ORDER BY a", "EXPLAIN SELECT * FROM t", + // A WHERE / LIMIT only some subqueries bound (#91). + "(SELECT a FROM t WHERE id = 1) UNION ALL (SELECT b FROM u WHERE id = 2)", + "(SELECT a FROM t ORDER BY a LIMIT 10) UNION ALL (SELECT b FROM u ORDER BY b LIMIT 10)", + "SELECT * FROM t JOIN u ON u.id IN (SELECT id FROM v WHERE flag = 1)", + "SELECT a FROM (SELECT a FROM t LIMIT 3) s", + "SELECT a FROM t UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", + "SELECT a FROM (SELECT a FROM t WHERE x = 1) s", + "SELECT a FROM t WHERE id IN (SELECT id FROM u LIMIT 1) ORDER BY a", + "SELECT (SELECT max(b) FROM u WHERE u.id = t.id) FROM t", + "UPDATE t SET a = (SELECT b FROM u WHERE u.id = t.id)", + "WITH c AS (SELECT a FROM t WHERE x = 1 LIMIT 3) SELECT a FROM c", + "WITH c AS (SELECT id FROM u WHERE x = 1) DELETE FROM t", } fallback := analyzer.Default() exact := analyzer.Default().WithParser(New()) coreKnows := fallbackKnowsInsertLikeKeywords() + coreScopes := fallbackScopesWhereLimit() coreUnwraps := fallbackUnwrapsStatementParens() for _, sql := range corpus { @@ -329,6 +342,9 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { if !coreKnows && needsCoreKeywordSupport(sql) { t.Skip("linked core predates the fallback's row-inserting keyword support") } + if !coreScopes && strings.Contains(sql, "(SELECT") { + t.Skip("linked core predates the fallback's scoped WHERE / LIMIT (#91)") + } if !coreUnwraps && strings.HasPrefix(sql, "(") { t.Skip("linked core predates the fallback reading a parenthesised statement's ORDER BY") } @@ -386,3 +402,10 @@ func fallbackUnwrapsStatementParens() bool { st, _ := analyzer.NewFallbackParser().Parse("(SELECT a FROM t ORDER BY a)") return st.HasOrderBy } + +// fallbackScopesWhereLimit reports whether the linked core ignores a WHERE +// inside a scalar subquery when deciding whether a statement is filtered. +func fallbackScopesWhereLimit() bool { + st, _ := analyzer.NewFallbackParser().Parse("SELECT (SELECT b FROM u WHERE x = 1) FROM t") + return !st.HasWhere +} diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 4aa66f2..e3ccac6 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -78,10 +78,14 @@ The rest stay best-effort heuristics regardless of the parser, and each field's doc comment says so. This is a documented carve-out, not a gap waiting to be closed. -For a set operation (`UNION`, `INTERSECT`, `EXCEPT`), `HasWhere` and -`HasLimit` also stay lexical: the fallback counts a `WHERE` or `LIMIT` -anywhere, including inside an operand's subquery, and reading them per -operand would report findings the fallback does not. +_Changed in 0.6._ `HasWhere` and `HasLimit` count a `WHERE` or `LIMIT` +that bounds the statement's rows: at the top level, or for a `SELECT` +inside a derived table, a CTE body or a parenthesised set-operation +operand. One inside `IN (...)`, a scalar subquery or a function argument +bounds only that subquery and does not count. The parsers read the top +level from the AST and take the derived-table, CTE and set-operand part +from the fallback, so both always agree. Before 0.6 the fallback counted a +`WHERE` or `LIMIT` anywhere and the parsers only at the top level. _Changed in 0.6._ A statement the grammar accepts but the parser does not model — DDL, `EXPLAIN`, `CREATE VIEW … AS SELECT` — keeps the fallback's From 0b15ec68ca8636fa9947b7e61a8b1229d392432d Mon Sep 17 00:00:00 2001 From: kartik Date: Mon, 5 Oct 2026 16:34:07 +0530 Subject: [PATCH 2/5] fix(bunguard): register the hook with WithQueryHook bun v1.3 deprecates AddQueryHook, which fails staticcheck SA1019 in the test setup. WithQueryHook returns a clone, so the examples reassign db. --- integrations/bunguard/bunguard.go | 2 +- integrations/bunguard/bunguard_test.go | 2 +- website/docs/bun.md | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/integrations/bunguard/bunguard.go b/integrations/bunguard/bunguard.go index dc3a5bf..fc67125 100644 --- a/integrations/bunguard/bunguard.go +++ b/integrations/bunguard/bunguard.go @@ -8,7 +8,7 @@ // // sqldb := sql.OpenDB(pgdriver.NewConnector(pgdriver.WithDSN(dsn))) // db := bun.NewDB(sqldb, pgdialect.New()) -// db.AddQueryHook(bunguard.New( +// db = db.WithQueryHook(bunguard.New( // middleware.WithSlowQueryThreshold(500*time.Millisecond), // middleware.WithN1Detection(10, time.Second), // )) diff --git a/integrations/bunguard/bunguard_test.go b/integrations/bunguard/bunguard_test.go index 5b5fbd5..c05bcbc 100644 --- a/integrations/bunguard/bunguard_test.go +++ b/integrations/bunguard/bunguard_test.go @@ -73,7 +73,7 @@ func newDBWithCapture(t *testing.T, opts ...middleware.Option) (*bun.DB, *captur cap := &capture{} opts = append([]middleware.Option{middleware.WithReporter(cap)}, opts...) hook := New(opts...) - db.AddQueryHook(hook) + db = db.WithQueryHook(hook) // Hook registered after seeding, so capture starts clean — every test // asserts only on findings from its own queries. return db, cap, hook diff --git a/website/docs/bun.md b/website/docs/bun.md index 8b5a7f7..85232c2 100644 --- a/website/docs/bun.md +++ b/website/docs/bun.md @@ -26,7 +26,7 @@ import ( sqldb := sql.OpenDB(pgdriver.NewConnector(pgdriver.WithDSN(dsn))) db := bun.NewDB(sqldb, pgdialect.New()) -db.AddQueryHook(bunguard.New( +db = db.WithQueryHook(bunguard.New( middleware.WithSlowQueryThreshold(500*time.Millisecond), middleware.WithN1Detection(10, time.Second), )) @@ -36,13 +36,13 @@ db.AddQueryHook(bunguard.New( | Symbol | Use | | --- | --- | -| `bunguard.New(opts ...middleware.Option) *QueryHook` | Build the hook. Pass to `db.AddQueryHook`. | +| `bunguard.New(opts ...middleware.Option) *QueryHook` | Build the hook. Pass to `db.WithQueryHook`. | | `(*QueryHook).ResetN1()` | Clear N+1 state at a request boundary. | | `BeforeQuery`, `AfterQuery` | The `bun.QueryHook` methods; you do not call them. | ```go hook := bunguard.New(middleware.WithN1Detection(10, time.Second)) -db.AddQueryHook(hook) +db = db.WithQueryHook(hook) func handler(w http.ResponseWriter, r *http.Request) { defer hook.ResetN1() From 786edcfd3335c5d189d6cb4ccd58c62ed1e0d370 Mon Sep 17 00:00:00 2001 From: kartik Date: Mon, 5 Oct 2026 16:37:57 +0530 Subject: [PATCH 3/5] perf(analyzer): skip row-source scan when a keyword is absent or top-level Most queries have no LIMIT, so scopedBounds now returns on a plain match check and builds the row-source spans only when a keyword sits inside parentheses. --- analyzer/fallback.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/analyzer/fallback.go b/analyzer/fallback.go index eec96a4..fa75f74 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -513,12 +513,19 @@ func fromRegions(sanitized string) []span { func scopedBounds(sanitized string, kind StmtKind) (where, limit bool) { s := unwrapStatementParens(sanitized) var sources []span - if kind == StmtSelect { - sources = rowSourceSpans(s) - } + haveSources := false scoped := func(re *regexp.Regexp) bool { + if !re.MatchString(s) { + return false + } for _, loc := range re.FindAllStringIndex(s, -1) { - if parenDepthBefore(s, loc[0]) == 0 || inSpan(sources, loc[0]) { + if parenDepthBefore(s, loc[0]) == 0 { + return true + } + if kind == StmtSelect && !haveSources { + sources, haveSources = rowSourceSpans(s), true + } + if inSpan(sources, loc[0]) { return true } } From 1cac5e583c69eb0be753c57159ada232195ac9e6 Mon Sep 17 00:00:00 2001 From: kartik Date: Mon, 5 Oct 2026 16:41:20 +0530 Subject: [PATCH 4/5] fix(analyzer): scope WHERE/LIMIT in one pass, through nested parentheses scopedBounds now walks the text once and counts a keyword only where every enclosing parenthesis is a row source, so a scalar subquery nested inside a derived table no longer bounds the statement. It also replaces the per-match prefix rescans (quadratic in the number of clauses) in scopedBounds and fromRegions. --- AGENTS.md | 2 +- analyzer/fallback.go | 152 +++++++++++++----------- analyzer/fallback_test.go | 1 + parsers/mysqlparser/mysqlparser_test.go | 1 + parsers/pgparser/pgparser_test.go | 1 + 5 files changed, 88 insertions(+), 69 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4747514..d3442ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ When changing the public API or Go version, update all nine `go.mod` files and ` **The analyzer is parser-pluggable.** `analyzer.Analyzer` runs `Rule`s against a normalized, dialect-agnostic `Statement` produced by an `analyzer.Parser` (`analyzer/parser.go`, `statement.go`). The default `FallbackParser` (`fallback.go`) is zero-dependency, strips comments/string literals, and never errors. `analyzer.Analyze` degrades to the FallbackParser if a configured parser errors, so analysis never breaks the caller's query path. Real grammars are supplied via `middleware.WithParser(...)` / `analyzer.Default().WithParser(...)` using the `parsers/*` modules. Rules read the `Statement`, never raw SQL. -**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. The converse holds too: **a dialect parser resets the structural fields only inside a `case` for an AST node it models, and any other node returns the fallback's `Statement` untouched with `Exact = false`.** Blanking them up front dropped every finding on `CREATE VIEW v AS SELECT * FROM t`, `EXPLAIN SELECT *` and the like (#81), because nothing had been derived to replace them — a false negative the "never adds" test cannot see, so `TestParser_KeepsFallbackFactsForUnmodelledStatements` pins it. A field refilled from the AST must mean what its name says: `HasLimit` is a row count, not the presence of a limit node, which pgparser also builds for a bare `OFFSET` (#82). `HasWhere`/`HasLimit` count a clause only where it bounds the statement's rows: the top level, and for a `SELECT` a derived table, CTE body or parenthesised set-operation operand (`scopedBounds` / `rowSourceSpans`), never `IN (...)`, a scalar subquery or a function argument. The parsers OR their top-level AST answer with the fallback's (`keepFallbackBounds`) for every `SELECT`, so the two agree by construction (#91). +**A dialect parser may only remove findings, never add one.** Opting into a real grammar is sold as trading false positives away; a grammar-only finding inverts that, and the reporting surface is identical, so nothing warns. `TestParser_NeverAddsFindingTheFallbackDoesNot` in each `parsers/*` module runs a corpus through both the grammar and the FallbackParser and fails on any rule the grammar reports and the fallback does not. It is an invariant over a corpus, not a proof: the shape that breaks it is a statement the grammar understands and the fallback's keyword list does not, which is exactly how every instance in #68 arose (`INSERT ... DEFAULT VALUES` and `UPSERT`, plain and CTE-prefixed, under `pgparser`; `REPLACE` and the `INTO`-less `INSERT t VALUES (...)` under `mysqlparser`). So **when a parser learns a statement kind, the fallback has to learn it too** — `detectKind` and `insertColumnsListed` are the pair to check — and a new structural field read by a rule needs a corpus row here. `detectKind` recognizes the insert-like keywords in two places, leading and after a `WITH` clause, and both require a table name after the keyword so a `REPLACE(str, from, to)` call is never read as the statement. `insertColumnsListed` anchors on the **statement head** whenever it starts the statement and falls back to the `INTO` clause only for the CTE-prefixed forms, where the keyword sits mid-statement and can also occur inside the CTE body. That order matters: searching for `INTO` first matches an identifier named `into` in the column list of a form that omits the keyword, reads the list as the target table, and reports a statement that does name its columns. Because the test compares a satellite module against the core's fallback, a corpus row depending on core support the published core lacks is skipped rather than failed, so `GOWORK=off` stays green between lockstep tags. The converse holds too: **a dialect parser resets the structural fields only inside a `case` for an AST node it models, and any other node returns the fallback's `Statement` untouched with `Exact = false`.** Blanking them up front dropped every finding on `CREATE VIEW v AS SELECT * FROM t`, `EXPLAIN SELECT *` and the like (#81), because nothing had been derived to replace them — a false negative the "never adds" test cannot see, so `TestParser_KeepsFallbackFactsForUnmodelledStatements` pins it. A field refilled from the AST must mean what its name says: `HasLimit` is a row count, not the presence of a limit node, which pgparser also builds for a bare `OFFSET` (#82). `HasWhere`/`HasLimit` count a clause only where it bounds the statement's rows: the top level, and for a `SELECT` a derived table, CTE body or parenthesised set-operation operand (`scopedBounds`, which counts a keyword only where every enclosing parenthesis is a row source, so a scalar subquery nested inside a derived table does not count either), never `IN (...)`, a scalar subquery or a function argument. The parsers OR their top-level AST answer with the fallback's (`keepFallbackBounds`) for every `SELECT`, so the two agree by construction (#91). **Rules self-register; config is resolved once, never per query.** Built-in rules call `analyzer.Register(RuleSpec{...})` from `init()` in `analyzer/rules.go` — a stable name, default severity, and a settings-aware `Factory`. To add a rule, write it and add one `Register` call; **do not** hand-maintain a rule list in `Default()`. Being addressable by name is what makes enable/disable, severity overrides, per-rule `Settings`, and suppressions work uniformly. `analyzer.Profile` (disabled set, `only` whitelist, severity map, per-rule settings) is applied in `DefaultWithProfile` at construction; the per-query `Analyze` path does no config work (it runs on every query through the driver — keep it allocation-light). `analyzer` must stay free of `config`/YAML imports. diff --git a/analyzer/fallback.go b/analyzer/fallback.go index fa75f74..de42e2e 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -483,21 +483,14 @@ func fromRegionBounds(sanitized string) (lo, hi int) { // fromRegions returns every top-level FROM clause, one per SELECT arm of a set // operation, each running to the next top-level clause keyword. func fromRegions(sanitized string) []span { - var ends []int - for _, loc := range fbFromRegionEndRe.FindAllStringIndex(sanitized, -1) { - if parenDepthBefore(sanitized, loc[0]) == 0 { - ends = append(ends, loc[0]) - } - } - var out []span - for _, loc := range fbFromRe.FindAllStringIndex(sanitized, -1) { - if parenDepthBefore(sanitized, loc[0]) != 0 { - continue - } + ends := topLevelMatches(sanitized, fbFromRegionEndRe) + froms := topLevelMatches(sanitized, fbFromRe) + out := make([]span, 0, len(froms)) + for _, loc := range froms { hi := len(sanitized) for _, e := range ends { - if e >= loc[1] { - hi = e + if e[0] >= loc[1] { + hi = e[0] break } } @@ -506,55 +499,95 @@ func fromRegions(sanitized string) []span { return out } -// scopedBounds reports whether a WHERE and a LIMIT bound the statement's rows: -// at the top level, or for a SELECT also inside a FROM-clause derived table, a -// CTE body or a set-operation operand. One inside IN (...), a scalar subquery -// or a function argument bounds only that subquery (#91). -func scopedBounds(sanitized string, kind StmtKind) (where, limit bool) { - s := unwrapStatementParens(sanitized) - var sources []span - haveSources := false - scoped := func(re *regexp.Regexp) bool { - if !re.MatchString(s) { - return false - } - for _, loc := range re.FindAllStringIndex(s, -1) { - if parenDepthBefore(s, loc[0]) == 0 { - return true - } - if kind == StmtSelect && !haveSources { - sources, haveSources = rowSourceSpans(s), true - } - if inSpan(sources, loc[0]) { - return true +// topLevelMatches returns the matches of re at parenthesis depth zero, finding +// the depth in one pass over s rather than rescanning the prefix per match. +func topLevelMatches(s string, re *regexp.Regexp) [][]int { + locs := re.FindAllStringIndex(s, -1) + out := locs[:0] + depth, pos := 0, 0 + for _, loc := range locs { + for ; pos < loc[0]; pos++ { + switch s[pos] { + case '(': + depth++ + case ')': + depth = max(depth-1, 0) } } - return false + if depth == 0 { + out = append(out, loc) + } } - return scoped(fbWhereRe), scoped(fbLimitRe) + return out } -// rowSourceSpans returns the top-level parenthesised groups that feed rows to -// the statement: derived tables, CTE bodies and set-operation operands. An ON -// condition or a function argument in FROM is not one. -func rowSourceSpans(s string) []span { - froms := fromRegions(s) - var out []span - depth := 0 +// scopedBounds reports whether a WHERE and a LIMIT bound the statement's rows: +// at the top level, or for a SELECT also where every enclosing parenthesis is a +// row source (a FROM-clause derived table, a CTE body or a set-operation +// operand). One inside IN (...), a scalar subquery or a function argument +// bounds only that subquery, however deeply it is nested in a derived table +// (#91). One pass over the text, and none when neither keyword occurs. +func scopedBounds(sanitized string, kind StmtKind) (where, limit bool) { + s := unwrapStatementParens(sanitized) + wl := fbWhereRe.FindAllStringIndex(s, -1) + ll := fbLimitRe.FindAllStringIndex(s, -1) + if len(wl) == 0 && len(ll) == 0 { + return false, false + } + var froms []span + if kind == StmtSelect { + froms = fromRegions(s) + } + var sc rowScope for i := range len(s) { + var hit bool + if wl, hit = advance(wl, i); hit && sc.nonRow == 0 { + where = true + } + if ll, hit = advance(ll, i); hit && sc.nonRow == 0 { + limit = true + } switch s[i] { case '(': - if depth == 0 && opensRowSource(s[:i], inSpan(froms, i)) { - out = append(out, span{i, closingParen(s, i)}) - } - depth++ + sc.push(kind != StmtSelect || !opensRowSource(s[:i], len(sc.open) == 0 && inSpan(froms, i))) case ')': - if depth > 0 { - depth-- - } + sc.pop() } } - return out + return where, limit +} + +// rowScope tracks the open parentheses: open[i] is true when the i-th is not a +// row source, and nonRow counts those. +type rowScope struct { + open []bool + nonRow int +} + +func (r *rowScope) push(notRowSource bool) { + r.open = append(r.open, notRowSource) + if notRowSource { + r.nonRow++ + } +} + +func (r *rowScope) pop() { + n := len(r.open) + if n == 0 { + return + } + if r.open[n-1] { + r.nonRow-- + } + r.open = r.open[:n-1] +} + +// advance consumes locs[0] when it starts at i. +func advance(locs [][]int, i int) ([][]int, bool) { + if len(locs) > 0 && locs[0][0] == i { + return locs[1:], true + } + return locs, false } // opensRowSource reports whether a "(" after prefix opens a row source. @@ -599,23 +632,6 @@ func hasTrailingWord(p, w string) bool { return len(p) == len(w) || !isIdentByte(p[len(p)-len(w)-1]) } -// closingParen returns the index just past the ')' matching s[open], or len(s). -func closingParen(s string, open int) int { - depth := 0 - for i := open; i < len(s); i++ { - switch s[i] { - case '(': - depth++ - case ')': - depth-- - if depth == 0 { - return i + 1 - } - } - } - return len(s) -} - func inSpan(spans []span, pos int) bool { for _, sp := range spans { if pos >= sp.lo && pos < sp.hi { diff --git a/analyzer/fallback_test.go b/analyzer/fallback_test.go index 4b870f8..dc59d44 100644 --- a/analyzer/fallback_test.go +++ b/analyzer/fallback_test.go @@ -175,6 +175,7 @@ func TestFallbackScopedWhereLimit(t *testing.T) { {"SELECT a FROM t WHERE x = 1 UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", true, true}, {"SELECT * FROM t CROSS JOIN LATERAL (SELECT id FROM v WHERE v.a = t.a LIMIT 1) s", true, true}, {"SELECT a, (SELECT b FROM u LIMIT 1) FROM t", false, false}, + {"SELECT a FROM (SELECT a, (SELECT 1 FROM v WHERE v.x = 1 LIMIT 1) FROM t) s", false, false}, {"SELECT * FROM generate_series(1, (SELECT max(n) FROM u WHERE x = 1))", false, false}, } for _, tt := range tests { diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index 5ab7527..bd7c4da 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -320,6 +320,7 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "(SELECT a FROM t ORDER BY a LIMIT 10) UNION ALL (SELECT b FROM u ORDER BY b LIMIT 10)", "SELECT * FROM t JOIN u ON u.id IN (SELECT id FROM v WHERE flag = 1)", "SELECT a FROM (SELECT a FROM t LIMIT 3) s", + "SELECT a FROM (SELECT a, (SELECT 1 FROM v WHERE v.x = 1) FROM t) s", "SELECT a FROM t UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", "SELECT a FROM (SELECT a FROM t WHERE x = 1) s", "WITH c AS (SELECT a FROM t WHERE x = 1 LIMIT 3) SELECT a FROM c", diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index b7c4153..3fe6fbe 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -322,6 +322,7 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "(SELECT a FROM t ORDER BY a LIMIT 10) UNION ALL (SELECT b FROM u ORDER BY b LIMIT 10)", "SELECT * FROM t JOIN u ON u.id IN (SELECT id FROM v WHERE flag = 1)", "SELECT a FROM (SELECT a FROM t LIMIT 3) s", + "SELECT a FROM (SELECT a, (SELECT 1 FROM v WHERE v.x = 1) FROM t) s", "SELECT a FROM t UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", "SELECT a FROM (SELECT a FROM t WHERE x = 1) s", "SELECT a FROM t WHERE id IN (SELECT id FROM u LIMIT 1) ORDER BY a", From 0379485aea4249a2171c9bb85e2ce370bcb6d75f Mon Sep 17 00:00:00 2001 From: kartik Date: Mon, 5 Oct 2026 16:51:10 +0530 Subject: [PATCH 5/5] fix(analyzer): IS [NOT] DISTINCT FROM (...) is not a row source A parenthesis after DISTINCT FROM holds a scalar subquery, not a derived table, so a WHERE or LIMIT inside it no longer bounds the statement. --- analyzer/fallback.go | 6 +++++- analyzer/fallback_test.go | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/analyzer/fallback.go b/analyzer/fallback.go index de42e2e..e570ba3 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -596,7 +596,11 @@ func opensRowSource(prefix string, inFrom bool) bool { if p == "" || (inFrom && strings.HasSuffix(p, ",")) { return true } - for _, w := range []string{"FROM", "JOIN", "LATERAL"} { + // IS [NOT] DISTINCT FROM (...) compares against a scalar subquery. + if hasTrailingWord(p, "FROM") && !hasTrailingWord(trimTrailingWord(p, "FROM"), "DISTINCT") { + return true + } + for _, w := range []string{"JOIN", "LATERAL"} { if hasTrailingWord(p, w) { return true } diff --git a/analyzer/fallback_test.go b/analyzer/fallback_test.go index dc59d44..c569a2d 100644 --- a/analyzer/fallback_test.go +++ b/analyzer/fallback_test.go @@ -175,6 +175,9 @@ func TestFallbackScopedWhereLimit(t *testing.T) { {"SELECT a FROM t WHERE x = 1 UNION SELECT a FROM u, (SELECT 1 FROM v LIMIT 1) s", true, true}, {"SELECT * FROM t CROSS JOIN LATERAL (SELECT id FROM v WHERE v.a = t.a LIMIT 1) s", true, true}, {"SELECT a, (SELECT b FROM u LIMIT 1) FROM t", false, false}, + {"SELECT a FROM t WHERE a IS DISTINCT FROM (SELECT x FROM u LIMIT 1)", true, false}, + {"SELECT a IS DISTINCT FROM (SELECT x FROM u WHERE y = 1 LIMIT 1) FROM t", false, false}, + {"SELECT a IS NOT DISTINCT FROM (SELECT x FROM u WHERE y = 1 LIMIT 1) FROM t", false, false}, {"SELECT a FROM (SELECT a, (SELECT 1 FROM v WHERE v.x = 1 LIMIT 1) FROM t) s", false, false}, {"SELECT * FROM generate_series(1, (SELECT max(n) FROM u WHERE x = 1))", false, false}, }