From 09a608f40df5782ab5be48068680cc1b8694fd0c Mon Sep 17 00:00:00 2001 From: Torben Sorensen Date: Thu, 4 Dec 2025 14:03:17 -0800 Subject: [PATCH] refactor: improve type safety by making 'next' optional in async route handlers and updating type imports --- src/routes/admin.ts | 3 +-- src/routes/ai.test.ts | 1 - src/routes/ai.ts | 2 +- src/routes/auth.test.ts | 9 ++++++--- src/routes/passport.test.ts | 7 ++++++- src/routes/public.routes.test.ts | 7 ++++--- src/routes/system.test.ts | 11 +++++------ src/services/aiApiClient.test.ts | 5 ++--- src/services/aiApiClient.ts | 2 +- src/services/aiService.server.ts | 2 +- src/services/db/connection.test.ts | 9 ++++----- src/services/db/user.test.ts | 2 +- src/services/express.d.ts | 20 -------------------- src/types/express.d.ts | 6 +++--- 14 files changed, 35 insertions(+), 51 deletions(-) delete mode 100644 src/services/express.d.ts diff --git a/src/routes/admin.ts b/src/routes/admin.ts index debf99b0..b84e5e38 100644 --- a/src/routes/admin.ts +++ b/src/routes/admin.ts @@ -7,7 +7,6 @@ import multer from 'multer'; import * as db from '../services/db'; import { logger } from '../services/logger.server'; import { UserProfile } from '../types'; -import { AsyncRequestHandler } from '../types/express'; import { clearGeocodeCache } from '../services/geocodingService.server'; // --- Bull Board (Job Queue UI) Imports --- @@ -244,7 +243,7 @@ router.post('/trigger/analytics-report', async (req, res, next: NextFunction) => * POST /api/admin/flyers/:flyerId/cleanup - Enqueue a job to clean up a flyer's files. * This is triggered by an admin after they have verified the flyer processing was successful. */ -router.post('/flyers/:flyerId/cleanup', async (req, res, next: NextFunction) => { +router.post('/flyers/:flyerId/cleanup', async (req, res) => { const adminUser = req.user as UserProfile; const flyerId = parseInt(req.params.flyerId, 10); diff --git a/src/routes/ai.test.ts b/src/routes/ai.test.ts index 747c9258..973b21a3 100644 --- a/src/routes/ai.test.ts +++ b/src/routes/ai.test.ts @@ -4,7 +4,6 @@ import supertest from 'supertest'; import express, { type Request, type Response, type NextFunction } from 'express'; import path from 'node:path'; import aiRouter from './ai'; -import * as aiService from '../services/aiService.server'; import { UserProfile } from '../types'; import * as db from '../services/db'; diff --git a/src/routes/ai.ts b/src/routes/ai.ts index 65c4c5dd..81eef0a4 100644 --- a/src/routes/ai.ts +++ b/src/routes/ai.ts @@ -356,7 +356,7 @@ router.post('/search-web', passport.authenticate('jwt', { session: false }), asy } }); -router.post('/plan-trip', passport.authenticate('jwt', { session: false }), async (req, res, next: NextFunction) => { +router.post('/plan-trip', passport.authenticate('jwt', { session: false }), async (req, res) => { // try { // const { items, store, userLocation } = req.body; // logger.info(`Server-side trip planning requested for user.`); diff --git a/src/routes/auth.test.ts b/src/routes/auth.test.ts index cad64cec..dae0b1f7 100644 --- a/src/routes/auth.test.ts +++ b/src/routes/auth.test.ts @@ -1,7 +1,7 @@ // src/routes/auth.test.ts import { describe, it, expect, vi, beforeEach } from 'vitest'; import supertest from 'supertest'; -import express from 'express'; +import express, { Request, Response, NextFunction } from 'express'; import cookieParser from 'cookie-parser'; import * as bcrypt from 'bcrypt'; import authRouter from './auth'; @@ -33,10 +33,13 @@ vi.mock('bcrypt', async (importOriginal) => { return { ...actual, compare: vi.fn() }; }); +// Define a type for the custom passport callback to avoid `any`. +type PassportCallback = (error: Error | null, user: Express.User | false, info?: { message: string }) => void; + // Mock Passport middleware vi.mock('./passport', () => ({ default: { - authenticate: (strategy: string, options: any, callback: any) => (req: any, res: any, next: any) => { + authenticate: (strategy: string, options: Record, callback: PassportCallback) => (req: Request) => { // Logic to simulate passport authentication outcome based on test input if (req.body.password === 'wrong_password') { // Simulate incorrect credentials @@ -53,7 +56,7 @@ vi.mock('./passport', () => ({ // Default success case const user = { user_id: 'user-123', email: req.body.email }; - callback(null, user, null); + callback(null, user, undefined); }, }, })); diff --git a/src/routes/passport.test.ts b/src/routes/passport.test.ts index 732b6102..f2f25990 100644 --- a/src/routes/passport.test.ts +++ b/src/routes/passport.test.ts @@ -1,6 +1,7 @@ // src/routes/passport.test.ts import { describe, it, expect, vi, beforeEach, type Mocked } from 'vitest'; import { Request, Response, NextFunction } from 'express'; +import { Profile } from '../types'; // Define a type for the JWT verify callback function for type safety. type VerifyCallback = (payload: { user_id: string }, done: (error: Error | null, user?: object | false) => void) => Promise; @@ -65,7 +66,11 @@ describe('Passport Configuration', () => { it('should call done(null, userProfile) on successful authentication', async () => { // Arrange const jwtPayload = { user_id: 'user-123' }; - const mockProfile: any = { user_id: 'user-123', role: 'user' }; + const mockProfile: Profile = { + user_id: 'user-123', + role: 'user', + points: 100, // Add missing required property + }; vi.mocked(mockedDb.findUserProfileById).mockResolvedValue(mockProfile); const done = vi.fn(); diff --git a/src/routes/public.routes.test.ts b/src/routes/public.routes.test.ts index 2f17d9f3..3ad9b7bf 100644 --- a/src/routes/public.routes.test.ts +++ b/src/routes/public.routes.test.ts @@ -5,6 +5,7 @@ import express from 'express'; import publicRouter from './public'; // Import the router we want to test import * as db from '../services/db'; import * as fs from 'fs/promises'; +import { Flyer } from '../types'; // 1. Mock the Service Layer directly. // This decouples the route tests from the SQL implementation details. @@ -114,9 +115,9 @@ describe('Public Routes (/api)', () => { describe('GET /flyers', () => { it('should return a list of flyers on success', async () => { - const mockFlyers: any[] = [ - { flyer_id: 1, file_name: 'flyer_a.jpg', image_url: '/a.jpg', created_at: new Date().toISOString() }, - { flyer_id: 2, file_name: 'flyer_b.jpg', image_url: '/b.jpg', created_at: new Date().toISOString() }, + const mockFlyers: Flyer[] = [ + { flyer_id: 1, file_name: 'flyer_a.jpg', image_url: '/a.jpg', created_at: new Date().toISOString(), item_count: 10 }, + { flyer_id: 2, file_name: 'flyer_b.jpg', image_url: '/b.jpg', created_at: new Date().toISOString(), item_count: 20 }, ]; // Mock the service function vi.mocked(db.getFlyers).mockResolvedValue(mockFlyers); diff --git a/src/routes/system.test.ts b/src/routes/system.test.ts index 17cfcd3b..c6fd9884 100644 --- a/src/routes/system.test.ts +++ b/src/routes/system.test.ts @@ -3,7 +3,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import supertest from 'supertest'; import express from 'express'; import type { ExecException, ChildProcess } from 'child_process'; -import { ExecOptions } from 'child_process'; import systemRouter from './system'; // Define a type for the exec callback to avoid using `any`. @@ -54,7 +53,7 @@ describe('System Routes (/api/system)', () => { // The `exec` callback receives (error, stdout, stderr). For success, error is null. // We must match the overloaded signature of `exec`. The second argument can be options or the callback. // By using `...args: any[]`, we create a generic mock that can handle all overloads. - vi.mocked(exec).mockImplementation((...args: any[]) => { + vi.mocked(exec).mockImplementation((...args: [string, ...unknown[]]) => { // The callback is always the last function argument. const callback = args.find(arg => typeof arg === 'function') as ExecCallback; // For this test, we simulate success by calling the callback with no error. @@ -77,7 +76,7 @@ describe('System Routes (/api/system)', () => { const pm2StoppedOutput = ` │ status │ stopped │ `; - vi.mocked(exec).mockImplementation((...args: any[]) => { + vi.mocked(exec).mockImplementation((...args: [string, ...unknown[]]) => { const callback = args.find(arg => typeof arg === 'function') as ExecCallback; callback(null, pm2StoppedOutput, ''); return {} as ChildProcess; @@ -94,7 +93,7 @@ describe('System Routes (/api/system)', () => { it('should return success: false when pm2 process does not exist', async () => { // Arrange: Simulate the error and stdout when a process is not found. const processNotFoundOutput = "[PM2][ERROR] Process or Namespace flyer-crawler-api doesn't exist"; - vi.mocked(exec).mockImplementation((...args: any[]) => { + vi.mocked(exec).mockImplementation((...args: [string, ...unknown[]]) => { const callback = args.find(arg => typeof arg === 'function') as ExecCallback; callback(new Error('Command failed') as ExecException, processNotFoundOutput, ''); return {} as ChildProcess; @@ -110,7 +109,7 @@ describe('System Routes (/api/system)', () => { it('should return 500 on a generic exec error', async () => { // Arrange: Simulate a generic failure of the `exec` command. - vi.mocked(exec).mockImplementation((...args: any[]) => { + vi.mocked(exec).mockImplementation((...args: [string, ...unknown[]]) => { const callback = args.find(arg => typeof arg === 'function') as ExecCallback; callback(new Error('Generic exec error') as ExecException, '', 'Some stderr output'); return {} as ChildProcess; @@ -128,7 +127,7 @@ describe('System Routes (/api/system)', () => { // Arrange: Simulate a scenario where the command writes to stderr but doesn't // produce a formal error object for the callback's first argument. const stderrMessage = 'A non-fatal warning or configuration issue.'; - vi.mocked(exec).mockImplementation((...args: any[]) => { + vi.mocked(exec).mockImplementation((...args: [string, ...unknown[]]) => { const callback = args.find(arg => typeof arg === 'function') as ExecCallback; callback(null, '', stderrMessage); return {} as ChildProcess; diff --git a/src/services/aiApiClient.test.ts b/src/services/aiApiClient.test.ts index c2bcf676..da3c60a6 100644 --- a/src/services/aiApiClient.test.ts +++ b/src/services/aiApiClient.test.ts @@ -7,7 +7,6 @@ import { http, HttpResponse } from 'msw'; vi.unmock('./aiApiClient'); import * as aiApiClient from './aiApiClient'; -import { FlyerItem } from '../types'; // 1. Mock logger to keep output clean vi.mock('./logger', () => ({ @@ -40,7 +39,7 @@ const server = setupServer( if (contentType?.includes('application/json')) { try { body = await request.json() as Record; - } catch (err) { /* ignore parse error */ } + } catch { /* ignore parse error */ } } else if (contentType?.includes('multipart/form-data')) { try { // This is the key part. We read the formData from the request. @@ -53,7 +52,7 @@ const server = setupServer( } formDataBody._isFormData = true; body = formDataBody; - } catch (err) { /* ignore parse error */ } + } catch { /* ignore parse error */ } } requestSpy({ diff --git a/src/services/aiApiClient.ts b/src/services/aiApiClient.ts index 1b5afb08..bbb363fa 100644 --- a/src/services/aiApiClient.ts +++ b/src/services/aiApiClient.ts @@ -4,7 +4,7 @@ * It communicates with the application's own backend endpoints, which then securely * call the Google AI services. This ensures no API keys are exposed on the client. */ -import type { FlyerItem, MasterGroceryItem, Store } from "../types"; +import type { FlyerItem } from "../types"; import { logger } from "./logger"; import { apiFetchWithAuth } from './apiClient'; diff --git a/src/services/aiService.server.ts b/src/services/aiService.server.ts index e86810c8..f60d2c1c 100644 --- a/src/services/aiService.server.ts +++ b/src/services/aiService.server.ts @@ -274,7 +274,7 @@ export const extractTextFromImageArea = async ( * @param userLocation The user's current geographic coordinates. * @returns A text response with trip planning advice and a list of map sources. */ -export const planTripWithMaps = async (items: FlyerItem[], store: { name: string } | undefined, userLocation: GeolocationCoordinates): Promise<{text: string; sources: { uri: string; title: string; }[]}> => { +export const planTripWithMaps = async (): Promise<{text: string; sources: { uri: string; title: string; }[]}> => { // const topItems = items.slice(0, 5).map(i => i.item).join(', '); // const storeName = store?.name || 'the grocery store'; diff --git a/src/services/db/connection.test.ts b/src/services/db/connection.test.ts index 3e5cad06..dc2bd83f 100644 --- a/src/services/db/connection.test.ts +++ b/src/services/db/connection.test.ts @@ -1,6 +1,5 @@ // src/services/db/connection.test.ts -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { getPool, checkTablesExist, getPoolStatus } from './connection'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; // Mock the logger vi.mock('../logger', () => ({ @@ -71,8 +70,8 @@ describe('DB Connection Service', () => { const pool = getPool(); // Mock the query response on the *instance* returned by getPool - // We cast to any to avoid strict typing issues with the mock injection - (pool.query as any) = vi.fn().mockResolvedValue({ rows: [{ table_name: 'users' }, { table_name: 'flyers' }] }); + // Use `as Mock` for type-safe mocking. + (pool.query as Mock).mockResolvedValue({ rows: [{ table_name: 'users' }, { table_name: 'flyers' }] }); const tableNames = ['users', 'flyers']; const missingTables = await checkTablesExist(tableNames); @@ -85,7 +84,7 @@ describe('DB Connection Service', () => { const { getPool, checkTablesExist } = await import('./connection'); const pool = getPool(); - (pool.query as any) = vi.fn().mockResolvedValue({ rows: [{ table_name: 'users' }] }); + (pool.query as Mock).mockResolvedValue({ rows: [{ table_name: 'users' }] }); const tableNames = ['users', 'flyers', 'products']; const missingTables = await checkTablesExist(tableNames); diff --git a/src/services/db/user.test.ts b/src/services/db/user.test.ts index 0e91248e..b0f5e31d 100644 --- a/src/services/db/user.test.ts +++ b/src/services/db/user.test.ts @@ -51,7 +51,7 @@ describe('User DB Service', () => { totalCount: 0, idleCount: 0, waitingCount: 0, - } as any; + } as unknown as Pool; }); vi.clearAllMocks(); diff --git a/src/services/express.d.ts b/src/services/express.d.ts deleted file mode 100644 index 8280ce97..00000000 --- a/src/services/express.d.ts +++ /dev/null @@ -1,20 +0,0 @@ -// src/services/express.d.ts -import { Request, Response, NextFunction, RequestHandler } from 'express'; - -/** - * Defines a more accurate type for asynchronous Express route handlers. - * Standard `RequestHandler` requires `next` to be present, which is often unused - * in modern async/await handlers, leading to linting errors. This type makes `next` optional. - * - * It also allows for more specific typing of the request body, query, and params. - */ -export type AsyncRequestHandler< - P = Record, - ResBody = any, - ReqBody = any, - ReqQuery = qs.ParsedQs -> = ( - req: Request, - res: Response, - next?: NextFunction -) => Promise; \ No newline at end of file diff --git a/src/types/express.d.ts b/src/types/express.d.ts index 7cf1721a..9598737b 100644 --- a/src/types/express.d.ts +++ b/src/types/express.d.ts @@ -1,5 +1,5 @@ // src/types/express.d.ts -import { Request, Response, NextFunction, RequestHandler } from 'express'; +import { Request, Response, NextFunction } from 'express'; import * as qs from 'qs'; /** @@ -11,8 +11,8 @@ import * as qs from 'qs'; */ export type AsyncRequestHandler< P = Record, - ResBody = any, - ReqBody = any, + ResBody = unknown, + ReqBody = unknown, ReqQuery = qs.ParsedQs > = ( req: Request,