refactor: deduplicate cache, pagination, and OAuth error helpers#101
refactor: deduplicate cache, pagination, and OAuth error helpers#101
Conversation
…lpers - Consolidate three identical cache type constant sets into single CacheType* set - Extract validateCacheType helper to replace tripled validation logic - Create generic initializeCache[T any] to deduplicate three cache init functions - Eliminate redundant DB lookup in RequireAdmin by reading cached user from context - Add parsePaginationParams, respondOAuthError, and getUserFromContext helpers - Replace 15+ inline OAuth error responses with respondOAuthError calls - Extract defaultTokenType helper for duplicated token type defaulting - Remove unused imports from handlers after consolidation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors repeated logic across handlers and bootstrap code by introducing small shared helpers (pagination parsing, OAuth error responses, user retrieval), simplifying admin middleware by relying on RequireAuth’s cached user, and consolidating cache-type constants plus cache initialization/validation logic.
Changes:
- Centralize handler utilities: pagination param parsing, OAuth error JSON responses, and user extraction from
gin.Context. - Simplify
RequireAdminto rely onRequireAuth-loaded user in context rather than re-fetching viaUserService. - Unify cache-type constants and deduplicate cache init + config validation logic for metrics/user/client-count caches.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/token/http_api.go | Extracts token-type defaulting into a helper for reuse. |
| internal/middleware/auth.go | Updates RequireAdmin to use the cached user from context. |
| internal/handlers/utils.go | Adds shared helper functions for pagination, OAuth errors, and user lookup. |
| internal/handlers/user_client.go | Switches to shared pagination/user-context helpers. |
| internal/handlers/token.go | Switches repeated OAuth error responses to respondOAuthError. |
| internal/handlers/session.go | Switches pagination parsing to shared helper. |
| internal/handlers/docs.go | Uses shared user-context helper for optional navbar user info. |
| internal/handlers/client.go | Switches to shared pagination/user-context helpers. |
| internal/handlers/authorization.go | Uses shared user-context helper (fallbacking to DB lookup). |
| internal/handlers/audit.go | Uses shared user-context helper. |
| internal/config/config.go | Consolidates cache-type constants; deduplicates cache-type validation. |
| internal/config/config_test.go | Updates tests to use unified cache-type constants and expectations. |
| internal/bootstrap/cache.go | Deduplicates cache initialization via a generic helper and shared options struct. |
| internal/bootstrap/bootstrap_test.go | Updates bootstrap test to use unified cache-type constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/handlers/token.go
Outdated
| respondOAuthError( | ||
| c, | ||
| http.StatusBadRequest, |
There was a problem hiding this comment.
In the refresh_token grant error mapping, client authentication failures are returned as 400 invalid_client. Per RFC 6749 §5.2, invalid_client should be a 401 response and include a WWW-Authenticate header (similar to the client_credentials grant handling below). Consider switching this branch to 401 and setting the header to keep behavior consistent and standards-compliant.
| respondOAuthError( | |
| c, | |
| http.StatusBadRequest, | |
| // Per RFC 6749 §5.2, invalid_client should use 401 and include WWW-Authenticate | |
| c.Header("WWW-Authenticate", `Basic realm="token"`) | |
| respondOAuthError( | |
| c, | |
| http.StatusUnauthorized, |
| func RequireAdmin(userService *services.UserService) gin.HandlerFunc { | ||
| // RequireAdmin is a middleware that requires the user to have admin role. | ||
| // This middleware should be used after RequireAuth, which already fetches | ||
| // and caches the user in the gin context via loadUserFromSession. |
There was a problem hiding this comment.
RequireAdmin no longer uses the provided UserService, but the parameter is still part of the API (now named _). This is misleading for callers and suggests a dependency that no longer exists. Consider removing the parameter entirely and updating call sites, or (if you need to keep the signature) document why the argument is intentionally unused.
| // and caches the user in the gin context via loadUserFromSession. | |
| // and caches the user in the gin context via loadUserFromSession. | |
| // | |
| // The *services.UserService parameter is intentionally unused and retained | |
| // only for backward compatibility with existing call sites. RequireAdmin | |
| // no longer depends on UserService and instead reads the user from context. |
- Return 401 with WWW-Authenticate header for invalid_client per RFC 6749 §5.2 - Remove unused userService parameter from RequireAdmin middleware - Add unit tests for RequireAdmin, parsePaginationParams, respondOAuthError, getUserFromContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
MetricsCacheType*,UserCacheType*,ClientCountCacheType*) into a singleCacheType*set with a sharedvalidateCacheTypehelper, reducing ~80 lines of duplicated validation logicinitializeCache[T any]inbootstrap/cache.goto replace three nearly identical cache init functions (~160 lines removed)RequireAdminmiddleware by reading the user already cached in gin context byRequireAuth(saves one DB query per admin request)parsePaginationParams,respondOAuthError, andgetUserFromContexthandler helpers to replace repeated patterns across 6+ handler filesdefaultTokenTypeintoken/http_api.goto deduplicate token type defaultingNet result: 350 insertions, 478 deletions (−128 lines)
Test plan
make generate— templates and swagger compilemake test— all existing tests passmake lint— zero lint issues🤖 Generated with Claude Code