refactor: simplify code across packages for clarity#196
Merged
Conversation
- Replace if/else and errors.Is chains with switch and guard clauses - Extract shared helpers for duplicated validation and rendering logic - Deduplicate cache-aside logic behind a single generic helper - Hoist repeated periodic-job bodies into reusable closures - Collapse redundant token expiry and audit field-masking checks Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a broad, behavior-preserving refactor pass across core AuthGate packages (token issuance, middleware, config validation, caching, handlers, and bootstrap), aiming to simplify control flow, reduce duplication, and centralize repeated helper logic without changing exported APIs or runtime behavior.
Changes:
- Extracted/shared helpers to reduce duplication (e.g., cache-aside singleflight helper, repeated config validation, shared render/abort helpers).
- Simplified control flow via guard clauses /
switchblocks across handlers, middleware, and services. - Small internal cleanups (output file selection in asset build script, context string getter helper, minor bootstrap job refactors).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/build_assets.go | Extracts shared helpers for esbuild error handling and output file selection. |
| internal/util/url.go | Simplifies relative redirect safety checks into a single boolean expression. |
| internal/util/context.go | Adds a shared context string accessor and reuses it for request metadata getters. |
| internal/token/local.go | Simplifies symmetric/asymmetric public key selection and TTL/expiry calculation flow. |
| internal/store/user.go | Refactors username-conflict checks into a switch/guard style without changing outcomes. |
| internal/store/sqlite.go | Simplifies default admin password selection while preserving generated-password tracking. |
| internal/services/token_exchange.go | Preserves device-code metrics while simplifying expired vs invalid branching. |
| internal/services/audit.go | Extracts case-insensitive substring matching helper and reuses it for masking decisions. |
| internal/middleware/metrics_auth.go | Centralizes repeated 401 abort logic while preserving constant-time token compare. |
| internal/middleware/csrf.go | Extracts state-changing method detection and centralizes CSRF-forbidden rendering. |
| internal/middleware/auth.go | Extracts session-clear+save helper to reduce duplication on stale/disabled sessions. |
| internal/metrics/http.go | Adds a small helper for success/failure label mapping and simplifies metrics middleware gating. |
| internal/handlers/device.go | Simplifies default scope assignment and error mapping via switch cases. |
| internal/handlers/auth.go | Centralizes login page rendering props in a helper to reduce duplication. |
| internal/handlers/audit.go | Extracts shared audit filter parsing used by both list and CSV export. |
| internal/core/client_type.go | Reuses OrDefault() to simplify normalization logic. |
| internal/config/config.go | Refactors cache-type validation and repeated duration checks into shared helpers (error strings preserved). |
| internal/cache/util.go | Introduces a shared generic cache-aside + singleflight helper for reuse by cache implementations. |
| internal/cache/rueidis.go | Switches GetWithFetch to reuse the shared cache-aside helper. |
| internal/cache/memory.go | Switches GetWithFetch to reuse the shared cache-aside helper; simplifies reaper enable/disable logic. |
| internal/bootstrap/server.go | Extracts repeated cleanup/update job bodies into local closures for clarity. |
| internal/bootstrap/ratelimit.go | Simplifies enabled/disabled rate limit middleware selection with an early return. |
| internal/bootstrap/providers.go | Removes redundant err declaration while keeping key derivation logic intact. |
| internal/auth/oauth_provider.go | Simplifies display-name fallback branch while preserving provider naming behavior. |
Files not reviewed (1)
- scripts/build_assets.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
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.
Summary
Behavior-preserving simplification pass across the codebase, applied package-by-package. Replaces if/else and
errors.Ischains withswitch/guard clauses, extracts shared helpers for duplicated logic, and removes dead/redundant branches. No functional, API, or schema changes; net −88 lines across 24 files.AI Authorship
code-simplifieragent dispatched per packageChange classification
Touches auth, token issuance, middleware (CSRF/session/metrics-auth), config validation, and the store layer. Security-sensitive invariants (constant-time comparisons, PKCE/RFC 8707 logic, JWT claim namespacing,
StringArraynull-vs-[]serialization) were explicitly preserved, but should be confirmed by review.Plan reference
No plan.md. Goal: improve clarity/consistency/maintainability without changing behavior or exported signatures, following the repo's coding conventions (early returns,
switch/lookup tables over if/else chains,http.Status*constants).Verification
make test— full suite green)make generate(clean),make fmt(clean),make build(OK),make lint(0 issues)make generate && make lint && make testVerifiability check
Security check
Risk & rollback
switchnot matching the originalerrors.Ischain, or a guard clause changing an edge case). Mitigated by full test + lint pass, but unreviewed.382c557or close the PR; no migrations or data changes.Reviewer guide
internal/services/token_exchange.go,internal/token/local.go— token issuance/expiry logicinternal/middleware/csrf.go,middleware/auth.go,middleware/metrics_auth.go— security checks and constant-time comparesinternal/config/config.go— validation helpers (error messages must stay byte-identical)internal/cache/util.go(+memory.go/rueidis.go) — new generic cache-aside helper, verify singleflight re-check orderinginternal/util/*,internal/core/client_type.go,internal/handlers/audit.go/auth.go/device.go,internal/store/*,scripts/build_assets.go,internal/metrics/http.go🤖 Generated with Claude Code