diff --git a/.changeset/nested-event-filter-exists-false.md b/.changeset/nested-event-filter-exists-false.md new file mode 100644 index 00000000000..8d63d0bc82e --- /dev/null +++ b/.changeset/nested-event-filter-exists-false.md @@ -0,0 +1,7 @@ +--- +"@trigger.dev/core": patch +--- + +Fix `eventFilterMatches` so nested filters that assert the absence of a value match when an ancestor key is missing or `null`, instead of failing before the filter is evaluated. A missing payload is now matched like an empty object: it's how `$exists` behaves on nested paths elsewhere, and how these filters already behaved for a missing key at the top level. + +This changes the result only for filters whose leaves match a missing key today, i.e. `$exists: false`, `$isNull: false` and `$anythingBut`. Filters that require a value (`$exists: true`, `$startsWith`, `$gt`, value arrays, ...) are unchanged. It fixes `retry.fetch`'s `bodyFilter` (e.g. `{ error: { retryAfter: [{ $exists: false }] } }`) not matching responses that have no `error` object. diff --git a/packages/core/src/eventFilterMatches.ts b/packages/core/src/eventFilterMatches.ts index 86a78aef750..23138fe16fe 100644 --- a/packages/core/src/eventFilterMatches.ts +++ b/packages/core/src/eventFilterMatches.ts @@ -5,16 +5,17 @@ import type { EventFilter } from "./schemas/eventFilter.js"; // If the values of the array are objects, then we are doing content filtering // An example would be [{ $endsWith: ".png" }, { $startsWith: "images/" } ] export function eventFilterMatches(payload: any, filter: EventFilter): boolean { - if (payload === undefined || payload === null) { - if (Object.entries(filter).length === 0) { - return true; - } else { - return false; - } - } + // A missing (undefined/null) payload is matched like an empty object, so that a missing payload and + // a missing key are treated the same way. Filters that assert the absence of a value (`$exists: + // false`, `$isNull: false`, `$anythingBut`) therefore match when an ancestor key is missing or + // null, instead of failing before the filter is evaluated. This is the same result nested paths + // give in MongoDB-style filters, and the same result these filters already gave for a missing key + // at the top level. Filters that need a value (`$exists: true`, `$startsWith`, `$gt`, ...) still + // don't match `undefined`. + const payloadObject = payload === undefined || payload === null ? {} : payload; for (const [patternKey, patternValue] of Object.entries(filter)) { - const payloadValue = payload[patternKey]; + const payloadValue = payloadObject[patternKey]; if (Array.isArray(patternValue)) { if (patternValue.length === 0) { diff --git a/packages/core/test/eventFilterMatches.test.ts b/packages/core/test/eventFilterMatches.test.ts index 4df17fa235a..8209c9b07d4 100644 --- a/packages/core/test/eventFilterMatches.test.ts +++ b/packages/core/test/eventFilterMatches.test.ts @@ -8,6 +8,12 @@ describe("eventFilterMatches", () => { expect(eventFilterMatches(payload, filter)).toBe(true); }); + it("should return true if the payload is null and the filter is empty", () => { + const payload = null; + const filter: EventFilter = {}; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + it("should return false if the payload is undefined and the filter isn't empty", () => { const payload = undefined; const filter: EventFilter = { @@ -16,6 +22,22 @@ describe("eventFilterMatches", () => { expect(eventFilterMatches(payload, filter)).toBe(false); }); + it("should return true if the payload is undefined and the filter asserts the absence of a key", () => { + const payload = undefined; + const filter: EventFilter = { + name: [{ $exists: false }], + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true if the payload is null and the filter asserts the absence of a key", () => { + const payload = null; + const filter: EventFilter = { + name: [{ $exists: false }], + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + it("should return true when payload matches string filter", () => { const payload = { name: "John", @@ -209,6 +231,139 @@ describe("eventFilterMatches", () => { expect(eventFilterMatches(payload, filter)).toBe(true); }); + it("should return true when payload does NOT have a nested key that $exists = false", () => { + const payload = { + name: "John", + age: 30, + isAdmin: false, + hobbies: ["reading", "swimming"], + address: { + street: "123 Main St", + city: "Anytown", + }, + }; + const filter: EventFilter = { + address: { + zip: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when the parent key of a nested $exists = false filter is missing", () => { + const payload = {}; + const filter: EventFilter = { + user: { + email: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when the parent key of a nested $exists = false filter is null", () => { + const payload = { + user: null, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when the parent key of a nested $exists = false filter is an empty object", () => { + const payload = { + user: {}, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when the payload is null and a nested $exists = false filter is used", () => { + const payload = null; + const filter: EventFilter = { + user: { + email: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when the parent key of a nested $exists = false filter isn't an object", () => { + const payload = { + user: 42, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when a nested $isNull = false filter has no parent key", () => { + const payload = { + name: "John", + }; + const filter: EventFilter = { + user: { + email: [{ $isNull: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when a nested $anythingBut filter has no parent key", () => { + const payload = { + name: "John", + }; + const filter: EventFilter = { + user: { + email: [{ $anythingBut: "banned@example.com" }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when an ancestor key of a nested $exists = false filter is missing", () => { + const payload = { + account: {}, + }; + const filter: EventFilter = { + account: { + profile: { + settings: { + theme: [{ $exists: false }], + }, + }, + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + + it("should return true when an array item does NOT have a nested key that $exists = false", () => { + const payload = { + items: [ + { id: 1 }, + { + id: 2, + deletedAt: "2024-01-01T00:00:00.000Z", + }, + ], + }; + const filter: EventFilter = { + items: { + deletedAt: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(true); + }); + it("should return true when payload does match numeric condition", () => { const payload = { name: "John", @@ -560,6 +715,167 @@ describe("eventFilterMatches", () => { expect(eventFilterMatches(payload, filter)).toBe(false); }); + it("should return false when the parent key of a nested $exists = true filter is missing", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: true }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested $exists = true filter is null", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + user: null, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: true }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested $exists = true filter isn't an object", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + user: 42, + }; + const filter: EventFilter = { + user: { + email: [{ $exists: true }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when a nested $isNull = true filter has no parent key", () => { + const payload = { + name: "Jane", + }; + const filter: EventFilter = { + user: { + email: [{ $isNull: true }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when payload does have a nested key that $exists = false", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + address: { + street: "456 Elm St", + city: "Othertown", + }, + }; + const filter: EventFilter = { + address: { + city: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested value filter is missing", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + }; + const filter: EventFilter = { + user: { + email: ["john@example.com"], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested value filter is null", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + user: null, + }; + const filter: EventFilter = { + user: { + email: ["john@example.com"], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested $startsWith filter is missing", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + }; + const filter: EventFilter = { + user: { + email: [{ $startsWith: "john" }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when the parent key of a nested $gt filter is missing", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + }; + const filter: EventFilter = { + metrics: { + score: [{ $gt: 10 }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when a nested filter doesn't match even though a sibling key does", () => { + const payload = { + name: "Jane", + age: 25, + isAdmin: true, + }; + const filter: EventFilter = { + name: ["Jane"], + user: { + email: [{ $exists: true }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + + it("should return false when a nested filter doesn't match any item of an array", () => { + const payload = { + items: [ + { id: 1, deletedAt: "2024-01-01T00:00:00.000Z" }, + { id: 2, deletedAt: "2024-02-01T00:00:00.000Z" }, + ], + }; + const filter: EventFilter = { + items: { + deletedAt: [{ $exists: false }], + }, + }; + expect(eventFilterMatches(payload, filter)).toBe(false); + }); + it("should return false when the payload does not match the numeric filters", () => { const payload = { name: "Jane", diff --git a/packages/trigger-sdk/src/v3/retry.test.ts b/packages/trigger-sdk/src/v3/retry.test.ts index 3b22c2cecc2..4d4cf3852a7 100644 --- a/packages/trigger-sdk/src/v3/retry.test.ts +++ b/packages/trigger-sdk/src/v3/retry.test.ts @@ -1,9 +1,13 @@ +import { createServer } from "node:http"; import { SEMATTRS_HTTP_URL } from "@opentelemetry/semantic-conventions"; +import type { EventFilter } from "@trigger.dev/core/v3"; +import { runInMockTaskContext } from "@trigger.dev/core/v3/test"; import { describe, expect, it } from "vitest"; import { createAttributesFromHeaders, createFetchAttributes, createFetchResponseAttributes, + retry, } from "./retry.js"; describe("retry fetch telemetry", () => { @@ -54,3 +58,116 @@ describe("retry fetch telemetry", () => { expect(attributes).not.toHaveProperty("http.response.header.set-cookie"); }); }); + +/** + * Starts a local HTTP server that answers with JSON, and records the `x-retry-count` + * header of every request it receives (the SDK sends the current attempt number). + */ +async function startJsonServer( + responder: (requestIndex: number) => { status: number; body: unknown } +) { + const retryCounts: (string | undefined)[] = []; + + const server = createServer((request, response) => { + const requestIndex = retryCounts.push(request.headers["x-retry-count"] as string | undefined); + + const { status, body } = responder(requestIndex); + + response.writeHead(status, { "content-type": "application/json" }); + response.end(JSON.stringify(body)); + }); + + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + + const address = server.address(); + + if (typeof address !== "object" || address === null) { + throw new Error("Failed to start the retry test server"); + } + + return { + url: `http://127.0.0.1:${address.port}/`, + retryCounts, + close: () => + new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }), + }; +} + +/** Retries a 429 using a `backoff` strategy that waits ~1ms, when the `bodyFilter` matches. */ +const fetchWithBodyFilter = (url: string, bodyFilter: EventFilter) => + runInMockTaskContext(() => + retry.fetch(url, { + retry: { + byStatus: { + "429": { + strategy: "backoff", + bodyFilter, + maxAttempts: 3, + minTimeoutInMs: 1, + maxTimeoutInMs: 5, + factor: 1, + randomize: false, + }, + }, + }, + }) + ); + +describe("retry.fetch bodyFilter", () => { + it("retries when a nested $exists = false filter matches a body without the parent key", async () => { + const server = await startJsonServer((requestIndex) => + requestIndex === 1 + ? { status: 429, body: { message: "slow down" } } + : { status: 200, body: { ok: true } } + ); + + try { + const response = await fetchWithBodyFilter(server.url, { + error: { retryAfter: [{ $exists: false }] }, + }); + + expect(response.status).toBe(200); + expect(server.retryCounts).toEqual(["1", "2"]); + } finally { + await server.close(); + } + }); + + it("does not retry when a nested $exists = false filter has the key in the body", async () => { + const server = await startJsonServer(() => ({ + status: 429, + body: { error: { retryAfter: 30 } }, + })); + + try { + const response = await fetchWithBodyFilter(server.url, { + error: { retryAfter: [{ $exists: false }] }, + }); + + expect(response.status).toBe(429); + expect(server.retryCounts).toEqual(["1"]); + } finally { + await server.close(); + } + }); + + it("does not retry when a nested $exists = true filter has no parent key in the body", async () => { + const server = await startJsonServer(() => ({ + status: 429, + body: { message: "slow down" }, + })); + + try { + const response = await fetchWithBodyFilter(server.url, { + error: { retryAfter: [{ $exists: true }] }, + }); + + expect(response.status).toBe(429); + expect(server.retryCounts).toEqual(["1"]); + } finally { + await server.close(); + } + }); +});