Fix: XAUTOCLAIM after a TRIM with pending messages returns nil

[Calyhre]

Description

Running a XAUTOCLAIM after a TRIM while messages are pending results in Redis returning an array with nil as the unique element.

0.0.0.0:6379> XAUTOCLAIM stream group user 0 0-0 COUNT 1
1) "1689008209763-0"
2) 1) (nil)

The generic transformer will then crash here trying to iterate on null with TypeError: .for is not iterable

You can reproduce the issue with this:

const id = await client.XADD("stream", "*", { foo: "bar" });
await client.XREADGROUP("group", "consumer-1", { key: "stream", id: ">" });
await client.XTRIM("stream", "MAXLEN", 0);
await client.XAUTOCLAIM("stream", "group", "consumer-2", 0, id); // <- Will crash

---

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

[leibale]

@Calyhre Nice catch! but only XAUTOCLAIM can return null, not XCLAIM and XRANGE, so the change shouldn't be made in the transformStreamMessagesReply function (which all of them are using).
I've already fixed it in the v5 (see c9dae34), but I guess we need to backport it to v4 as well...

edit: actually XCLAIM might return null as well, but its not in the same format as XAUTOCLAIM. I'll take care of it later.

[Calyhre]

@leibale Nice! I also noticed the issue disappeared in Redis V7.

but only XAUTOCLAIM can return null, not XCLAIM and XRANGE, so the change shouldn't be made in the transformStreamMessagesReply function (which all of them are using).

I moved the fix to it's own method in xAutoClaim to not pollute the other methods. Let me know what you think.

[leibale]

@Calyhre actually both XCLAIM and XAUTOCLAIM might return null before redis 7, see here for more details and why it has changed.
I've fixed it in v5 (see 8369448), wanna backport it one last time? 🙏

[Calyhre]

@leibale Alright, I think I ended up porting a very similar fix to the one you did on V5. Let me know what you think.

I do have one doubt though: It seems right to me to reflect the differences between Redis versions (returning null or not based on Redis answer), but the end type is not version dependent. Using Redis V7, x[Auto]Claim messages will be typed with as StreamMessageReply | null even though null is not possible anymore in this case.

This might be complicated, but should the typing be dependent of the Redis server version?

[leibale]

@Calyhre Thanks for working on that! I'll review it next week :)

Regarding the differences between Redis versions - at the moment all the "command interfaces" are implemented with Redis 5-7.2 in mind. We might split the commands into their own sub-package at some point, then we'll be able to publish @redis/${redis-version}-commands or something like that... (BTW, we have the exact same problem with command arguments, for example, SET with NX and GET is only supported in Redis 7+, but we show it for all users regardless of their Redis version...)

edit: BTW, in v5 we're adding support for RESP3, so if we wanna support all the "current" Redis versions it'll be
Redis version: [5, 6, 6.2, 7, 7.2]
RESP: [2, 3]
which means 10 variations on all the commands...

[codecov-commenter]

Codecov Report

Patch coverage: 98.38% and project coverage change: +0.04% 🎉

Comparison is base (a7d5bc7) 95.70% compared to head (29bf1c0) 95.75%.
Report is 22 commits behind head on master.

❗ Current head 29bf1c0 differs from pull request most recent head 76d36f2. Consider uploading reports for the commit 76d36f2 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2565      +/-   ##
==========================================
+ Coverage   95.70%   95.75%   +0.04%     
==========================================
  Files         456      462       +6     
  Lines        4561     4567       +6     
  Branches      524      512      -12     
==========================================
+ Hits         4365     4373       +8     
  Misses        127      127              
+ Partials       69       67       -2     

Files Changed	Coverage Δ
packages/search/lib/commands/SEARCH.ts	82.60% <ø> (ø)
packages/client/lib/client/index.ts	92.74% <75.00%> (-0.84%)	⬇️
packages/client/lib/client/commands.ts	100.00% <100.00%> (ø)
packages/client/lib/cluster/commands.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/CLIENT_NO-TOUCH.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/CLUSTER_MYSHARDID.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/LATENCY_HISTORY.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/PUBSUB_SHARDNUMSUB.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/RESTORE.ts	100.00% <100.00%> (ø)
packages/client/lib/commands/XAUTOCLAIM.ts	100.00% <100.00%> (ø)
... and 4 more

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leibale]

@Calyhre [email protected]/@redis/[email protected] is on npm 🎉

[Calyhre]

Awesome thank you! And sorry for committing the hard coded redis test urls 🤦

[leibale]

@Calyhre I'll take PRs where all I need to do is remove "hard coded redis tests URLs" any day.. ;) Thank you!