address review comments

redis · Apr 3, 2024 · 657167c · 657167c
1 parent d96aacd
commit 657167c
Show file tree

Hide file tree

Showing 11 changed files with 96 additions and 87 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts
@@ -155,7 +155,8 @@ export default class RedisClient<
 
     return async function (this: ProxyClient, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
+
         command.parseCommand(parser, ...args);
 
         return this.executeCommand(parser, this._commandOptions, transformReply);
@@ -174,7 +175,7 @@ export default class RedisClient<
 
     return async function (this: NamespaceProxyClient, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         command.parseCommand(parser, ...args);
 
         return this._self.executeCommand(parser, this._self._commandOptions, transformReply);
@@ -194,7 +195,7 @@ export default class RedisClient<
 
     return async function (this: NamespaceProxyClient, ...args: Array<unknown>) {
       if (fn.parseCommand) {
-        const parser = this._self._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         fn.parseCommand(parser, ...args);
 
@@ -218,7 +219,7 @@ export default class RedisClient<
 
     return async function (this: ProxyClient, ...args: Array<unknown>) {
       if (script.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         script.parseCommand(parser, ...args);
 
@@ -344,10 +345,6 @@ export default class RedisClient<
     this._self.#dirtyWatch = msg;
   }
 
-  #newCommandParser(resp: RespVersions): CommandParser {
-    return new BasicCommandParser(resp);
-  }
-
   constructor(options?: RedisClientOptions<M, F, S, RESP, TYPE_MAPPING>) {
     super();
     this.#options = this.#initiateOptions(options);

diff --git a/packages/client/lib/client/multi-command.ts b/packages/client/lib/client/multi-command.ts
@@ -176,7 +176,8 @@ export default class RedisClientMultiCommand<REPLIES = []> {
         redisArgs = parser.redisArgs;
         redisArgs.preserve = parser.preserve;
       } else {
-        redisArgs = script.transformArguments(...args);
+        redisArgs = prefix;
+        redisArgs.push(...script.transformArguments(...args));
       }
 
       return this.addCommand(

diff --git a/packages/client/lib/client/pool.ts b/packages/client/lib/client/pool.ts
@@ -7,7 +7,7 @@ import { TimeoutError } from '../errors';
 import { attachConfig, functionArgumentsPrefix, getTransformReply, scriptArgumentsPrefix } from '../commander';
 import { CommandOptions } from './commands-queue';
 import RedisClientMultiCommand, { RedisClientMultiCommandType } from './multi-command';
-import { CommandParser, BasicCommandParser } from './parser';
+import { BasicCommandParser } from './parser';
 
 export interface RedisPoolOptions {
   /**
@@ -64,7 +64,7 @@ export class RedisClientPool<
 
     return async function (this: ProxyPool, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         command.parseCommand(parser, ...args);
 
         return this.execute(client => client.executeCommand(parser, this._commandOptions, transformReply))
@@ -83,7 +83,7 @@ export class RedisClientPool<
 
     return async function (this: NamespaceProxyPool, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         command.parseCommand(parser, ...args);
 
         return this._self.execute(client => client.executeCommand(parser, this._self._commandOptions, transformReply))
@@ -103,7 +103,7 @@ export class RedisClientPool<
 
     return async function (this: NamespaceProxyPool, ...args: Array<unknown>) {
       if (fn.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         fn.parseCommand(parser, ...args);
 
@@ -127,7 +127,7 @@ export class RedisClientPool<
 
     return async function (this: ProxyPool, ...args: Array<unknown>) {
       if (script.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         script.parseCommand(parser, ...args);
 
@@ -242,9 +242,6 @@ export class RedisClientPool<
     return this._self.#isClosing;
   }
 
-  #newCommandParser(resp: RespVersions): CommandParser {
-    return new BasicCommandParser(resp);
-  }
 
   /**
    * You are probably looking for {@link RedisClient.createPool `RedisClient.createPool`},

diff --git a/packages/client/lib/cluster/index.ts b/packages/client/lib/cluster/index.ts
@@ -10,7 +10,8 @@ import { PubSubListener } from '../client/pub-sub';
 import { ErrorReply } from '../errors';
 import { RedisTcpSocketOptions } from '../client/socket';
 import ASKING from '../commands/ASKING';
-import { BasicCommandParser, CommandParser } from '../client/parser';
+import { BasicCommandParser } from '../client/parser';
+import { parseArgs } from '../commands/generic-transformers';
 ;
 
 interface ClusterCommander<
@@ -175,7 +176,7 @@ export default class RedisCluster<
 
     return async function (this: ProxyCluster, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         command.parseCommand(parser, ...args);
 
         return this._self.#execute(
@@ -212,7 +213,7 @@ export default class RedisCluster<
 
     return async function (this: NamespaceProxyCluster, ...args: Array<unknown>) {
       if (command.parseCommand) {
-        const parser = this._self._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         command.parseCommand(parser, ...args);
 
         return this._self.#execute(
@@ -249,7 +250,7 @@ export default class RedisCluster<
 
     return async function (this: NamespaceProxyCluster, ...args: Array<unknown>) {
       if (fn.parseCommand) {
-        const parser = this._self._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         fn.parseCommand(parser, ...args);
 
@@ -288,7 +289,7 @@ export default class RedisCluster<
 
     return async function (this: ProxyCluster, ...args: Array<unknown>) {
       if (script.parseCommand) {
-        const parser = this._self.#newCommandParser(resp);
+        const parser = new BasicCommandParser(resp);
         parser.pushVariadic(prefix);
         script.parseCommand(parser, ...args);
 
@@ -409,10 +410,6 @@ export default class RedisCluster<
     return this._self.#slots.isOpen;
   }
 
-  #newCommandParser(resp: RespVersions): CommandParser {
-    return new BasicCommandParser(resp);
-  }
-
   constructor(options: RedisClusterOptions<M, F, S, RESP, TYPE_MAPPING/*, POLICIES*/>) {
     super();
 
@@ -495,25 +492,6 @@ export default class RedisCluster<
   //   return this._commandOptionsProxy('policies', policies);
   // }
 
-  #handleAsk<T>(
-    fn: (client: RedisClientType<M, F, S, RESP, TYPE_MAPPING>, opts?: ClusterCommandOptions) => Promise<T>
-  ) {
-    return async (client: RedisClientType<M, F, S, RESP, TYPE_MAPPING>, options?: ClusterCommandOptions) => {
-      const chainId = Symbol("asking chain");
-      const opts = options ? {...options} : {};
-      opts.chainId = chainId;
-
-      const ret = await Promise.all(
-        [
-          client.sendCommand(ASKING.transformArguments(), {chainId: chainId}),
-          fn(client, opts)
-        ]
-      );
-
-      return ret[1];
-    };
-  }
-
   async #execute<T>(
     firstKey: RedisArgument | undefined,
     isReadonly: boolean | undefined,
@@ -523,13 +501,14 @@ export default class RedisCluster<
     const maxCommandRedirections = this.#options.maxCommandRedirections ?? 16;
     let client = await this.#slots.getClient(firstKey, isReadonly);
     let i = 0;
-    let myFn = fn;
+    let myOpts = options;
 
     while (true) {
       try {
-        return await myFn(client, options);
+        return await fn(client, myOpts);
       } catch (err) {
-        myFn = fn;
+        // reset to passed in options, if changed by an ask request 
+        myOpts = options;
         // TODO: error class
         if (++i > maxCommandRedirections || !(err instanceof Error)) {
           throw err;
@@ -547,8 +526,14 @@ export default class RedisCluster<
             throw new Error(`Cannot find node ${address}`);
           }
 
-          myFn = this.#handleAsk(fn);
           client = redirectTo;
+
+          const chainId = Symbol('Asking Chain');
+          const myOpts = options ? {...options} : {};
+          myOpts.chainId = chainId;
+
+          client.sendCommand(parseArgs(ASKING), {chainId: chainId}).catch(err => { console.log(`Asking Failed: ${err}`) } );
+
           continue;
         }
 

diff --git a/packages/client/lib/commands/GET.spec.ts b/packages/client/lib/commands/GET.spec.ts
@@ -1,5 +1,6 @@
 import { strict as assert } from 'node:assert';
-import testUtils, { GLOBAL, parseArgs } from '../test-utils';
+import testUtils, { GLOBAL } from '../test-utils';
+import { parseArgs } from './generic-transformers';
 import GET from './GET';
 
 describe('GET', () => {

diff --git a/packages/client/lib/commands/generic-transformers.ts b/packages/client/lib/commands/generic-transformers.ts
@@ -1,4 +1,5 @@
-import { UnwrapReply, ArrayReply, BlobStringReply, BooleanReply, CommandArguments, DoubleReply, NullReply, NumberReply, RedisArgument, TuplesReply } from '../RESP/types';
+import { UnwrapReply, ArrayReply, BlobStringReply, BooleanReply, CommandArguments, DoubleReply, NullReply, NumberReply, RedisArgument, TuplesReply, Command } from '../RESP/types';
+import { BasicCommandParser } from '../client/parser';
 
 export function isNullReply(reply: unknown): reply is NullReply {
   return reply === null;
@@ -446,3 +447,30 @@ function isPlainKey(key: RedisArgument | ZKeyAndWeight): key is RedisArgument {
 function isPlainKeys(keys: Array<RedisArgument> | Array<ZKeyAndWeight>): keys is Array<RedisArgument> {
   return isPlainKey(keys[0]);
 }
+
+/**
+ * @deprecated
+ */
+export function parseArgs(command: Command, ...args: Array<any>) {
+  if (command.parseCommand) {
+    const parser = new BasicCommandParser();
+    command.parseCommand!(parser, ...args);
+
+    const redisArgs: CommandArguments = parser.redisArgs;
+    if (parser.preserve) {
+      redisArgs.preserve = parser.preserve;
+    }
+    return redisArgs;
+  } else {
+    return command.transformArguments(...args);
+  }
+}
+
+/**
+ * @deprecated
+ */
+export function parseArgsWith(command: Command, ...args: Array<any>) {
+  const parser = new BasicCommandParser();
+  command.parseCommand!(parser, ...args);
+  return parser.preserve;
+}