From 79a3b516cccab588e20f3c7680f584e869de6d3d Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 05:18:35 +0530 Subject: [PATCH 1/4] fix(parsers): keep fallback facts for unmodelled statements, bound HasLimit by row count Both dialect parsers blanked every structural field before switching on the AST and refilled them only for SELECT/INSERT/UPDATE/DELETE, so any other statement the grammar accepted lost what the fallback had found and was still marked Exact: CREATE VIEW ... AS SELECT *, CREATE TABLE ... AS SELECT * and EXPLAIN SELECT * all dropped select-star. Fields are now reset only inside a handled case; anything else returns the fallback's Statement untouched with Exact=false (#81). Both parsers also read the row source of INSERT ... SELECT *, and mysqlparser reads a UNION's ORDER BY/LIMIT as pgparser does instead of treating it as StmtOther. pgparser set HasLimit from the presence of a limit node, which the grammar builds for a bare OFFSET too, silencing select-without-limit and orderby-without-limit. HasLimit now requires a row count; LIMIT ALL still counts, matching the fallback (#82). That exposed a pre-existing parity gap: the fallback read the ORDER BY of a fully parenthesised statement as a subquery's, so pgparser reported orderby-without-limit where the fallback did not. The fallback now unwraps statement-enclosing parentheses first. Closes #81 Closes #82 --- .codeant/review.json | 2 +- .coderabbit.yaml | 5 ++ .greptile/config.json | 2 +- AGENTS.md | 2 +- CHANGELOG.md | 27 ++++++ analyzer/analyzer_test.go | 6 ++ analyzer/fallback.go | 23 +++++ analyzer/statement.go | 7 +- parsers/mysqlparser/mysqlparser.go | 92 +++++++++++++++---- parsers/mysqlparser/mysqlparser_test.go | 88 ++++++++++++++++++ parsers/pgparser/pgparser.go | 60 ++++++++++--- parsers/pgparser/pgparser_test.go | 114 ++++++++++++++++++++++++ website/docs/parsers.md | 21 ++++- 13 files changed, 411 insertions(+), 38 deletions(-) diff --git a/.codeant/review.json b/.codeant/review.json index 0392eff..852398d 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -56,7 +56,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. 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). 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 284c501..a274867 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -150,6 +150,11 @@ reviews: derive structural facts from the AST but intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics — that carve- out is documented, not a bug. + Structural fields are reset only inside the case for an AST node the + parser models; any other node returns the fallback's Statement untouched + with Exact=false, because blanking fields nothing was derived for drops + findings silently (#81). HasLimit means a row count, not a limit node + (#82). - path: "config/**" instructions: >- diff --git a/.greptile/config.json b/.greptile/config.json index 9731818..20555f8 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -62,7 +62,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. 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). 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 90f493c..b539872 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. +**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). **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 107a2bc..4faf1eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,30 @@ the same version in lockstep. ## [Unreleased] +### Fixed + +- **The dialect parsers no longer drop findings on statements they do not + model** ([#81]). `pgparser` and `mysqlparser` cleared every structural + field before looking at the AST and refilled them only for + `SELECT`/`INSERT`/`UPDATE`/`DELETE`, so anything else the grammar accepted + lost what the fallback had found and was still marked `Exact`: + `CREATE VIEW v AS SELECT * FROM t`, `CREATE TABLE … AS SELECT *` and + `EXPLAIN SELECT *` all lost `select-star`. Those statements now keep the + fallback's `Statement` with `Exact == false`. Both parsers also read the + row source of `INSERT … SELECT *` now, and `mysqlparser` reads a `UNION`'s + `ORDER BY`/`LIMIT` as `pgparser` already did instead of treating it as + `StmtOther`. +- **`pgparser` no longer treats a bare `OFFSET` as a `LIMIT`** ([#82]). The + grammar builds a limit node for `OFFSET n` alone, and `HasLimit` was set + from the node rather than from a row count, so `select-without-limit` and + `orderby-without-limit` never fired on `SELECT a FROM t ORDER BY a OFFSET + 5000`. `LIMIT ALL` still counts as a limit. +- The fallback parser reads the `ORDER BY` of a statement wrapped in + parentheses, `(SELECT … ORDER BY a)`, as the statement's own, as both + grammars do. It read it as a subquery's and missed + `orderby-without-limit`, which made `pgparser` report a finding the + fallback did not. + ### Security - **`parsers/pgparser` no longer pulls a six-year-old gRPC stack.** @@ -27,6 +51,9 @@ the same version in lockstep. `colord` and `serialize-javascript`. Build-time only — nothing here ships to consumers of the Go modules or to readers of the published site. +[#81]: https://github.com/KARTIKrocks/sqlguard/issues/81 +[#82]: https://github.com/KARTIKrocks/sqlguard/issues/82 + ## [0.5.0] - 2026-09-25 ### Fixed diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index e3373ad..a013221 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -195,6 +195,12 @@ func TestCheckOrderByWithoutLimit(t *testing.T) { {"window order by", "SELECT row_number() OVER (ORDER BY id) FROM users", false}, {"ordered aggregate", "SELECT GROUP_CONCAT(x ORDER BY y) FROM t", false}, {"window order by with top-level order by", "SELECT rank() OVER (ORDER BY a) FROM t ORDER BY b", true}, + // Parentheses around the whole statement are not a subquery. + {"parenthesised statement", "(SELECT id FROM users ORDER BY name)", true}, + {"doubly parenthesised statement", "((SELECT id FROM users ORDER BY name));", true}, + {"parenthesised statement with limit", "(SELECT id FROM users ORDER BY name LIMIT 10)", false}, + {"parenthesised union arm", "(SELECT a FROM t ORDER BY a) UNION (SELECT b FROM u)", false}, + {"subquery order by", "SELECT id FROM (SELECT id FROM users ORDER BY name) s", false}, } for _, tt := range tests { diff --git a/analyzer/fallback.go b/analyzer/fallback.go index eaf9af4..e9f11c6 100644 --- a/analyzer/fallback.go +++ b/analyzer/fallback.go @@ -370,7 +370,11 @@ func hasTopLevelJoin(region string) bool { // zero — a result-set sort, not a window-function (OVER (ORDER BY ...)), // ordered-aggregate (GROUP_CONCAT(... ORDER BY ...), WITHIN GROUP (ORDER BY // ...)), or subquery ordering, none of which sort the statement's result set. +// Parentheses around the whole statement are not a subquery, so they are +// unwrapped first: "(SELECT ... ORDER BY a)" sorts its result like the bare +// form, and the dialect grammars read it that way. func hasTopLevelOrderBy(sanitized string) bool { + sanitized = unwrapStatementParens(sanitized) for _, loc := range fbOrderByRe.FindAllStringIndex(sanitized, -1) { if parenDepthBefore(sanitized, loc[0]) == 0 { return true @@ -439,6 +443,25 @@ func parenContent(s string, open int) (string, bool) { return "", false } +// unwrapStatementParens strips parentheses that enclose the entire statement +// (an optional trailing ";" aside), repeatedly, so "((SELECT 1))" yields +// "SELECT 1". A statement that merely starts with a parenthesised operand, like +// "(SELECT a FROM t) UNION (SELECT b FROM u)", is returned unchanged. +func unwrapStatementParens(sanitized string) string { + for { + s := strings.TrimSpace(sanitized) + s = strings.TrimSpace(strings.TrimSuffix(s, ";")) + if !strings.HasPrefix(s, "(") { + return sanitized + } + inner, ok := parenContent(s, 0) + if !ok || len(inner)+2 != len(s) { + return sanitized + } + sanitized = inner + } +} + // fromRegion returns the slice of sanitized SQL between the first top-level // FROM keyword and the next top-level clause keyword (WHERE, GROUP BY, a set // operator, ...), or "" when there is no top-level FROM. "Top-level" means at diff --git a/analyzer/statement.go b/analyzer/statement.go index 2d4bbdd..8b01115 100644 --- a/analyzer/statement.go +++ b/analyzer/statement.go @@ -132,9 +132,12 @@ type Statement struct { // treat zero as unknown and not as "short", to avoid false negatives. LeadingWildcardTermLen int - // Exact is true when the Statement was produced by a real SQL parser - // (structural analysis), false when produced by the regex fallback + // Exact is true when a real SQL parser derived the Statement's structural + // facts from an AST, false when they came from the regex fallback // (best-effort). Rules may use this to suppress lower-confidence findings. + // A dialect parser that accepts a statement it does not model (DDL, + // EXPLAIN, CREATE VIEW ... AS SELECT) returns the fallback's Statement + // unchanged, so Exact is false there too. // // "Exact" covers the structural facts the dialect parsers derive from the // AST: Kind, HasWhere/HasLimit/HasOrderBy/HasFrom, SelectStar, diff --git a/parsers/mysqlparser/mysqlparser.go b/parsers/mysqlparser/mysqlparser.go index 9c9a63b..173caf4 100644 --- a/parsers/mysqlparser/mysqlparser.go +++ b/parsers/mysqlparser/mysqlparser.go @@ -56,51 +56,107 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { return st, nil } - st.Kind = analyzer.StmtOther - st.HasWhere = false - st.HasLimit = false - st.HasOrderBy = false - st.HasFrom = false - st.SelectStar = false - st.SelectDistinct = false - st.OffsetValue = 0 - st.InsertColumnsListed = false - switch n := ast.(type) { case *sqlparser.Select: + resetStructural(st) st.Kind = analyzer.StmtSelect st.HasWhere = n.Where != nil - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.HasOrderBy = len(n.OrderBy) > 0 st.HasFrom = hasRealFrom(n.From) st.SelectDistinct = n.Distinct != "" st.OffsetValue = offsetValue(n.Limit) - for _, e := range n.SelectExprs { - if _, ok := e.(*sqlparser.StarExpr); ok { // '*' or 'table.*' - st.SelectStar = true - } - } + st.SelectStar = hasStar(n.SelectExprs) + case *sqlparser.Union: + // Same shape as pgparser's set operations: the ORDER BY / LIMIT that + // apply to the whole result are read, the arms' FROM/WHERE are not. + resetStructural(st) + st.Kind = analyzer.StmtSelect + st.HasLimit = hasRowLimit(n.Limit) + st.HasOrderBy = len(n.OrderBy) > 0 + st.OffsetValue = offsetValue(n.Limit) case *sqlparser.Delete: + resetStructural(st) st.Kind = analyzer.StmtDelete st.HasWhere = n.Where != nil - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.HasOrderBy = len(n.OrderBy) > 0 st.OffsetValue = offsetValue(n.Limit) case *sqlparser.Update: + resetStructural(st) st.Kind = analyzer.StmtUpdate st.HasWhere = n.Where != nil - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.HasOrderBy = len(n.OrderBy) > 0 st.OffsetValue = offsetValue(n.Limit) case *sqlparser.Insert: + resetStructural(st) st.Kind = analyzer.StmtInsert st.InsertColumnsListed = len(n.Columns) > 0 + // INSERT ... SELECT * copies columns by position, so a star in the + // row source is the select-star case, not an incidental one. + st.SelectStar = rowSourceStar(n.Rows) + default: + // A statement the grammar parsed but this parser does not model + // (CREATE VIEW ... AS SELECT, EXPLAIN, DDL, SHOW, ...). Nothing + // structural was derived from its AST, so the fallback's facts stand + // and the Statement is not Exact. Blanking them instead would silently + // drop findings the default parser reports (#81). + return st, nil } st.Exact = true return st, nil } +// resetStructural clears the fields a handled AST node recomputes, so a +// fallback guess can't survive into a Statement marked Exact. Only called for +// nodes this parser models; the rest keep the fallback's values. +func resetStructural(st *analyzer.Statement) { + st.Kind = analyzer.StmtOther + st.HasWhere = false + st.HasLimit = false + st.HasOrderBy = false + st.HasFrom = false + st.SelectStar = false + st.SelectDistinct = false + st.OffsetValue = 0 + st.InsertColumnsListed = false +} + +// hasRowLimit reports whether a limit clause bounds the row count, mirroring +// pgparser. MySQL has no bare OFFSET (the grammar rejects it and the fallback +// takes over), so today every Limit node carries a Rowcount; the check keeps +// the two parsers answering the same question. +func hasRowLimit(lim *sqlparser.Limit) bool { + return lim != nil && lim.Rowcount != nil +} + +// hasStar reports whether a select list contains '*' or 'table.*'. +func hasStar(exprs sqlparser.SelectExprs) bool { + for _, e := range exprs { + if _, ok := e.(*sqlparser.StarExpr); ok { + return true + } + } + return false +} + +// rowSourceStar reports whether an INSERT's row source is a SELECT whose own +// select list uses a star. VALUES rows and set operations report false. +func rowSourceStar(rows sqlparser.InsertRows) bool { + for { + switch r := rows.(type) { + case *sqlparser.Select: + return hasStar(r.SelectExprs) + case *sqlparser.ParenSelect: + rows = r.Select + default: + return false + } + } +} + // offsetValue extracts a literal OFFSET as an int, or 0 when there is no limit // clause, no offset, or a non-literal (parameterized) offset — matching the // large-offset rule's contract that only statically-known offsets are flagged. diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index ec61731..f29263c 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -1,6 +1,7 @@ package mysqlparser import ( + "reflect" "strings" "testing" @@ -99,6 +100,33 @@ func TestParser_ExactStructuralFacts(t *testing.T) { sql: "SELECT id FROM users WHERE x = 1 LIMIT 5000, 10", want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasWhere: true, HasLimit: true, OffsetValue: 5000, Exact: true}, }, + { + name: "insert select star", + sql: "INSERT INTO t (a) SELECT * FROM u", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, SelectStar: true, Exact: true}, + }, + { + name: "replace select star", + sql: "REPLACE INTO t (a) SELECT * FROM u", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, SelectStar: true, Exact: true}, + }, + { + name: "insert values has no star", + sql: "INSERT INTO t (a) VALUES (1)", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, Exact: true}, + }, + { + // Set operations read like pgparser's: the ORDER BY / LIMIT that + // apply to the whole result, not the arms' own clauses. + name: "union with order by", + sql: "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasOrderBy: true, Exact: true}, + }, + { + name: "union with limit", + sql: "SELECT a FROM t UNION SELECT b FROM u ORDER BY a LIMIT 10", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasOrderBy: true, HasLimit: true, Exact: true}, + }, } for _, tt := range tests { @@ -151,6 +179,59 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { } } +// TestParser_KeepsFallbackFactsForUnmodelledStatements pins the other half of +// the parity contract: a statement the grammar parses but this parser has no +// case for derives nothing from its AST, so it must come back exactly as the +// fallback built it, not Exact. Blanking the structural fields there dropped +// select-star from CREATE VIEW ... AS SELECT * and its kin (#81). +func TestParser_KeepsFallbackFactsForUnmodelledStatements(t *testing.T) { + for _, sql := range []string{ + "CREATE VIEW v AS SELECT * FROM t", + "EXPLAIN SELECT * FROM t", + "ALTER TABLE t ADD COLUMN c INT NOT NULL", + "CREATE TABLE t (id INT)", + "SHOW TABLES", + "SET autocommit = 1", + } { + t.Run(sql, func(t *testing.T) { + want, _ := analyzer.NewFallbackParser().Parse(sql) + got, err := New().Parse(sql) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(got, want) { + t.Errorf("Parse(%q)\n got: %+v\nwant: %+v", sql, *got, *want) + } + }) + } +} + +// TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop pins specific findings +// the grammar once dropped without having derived anything that disproves them. +func TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop(t *testing.T) { + a := analyzer.Default().WithParser(New()) + tests := []struct { + sql string + want []string + }{ + {"CREATE VIEW v AS SELECT * FROM t", []string{"select-star"}}, + {"EXPLAIN SELECT * FROM t", []string{"select-star"}}, + {"INSERT INTO t (a) SELECT * FROM u", []string{"select-star"}}, + {"REPLACE INTO t (a) SELECT * FROM u", []string{"select-star"}}, + {"SELECT a FROM t UNION SELECT b FROM u ORDER BY a", []string{"orderby-without-limit"}}, + } + for _, tt := range tests { + t.Run(tt.sql, func(t *testing.T) { + got := ruleSet(a.Analyze(tt.sql)) + for _, name := range tt.want { + if _, ok := got[name]; !ok { + t.Errorf("missing %q, got %v", name, got) + } + } + }) + } +} + // TestParser_NeverAddsFindingTheFallbackDoesNot pins the direction of the // trade-off website/docs/parsers.md promises: opting into the real grammar // removes findings the heuristics guessed wrong, and never adds one the @@ -213,6 +294,13 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "SELECT a FROM t WHERE name LIKE '%abc%'", "SELECT `a` FROM `t`", "(SELECT a FROM t) UNION (SELECT b FROM u)", + "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", + "SELECT a FROM t UNION SELECT b FROM u ORDER BY a LIMIT 10", + "INSERT INTO t (a) SELECT * FROM u", + "REPLACE INTO t (a) SELECT * FROM u", + "CREATE VIEW v AS SELECT * FROM t", + "EXPLAIN SELECT * FROM t", + "SHOW TABLES", } fallback := analyzer.Default() diff --git a/parsers/pgparser/pgparser.go b/parsers/pgparser/pgparser.go index 64f0992..bf3a95d 100644 --- a/parsers/pgparser/pgparser.go +++ b/parsers/pgparser/pgparser.go @@ -53,47 +53,79 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { return st, nil } - st.Kind = analyzer.StmtOther - st.HasWhere = false - st.HasLimit = false - st.HasOrderBy = false - st.HasFrom = false - st.SelectStar = false - st.SelectDistinct = false - st.OffsetValue = 0 - st.InsertColumnsListed = false - switch n := stmts[0].AST.(type) { case *tree.Select: + resetStructural(st) st.Kind = analyzer.StmtSelect st.HasOrderBy = len(n.OrderBy) > 0 - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.OffsetValue = offsetValue(n.Limit) fillSelectBody(st, n.Select) case *tree.SelectClause: + resetStructural(st) st.Kind = analyzer.StmtSelect fillSelectClause(st, n) case *tree.Delete: + resetStructural(st) st.Kind = analyzer.StmtDelete st.HasWhere = n.Where != nil - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.HasOrderBy = len(n.OrderBy) > 0 st.OffsetValue = offsetValue(n.Limit) case *tree.Update: + resetStructural(st) st.Kind = analyzer.StmtUpdate st.HasWhere = n.Where != nil - st.HasLimit = n.Limit != nil + st.HasLimit = hasRowLimit(n.Limit) st.HasOrderBy = len(n.OrderBy) > 0 st.OffsetValue = offsetValue(n.Limit) case *tree.Insert: + resetStructural(st) st.Kind = analyzer.StmtInsert st.InsertColumnsListed = len(n.Columns) > 0 || defaultValues(n) + if !defaultValues(n) { + // INSERT ... SELECT * copies columns by position, so a star in the + // row source is the select-star case, not an incidental one. + var src analyzer.Statement + fillSelectBody(&src, n.Rows.Select) + st.SelectStar = src.SelectStar + } + default: + // A statement the grammar parsed but this parser does not model + // (CREATE VIEW ... AS SELECT, EXPLAIN, DDL, ...). Nothing structural + // was derived from its AST, so the fallback's facts stand and the + // Statement is not Exact. Blanking them instead would silently drop + // findings the default parser reports (#81). + return st, nil } st.Exact = true return st, nil } +// resetStructural clears the fields a handled AST node recomputes, so a +// fallback guess can't survive into a Statement marked Exact. Only called for +// nodes this parser models; the rest keep the fallback's values. +func resetStructural(st *analyzer.Statement) { + st.Kind = analyzer.StmtOther + st.HasWhere = false + st.HasLimit = false + st.HasOrderBy = false + st.HasFrom = false + st.SelectStar = false + st.SelectDistinct = false + st.OffsetValue = 0 + st.InsertColumnsListed = false +} + +// hasRowLimit reports whether a limit clause bounds the row count. The grammar +// builds a Limit node for a bare OFFSET too, which bounds nothing (#82). +// LIMIT ALL is unbounded as well, but it is an explicit statement that no limit +// is wanted, and the fallback reads it as a LIMIT, so it counts here too. +func hasRowLimit(lim *tree.Limit) bool { + return lim != nil && (lim.Count != nil || lim.LimitAll) +} + // defaultValues reports whether an INSERT is the "DEFAULT VALUES" form, which // the grammar encodes as an absent row source. It names no columns and needs // none — it inserts no data, so there is no positional column-order risk for @@ -111,7 +143,7 @@ func fillSelectBody(st *analyzer.Statement, sel tree.SelectStatement) { case *tree.ParenSelect: if c.Select != nil { st.HasOrderBy = st.HasOrderBy || len(c.Select.OrderBy) > 0 - st.HasLimit = st.HasLimit || c.Select.Limit != nil + st.HasLimit = st.HasLimit || hasRowLimit(c.Select.Limit) if v := offsetValue(c.Select.Limit); v > st.OffsetValue { st.OffsetValue = v } diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index 629d560..650a201 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -1,6 +1,7 @@ package pgparser import ( + "reflect" "strings" "testing" @@ -91,6 +92,39 @@ func TestParser_ExactStructuralFacts(t *testing.T) { sql: "SELECT id FROM users WHERE x = 1 LIMIT 10 OFFSET $1", want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasWhere: true, HasLimit: true, Exact: true}, }, + { + // The grammar builds a Limit node for a bare OFFSET; it bounds + // nothing, so HasLimit must stay false (#82). + name: "offset without limit is unbounded", + sql: "SELECT a FROM t ORDER BY a OFFSET 5000", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasOrderBy: true, OffsetValue: 5000, Exact: true}, + }, + { + name: "parenthesised offset without limit is unbounded", + sql: "(SELECT a FROM t ORDER BY a OFFSET 5)", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasOrderBy: true, OffsetValue: 5, Exact: true}, + }, + { + // An explicit opt-out; the fallback reads it as a LIMIT too. + name: "limit all counts as a limit", + sql: "SELECT a FROM t ORDER BY a LIMIT ALL", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasOrderBy: true, HasLimit: true, Exact: true}, + }, + { + name: "insert select star", + sql: "INSERT INTO t (a) SELECT * FROM u", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, SelectStar: true, Exact: true}, + }, + { + name: "cte-prefixed insert select star", + sql: "WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, SelectStar: true, Exact: true}, + }, + { + name: "insert values has no star", + sql: "INSERT INTO t (a) VALUES (1)", + want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, Exact: true}, + }, } for _, tt := range tests { @@ -144,6 +178,62 @@ func TestParser_IntegratesWithAnalyzer(t *testing.T) { } } +// TestParser_KeepsFallbackFactsForUnmodelledStatements pins the other half of +// the parity contract: a statement the grammar parses but this parser has no +// case for derives nothing from its AST, so it must come back exactly as the +// fallback built it, not Exact. Blanking the structural fields there dropped +// select-star from CREATE VIEW ... AS SELECT * and its kin (#81). +func TestParser_KeepsFallbackFactsForUnmodelledStatements(t *testing.T) { + for _, sql := range []string{ + "CREATE VIEW v AS SELECT * FROM t", + "CREATE TABLE c AS SELECT * FROM t", + "CREATE TABLE c AS SELECT a FROM t ORDER BY a", + "EXPLAIN SELECT * FROM t", + "ALTER TABLE t ADD COLUMN c INT NOT NULL", + "CREATE TABLE t (id INT)", + "SET search_path = public", + } { + t.Run(sql, func(t *testing.T) { + want, _ := analyzer.NewFallbackParser().Parse(sql) + got, err := New().Parse(sql) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(got, want) { + t.Errorf("Parse(%q)\n got: %+v\nwant: %+v", sql, *got, *want) + } + }) + } +} + +// TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop pins specific findings +// the grammar once dropped without having derived anything that disproves them. +func TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop(t *testing.T) { + a := analyzer.Default().WithParser(New()) + tests := []struct { + sql string + want []string + }{ + {"CREATE VIEW v AS SELECT * FROM t", []string{"select-star"}}, // #81 + {"CREATE TABLE c AS SELECT * FROM t", []string{"select-star"}}, // #81 + {"EXPLAIN SELECT * FROM t", []string{"select-star"}}, // #81 + {"INSERT INTO t (a) SELECT * FROM u", []string{"select-star"}}, // #81 + {"WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", []string{"select-star"}}, // #81 + {"SELECT a FROM t OFFSET 100", []string{"select-without-limit"}}, // #82 + {"SELECT a FROM t ORDER BY a OFFSET 5000", []string{"large-offset", "orderby-without-limit", "select-without-limit"}}, // #82 + } + for _, tt := range tests { + t.Run(tt.sql, func(t *testing.T) { + got := ruleSet(a.Analyze(tt.sql)) + for _, name := range tt.want { + if _, ok := got[name]; !ok { + t.Errorf("missing %q, got %v", name, got) + } + } + }) + } +} + // TestParser_NeverAddsFindingTheFallbackDoesNot pins the direction of the // trade-off website/docs/parsers.md promises: opting into the real grammar // removes findings the heuristics guessed wrong, and never adds one the @@ -207,17 +297,32 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "VALUES (1), (2)", "SELECT t.* FROM t", "SELECT * FROM t LIMIT 1 OFFSET 2000", + "SELECT a FROM t OFFSET 100", + "SELECT a FROM t ORDER BY a LIMIT ALL", + "(SELECT a FROM t ORDER BY a)", + "(SELECT a FROM t ORDER BY a OFFSET 5)", + "(SELECT a FROM t ORDER BY a LIMIT 5)", + "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", + "INSERT INTO t (a) SELECT * FROM u", + "WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", + "CREATE VIEW v AS SELECT * FROM t", + "CREATE TABLE c AS SELECT a FROM t ORDER BY a", + "EXPLAIN SELECT * FROM t", } fallback := analyzer.Default() exact := analyzer.Default().WithParser(New()) coreKnows := fallbackKnowsInsertLikeKeywords() + coreUnwraps := fallbackUnwrapsStatementParens() 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 !coreUnwraps && strings.HasPrefix(sql, "(") { + t.Skip("linked core predates the fallback reading a parenthesised statement's ORDER BY") + } base := ruleSet(fallback.Analyze(sql)) for name := range ruleSet(exact.Analyze(sql)) { if _, ok := base[name]; !ok { @@ -263,3 +368,12 @@ func fallbackKnowsInsertLikeKeywords() bool { st, _ := analyzer.NewFallbackParser().Parse("REPLACE INTO t VALUES (1)") return st.Kind == analyzer.StmtInsert } + +// fallbackUnwrapsStatementParens reports whether the linked core reads the +// ORDER BY of a statement wrapped in parentheses as the statement's own, as the +// grammar does. Skipped against an older core for the same reason as +// fallbackKnowsInsertLikeKeywords. +func fallbackUnwrapsStatementParens() bool { + st, _ := analyzer.NewFallbackParser().Parse("(SELECT a FROM t ORDER BY a)") + return st.HasOrderBy +} diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 0d5906d..bce791b 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -72,7 +72,12 @@ Neither parser uses cgo. | `LeadingWildcardLike`, `LeadingWildcardTermLen`, `NonSargablePredicate`, `AddNotNullNoDefault` | lexical | **still lexical** — they read literal values or DDL text the AST does not carry | The first group is the false-positive-prone set; those become exact -(`Statement.Exact == true`). The rest stay best-effort heuristics +(`Statement.Exact == true`) for the statements each parser models: +`SELECT` (including set operations), `INSERT`, `UPDATE` and `DELETE`. +_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 +facts and `Exact == false`; before, every structural field on it was +cleared and the statement was still marked exact. 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. @@ -95,6 +100,20 @@ grammar recognised as inserting rows and the fallback did not: - `mysqlparser` reported it on `REPLACE INTO t VALUES (…)` and on the forms that omit MySQL's optional `INTO`, such as `INSERT t VALUES (…)`. +_Changed in 0.6._ **A dialect parser also should not drop a finding it +has no reason to drop.** Removing a finding is only an improvement when +the grammar derived something that disproves it. In 0.5 and earlier +both parsers dropped findings they had derived nothing about: + +- `select-star` on `CREATE VIEW v AS SELECT * FROM t`, + `CREATE TABLE c AS SELECT * FROM t` and `EXPLAIN SELECT * FROM t`, + whose structural fields were cleared rather than kept from the fallback. +- `select-star` on `INSERT INTO t (a) SELECT * FROM u`, whose row source + was never inspected. +- Under `pgparser`, `select-without-limit` and `orderby-without-limit` on a + bare `OFFSET` with no `LIMIT`, which was read as a limit. `LIMIT ALL` + still counts as one: it states that no limit is wanted. + ## Degradation on parse failure A real grammar will reject SQL it does not know: dynamic fragments, a From b015d3833c882e7cb11a48dd7279ea8ed7888974 Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 05:22:04 +0530 Subject: [PATCH 2/4] fix(parsers): read every operand of a set operation A set operation was marked Exact with SelectStar and HasFrom false and its operands never read, so SELECT * FROM t UNION SELECT * FROM u lost select-star and select-without-limit under both parsers, and an INSERT ... SELECT * ... UNION row source lost select-star. pgparser had done this deliberately; the new mysqlparser Union case copied it. Each operand is now folded in: a fact is true when any operand has it, matching how the fallback reads the same text, and only the ORDER BY that applies to the whole result counts. --- CHANGELOG.md | 8 ++-- parsers/mysqlparser/mysqlparser.go | 64 ++++++++++++++----------- parsers/mysqlparser/mysqlparser_test.go | 19 ++++++-- parsers/pgparser/pgparser.go | 30 +++++++++++- parsers/pgparser/pgparser_test.go | 14 ++++-- website/docs/parsers.md | 2 + 6 files changed, 96 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4faf1eb..ce4bfa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,9 +19,11 @@ the same version in lockstep. `CREATE VIEW v AS SELECT * FROM t`, `CREATE TABLE … AS SELECT *` and `EXPLAIN SELECT *` all lost `select-star`. Those statements now keep the fallback's `Statement` with `Exact == false`. Both parsers also read the - row source of `INSERT … SELECT *` now, and `mysqlparser` reads a `UNION`'s - `ORDER BY`/`LIMIT` as `pgparser` already did instead of treating it as - `StmtOther`. + row source of `INSERT … SELECT *` now, and read every operand of a + `UNION`/`INTERSECT`/`EXCEPT` instead of none of them, so + `SELECT * FROM t UNION SELECT * FROM u` reports `select-star` and + `select-without-limit` as the fallback does. `mysqlparser` had treated a + `UNION` as `StmtOther`. - **`pgparser` no longer treats a bare `OFFSET` as a `LIMIT`** ([#82]). The grammar builds a limit node for `OFFSET n` alone, and `HasLimit` was set from the node rather than from a row count, so `select-without-limit` and diff --git a/parsers/mysqlparser/mysqlparser.go b/parsers/mysqlparser/mysqlparser.go index 173caf4..7288881 100644 --- a/parsers/mysqlparser/mysqlparser.go +++ b/parsers/mysqlparser/mysqlparser.go @@ -60,21 +60,11 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { case *sqlparser.Select: resetStructural(st) st.Kind = analyzer.StmtSelect - st.HasWhere = n.Where != nil - st.HasLimit = hasRowLimit(n.Limit) - st.HasOrderBy = len(n.OrderBy) > 0 - st.HasFrom = hasRealFrom(n.From) - st.SelectDistinct = n.Distinct != "" - st.OffsetValue = offsetValue(n.Limit) - st.SelectStar = hasStar(n.SelectExprs) + fillSelect(st, n, true) case *sqlparser.Union: - // Same shape as pgparser's set operations: the ORDER BY / LIMIT that - // apply to the whole result are read, the arms' FROM/WHERE are not. resetStructural(st) st.Kind = analyzer.StmtSelect - st.HasLimit = hasRowLimit(n.Limit) - st.HasOrderBy = len(n.OrderBy) > 0 - st.OffsetValue = offsetValue(n.Limit) + fillSelect(st, n, true) case *sqlparser.Delete: resetStructural(st) st.Kind = analyzer.StmtDelete @@ -95,7 +85,11 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { st.InsertColumnsListed = len(n.Columns) > 0 // INSERT ... SELECT * copies columns by position, so a star in the // row source is the select-star case, not an incidental one. - st.SelectStar = rowSourceStar(n.Rows) + if sel, ok := n.Rows.(sqlparser.SelectStatement); ok { + var src analyzer.Statement + fillSelect(&src, sel, true) + st.SelectStar = src.SelectStar + } default: // A statement the grammar parsed but this parser does not model // (CREATE VIEW ... AS SELECT, EXPLAIN, DDL, SHOW, ...). Nothing @@ -132,6 +126,35 @@ func hasRowLimit(lim *sqlparser.Limit) bool { return lim != nil && lim.Rowcount != nil } +// fillSelect folds a SELECT, a parenthesised SELECT or a set operation into +// st. top is false for the operands of a set operation, whose ORDER BY orders +// that operand rather than the result and so is not merged. Every other fact +// is true when any operand has it, which is how the fallback reads the same +// text. For WHERE and LIMIT that is generous — one filtered operand does not +// bound the other — but reading them per operand would report +// select-without-limit where the fallback does not, and a parser may only +// remove findings. +func fillSelect(st *analyzer.Statement, sel sqlparser.SelectStatement, top bool) { + switch s := sel.(type) { + case *sqlparser.Select: + st.HasWhere = st.HasWhere || s.Where != nil + st.HasLimit = st.HasLimit || hasRowLimit(s.Limit) + st.HasOrderBy = st.HasOrderBy || (top && len(s.OrderBy) > 0) + st.HasFrom = st.HasFrom || hasRealFrom(s.From) + st.SelectDistinct = st.SelectDistinct || s.Distinct != "" + st.SelectStar = st.SelectStar || hasStar(s.SelectExprs) + st.OffsetValue = max(st.OffsetValue, offsetValue(s.Limit)) + case *sqlparser.ParenSelect: + fillSelect(st, s.Select, top) + case *sqlparser.Union: + st.HasLimit = st.HasLimit || hasRowLimit(s.Limit) + st.HasOrderBy = st.HasOrderBy || (top && len(s.OrderBy) > 0) + st.OffsetValue = max(st.OffsetValue, offsetValue(s.Limit)) + fillSelect(st, s.Left, false) + fillSelect(st, s.Right, false) + } +} + // hasStar reports whether a select list contains '*' or 'table.*'. func hasStar(exprs sqlparser.SelectExprs) bool { for _, e := range exprs { @@ -142,21 +165,6 @@ func hasStar(exprs sqlparser.SelectExprs) bool { return false } -// rowSourceStar reports whether an INSERT's row source is a SELECT whose own -// select list uses a star. VALUES rows and set operations report false. -func rowSourceStar(rows sqlparser.InsertRows) bool { - for { - switch r := rows.(type) { - case *sqlparser.Select: - return hasStar(r.SelectExprs) - case *sqlparser.ParenSelect: - rows = r.Select - default: - return false - } - } -} - // offsetValue extracts a literal OFFSET as an int, or 0 when there is no limit // clause, no offset, or a non-literal (parameterized) offset — matching the // large-offset rule's contract that only statically-known offsets are flagged. diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index f29263c..a315ef3 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -116,16 +116,21 @@ func TestParser_ExactStructuralFacts(t *testing.T) { want: analyzer.Statement{Kind: analyzer.StmtInsert, InsertColumnsListed: true, Exact: true}, }, { - // Set operations read like pgparser's: the ORDER BY / LIMIT that - // apply to the whole result, not the arms' own clauses. + // A set operation merges its operands' facts; only the ORDER BY + // that applies to the whole result counts. name: "union with order by", sql: "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", - want: analyzer.Statement{Kind: analyzer.StmtSelect, HasOrderBy: true, Exact: true}, + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasOrderBy: true, Exact: true}, }, { name: "union with limit", sql: "SELECT a FROM t UNION SELECT b FROM u ORDER BY a LIMIT 10", - want: analyzer.Statement{Kind: analyzer.StmtSelect, HasOrderBy: true, HasLimit: true, Exact: true}, + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, HasOrderBy: true, HasLimit: true, Exact: true}, + }, + { + name: "union arm star", + sql: "SELECT a FROM t UNION SELECT * FROM u", + want: analyzer.Statement{Kind: analyzer.StmtSelect, HasFrom: true, SelectStar: true, Exact: true}, }, } @@ -217,6 +222,9 @@ func TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop(t *testing.T) { {"CREATE VIEW v AS SELECT * FROM t", []string{"select-star"}}, {"EXPLAIN SELECT * FROM t", []string{"select-star"}}, {"INSERT INTO t (a) SELECT * FROM u", []string{"select-star"}}, + {"SELECT * FROM t UNION SELECT * FROM u", []string{"select-star", "select-without-limit"}}, + {"SELECT a FROM t UNION SELECT b FROM u ORDER BY a", []string{"orderby-without-limit", "select-without-limit"}}, + {"INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", []string{"select-star"}}, {"REPLACE INTO t (a) SELECT * FROM u", []string{"select-star"}}, {"SELECT a FROM t UNION SELECT b FROM u ORDER BY a", []string{"orderby-without-limit"}}, } @@ -296,6 +304,9 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "(SELECT a FROM t) UNION (SELECT b FROM u)", "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", "SELECT a FROM t UNION SELECT b FROM u ORDER BY a LIMIT 10", + "SELECT * FROM t UNION SELECT * FROM u", + "SELECT a FROM t WHERE x = 1 UNION SELECT b FROM u", + "INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", "INSERT INTO t (a) SELECT * FROM u", "REPLACE INTO t (a) SELECT * FROM u", "CREATE VIEW v AS SELECT * FROM t", diff --git a/parsers/pgparser/pgparser.go b/parsers/pgparser/pgparser.go index bf3a95d..3c8cc0d 100644 --- a/parsers/pgparser/pgparser.go +++ b/parsers/pgparser/pgparser.go @@ -149,9 +149,35 @@ func fillSelectBody(st *analyzer.Statement, sel tree.SelectStatement) { } fillSelectBody(st, c.Select.Select) } + case *tree.UnionClause: + mergeArm(st, c.Left) + mergeArm(st, c.Right) } - // UnionClause / ValuesClause: leave structural defaults; the rules that - // matter for those forms don't trigger on set operations. + // ValuesClause: no FROM, WHERE or select list to read. +} + +// mergeArm folds one operand of a set operation (UNION / INTERSECT / EXCEPT) +// into st. Each fact is true when any arm has it, which is how the fallback +// reads the same text: a star or a FROM in either arm is one in the statement, +// and a WHERE or LIMIT in either arm counts too. The last two are generous — +// one filtered arm does not bound the other — but reading them per-arm would +// report select-without-limit where the fallback does not, and a parser may +// only remove findings. An arm's ORDER BY is not merged: it orders that arm, +// not the result. +func mergeArm(st *analyzer.Statement, arm *tree.Select) { + if arm == nil { + return + } + var a analyzer.Statement + a.HasLimit = hasRowLimit(arm.Limit) + a.OffsetValue = offsetValue(arm.Limit) + fillSelectBody(&a, arm.Select) + st.HasWhere = st.HasWhere || a.HasWhere + st.HasFrom = st.HasFrom || a.HasFrom + st.HasLimit = st.HasLimit || a.HasLimit + st.SelectStar = st.SelectStar || a.SelectStar + st.SelectDistinct = st.SelectDistinct || a.SelectDistinct + st.OffsetValue = max(st.OffsetValue, a.OffsetValue) } // offsetValue extracts a literal OFFSET as an int, or 0 when there is no limit diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index 650a201..7bab0e8 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -214,10 +214,13 @@ func TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop(t *testing.T) { sql string want []string }{ - {"CREATE VIEW v AS SELECT * FROM t", []string{"select-star"}}, // #81 - {"CREATE TABLE c AS SELECT * FROM t", []string{"select-star"}}, // #81 - {"EXPLAIN SELECT * FROM t", []string{"select-star"}}, // #81 - {"INSERT INTO t (a) SELECT * FROM u", []string{"select-star"}}, // #81 + {"CREATE VIEW v AS SELECT * FROM t", []string{"select-star"}}, // #81 + {"CREATE TABLE c AS SELECT * FROM t", []string{"select-star"}}, // #81 + {"EXPLAIN SELECT * FROM t", []string{"select-star"}}, // #81 + {"INSERT INTO t (a) SELECT * FROM u", []string{"select-star"}}, // #81 + {"SELECT * FROM t UNION SELECT * FROM u", []string{"select-star", "select-without-limit"}}, + {"SELECT a FROM t UNION SELECT b FROM u ORDER BY a", []string{"orderby-without-limit", "select-without-limit"}}, + {"INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", []string{"select-star"}}, {"WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", []string{"select-star"}}, // #81 {"SELECT a FROM t OFFSET 100", []string{"select-without-limit"}}, // #82 {"SELECT a FROM t ORDER BY a OFFSET 5000", []string{"large-offset", "orderby-without-limit", "select-without-limit"}}, // #82 @@ -303,6 +306,9 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "(SELECT a FROM t ORDER BY a OFFSET 5)", "(SELECT a FROM t ORDER BY a LIMIT 5)", "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", + "SELECT * FROM t UNION SELECT * FROM u", + "SELECT a FROM t WHERE x = 1 UNION SELECT b FROM u", + "INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", "INSERT INTO t (a) SELECT * FROM u", "WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", "CREATE VIEW v AS SELECT * FROM t", diff --git a/website/docs/parsers.md b/website/docs/parsers.md index bce791b..9ceddae 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -110,6 +110,8 @@ both parsers dropped findings they had derived nothing about: whose structural fields were cleared rather than kept from the fallback. - `select-star` on `INSERT INTO t (a) SELECT * FROM u`, whose row source was never inspected. +- `select-star` and `select-without-limit` on + `SELECT * FROM t UNION SELECT * FROM u`, whose operands were never read. - Under `pgparser`, `select-without-limit` and `orderby-without-limit` on a bare `OFFSET` with no `LIMIT`, which was read as a limit. `LIMIT ALL` still counts as one: it states that no limit is wanted. From 16c4b2f522f7de8cb430f7209cf5f451f2644ec5 Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 05:31:28 +0530 Subject: [PATCH 3/4] fix(parsers): take a set operation's WHERE/LIMIT from the fallback Merging operands made HasFrom true for a set operation, but HasWhere and HasLimit were read only at each operand's top level while the fallback counts them anywhere. SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u LIMIT 3) s then reported select-without-limit that the fallback does not. A set operation now takes both from the fallback; FROM, star, DISTINCT and OFFSET are still merged from the operands, and the ORDER BY and LIMIT on the whole result are still read from the AST. --- parsers/mysqlparser/mysqlparser.go | 38 +++++++++++++------- parsers/mysqlparser/mysqlparser_test.go | 3 ++ parsers/pgparser/pgparser.go | 46 +++++++++++++++++++------ parsers/pgparser/pgparser_test.go | 3 ++ 4 files changed, 68 insertions(+), 22 deletions(-) diff --git a/parsers/mysqlparser/mysqlparser.go b/parsers/mysqlparser/mysqlparser.go index 7288881..fdc6689 100644 --- a/parsers/mysqlparser/mysqlparser.go +++ b/parsers/mysqlparser/mysqlparser.go @@ -62,9 +62,11 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { st.Kind = analyzer.StmtSelect fillSelect(st, n, true) case *sqlparser.Union: + fb := *st resetStructural(st) st.Kind = analyzer.StmtSelect fillSelect(st, n, true) + keepFallbackBounds(st, &fb) case *sqlparser.Delete: resetStructural(st) st.Kind = analyzer.StmtDelete @@ -127,19 +129,18 @@ func hasRowLimit(lim *sqlparser.Limit) bool { } // fillSelect folds a SELECT, a parenthesised SELECT or a set operation into -// st. top is false for the operands of a set operation, whose ORDER BY orders -// that operand rather than the result and so is not merged. Every other fact -// is true when any operand has it, which is how the fallback reads the same -// text. For WHERE and LIMIT that is generous — one filtered operand does not -// bound the other — but reading them per operand would report -// select-without-limit where the fallback does not, and a parser may only -// remove findings. +// 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. func fillSelect(st *analyzer.Statement, sel sqlparser.SelectStatement, top bool) { switch s := sel.(type) { case *sqlparser.Select: - st.HasWhere = st.HasWhere || s.Where != nil - st.HasLimit = st.HasLimit || hasRowLimit(s.Limit) - st.HasOrderBy = st.HasOrderBy || (top && len(s.OrderBy) > 0) + if top { + st.HasWhere = s.Where != nil + st.HasLimit = hasRowLimit(s.Limit) + st.HasOrderBy = len(s.OrderBy) > 0 + } st.HasFrom = st.HasFrom || hasRealFrom(s.From) st.SelectDistinct = st.SelectDistinct || s.Distinct != "" st.SelectStar = st.SelectStar || hasStar(s.SelectExprs) @@ -147,14 +148,27 @@ func fillSelect(st *analyzer.Statement, sel sqlparser.SelectStatement, top bool) case *sqlparser.ParenSelect: fillSelect(st, s.Select, top) case *sqlparser.Union: - st.HasLimit = st.HasLimit || hasRowLimit(s.Limit) - st.HasOrderBy = st.HasOrderBy || (top && len(s.OrderBy) > 0) + if top { + st.HasLimit = hasRowLimit(s.Limit) + st.HasOrderBy = len(s.OrderBy) > 0 + } st.OffsetValue = max(st.OffsetValue, offsetValue(s.Limit)) fillSelect(st, s.Left, false) fillSelect(st, s.Right, 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. +func keepFallbackBounds(st, fb *analyzer.Statement) { + st.HasWhere = fb.HasWhere + st.HasLimit = st.HasLimit || fb.HasLimit +} + // hasStar reports whether a select list contains '*' or 'table.*'. func hasStar(exprs sqlparser.SelectExprs) bool { for _, e := range exprs { diff --git a/parsers/mysqlparser/mysqlparser_test.go b/parsers/mysqlparser/mysqlparser_test.go index a315ef3..0f2d74c 100644 --- a/parsers/mysqlparser/mysqlparser_test.go +++ b/parsers/mysqlparser/mysqlparser_test.go @@ -306,6 +306,9 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "SELECT a FROM t UNION SELECT b FROM u ORDER BY a LIMIT 10", "SELECT * FROM t UNION SELECT * FROM u", "SELECT a FROM t WHERE x = 1 UNION SELECT b FROM u", + "SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u LIMIT 3) s", + "SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u WHERE x = 1) s", + "SELECT a FROM t UNION SELECT b FROM u WHERE b IN (SELECT c FROM v LIMIT 1) ORDER BY a", "INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", "INSERT INTO t (a) SELECT * FROM u", "REPLACE INTO t (a) SELECT * FROM u", diff --git a/parsers/pgparser/pgparser.go b/parsers/pgparser/pgparser.go index 3c8cc0d..86a2f04 100644 --- a/parsers/pgparser/pgparser.go +++ b/parsers/pgparser/pgparser.go @@ -55,12 +55,16 @@ func (p *Parser) Parse(sql string) (*analyzer.Statement, error) { switch n := stmts[0].AST.(type) { case *tree.Select: + fb := *st resetStructural(st) st.Kind = analyzer.StmtSelect st.HasOrderBy = len(n.OrderBy) > 0 st.HasLimit = hasRowLimit(n.Limit) st.OffsetValue = offsetValue(n.Limit) fillSelectBody(st, n.Select) + if isSetOperation(n.Select) { + keepFallbackBounds(st, &fb) + } case *tree.SelectClause: resetStructural(st) st.Kind = analyzer.StmtSelect @@ -157,29 +161,51 @@ func fillSelectBody(st *analyzer.Statement, sel tree.SelectStatement) { } // mergeArm folds one operand of a set operation (UNION / INTERSECT / EXCEPT) -// into st. Each fact is true when any arm has it, which is how the fallback -// reads the same text: a star or a FROM in either arm is one in the statement, -// and a WHERE or LIMIT in either arm counts too. The last two are generous — -// one filtered arm does not bound the other — but reading them per-arm would -// report select-without-limit where the fallback does not, and a parser may -// only remove findings. An arm's ORDER BY is not merged: it orders that arm, -// not the result. +// 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 +// 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 { return } var a analyzer.Statement - a.HasLimit = hasRowLimit(arm.Limit) a.OffsetValue = offsetValue(arm.Limit) fillSelectBody(&a, arm.Select) - st.HasWhere = st.HasWhere || a.HasWhere st.HasFrom = st.HasFrom || a.HasFrom - st.HasLimit = st.HasLimit || a.HasLimit st.SelectStar = st.SelectStar || a.SelectStar st.SelectDistinct = st.SelectDistinct || a.SelectDistinct 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. +func keepFallbackBounds(st, fb *analyzer.Statement) { + st.HasWhere = fb.HasWhere + st.HasLimit = st.HasLimit || fb.HasLimit +} + // offsetValue extracts a literal OFFSET as an int, or 0 when there is no limit // clause, no offset, or a non-literal (parameterized) offset — matching the // large-offset rule's contract that only statically-known offsets are flagged. diff --git a/parsers/pgparser/pgparser_test.go b/parsers/pgparser/pgparser_test.go index 7bab0e8..bc8f590 100644 --- a/parsers/pgparser/pgparser_test.go +++ b/parsers/pgparser/pgparser_test.go @@ -308,6 +308,9 @@ func TestParser_NeverAddsFindingTheFallbackDoesNot(t *testing.T) { "SELECT a FROM t UNION SELECT b FROM u ORDER BY a", "SELECT * FROM t UNION SELECT * FROM u", "SELECT a FROM t WHERE x = 1 UNION SELECT b FROM u", + "SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u LIMIT 3) s", + "SELECT a FROM t UNION SELECT b FROM (SELECT b FROM u WHERE x = 1) s", + "SELECT a FROM t UNION SELECT b FROM u WHERE b IN (SELECT c FROM v LIMIT 1) ORDER BY a", "INSERT INTO t (a) SELECT * FROM u UNION SELECT * FROM v", "INSERT INTO t (a) SELECT * FROM u", "WITH c AS (SELECT 1 AS n) INSERT INTO t (a) SELECT * FROM c", From 7c7772a95f1e84674558593157068e77136edc77 Mon Sep 17 00:00:00 2001 From: kartik Date: Sat, 26 Sep 2026 05:53:04 +0530 Subject: [PATCH 4/4] docs(parsers): document that set operations take WHERE/LIMIT from the fallback Statement.Exact promised HasWhere and HasLimit were AST-derived whenever it was true, but for a set operation both come from the fallback so the grammar never reports select-without-limit the default parser does not. List that alongside the other fields that stay lexical under a parser. --- analyzer/statement.go | 5 ++++- website/docs/parsers.md | 15 +++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/analyzer/statement.go b/analyzer/statement.go index 8b01115..325da16 100644 --- a/analyzer/statement.go +++ b/analyzer/statement.go @@ -146,6 +146,9 @@ 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. + // 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. Exact bool } diff --git a/website/docs/parsers.md b/website/docs/parsers.md index 9ceddae..4aa66f2 100644 --- a/website/docs/parsers.md +++ b/website/docs/parsers.md @@ -74,12 +74,19 @@ Neither parser uses cgo. The first group is the false-positive-prone set; those become exact (`Statement.Exact == true`) for the statements each parser models: `SELECT` (including set operations), `INSERT`, `UPDATE` and `DELETE`. +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._ A statement the grammar accepts but the parser does not model — DDL, `EXPLAIN`, `CREATE VIEW … AS SELECT` — keeps the fallback's -facts and `Exact == false`; before, every structural field on it was -cleared and the statement was still marked exact. 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. +facts and `Exact == false`. Before, every structural field on it was +cleared and the statement was still marked exact. _Added in 0.5._ **A dialect parser is meant to remove findings, never add them.** Opting in can drop a finding the heuristics guessed wrong; it