From 9519004486b8d700fa24d7c9be989a044207d826 Mon Sep 17 00:00:00 2001 From: Leibale Date: Tue, 5 Dec 2023 12:39:46 -0500 Subject: [PATCH 1/5] fix #2665 - handle errors in multi/pipeline replies --- packages/client/lib/client/index.spec.ts | 22 +++++++++++++++++++++- packages/client/lib/errors.ts | 11 +++++++++++ packages/client/lib/multi-command.ts | 17 ++++++++++++----- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index 3278d27775b..f7a7942fae2 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -3,7 +3,7 @@ import testUtils, { GLOBAL, waitTillBeenCalled } from '../test-utils'; import RedisClient, { RedisClientType } from '.'; import { RedisClientMultiCommandType } from './multi-command'; import { RedisCommandRawReply, RedisModules, RedisFunctions, RedisScripts } from '../commands'; -import { AbortError, ClientClosedError, ClientOfflineError, ConnectionTimeoutError, DisconnectsClientError, SocketClosedUnexpectedlyError, WatchError } from '../errors'; +import { AbortError, ClientClosedError, ClientOfflineError, ConnectionTimeoutError, DisconnectsClientError, ErrorReply, MultiErrorReply, SocketClosedUnexpectedlyError, WatchError } from '../errors'; import { defineScript } from '../lua-script'; import { spy } from 'sinon'; import { once } from 'events'; @@ -602,6 +602,26 @@ describe('Client', () => { ...GLOBAL.SERVERS.OPEN, minimumDockerVersion: [6, 2] // CLIENT INFO }); + + testUtils.testWithClient('should handle error replies (#2665)', async client => { + await assert.rejects( + client.multi() + .set('key', 'value') + .hGetAll('key') + .exec(), + err => { + console.log(err); + assert.ok(err instanceof MultiErrorReply); + assert.equal(err.replies.length, 2); + assert.deepEqual(err.errorIndexes, [1]); + assert.ok(err.replies[1] instanceof ErrorReply); + return true; + } + ); + }, { + ...GLOBAL.SERVERS.OPEN, + minimumDockerVersion: [6, 2] // CLIENT INFO + }); }); testUtils.testWithClient('scripts', async client => { diff --git a/packages/client/lib/errors.ts b/packages/client/lib/errors.ts index 30709703153..19bed356e07 100644 --- a/packages/client/lib/errors.ts +++ b/packages/client/lib/errors.ts @@ -63,3 +63,14 @@ export class ErrorReply extends Error { this.stack = undefined; } } + +export class MultiErrorReply extends ErrorReply { + replies: Array; + errorIndexes: Array; + + constructor(replies: Array, errorIndexes: Array) { + super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`); + this.replies = replies; + this.errorIndexes = errorIndexes; + } +} diff --git a/packages/client/lib/multi-command.ts b/packages/client/lib/multi-command.ts index 08f23ffa454..77f3d45d3cb 100644 --- a/packages/client/lib/multi-command.ts +++ b/packages/client/lib/multi-command.ts @@ -1,6 +1,6 @@ import { fCallArguments } from './commander'; import { RedisCommand, RedisCommandArguments, RedisCommandRawReply, RedisFunction, RedisScript } from './commands'; -import { WatchError } from './errors'; +import { ErrorReply, MultiErrorReply, WatchError } from './errors'; export interface RedisMultiQueuedCommand { args: RedisCommandArguments; @@ -79,9 +79,16 @@ export default class RedisMultiCommand { } transformReplies(rawReplies: Array): Array { - return rawReplies.map((reply, i) => { - const { transformReply, args } = this.queue[i]; - return transformReply ? transformReply(reply, args.preserve) : reply; - }); + const errorIndexes: Array = [], + replies = rawReplies.map((reply, i) => { + if (reply instanceof ErrorReply) { + errorIndexes.push(i); + } + const { transformReply, args } = this.queue[i]; + return transformReply ? transformReply(reply, args.preserve) : reply; + }); + + if (errorIndexes.length) throw new MultiErrorReply(replies, errorIndexes); + return replies; } } From eca318fd60ebf69fcb1a4ef05e3de66554c13d74 Mon Sep 17 00:00:00 2001 From: Leibale Date: Tue, 5 Dec 2023 12:45:04 -0500 Subject: [PATCH 2/5] fix MultiErrorReply replies type --- packages/client/lib/errors.ts | 8 +++++--- packages/client/lib/multi-command.ts | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/client/lib/errors.ts b/packages/client/lib/errors.ts index 19bed356e07..7afcc5b3f8f 100644 --- a/packages/client/lib/errors.ts +++ b/packages/client/lib/errors.ts @@ -1,3 +1,5 @@ +import { RedisCommandRawReply } from './commands'; + export class AbortError extends Error { constructor() { super('The command was aborted'); @@ -65,10 +67,10 @@ export class ErrorReply extends Error { } export class MultiErrorReply extends ErrorReply { - replies: Array; - errorIndexes: Array; + replies; + errorIndexes; - constructor(replies: Array, errorIndexes: Array) { + constructor(replies: Array, errorIndexes: Array) { super(`${errorIndexes.length} commands failed, see .replies and .errorIndexes for more information`); this.replies = replies; this.errorIndexes = errorIndexes; diff --git a/packages/client/lib/multi-command.ts b/packages/client/lib/multi-command.ts index 77f3d45d3cb..89b934b192c 100644 --- a/packages/client/lib/multi-command.ts +++ b/packages/client/lib/multi-command.ts @@ -69,7 +69,7 @@ export default class RedisMultiCommand { return transformedArguments; } - handleExecReplies(rawReplies: Array): Array { + handleExecReplies(rawReplies: Array): Array { const execReply = rawReplies[rawReplies.length - 1] as (null | Array); if (execReply === null) { throw new WatchError(); @@ -78,7 +78,7 @@ export default class RedisMultiCommand { return this.transformReplies(execReply); } - transformReplies(rawReplies: Array): Array { + transformReplies(rawReplies: Array): Array { const errorIndexes: Array = [], replies = rawReplies.map((reply, i) => { if (reply instanceof ErrorReply) { From f3b3fe536de74c6f02af65b742007b9e998f0c5a Mon Sep 17 00:00:00 2001 From: Leibale Date: Tue, 5 Dec 2023 13:02:29 -0500 Subject: [PATCH 3/5] run tests on all versions, remove console.log, fix bug --- packages/client/lib/client/index.spec.ts | 6 +----- packages/client/lib/multi-command.ts | 1 + 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index f7a7942fae2..38225bf4f1b 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -610,7 +610,6 @@ describe('Client', () => { .hGetAll('key') .exec(), err => { - console.log(err); assert.ok(err instanceof MultiErrorReply); assert.equal(err.replies.length, 2); assert.deepEqual(err.errorIndexes, [1]); @@ -618,10 +617,7 @@ describe('Client', () => { return true; } ); - }, { - ...GLOBAL.SERVERS.OPEN, - minimumDockerVersion: [6, 2] // CLIENT INFO - }); + }, GLOBAL.SERVERS.OPEN); }); testUtils.testWithClient('scripts', async client => { diff --git a/packages/client/lib/multi-command.ts b/packages/client/lib/multi-command.ts index 89b934b192c..642c2ea36c0 100644 --- a/packages/client/lib/multi-command.ts +++ b/packages/client/lib/multi-command.ts @@ -83,6 +83,7 @@ export default class RedisMultiCommand { replies = rawReplies.map((reply, i) => { if (reply instanceof ErrorReply) { errorIndexes.push(i); + return reply; } const { transformReply, args } = this.queue[i]; return transformReply ? transformReply(reply, args.preserve) : reply; From d04dd3793041e8795f7006eb4be515bd548d61d2 Mon Sep 17 00:00:00 2001 From: Leibale Eidelman Date: Tue, 5 Dec 2023 13:33:47 -0500 Subject: [PATCH 4/5] add errors iterator helper --- packages/client/lib/errors.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/client/lib/errors.ts b/packages/client/lib/errors.ts index 7afcc5b3f8f..aa97d9cf26d 100644 --- a/packages/client/lib/errors.ts +++ b/packages/client/lib/errors.ts @@ -75,4 +75,10 @@ export class MultiErrorReply extends ErrorReply { this.replies = replies; this.errorIndexes = errorIndexes; } + + *errors() { + for (const index of this.errorIndexes) { + yield this.replies[index]; + } + } } From e9d919ef7e78ea63caf470b51bb673766935dde9 Mon Sep 17 00:00:00 2001 From: Leibale Eidelman Date: Tue, 12 Dec 2023 16:19:12 -0500 Subject: [PATCH 5/5] test `.errors()` as well --- packages/client/lib/client/index.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index 38225bf4f1b..4442d3adb83 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -614,6 +614,7 @@ describe('Client', () => { assert.equal(err.replies.length, 2); assert.deepEqual(err.errorIndexes, [1]); assert.ok(err.replies[1] instanceof ErrorReply); + assert.deepEqual([...err.errors()], [err.replies[1]]); return true; } );