diff --git a/docs/adr/0001-standardized-error-handling.md b/docs/adr/0001-standardized-error-handling.md new file mode 100644 index 00000000..a495135f --- /dev/null +++ b/docs/adr/0001-standardized-error-handling.md @@ -0,0 +1,43 @@ +# ADR-001: Standardized Error Handling for Service and Repository Layers + +**Date**: 2025-12-12 + +**Status**: Accepted + +## Context + +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. + +This pattern led to increased development friction, difficult-to-diagnose bugs, and a fragile test suite. + +## Decision + +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. + +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). + +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 + +### Positive + +* **Robustness**: Eliminates an entire class of bugs where `undefined` is passed to `res.json()`, preventing incorrect `500` errors. +* **Consistency & Predictability**: All data-fetching methods now have a predictable contract. They either return the expected data or throw a specific, typed error. +* **Developer Experience**: Route handlers become simpler, cleaner, and easier to write correctly. The cognitive load on developers is reduced as they no longer need to remember to check for `undefined`. +* **Improved Testability**: Tests become more reliable and realistic. Mocks can now throw the *exact* error type (`new NotFoundError()`) that the real implementation would, ensuring tests accurately reflect the application's behavior. +* **Centralized Control**: Error-to-HTTP-status logic is centralized in the `errorHandler` middleware, making it easy to manage and modify error responses globally. + +### 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. \ No newline at end of file diff --git a/docs/adr/0002-standardized-transaction-management.md b/docs/adr/0002-standardized-transaction-management.md new file mode 100644 index 00000000..5a9a1f82 --- /dev/null +++ b/docs/adr/0002-standardized-transaction-management.md @@ -0,0 +1,62 @@ +# ADR-002: Standardized Transaction Management and Unit of Work Pattern + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +Following the standardization of error handling in ADR-001, the next most common source of architectural inconsistency and potential bugs is database transaction management. Currently, several repository methods (`createUser`, `createFlyerAndItems`, etc.) implement their own transaction logic by manually acquiring a client from the connection pool, and then managing `BEGIN`, `COMMIT`, and `ROLLBACK` states. + +This manual approach has several drawbacks: +1. **Repetitive Boilerplate**: The `try/catch/finally` block for transaction management is duplicated across multiple files. +2. **Error-Prone**: It is easy to forget to `client.release()` in all code paths, which can lead to connection pool exhaustion and bring down the application. +3. **Poor Composability**: It is difficult to compose multiple repository methods into a single, atomic "Unit of Work". For example, a service function that needs to update a user's points and create a budget in a single transaction cannot easily do so if both underlying repository methods create their own transactions. + +## Decision + +We will implement a standardized "Unit of Work" pattern through a high-level `withTransaction` helper function. This function will abstract away the complexity of transaction management. + +1. **`withTransaction` Helper**: A new helper function, `withTransaction(callback: (client: PoolClient) => Promise): Promise`, will be created. This function will be responsible for: + * Acquiring a client from the database pool. + * Starting a transaction (`BEGIN`). + * Executing the `callback` function, passing the transactional client to it. + * If the callback succeeds, it will `COMMIT` the transaction. + * If the callback throws an error, it will `ROLLBACK` the transaction and re-throw the error. + * In all cases, it will `RELEASE` the client back to the pool. + +2. **Repository Method Signature**: Repository methods that need to be part of a transaction will be updated to optionally accept a `PoolClient` in their constructor or as a method parameter. By default, they will use the global pool. When called from within a `withTransaction` block, they will be passed the transactional client. + +3. **Service Layer Orchestration**: Service-layer functions that orchestrate multi-step operations will use `withTransaction` to ensure atomicity. They will instantiate or call repository methods, providing them with the transactional client from the callback. + +### Example Usage: + +```typescript +// In a service file... +async function registerUserAndCreateDefaultList(userData) { + return withTransaction(async (client) => { + // Pass the transactional client to the repositories + const userRepo = new UserRepository(client); + const shoppingRepo = new ShoppingRepository(client); + + const newUser = await userRepo.createUser(userData); + await shoppingRepo.createShoppingList(newUser.user_id, "My First List"); + + return newUser; + }); +} +``` + +## Consequences + +### Positive + +* **DRY (Don't Repeat Yourself)**: Transaction logic is defined in one place, eliminating boilerplate from repository and service files. +* **Safety and Reliability**: Guarantees that clients are always released, preventing connection leaks. Ensures proper rollback on any error. +* **Clear Composability**: Provides a clear and explicit pattern for composing multiple database operations into a single atomic unit. +* **Improved Readability**: Service logic becomes cleaner, focusing on the business operations rather than the mechanics of transaction management. + +### Negative + +* **Learning Curve**: Developers will need to learn and adopt the `withTransaction` pattern for all transactional database work. +* **Refactoring Effort**: Existing methods that manually manage transactions (`createUser`, `createBudget`, etc.) will need to be refactored to use the new pattern. \ No newline at end of file diff --git a/docs/adr/0003-standardized-input-validation-using-middleware.md b/docs/adr/0003-standardized-input-validation-using-middleware.md new file mode 100644 index 00000000..a7cb6484 --- /dev/null +++ b/docs/adr/0003-standardized-input-validation-using-middleware.md @@ -0,0 +1,70 @@ +# ADR-003: Standardized Input Validation using Middleware + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +Our Express route handlers currently perform manual validation of request parameters, queries, and bodies. This involves repetitive boilerplate code using `parseInt`, `isNaN`, and type checks like `Array.isArray`. This approach has several disadvantages: + +1. **Code Duplication**: The same validation logic (e.g., checking for a valid integer ID) is repeated across many different routes. +2. **Cluttered Business Logic**: Route handlers are cluttered with validation code, obscuring their primary business logic. +3. **Inconsistent Error Messages**: Manual validation can lead to inconsistent error messages for similar validation failures across the API. +4. **Error-Prone**: It is easy to forget a validation check, leading to unexpected data types being passed to service and repository layers, which could cause runtime errors. + +## Decision + +We will adopt a schema-based approach for input validation using the `zod` library and a custom Express middleware. + +1. **Adopt `zod` for Schema Definition**: We will use `zod` to define clear, type-safe schemas for the `params`, `query`, and `body` of each API request. `zod` provides powerful and declarative validation rules and automatically infers TypeScript types. + +2. **Create a Reusable Validation Middleware**: A generic `validateRequest(schema)` middleware will be created. This middleware will take a `zod` schema, parse the incoming request against it, and handle success and error cases. + * On successful validation, the parsed and typed data will be attached to the `req` object (e.g., `req.body` will be replaced with the parsed body), and `next()` will be called. + * On validation failure, the middleware will call `next()` with a custom `ValidationError` containing a structured list of issues, which `ADR-001`'s `errorHandler` can then format into a user-friendly `400 Bad Request` response. + +3. **Refactor Routes**: All route handlers will be refactored to use this new middleware, removing all manual validation logic. + +### Example Usage: + +```typescript +// In flyer.routes.ts + +import { z } from 'zod'; +import { validateRequest } from '../middleware/validation'; + +// Define the schema for the GET /:id route +const getFlyerSchema = z.object({ + params: z.object({ + id: z.string().pipe(z.coerce.number().int().positive()), + }), +}); + +// Apply the middleware to the route +router.get('/:id', validateRequest(getFlyerSchema), async (req, res, next) => { + try { + // req.params.id is now guaranteed to be a positive number + const flyerId = req.params.id; + const flyer = await db.flyerRepo.getFlyerById(flyerId); + res.json(flyer); + } catch (error) { + next(error); + } +}); +``` + +## Consequences + +### Positive + +* **DRY and Declarative**: Validation logic is defined once in a schema and removed from route handlers. +* **Improved Readability**: Route handlers become much cleaner and focus exclusively on their core business logic. +* **Type Safety**: `zod` schemas provide strong compile-time and runtime type safety, reducing bugs. +* **Consistent and Detailed Errors**: The `errorHandler` can be configured to provide consistent, detailed validation error messages for all routes (e.g., "Query parameter 'limit' must be a positive integer"). +* **Robustness**: Prevents invalid data from ever reaching the service or database layers. + +### Negative + +* **New Dependency**: Introduces `zod` as a new project dependency. +* **Learning Curve**: Developers need to learn the `zod` schema definition syntax. +* **Refactoring Effort**: Requires a one-time effort to create schemas and refactor all existing routes to use the `validateRequest` middleware. \ No newline at end of file diff --git a/docs/adr/0004-standardized-application-wide-structured-logging.md b/docs/adr/0004-standardized-application-wide-structured-logging.md new file mode 100644 index 00000000..6cfc1c82 --- /dev/null +++ b/docs/adr/0004-standardized-application-wide-structured-logging.md @@ -0,0 +1,85 @@ +# ADR-004: Standardized Application-Wide Structured Logging + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +Our application currently uses a `logger` service, but the implementation of logging varies significantly across different modules. The `errorHandler` middleware produces high-quality, structured JSON logs for errors, but logging within route handlers and service layers is often ad-hoc, using plain strings or inconsistent object structures. + +This inconsistency leads to several problems: +1. **Difficult Debugging**: It is hard to trace a single user request through the system or correlate events related to a specific operation. +2. **Ineffective Log Analysis**: Inconsistent log formats make it difficult to effectively query, filter, and create dashboards in a log management system (like Datadog, Splunk, or the ELK stack). +3. **Security Risks**: There is no enforced standard for redacting sensitive information (like passwords or tokens) in logs outside of the `errorHandler`, increasing the risk of accidental data exposure. +4. **Missing Context**: Logs often lack crucial context, such as a unique request ID, the authenticated user's ID, or the source IP address, making them less useful for diagnosing issues. + +## Decision + +We will adopt a standardized, application-wide structured logging policy. All log entries MUST be in JSON format and adhere to a consistent schema. + +1. **Request-Scoped Logger with Context**: We will create a middleware that runs at the beginning of the request lifecycle. This middleware will: + * Generate a unique `request_id` for each incoming request. + * Create a request-scoped logger instance (a "child logger") that automatically includes the `request_id`, `user_id` (if authenticated), and `ip_address` in every log message it generates. + * Attach this child logger to the `req` object (e.g., `req.log`). + +2. **Mandatory Use of Request-Scoped Logger**: All route handlers and any service functions called by them **MUST** use the request-scoped logger (`req.log`) instead of the global logger instance. This ensures all logs for a given request are automatically correlated. + +3. **Standardized Log Schema**: All log messages should follow a base schema. The logger configuration will be updated to enforce this. + * **Base Fields**: `level`, `timestamp`, `message`, `request_id`, `user_id`, `ip_address`. + * **Error Fields**: When logging an error, the log entry MUST include an `error` object with `name`, `message`, and `stack`. + +4. **Standardized Logging Practices**: + * **INFO**: Log key business events, such as `User logged in` or `Flyer processed`. + * **WARN**: Log recoverable errors or unusual situations that do not break the request, such as `Client Error: 404 on GET /api/non-existent-route` or `Retrying failed database connection`. + * **ERROR**: Log only unhandled or server-side errors that cause a request to fail (typically handled by the `errorHandler`). Avoid logging expected client errors (like 4xx) at this level. + * **DEBUG**: Log detailed diagnostic information useful during development, such as function entry/exit points or variable states. + +### Example Usage: + +```typescript +// In a new middleware file: logger.middleware.ts +import { logger } from '../services/logger.server'; +import { randomUUID } from 'crypto'; + +export const requestLogger = (req, res, next) => { + const requestId = randomUUID(); + // Create a child logger with context for this request + req.log = logger.child({ + request_id: requestId, + user_id: req.user?.user_id, // Assumes user is attached by auth middleware + ip_address: req.ip, + }); + next(); +}; + +// In server/app setup: +// app.use(requestLogger); // Add this early in the middleware chain + +// In a route handler: +router.get('/:id', async (req, res, next) => { + // Use the request-scoped logger + req.log.info({ flyerId: req.params.id }, 'Fetching flyer by ID'); + try { + // ... business logic ... + res.json(flyer); + } catch (error) { + // The error itself will be logged with full context by the errorHandler + next(error); + } +}); +``` + +## Consequences + +### Positive + +* **Enhanced Observability**: Every log line from a single request can be instantly grouped and analyzed, dramatically speeding up debugging. +* **Improved Security**: Centralizing the addition of context (like `user_id`) reduces the chance of developers manually logging sensitive data. +* **Scalable Log Management**: Consistent JSON logs are easily ingested and indexed by any modern log aggregation tool. +* **Clearer Code**: Removes the need to manually pass contextual information (like user ID) down to service functions just for logging purposes. + +### Negative + +* **Refactoring Effort**: Requires adding the `requestLogger` middleware and refactoring all routes and services to use `req.log` instead of the global `logger`. +* **Slight Performance Overhead**: Creating a child logger for every request adds a minor performance cost, though this is negligible for most modern logging libraries. \ No newline at end of file diff --git a/docs/adr/0005-frontend-state-management-and-server-cache-strategy.md b/docs/adr/0005-frontend-state-management-and-server-cache-strategy.md new file mode 100644 index 00000000..94267f35 --- /dev/null +++ b/docs/adr/0005-frontend-state-management-and-server-cache-strategy.md @@ -0,0 +1,18 @@ +# ADR-005: Frontend State Management and Server Cache Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The frontend currently uses React's built-in hooks (`useState`, `useEffect`, `useContext`) for state management, as seen in `useAuth.tsx`. While effective for simple cases, this manual approach becomes complex and error-prone when fetching, caching, and synchronizing data with the server. It often leads to custom logic for loading states, errors, re-fetching, and optimistic updates. + +## Decision + +We will adopt a dedicated library for managing server state, such as **TanStack Query (formerly React Query)** or **SWR**, for all server-side data fetching on the client. This will abstract away the complexities of caching, background re-validation, and request deduplication. + +## Consequences + +* **Positive**: Leads to a more performant, predictable, and simpler frontend codebase. Standardizes how the client-side communicates with the server and handles loading/error states. Improves user experience through intelligent caching. +* **Negative**: Introduces a new frontend dependency. Requires a learning curve for developers unfamiliar with the library. Requires refactoring of existing data-fetching logic. \ No newline at end of file diff --git a/docs/adr/0006-background-job-processing-and-task-queues.md b/docs/adr/0006-background-job-processing-and-task-queues.md new file mode 100644 index 00000000..c2c8a8c1 --- /dev/null +++ b/docs/adr/0006-background-job-processing-and-task-queues.md @@ -0,0 +1,18 @@ +# ADR-006: Background Job Processing and Task Queues + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The application's core purpose involves long-running, resource-intensive tasks like OCR processing of flyers. Executing these tasks within the lifecycle of an API request is unreliable and does not scale. A failure during processing could be lost without a robust system. + +## Decision + +We will implement a dedicated background job processing system using a task queue library like **BullMQ** (with Redis). All flyer ingestion, OCR processing, and other long-running tasks (e.g., sending bulk emails) MUST be dispatched as jobs to this queue. + +## Consequences + +* **Positive**: Decouples the API from heavy processing, allows for retries on failure, and enables scaling the processing workers independently. Increases application reliability and resilience. +* **Negative**: Introduces a new dependency (Redis) into the infrastructure. Requires refactoring of the flyer processing logic to work within a job queue structure. \ No newline at end of file diff --git a/docs/adr/0007-configuration-and-secrets-management.md b/docs/adr/0007-configuration-and-secrets-management.md new file mode 100644 index 00000000..a6c47f69 --- /dev/null +++ b/docs/adr/0007-configuration-and-secrets-management.md @@ -0,0 +1,18 @@ +# ADR-007: Configuration and Secrets Management + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The application currently accesses environment variables directly via `process.env`. This can lead to missing variables at runtime, inconsistent naming, and a lack of type safety. It is difficult to know at a glance which environment variables are required for the application to run. + +## Decision + +We will introduce a centralized, schema-validated configuration service. We will use a library like `zod` to define a schema for all required environment variables. A singleton service will parse `process.env` at application startup, validate it against the schema, and provide a type-safe configuration object to the rest of the app. The application will fail fast on startup if any required configuration is missing or invalid. + +## Consequences + +* **Positive**: Improves application reliability and developer experience by catching configuration errors at startup rather than at runtime. Provides a single source of truth for all required configuration. +* **Negative**: Adds a small amount of boilerplate for defining the configuration schema. Requires a one-time effort to refactor all `process.env` access points to use the new configuration service. \ No newline at end of file diff --git a/docs/adr/0008-api-versioning-strategy.md b/docs/adr/0008-api-versioning-strategy.md new file mode 100644 index 00000000..5df65e23 --- /dev/null +++ b/docs/adr/0008-api-versioning-strategy.md @@ -0,0 +1,18 @@ +# ADR-008: API Versioning Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +As the application grows, the API will need to evolve. Making breaking changes to existing endpoints can disrupt clients (e.g., a mobile app or the web frontend). The current routing has no formal versioning scheme. + +## Decision + +We will adopt a URI-based versioning strategy for the API. All new and existing routes will be prefixed with a version number (e.g., `/api/v1/flyers`). This ADR establishes a clear policy for when to introduce a new version (`v2`) and how to manage deprecation of old versions. + +## Consequences + +* **Positive**: Establishes a critical pattern for long-term maintainability. Allows the API to evolve without breaking existing clients. +* **Negative**: Adds a small amount of complexity to the routing setup. Requires discipline to manage versions and deprecations correctly. \ No newline at end of file diff --git a/docs/adr/0009-caching-strategy-for-read-heavy-operations.md b/docs/adr/0009-caching-strategy-for-read-heavy-operations.md new file mode 100644 index 00000000..a7ff0dbd --- /dev/null +++ b/docs/adr/0009-caching-strategy-for-read-heavy-operations.md @@ -0,0 +1,21 @@ +# ADR-009: Caching Strategy for Read-Heavy Operations + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The application has several read-heavy endpoints (e.g., getting flyer items, recipes, brands). As traffic increases, these endpoints will put significant load on the database, even for data that changes infrequently. + +## Decision + +We will implement a multi-layered caching strategy using an in-memory data store like **Redis**. +1. **Define Cacheable Data**: Identify data suitable for caching (e.g., flyer data, recipe details, brand lists). +2. **Define Invalidation Strategy**: Determine the cache invalidation strategy (e.g., time-to-live (TTL), event-based invalidation on data update). +3. **Implement Cache-Aside Pattern**: The repository layer will be updated to implement a "Cache-Aside" pattern, where methods first check Redis for data before falling back to the database. + +## Consequences + +* **Positive**: Directly addresses application performance and scalability. Reduces database load and improves API response times for common requests. +* **Negative**: Introduces Redis as a dependency if not already used. Adds complexity to the data-fetching logic and requires careful management of cache invalidation to prevent stale data. \ No newline at end of file diff --git a/docs/adr/0010-testing-strategy-and-standards.md b/docs/adr/0010-testing-strategy-and-standards.md new file mode 100644 index 00000000..f5d2e7f0 --- /dev/null +++ b/docs/adr/0010-testing-strategy-and-standards.md @@ -0,0 +1,21 @@ +# ADR-010: Testing Strategy and Standards + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The project has a good foundation of unit and integration tests. However, there is no formal document defining the scope, purpose, and expected coverage for different types of tests. This can lead to inconsistent testing quality and gaps in coverage. + +## Decision + +We will formalize the testing pyramid for the project, defining the role of each testing layer: +1. **Unit Tests (Vitest)**: For isolated functions, components, and repository methods with mocked dependencies. High coverage is expected. +2. **Integration Tests (Supertest)**: For API routes, testing the interaction between controllers, services, and mocked database layers. Focus on contract and middleware correctness. +3. **End-to-End (E2E) Tests (Playwright/Cypress)**: For critical user flows (e.g., login, flyer upload, checkout), running against a real browser and a test database to ensure the entire system works together. + +## Consequences + +* **Positive**: Ensures a consistent and comprehensive approach to quality assurance. Gives developers confidence when refactoring or adding new features. Clearly defines "done" for a new feature. +* **Negative**: May require investment in setting up and maintaining the E2E testing environment. Can slightly increase the time required to develop a feature if all test layers are required. \ No newline at end of file diff --git a/docs/adr/0011-advanced-authorization-and-access-control-strategy.md b/docs/adr/0011-advanced-authorization-and-access-control-strategy.md new file mode 100644 index 00000000..190a2b6c --- /dev/null +++ b/docs/adr/0011-advanced-authorization-and-access-control-strategy.md @@ -0,0 +1,18 @@ +# ADR-011: Advanced Authorization and Access Control Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The current authorization model relies on basic role checks (e.g., `isAdmin` middleware). As the application grows to include more user roles (e.g., 'moderator', 'premium_user', 'store_manager') and more granular permissions, this approach will become unmanageable and lead to scattered permission logic throughout the codebase. + +## Decision + +We will formalize a centralized Role-Based Access Control (RBAC) or Attribute-Based Access Control (ABAC) system. This will involve defining how permissions are structured, checked, and managed, likely through a dedicated authorization library (e.g., `casl`) or a custom middleware that consumes a clear set of role definitions. + +## Consequences + +* **Positive**: Ensures authorization logic is consistent, easy to audit, and decoupled from business logic. Improves security by centralizing access control. +* **Negative**: Requires a significant refactoring effort to integrate the new authorization system across all protected routes and features. Introduces a new dependency if an external library is chosen. diff --git a/docs/adr/0012-frontend-component-library-and-design-system.md b/docs/adr/0012-frontend-component-library-and-design-system.md new file mode 100644 index 00000000..92622057 --- /dev/null +++ b/docs/adr/0012-frontend-component-library-and-design-system.md @@ -0,0 +1,18 @@ +# ADR-012: Frontend Component Library and Design System + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The frontend is built with React, but there is no formal strategy for component reuse, styling consistency, or UI documentation. As more features are added, this can lead to a fragmented user experience, duplicated effort, and a codebase that is difficult to maintain. + +## Decision + +We will establish a formal Design System and Component Library. This will involve using a tool like **Storybook** to develop, document, and test UI components in isolation. It will establish clear guidelines for styling, theming (e.g., dark mode), and accessibility. + +## Consequences + +* **Positive**: Ensures a consistent and high-quality user interface. Accelerates frontend development by providing reusable, well-documented components. Improves maintainability and reduces technical debt. +* **Negative**: Requires an initial investment in setting up Storybook and migrating existing components. Adds a new dependency and a new workflow for frontend development. diff --git a/docs/adr/0013-database-schema-migration-strategy.md b/docs/adr/0013-database-schema-migration-strategy.md new file mode 100644 index 00000000..618295df --- /dev/null +++ b/docs/adr/0013-database-schema-migration-strategy.md @@ -0,0 +1,18 @@ +# ADR-013: Database Schema Migration Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The current database schema is managed by manually running a large `schema.sql.txt` file. This approach is highly error-prone, makes it difficult to track changes, and is not feasible for updating a live production database without downtime or data loss. + +## Decision + +We will adopt a dedicated database migration tool, such as **`node-pg-migrate`** or **Knex.js**. All future schema changes (e.g., adding a column, creating a table) will be written as individual, version-controlled migration scripts. + +## Consequences + +* **Positive**: Provides a safe, repeatable, and reversible way to evolve the database schema. Improves team collaboration on database changes. Reduces the risk of data loss or downtime during deployments. +* **Negative**: Requires an initial setup and learning curve for the chosen migration tool. All future schema changes must adhere to the migration workflow. diff --git a/docs/adr/0014-containerization-and-deployment-strategy.md b/docs/adr/0014-containerization-and-deployment-strategy.md new file mode 100644 index 00000000..c48849a4 --- /dev/null +++ b/docs/adr/0014-containerization-and-deployment-strategy.md @@ -0,0 +1,18 @@ +# ADR-014: Containerization and Deployment Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The project is currently run using `pm2`, and the `README.md` contains manual setup instructions. While functional, this lacks the portability, scalability, and consistency of modern deployment practices. + +## Decision + +We will standardize the deployment process by containerizing the application using **Docker**. This will involve defining a `Dockerfile` for building a production-ready image and a `docker-compose.yml` file for orchestrating the application, database, and other services (like Redis) in a development environment. + +## Consequences + +* **Positive**: Ensures consistency between development and production environments. Simplifies the setup for new developers. Improves portability and scalability of the application. +* **Negative**: Requires learning Docker and containerization concepts. Adds `Dockerfile` and `docker-compose.yml` to the project's configuration. diff --git a/docs/adr/0015-application-performance-monitoring-and-error-tracking.md b/docs/adr/0015-application-performance-monitoring-and-error-tracking.md new file mode 100644 index 00000000..4bf3219e --- /dev/null +++ b/docs/adr/0015-application-performance-monitoring-and-error-tracking.md @@ -0,0 +1,18 @@ +# ADR-015: Application Performance Monitoring (APM) and Error Tracking + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +While `ADR-004` established structured logging, the application lacks a high-level, aggregated view of its health, performance, and errors. It's difficult to spot trends, identify slow API endpoints, or be proactively notified of new types of errors. + +## Decision + +We will integrate a dedicated Application Performance Monitoring (APM) and error tracking service like **Sentry**, **Datadog**, or **New Relic**. This will define how the service is integrated to automatically capture and report unhandled exceptions, performance data (e.g., transaction traces, database query times), and release health. + +## Consequences + +**Positive**: Provides critical observability into the application's real-world behavior. Enables proactive identification and resolution of performance bottlenecks and errors. Improves overall application reliability and user experience. +**Negative**: Introduces a new third-party dependency and potential subscription costs. Requires initial setup and configuration of the APM/error tracking agent. diff --git a/docs/adr/0016-api-security-hardening.md b/docs/adr/0016-api-security-hardening.md new file mode 100644 index 00000000..4c78b82d --- /dev/null +++ b/docs/adr/0016-api-security-hardening.md @@ -0,0 +1,22 @@ +# ADR-016: API Security Hardening + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +While authentication (`ADR-011`) is covered, the API lacks formal policies for protection against common web vulnerabilities and abuse. Security measures like rate limiting, secure HTTP headers, and Cross-Origin Resource Sharing (CORS) are not standardized. + +## Decision + +We will implement a multi-layered security approach for the API: + +1. **`helmet`**: Use the `helmet` library to set crucial security headers (e.g., Content-Security-Policy, X-Content-Type-Options). +2. **Rate Limiting**: Apply rate limiting using a library like `express-rate-limit` on sensitive endpoints (e.g., `/login`, `/register`) to prevent brute-force attacks. +3. **CORS**: Establish a strict CORS policy to control which domains can access the API. + +## Consequences + +* **Positive**: Significantly improves the application's security posture against common web vulnerabilities like XSS, clickjacking, and brute-force attacks. +* **Negative**: Requires careful configuration of CORS and rate limits to avoid blocking legitimate traffic. Content-Security-Policy can be complex to configure correctly. \ No newline at end of file diff --git a/docs/adr/0017-ci-cd-and-branching-strategy.md b/docs/adr/0017-ci-cd-and-branching-strategy.md new file mode 100644 index 00000000..2ed9a437 --- /dev/null +++ b/docs/adr/0017-ci-cd-and-branching-strategy.md @@ -0,0 +1,18 @@ +# ADR-017: CI/CD and Branching Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The project has Gitea workflows but lacks a documented standard for how code moves from a developer's machine to production. This can lead to inconsistent deployment processes and uncertainty about code quality. + +## Decision + +We will formalize the end-to-end CI/CD process. This ADR will define the project's **branching strategy** (e.g., GitFlow or Trunk-Based Development), establish mandatory checks in the pipeline (e.g., linting, unit tests, vulnerability scanning), and specify the process for building and publishing Docker images (`ADR-014`) to a registry. + +## Consequences + +* **Positive**: Automates quality control and creates a safe, repeatable path to production. Increases development velocity and reduces deployment-related errors. +* **Negative**: Initial setup effort for the CI/CD pipeline. May slightly increase the time to merge code due to mandatory checks. \ No newline at end of file diff --git a/docs/adr/0018-api-documentation-strategy.md b/docs/adr/0018-api-documentation-strategy.md new file mode 100644 index 00000000..0fd58143 --- /dev/null +++ b/docs/adr/0018-api-documentation-strategy.md @@ -0,0 +1,18 @@ +# ADR-018: API Documentation Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +As the API grows, it becomes increasingly difficult for frontend developers and other consumers to understand its endpoints, request formats, and response structures. There is no single source of truth for API documentation. + +## Decision + +We will adopt **OpenAPI (Swagger)** for API documentation. We will use tools (e.g., JSDoc annotations with `swagger-jsdoc`) to generate an `openapi.json` specification directly from the route handler source code. This specification will be served via a UI like Swagger UI for interactive exploration. + +## Consequences + +* **Positive**: Creates a single source of truth for API documentation that stays in sync with the code. Enables auto-generation of client SDKs and simplifies testing. +* **Negative**: Requires developers to maintain JSDoc annotations on all routes. Adds a build step and new dependencies to the project. \ No newline at end of file diff --git a/docs/adr/0019-data-backup-and-recovery-strategy.md b/docs/adr/0019-data-backup-and-recovery-strategy.md new file mode 100644 index 00000000..648fb93c --- /dev/null +++ b/docs/adr/0019-data-backup-and-recovery-strategy.md @@ -0,0 +1,18 @@ +# ADR-019: Data Backup and Recovery Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The application's data, stored in PostgreSQL, is critical. Currently, there is no formalized or automated strategy for creating backups or for recovering data in the event of hardware failure, data corruption, or accidental deletion. + +## Decision + +We will implement a formal data backup and recovery strategy. This will involve using standard PostgreSQL tools (`pg_dump`) to perform **regular, automated backups** (e.g., daily). Backup files will be stored securely in a separate, off-site location (e.g., a cloud storage bucket). The ADR will also define the Recovery Time Objective (RTO) and Recovery Point Objective (RPO) and document the step-by-step procedure for restoring from a backup. + +## Consequences + +* **Positive**: Protects against catastrophic data loss, ensuring business continuity. Provides a clear, tested plan for disaster recovery. +* **Negative**: Requires setup and maintenance of backup scripts and secure storage. Incurs storage costs for backup files. \ No newline at end of file diff --git a/docs/adr/0020-health-checks-and-liveness-readiness-probes.md b/docs/adr/0020-health-checks-and-liveness-readiness-probes.md new file mode 100644 index 00000000..1bb64470 --- /dev/null +++ b/docs/adr/0020-health-checks-and-liveness-readiness-probes.md @@ -0,0 +1,20 @@ +# ADR-020: Health Checks and Liveness/Readiness Probes + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +When the application is containerized (`ADR-014`), the container orchestrator (e.g., Kubernetes, Docker Swarm) needs a way to determine if the application is running correctly. Without this, it cannot manage application lifecycle events like restarts or rolling updates effectively. + +## Decision + +We will implement dedicated health check endpoints in the Express application. +* A **Liveness Probe** (`/api/health/live`) will return a `200 OK` to indicate the server is running. If it fails, the orchestrator should restart the container. +* A **Readiness Probe** (`/api/health/ready`) will return a `200 OK` only if the application is ready to accept traffic (e.g., database connection is established). If it fails, the orchestrator will temporarily remove the container from the load balancer. + +## Consequences + +* **Positive**: Enables robust, automated application lifecycle management in a containerized environment. Prevents traffic from being sent to unhealthy or uninitialized application instances. +* **Negative**: Adds a small amount of code for the health check endpoints. Requires configuration in the container orchestration layer. \ No newline at end of file diff --git a/docs/adr/0021-code-formatting-and-linting-unification.md b/docs/adr/0021-code-formatting-and-linting-unification.md new file mode 100644 index 00000000..091fd88e --- /dev/null +++ b/docs/adr/0021-code-formatting-and-linting-unification.md @@ -0,0 +1,19 @@ +# ADR-021: Code Formatting and Linting Unification + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The project contains both frontend (React) and backend (Node.js) code. While linters may be in use, there isn't a single, enforced standard for code style and quality across the entire repository. This leads to inconsistent code and time wasted in code reviews on stylistic debates. + +## Decision + +We will mandate the use of **Prettier** for automated code formatting and a unified **ESLint** configuration for code quality rules across both frontend and backend. This will be enforced automatically using a pre-commit hook managed by a tool like **Husky**. + +## Consequences + +**Positive**: Improves developer experience and team velocity by automating code consistency. Reduces time spent on stylistic code review comments. Enhances code readability and maintainability. + +**Negative**: Requires an initial setup and configuration of Prettier, ESLint, and Husky. May require a one-time reformatting of the entire codebase. diff --git a/docs/adr/0022-real-time-notification-system.md b/docs/adr/0022-real-time-notification-system.md new file mode 100644 index 00000000..49f15380 --- /dev/null +++ b/docs/adr/0022-real-time-notification-system.md @@ -0,0 +1,18 @@ +# ADR-022: Real-time Notification System + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +A core feature is providing "Active Deal Alerts" to users. The current HTTP-based architecture is not suitable for pushing real-time updates to clients efficiently. Relying on traditional polling would be inefficient and slow. + +## Decision + +We will implement a real-time communication system using **WebSockets** (e.g., with the `ws` library or Socket.IO). This will involve an architecture for a notification service that listens for backend events (like a new deal from a background job) and pushes live updates to connected clients. + +## Consequences + +* **Positive**: Enables a core, user-facing feature in a scalable and efficient manner. Significantly improves user engagement and experience. +* **Negative**: Introduces a new dependency (e.g., WebSocket library) and adds complexity to the backend and frontend architecture. Requires careful handling of connection management and scaling. \ No newline at end of file diff --git a/docs/adr/0023-database-schema-migration-strategy.md b/docs/adr/0023-database-schema-migration-strategy.md new file mode 100644 index 00000000..3011710c --- /dev/null +++ b/docs/adr/0023-database-schema-migration-strategy.md @@ -0,0 +1,18 @@ +# ADR-023: Database Schema Migration Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The `README.md` indicates that the database schema is managed by manually running a large `schema.sql.txt` file. This approach is highly error-prone, makes tracking changes difficult, and is not feasible for updating a live production database without downtime or data loss. + +## Decision + +We will adopt a dedicated database migration tool, such as **`node-pg-migrate`**. All future schema changes (e.g., `ALTER TABLE`, `CREATE INDEX`) will be written as individual, version-controlled, timestamped migration scripts that can be applied and rolled back automatically. + +## Consequences + +* **Positive**: Provides a safe, repeatable, and reversible way to evolve the database schema. Improves team collaboration on database changes. Reduces the risk of data loss or downtime during deployments. +* **Negative**: Requires an initial setup and learning curve for the chosen migration tool. All future schema changes must adhere to the migration workflow. \ No newline at end of file diff --git a/docs/adr/0024-feature-flagging-strategy.md b/docs/adr/0024-feature-flagging-strategy.md new file mode 100644 index 00000000..633c713e --- /dev/null +++ b/docs/adr/0024-feature-flagging-strategy.md @@ -0,0 +1,18 @@ +# ADR-024: Feature Flagging Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +As the application grows, there is no way to roll out new features to a subset of users (e.g., for beta testing) or to quickly disable a problematic feature in production without a full redeployment. + +## Decision + +We will implement a feature flagging system. This could start with a simple configuration-based approach (defined in `ADR-007`) and evolve to use a dedicated service like **Flagsmith** or **LaunchDarkly**. This ADR will define how feature flags are created, managed, and checked in both the backend and frontend code. + +## Consequences + +* **Positive**: Decouples feature releases from code deployments, reducing risk and allowing for more controlled, gradual rollouts and A/B testing. Enables easier experimentation and faster iteration. +* **Negative**: Adds complexity to the codebase with conditional logic around features. Requires careful management of feature flag states to avoid technical debt. \ No newline at end of file diff --git a/docs/adr/0025-internationalization-and-localization-strategy.md b/docs/adr/0025-internationalization-and-localization-strategy.md new file mode 100644 index 00000000..1a3d1614 --- /dev/null +++ b/docs/adr/0025-internationalization-and-localization-strategy.md @@ -0,0 +1,18 @@ +# ADR-025: Internationalization (i18n) and Localization (l10n) Strategy + +**Date**: 2025-12-12 + +**Status**: Proposed + +## Context + +The application currently displays all text, dates, and currency in a single format (likely English/USD). To expand its user base, it will need to support multiple languages and regional formats. + +## Decision + +We will adopt a framework for internationalization. For the frontend, this will involve a library like **`react-i18next`** to manage translation files (e.g., `en.json`, `es.json`). For the backend, it will define how to handle locale-specific data (like currency formatting in API responses) and store user language preferences. + +## Consequences + +* **Positive**: Lays the architectural groundwork for future global expansion. Improves user experience for non-English speakers. +* **Negative**: Requires significant effort to translate all user-facing text. Adds complexity to both frontend and backend code for handling locale-specific data. \ No newline at end of file diff --git a/docs/adr/index.md b/docs/adr/index.md new file mode 100644 index 00000000..813df142 --- /dev/null +++ b/docs/adr/index.md @@ -0,0 +1,52 @@ +# Architectural Decision Records + +This directory contains a log of the architectural decisions made for the Flyer Crawler project. + +## 1. Foundational / Core Infrastructure + +* **[ADR-002](./0002-standardized-transaction-management.md)**: Standardized Transaction Management and Unit of Work Pattern (Proposed) +* **[ADR-007](./0007-configuration-and-secrets-management.md)**: Configuration and Secrets Management (Proposed) +* **[ADR-020](./0020-health-checks-and-liveness-readiness-probes.md)**: Health Checks and Liveness/Readiness Probes (Proposed) + +## 2. Data Management + +* **[ADR-009](./0009-caching-strategy-for-read-heavy-operations.md)**: Caching Strategy for Read-Heavy Operations (Proposed) +* **[ADR-013](./0013-database-schema-migration-strategy.md)**: Database Schema Migration Strategy (Proposed) +* **[ADR-019](./0019-data-backup-and-recovery-strategy.md)**: Data Backup and Recovery Strategy (Proposed) +* **[ADR-023](./0023-database-schema-migration-strategy.md)**: Database Schema Migration Strategy (Proposed) + +## 3. API & Integration + +* **[ADR-003](./0003-standardized-input-validation-using-middleware.md)**: Standardized Input Validation using Middleware (Proposed) +* **[ADR-008](./0008-api-versioning-strategy.md)**: API Versioning Strategy (Proposed) +* **[ADR-018](./0018-api-documentation-strategy.md)**: API Documentation Strategy (Proposed) +* **[ADR-022](./0022-real-time-notification-system.md)**: Real-time Notification System (Proposed) + +## 4. Security & Compliance + +* **[ADR-001](./0001-standardized-error-handling.md)**: Standardized Error Handling for Service and Repository Layers (Accepted) +* **[ADR-011](./0011-advanced-authorization-and-access-control-strategy.md)**: Advanced Authorization and Access Control Strategy (Proposed) +* **[ADR-016](./0016-api-security-hardening.md)**: API Security Hardening (Proposed) + +## 5. Observability & Monitoring + +* **[ADR-004](./0004-standardized-application-wide-structured-logging.md)**: Standardized Application-Wide Structured Logging (Proposed) +* **[ADR-015](./0015-application-performance-monitoring-and-error-tracking.md)**: Application Performance Monitoring (APM) and Error Tracking (Proposed) + +## 6. Deployment & Operations + +* **[ADR-006](./0006-background-job-processing-and-task-queues.md)**: Background Job Processing and Task Queues (Proposed) +* **[ADR-014](./0014-containerization-and-deployment-strategy.md)**: Containerization and Deployment Strategy (Proposed) +* **[ADR-017](./0017-ci-cd-and-branching-strategy.md)**: CI/CD and Branching Strategy (Proposed) +* **[ADR-024](./0024-feature-flagging-strategy.md)**: Feature Flagging Strategy (Proposed) + +## 7. Frontend / User Interface + +* **[ADR-005](./0005-frontend-state-management-and-server-cache-strategy.md)**: Frontend State Management and Server Cache Strategy (Proposed) +* **[ADR-012](./0012-frontend-component-library-and-design-system.md)**: Frontend Component Library and Design System (Proposed) +* **[ADR-025](./0025-internationalization-and-localization-strategy.md)**: Internationalization (i18n) and Localization (l10n) Strategy (Proposed) + +## 8. Development Workflow & Quality + +* **[ADR-010](./0010-testing-strategy-and-standards.md)**: Testing Strategy and Standards (Proposed) +* **[ADR-021](./0021-code-formatting-and-linting-unification.md)**: Code Formatting and Linting Unification (Proposed) diff --git a/src/hooks/useAuth.test.tsx b/src/hooks/useAuth.test.tsx index 7082b16f..a0b08415 100644 --- a/src/hooks/useAuth.test.tsx +++ b/src/hooks/useAuth.test.tsx @@ -59,22 +59,14 @@ describe('useAuth Hook and AuthProvider', () => { console.error = originalError; }); - it('initializes with a "Determining..." state and isLoading as true', async () => { - // Use fake timers to control async operations and prevent useEffect from running immediately. - vi.useFakeTimers(); - + it('initializes with a "Determining..." state and isLoading as true', () => { const { result } = renderHook(() => useAuth(), { wrapper }); - // At this point, the component has rendered with its initial state, - // but the useEffect has been queued and not yet executed. + // Immediately after the initial render, before the useEffect has resolved, + // the state should be in its initial "Determining..." phase. expect(result.current.authStatus).toBe('Determining...'); expect(result.current.isLoading).toBe(true); expect(result.current.user).toBeNull(); - - // Allow promises to resolve and timers to run to avoid leaving the test in a pending state. - await act(async () => { - vi.runAllTimers(); - }); }); describe('Initial Auth Check (useEffect)', () => { diff --git a/src/middleware/errorHandler.ts b/src/middleware/errorHandler.ts index 3404b933..0c4b3b45 100644 --- a/src/middleware/errorHandler.ts +++ b/src/middleware/errorHandler.ts @@ -55,16 +55,16 @@ export const errorHandler = (err: HttpError, req: Request, res: Response, next: let message = err.message; let errorId: string | undefined; - // Refine the status code for known error types. This block should come first. - if (err instanceof DatabaseError) { - // This will handle UniqueConstraintError, ForeignKeyConstraintError, NotFoundError, etc. - statusCode = err.status; + // 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 UniqueConstraintError) { - statusCode = 409; // Conflict } else if (err instanceof NotFoundError) { statusCode = 404; + } else if (err instanceof DatabaseError) { + // This is a generic fallback for other database errors. + statusCode = err.status; } else if (err.name === 'UnauthorizedError') { statusCode = err.status || 401; } diff --git a/src/routes/auth.routes.test.ts b/src/routes/auth.routes.test.ts index e2b83da0..715892ff 100644 --- a/src/routes/auth.routes.test.ts +++ b/src/routes/auth.routes.test.ts @@ -25,7 +25,7 @@ const passportMocks = vi.hoisted(() => { return callback(null, false, { message: 'Incorrect email or password.' }); } if (req.body.email === 'locked@test.com') { - return callback(null, false, { message: 'Account is temporarily locked.' }); + return callback(null, false, { message: 'Account is temporarily locked. Please try again in 15 minutes.' }); } if (req.body.email === 'notfound@test.com') { return callback(null, false, { message: 'Login failed' }); diff --git a/src/routes/budget.routes.test.ts b/src/routes/budget.routes.test.ts index 359b01f2..8a111862 100644 --- a/src/routes/budget.routes.test.ts +++ b/src/routes/budget.routes.test.ts @@ -171,7 +171,7 @@ describe('Budget Routes (/api/budgets)', () => { }); it('should return 404 if the budget is not found', async () => { - vi.mocked(db.budgetRepo.deleteBudget).mockRejectedValue(new Error('Budget not found')); + vi.mocked(db.budgetRepo.deleteBudget).mockRejectedValue(new NotFoundError('Budget not found')); const response = await supertest(app).delete('/api/budgets/999'); expect(response.status).toBe(404); expect(response.body.message).toBe('Budget not found'); diff --git a/src/routes/flyer.routes.ts b/src/routes/flyer.routes.ts index 181578fc..e13a85d4 100644 --- a/src/routes/flyer.routes.ts +++ b/src/routes/flyer.routes.ts @@ -62,6 +62,10 @@ router.post('/items/batch-fetch', async (req, res, next: NextFunction) => { if (!Array.isArray(flyerIds)) { return res.status(400).json({ message: 'flyerIds must be an array.' }); } + // If the array is empty, return an empty array immediately to avoid an unnecessary DB call. + if (flyerIds.length === 0) { + return res.json([]); + } try { const items = await db.flyerRepo.getFlyerItemsForFlyers(flyerIds); res.json(items); diff --git a/src/services/db/0001-standardized-error-handling.md b/src/services/db/0001-standardized-error-handling.md new file mode 100644 index 00000000..a495135f --- /dev/null +++ b/src/services/db/0001-standardized-error-handling.md @@ -0,0 +1,43 @@ +# ADR-001: Standardized Error Handling for Service and Repository Layers + +**Date**: 2025-12-12 + +**Status**: Accepted + +## Context + +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. + +This pattern led to increased development friction, difficult-to-diagnose bugs, and a fragile test suite. + +## Decision + +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. + +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). + +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 + +### Positive + +* **Robustness**: Eliminates an entire class of bugs where `undefined` is passed to `res.json()`, preventing incorrect `500` errors. +* **Consistency & Predictability**: All data-fetching methods now have a predictable contract. They either return the expected data or throw a specific, typed error. +* **Developer Experience**: Route handlers become simpler, cleaner, and easier to write correctly. The cognitive load on developers is reduced as they no longer need to remember to check for `undefined`. +* **Improved Testability**: Tests become more reliable and realistic. Mocks can now throw the *exact* error type (`new NotFoundError()`) that the real implementation would, ensuring tests accurately reflect the application's behavior. +* **Centralized Control**: Error-to-HTTP-status logic is centralized in the `errorHandler` middleware, making it easy to manage and modify error responses globally. + +### 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. \ No newline at end of file diff --git a/src/services/db/0002- b/src/services/db/0002- new file mode 100644 index 00000000..e69de29b diff --git a/src/services/db/recipe.db.ts b/src/services/db/recipe.db.ts index 7d414d1c..9ae567a2 100644 --- a/src/services/db/recipe.db.ts +++ b/src/services/db/recipe.db.ts @@ -211,7 +211,9 @@ export class RecipeRepository { `; const res = await this.db.query(query, [recipeId]); if (res.rowCount === 0) { - throw new NotFoundError(`Recipe with ID ${recipeId} not found.`); + // A recipe not being found is an expected case, so we throw a specific + // error that our error handler middleware can convert to a 404 status. + throw new NotFoundError(`Recipe with ID ${recipeId} not found`); } return res.rows[0]; } catch (error) {