Merge pull request #129 from shoaib050326/codex/issue-13-error-handling

feat(backend): add centralized error handling

Author: Smartdevs17

backend/package-lock.json

@@ -17,6 +17,7 @@
     "cron-parser": "^4.9.0",
     "dotenv": "^16.4.5",
     "express": "^4.21.0",
+    "express-rate-limit": "^7.4.1",
     "node-cron": "^3.0.3",
     "openai": "^4.67.0",
     "zod": "^3.23.8"

backend/package.json

@@ -1,6 +1,6 @@
-import express, { Request, Response, NextFunction } from 'express';
 import { AsyncLocalStorage } from 'node:async_hooks';
 import { randomUUID } from 'node:crypto';
+import express, { Request, Response, NextFunction } from 'express';
 import cors from 'cors';
 import dotenv from 'dotenv';
 import rateLimit from 'express-rate-limit';
@@ -11,6 +11,7 @@ import { catalogRouter } from './routes/catalog.js';
 import { jobsRouter } from './routes/jobs.js';
 import { healthRouter } from './routes/health.js';
 import { startJobs, getJobScheduler } from './jobs/index.js';
+import { errorHandler, notFoundHandler } from './middleware/errorHandler.js';
 
 dotenv.config();
 
@@ -43,16 +44,32 @@ console.error = (...args) => originalConsole.error(...formatMessage(args));
 const app = express();
 const PORT = process.env.PORT || 3001;
 
-const allowedOrigins = process.env.CORS_ALLOWED_ORIGINS 
-  ? process.env.CORS_ALLOWED_ORIGINS.split(',').map(o => o.trim())
+const allowedOrigins = process.env.CORS_ALLOWED_ORIGINS
+  ? process.env.CORS_ALLOWED_ORIGINS.split(',').map((origin) => origin.trim())
   : '*';
 
-app.use(cors({
-  origin: allowedOrigins,
-  credentials: true,
-  methods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'],
-  allowedHeaders: ['Content-Type', 'Authorization', 'X-Trace-Id'],
-}));
+const generalLimiter = rateLimit({
+  windowMs: 15 * 60 * 1000,
+  max: 100,
+  standardHeaders: true,
+  legacyHeaders: false,
+});
+
+const invoiceLimiter = rateLimit({
+  windowMs: 15 * 60 * 1000,
+  max: 20,
+  standardHeaders: true,
+  legacyHeaders: false,
+});
+
+app.use(
+  cors({
+    origin: allowedOrigins,
+    credentials: true,
+    methods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'],
+    allowedHeaders: ['Content-Type', 'Authorization', 'X-Trace-Id'],
+  })
+);
 app.use(express.json());
 
 // Trace ID middleware
@@ -86,6 +103,8 @@ app.use('/api/v1/invoice', invoiceRouter);
 app.use('/api/v1/stellar', stellarRouter);
 app.use('/api/v1/catalog', catalogRouter);
 app.use('/api/v1/jobs', jobsRouter);
+app.use(notFoundHandler);
+app.use(errorHandler);
 
 const jobsEnabled = process.env.JOBS_ENABLED !== 'false';
 if (jobsEnabled) {
@@ -99,12 +118,10 @@ const server = app.listen(PORT, () => {
 // Graceful shutdown
 const shutdown = (signal: string) => {
   console.log(`${signal} received. Starting graceful shutdown...`);
-  
-  // 1. Stop accepting new requests
+
   server.close(() => {
     console.log('HTTP server closed.');
-    
-    // 2. Stop jobs
+
     try {
       const scheduler = getJobScheduler();
       if (scheduler) {
@@ -119,7 +136,6 @@ const shutdown = (signal: string) => {
     process.exit(0);
   });
 
-  // Force exit if server.close takes too long (e.g. 10s)
   setTimeout(() => {
     console.error('Could not close connections in time, forceful shutdown');
     process.exit(1);

backend/src/index.ts

@@ -0,0 +1,56 @@
+import type { NextFunction, Request, RequestHandler, Response } from 'express';
+
+type AsyncRouteHandler = (req: Request, res: Response, next: NextFunction) => Promise<unknown>;
+
+export class AppError extends Error {
+  statusCode: number;
+  code: string;
+  details?: unknown;
+
+  constructor(statusCode: number, message: string, code = 'INTERNAL_SERVER_ERROR', details?: unknown) {
+    super(message);
+    this.name = 'AppError';
+    this.statusCode = statusCode;
+    this.code = code;
+    this.details = details;
+  }
+}
+
+export function asyncHandler(handler: AsyncRouteHandler): RequestHandler {
+  return (req, res, next) => {
+    Promise.resolve(handler(req, res, next)).catch(next);
+  };
+}
+
+export function notFoundHandler(req: Request, _res: Response, next: NextFunction) {
+  next(new AppError(404, `Route not found: ${req.method} ${req.originalUrl}`, 'NOT_FOUND'));
+}
+
+export function errorHandler(err: unknown, _req: Request, res: Response, _next: NextFunction) {
+  const isAppError = err instanceof AppError;
+  const statusCode = isAppError ? err.statusCode : 500;
+  const code = isAppError ? err.code : 'INTERNAL_SERVER_ERROR';
+  const isProduction = process.env.NODE_ENV === 'production';
+  const message = isAppError
+    ? err.message
+    : isProduction
+      ? 'Internal server error'
+      : err instanceof Error
+        ? err.message
+        : 'Unexpected error';
+
+  const logMethod = statusCode >= 500 ? console.error : console.warn;
+  logMethod(`[${code}] ${message}`, err);
+
+  res.status(statusCode).json({
+    error: {
+      code,
+      message,
+      status: statusCode,
+      ...(isAppError && err.details !== undefined ? { details: err.details } : {}),
+      ...(!isProduction && !isAppError && err instanceof Error && err.stack
+        ? { stack: err.stack }
+        : {}),
+    },
+  });
+}

backend/src/middleware/errorHandler.ts

@@ -1,6 +1,6 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { healthRouter } from '../health.js';
-import { Request, Response } from 'express';
+import { Request, Response, Router, RequestHandler } from 'express';
 import { server as stellarServer } from '../../services/stellar.js';
 import { getJobScheduler } from '../../jobs/index.js';
 
@@ -20,6 +20,17 @@ describe('Health Router', () => {
   let resJson: any;
   let resStatus: any;
 
+  const getRouteHandler = (router: Router, path: string): RequestHandler => {
+    const layer = router.stack.find((entry) => entry.route?.path === path);
+    const handler = layer?.route?.stack[0]?.handle;
+
+    if (!handler) {
+      throw new Error(`Route handler not found for ${path}`);
+    }
+
+    return handler;
+  };
+
   beforeEach(() => {
     vi.clearAllMocks();
     resJson = vi.fn();
@@ -32,13 +43,16 @@ describe('Health Router', () => {
 
   describe('GET /health', () => {
     it('returns 200 and healthy status when all dependencies are up', async () => {
-      vi.mocked(stellarServer.root).mockResolvedValue({} as any);
+      const mockedStellarRoot = vi.mocked(
+        (stellarServer as unknown as { root: () => Promise<unknown> }).root
+      );
+
+      mockedStellarRoot.mockResolvedValue({});
       vi.mocked(getJobScheduler).mockReturnValue({} as any);
       process.env.OPENAI_API_KEY = 'test-key';
 
-      // Access the private handler (for testing purposes in vitest)
-      const handler = (healthRouter.stack.find(s => s.route.path === '/health')?.route.stack[0].handle);
-      await handler(mockReq as Request, mockRes as Response);
+      const handler = getRouteHandler(healthRouter, '/health');
+      await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(200);
       expect(resJson).toHaveBeenCalledWith(expect.objectContaining({
@@ -52,13 +66,17 @@ describe('Health Router', () => {
     });
 
     it('returns 200 and degraded status when OpenAI is missing', async () => {
-      vi.mocked(stellarServer.root).mockResolvedValue({} as any);
+      const mockedStellarRoot = vi.mocked(
+        (stellarServer as unknown as { root: () => Promise<unknown> }).root
+      );
+
+      mockedStellarRoot.mockResolvedValue({});
       vi.mocked(getJobScheduler).mockReturnValue({} as any);
       const originalKey = process.env.OPENAI_API_KEY;
       delete process.env.OPENAI_API_KEY;
 
-      const handler = (healthRouter.stack.find(s => s.route.path === '/health')?.route.stack[0].handle);
-      await handler(mockReq as Request, mockRes as Response);
+      const handler = getRouteHandler(healthRouter, '/health');
+      await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(200);
       expect(resJson).toHaveBeenCalledWith(expect.objectContaining({
@@ -74,12 +92,16 @@ describe('Health Router', () => {
     });
 
     it('returns 503 and unhealthy status when Stellar Horizon is down', async () => {
-      vi.mocked(stellarServer.root).mockRejectedValue(new Error('Horizon Down'));
+      const mockedStellarRoot = vi.mocked(
+        (stellarServer as unknown as { root: () => Promise<unknown> }).root
+      );
+
+      mockedStellarRoot.mockRejectedValue(new Error('Horizon Down'));
       vi.mocked(getJobScheduler).mockReturnValue({} as any);
       process.env.OPENAI_API_KEY = 'test-key';
 
-      const handler = (healthRouter.stack.find(s => s.route.path === '/health')?.route.stack[0].handle);
-      await handler(mockReq as Request, mockRes as Response);
+      const handler = getRouteHandler(healthRouter, '/health');
+      await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(503);
       expect(resJson).toHaveBeenCalledWith(expect.objectContaining({
@@ -95,8 +117,8 @@ describe('Health Router', () => {
 
   describe('GET /ready', () => {
     it('returns 200 and ready status', async () => {
-      const handler = (healthRouter.stack.find(s => s.route.path === '/ready')?.route.stack[0].handle);
-      await handler(mockReq as Request, mockRes as Response);
+      const handler = getRouteHandler(healthRouter, '/ready');
+      await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(200);
       expect(resJson).toHaveBeenCalledWith(expect.objectContaining({

backend/src/routes/__tests__/health.test.ts

@@ -1,14 +1,25 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { invoiceRouter } from '../invoice.js';
 import { verificationRouter } from '../verification.js';
-import { Request, Response } from 'express';
+import { Request, Response, Router, RequestHandler } from 'express';
 
 describe('Schema Validation', () => {
   let mockReq: Partial<Request>;
   let mockRes: Partial<Response>;
   let resJson: any;
   let resStatus: any;
 
+  const getNamedRouteHandler = (router: Router, path: string, name: string): RequestHandler => {
+    const routeLayer = router.stack.find((entry) => entry.route?.path === path);
+    const handler = routeLayer?.route?.stack.find((entry) => entry.name === name)?.handle;
+
+    if (!handler) {
+      throw new Error(`Route handler not found for ${path}:${name}`);
+    }
+
+    return handler;
+  };
+
   beforeEach(() => {
     resJson = vi.fn();
     resStatus = vi.fn().mockReturnValue({ json: resJson });
@@ -21,8 +32,8 @@ describe('Schema Validation', () => {
   describe('Invoice Validation', () => {
     it('returns 400 when projectId is missing', async () => {
       mockReq.body = { workDescription: 'Test' };
-      const handler = (invoiceRouter.stack.find(s => s.route?.path === '/generate')?.route.stack.find((s: any) => s.name === 'validateMiddleware')?.handle);
-      
+      const handler = getNamedRouteHandler(invoiceRouter, '/generate', 'validateMiddleware');
+
       await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(400);
@@ -42,8 +53,8 @@ describe('Schema Validation', () => {
         milestoneDescription: 'Test',
         projectId: 'P1'
       };
-      const handler = (verificationRouter.stack.find(s => s.route?.path === '/verify')?.route.stack.find((s: any) => s.name === 'validateMiddleware')?.handle);
-      
+      const handler = getNamedRouteHandler(verificationRouter, '/verify', 'validateMiddleware');
+
       await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(400);
@@ -56,8 +67,12 @@ describe('Schema Validation', () => {
 
     it('returns 400 for empty bulk verification items', async () => {
       mockReq.body = { items: [] };
-      const handler = (verificationRouter.stack.find(s => s.route?.path === '/verify/batch')?.route.stack.find((s: any) => s.name === 'validateMiddleware')?.handle);
-      
+      const handler = getNamedRouteHandler(
+        verificationRouter,
+        '/verify/batch',
+        'validateMiddleware'
+      );
+
       await handler(mockReq as Request, mockRes as Response, vi.fn());
 
       expect(resStatus).toHaveBeenCalledWith(400);

backend/src/routes/__tests__/validation.test.ts

@@ -1,14 +1,13 @@
 import { Router } from 'express';
 import { getCatalog } from '../services/catalog.js';
+import { asyncHandler } from '../middleware/errorHandler.js';
 
 export const catalogRouter = Router();
 
-catalogRouter.get('/', (req, res) => {
-  try {
+catalogRouter.get(
+  '/',
+  asyncHandler(async (_req, res) => {
     const catalog = getCatalog();
     res.json(catalog);
-  } catch (error) {
-    console.error('Catalog error:', error);
-    res.status(500).json({ message: 'Failed to fetch catalog' });
-  }
-});
+  })
+);