Refactored and addressed PR comments

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
valkey-io · Jul 17, 2024 · 206b1ce · 206b1ce
1 parent e2647c9
commit 206b1ce
Show file tree

Hide file tree

Showing 6 changed files with 135 additions and 77 deletions.
diff --git a/node/src/BaseClient.ts b/node/src/BaseClient.ts
@@ -10,12 +10,12 @@ import {
 } from "glide-rs";
 import * as net from "net";
 import { Buffer, BufferWriter, Reader, Writer } from "protobufjs";
+import { LPosOptions } from "./command-options/LPosOptions";
 import {
     AggregationType,
     ExpireOptions,
     InsertPosition,
     KeyWeight,
-    LPosOptions,
     RangeByIndex,
     RangeByLex,
     RangeByScore,
@@ -2727,8 +2727,8 @@ export class BaseClient {
      * @example
      * ```typescript
      * await client.rpush("myList", ["a", "b", "c", "d", "e", "e"]);
-     * console.log(await client.lpos("myList", "e", { rank: 2 })); // Output: 5 - the second occurrence of "e" is in the fifth element.
-     * console.log(await client.lpos("myList", "e", { count: 3 })); // Output: [ 4, 5, 6 ] - the elements of the occurrences of "e".
+     * console.log(await client.lpos("myList", "e", new LPosOptions({ rank: 2 }))); // Output: 5 - the second occurrence of "e" is at index 5.
+     * console.log(await client.lpos("myList", "e", new LPosOptions({ count: 3 }))); // Output: [ 4, 5 ] - indices for the occurrences of "e" in list "myList".
      * ```
      */
     public lpos(

diff --git a/node/src/Commands.ts b/node/src/Commands.ts
@@ -4,6 +4,7 @@
 
 import { createLeakedStringVec, MAX_REQUEST_ARGS_LEN } from "glide-rs";
 import Long from "long";
+import { LPosOptions } from "./command-options/LPosOptions";
 
 import { command_request } from "./ProtobufMessage";
 
@@ -1550,29 +1551,6 @@ export function createObjectRefcount(key: string): command_request.Command {
     return createCommand(RequestType.ObjectRefCount, [key]);
 }
 
-export type LPosOptions = {
-    rank?: number;
-    count?: number;
-    maxLength?: number;
-};
-
-function addLPosOptions(options: LPosOptions, args: string[]) {
-    if (options.rank !== undefined) {
-        args.push("RANK");
-        args.push(options.rank.toString());
-    }
-
-    if (options.count !== undefined) {
-        args.push("COUNT");
-        args.push(options.count.toString());
-    }
-
-    if (options.maxLength !== undefined) {
-        args.push("MAXLEN");
-        args.push(options.maxLength.toString());
-    }
-}
-
 /**
  * @internal
  */
@@ -1581,10 +1559,10 @@ export function createLPos(
     element: string,
     options?: LPosOptions,
 ): command_request.Command {
-    const args: string[] = [key, element];
+    let args: string[] = [key, element];
 
     if (options) {
-        addLPosOptions(options, args);
+        args = args.concat(options.toArgs());
     }
 
     return createCommand(RequestType.LPos, args);

diff --git a/node/src/Transaction.ts b/node/src/Transaction.ts
@@ -2,13 +2,13 @@
  * Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
  */
 
+import { LPosOptions } from "./command-options/LPosOptions";
 import {
     AggregationType,
     ExpireOptions,
     InfoOptions,
     InsertPosition,
     KeyWeight,
-    LPosOptions,
     RangeByIndex,
     RangeByLex,
     RangeByScore,

diff --git a/node/src/command-options/LPosOptions.ts b/node/src/command-options/LPosOptions.ts
@@ -0,0 +1,65 @@
+/**
+ * Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0
+ */
+
+/**
+ * Optional arguments to LPOS command.
+ *
+ * See https://valkey.io/commands/lpos/ for more details.
+ */
+export class LPosOptions {
+    /** Redis API keyword use to determine the rank of the match to return. */
+    public static RANK_REDIS_API = "RANK";
+    /** Redis API keyword used to extract specific number of matching indices from a list. */
+    public static COUNT_REDIS_API = "COUNT";
+    /** Redis API keyword used to determine the maximum number of list items to compare. */
+
+    public static MAXLEN_REDIS_API = "MAXLEN";
+    /** The rank of the match to return. */
+    private rank?: number;
+    /** The specific number of matching indices from a list. */
+    private count?: number;
+    /** The maximum number of comparisons to make between the element and the items in the list. */
+    private maxLength?: number;
+
+    constructor({
+        rank,
+        count,
+        maxLength,
+    }: {
+        rank?: number;
+        count?: number;
+        maxLength?: number;
+    }) {
+        this.rank = rank;
+        this.count = count;
+        this.maxLength = maxLength;
+    }
+
+    /**
+     *
+     * Converts LPosOptions into a string[].
+     *
+     * @returns string[]
+     */
+    public toArgs(): string[] {
+        const args: string[] = [];
+
+        if (this.rank !== undefined) {
+            args.push(LPosOptions.RANK_REDIS_API);
+            args.push(this.rank.toString());
+        }
+
+        if (this.count !== undefined) {
+            args.push(LPosOptions.COUNT_REDIS_API);
+            args.push(this.count.toString());
+        }
+
+        if (this.maxLength !== undefined) {
+            args.push(LPosOptions.MAXLEN_REDIS_API);
+            args.push(this.maxLength.toString());
+        }
+
+        return args;
+    }
+}
diff --git a/node/tests/SharedTests.ts b/node/tests/SharedTests.ts
@@ -13,6 +13,7 @@ import {
     InfoOptions,
     InsertPosition,
     ProtocolVersion,
+    RequestError,
     Script,
     parseInfoResponse,
 } from "../";
@@ -25,6 +26,7 @@ import {
     intoArray,
     intoString,
 } from "./TestUtilities";
+import { LPosOptions } from "../build-ts/src/command-options/LPosOptions";
 
 async function getVersion(): Promise<[number, number, number]> {
     const versionString = await new Promise<string>((resolve, reject) => {
@@ -3537,43 +3539,45 @@ export function runBaseTests<Context>(config: {
 
                 // simplest case
                 expect(await client.lpos(key, "a")).toEqual(0);
-                expect(await client.lpos(key, "b", { rank: 2 })).toEqual(5);
+                expect(
+                    await client.lpos(key, "b", new LPosOptions({ rank: 2 })),
+                ).toEqual(5);
 
                 // element doesn't exist
                 expect(await client.lpos(key, "e")).toBeNull();
 
                 // reverse traversal
-                expect(await client.lpos(key, "b", { rank: -2 })).toEqual(2);
+                expect(
+                    await client.lpos(key, "b", new LPosOptions({ rank: -2 })),
+                ).toEqual(2);
 
                 // unlimited comparisons
                 expect(
-                    await client.lpos(key, "a", { rank: 1, maxLength: 0 }),
+                    await client.lpos(
+                        key,
+                        "a",
+                        new LPosOptions({ rank: 1, maxLength: 0 }),
+                    ),
                 ).toEqual(0);
 
                 // limited comparisons
                 expect(
-                    await client.lpos(key, "c", { rank: 1, maxLength: 2 }),
+                    await client.lpos(
+                        key,
+                        "c",
+                        new LPosOptions({ rank: 1, maxLength: 2 }),
+                    ),
                 ).toBeNull();
 
                 // invalid rank value
-                try {
-                    expect(await client.lpos(key, "a", { rank: 0 })).toThrow();
-                } catch (e) {
-                    expect((e as Error).message).toMatch(
-                        "An error was signalled by the server - ResponseError: RANK can't be zero: use 1 to start from the first match, 2 from the second ... or use negative to start from the end of the list",
-                    );
-                }
+                await expect(
+                    client.lpos(key, "a", new LPosOptions({ rank: 0 })),
+                ).rejects.toThrow(RequestError);
 
                 // invalid maxlen value
-                try {
-                    expect(
-                        await client.lpos(key, "a", { maxLength: -1 }),
-                    ).toThrow();
-                } catch (e) {
-                    expect((e as Error).message).toMatch(
-                        "An error was signalled by the server - ResponseError: MAXLEN can't be negative",
-                    );
-                }
+                await expect(
+                    client.lpos(key, "a", new LPosOptions({ maxLength: -1 })),
+                ).rejects.toThrow(RequestError);
 
                 // non-existent key
                 expect(await client.lpos("non-existent_key", "e")).toBeNull();
@@ -3582,45 +3586,51 @@ export function runBaseTests<Context>(config: {
                 const wrongDataType = `{key}:${uuidv4()}`;
                 expect(await client.sadd(wrongDataType, ["a", "b"])).toEqual(2);
 
-                try {
-                    expect(await client.lpos(wrongDataType, "a")).toThrow();
-                } catch (e) {
-                    expect((e as Error).message).toMatch(
-                        "WRONGTYPE: Operation against a key holding the wrong kind of value",
-                    );
-                }
+                await expect(client.lpos(wrongDataType, "a")).rejects.toThrow(
+                    RequestError,
+                );
 
                 // invalid count value
-                try {
-                    expect(
-                        await client.lpos(key, "a", { count: -1 }),
-                    ).toThrow();
-                } catch (e) {
-                    expect((e as Error).message).toMatch(
-                        "An error was signalled by the server - ResponseError: COUNT can't be negative",
-                    );
-                }
+                await expect(
+                    client.lpos(key, "a", new LPosOptions({ count: -1 })),
+                ).rejects.toThrow(RequestError);
 
                 // with count
-                expect(await client.lpos(key, "a", { count: 2 })).toEqual([
-                    0, 1,
-                ]);
-                expect(await client.lpos(key, "a", { count: 0 })).toEqual([
-                    0, 1, 4,
-                ]);
                 expect(
-                    await client.lpos(key, "a", { rank: 1, count: 0 }),
+                    await client.lpos(key, "a", new LPosOptions({ count: 2 })),
+                ).toEqual([0, 1]);
+                expect(
+                    await client.lpos(key, "a", new LPosOptions({ count: 0 })),
+                ).toEqual([0, 1, 4]);
+                expect(
+                    await client.lpos(
+                        key,
+                        "a",
+                        new LPosOptions({ rank: 1, count: 0 }),
+                    ),
                 ).toEqual([0, 1, 4]);
                 expect(
-                    await client.lpos(key, "a", { rank: 2, count: 0 }),
+                    await client.lpos(
+                        key,
+                        "a",
+                        new LPosOptions({ rank: 2, count: 0 }),
+                    ),
                 ).toEqual([1, 4]);
                 expect(
-                    await client.lpos(key, "a", { rank: 3, count: 0 }),
+                    await client.lpos(
+                        key,
+                        "a",
+                        new LPosOptions({ rank: 3, count: 0 }),
+                    ),
                 ).toEqual([4]);
 
                 // reverse traversal
                 expect(
-                    await client.lpos(key, "a", { rank: -1, count: 0 }),
+                    await client.lpos(
+                        key,
+                        "a",
+                        new LPosOptions({ rank: -1, count: 0 }),
+                    ),
                 ).toEqual([4, 1, 0]);
             }, protocol);
         },

diff --git a/node/tests/TestUtilities.ts b/node/tests/TestUtilities.ts
@@ -19,6 +19,7 @@ import {
     Transaction,
 } from "..";
 import { checkIfServerVersionLessThan } from "./SharedTests";
+import { LPosOptions } from "../build-ts/src/command-options/LPosOptions";
 
 beforeAll(() => {
     Logger.init("info");
@@ -518,9 +519,13 @@ export async function transactionTest(
         field + "3",
     ]);
     args.push(5);
-    baseTransaction.lpos(key15, field + "1", { rank: 2 });
+    baseTransaction.lpos(key15, field + "1", new LPosOptions({ rank: 2 }));
     args.push(1);
-    baseTransaction.lpos(key15, field + "1", { rank: 2, count: 0 });
+    baseTransaction.lpos(
+        key15,
+        field + "1",
+        new LPosOptions({ rank: 2, count: 0 }),
+    );
     args.push([1]);
     return args;
 }