#2038 Resolve legacy mode hGetAll returning in the wrong format compared to v3 results

[ChronicLynx]

Description

Resolves Issue #2038

Noteworthy:

• In V3 empty replies came back as null where in V4 empty replies are delivered as an empty object ({}) or array([])
• I did not fix empty reply behavior pertaining to legacyMode, perhaps it warrants more investigation but is outside the scope of 2038

Left Side V4 vs Right Side V3

---

During a migration to version 4 our team encountered an issue regarding legacy mode return formatting.

multi.hGetAll was returning as a tuple instead of an object. Prior to upgrading and enabling legacyMode it was returning the value as an object, and the v4 version also returns the reply as an object. Thanks to our joi data validation we were able to catch the mismatch and investigate.

I believe this is because the legacyMode addCommand for non-v4 currently calls
this.#multi.addCommand(transformLegacyCommandArguments(args)); and omits the transformReply which is exported by the various commands and seemingly needed for hGetAll to match the v3 formatting.

I've added an optional boolean to RedisCommands needing this transform (hGetAll for now). As COMMANDS is lacking typing I cast it as any to avoid errors when accessing the object using the command name string.

With this fix in place the legacy mode multi.hGetAll is now returning an object as expected.

Additional Work

#2038 points out that this issue may also spread to the sendCommand method which takes a very similar approach to legacy splitting as the addCommand method did.

I've also applied the transform in this case, which formats the reply as an object instead of a tuple, mimicking v3 behavior.

Important:

By applying transformReply to everything (as a test case) I've noticed odd behavior. hGetAll with transformReply applied matches the old v3 behavior. Something like client.scan returned a tuple originally but a transformReply causes it to incorrectly return as an object.

Since hGetAll is the only one known to have an issue I've limited the scope to the hGetAll / HGETALL command. I believe applying the transform to everything is dangerous, as it appears this reply type mismatch either only happens to hGetAll or a small subset of commands that have yet to be discovered.

Testing Issues

I was able to run this through my own project test suite and the issue appears to be resolved.

I've partially figured out how to run the official test suite locally but still having some trouble with some Cluster tests... perhaps I'm not setting it up correctly.

npm run test -w ./packages/client -- --redis-version 7.0

Local Test Run (exact same results as master test run):
1119 passing
2 pending
9 failing

---

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]

@ChronicLynx thanks for contributing! I made some changes to your code and added some tests, wanna review it?

[codecov-commenter]

Codecov Report

Base: 95.85% // Head: 95.90% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (6971453) compared to base (c5b6f77).
Patch coverage: 95.83% of modified lines in pull request are covered.

❗ Current head 6971453 differs from pull request most recent head e363469. Consider uploading reports for the commit e363469 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2367      +/-   ##
==========================================
+ Coverage   95.85%   95.90%   +0.05%     
==========================================
  Files         449      449              
  Lines        4224     4227       +3     
  Branches      471      474       +3     
==========================================
+ Hits         4049     4054       +5     
+ Misses        107      103       -4     
- Partials       68       70       +2     

Impacted Files	Coverage Δ
packages/client/lib/client/index.ts	91.96% <93.75%> (+0.83%)	⬆️
packages/client/lib/client/multi-command.ts	88.46% <100.00%> (+0.22%)	⬆️
packages/client/lib/commands/HGETALL.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ChronicLynx]

@leibale This looks excellent, great addition to the preceding code! Your deep knowledge of the codebase really shines through with how you were able to expand upon and improve the initial concept.

[ChronicLynx]

@leibale I updated this to correct the git author information for my commits. You'll just have to kick off the tests again. Sorry about that! Had it set wrong locally on my machine.