From 3bf0ec9a50d157590e63a504946be6b1af0f152b Mon Sep 17 00:00:00 2001 From: RahRha-v3-2 <197854361+RahRha-v3-2@users.noreply.github.com> Date: Fri, 22 May 2026 21:33:49 +0000 Subject: [PATCH] fix(api-gateway): handle invalid json and add db test coverage - Added specific error handling for SyntaxError thrown by body-parser on invalid JSON payloads to return a clean 400 Bad Request instead of a 500 error. - Increased unit test coverage for the database layer (`src/db/pool.ts` and `src/db/migrate.ts`), covering pool creation, connection verification, and the sequential migration logic. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> --- api-gateway/src/middleware/errorHandler.ts | 11 +- api-gateway/tests/unit/migrate.test.ts | 116 +++++++++++++++++++++ api-gateway/tests/unit/pool.test.ts | 93 +++++++++++++++++ 3 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 api-gateway/tests/unit/migrate.test.ts create mode 100644 api-gateway/tests/unit/pool.test.ts diff --git a/api-gateway/src/middleware/errorHandler.ts b/api-gateway/src/middleware/errorHandler.ts index 57d1c8f..df11f0c 100644 --- a/api-gateway/src/middleware/errorHandler.ts +++ b/api-gateway/src/middleware/errorHandler.ts @@ -36,7 +36,7 @@ export function errorHandler(err: unknown, req: Request, res: Response, _next: N return; } - const e = err as Error & { status?: number; code?: string }; + const e = err as Error & { status?: number; code?: string; type?: string; body?: string }; if (e?.message === 'Origin not allowed by CORS') { res.status(403).json({ error: { code: 'CORS_FORBIDDEN', message: 'Origin not allowed', requestId }, @@ -44,6 +44,15 @@ export function errorHandler(err: unknown, req: Request, res: Response, _next: N return; } + // Handle invalid JSON parsing from express.json() + if (e instanceof SyntaxError && e.status === 400 && 'body' in e) { + logger.warn('Invalid JSON in request body', { requestId, path: req.path }); + res.status(400).json({ + error: { code: 'BAD_REQUEST', message: 'Invalid JSON payload', requestId }, + }); + return; + } + logger.error('Unhandled error', { requestId, name: e?.name, diff --git a/api-gateway/tests/unit/migrate.test.ts b/api-gateway/tests/unit/migrate.test.ts new file mode 100644 index 0000000..c926448 --- /dev/null +++ b/api-gateway/tests/unit/migrate.test.ts @@ -0,0 +1,116 @@ +import * as fs from 'fs'; +import * as poolModule from '../../src/db/pool'; +import { runMigrations } from '../../src/db/migrate'; + +jest.mock('fs'); +jest.mock('../../src/db/pool', () => ({ + getPool: jest.fn(), + verifyDatabaseConnection: jest.fn(), + closePool: jest.fn(), +})); +jest.mock('../../src/config/env', () => { + let hasDb = true; + return { + env: { LINODE_DB_HOST: 'localhost' }, + get hasDatabase() { return hasDb; }, + setHasDatabase(val: boolean) { hasDb = val; } + }; +}); + +describe('migrate', () => { + let mClient: any; + let mPool: any; + + beforeEach(() => { + jest.clearAllMocks(); + mClient = { + query: jest.fn().mockResolvedValue({ rows: [] }), + release: jest.fn(), + }; + mPool = { + connect: jest.fn().mockResolvedValue(mClient), + query: jest.fn().mockResolvedValue({ rows: [] }), + }; + (poolModule.getPool as jest.Mock).mockReturnValue(mPool); + (poolModule.verifyDatabaseConnection as jest.Mock).mockResolvedValue(true); + require('../../src/config/env').setHasDatabase(true); + (fs.existsSync as jest.Mock).mockReturnValue(true); + (fs.readdirSync as jest.Mock).mockReturnValue([]); + }); + + it('skips migrations if no database is configured', async () => { + require('../../src/config/env').setHasDatabase(false); + const result = await runMigrations(); + expect(result).toEqual({ applied: [], skipped: [] }); + expect(poolModule.verifyDatabaseConnection).not.toHaveBeenCalled(); + }); + + it('throws an error if database connection verification fails', async () => { + (poolModule.verifyDatabaseConnection as jest.Mock).mockResolvedValue(false); + await expect(runMigrations()).rejects.toThrow('Cannot connect to database'); + }); + + it('applies new migrations successfully', async () => { + (fs.readdirSync as jest.Mock).mockReturnValue(['001_test.sql']); + (fs.readFileSync as jest.Mock).mockReturnValue('CREATE TABLE test (id int);'); + mPool.query.mockImplementation((q: string) => { + if(q.includes("SELECT name FROM schema_migrations")) { + return Promise.resolve({ rows: [] }); + } + return Promise.resolve({ rows: [] }); + }); + + const result = await runMigrations(); + + expect(result.applied).toEqual(['001_test.sql']); + expect(result.skipped).toEqual([]); + + expect(mClient.query).toHaveBeenCalledWith('BEGIN'); + expect(mClient.query).toHaveBeenCalledWith('CREATE TABLE test (id int);'); + expect(mClient.query).toHaveBeenCalledWith( + expect.stringContaining('INSERT INTO schema_migrations'), + expect.arrayContaining(['001_test.sql', expect.any(String)]) + ); + expect(mClient.query).toHaveBeenCalledWith('COMMIT'); + expect(mClient.release).toHaveBeenCalled(); + }); + + it('skips already applied migrations', async () => { + (fs.readdirSync as jest.Mock).mockReturnValue(['001_test.sql']); + mPool.query.mockImplementation((q: string) => { + if(q.includes("SELECT name FROM schema_migrations")) { + return Promise.resolve({ rows: [{ name: '001_test.sql' }] }); + } + return Promise.resolve({ rows: [] }); + }); + + const result = await runMigrations(); + + expect(result.applied).toEqual([]); + expect(result.skipped).toEqual(['001_test.sql']); + expect(mClient.query).not.toHaveBeenCalledWith('BEGIN'); + }); + + it('rolls back on migration failure', async () => { + (fs.readdirSync as jest.Mock).mockReturnValue(['001_test.sql']); + (fs.readFileSync as jest.Mock).mockReturnValue('INVALID SQL;'); + mPool.query.mockImplementation((q: string) => { + if(q.includes("SELECT name FROM schema_migrations")) { + return Promise.resolve({ rows: [] }); + } + return Promise.resolve({ rows: [] }); + }); + + // Fail the specific migration execution + mClient.query.mockImplementation((q: string) => { + if (q === 'INVALID SQL;') return Promise.reject(new Error('Syntax Error')); + return Promise.resolve({ rows: [] }); + }); + + await expect(runMigrations()).rejects.toThrow('Failed migration 001_test.sql: Syntax Error'); + + expect(mClient.query).toHaveBeenCalledWith('BEGIN'); + expect(mClient.query).toHaveBeenCalledWith('ROLLBACK'); + expect(mClient.release).toHaveBeenCalled(); + }); +}); diff --git a/api-gateway/tests/unit/pool.test.ts b/api-gateway/tests/unit/pool.test.ts new file mode 100644 index 0000000..2c0869a --- /dev/null +++ b/api-gateway/tests/unit/pool.test.ts @@ -0,0 +1,93 @@ +import { Pool } from 'pg'; +import * as poolModule from '../../src/db/pool'; + +jest.mock('pg', () => { + const mPool = { + on: jest.fn(), + query: jest.fn(), + end: jest.fn(), + }; + return { Pool: jest.fn(() => mPool) }; +}); + +jest.mock('../../src/config/env', () => { + let hasDb = true; + return { + env: { + LINODE_DB_HOST: 'localhost', + LINODE_DB_PORT: 5432, + LINODE_DB_USER: 'test', + LINODE_DB_PASSWORD: 'password', + LINODE_DB_NAME: 'testdb', + LINODE_DB_SSL: false, + }, + get hasDatabase() { return hasDb; }, + setHasDatabase(val: boolean) { hasDb = val; } + }; +}); + +describe('db pool', () => { + let mPool: any; + + beforeEach(() => { + (Pool as unknown as jest.Mock).mockClear(); + mPool = new Pool(); + require('../../src/config/env').setHasDatabase(true); + // Because Pool module retains the singleton `pool` instance, we have to clear it before each test to test its instantiation + // However closePool sets it to null, so we just make sure to call it + }); + + afterEach(async () => { + await poolModule.closePool(); + }); + + it('getPool returns null if hasDatabase is false', () => { + require('../../src/config/env').setHasDatabase(false); + const pool = poolModule.getPool(); + expect(pool).toBeNull(); + }); + + it('getPool creates and returns a Pool if hasDatabase is true', () => { + (Pool as unknown as jest.Mock).mockClear(); + const pool = poolModule.getPool(); + expect(pool).toBeDefined(); + expect(Pool).toHaveBeenCalledTimes(1); + + // returns cached pool + const pool2 = poolModule.getPool(); + expect(pool2).toBe(pool); + expect(Pool).toHaveBeenCalledTimes(1); + }); + + it('verifyDatabaseConnection returns false if getPool returns null', async () => { + require('../../src/config/env').setHasDatabase(false); + const result = await poolModule.verifyDatabaseConnection(); + expect(result).toBe(false); + expect(poolModule.isDatabaseHealthy()).toBe(false); + }); + + it('verifyDatabaseConnection returns true and sets connectionVerified if query succeeds', async () => { + mPool.query.mockResolvedValueOnce({ rows: [] }); + const result = await poolModule.verifyDatabaseConnection(); + expect(result).toBe(true); + expect(poolModule.isDatabaseHealthy()).toBe(true); + expect(mPool.query).toHaveBeenCalledWith('SELECT 1'); + }); + + it('verifyDatabaseConnection returns false and unsets connectionVerified if query fails', async () => { + mPool.query.mockRejectedValueOnce(new Error('connection failed')); + const result = await poolModule.verifyDatabaseConnection(); + expect(result).toBe(false); + expect(poolModule.isDatabaseHealthy()).toBe(false); + }); + + it('closePool ends the pool and unsets connectionVerified', async () => { + mPool.query.mockResolvedValueOnce({ rows: [] }); + await poolModule.verifyDatabaseConnection(); + expect(poolModule.isDatabaseHealthy()).toBe(true); + + await poolModule.closePool(); + expect(mPool.end).toHaveBeenCalled(); + expect(poolModule.isDatabaseHealthy()).toBe(false); + }); +});