Repository navigation
Conversation
…auth.API* request and response types CreateUserAPI, UpdateUserAPI, CreateGroupAPI and UpdateGroupAPI took and returned any, and each service implementation opened with a type assertion that failed with "invalid request type" at runtime. The import cycle that once justified any is gone (api already imports auth), so the interface, auth.Service, the server adapter and every test double now use the concrete request types and *APICreateUserResponse / *APIUser / *APIGroup responses. A wrong request type is now a compile error. No behavior change: the handlers decode into the same structs and return the same values to the same JSON encoder. A new HandleRequest-level test pins the four endpoints' request values and response bytes; it passes unchanged on both sides of this commit. The three adapter tests that passed maps (and so only ever hit the type-assertion error) now send typed requests and assert the store write and response. Closes LeanerCloud#169 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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.
Root cause
AuthServiceInterface(internal/api/types.go) declared the four user and group write methods withanyrequest and response types:Each
auth.Serviceimplementation started with a type assertion that returnedfmt.Errorf("invalid request type")on a mismatch. A caller passing the wrong type therefore failed at runtime (as a 500) instead of at compile time. The comment ininternal/auth/service_api.gosaidanywas there "to avoid import cycles with the api package". That reason no longer holds:internal/apialready importsinternal/auth(auth.PermissionConstraints,auth.APIPermissionand others appear in the same interface), so the interface can name theauth.API*types directly. No types need to move.Fix
Signatures only; no behavior change.
internal/api/types.go: the four methods now takeauth.APICreateUserRequest/auth.APIUpdateUserRequest/auth.APICreateGroupRequest/auth.APIUpdateGroupRequest. They return*auth.APICreateUserResponse/*auth.APIUser/*auth.APIGroup/*auth.APIGroup. These are exactly the dynamic types the service already returned.internal/auth/service_api.go: the implementations take the concrete types, the four type assertions and their"invalid request type"errors are gone, and the stale import-cycle comment is corrected.internal/server/app.go: theauthServiceAdaptersignatures are updated. The bodies are unchanged pass-throughs.MockAuthService(mocks_test.go),mockAuthForExchange(handler_ri_exchange_test.go),realGroupAuth(group_explicit_zero_cap_test.go) andadminUserUpdateAuth(handler_users_update_race_integration_test.go, integration tag).The handlers did not change. They already decode into these structs and pass the result straight to the JSON encoder, so the encoder receives the same values as before.
Test changes that fall out of the type change
MockAuthServicenow type-asserts its canned return values, the same way it does forGetUser. Several handler tests returnedmap[...]fixtures, which can no longer satisfy the interface. They now return the realauth.API*types.TestHandler_createUser_Successhad also been returning*auth.APIUserwhere the real service returns*auth.APICreateUserResponse; the stronger mock types caught that drift.internal/auth/service_api_test.go: the four"invalid request type"subtests are removed because that code path is now a compile error. Theresult.(*T)assertions become direct field access.internal/server/adapter_test.go:TestAuthServiceAdapter_CreateUserAPI/_CreateGroupAPI/_UpdateGroupAPIpassedmap[string]interface{}. They only ever reached the type-assertion error, and they ended in_ = err. They now send typed requests and assert the store write and the returned value.Wire format: proved byte-identical
New
internal/api/user_group_wire_format_test.go:TestUserGroupWrites_WireFormatdrivesHandler.HandleRequest(body decode → router → handler → interface →json.Marshal) forPOST /api/users,PUT /api/users/{id},POST /api/groupsandPUT /api/groups/{id}. For each request it asserts:max_amountmarker thatAPIPermissionConstraint.UnmarshalJSONsets (fix(frontend): a max_amount spend cap of 0 is silently erased when a group is edited #133), and the non-nil emptyallowed_accountsthat means "clear the restriction".main.The test file is identical before and after the refactor: sha256
b91f2ee923024cbb16ebe5d936a12c1dc8aaac2382ad6d84705bf5039086fb58on both sides, and it passes on both.TestAuthServiceInterface_UserGroupWritesAreTypeduses reflection to pin the four methods' request and response types. It is the fail-before/pass-after guard against a regression back toany.Typed-nil note: the real service returned a typed
*APIUser/*APIGroupinsideanybefore this change too, so even a hypothetical nil-pointer success would encode the same (null) on both sides. Only the old mock could return an untypednil.Closes #169
Verification
Unit, mock and fixture evidence on this branch (
7d75eb53). The env for every command wasAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.Fail-before, on unmodified
mainwith only the new test file added:Pass-after, on this branch with the same test file (same sha256):
Other checks:
No DB, migration or concurrency change, so no new race test is needed.
Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope (deliberately left)
The issue scopes the four user and group write methods, and that is what this PR changes. Other
AuthServiceInterfacemethods still useanyand could get the same treatment in a follow-up:ListUsersAPI,GetGroupAPI,ListGroupsAPIandGetUserPermissionsAPI(response only).handler_auth.goandhandler_registrations.gostill type-assert the latter two results, and the comment there still cites the import cycle.CreateAPIKeyAPI(request and response),ListUserAPIKeysAPI,GetAPIKeysUsageStatsAPIandValidateUserAPIKeyAPI((any, any, error)), whichmiddleware.goandhandler_auth.gotype-assert.Labels
Could a maintainer please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/m.🤖 Generated with Claude Code