fix(parsers): stop dialect parsers dropping findings the fallback reports - #90
Conversation
…sLimit 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
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.
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.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: KARTIKrocks/sqlguard/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: KARTIKrocks/sqlguard/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe fallback analyzer now recognizes ORDER BY on fully parenthesized statements. The MySQL and PostgreSQL parsers preserve fallback facts for unmodeled statements and update structural facts for modeled queries, including INSERT SELECT and set operations. PostgreSQL limit detection excludes bare OFFSET. ChangesParser fact preservation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable risk introduced by this change remains; the PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Parentheses open, then close, Comment |
codeant-ai
Bot
added
the
size:XL
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // 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) |
There was a problem hiding this comment.
Suggestion: Only return true when the LIMIT expression represents a finite row count; LIMIT ALL is unbounded and must leave HasLimit false so limit-related rules still report it.
Severity Level: Major Custom_rule
Rule source 📖
.codeant/review.json line 58 (rule "parser-never-breaks-query-path")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** parsers/pgparser/pgparser.go
**Line:** 130:130
**Comment:**
*Custom Rule: Only return true when the LIMIT expression represents a finite row count; LIMIT ALL is unbounded and must leave HasLimit false so limit-related rules still report it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Declining this one. The FallbackParser reads LIMIT ALL as a LIMIT (fbLimitRe matches it), so leaving HasLimit false here would make pgparser report select-without-limit / orderby-without-limit on SELECT a FROM t ORDER BY a LIMIT ALL where the default parser reports nothing, which breaks the "a dialect parser may only remove findings" half of the same rule this suggestion cites. It is also an explicit statement that no limit is wanted. The behaviour is deliberate and pinned: the limit all counts as a limit row in TestParser_ExactStructuralFacts, the SELECT a FROM t ORDER BY a LIMIT ALL parity corpus row, and the hasRowLimit doc comment.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In the PostgreSQL parser, treat explicit LIMIT ALL as a limit for structural facts to preserve parity with the fallback parser and avoid reporting limit-related findings.
Applied to:
parsers/pgparser/pgparser.go
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| st.HasWhere = n.Where != nil | ||
| st.HasLimit = n.Limit != nil | ||
| st.HasLimit = hasRowLimit(n.Limit) |
There was a problem hiding this comment.
Suggestion: Preserve the fallback WHERE result when the DELETE AST lacks a top-level WHERE, rather than overwriting a fallback-detected clause from a nested predicate.
Severity Level: Major Custom_rule
Rule source 📖
.codeant/review.json line 58 (rule "parser-never-breaks-query-path")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** parsers/mysqlparser/mysqlparser.go
**Line:** 73:74
**Comment:**
*Custom Rule: Preserve the fallback WHERE result when the DELETE AST lacks a top-level WHERE, rather than overwriting a fallback-detected clause from a nested predicate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Valid, but it predates this PR: the DELETE case here is unchanged apart from resetStructural and hasRowLimit, and on main (e4f9d58) mysqlparser already reports delete-without-where on DELETE t FROM t JOIN (SELECT id FROM u WHERE x = 1) s ON s.id = t.id while the fallback does not. It is one instance of a wider gap (the parsers read WHERE/LIMIT at the top level, the fallback anywhere) that needs a decision about which side is right per case, so it is tracked separately in #91 rather than patched here.
| st.HasLimit = hasRowLimit(s.Limit) | ||
| st.HasOrderBy = len(s.OrderBy) > 0 |
There was a problem hiding this comment.
Suggestion: Merge the AST result with the fallback value instead of replacing it, so LIMIT clauses inside nested queries are not lost when the modeled statement has no top-level LIMIT.
Severity Level: Major Custom_rule
Rule source 📖
.codeant/review.json line 58 (rule "parser-never-breaks-query-path")
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** parsers/mysqlparser/mysqlparser.go
**Line:** 141:142
**Comment:**
*Custom Rule: Merge the AST result with the fallback value instead of replacing it, so LIMIT clauses inside nested queries are not lost when the modeled statement has no top-level LIMIT.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Valid, but it predates this PR: this is a refactor of the old SELECT case, and the top-level read of LIMIT is unchanged, so on main (e4f9d58) mysqlparser (and pgparser) already report select-without-limit on SELECT a FROM (SELECT a FROM t LIMIT 3) s while the fallback does not. Merging the fallback value unconditionally would make the parser's HasLimit identical to the fallback's everywhere, and a subquery LIMIT inside IN (...) does not bound the outer result, so the right fix differs per case. Tracked in #91 together with the DELETE/WHERE instance. For set operations this PR already takes HasWhere/HasLimit from the fallback (keepFallbackBounds).
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not unconditionally merge AST-derived bounds with fallback values; preserve statement-level semantics for nested subqueries and set operations, and handle fallback bounds per statement context.
Applied to:
parsers/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| case *sqlparser.Union: | ||
| fb := *st | ||
| resetStructural(st) | ||
| st.Kind = analyzer.StmtSelect | ||
| fillSelect(st, n, true) | ||
| keepFallbackBounds(st, &fb) |
There was a problem hiding this comment.
Suggestion: Exact is set true even though UNION HasWhere and HasLimit come from fallback text, which can include clauses inside nested subqueries.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** parsers/mysqlparser/mysqlparser.go
**Line:** 64:69
**Comment:**
*Api Mismatch: `Exact` is set true even though UNION `HasWhere` and `HasLimit` come from fallback text, which can include clauses inside nested subqueries.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch, fixed in 7c7772a by documenting it rather than dropping Exact. Exact is a single flag and the set operation's Kind, HasFrom, SelectStar, SelectDistinct, OffsetValue and result-level HasOrderBy are still AST-derived, so it stays true. Statement.Exact's doc comment and website/docs/parsers.md now list set-operation HasWhere/HasLimit alongside the other fields that stay lexical under a parser (MaxInListLen, ImplicitCommaJoin, ...), with the reason: the fallback counts them anywhere, and reading them per operand would report findings the fallback does not. No rule reads Exact, so findings are unaffected either way.
… 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.
1 participant
User description
Closes #81. Closes #82.
What was wrong
Opting into
pgparser/mysqlparsersilently dropped findings the default parser reports — the inverse of the trade-offwebsite/docs/parsers.mdpromises. The existing parity test (TestParser_NeverAddsFindingTheFallbackDoesNot) only catches findings a parser adds, so none of this was visible.Exact:CREATE VIEW v AS SELECT * FROM t,CREATE TABLE … AS SELECT *,EXPLAIN SELECT *all lostselect-star.HasLimitfrom the presence of a limit node, which the grammar also builds for a bareOFFSET, soselect-without-limit/orderby-without-limitnever fired onSELECT a FROM t ORDER BY a OFFSET 5000.What changed
StatementwithExact = false. Resets now happen only inside a handledcase. This settles the design question left open on [Bug]: dialect parsers blank every structural field, then mark non-DML statements Exact #81; no rule readsExact, so the flag change alters no findings.HasLimitrequires a row count (pgparser).LIMIT ALLstill counts as a limit — the fallback reads it that way, and treating it as unbounded would make the parser add findings.INSERT … SELECT *row sources are read by both parsers (plain and CTE-prefixed).UNION/INTERSECT/EXCEPT): FROM, star, DISTINCT and OFFSET are merged across operands; only the result-level ORDER BY counts. WHERE/LIMIT presence for a set operation is taken from the fallback, which counts them anywhere — reading them per operand madeSELECT a FROM t UNION SELECT b FROM (SELECT b FROM u LIMIT 3) sreport a finding the fallback does not. mysqlparser previously treated aUNIONasStmtOther; pgparser left set operations blank deliberately.(SELECT … ORDER BY a)was read as a subquery, a pre-existing parity break where pgparser already reportedorderby-without-limitand the fallback did not. Fixed in the fallback, per the "when a parser learns it, the fallback learns it too" rule.Tests
TestParser_KeepsFallbackFactsForUnmodelledStatements(both parsers) — unmodelled statements come backreflect.DeepEqualto the fallback's.TestParser_KeepsFindingsTheGrammarHasNoReasonToDrop(both parsers) — pins every finding from [Bug]: dialect parsers blank every structural field, then mark non-DML statements Exact #81/[Bug]: exact parsers drop select-without-limit and orderby-without-limit on OFFSET without LIMIT #82 and the set-operation cases.GOWORK=offstays green.make fmt-check vet lint test-race lint-docsgreen across all nine modules; both parser modules also green underGOWORK=off.Docs
AGENTS.md and all three reviewer configs (
.greptile/config.json,.codeant/review.json,.coderabbit.yaml) now carry the converse invariant: a parser must not drop a finding it has no reason to drop. AlsoStatement.Exact's doc comment,website/docs/parsers.md(_Changed in 0.6._— adjust if this ships as a 0.5.x patch) and the CHANGELOG.Known, out of scope
Already on
main, not introduced here: for a plain SELECT the parsers readLIMIT/WHEREonly at the top level while the fallback counts them anywhere, so e.g.SELECT a FROM (SELECT a FROM t LIMIT 3) sgetsselect-without-limitunder a parser but not the fallback. Resolving it means deciding what a subquery LIMIT bounds; it deserves its own issue.CodeAnt-AI Description
Preserve SQL findings across dialect parsers and correctly handle limits and set operations
What Changed
CREATE VIEW,EXPLAIN, and DDL retain the fallback parser’s findings instead of losing structural facts.INSERT ... SELECT *and every operand ofUNION,INTERSECT, andEXCEPTnow contribute relevant findings.OFFSETis no longer treated as a row limit;LIMIT ALLremains recognized as a limit.ORDER BYclauses.Impact
✅ Fewer missed select-star findings✅ Correct warnings for queries with OFFSET but no LIMIT✅ Consistent findings across dialect and fallback parsers💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Bug Fixes
INSERT … SELECTqueries and set operations, preserving findings when statements use forms the parser doesn’t model.ORDER BYchecks for parenthesized statements and subqueries.OFFSETis not treated as a row limit;LIMIT ALLremains recognized.Documentation