fix: analyze a query once per execution, not once per attempt (#67) - #85
Merged
Merged
Conversation
…67) wConn.QueryContext/ExecContext analyzed the query before handing it to the base driver. A base with no direct Queryer/Execer was handled, but not one that has the entry point and declines an individual call with driver.ErrSkip: database/sql then falls back to Prepare+Query, which re-enters through wStmt and analyzes the same execution a second time. That is the common path, not an edge case — go-sql-driver/mysql returns ErrSkip for every parameterized query unless interpolateParams=true — so on MySQL essentially all traffic was analyzed twice. The duplicate static finding hid behind the default dedup window, but the N+1 counter is not deduped: every configured threshold was effectively halved. The base is now called first and analyzeExecuted skips analysis when the answer is ErrSkip, leaving the prepare path as the single analysis point. Findings are produced after execution on this path, which nothing consumes, and the latency window is read before the rules run so analysis time cannot push a query past the slow-query threshold. The fake ErrSkip driver in driver_fallback_test.go pins the behaviour (2 findings and a tripped N+1 threshold before the fix), alongside a direct-path driver proving bases that do execute are unaffected.
Review follow-up on the #67 fix: ErrSkip was not the only per-call answer that means "this did not execute". database/sql retries a query on driver.ErrBadConn — twice from the pool, then once on a brand-new connection — and every attempt re-enters the wrapper. One logical query was therefore analyzed up to three times, at the conn and the stmt level alike, which is the same N+1 inflation #67 fixed and shows up whenever pooled connections go stale (MySQL's wait_timeout, a restart, a failover). ErrBadConn's contract forbids returning it when the operation may have been performed, so a declined attempt ran nothing and analyzeExecuted now skips it. wStmt likewise analyzed before converting named parameters for a base that predates them, so a call failing with "driver does not support named parameters" — without reaching the database — still produced findings and bumped the N+1 counter. Conversion now runs first, and the statement paths use analyzeExecuted like the conn paths, so Guard.Observe is left to the out-of-tree integrations, which are only told a query ran. Three fake-driver tests pin it; against the previous commit they report 3 analyses for one retried query, a tripped N+1 threshold, and 1 analysis for a query that never ran.
🤖 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 · |
Contributor
|
Warning Review limit reachedNext included review available in 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: KARTIKrocks/sqlguard/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
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. Comment |
codeant-ai
Bot
added
the
size:L
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
CodeAnt Nitpicks1 custom suggestion1. Remove or rephrase the upstream driver option name, because documentation option names must be exported identifiers or registered rule names; describe the behavior without naming
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Fixes #67.
The bug
wConn.QueryContext/ExecContextanalyzed the query before handing it tothe base driver. The existing comment covered a base with no direct
Queryer/Execerat all, but not one that has the entry point anddeclines an individual call with
driver.ErrSkip.database/sqlthen fallsback to Prepare+Query, which re-enters through
wStmt— and the sameexecution is analyzed a second time.
go-sql-driver/mysqlreturnsErrSkipfor every parameterized query unlessinterpolateParams=true(off by default), so on MySQL essentially allapplication traffic was double-analyzed. The duplicate static finding hid
behind the default one-minute dedup window; the N+1 counter is not deduped, so
every configured threshold was effectively halved —
WithN1Detection(10, …)fired at 5 real queries.
The fix
Call the base first, then
analyzeExecuted, which returns withoutCheck/CheckLatencywhen the answer iserrors.Is(err, driver.ErrSkip).The prepare path is left as the single analysis point.
Two consequences, both deliberate:
findings pre-execution.
push a query past the slow-query threshold (it previously sat outside the
window because
Checkran before the timer started).Second commit: the other "did not run" answers
A review pass found
ErrSkipis not the only one, and that the invariant thefirst commit writes into
AGENTS.mdwas not yet true:driver.ErrBadConn.database/sqlretries the query on anotherconnection — twice from the pool, then once on a fresh one — and each attempt
re-enters the wrapper. Reproduced: 3 base calls, 3 analyses for one
db.Query.ErrBadConn's contract forbids returning it when the operationmay have been performed, so a declined attempt ran nothing. Surfaces whenever
pooled connections go stale — MySQL's
wait_timeout, a restart, a failover.wStmt's context methodsanalyzed before converting named parameters for a base that predates them, so
a call failing with
sqlguard: driver does not support named parametersstill produced findings and bumped the N+1 counter.
So the statement paths use
analyzeExecutedtoo, and conversion runs first.Guard.Observeis now left to the out-of-tree integrations, which are onlyever told a query ran —
database/sqlhas noErrSkipfallback forStmtQueryContext/StmtExecContext, but it does retry them onErrBadConn.Against the first commit the three added tests report
got 3, a tripped N+1threshold, and
got 1for a query that never ran.Tests
middleware/driver_fallback_test.gogainsfakeErrSkipDriver— a conn thatimplements
QueryerContext/ExecerContextand declines withErrSkip,mirroring mysql. Reverting the
driver.gochange:fakeBadConnDrivercovers the retry path andfakeQueryerDrivercovers the other half of the acceptance criteria: a basethat does handle the direct path executes the query itself and is still
analyzed exactly once. The shared N+1 assertion now runs against all three
fake drivers.
Also
AGENTS.md: records the analyze-after-the-base invariant, soGuard.Observeis not restored here by a later simplification. The reviewer configs merged
in chore: configure CodeAnt AI and sync the other reviewer configs #84 already name this rule
analyze-once-per-executionand describeanalyzeExecutedand thefakeBadConnDriverfixtures; this PR is what makesthe code satisfy it.
CHANGELOG.md+website/docs/middleware.md(_Changed in 0.5._).make fmt-check vet lint test test-race lint-docsall green, rebased oncurrent
main.CodeAnt-AI Description
Analyze each database execution exactly once
What Changed
ErrSkipare analyzed only on the prepare-and-execute fallback path, preventing duplicate findings and inflated N+1 countsErrBadConnno longer count failed connection attempts as executionsImpact
✅ Accurate N+1 thresholds on MySQL parameterized queries✅ No duplicate findings during connection retries✅ No reports for queries rejected before execution💡 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.