Merge pull request 'fix(tests): close the route guard's factory hole and two regex warnings (#307)' (#312) from fix/307-sonar-cleanup into main

Reviewed-on: #312

backend/src/itemFilters.ts

@@ -307,7 +307,17 @@ export function itemFilterExpressions(
     // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits
     // the placeholder list itself, so one status and several use the same
     // expression and the explicit ::text[] cast is no longer needed.
-    clauses.push(eb('i.status', 'in', filters.status));
+    //
+    // The empty list is spelled out rather than left to `in`, which would emit
+    // `in ()` — a Postgres syntax error, where `= ANY` on an empty array was
+    // valid and matched nothing. Unreachable through parseItemFilters, which
+    // refuses a list that names nothing, but the obvious alternative is wrong
+    // in the opposite direction: dropping the clause entirely would make an
+    // empty status filter match *every* status, where the behaviour this
+    // replaced matched none. See #307.
+    clauses.push(
+      filters.status.length ? eb('i.status', 'in', filters.status) : sql<SqlBool>`false`
+    );
   }
 
   if (filters.favoritesOnly) {

backend/tests/integration/schemaMirror.integration.test.ts

@@ -30,7 +30,11 @@ const SCHEMA = readFileSync(
 function mirroredColumns(): Map<string, Set<string>> {
   const block = /export interface DB \{([^}]*)\}/.exec(SCHEMA)?.[1] ?? '';
   const byTable = new Map<string, Set<string>>();
-  for (const [, table, iface] of block.matchAll(/^\s*([a-z0-9_]+):\s*([A-Za-z0-9_]+);/gm)) {
+  // Literal spaces rather than `\s*`, and `\w` rather than `[A-Za-z0-9_]`.
+  // kysely-codegen indents two spaces and puts exactly one after the colon, so
+  // the looser form bought nothing and backtracked for it — SonarQube flagged
+  // both (#307).
+  for (const [, table, iface] of block.matchAll(/^ {2}([a-z0-9_]+): (\w+);/gm)) {
     const declaration =
       new RegExp(`export interface ${iface!} \\{([^}]*)\\}`).exec(SCHEMA)?.[1] ?? '';
     byTable.set(

backend/tests/unit/routesAreWrapped.test.ts

@@ -23,14 +23,18 @@ const SRC = join(__dirname, '..', '..', 'src');
 const REGISTRATION = /\b(?:app|\w*[Rr]outer)\.(?:get|post|put|patch|delete|all|use)\s*\(/g;
 
 /**
- * Returns the full text of the registration call starting at `open` (the index
- * of its `(`), by counting parens to the matching close. Quotes and comments
- * are skipped so a path like `'/items/:id'` or a `)` inside a string cannot
- * throw the count off.
+ * Returns the text from `start` to the delimiter matching the one it opens
+ * with, counting depth. Quotes and comments are skipped so a route path like
+ * `'/items/:id'`, or a brace inside a string, cannot throw the count off.
+ *
+ * Parameterised over the delimiter pair because two callers need the same walk:
+ * a registration is bounded by parens and a function body by braces, and #307
+ * added the second one as a near-copy of the first before this was factored.
  */
-function registrationAt(source: string, open: number): string {
+function matchedSpan(source: string, start: number, open: string, close: string): string {
   let depth = 0;
-  for (let i = open; i < source.length; i++) {
+
+  for (let i = start; i < source.length; i++) {
     const c = source[i];
 
     if (c === "'" || c === '"' || c === '`') {
@@ -47,13 +51,22 @@ function registrationAt(source: string, open: number): string {
       continue;
     }
 
-    if (c === '(') depth++;
-    if (c === ')') {
+    if (c === open) depth++;
+    if (c === close) {
       depth--;
-      if (depth === 0) return source.slice(open, i + 1);
+      if (depth === 0) return source.slice(start, i + 1);
     }
   }
-  return source.slice(open);
+
+  return source.slice(start);
+}
+
+/**
+ * The full text of the registration call starting at `open`, the index of its
+ * `(`.
+ */
+function registrationAt(source: string, open: number): string {
+  return matchedSpan(source, open, '(', ')');
 }
 
 /** Returns the index of the closing quote of the string opening at `start`. */
@@ -69,8 +82,53 @@ function skipString(source: string, start: number): number {
   return source.length;
 }
 
+/** The body of the function declared at `declared`, braces included. */
+function bodyOf(source: string, declared: number): string {
+  const start = source.indexOf('{', declared);
+  return start === -1 ? '' : matchedSpan(source, start, '{', '}');
+}
+
 /**
- * Every `async` inside a registration must sit directly behind `asyncRoute(`.
+ * The bodies of any handler factories a registration calls.
+ *
+ * `router.post('/x', rotationRoute('left'))` carries no `async` token of its
+ * own, so the token check below passes it by finding nothing at all. That is
+ * exactly how the two rotation routes added in #301 sailed through a guard that
+ * exists because this convention had already been half-forgotten once — it did
+ * not find them wrapped, it found nothing and said yes. The handler lives in
+ * the factory, so the factory is what has to be read.
+ *
+ * Only functions declared in the same file are followed. `express.json()` and
+ * `cookieParser()` are imported and are not handler factories at all; there is
+ * no honest way to resolve those textually, and guessing at them would trade
+ * this hole for false positives on every library call in app.ts.
+ */
+function factoryBodies(source: string, call: string): string[] {
+  const bodies: string[] = [];
+
+  for (const [, name] of call.matchAll(/\b([a-zA-Z_]\w*)\s*\(/g)) {
+    if (name === 'asyncRoute') continue;
+
+    const declared = new RegExp(`function\\s+${name}\\s*\\(`).exec(source);
+    if (!declared) continue;
+
+    bodies.push(bodyOf(source, declared.index));
+  }
+
+  return bodies;
+}
+
+/** Whether `text` contains an `async` that is not directly behind `asyncRoute(`. */
+function hasUnwrappedAsync(text: string): boolean {
+  for (const found of text.matchAll(/\basync\b/g)) {
+    if (!text.slice(0, found.index!).trimEnd().endsWith('asyncRoute(')) return true;
+  }
+  return false;
+}
+
+/**
+ * Every `async` inside a registration must sit directly behind `asyncRoute(`,
+ * and so must every `async` inside a factory that registration calls.
  * Checking the token rather than the line catches a handler whose `async`
  * lands on its own line, which a line-oriented grep would wave through.
  */
@@ -81,11 +139,9 @@ function unwrappedHandlers(source: string): string[] {
     const open = match.index! + match[0].length - 1;
     const call = registrationAt(source, open);
 
-    for (const found of call.matchAll(/\basync\b/g)) {
-      const before = call.slice(0, found.index!).trimEnd();
-      if (!before.endsWith('asyncRoute(')) {
-        offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`);
-      }
+    const suspect = [call, ...factoryBodies(source, call)];
+    if (suspect.some(hasUnwrappedAsync)) {
+      offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`);
     }
   }
 
@@ -151,4 +207,40 @@ describe('the guard itself', () => {
 
     expect(unwrappedHandlers(sync)).toEqual([]);
   });
+
+  // The hole #307 closed. A registration built by a factory carries no `async`
+  // of its own, so before this the guard looked at these two lines, found
+  // nothing to object to, and passed — which is not the same as finding them
+  // wrapped. The rotation routes in #301 were the first factory in src/routes
+  // and went through exactly this way.
+  it('follows a factory to the handler it returns', () => {
+    const bad = `
+      function rotationRoute(direction) {
+        return async (req, res) => { res.json({ direction }); };
+      }
+      router.post('/rotate-left', rotationRoute('left'));`;
+
+    expect(unwrappedHandlers(bad)).toHaveLength(1);
+  });
+
+  it('accepts a factory that returns a wrapped handler', () => {
+    const good = `
+      function rotationRoute(direction) {
+        return asyncRoute(async (req, res) => { res.json({ direction }); });
+      }
+      router.post('/rotate-left', rotationRoute('left'));`;
+
+    expect(unwrappedHandlers(good)).toEqual([]);
+  });
+
+  // Imported calls are left alone deliberately. app.ts registers
+  // `express.json()`, `cookieParser()` and `uploadsRouter()`, none of which is
+  // a handler factory and none of which can be read from this file — treating
+  // an unresolvable name as an offender would trade one hole for a permanently
+  // red test.
+  it('ignores a call it cannot resolve in the same file', () => {
+    const imported = `router.use('/uploads', uploadsRouter());`;
+
+    expect(unwrappedHandlers(imported)).toEqual([]);
+  });
 });