From c5d3b0fe048588e5ecdc373dc6fb24aca9940c3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cahid=20Arda=20=C3=96z?= Date: Wed, 9 Oct 2024 15:24:09 +0300 Subject: [PATCH 1/2] fix: accept vercel kv env variables (#1315) --- platforms/nodejs.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/platforms/nodejs.ts b/platforms/nodejs.ts index 9bfb4879..f86b88af 100644 --- a/platforms/nodejs.ts +++ b/platforms/nodejs.ts @@ -132,7 +132,7 @@ export class Redis extends core.Redis { baseUrl: configOrRequester.url, retry: configOrRequester.retry, headers: { authorization: `Bearer ${configOrRequester.token}` }, - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + agent: configOrRequester.agent, responseEncoding: configOrRequester.responseEncoding, cache: configOrRequester.cache ?? "no-store", @@ -172,19 +172,19 @@ export class Redis extends core.Redis { */ static fromEnv(config?: Omit): Redis { // @ts-ignore process will be defined in node - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (process.env === undefined) { throw new TypeError( 'Unable to get environment variables, `process.env` is undefined. If you are deploying to cloudflare, please import from "@upstash/redis/cloudflare" instead' ); } // @ts-ignore process will be defined in node - const url = process.env.UPSTASH_REDIS_REST_URL; + const url = process.env.UPSTASH_REDIS_REST_URL || process.env.KV_REST_API_URL; if (!url) { throw new Error("Unable to find environment variable: `UPSTASH_REDIS_REST_URL`"); } // @ts-ignore process will be defined in node - const token = process.env.UPSTASH_REDIS_REST_TOKEN; + const token = process.env.UPSTASH_REDIS_REST_TOKEN || process.env.KV_REST_API_TOKEN; if (!token) { throw new Error("Unable to find environment variable: `UPSTASH_REDIS_REST_TOKEN`"); } From ef1ca9829e359ba31196db06ff1da80cd201974b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cahid=20Arda=20=C3=96z?= Date: Wed, 9 Oct 2024 17:58:42 +0300 Subject: [PATCH 2/2] In auto pipeline, throw errors seperately (#1305) * fix: in auto pipeline, throw errors seperately * fix: add tests * fix: overload exec method * fix: use interface to overload exec * fix: return type * fix: check undefined result in error --- pkg/auto-pipeline.test.ts | 45 ++++++++++++++++++++ pkg/auto-pipeline.ts | 12 ++++-- pkg/pipeline.test.ts | 49 +++++++++++++++++++++ pkg/pipeline.ts | 89 +++++++++++++++++++++++++++------------ 4 files changed, 164 insertions(+), 31 deletions(-) diff --git a/pkg/auto-pipeline.test.ts b/pkg/auto-pipeline.test.ts index ac0a0f36..dfcb39b0 100644 --- a/pkg/auto-pipeline.test.ts +++ b/pkg/auto-pipeline.test.ts @@ -318,4 +318,49 @@ describe("Auto pipeline", () => { expect(res).toEqual(["OK", "OK", "bar", { hello: "world" }, 1, null]); }); + + test.only("should throw errors granularly", async () => { + // in this test, we have two methods being called parallel. both + // use redis, but one of them has try/catch. when the request in + // try fails, it shouldn't make the request in the parallel request + // fail + const redis = Redis.fromEnv({ + enableAutoPipelining: true, + }); + + const scriptLoadCommand = new ScriptLoadCommand(["redis.call('SET', 'foobar', 'foobar')"]); + const scriptHash = await scriptLoadCommand.exec(client); + await redis.scriptFlush(); + + const methodOne = async () => { + // method with try catch + try { + await redis.evalsha(scriptHash, [], []); + throw new Error("test should have thrown in the command above"); + } catch (error_) { + const error = error_ as Error; + + if (error.message.includes("NOSCRIPT")) { + await scriptLoadCommand.exec(client); + await redis.evalsha(scriptHash, [], []); + return true; + } else { + throw new Error("incorrect error was thrown:", error); + } + } + }; + + const methodTwo = async () => { + await redis.set("barfoo", "barfoo"); + return await redis.get("barfoo"); + }; + + const [result1, result2] = await Promise.all([methodOne(), methodTwo()]); + expect(result1).toBeTrue(); + expect(result2).toBe("barfoo"); + + // first method executed correctly + const result = await redis.get("foobar"); + expect(result).toBe("foobar"); + }); }); diff --git a/pkg/auto-pipeline.ts b/pkg/auto-pipeline.ts index e3258f18..e31399c7 100644 --- a/pkg/auto-pipeline.ts +++ b/pkg/auto-pipeline.ts @@ -1,4 +1,6 @@ import type { Command } from "./commands/command"; +import { UpstashError } from "./error"; +import type { UpstashResponse } from "./http"; import type { Pipeline } from "./pipeline"; import type { Redis } from "./redis"; import type { CommandArgs } from "./types"; @@ -86,7 +88,7 @@ class AutoPipelineExecutor { const pipelineDone = this.deferExecution().then(() => { if (!this.pipelinePromises.has(pipeline)) { - const pipelinePromise = pipeline.exec(); + const pipelinePromise = pipeline.exec({ keepErrors: true }); this.pipelineCounter += 1; this.pipelinePromises.set(pipeline, pipelinePromise); @@ -96,8 +98,12 @@ class AutoPipelineExecutor { return this.pipelinePromises.get(pipeline)!; }); - const results = await pipelineDone; - return results[index] as T; + const results = (await pipelineDone) as UpstashResponse[]; + const commandResult = results[index]; + if (commandResult.error) { + throw new UpstashError(`Command failed: ${commandResult.error}`); + } + return commandResult.result as T; } private async deferExecution() { diff --git a/pkg/pipeline.test.ts b/pkg/pipeline.test.ts index 4b91de20..dc18ad2a 100644 --- a/pkg/pipeline.test.ts +++ b/pkg/pipeline.test.ts @@ -252,3 +252,52 @@ describe("use all the things", () => { expect(res.length).toEqual(121); }); }); +describe("keep errors", () => { + test("should return results in case of success", async () => { + const p = new Pipeline({ client }); + p.set("foo", "1"); + p.set("bar", "2"); + p.get("foo"); + p.get("bar"); + const results = await p.exec({ keepErrors: true }); + + // errors are undefined + for (const { error } of results) { + expect(error).toBeUndefined(); + } + expect(results[2].result).toBe(1); + expect(results[3].result).toBe(2); + }); + + test("should throw without keepErrors", async () => { + const p = new Pipeline({ client }); + p.set("foo", "1"); + p.set("bar", "2"); + p.evalsha("wrong-sha1", [], []); + p.get("foo"); + p.get("bar"); + expect(() => p.exec()).toThrow( + "Command 3 [ evalsha ] failed: NOSCRIPT No matching script. Please use EVAL." + ); + }); + + test("should return errors with keepErrors", async () => { + const p = new Pipeline({ client }); + p.set("foo", "1"); + p.set("bar", "2"); + p.evalsha("wrong-sha1", [], []); + p.get("foo"); + p.get("bar"); + const results = await p.exec<[string, string, string, number, number]>({ keepErrors: true }); + + expect(results[0].error).toBeUndefined(); + expect(results[1].error).toBeUndefined(); + expect(results[2].error).toBe("NOSCRIPT No matching script. Please use EVAL."); + expect(results[3].error).toBeUndefined(); + expect(results[4].error).toBeUndefined(); + + expect(results[2].result).toBeUndefined(); + expect(results[3].result).toBe(1); + expect(results[4].result).toBe(2); + }); +}); diff --git a/pkg/pipeline.ts b/pkg/pipeline.ts index 537bb931..ce235109 100644 --- a/pkg/pipeline.ts +++ b/pkg/pipeline.ts @@ -185,6 +185,46 @@ type InferResponseData = { [K in keyof T]: T[K] extends Command ? TData : unknown; }; +interface ExecMethod[]> { + /** + * Send the pipeline request to upstash. + * + * Returns an array with the results of all pipelined commands. + * + * If all commands are statically chained from start to finish, types are inferred. You can still define a return type manually if necessary though: + * ```ts + * const p = redis.pipeline() + * p.get("key") + * const result = p.exec<[{ greeting: string }]>() + * ``` + * + * If one of the commands get an error, the whole pipeline fails. Alternatively, you can set the keepErrors option to true in order to get the errors individually. + * + * If keepErrors is set to true, a list of objects is returned where each object corresponds to a command and is of type: `{ result: unknown, error?: string }`. + * + * ```ts + * const p = redis.pipeline() + * p.get("key") + * + * const result = await p.exec({ keepErrors: true }); + * const getResult = result[0].result + * const getError = result[0].error + * ``` + */ + < + TCommandResults extends unknown[] = [] extends TCommands + ? unknown[] + : InferResponseData, + >(): Promise; + < + TCommandResults extends unknown[] = [] extends TCommands + ? unknown[] + : InferResponseData, + >(options: { + keepErrors: true; + }): Promise<{ [K in keyof TCommandResults]: UpstashResponse }>; +} + /** * Upstash REST API supports command pipelining to send multiple commands in * batch, instead of sending each command one by one and waiting for a response. @@ -246,9 +286,11 @@ export class Pipeline[] = []> { TCommandResults extends unknown[] = [] extends TCommands ? unknown[] : InferResponseData, - >(): Promise => { + >(options?: { + keepErrors: true; + }): Promise => { const start = performance.now(); - const result = await originalExec(); + const result = await (options ? originalExec(options) : originalExec()); const end = performance.now(); const loggerResult = (end - start).toFixed(2); // eslint-disable-next-line no-console @@ -262,23 +304,7 @@ export class Pipeline[] = []> { } } - /** - * Send the pipeline request to upstash. - * - * Returns an array with the results of all pipelined commands. - * - * If all commands are statically chained from start to finish, types are inferred. You can still define a return type manually if necessary though: - * ```ts - * const p = redis.pipeline() - * p.get("key") - * const result = p.exec<[{ greeting: string }]>() - * ``` - */ - exec = async < - TCommandResults extends unknown[] = [] extends TCommands - ? unknown[] - : InferResponseData, - >(): Promise => { + exec: ExecMethod = async (options?: { keepErrors: true }) => { if (this.commands.length === 0) { throw new Error("Pipeline is empty"); } @@ -289,15 +315,22 @@ export class Pipeline[] = []> { body: Object.values(this.commands).map((c) => c.command), })) as UpstashResponse[]; - return res.map(({ error, result }, i) => { - if (error) { - throw new UpstashError( - `Command ${i + 1} [ ${this.commands[i].command[0]} ] failed: ${error}` - ); - } - - return this.commands[i].deserialize(result); - }) as TCommandResults; + return options?.keepErrors + ? res.map(({ error, result }, i) => { + return { + error: error, + result: this.commands[i].deserialize(result), + }; + }) + : res.map(({ error, result }, i) => { + if (error) { + throw new UpstashError( + `Command ${i + 1} [ ${this.commands[i].command[0]} ] failed: ${error}` + ); + } + + return this.commands[i].deserialize(result); + }); }; /**