testing ADR - architectural decisions

src/middleware/errorHandler.ts

@@ -58,12 +58,12 @@ export const errorHandler = (err: HttpError, req: Request, res: Response, next:
   // Refine the status code for known error types. Check for most specific types first.
   if (err instanceof UniqueConstraintError) {
     statusCode = 409; // Conflict
-  } else if (err instanceof ForeignKeyConstraintError) {
-    statusCode = 400;
   } else if (err instanceof NotFoundError) {
     statusCode = 404;
+  } else if (err instanceof ForeignKeyConstraintError) {
+    statusCode = 400;
   } else if (err instanceof DatabaseError) {
-    // This is a generic fallback for other database errors.
+    // This is a generic fallback for other database errors that are not the specific subclasses above.
     statusCode = err.status;
   } else if (err.name === 'UnauthorizedError') {
     statusCode = err.status || 401;

src/middleware/validation.middleware.test.ts

@@ -0,0 +1,104 @@
+// src/middleware/validation.middleware.test.ts
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import supertest from 'supertest';
+import express, { Request, Response } from 'express';
+import { z } from 'zod';
+import { validateRequest } from './validation.middleware';
+import { errorHandler } from './errorHandler';
+import { ValidationError } from '../services/db/errors.db';
+
+// Mock the logger to prevent console output during tests
+vi.mock('../services/logger.server', () => ({
+  logger: {
+    error: vi.fn(),
+    warn: vi.fn(),
+  },
+}));
+
+// 1. Create a minimal Express app for testing the middleware
+const app = express();
+app.use(express.json());
+
+// 2. Define a sample Zod schema for a test route
+const testSchema = z.object({
+  params: z.object({
+    id: z.coerce.number().int().positive('ID must be a positive integer.'),
+  }),
+  query: z.object({
+    limit: z.coerce.number().int().min(1).optional().default(10),
+  }),
+  body: z.object({
+    name: z.string().min(3, 'Name must be at least 3 characters long.'),
+    is_active: z.boolean().optional(),
+  }),
+});
+
+// 3. Setup a test route that uses the validation middleware
+app.post(
+  '/test/:id',
+  validateRequest(testSchema),
+  (req: Request, res: Response) => {
+    // This handler only runs if validation succeeds.
+    // We send back the parsed data to confirm it was correctly attached to the request.
+    res.status(200).json({
+      params: req.params,
+      query: req.query,
+      body: req.body,
+    });
+  }
+);
+
+// 4. Apply the actual global errorHandler to handle ValidationErrors thrown by the middleware
+app.use(errorHandler);
+
+describe('validateRequest Middleware', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('should call next() and attach parsed data for a valid request', async () => {
+    const requestBody = { name: 'Test Item', is_active: true };
+    const response = await supertest(app)
+      .post('/test/123?limit=5')
+      .send(requestBody);
+
+    expect(response.status).toBe(200);
+    // Check that data was coerced and correctly typed
+    expect(response.body).toEqual({
+      params: { id: 123 }, // Coerced to number
+      query: { limit: 5 }, // Coerced to number
+      body: requestBody,
+    });
+  });
+
+  it('should return a 400 error for invalid params', async () => {
+    const response = await supertest(app)
+      .post('/test/abc') // 'abc' is not a valid number
+      .send({ name: 'Valid Name' });
+
+    expect(response.status).toBe(400);
+    expect(response.body.message).toBe('The request data is invalid.');
+    expect(response.body.errors[0].path).toEqual(['params', 'id']);
+  });
+
+  it('should return a 400 error for invalid query parameters', async () => {
+    const response = await supertest(app)
+      .post('/test/123?limit=-1') // -1 is not a valid limit
+      .send({ name: 'Valid Name' });
+
+    expect(response.status).toBe(400);
+    expect(response.body.message).toBe('The request data is invalid.');
+    expect(response.body.errors[0].path).toEqual(['query', 'limit']);
+  });
+
+  it('should return a 400 error for an invalid request body', async () => {
+    const response = await supertest(app)
+      .post('/test/123')
+      .send({ name: 'a' }); // 'a' is too short
+
+    expect(response.status).toBe(400);
+    expect(response.body.message).toBe('The request data is invalid.');
+    expect(response.body.errors[0].path).toEqual(['body', 'name']);
+    expect(response.body.errors[0].message).toBe('Name must be at least 3 characters long.');
+  });
+});

src/routes/user.routes.test.ts

@@ -295,6 +295,7 @@ describe('User Routes (/api/users)', () => {
 
         it('should return 404 if list to delete is not found', async () => {
           vi.mocked(db.shoppingRepo.deleteShoppingList).mockRejectedValue(new Error('not found'));
+          vi.mocked(db.shoppingRepo.deleteShoppingList).mockRejectedValue(new NotFoundError('not found'));
           const response = await supertest(app).delete('/api/users/shopping-lists/999');
           expect(response.status).toBe(404);
         });
@@ -355,6 +356,7 @@ describe('User Routes (/api/users)', () => {
 
       it('should return 404 if item to update is not found', async () => {
         vi.mocked(db.shoppingRepo.updateShoppingListItem).mockRejectedValue(new Error('not found'));
+        vi.mocked(db.shoppingRepo.updateShoppingListItem).mockRejectedValue(new NotFoundError('not found'));
         const response = await supertest(app).put('/api/users/shopping-lists/items/999').send({ is_purchased: true });
         expect(response.status).toBe(404);
       });
@@ -392,6 +394,7 @@ describe('User Routes (/api/users)', () => {
 
         it('should return 404 if item to delete is not found', async () => {
           vi.mocked(db.shoppingRepo.removeShoppingListItem).mockRejectedValue(new Error('not found'));
+          vi.mocked(db.shoppingRepo.removeShoppingListItem).mockRejectedValue(new NotFoundError('not found'));
           const response = await supertest(app).delete('/api/users/shopping-lists/items/999');
           expect(response.status).toBe(404);
         });
@@ -843,13 +846,14 @@ describe('User Routes (/api/users)', () => {
 
       it('DELETE /recipes/:recipeId should return 404 if recipe to delete is not found', async () => {
         vi.mocked(db.recipeRepo.deleteRecipe).mockRejectedValue(new Error('not found'));
+        vi.mocked(db.recipeRepo.deleteRecipe).mockRejectedValue(new NotFoundError('not found'));
         const response = await supertest(app).delete('/api/users/recipes/999');
         expect(response.status).toBe(404);
       });
 
       it('PUT /recipes/:recipeId should update a user\'s own recipe', async () => {
         const updates = { description: 'A new delicious description.' };
-        const mockUpdatedRecipe = { ...createMockRecipe(), ...updates };
+        const mockUpdatedRecipe = { ...createMockRecipe({ recipe_id: 1 }), ...updates };
         vi.mocked(db.recipeRepo.updateRecipe).mockResolvedValue(mockUpdatedRecipe);
 
         const response = await supertest(app)
@@ -863,6 +867,7 @@ describe('User Routes (/api/users)', () => {
 
       it('PUT /recipes/:recipeId should return 404 if recipe not found', async () => {
         vi.mocked(db.recipeRepo.updateRecipe).mockRejectedValue(new Error('not found'));
+        vi.mocked(db.recipeRepo.updateRecipe).mockRejectedValue(new NotFoundError('not found'));
         const response = await supertest(app).put('/api/users/recipes/999').send({ name: 'New Name' });
         expect(response.status).toBe(404);
       });

src/services/db/0001-standardized-error-handling.md

@@ -8,10 +8,10 @@
 
 Our application has experienced a recurring pattern of bugs and brittle tests related to error handling, specifically for "resource not found" scenarios. The root causes identified are:
 
-1.  **Inconsistent Return Types**: Database repository methods that fetch a single entity (e.g., `getUserById`, `getRecipeById`) had inconsistent behavior when an entity was not found. Some returned `undefined`, some returned `null`, and others threw a generic `Error`.
-2.  **Burden on Callers**: This inconsistency forced route handlers (the callers) to implement defensive checks for `undefined` or `null` before sending a response. These checks were often forgotten or implemented incorrectly.
-3.  **Incorrect HTTP Status Codes**: When a route handler forgot to check for an `undefined` result and passed it to `res.json()`, the Express framework would interpret this as a server-side failure, resulting in an incorrect `500 Internal Server Error` instead of the correct `404 Not Found`.
-4.  **Brittle Tests**: Unit and integration tests for routes were unreliable. Mocks often threw a generic `new Error()` when the actual implementation returned `undefined` or a specific custom error, leading to unexpected `500` status codes in test environments.
+1. **Inconsistent Return Types**: Database repository methods that fetch a single entity (e.g., `getUserById`, `getRecipeById`) had inconsistent behavior when an entity was not found. Some returned `undefined`, some returned `null`, and others threw a generic `Error`.
+2. **Burden on Callers**: This inconsistency forced route handlers (the callers) to implement defensive checks for `undefined` or `null` before sending a response. These checks were often forgotten or implemented incorrectly.
+3. **Incorrect HTTP Status Codes**: When a route handler forgot to check for an `undefined` result and passed it to `res.json()`, the Express framework would interpret this as a server-side failure, resulting in an incorrect `500 Internal Server Error` instead of the correct `404 Not Found`.
+4. **Brittle Tests**: Unit and integration tests for routes were unreliable. Mocks often threw a generic `new Error()` when the actual implementation returned `undefined` or a specific custom error, leading to unexpected `500` status codes in test environments.
 
 This pattern led to increased development friction, difficult-to-diagnose bugs, and a fragile test suite.
 
@@ -19,13 +19,13 @@ This pattern led to increased development friction, difficult-to-diagnose bugs,
 
 We will adopt a strict, consistent error-handling contract for the service and repository layers.
 
-1.  **Always Throw on Not Found**: Any function or method responsible for fetching a single, specific resource (e.g., by ID, checksum, or other unique identifier) **MUST** throw a `NotFoundError` if that resource does not exist. It **MUST NOT** return `null` or `undefined` to signify absence.
+1. **Always Throw on Not Found**: Any function or method responsible for fetching a single, specific resource (e.g., by ID, checksum, or other unique identifier) **MUST** throw a `NotFoundError` if that resource does not exist. It **MUST NOT** return `null` or `undefined` to signify absence.
 
-2.  **Use Specific, Custom Errors**: For other known, predictable failure modes (e.g., unique constraint violations, foreign key violations), the repository layer **MUST** throw the corresponding custom `DatabaseError` subclass (e.g., `UniqueConstraintError`, `ForeignKeyConstraintError`).
+2. **Use Specific, Custom Errors**: For other known, predictable failure modes (e.g., unique constraint violations, foreign key violations), the repository layer **MUST** throw the corresponding custom `DatabaseError` subclass (e.g., `UniqueConstraintError`, `ForeignKeyConstraintError`).
 
-3.  **Centralize HTTP Status Mapping**: The `errorHandler` middleware is the **single source of truth** for mapping these specific error types to their corresponding HTTP status codes (e.g., `NotFoundError` -> 404, `UniqueConstraintError` -> 409).
+3. **Centralize HTTP Status Mapping**: The `errorHandler` middleware is the **single source of truth** for mapping these specific error types to their corresponding HTTP status codes (e.g., `NotFoundError` -> 404, `UniqueConstraintError` -> 409).
 
-4.  **Simplify Route Handlers**: Route handlers should be simplified to use a standard `try...catch` block. All errors caught from the service/repository layer should be passed directly to `next(error)`, relying on the `errorHandler` middleware to format the final response. No special `if (result === undefined)` checks are needed.
+4. **Simplify Route Handlers**: Route handlers should be simplified to use a standard `try...catch` block. All errors caught from the service/repository layer should be passed directly to `next(error)`, relying on the `errorHandler` middleware to format the final response. No special `if (result === undefined)` checks are needed.
 
 ## Consequences
 
@@ -40,4 +40,4 @@ We will adopt a strict, consistent error-handling contract for the service and r
 ### Negative
 
 **Initial Refactoring**: Requires a one-time effort to audit and refactor all existing repository methods to conform to this new standard.
-**Convention Adherence**: Developers must be aware of and adhere to this convention. This ADR serves as the primary documentation for this pattern.
+**Convention Adherence**: Developers must be aware of and adhere to this convention. This ADR serves as the primary documentation for this pattern.

src/services/db/errors.db.ts

@@ -48,7 +48,11 @@ export class NotFoundError extends DatabaseError {
  * Thrown when request validation fails (e.g., missing body fields or invalid params).
  */
 export class ValidationError extends DatabaseError {
-  constructor(message = 'The request data is invalid.') {
+  public validationErrors: any[];
+
+  constructor(errors: any[], message = 'The request data is invalid.') {
     super(message, 400); // 400 Bad Request
+    this.name = 'ValidationError';
+    this.validationErrors = errors;
   }
 }