refactor(management): readability + maintainability sweep#65
Merged
Conversation
service.go was 383 LOC spanning five distinct domains (principal, role, provisioning rule, identity, API token). Split per the exchange/ precedent from PR #63 so a reader can jump straight to the domain they care about. Final layout: - service.go now holds only the scaffolding: Options, Service, NewService. - principal.go, role.go, provisioning.go, identity.go, apitoken.go each hold the methods (and types) for one domain. - types.go is deleted; its IssueAPITokenRequest and IssuedAPIToken move to apitoken.go since they are API-token-specific. The APITokens port moves there too — each domain file owns its own port. No logic changes, no godoc changes (godoc work happens in commit 2).
…n comment The Service struct lacked a godoc. Add a one-liner that names the sparse- options pattern and the app-owned setup-flow audience captured in the package doc. IssueAPIToken is the only method on the package with cross-port logic: it calls FindPrincipal first to validate the principal exists before delegating to the apikey service. Annotate the precondition so the next reader can see *why* both ports are required and *why* the lookup happens before delegation. No per-field godocs on the 15 private Service fields — the field name plus port type already says the same thing, and 15 lines of `// X is the X port` would be noise.
Mechanical lift only — no test logic changes, no fakes touched. service_test.go at 1358 LOC was well past the ~400-line split threshold the earlier passes used. 26 Test functions naturally group by domain. Final layout: - helpers_test.go: all builders (newService, newServiceWithRoles, newServiceWithProvisioningRules, newManagementService), grouping interfaces (roleStore, principalStore, provisioningRuleStore), all fake types and their constructors, fixedTime, provisioningRule, the shared const block. - service_test.go: cross-cutting tests (NewService sparse-options, CoreMethodsRequirePorts, IssueAPITokenRequiresPrincipalFinderBeforeIssuing, DoesNotRequireRolePorts, DoesNotRequireProvisioningRulePorts) plus the two integration anchors (PropagatesContextCancellation, IssueAPITokenResolvesThroughMemoryStore). - principal_test.go, role_test.go, provisioning_test.go, identity_test.go, apitoken_test.go: per-domain method tests. Tests still use the hand-rolled fakes after this commit. The mockery migration is commit 4, separated to keep the file split mechanical. Matches the helpers_test.go precedent from PRs #61 (access/jwt), #63 (exchange), and #64 (http/auth, http/compose).
Add 13 new entries to .mockery.yaml — PrincipalCreator, PrincipalLister, RoleCreator, RoleActionGranter, PrincipalRoleAssigner, PrincipalRoleUnassigner, PrincipalRoleAssignmentLister, ProvisioningRuleCreator, ProvisioningRuleUpdater, ProvisioningRuleDeleter, ProvisioningRuleFinder, ProvisioningRuleLister, IdentityLinker. Each follows the same shape as the 5 entries from PRs #61-#64. .mockery.yaml now covers 18 root-package interfaces. Migrate the 24 mockery-eligible test functions across principal_test.go, role_test.go, provisioning_test.go, identity_test.go, apitoken_test.go, and the sparse-options tests in service_test.go to use the generated mocks with declarative .EXPECT() setups. The "this exact request was passed" assertions collapse from a separately-tracked field-of-the-fake into the mock's argument matcher. Delete the aggregated hand-rolled types fakePrincipalCreator, fakeIdentityLinker, fakeRoleStore, and fakeProvisioningRuleStore, their grouping interfaces roleStore/principalStore/provisioningRuleStore, and the service builders newService/newServiceWithRoles/ newServiceWithProvisioningRules that combined them. helpers_test.go is now ~115 LOC (down from 408). Surviving in helpers_test.go: fakeAPITokens (package-local port + the sub-package apikey.TokenMetadataLister; both stay hand-rolled per the project's mockery pattern), newFakeAPITokens, fixedTime, provisioningRule, the shared const block, and newManagementService (used only by the two integration anchors in service_test.go). Two integration anchors in service_test.go retained unchanged: TestServicePropagatesContextCancellation and TestServiceIssueAPITokenResolvesThroughMemoryStore (real memory.Store + real apikey.Service).
staticcheck (S1016) flagged two field-by-field copy patterns in the
migrated tests:
- role_test.go: authkit.CreateRoleRequest{want.ID, want.DisplayName, want.Description}
- provisioning_test.go: authkit.CreateProvisioningRuleRequest{rule.ID, ...}
Both source/destination pairs have identical field layouts, so a direct
type conversion (T(src)) replaces the literal. Pure cleanup, no behaviour
change.
This was referenced May 28, 2026
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.
Summary
Fifth pass of the multi-session refactor sweep (after
access/PR #61,authz/PR #62,exchange/PR #63,http/PR #64). Same 10-criteria bar applied tomanagement/. Non-goals: bugs, correctness, performance.management/is the largest target of the sweep so far (1778 LOC) and has zero production consumers — the package is the canonical management facade for app authors (CLIs, seed scripts, admin handlers). The Service composes 15 root-package ports plus a package-localAPITokensport.Changes per commit
refactor(management): split service into per-domain files—service.go(383 LOC) split into five per-domain files:principal.go,role.go,provisioning.go,identity.go,apitoken.go.service.gonow holds only the scaffolding (Options,Service,NewService). TheAPITokensport +IssueAPITokenRequest+IssuedAPITokenmove intoapitoken.go;types.gois deleted. Matches the per-domain split precedent from PR refactor(exchange): readability + maintainability sweep #63.refactor(management): add Service godoc and IssueAPIToken precondition comment— godoc on theServicestruct; inline comment on the principal-existence precondition inIssueAPIToken(the one cross-port method, where refusing to mint a token for a missing principal is the security-relevant invariant).test(management): split service_test into per-domain files— mechanical lift of the 1358-line test file into per-domain files (principal_test.go,role_test.go,provisioning_test.go,identity_test.go,apitoken_test.go, plushelpers_test.goand a tighterservice_test.gofor cross-cutting tests + integration anchors). Tests still use the hand-rolled fakes after this commit.test(management): migrate root-port fakes to mockery— adds 13 new entries to.mockery.yaml(the largest mockery expansion in the sweep so far;.mockery.yamlnow covers 18 root-package interfaces). Aggregated hand-rolledfakePrincipalCreator,fakeIdentityLinker,fakeRoleStore,fakeProvisioningRuleStoreand their grouping interfaces are deleted.fakeAPITokenssurvives — it backs the package-localmanagement.APITokensport plus the sub-packageapikey.TokenMetadataLister, both of which stay hand-rolled per the established pattern. Two integration anchors inservice_test.go(realmemory.Store+ realapikey.Service) retained unchanged.chore(management): collapse struct field copies into type conversions— staticcheck (S1016) cleanup after the migration; two field-by-field literals becomeT(src)conversions.Architecture decisions reaffirmed
management.APITokensstays an interface. It has two methods (so the PR refactor(http): readability + maintainability sweep #64 function-type demotion doesn't apply), and the ISP rationale of hidingproof/apikeyfrom the management surface justifies the port even with one implementation. Different from thecompose.PrincipalAuthenticatorSpeccase in PR refactor(http): readability + maintainability sweep #64.Test plan
moon run root:check --summary minimal— format, lint, build, unit, Testcontainers integration. All green.go doc github.com/meigma/authkit/management— public surface unchanged.Deliberately deferred
IssuedAPIToken.Plaintextrename — waits for theproof/apikey/pass so the field name realigns with whateverapikey.IssuedToken.Plaintextbecomes (TokenorSecret), avoiding mid-sweep asymmetry. Carried open thread from PR refactor(exchange): readability + maintainability sweep #63..mockery.yaml— still unwired. The file now has 18 entries; the open thread is becoming more pressing. Future task.🤖 Generated with Claude Code