diff --git a/packages/orm/src/builders/buildWhere.test.ts b/packages/orm/src/builders/buildWhere.test.ts index f282dcd..3f59f37 100644 --- a/packages/orm/src/builders/buildWhere.test.ts +++ b/packages/orm/src/builders/buildWhere.test.ts @@ -84,7 +84,17 @@ describe("buildWhere", () => { test("builds NOT clause", () => { const where = { [$not]: { active: true } }; const result = buildWhere(db.config, [], table, where)!; - expect(result.text).toBe('NOT "u"."active" IS TRUE'); + expect(result.text).toBe('NOT ("u"."active" IS TRUE)'); + }); + + test("brackets a NOT clause over several conditions", () => { + // without brackets the NOT would bind to the first condition only + const where = { [$not]: { active: true, id: 1 } }; + const result = buildWhere(db.config, [], table, where)!; + expect(result.text).toBe( + 'NOT ("u"."active" IS TRUE AND "u"."id" = $1)', + ); + expect(result.values).toEqual([1]); }); test("throws error for unrecognized field", () => { @@ -224,4 +234,64 @@ describe("buildWhere", () => { expect(result.text).toBe('"u"."active" IS TRUE AND "u"."id" = $1'); expect(result.values).toEqual([1]); }); + + describe("where middleware with logical operators", () => { + // a stand-in for the soft delete / org scope middleware: it injects a + // condition that must hold for every row the query touches + const middleware: Middleware[] = [ + { + where: (_config, _modelName, where) => ({ + active: true, + ...where, + }), + }, + ]; + + test("applies middleware once, outside a NOT clause", () => { + const where = { [$not]: { id: { [$in]: [1, 2] } } }; + const result = buildWhere(db.config, middleware, table, where)!; + + // the middleware's condition must not be negated: injecting it + // inside the NOT as well would give `active IS TRUE AND NOT + // (active IS TRUE AND ...)`, which no row can satisfy + expect(result.text).toBe( + '"u"."active" IS TRUE AND NOT ("u"."id" IN ($1, $2))', + ); + expect(result.values).toEqual([1, 2]); + }); + + test("applies middleware once, outside an AND clause", () => { + const where = { [$and]: [{ id: 1 }, { name: "test" }] }; + const result = buildWhere(db.config, middleware, table, where)!; + + expect(result.text).toBe( + '"u"."active" IS TRUE AND ("u"."id" = $1 AND "u"."name" = $2)', + ); + expect(result.values).toEqual([1, "test"]); + }); + + test("applies middleware once, outside an OR clause", () => { + const where = { [$or]: [{ id: 1 }, { name: "test" }] }; + const result = buildWhere(db.config, middleware, table, where)!; + + // the middleware's condition applies to the whole clause, rather + // than being repeated inside each branch of the OR + expect(result.text).toBe( + '"u"."active" IS TRUE AND ("u"."id" = $1 OR "u"."name" = $2)', + ); + expect(result.values).toEqual([1, "test"]); + }); + + test("applies middleware once to nested logical operators", () => { + const where = { + [$or]: [{ id: 1 }, { [$not]: { name: "test" } }], + }; + const result = buildWhere(db.config, middleware, table, where)!; + + expect(result.text).toBe( + '"u"."active" IS TRUE AND ("u"."id" = $1 OR NOT ("u"."name" = $2))', + ); + expect(result.values).toEqual([1, "test"]); + }); + }); }); diff --git a/packages/orm/src/builders/buildWhere.ts b/packages/orm/src/builders/buildWhere.ts index e8fa6ef..ced1a69 100644 --- a/packages/orm/src/builders/buildWhere.ts +++ b/packages/orm/src/builders/buildWhere.ts @@ -13,6 +13,15 @@ import { applyWhereMiddleware } from "../util/applyWhereMiddleware.js"; import { hasClauses } from "../util/hasClauses.js"; import { Table } from "./types.js"; +/** + * Builds the SQL for a where clause, applying any where middleware to it first. + * + * NB. the middleware is applied here, once, to the top-level clause only - not + * in `buildWhereClause` as it recurses. Injecting the middleware's conditions + * into every logical subclause would be redundant inside `$and` and `$or`, and + * actively wrong inside `$not`, where negating them contradicts the copy + * applied at the top level and the query matches nothing. + */ export const buildWhere = ( config: NormalizedConfig, middleware: Middleware[], @@ -22,11 +31,22 @@ export const buildWhere = ( OperatorDefinitions, ModelName >, -): SQLStatement | null => { - // Apply middleware to the where clause - const processedWhere = - applyWhereMiddleware(config, middleware, table.model, where) ?? {}; +): SQLStatement | null => + buildWhereClause( + config, + table, + applyWhereMiddleware(config, middleware, table.model, where) ?? {}, + ); +const buildWhereClause = ( + config: NormalizedConfig, + table: Table, + processedWhere: WhereClause< + ModelDefinitions, + OperatorDefinitions, + ModelName + >, +): SQLStatement | null => { if (!hasClauses(processedWhere)) { return null; } @@ -99,7 +119,7 @@ export const buildWhere = ( // logical operators if (processedWhere[$and]) { const subclauses = processedWhere[$and].map((clause) => { - const subclause = buildWhere(config, middleware, table, clause); + const subclause = buildWhereClause(config, table, clause); if (!subclause) { throw new Error("AND clause must not be empty"); } @@ -110,7 +130,7 @@ export const buildWhere = ( if (processedWhere[$or]) { const subclauses = processedWhere[$or].map((clause) => { - const subclause = buildWhere(config, middleware, table, clause); + const subclause = buildWhereClause(config, table, clause); if (!subclause) { throw new Error("OR clause must not be empty"); } @@ -120,16 +140,13 @@ export const buildWhere = ( } if (processedWhere[$not]) { - const subclause = buildWhere( - config, - middleware, - table, - processedWhere[$not], - ); + const subclause = buildWhereClause(config, table, processedWhere[$not]); if (!subclause) { throw new Error("NOT clause must not be empty"); } - clauses.push(sql`NOT ${subclause}`); + // the brackets matter: without them NOT binds to the first conjunct + // of the subclause only, rather than to the subclause as a whole + clauses.push(sql`NOT (${subclause})`); } return sql.join(clauses, " AND "); diff --git a/packages/orm/src/tests/orm.middleware.where.test.ts b/packages/orm/src/tests/orm.middleware.where.test.ts index 30895c9..b6eeeed 100644 --- a/packages/orm/src/tests/orm.middleware.where.test.ts +++ b/packages/orm/src/tests/orm.middleware.where.test.ts @@ -1095,6 +1095,75 @@ describe("where middleware", () => { { rollback: true }, ); }); + + test("does not apply middleware inside a $not clause", async () => { + await db.transact( + async (db) => { + await db.createMany("user", { + values: [ + { + id: 130, + name: "Keep", + email: "keep@example.com", + role: "user", + deletedAt: null, + }, + { + id: 131, + name: "Drop 1", + email: "drop1@example.com", + role: "user", + deletedAt: null, + }, + { + id: 132, + name: "Drop 2", + email: "drop2@example.com", + role: "user", + deletedAt: null, + }, + { + id: 133, + name: "Already Deleted", + email: "already-deleted@example.com", + role: "user", + deletedAt: new Date(), + }, + ], + }); + + // delete everything in this set except id 130. the + // middleware's `deletedAt IS NULL` must be applied once, to + // the top level: applied inside the $not as well it becomes + // `deletedAt IS NULL AND NOT (deletedAt IS NULL AND ...)`, + // which matches no row at all + const count = await db.deleteMany("user", { + where: { + id: { [$in]: [130, 131, 132, 133] }, + [$not]: { id: { [$in]: [130] } }, + }, + }); + + expect(count).toBe(2); + + const remaining = await db.findMany("user", { + where: { id: { [$in]: [130, 131, 132, 133] } }, + select: ["id"], + orderBy: ["id"], + }); + + // 130 excluded by the $not, 133 excluded by the middleware + expect(remaining).toEqual([{ id: 130 }]); + + const softDeleted = await db.findMany("user", { + where: { id: 133, deletedAt: { [$not]: null } }, + select: ["id"], + }); + expect(softDeleted).toEqual([{ id: 133 }]); + }, + { rollback: true }, + ); + }); }); describe("count", () => {