fix: replace sql.ErrNoRows with auth.ErrNotFound across handlers and tests#17
Conversation
Agent-Logs-Url: https://github.com/amalgamated-tools/goauth/sessions/b01ffef3-a610-46f6-ac18-97540ee4dcb1 Co-authored-by: veverkap <22348+veverkap@users.noreply.github.com>
|
/grumpy |
|
😤 Fine. Grumpy Code Reviewer 🔥 finished the review. It wasn't completely terrible. I guess. 🙄 |
There was a problem hiding this comment.
Pull request overview
This PR standardizes “not found” handling across the handler layer by replacing direct use of database/sql.ErrNoRows with the store-abstraction sentinel auth.ErrNotFound, aligning handlers, tests, and documentation with the intended store contract.
Changes:
- Update handlers to check
auth.ErrNotFound(viaerrors.Is) and dropdatabase/sqlimports. - Update handler test mocks to return
auth.ErrNotFoundinstead ofsql.ErrNoRows. - Update store-interface documentation in
auth/types.goand consumer guidance inREADME.md.
Show a summary per file
| File | Description |
|---|---|
| handler/session.go | Uses auth.ErrNotFound for revoke/delete “not found” behavior; removes database/sql dependency. |
| handler/session_test.go | Updates mock not-found sentinel to auth.ErrNotFound; removes database/sql import. |
| handler/passkey.go | Uses auth.ErrNotFound for credential delete “not found” path; removes database/sql dependency. |
| handler/passkey_test.go | Updates mock store methods to return auth.ErrNotFound and removes database/sql import. |
| handler/oidc.go | Replaces err != sql.ErrNoRows with !errors.Is(err, auth.ErrNotFound) in findOrCreateUser; removes database/sql import. |
| handler/oidc_test.go | Updates mock not-found sentinel to auth.ErrNotFound; removes database/sql import. |
| handler/email_verification.go | Uses auth.ErrNotFound for “user not found” and “token not found” handling; removes database/sql dependency. |
| handler/email_verification_test.go | Updates mock not-found sentinel to auth.ErrNotFound and adjusts related comment. |
| handler/apikey.go | Uses auth.ErrNotFound for delete “not found” path; removes database/sql dependency. |
| handler/apikey_test.go | Updates mock delete not-found sentinel to auth.ErrNotFound; removes database/sql import. |
| auth/types.go | Updates store interface comments to specify ErrNotFound instead of sql.ErrNoRows. |
| README.md | Updates documentation guidance to return auth.ErrNotFound for missing records. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 1
| }, | ||
| } | ||
| // Non-sql.ErrNoRows errors log and return 200 to avoid leaking info. | ||
| // Non-ErrNotFound errors log and return 200 to avoid leaking info. |
There was a problem hiding this comment.
The updated comment is a bit ambiguous about which ErrNotFound it refers to. Consider spelling it out as auth.ErrNotFound to match the rest of the file and avoid confusion for readers scanning quickly.
| // Non-ErrNotFound errors log and return 200 to avoid leaking info. | |
| // Non-auth.ErrNotFound errors log and return 200 to avoid leaking info. |
There was a problem hiding this comment.
The actual code fix is correct — errors.Is(err, auth.ErrNotFound) replaces direct sentinel comparisons and database/sql is properly evicted from the handler layer. Fine.
But the documentation updates are a mess. The PR established a pattern of documenting ErrNotFound in interface comments, applied it to SessionStore and EmailVerificationStore, then stopped. PasskeyStore, APIKeyStore, and half of UserStore are left undocumented. The README is similarly incomplete. Fix the docs to match what the handlers actually depend on.
😤 Reluctantly reviewed by Grumpy Code Reviewer 🔥
To install this agentic workflow, run
gh aw add amalgamated-tools/biblioteka/.github/workflows/grumpy-reviewer.md@0dff8ccb0ced8d634877b4201f25795e659dced0
Merge origin/main into copilot/fix-mocks-and-handlers. Resolved conflict in email_verification_test.go by using the more accurate auth.ErrNotFound reference in the comment.
Handlers were comparing against
sql.ErrNoRowsdirectly, leaking a driver-specific sentinel through the store abstraction boundary. This made fixing the store layer in isolation impractical — any correction would silently break the not-found paths in handlers and discourage the fix.Changes
Handlers — swap
sql.ErrNoRowsforauth.ErrNotFound, dropdatabase/sqlimports:email_verification.go: send path (silent 200) + consume path (400)oidc.go:findOrCreateUsersubject/email lookups — also convertserr != sql.ErrNoRowstoerrors.Is(err, auth.ErrNotFound)session.go,apikey.go,passkey.go: not-found → 404 on deleteTest mocks — align mock return values with the corrected contract:
sql.ErrNoRowsreturns inemail_verification_test.go,oidc_test.go,session_test.go,apikey_test.go,passkey_test.goreplaced withauth.ErrNotFoundDocs — update
auth/types.gointerface comments andREADME.mdto referenceauth.ErrNotFoundinstead ofsql.ErrNoRows