refactor(admin): push handler business logic into feature services#478
Merged
Conversation
Four blocks the architecture review flagged as business logic living in admin HTTP handlers move to their owning packages, with unit tests in the new homes; handlers keep only decode/gate/delegate/encode: - failover.GenerateSuggestions owns dashboard suggestion generation (registry iteration, text-generation category filter, source matching, resolver ranking, View assembly); the handler parses params and encodes. - budget.CheckResult gains UsageRatio (deliberately unclamped so >1 reads as exceeded) and PeriodRatio (clamped elapsed-period fraction), replacing inline math and clampBudgetRatio in the handler. - usage.SummarizeUsageForRequestIDs collapses the load-then-summarize dance behind one call on a narrow RequestUsageLoader seam; the audit handler keeps its warn-and-continue enrichment behavior. - virtualmodels.Service.ResolveUpsertEnabled owns the preserve-stored- Enabled-on-omit/rename rule the upsert mapping previously encoded. No behavior change; admin handler tests pass unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR extracts previously inline logic in admin handlers into reusable package-level helpers: usage summarization ( ChangesUsage Summarization Helper
Budget Ratio Calculation
Failover Suggestion Generation
Virtual Model Enabled Resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AuditHandler as auditLogResponse
participant Usage as SummarizeUsageForRequestIDs
participant Loader as RequestUsageLoader
AuditHandler->>Usage: SummarizeUsageForRequestIDs(ctx, usageReader, requestIDs)
Usage->>Loader: GetUsageByRequestIDs(ctx, requestIDs)
Loader-->>Usage: usage entries or error
Usage-->>AuditHandler: summaries map or error
AuditHandler->>AuditHandler: log warning and continue on error
sequenceDiagram
participant AdminHandler as GenerateFailoverRules
participant Failover as failover.GenerateSuggestions
participant Resolver as failover resolver
participant Registry as Registry
AdminHandler->>Failover: GenerateSuggestions(registry, rules, primaryModel)
Failover->>Resolver: create resolver from FailoverConfig
Failover->>Registry: iterate text-generation models
Failover->>Resolver: SuggestFailovers(OperationChatCompletions)
Resolver-->>Failover: candidate targets
Failover-->>AdminHandler: []View suggestions
Possibly related PRs
Poem
🚥 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. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/virtualmodels/service_test.go`:
- Around line 497-523: Refactor TestService_ResolveUpsertEnabled into a
table-driven test to cover the four precedence cases in a clearer, more
idiomatic way. Keep the existing setup with newTestService, svc.Upsert, and
ResolveUpsertEnabled, but move the explicit override, omit-preserves,
rename-preserves, and default-enabled checks into named table entries with
expected outcomes and failure messages. This will make the scenarios easier to
read and extend while keeping the same behavior coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c147044-2aa2-4a23-b9b6-0f68dd32f5f4
📒 Files selected for processing (12)
internal/admin/handler_audit.gointernal/admin/handler_budgets.gointernal/admin/handler_failover.gointernal/admin/handler_virtualmodels.gointernal/budget/types.gointernal/budget/types_test.gointernal/failover/suggest.gointernal/failover/suggest_test.gointernal/usage/request_summary.gointernal/usage/request_summary_loader_test.gointernal/virtualmodels/service.gointernal/virtualmodels/service_test.go
Comment on lines
+497
to
+523
| func TestService_ResolveUpsertEnabled(t *testing.T) { | ||
| t.Parallel() | ||
| svc := newTestService(t) | ||
| ctx := context.Background() | ||
|
|
||
| if err := svc.Upsert(ctx, VirtualModel{ | ||
| Source: "fast", | ||
| Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, | ||
| Enabled: false, | ||
| }); err != nil { | ||
| t.Fatalf("Upsert() error = %v", err) | ||
| } | ||
|
|
||
| enabled := true | ||
| if got := svc.ResolveUpsertEnabled("fast", "", &enabled); !got { | ||
| t.Fatal("explicit request value must win over the stored flag") | ||
| } | ||
| if got := svc.ResolveUpsertEnabled("fast", "", nil); got { | ||
| t.Fatal("omitted flag must preserve the stored (disabled) value") | ||
| } | ||
| if got := svc.ResolveUpsertEnabled("renamed", "fast", nil); got { | ||
| t.Fatal("rename must preserve the flag of the row being renamed") | ||
| } | ||
| if got := svc.ResolveUpsertEnabled("brand-new", "", nil); !got { | ||
| t.Fatal("new rows must default to enabled") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider a table-driven test for the four scenarios.
The test exercises four distinct precedence branches (explicit override, omit-preserves, rename-preserves, default-enabled) via sequential if blocks. A table-driven structure would make each case's inputs/expectations easier to scan and extend.
♻️ Proposed table-driven refactor
func TestService_ResolveUpsertEnabled(t *testing.T) {
t.Parallel()
svc := newTestService(t)
ctx := context.Background()
if err := svc.Upsert(ctx, VirtualModel{
Source: "fast",
Targets: []Target{{Provider: "openai", Model: "gpt-4o"}},
Enabled: false,
}); err != nil {
t.Fatalf("Upsert() error = %v", err)
}
enabled := true
- if got := svc.ResolveUpsertEnabled("fast", "", &enabled); !got {
- t.Fatal("explicit request value must win over the stored flag")
- }
- if got := svc.ResolveUpsertEnabled("fast", "", nil); got {
- t.Fatal("omitted flag must preserve the stored (disabled) value")
- }
- if got := svc.ResolveUpsertEnabled("renamed", "fast", nil); got {
- t.Fatal("rename must preserve the flag of the row being renamed")
- }
- if got := svc.ResolveUpsertEnabled("brand-new", "", nil); !got {
- t.Fatal("new rows must default to enabled")
- }
+ tests := []struct {
+ name string
+ source string
+ oldSource string
+ requested *bool
+ want bool
+ }{
+ {"explicit value wins", "fast", "", &enabled, true},
+ {"omitted preserves stored disabled", "fast", "", nil, false},
+ {"rename preserves source row flag", "renamed", "fast", nil, false},
+ {"new row defaults to enabled", "brand-new", "", nil, true},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ if got := svc.ResolveUpsertEnabled(tt.source, tt.oldSource, tt.requested); got != tt.want {
+ t.Fatalf("ResolveUpsertEnabled() = %v, want %v", got, tt.want)
+ }
+ })
+ }
}As per coding guidelines, **/*.go code should "use idiomatic Go, prefer clear names, small interfaces, simple structs, and table-driven tests".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestService_ResolveUpsertEnabled(t *testing.T) { | |
| t.Parallel() | |
| svc := newTestService(t) | |
| ctx := context.Background() | |
| if err := svc.Upsert(ctx, VirtualModel{ | |
| Source: "fast", | |
| Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, | |
| Enabled: false, | |
| }); err != nil { | |
| t.Fatalf("Upsert() error = %v", err) | |
| } | |
| enabled := true | |
| if got := svc.ResolveUpsertEnabled("fast", "", &enabled); !got { | |
| t.Fatal("explicit request value must win over the stored flag") | |
| } | |
| if got := svc.ResolveUpsertEnabled("fast", "", nil); got { | |
| t.Fatal("omitted flag must preserve the stored (disabled) value") | |
| } | |
| if got := svc.ResolveUpsertEnabled("renamed", "fast", nil); got { | |
| t.Fatal("rename must preserve the flag of the row being renamed") | |
| } | |
| if got := svc.ResolveUpsertEnabled("brand-new", "", nil); !got { | |
| t.Fatal("new rows must default to enabled") | |
| } | |
| } | |
| func TestService_ResolveUpsertEnabled(t *testing.T) { | |
| t.Parallel() | |
| svc := newTestService(t) | |
| ctx := context.Background() | |
| if err := svc.Upsert(ctx, VirtualModel{ | |
| Source: "fast", | |
| Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, | |
| Enabled: false, | |
| }); err != nil { | |
| t.Fatalf("Upsert() error = %v", err) | |
| } | |
| enabled := true | |
| tests := []struct { | |
| name string | |
| source string | |
| oldSource string | |
| requested *bool | |
| want bool | |
| }{ | |
| {"explicit value wins", "fast", "", &enabled, true}, | |
| {"omitted preserves stored disabled", "fast", "", nil, false}, | |
| {"rename preserves source row flag", "renamed", "fast", nil, false}, | |
| {"new row defaults to enabled", "brand-new", "", nil, true}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if got := svc.ResolveUpsertEnabled(tt.source, tt.oldSource, tt.requested); got != tt.want { | |
| t.Fatalf("ResolveUpsertEnabled() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/virtualmodels/service_test.go` around lines 497 - 523, Refactor
TestService_ResolveUpsertEnabled into a table-driven test to cover the four
precedence cases in a clearer, more idiomatic way. Keep the existing setup with
newTestService, svc.Upsert, and ResolveUpsertEnabled, but move the explicit
override, omit-preserves, rename-preserves, and default-enabled checks into
named table entries with expected outcomes and failure messages. This will make
the scenarios easier to read and extend while keeping the same behavior
coverage.
Source: Coding guidelines
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
2 participants
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
Next architecture-review item (doc in #472, §6/admin): four blocks of business logic lived inside admin HTTP handlers instead of the feature services they belong to. Each moves to its owning package; the handlers keep only decode → gate → delegate → encode.
GenerateFailoverRuleshandler loop (registry iteration, category filter, source matching, resolver ranking,Viewassembly)failover.GenerateSuggestions(registry, rules, primaryModel)clampBudgetRatiobudget.CheckResult.UsageRatio()(deliberately unclamped — >1 reads as exceeded) and.PeriodRatio(now)(clamped)usage.SummarizeUsageForRequestIDs(ctx, loader, ids)on a narrowRequestUsageLoaderseamvirtualmodels.Service.ResolveUpsertEnabled(source, oldSource, requested)Each move gets unit tests in its new home (suggestion generation incl. category filter + primary-model filter, ratio edge cases incl. clamping bounds, loader error/nil paths, and all four enabled-resolution branches). The admin handler tests pass unchanged — no behavior change.
User-visible impact
None.
Testing
Full
go build ./...andgo test ./...;golangci-lint0 issues on all touched packages; pre-commitmake test-raceon the commit.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes