Skip to content

Commit aa75ee4

Browse files
ChronicLynxleibale
andauthored
#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 <[email protected]>
1 parent e895fa1 commit aa75ee4

File tree

5 files changed

+97
-78
lines changed

5 files changed

+97
-78
lines changed

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

Lines changed: 49 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { defineScript } from '../lua-script';
88
import { spy } from 'sinon';
99
import { once } from 'events';
1010
import { ClientKillFilters } from '../commands/CLIENT_KILL';
11+
import { promisify } from 'util';
1112

1213
export const SQUARE_SCRIPT = defineScript({
1314
SCRIPT: 'return ARGV[1] * ARGV[1];',
@@ -142,26 +143,9 @@ describe('Client', () => {
142143
});
143144

144145
describe('legacyMode', () => {
145-
function sendCommandAsync<
146-
M extends RedisModules,
147-
F extends RedisFunctions,
148-
S extends RedisScripts
149-
>(
150-
client: RedisClientType<M, F, S>,
151-
args: RedisCommandArguments
152-
): Promise<RedisCommandRawReply> {
153-
return new Promise((resolve, reject) => {
154-
(client as any).sendCommand(args, (err: Error | undefined, reply: RedisCommandRawReply) => {
155-
if (err) return reject(err);
156-
157-
resolve(reply);
158-
});
159-
});
160-
}
161-
162146
testUtils.testWithClient('client.sendCommand should call the callback', async client => {
163147
assert.equal(
164-
await sendCommandAsync(client, ['PING']),
148+
await promisify(client.sendCommand).call(client, 'PING'),
165149
'PONG'
166150
);
167151
}, {
@@ -193,26 +177,9 @@ describe('Client', () => {
193177
}
194178
});
195179

196-
function setAsync<
197-
M extends RedisModules,
198-
F extends RedisFunctions,
199-
S extends RedisScripts
200-
>(
201-
client: RedisClientType<M, F, S>,
202-
...args: Array<any>
203-
): Promise<RedisCommandRawReply> {
204-
return new Promise((resolve, reject) => {
205-
(client as any).set(...args, (err: Error | undefined, reply: RedisCommandRawReply) => {
206-
if (err) return reject(err);
207-
208-
resolve(reply);
209-
});
210-
});
211-
}
212-
213180
testUtils.testWithClient('client.{command} should accept vardict arguments', async client => {
214181
assert.equal(
215-
await setAsync(client, 'a', 'b'),
182+
await promisify(client.set).call(client, 'a', 'b'),
216183
'OK'
217184
);
218185
}, {
@@ -224,7 +191,7 @@ describe('Client', () => {
224191

225192
testUtils.testWithClient('client.{command} should accept arguments array', async client => {
226193
assert.equal(
227-
await setAsync(client, ['a', 'b']),
194+
await promisify(client.set).call(client, ['a', 'b']),
228195
'OK'
229196
);
230197
}, {
@@ -236,7 +203,7 @@ describe('Client', () => {
236203

237204
testUtils.testWithClient('client.{command} should accept mix of arrays and arguments', async client => {
238205
assert.equal(
239-
await setAsync(client, ['a'], 'b', ['EX', 1]),
206+
await promisify(client.set).call(client, ['a'], 'b', ['EX', 1]),
240207
'OK'
241208
);
242209
}, {
@@ -246,6 +213,26 @@ describe('Client', () => {
246213
}
247214
});
248215

216+
testUtils.testWithClient('client.hGetAll should return object', async client => {
217+
await client.v4.hSet('key', 'field', 'value');
218+
219+
assert.deepEqual(
220+
await promisify(client.hGetAll).call(client, 'key'),
221+
Object.create(null, {
222+
field: {
223+
value: 'value',
224+
configurable: true,
225+
enumerable: true
226+
}
227+
})
228+
);
229+
}, {
230+
...GLOBAL.SERVERS.OPEN,
231+
clientOptions: {
232+
legacyMode: true
233+
}
234+
});
235+
249236
function multiExecAsync<
250237
M extends RedisModules,
251238
F extends RedisFunctions,
@@ -330,6 +317,30 @@ describe('Client', () => {
330317
}
331318
});
332319

320+
testUtils.testWithClient('client.multi.hGetAll should return object', async client => {
321+
assert.deepEqual(
322+
await multiExecAsync(
323+
client.multi()
324+
.hSet('key', 'field', 'value')
325+
.hGetAll('key')
326+
),
327+
[
328+
1,
329+
Object.create(null, {
330+
field: {
331+
value: 'value',
332+
configurable: true,
333+
enumerable: true
334+
}
335+
})
336+
]
337+
);
338+
}, {
339+
...GLOBAL.SERVERS.OPEN,
340+
clientOptions: {
341+
legacyMode: true
342+
}
343+
});
333344
});
334345

335346
describe('events', () => {

packages/client/lib/client/index.ts

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import COMMANDS from './commands';
2-
import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisCommandReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, RedisCommandSignature, ConvertArgumentType, RedisFunction, ExcludeMappedString } from '../commands';
2+
import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisCommandReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, RedisCommandSignature, ConvertArgumentType, RedisFunction, ExcludeMappedString, RedisCommands } from '../commands';
33
import RedisSocket, { RedisSocketOptions, RedisTlsSocketOptions } from './socket';
44
import RedisCommandsQueue, { PubSubListener, PubSubSubscribeCommands, PubSubUnsubscribeCommands, QueueCommandOptions } from './commands-queue';
55
import RedisClientMultiCommand, { RedisClientMultiCommandType } from './multi-command';
@@ -300,34 +300,14 @@ export default class RedisClient<
300300

301301
(this as any).#v4.sendCommand = this.#sendCommand.bind(this);
302302
(this as any).sendCommand = (...args: Array<any>): void => {
303-
let callback: ClientLegacyCallback;
304-
if (typeof args[args.length - 1] === 'function') {
305-
callback = args.pop() as ClientLegacyCallback;
303+
const result = this.#legacySendCommand(...args);
304+
if (result) {
305+
result.promise.then(reply => result.callback(null, reply));
306306
}
307-
308-
this.#sendCommand(transformLegacyCommandArguments(args))
309-
.then((reply: RedisCommandRawReply) => {
310-
if (!callback) return;
311-
312-
// https://github.com/NodeRedis/node-redis#commands:~:text=minimal%20parsing
313-
314-
callback(null, reply);
315-
})
316-
.catch((err: Error) => {
317-
if (!callback) {
318-
this.emit('error', err);
319-
return;
320-
}
321-
322-
callback(err);
323-
});
324307
};
325308

326-
for (const name of Object.keys(COMMANDS)) {
327-
this.#defineLegacyCommand(name);
328-
}
329-
330-
for (const name of Object.keys(COMMANDS)) {
309+
for (const [ name, command ] of Object.entries(COMMANDS as RedisCommands)) {
310+
this.#defineLegacyCommand(name, command);
331311
(this as any)[name.toLowerCase()] = (this as any)[name];
332312
}
333313

@@ -346,10 +326,31 @@ export default class RedisClient<
346326
this.#defineLegacyCommand('quit');
347327
}
348328

349-
#defineLegacyCommand(name: string): void {
350-
this.#v4[name] = (this as any)[name].bind(this);
351-
(this as any)[name] =
352-
(...args: Array<unknown>): void => (this as any).sendCommand(name, ...args);
329+
#legacySendCommand(...args: Array<any>) {
330+
const callback = typeof args[args.length - 1] === 'function' ?
331+
args.pop() as ClientLegacyCallback :
332+
undefined;
333+
334+
const promise = this.#sendCommand(transformLegacyCommandArguments(args));
335+
if (callback) return {
336+
promise,
337+
callback
338+
};
339+
promise.catch(err => this.emit('error', err));
340+
}
341+
342+
#defineLegacyCommand(this: any, name: string, command?: RedisCommand): void {
343+
this.#v4[name] = this[name].bind(this);
344+
this[name] = command && command.TRANSFORM_LEGACY_REPLY && command.transformReply ?
345+
(...args: Array<unknown>) => {
346+
const result = this.#legacySendCommand(name, ...args);
347+
if (result) {
348+
result.promise.then((reply: any) => {
349+
result.callback(null, command.transformReply!(reply));
350+
});
351+
}
352+
} :
353+
(...args: Array<unknown>) => this.sendCommand(name, ...args);
353354
}
354355

355356
#pingTimer?: NodeJS.Timer;

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

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import COMMANDS from './commands';
2-
import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, ExcludeMappedString, RedisFunction } from '../commands';
2+
import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisFunctions, RedisModules, RedisExtensions, RedisScript, RedisScripts, ExcludeMappedString, RedisFunction, RedisCommands } from '../commands';
33
import RedisMultiCommand, { RedisMultiQueuedCommand } from '../multi-command';
44
import { attachCommands, attachExtensions, transformLegacyCommandArguments } from '../commander';
55

@@ -117,19 +117,23 @@ export default class RedisClientMultiCommand {
117117
});
118118
};
119119

120-
for (const name of Object.keys(COMMANDS)) {
121-
this.#defineLegacyCommand(name);
122-
}
123-
124-
for (const name of Object.keys(COMMANDS)) {
120+
for (const [ name, command ] of Object.entries(COMMANDS as RedisCommands)) {
121+
this.#defineLegacyCommand(name, command);
125122
(this as any)[name.toLowerCase()] = (this as any)[name];
126123
}
127124
}
128125

129-
#defineLegacyCommand(name: string): void {
130-
this.v4[name] = (this as any)[name].bind(this.v4);
131-
(this as any)[name] =
132-
(...args: Array<unknown>): void => (this as any).addCommand(name, ...args);
126+
#defineLegacyCommand(this: any, name: string, command?: RedisCommand): void {
127+
this.v4[name] = this[name].bind(this.v4);
128+
this[name] = command && command.TRANSFORM_LEGACY_REPLY && command.transformReply ?
129+
(...args: Array<unknown>) => {
130+
this.#multi.addCommand(
131+
[name, ...transformLegacyCommandArguments(args)],
132+
command.transformReply
133+
);
134+
return this;
135+
} :
136+
(...args: Array<unknown>) => this.addCommand(name, ...args);
133137
}
134138

135139
commandsExecutor(command: RedisCommand, args: Array<unknown>): this {

packages/client/lib/commands/HGETALL.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ export const FIRST_KEY_INDEX = 1;
44

55
export const IS_READ_ONLY = true;
66

7+
export const TRANSFORM_LEGACY_REPLY = true;
8+
79
export function transformArguments(key: RedisCommandArgument): RedisCommandArguments {
810
return ['HGETALL', key];
911
}

packages/client/lib/commands/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export type RedisCommandArguments = Array<RedisCommandArgument> & { preserve?: u
1111
export interface RedisCommand {
1212
FIRST_KEY_INDEX?: number | ((...args: Array<any>) => RedisCommandArgument | undefined);
1313
IS_READ_ONLY?: boolean;
14+
TRANSFORM_LEGACY_REPLY?: boolean;
1415
transformArguments(this: void, ...args: Array<any>): RedisCommandArguments;
1516
transformReply?(this: void, reply: any, preserved?: any): any;
1617
}

0 commit comments

Comments
 (0)