Fix $not where clauses matching no rows

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"]);
+        });
+    });
 });

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<ModelDefinitions>
     >,
-): 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<ModelDefinitions>
+    >,
+): 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 ");

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", () => {