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

* Ensure that transformReply is optionally passed through to commands in legacy mode within multi

* Execute transformReply on legacy #sendCommand

* Scope transform changes to hGetAll

* Extensible method of transforming legacy replies, expands RedisCommand interface

* check `TRANSFORM_LEGACY_REPLY` on client creation (rather then on command exec), add tests

Co-authored-by: Leibale Eidelman <me@leibale.com>

Author: ChronicLynx

packages/client/lib/client/index.spec.ts

@@ -8,6 +8,7 @@ import { defineScript } from '../lua-script';
 import { spy } from 'sinon';
 import { once } from 'events';
 import { ClientKillFilters } from '../commands/CLIENT_KILL';
+import { promisify } from 'util';
 
 export const SQUARE_SCRIPT = defineScript({
     SCRIPT: 'return ARGV[1] * ARGV[1];',
@@ -142,26 +143,9 @@ describe('Client', () => {
     });
 
     describe('legacyMode', () => {
-        function sendCommandAsync<
-            M extends RedisModules,
-            F extends RedisFunctions,
-            S extends RedisScripts
-        >(
-            client: RedisClientType<M, F, S>,
-            args: RedisCommandArguments
-        ): Promise<RedisCommandRawReply> {
-            return new Promise((resolve, reject) => {
-                (client as any).sendCommand(args, (err: Error | undefined, reply: RedisCommandRawReply) => {
-                    if (err) return reject(err);
-
-                    resolve(reply);
-                });
-            });
-        }
-
         testUtils.testWithClient('client.sendCommand should call the callback', async client => {
             assert.equal(
-                await sendCommandAsync(client, ['PING']),
+                await promisify(client.sendCommand).call(client, 'PING'),
                 'PONG'
             );
         }, {
@@ -193,26 +177,9 @@ describe('Client', () => {
             }
         });
 
-        function setAsync<
-            M extends RedisModules,
-            F extends RedisFunctions,
-            S extends RedisScripts
-        >(
-            client: RedisClientType<M, F, S>,
-            ...args: Array<any>
-        ): Promise<RedisCommandRawReply> {
-            return new Promise((resolve, reject) => {
-                (client as any).set(...args, (err: Error | undefined, reply: RedisCommandRawReply) => {
-                    if (err) return reject(err);
-
-                    resolve(reply);
-                });
-            });
-        }
-
         testUtils.testWithClient('client.{command} should accept vardict arguments', async client => {
             assert.equal(
-                await setAsync(client, 'a', 'b'),
+                await promisify(client.set).call(client, 'a', 'b'),
                 'OK'
             );
         }, {
@@ -224,7 +191,7 @@ describe('Client', () => {
 
         testUtils.testWithClient('client.{command} should accept arguments array', async client => {
             assert.equal(
-                await setAsync(client, ['a', 'b']),
+                await promisify(client.set).call(client, ['a', 'b']),
                 'OK'
             );
         }, {
@@ -236,7 +203,7 @@ describe('Client', () => {
 
         testUtils.testWithClient('client.{command} should accept mix of arrays and arguments', async client => {
             assert.equal(
-                await setAsync(client, ['a'], 'b', ['EX', 1]),
+                await promisify(client.set).call(client, ['a'], 'b', ['EX', 1]),
                 'OK'
             );
         }, {
@@ -246,6 +213,26 @@ describe('Client', () => {
             }
         });
 
+        testUtils.testWithClient('client.hGetAll should return object', async client => {
+            await client.v4.hSet('key', 'field', 'value');
+            
+            assert.deepEqual(
+                await promisify(client.hGetAll).call(client, 'key'),
+                Object.create(null, {
+                    field: {
+                        value: 'value',
+                        configurable: true,
+                        enumerable: true
+                    }
+                })
+            );
+        }, {
+            ...GLOBAL.SERVERS.OPEN,
+            clientOptions: {
+                legacyMode: true
+            }
+        });
+
         function multiExecAsync<
             M extends RedisModules,
             F extends RedisFunctions,
@@ -330,6 +317,30 @@ describe('Client', () => {
             }
         });
 
+        testUtils.testWithClient('client.multi.hGetAll should return object', async client => { 
+            assert.deepEqual(
+                await multiExecAsync(
+                    client.multi()
+                        .hSet('key', 'field', 'value')
+                        .hGetAll('key')
+                ),
+                [
+                    1,
+                    Object.create(null, {
+                        field: {
+                            value: 'value',
+                            configurable: true,
+                            enumerable: true
+                        }
+                    })
+                ]
+            );
+        }, {
+            ...GLOBAL.SERVERS.OPEN,
+            clientOptions: {
+                legacyMode: true
+            }
+        });
     });
 
     describe('events', () => {

packages/client/lib/client/index.ts

@@ -1,5 +1,5 @@
 import COMMANDS from './commands';
-import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisCommandReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, RedisCommandSignature, ConvertArgumentType, RedisFunction, ExcludeMappedString } from '../commands';
+import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisCommandReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, RedisCommandSignature, ConvertArgumentType, RedisFunction, ExcludeMappedString, RedisCommands } from '../commands';
 import RedisSocket, { RedisSocketOptions, RedisTlsSocketOptions } from './socket';
 import RedisCommandsQueue, { PubSubListener, PubSubSubscribeCommands, PubSubUnsubscribeCommands, QueueCommandOptions } from './commands-queue';
 import RedisClientMultiCommand, { RedisClientMultiCommandType } from './multi-command';
@@ -300,34 +300,14 @@ export default class RedisClient<
 
         (this as any).#v4.sendCommand = this.#sendCommand.bind(this);
         (this as any).sendCommand = (...args: Array<any>): void => {
-            let callback: ClientLegacyCallback;
-            if (typeof args[args.length - 1] === 'function') {
-                callback = args.pop() as ClientLegacyCallback;
+            const result = this.#legacySendCommand(...args);
+            if (result) {
+                result.promise.then(reply => result.callback(null, reply));
             }
-
-            this.#sendCommand(transformLegacyCommandArguments(args))
-                .then((reply: RedisCommandRawReply) => {
-                    if (!callback) return;
-
-                    // https://github.com/NodeRedis/node-redis#commands:~:text=minimal%20parsing
-
-                    callback(null, reply);
-                })
-                .catch((err: Error) => {
-                    if (!callback) {
-                        this.emit('error', err);
-                        return;
-                    }
-
-                    callback(err);
-                });
         };
 
-        for (const name of Object.keys(COMMANDS)) {
-            this.#defineLegacyCommand(name);
-        }
-
-        for (const name of Object.keys(COMMANDS)) {
+        for (const [ name, command ] of Object.entries(COMMANDS as RedisCommands)) {
+            this.#defineLegacyCommand(name, command);
             (this as any)[name.toLowerCase()] = (this as any)[name];
         }
 
@@ -346,10 +326,31 @@ export default class RedisClient<
         this.#defineLegacyCommand('quit');
     }
 
-    #defineLegacyCommand(name: string): void {
-        this.#v4[name] = (this as any)[name].bind(this);
-        (this as any)[name] =
-            (...args: Array<unknown>): void => (this as any).sendCommand(name, ...args);
+    #legacySendCommand(...args: Array<any>) {
+        const callback = typeof args[args.length - 1] === 'function' ?
+            args.pop() as ClientLegacyCallback :
+            undefined;
+
+        const promise = this.#sendCommand(transformLegacyCommandArguments(args));
+        if (callback) return {
+            promise,
+            callback
+        };
+        promise.catch(err => this.emit('error', err));
+    }
+
+    #defineLegacyCommand(this: any, name: string, command?: RedisCommand): void {
+        this.#v4[name] = this[name].bind(this);
+        this[name] = command && command.TRANSFORM_LEGACY_REPLY && command.transformReply ?
+            (...args: Array<unknown>) => {
+                const result = this.#legacySendCommand(name, ...args);
+                if (result) {
+                    result.promise.then((reply: any) => {
+                        result.callback(null, command.transformReply!(reply));
+                    });
+                }
+            } :
+            (...args: Array<unknown>) => this.sendCommand(name, ...args);
     }
 
     #pingTimer?: NodeJS.Timer;

packages/client/lib/client/multi-command.ts

@@ -1,5 +1,5 @@
 import COMMANDS from './commands';
-import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, ExcludeMappedString, RedisFunction } from '../commands';
+import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, ExcludeMappedString, RedisFunction, RedisCommands } from '../commands';
 import RedisMultiCommand, { RedisMultiQueuedCommand } from '../multi-command';
 import { attachCommands, attachExtensions, transformLegacyCommandArguments } from '../commander';
 
@@ -117,19 +117,23 @@ export default class RedisClientMultiCommand {
                 });
         };
 
-        for (const name of Object.keys(COMMANDS)) {
-            this.#defineLegacyCommand(name);
-        }
-
-        for (const name of Object.keys(COMMANDS)) {
+        for (const [ name, command ] of Object.entries(COMMANDS as RedisCommands)) {
+            this.#defineLegacyCommand(name, command);
             (this as any)[name.toLowerCase()] = (this as any)[name];
         }
     }
 
-    #defineLegacyCommand(name: string): void {
-        this.v4[name] = (this as any)[name].bind(this.v4);
-        (this as any)[name] =
-            (...args: Array<unknown>): void => (this as any).addCommand(name, ...args);
+    #defineLegacyCommand(this: any, name: string, command?: RedisCommand): void {
+        this.v4[name] = this[name].bind(this.v4);
+        this[name] = command && command.TRANSFORM_LEGACY_REPLY && command.transformReply ?
+            (...args: Array<unknown>) => {
+                this.#multi.addCommand(
+                    [name, ...transformLegacyCommandArguments(args)],
+                    command.transformReply
+                );
+                return this;
+            } :
+            (...args: Array<unknown>) => this.addCommand(name, ...args);
     }
 
     commandsExecutor(command: RedisCommand, args: Array<unknown>): this {

packages/client/lib/commands/HGETALL.ts

@@ -4,6 +4,8 @@ export const FIRST_KEY_INDEX = 1;
 
 export const IS_READ_ONLY = true;
 
+export const TRANSFORM_LEGACY_REPLY = true;
+
 export function transformArguments(key: RedisCommandArgument): RedisCommandArguments {
     return ['HGETALL', key];
 }

packages/client/lib/commands/index.ts

@@ -11,6 +11,7 @@ export type RedisCommandArguments = Array<RedisCommandArgument> & { preserve?: u
 export interface RedisCommand {
     FIRST_KEY_INDEX?: number | ((...args: Array<any>) => RedisCommandArgument | undefined);
     IS_READ_ONLY?: boolean;
+    TRANSFORM_LEGACY_REPLY?: boolean;
     transformArguments(this: void, ...args: Array<any>): RedisCommandArguments;
     transformReply?(this: void, reply: any, preserved?: any): any;
 }