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;
+}