audit(internal): API* → internal-request adapter field-drop hygiene (followup to #893) #19

Background

PR LeanerCloud/cloud-commitments-cli#893 fixed a silent-field-drop bug in Service.UpdateUserAPI (internal/auth/service_api.go:222-239):

APIUpdateUserRequest had Email string, but the internal UpdateUserRequest (internal/auth/types.go:239) had no Email field. The adapter copied only Role and Groups, dropping req.Email silently — UPDATE users then wrote the OLD email back. Handler returned HTTP 200, frontend showed success toast, DB unchanged.

The bug class: API-layer request struct exposes a field, internal-layer request struct doesn't, adapter copies field-by-field with no compiler check that all fields cross the boundary. Cause: drift between the two struct definitions over time as one side gains a field but the corresponding line is never added to the adapter.

Scope

Audit all *API*Request → *Request adapter pairs in the codebase for the same shape of bug. Specifically look at:

1. internal/auth/service_api.go (where the bug lived) — other adapters in the same file:

   • APIUpdateGroupRequest → UpdateGroup (groups can be edited the same way — verify every API field reaches the store)
   • APICreateUserRequest → CreateUserRequest (PR fix(auth/users): persist email change in admin update path (closes #892) cloud-commitments-cli#893 didn't touch this — verify all fields cross)
   • APICreateGroupRequest → CreateGroupRequest
   • Any *API*Permission* / *API*Constraint* adapters

2. internal/api/handler_*.go — handlers that translate {Path}Request JSON into store-layer struct calls:

   • Plan/account/service-config edit handlers
   • Purchase execution updates
   • User profile self-edit (updateUserEmail etc.)
   • Settings/override edits

3. General pattern grep:

   grep -rn "type API.*Request" internal/ --include="*.go"
   grep -rn "^func.*UpdateUser\|^func.*UpdateGroup\|^func.*CreateUser\|^func.*CreateGroup" internal/auth/ internal/api/ --include="*.go"
   

For each adapter, verify:

• Every field declared on the API-side request struct has a corresponding line in the adapter that copies it into the internal-side request OR explicitly documents why it's deliberately dropped (e.g. "system-managed field, can't be set via API").
• The store call uses the internal request, not the API request directly.
• A test exists that exercises EACH field of the API request and asserts it reaches the store (not just "HTTP 200 returned").

Why this matters

Silent field drops are the worst kind of API bug:

• No error surfaces — HTTP 200 returned, success toast shown
• DB state diverges from user intent silently
• Discovery is reactive ("user X complains they can't log in") rather than proactive (test failure, log alert, monitoring)

For admin-edit paths specifically, the impact is user lockout (LeanerCloud/cloud-commitments-cli#893's case) or privilege escalation if a Role field gets dropped (an admin promoting a user to admin via the UI thinks it worked, but the user stays at the lower role).

What to do

1. Enumerate every API*Request → *Request (or similar) adapter site via the grep above.

2. For each, check the field set on both sides.

3. For each mismatch, decide:

   • Real bug: API has a field the adapter drops + the user-visible UI suggests the field IS editable → fix.
   • Documented gap: API has a field but it's deliberately read-only or system-managed → add a code comment + skip.
   • API has extra fields no one uses: remove the dead field from the API struct.

4. For each adapter, ensure a test exists that exercises every API-side field and asserts each reaches the store. Add the missing tests in the same audit PR (or a follow-up if the audit surfaces many gaps).

Acceptance criteria

• Audit report committed as docs/internal/api-adapter-audit.md (or similar) listing every adapter checked, the field-set on both sides, and the disposition (matches / fixed / documented gap).
• All real bugs fixed in the same PR as the audit OR split into per-adapter follow-up PRs if any single fix is non-trivial.
• Per-adapter regression tests added — minimum 1 test per adapter that asserts every API field round-trips to the store.

Suggested implementation pattern (to prevent future drift)

Rather than field-by-field copying in adapters, consider:

// Embed the API request inside the internal request, OR
// derive the internal request via a single struct-tag-driven mapper.
// E.g. with mapstructure:
internal := UpdateUserRequest{}
if err := mapstructure.Decode(apiReq, &internal); err != nil { ... }

Either approach makes the field-set mismatch a compile-time error (embedded struct) or a test-discoverable runtime issue (mapstructure tag missing). This is a separate design decision worth discussing in the audit PR — not required, but should be considered.

Related

• PR fix(auth/users): persist email change in admin update path (closes #892) cloud-commitments-cli#893 (the trigger bug — admin email edit silently dropped)
• Issue bug(api/users): admin edit of another user's email returns success but doesn't persist cloud-commitments-cli#892 (where fix(auth/users): persist email change in admin update path (closes #892) cloud-commitments-cli#893's investigation was filed)

Why this is P2 not P1

Each individual silent-field-drop is potentially P1 (LeanerCloud/cloud-commitments-cli#893 was), but the audit itself is P2 because the most likely paths (admin-edit user fields) are now covered by LeanerCloud/cloud-commitments-cli#893's regression test. The audit catches the next instance of the same class before it manifests in production. If the audit surfaces another currently-broken adapter, that one's fix may be P1 in its own right.