[DX-1101] Fix keys revoke to use correct Control API endpoint#335
[DX-1101] Fix keys revoke to use correct Control API endpoint#335
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR fixes a bug where the Changes
Review Notes
|
There was a problem hiding this comment.
Review Summary
This PR fixes a real bug: revokeKey was calling DELETE /apps/{appId}/keys/{keyId} when the Control API spec requires POST /apps/{appId}/keys/{keyId}/revoke. The endpoint fix and test updates are correct.
One real gap to address:
The empty-body fix is untested. The PR description says the revoke endpoint returns 200 OK with an empty body, and the fix handles that case in request(). But all three test files mock with reply(200, {}) which sends non-empty JSON. The new empty-body branch (if (!text) return {} as T) is never exercised by any test.
If someone later changed the empty-body guard, the existing tests would still pass, masking the regression. To fix: add a success test in control-api.test.ts that uses reply(200, '') to simulate the real API behavior, and update the command-level mocks from reply(200, {}) to reply(200, '') as well.
Minor robustness note (non-blocking): if (!text) does not guard against whitespace-only bodies. if (!text.trim()) is a safer guard, though in practice the Control API returns empty string not whitespace.
Everything else looks good:
- Endpoint is correct per spec (POST .../revoke)
- response.text() then JSON.parse is the right pattern for handling optional response bodies
- The 204 early-return is preserved and unaffected
- All three test files consistently updated
- revoke.ts command logic did not need to change
revokeKeyto usePOST /apps/{app_id}/keys/{key_id}/revokeinstead ofDELETE /apps/{app_id}/keys/{key_id}, matching the actual Control API spechead :ok)