Repository navigation
fix(client): type ACL GETUSER flags as RESP3 SET - #3417
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 009c10b47f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| type AclUser = TuplesToMapReply<[ | ||
| [BlobStringReply<'flags'>, ArrayReply<BlobStringReply>], | ||
| [BlobStringReply<'flags'>, SetReply<BlobStringReply>], |
There was a problem hiding this comment.
Keep RESP2 flags typed as an array
When a RESP2 client uses .withTypeMapping({ [RESP_TYPES.SET]: Set }), Resp2Reply<AclUser> preserves this nested value's new SET tag, so aclGetUser() now declares flags as Set<string>. The RESP2 server still sends an ARRAY and the decoder returns an array—the existing integration assertion confirms this—so consumers can compile Set-only operations that fail at runtime. Give the RESP2 transform an explicitly array-typed flags result rather than sharing the RESP3 marker.
AGENTS.md reference: AGENTS.md:L49-L51
Useful? React with 👍 / 👎.
|
@abhijeet117 Thanks for the PR. We’ll review it when we have capacity. |
|
Thanks for the PR — the type change is correct and brings ACL GETUSER One correction before we merge: the description/test wording says default RESP3 clients receive a runtime |
nkaradzhov
left a comment
There was a problem hiding this comment.
Requesting changes: correct the description/test wording — default RESP3 returns an Array, not a Set (the Set only appears under a { SET: Set } mapping). The type change itself is fine. Details in my comment above.
|
Thanks for fixing the wording. The null guard is correct too. This looks good to me. LGTM. |
Summary
ACL GETUSER declares flags as an array, but acl.c emits the flags element with setDeferredSetLen, so RESP3 replies carry a set that decodes to a plain Array unless callers opt in with a { [RESP_TYPES.SET]: Set } type mapping. Sibling commands already model this shape with SetReply (FUNCTION LIST, CLIENT TRACKINGINFO). flags is now SetReply: replies remain arrays by default, and clients that map RESP_TYPES.SET to Set see Set.
Testing
Compile-time regression test resolves the reply under a SET type mapping and asserts flags is Set; it fails on master with TS2740 and passes after the fix. Parser spec, type tests, build and lint run locally; Docker-backed integration tests run in CI.
Checklist
Note
Low Risk
Type-only fix for RESP3 reply typing plus a types-test; no runtime or wire behavior changes.
Overview
ACL GETUSER now types the
flagsfield asSetReply<BlobStringReply>instead ofArrayReply, matching the RESP3 set Redis emits on the wire and aligning with commands like FUNCTION LIST and CLIENT TRACKINGINFO.Default decoding behavior is unchanged (still arrays unless callers opt in). With
{ [RESP_TYPES.SET]: Set }mapping,flagscorrectly resolves toSet<string>.A compile-time regression test under
types-tests/asserts that mapping so the wrong array typing cannot return.Reviewed by Cursor Bugbot for commit 9d82984. Bugbot is set up for automated code reviews on this repo. Configure here.