Skip to content

Commit f601f2c

Browse files
CahidArdaCahidArda
authored
DX-2769: reject empty id in single-resource endpoints (#202)
* fix: reject empty id in single-resource endpoints (DX-2769) * DX-2735: report failing step name via Upstash-Error-Step-Name header (#201) * feat: report failing step name via Upstash-Error-Step-Name header When a step throws during execution, attach the step name to the error and surface it on the 500 error response via the Upstash-Error-Step-Name header so Workflow Logs can show which step is being retried. * feat: expose stepName on failed next-step logs The server now reports the failing step name (from the Upstash-Error-Step-Name header) on the "next" step log group. Surface it as an optional stepName field in the logs response type. * fix: change requestcatcher to httpstatus * fix: update mock server URLs to use requestcatcher * feat: expose labels array on DLQ messages DLQMessage only surfaced the first label via the deprecated `label` field, unlike WorkflowRunLog which already exposes the full `labels` array. Add `labels: string[]` to DLQMessage (and PublicDLQMessage) and mark `label` deprecated, matching the run-log type. Adds a mocked test asserting both fields round-trip through dlq.list() and a live test confirming a run triggered with label: [a, b] surfaces label === a and labels === [a, b] on its DLQ message. * fix: bump version * fix: urls in the tests * fix: harden step-name error annotation and header sanitization - attachStepNameToError no longer throws on non-extensible/frozen errors, so it can never mask the original failure - sanitize the step name (strip control chars like CR/LF) before putting it in the Upstash-Error-Step-Name header so an invalid value can't break the 500 response * style: apply prettier formatting fixes to test files * test: replace requestcatcher.com with centralized example.com constant requestcatcher.com was failing live deliveries with TLS certificate errors. Replace it with the IANA-reserved example.com (valid cert, stable) and centralize the host into a single MOCK_DESTINATION_HOST constant in test-utils, referenced everywhere instead of repeating the literal. Examples define a local MOCK_DESTINATION_URL since they can't import test-utils. Also switch the triggerWorkflowDelete test from spyOn (which made a real network call) to the local mockQStashServer utility, removing the last external request dependency. * fix: address review comments on empty-id validation (DX-2769) - Validate every id in array/legacy forms of cancel() and DLQ resume/restart/delete via a shared toNonEmptyIdArray helper, not just the single-string overload, so cancel([""]) / delete([""]) fail fast instead of sending a bulk filter the server treats as a collection op. - assertNonEmptyId: use a falsy guard instead of id.length so a plain-JS caller passing undefined/null gets a consistent QstashError rather than a TypeError from reading .length. - makeNotifyRequest: treat workflowRunId with an explicit undefined check and validate it when provided (an empty string no longer silently changes the request path). - await the .rejects assertions inside mockQStashServer execute callbacks so the rejection is actually asserted. - add tests covering an empty string inside an id array for cancel and DLQ delete.
1 parent 257d563 commit f601f2c

15 files changed

Lines changed: 334 additions & 149 deletions

File tree

‎examples/cloudflare-workers-hono/ci.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import { serve } from "@upstash/workflow/nextjs"
66
import { describe, test, expect } from "vitest"
77
import { RedisEntry } from "./src/app"
88

9+
// Stable throwaway destination with a valid TLS cert (replaces requestcatcher.com)
10+
const MOCK_DESTINATION_URL = "https://example.com/"
11+
912
export const RETRY_COUNT = 10
1013
export const RETRY_INTERVAL_DURATION = 1000
1114
export const CHECK_WF_AFTER_INIT_DURATION = 10000
@@ -129,7 +132,7 @@ describe("cloudflare workers", () => {
129132
test("should send first invocation", async () => {
130133

131134
const qstashClient = new Client({
132-
baseUrl: "https://workflow-tests.requestcatcher.com/",
135+
baseUrl: MOCK_DESTINATION_URL,
133136
token: "mock"
134137
})
135138

@@ -147,7 +150,7 @@ describe("cloudflare workers", () => {
147150
}
148151
)
149152

150-
const request = new Request("https://workflow-tests.requestcatcher.com/")
153+
const request = new Request(MOCK_DESTINATION_URL)
151154
const response = await serveHandler(request)
152155

153156
// it should send a request, but get failed to parse error because

‎examples/nextjs-pages/ci.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import { Redis } from "@upstash/redis"
66
import { serve } from "@upstash/workflow/nextjs"
77
import { describe, test, expect } from "vitest"
88

9+
// Stable throwaway destination with a valid TLS cert (replaces requestcatcher.com)
10+
const MOCK_DESTINATION_URL = "https://example.com/"
11+
912
export const RETRY_COUNT = 20
1013
export const RETRY_INTERVAL_DURATION = 1000
1114
export const CHECK_WF_AFTER_INIT_DURATION = 10000
@@ -192,7 +195,7 @@ const testEndpoint = ({
192195
describe("nextjs-pages", () => {
193196
test("should send first invocation", async () => {
194197
const qstashClient = new Client({
195-
baseUrl: "https://workflow-tests.requestcatcher.com/",
198+
baseUrl: MOCK_DESTINATION_URL,
196199
token: "mock"
197200
})
198201

@@ -210,7 +213,7 @@ describe("nextjs-pages", () => {
210213
}
211214
)
212215

213-
const request = new Request("https://workflow-tests.requestcatcher.com/")
216+
const request = new Request(MOCK_DESTINATION_URL)
214217
const response = await serveHandler(request)
215218

216219
// it should send a request, but get failed to parse error because

‎examples/nextjs/ci.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ import { Client } from "@upstash/qstash"
33
import { serve } from "@upstash/workflow/nextjs"
44
import { describe, test, expect } from "vitest"
55

6+
// Stable throwaway destination with a valid TLS cert (replaces requestcatcher.com)
7+
const MOCK_DESTINATION_URL = "https://example.com/"
8+
69
const qstashClient = new Client({
7-
baseUrl: "https://workflow-tests.requestcatcher.com/",
10+
baseUrl: MOCK_DESTINATION_URL,
811
token: "mock"
912
})
1013

@@ -24,7 +27,7 @@ const { POST: serveHandler } = serve(
2427

2528
describe("nextjs tests", () => {
2629
test("should send first invocation", async () => {
27-
const request = new Request("https://workflow-tests.requestcatcher.com/")
30+
const request = new Request(MOCK_DESTINATION_URL)
2831
const response = await serveHandler(request)
2932

3033
// it should send a request, but get failed to parse error because

‎src/client/dlq.test.ts‎

Lines changed: 77 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
import { describe, test, expect } from "bun:test";
2-
import { MOCK_QSTASH_SERVER_URL, mockQStashServer, eventually } from "../test-utils";
2+
import {
3+
MOCK_DESTINATION_HOST,
4+
MOCK_QSTASH_SERVER_URL,
5+
mockQStashServer,
6+
eventually,
7+
} from "../test-utils";
38
import { Client } from ".";
49
import { nanoid } from "../utils";
510

@@ -15,7 +20,7 @@ const MOCK_DLQ_MESSAGES = [
1520
callerIP: "192.168.0.100",
1621
workflowRunId: `wfr-${nanoid()}`,
1722
workflowCreatedAt: 1645000000000,
18-
workflowUrl: "https://example.com/all-params-1",
23+
workflowUrl: `${MOCK_DESTINATION_HOST}/all-params-1`,
1924
responseStatus: 422,
2025
responseHeader: { "content-length": ["100"] },
2126
responseBody: "Validation Error",
@@ -25,7 +30,7 @@ const MOCK_DLQ_MESSAGES = [
2530
responseBody: "Internal Server Error",
2631
responseHeaders: { "content-type": ["application/json"] },
2732
},
28-
failureCallback: "https://example.com/failure-callback",
33+
failureCallback: `${MOCK_DESTINATION_HOST}/failure-callback`,
2934
label: WORKFLOW_LABEL,
3035
},
3136
{
@@ -38,11 +43,11 @@ const MOCK_DLQ_MESSAGES = [
3843
callerIP: "192.168.0.101",
3944
workflowRunId: `wfr-${nanoid()}`,
4045
workflowCreatedAt: 1645100000000,
41-
workflowUrl: "https://example.com/all-params-2",
46+
workflowUrl: `${MOCK_DESTINATION_HOST}/all-params-2`,
4247
responseStatus: 503,
4348
responseHeader: { "retry-after": ["60"] },
4449
responseBody: "Service Unavailable",
45-
failureCallback: "https://example.com/failure-callback",
50+
failureCallback: `${MOCK_DESTINATION_HOST}/failure-callback`,
4651
},
4752
] as Awaited<ReturnType<Client["dlq"]["list"]>>["messages"];
4853

@@ -148,7 +153,7 @@ describe("DLQ", () => {
148153
const filter = {
149154
fromDate: 1640995200000, // 2022-01-01
150155
toDate: 1672531200000, // 2023-01-01
151-
workflowUrl: "https://example.com",
156+
workflowUrl: MOCK_DESTINATION_HOST,
152157
};
153158

154159
await mockQStashServer({
@@ -175,7 +180,7 @@ describe("DLQ", () => {
175180
const filter = {
176181
fromDate: new Date(fromDateMs),
177182
toDate: new Date(toDateMs),
178-
workflowUrl: "https://example.com",
183+
workflowUrl: MOCK_DESTINATION_HOST,
179184
};
180185

181186
await mockQStashServer({
@@ -202,7 +207,7 @@ describe("DLQ", () => {
202207
const nextCursor = `next-${cursor}`;
203208
const filter = {
204209
fromDate: 1640995200000,
205-
workflowUrl: "https://example.com",
210+
workflowUrl: MOCK_DESTINATION_HOST,
206211
};
207212

208213
await mockQStashServer({
@@ -285,6 +290,16 @@ describe("DLQ", () => {
285290
});
286291
});
287292

293+
test("should not send request when called with an empty string", async () => {
294+
await mockQStashServer({
295+
execute: async () => {
296+
await expect(client.dlq.resume("")).rejects.toThrow("DLQ id cannot be empty");
297+
},
298+
responseFields: { status: 200, body: {} },
299+
receivesRequest: false,
300+
});
301+
});
302+
288303
test("should throw when dlqIds is empty in filter format", async () => {
289304
await mockQStashServer({
290305
execute: async () => {
@@ -328,7 +343,7 @@ describe("DLQ", () => {
328343
await mockQStashServer({
329344
execute: async () => {
330345
const result = await client.dlq.resume({
331-
filter: { label: "my-label", workflowUrl: "https://example.com/workflow" },
346+
filter: { label: "my-label", workflowUrl: `${MOCK_DESTINATION_HOST}/workflow` },
332347
});
333348
expect(result).toEqual({ cursor: undefined, workflowRuns: responses });
334349
},
@@ -338,7 +353,7 @@ describe("DLQ", () => {
338353
},
339354
receivesRequest: {
340355
method: "POST",
341-
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq/resume?label=my-label&workflowUrl=${encodeURIComponent("https://example.com/workflow")}&count=100`,
356+
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq/resume?label=my-label&workflowUrl=${encodeURIComponent(`${MOCK_DESTINATION_HOST}/workflow`)}&count=100`,
342357
token,
343358
},
344359
});
@@ -530,6 +545,16 @@ describe("DLQ", () => {
530545
});
531546
});
532547

548+
test("should not send request when called with an empty string", async () => {
549+
await mockQStashServer({
550+
execute: async () => {
551+
await expect(client.dlq.restart("")).rejects.toThrow("DLQ id cannot be empty");
552+
},
553+
responseFields: { status: 200, body: {} },
554+
receivesRequest: false,
555+
});
556+
});
557+
533558
test("should throw when dlqIds is empty in filter format", async () => {
534559
await mockQStashServer({
535560
execute: async () => {
@@ -573,7 +598,7 @@ describe("DLQ", () => {
573598
await mockQStashServer({
574599
execute: async () => {
575600
const result = await client.dlq.restart({
576-
filter: { label: "my-label", workflowUrl: "https://example.com/workflow" },
601+
filter: { label: "my-label", workflowUrl: `${MOCK_DESTINATION_HOST}/workflow` },
577602
});
578603
expect(result).toEqual({ cursor: undefined, workflowRuns: responses });
579604
},
@@ -583,7 +608,7 @@ describe("DLQ", () => {
583608
},
584609
receivesRequest: {
585610
method: "POST",
586-
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq/restart?label=my-label&workflowUrl=${encodeURIComponent("https://example.com/workflow")}&count=100`,
611+
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq/restart?label=my-label&workflowUrl=${encodeURIComponent(`${MOCK_DESTINATION_HOST}/workflow`)}&count=100`,
587612
token,
588613
},
589614
});
@@ -712,6 +737,18 @@ describe("DLQ", () => {
712737
},
713738
});
714739
});
740+
741+
test("should not send request when dlqId is an empty string", async () => {
742+
await mockQStashServer({
743+
execute: async () => {
744+
await expect(client.dlq.retryFailureFunction({ dlqId: "" })).rejects.toThrow(
745+
"DLQ id cannot be empty"
746+
);
747+
},
748+
responseFields: { status: 200, body: {} },
749+
receivesRequest: false,
750+
});
751+
});
715752
});
716753

717754
describe("delete", () => {
@@ -768,6 +805,28 @@ describe("DLQ", () => {
768805
});
769806
});
770807

808+
test("should not send request when called with an empty string", async () => {
809+
await mockQStashServer({
810+
execute: async () => {
811+
await expect(client.dlq.delete("")).rejects.toThrow("DLQ id cannot be empty");
812+
},
813+
responseFields: { status: 200, body: {} },
814+
receivesRequest: false,
815+
});
816+
});
817+
818+
test("should not send request when an array contains an empty string", async () => {
819+
await mockQStashServer({
820+
execute: async () => {
821+
await expect(client.dlq.delete(["valid-id", ""])).rejects.toThrow(
822+
"DLQ id cannot be empty"
823+
);
824+
},
825+
responseFields: { status: 200, body: {} },
826+
receivesRequest: false,
827+
});
828+
});
829+
771830
test("should throw when dlqIds is empty in filter format", async () => {
772831
await mockQStashServer({
773832
execute: async () => {
@@ -827,7 +886,7 @@ describe("DLQ", () => {
827886
await mockQStashServer({
828887
execute: async () => {
829888
const result = await client.dlq.delete({
830-
filter: { workflowUrl: "https://example.com/workflow" },
889+
filter: { workflowUrl: `${MOCK_DESTINATION_HOST}/workflow` },
831890
});
832891
expect(result.deleted).toBe(deleted);
833892
},
@@ -837,7 +896,7 @@ describe("DLQ", () => {
837896
},
838897
receivesRequest: {
839898
method: "DELETE",
840-
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?workflowUrl=${encodeURIComponent("https://example.com/workflow")}&count=100`,
899+
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?workflowUrl=${encodeURIComponent(`${MOCK_DESTINATION_HOST}/workflow`)}&count=100`,
841900
token,
842901
},
843902
});
@@ -898,7 +957,7 @@ describe("DLQ", () => {
898957
const result = await client.dlq.delete({
899958
filter: {
900959
label: "my-label",
901-
workflowUrl: "https://example.com/workflow",
960+
workflowUrl: `${MOCK_DESTINATION_HOST}/workflow`,
902961
fromDate: new Date(fromDateMs),
903962
},
904963
});
@@ -910,7 +969,7 @@ describe("DLQ", () => {
910969
},
911970
receivesRequest: {
912971
method: "DELETE",
913-
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?label=my-label&workflowUrl=${encodeURIComponent("https://example.com/workflow")}&fromDate=${fromDateMs}&count=100`,
972+
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?label=my-label&workflowUrl=${encodeURIComponent(`${MOCK_DESTINATION_HOST}/workflow`)}&fromDate=${fromDateMs}&count=100`,
914973
token,
915974
},
916975
});
@@ -924,7 +983,7 @@ describe("DLQ", () => {
924983
const result = await client.dlq.delete({
925984
filter: {
926985
label: "my-label",
927-
workflowUrl: "https://example.com/workflow",
986+
workflowUrl: `${MOCK_DESTINATION_HOST}/workflow`,
928987
fromDate: 1640995200000,
929988
},
930989
});
@@ -936,7 +995,7 @@ describe("DLQ", () => {
936995
},
937996
receivesRequest: {
938997
method: "DELETE",
939-
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?label=my-label&workflowUrl=${encodeURIComponent("https://example.com/workflow")}&fromDate=1640995200000&count=100`,
998+
url: `${MOCK_QSTASH_SERVER_URL}/v2/workflows/dlq?label=my-label&workflowUrl=${encodeURIComponent(`${MOCK_DESTINATION_HOST}/workflow`)}&fromDate=1640995200000&count=100`,
940999
token,
9411000
},
9421001
});

‎src/client/dlq.ts‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import { Client as QStashClient, FlowControl } from "@upstash/qstash";
22
import { DLQResumeRestartOptions, DLQResumeRestartResponse } from "./types";
3-
import { buildBulkActionQueryParameters, normalizeCursor } from "./utils";
3+
import {
4+
assertNonEmptyId,
5+
buildBulkActionQueryParameters,
6+
normalizeCursor,
7+
toNonEmptyIdArray,
8+
} from "./utils";
49
import { WorkflowDLQActionFilters, WorkflowDLQListFilters } from "./filter-types";
510
import { prepareFlowControl } from "../qstash/headers";
611

@@ -189,8 +194,11 @@ export class DLQ {
189194
}
190195

191196
// New format
192-
if (typeof request === "string") request = [request];
193-
if (Array.isArray(request) && request.length === 0) return { workflowRuns: [] };
197+
if (typeof request === "string" || Array.isArray(request)) {
198+
const ids = toNonEmptyIdArray(request, "DLQ id");
199+
if (ids.length === 0) return { workflowRuns: [] };
200+
request = ids;
201+
}
194202
const filters: WorkflowDLQActionFilters = Array.isArray(request)
195203
? { dlqIds: request }
196204
: request;
@@ -268,8 +276,11 @@ export class DLQ {
268276
}
269277

270278
// New format
271-
if (typeof request === "string") request = [request];
272-
if (Array.isArray(request) && request.length === 0) return { workflowRuns: [] };
279+
if (typeof request === "string" || Array.isArray(request)) {
280+
const ids = toNonEmptyIdArray(request, "DLQ id");
281+
if (ids.length === 0) return { workflowRuns: [] };
282+
request = ids;
283+
}
273284
const filters: WorkflowDLQActionFilters = Array.isArray(request)
274285
? { dlqIds: request }
275286
: request;
@@ -295,6 +306,7 @@ export class DLQ {
295306
* @returns response with workflow run information
296307
*/
297308
async retryFailureFunction({ dlqId }: Pick<DLQResumeRestartOptions<string>, "dlqId">) {
309+
assertNonEmptyId(dlqId, "DLQ id");
298310
const response = await this.client.http.request<DLQResumeRestartResponse>({
299311
path: ["v2", "workflows", "dlq", "callback", dlqId],
300312
method: "POST",
@@ -324,8 +336,11 @@ export class DLQ {
324336
* ```
325337
*/
326338
async delete(request: string | string[] | WorkflowDLQActionFilters) {
327-
if (typeof request === "string") request = [request];
328-
if (Array.isArray(request) && request.length === 0) return { deleted: 0 };
339+
if (typeof request === "string" || Array.isArray(request)) {
340+
const ids = toNonEmptyIdArray(request, "DLQ id");
341+
if (ids.length === 0) return { deleted: 0 };
342+
request = ids;
343+
}
329344
const filters: WorkflowDLQActionFilters = Array.isArray(request)
330345
? { dlqIds: request }
331346
: request;

0 commit comments

Comments
 (0)