refactor(backend): Split monolithic router into separate controllers (#168)

* Refactor router into separate controllers

* Fix extensions in imports

* Fix timeout when handler does not respond

* Add controller unit test

Author: Holzchopf

package-lock.json

@@ -57,6 +57,7 @@
     "@types/express": "^5.0.0",
     "@types/jasmine": "~5.1.0",
     "@types/jsonwebtoken": "^9.0.8",
+    "@types/supertest": "^6.0.2",
     "angular-eslint": "19.0.0",
     "autoprefixer": "^10.4.20",
     "eslint": "^9.15.0",
@@ -78,6 +79,7 @@
     "prettier-plugin-organize-imports": "^4.1.0",
     "prettier-plugin-packagejson": "^2.5.6",
     "prettier-plugin-tailwindcss": "^0.6.9",
+    "supertest": "^7.0.0",
     "tailwindcss": "^3.4.16",
     "tsc-alias": "^1.8.10",
     "typescript": "~5.6.2",

package.json

@@ -0,0 +1,38 @@
+import { appendDir } from '@common/endpoint-utils.js'
+import { Benchmark, BenchmarkPreview } from '@common/interfaces.js'
+import { StripDir } from '@common/utility-types.js'
+import { configuration } from '../config/config.mjs'
+import { SqlService } from '../services/sql-service.mjs'
+import { Controller, GetHandler } from './controller.mjs'
+
+export class BenchmarkController extends Controller {
+  constructor(config: configuration) {
+    super(config)
+
+    this.attachGet('/benchmarks', this.getBenchmarks)
+    this.attachGet('/benchmarks/:id', this.getBenchmarkById)
+  }
+
+  getBenchmarks: GetHandler<'/benchmarks'> = async (req, res) => {
+    const sql = SqlService.getInstance()
+    const rows = await sql.query<StripDir<BenchmarkPreview>>`
+      SELECT id, name, description FROM benchmarks
+      ORDER BY id ASC
+    `
+    const resources = appendDir('/benchmarks/', rows)
+    this.respond(res, resources)
+  }
+
+  getBenchmarkById: GetHandler<'/benchmarks/:id'> = async (req, res) => {
+    const ids = req.params.id.split(',').map((s) => +s)
+    const sql = SqlService.getInstance()
+    // id=ANY - dev.003
+    const rows = await sql.query<StripDir<Benchmark>>`
+      SELECT * FROM benchmarks
+      WHERE id=ANY(${ids})
+      LIMIT ${ids.length}
+    `
+    const benchmarks = appendDir('/benchmarks/', rows)
+    this.respond(res, benchmarks)
+  }
+}

ts/backend/src/app/features/controller/benchmark.controller.mts

@@ -0,0 +1,181 @@
+import { ApiEndpointsOfVerb, ApiGetEndpoints, ApiPatchEndpoints, ApiPostEndpoints } from '@common/api-endpoints.js'
+import { ApiResponse } from '@common/api-response.js'
+import express, { NextFunction, Request, Response, Router } from 'express'
+import type { RouteParameters } from 'express-serve-static-core'
+import { configuration } from '../config/config.mjs'
+
+/**
+ * Type for handlers for given method.
+ */
+export type HandlerOfVerb<V extends string, E extends keyof ApiEndpointsOfVerb<V>> = (
+  req: Request<
+    // allows inferring ":parameters"
+    RouteParameters<E>,
+    // unknown, due to chaining
+    unknown,
+    // type of request body and query taken from ApiEndpointDefinitions
+    ApiEndpointsOfVerb<V>[E]['request']['body'],
+    ApiEndpointsOfVerb<V>[E]['request']['query']
+  >,
+  res: Response<ApiEndpointsOfVerb<V>[E]['response']>,
+  next: NextFunction,
+) => void | Promise<void>
+
+/**
+ * Type for GET handlers.
+ */
+export type GetHandler<E extends keyof ApiGetEndpoints> = HandlerOfVerb<'GET', E>
+
+/**
+ * Type for POST handlers.
+ */
+export type PostHandler<E extends keyof ApiPostEndpoints> = HandlerOfVerb<'POST', E>
+
+/**
+ * Type for PATCH handlers.
+ */
+export type PatchHandler<E extends keyof ApiPatchEndpoints> = HandlerOfVerb<'PATCH', E>
+
+/**
+ * Base class for controllers.
+ * In your child class, attach handlers using `attach<Verb>` methods in the
+ * constructor.
+ */
+export class Controller {
+  /** Router object to pass in `express.use` */
+  router: Router
+
+  constructor(public config: configuration) {
+    this.router = express.Router()
+  }
+
+  /**
+   * Send a well-typed JSON response with optional debug info.
+   * @param res Express response.
+   * @param body Response body. Type is derived from endpoint registry.
+   * @param dbg Additional debug info.
+   * @see {@link ApiResponse}
+   */
+  respond<T>(res: Response<ApiResponse<T>>, body: T, dbg?: unknown) {
+    res.json({
+      body: body,
+      dbg,
+    })
+  }
+
+  /**
+   * Send a well-typed error with code 400 (Bad request), additional error text
+   * and optional debug info.
+   * @param res Express response.
+   * @param error Error object.
+   * @param dbg Additional debug info.
+   * @see {@link ApiResponse}
+   */
+  requestError<T>(res: Response<ApiResponse<T>>, error: ApiResponse<T>['error'], dbg?: unknown) {
+    res.status(400)
+    res.json({
+      error,
+      dbg,
+    })
+  }
+
+  /**
+   * Send a well-typed error with code 401 (Unauthorized), additional
+   * error text and optional debug info.
+   * @param res Express response.
+   * @param error Error object.
+   * @param dbg Additional debug info.
+   * @see {@link ApiResponse}
+   */
+  unauthorizedError<T>(res: Response<ApiResponse<T>>, error: ApiResponse<T>['error'], dbg?: unknown) {
+    res.status(401)
+    res.json({
+      error,
+      dbg,
+    })
+  }
+
+  /**
+   * Send a well-typed error with code 500 (Internal server error), additional
+   * error text and optional debug info.
+   * @param res Express response.
+   * @param error Error object.
+   * @param dbg Additional debug info.
+   * @see {@link ApiResponse}
+   */
+  serverError<T>(res: Response<ApiResponse<T>>, error: ApiResponse<T>['error'], dbg?: unknown) {
+    res.status(500)
+    res.json({
+      error,
+      dbg,
+    })
+  }
+
+  // Attach handler wrapper, serves two purposes:
+  // 1. Provides a common try catch wrap around the handler.
+  // 2. Allows type inferring from route (Express' types are too convoluted to
+  //    be overriden by declares IMHO).
+  private attach<V extends 'get' | 'post' | 'patch', E extends keyof ApiEndpointsOfVerb<Uppercase<V>>>(
+    verb: V,
+    endpoint: E,
+    handler: HandlerOfVerb<Uppercase<V>, E>,
+  ) {
+    this.router[verb](endpoint, async (req, res, next) => {
+      try {
+        await handler(req, res, next)
+        // force a server error if the handler did not respond
+        if (!res.writableEnded) {
+          next('Handler did not respond')
+        }
+      } catch (error) {
+        next(error)
+      }
+    })
+  }
+
+  /**
+   * Attach handler for GET endpoint.
+   */
+  attachGet<E extends keyof ApiGetEndpoints>(endpoint: E, handler: GetHandler<E>) {
+    this.attach('get', endpoint, handler)
+  }
+
+  /**
+   * Attach handler for POST endpoint.
+   */
+  attachPost<E extends keyof ApiPostEndpoints>(endpoint: E, handler: PostHandler<E>) {
+    this.attach('post', endpoint, handler)
+  }
+
+  /**
+   * Attach handler for PATCH endpoint.
+   */
+  attachPatch<E extends keyof ApiPatchEndpoints>(endpoint: E, handler: PatchHandler<E>) {
+    this.attach('patch', endpoint, handler)
+  }
+}
+
+/**
+ * Returns a short recap of requested endpoint (method + url) for logging.
+ */
+export function dbgRequestEndpoint(req: Request) {
+  return `${req.method} ${req.url}`
+}
+
+/**
+ * Returns an object based off Request reduced to the valuable information.
+ */
+export function dbgRequestObject(req: Request) {
+  return {
+    // full request URL (e.g. /mirror/1?test=2&tester=3)
+    url: req.url,
+    // query params (e.g. {test: 2, tester: 3})
+    query: req.query,
+    // path (e.g. /mirror/1)
+    path: req.path,
+    // params (e.g. {id: 1})
+    params: req.params,
+    // body (JSON, e.g. when POST requesting)
+    body: req.body,
+  }
+}

ts/backend/src/app/features/controller/controller.mts

@@ -0,0 +1,99 @@
+import type { Express } from 'express'
+import express from 'express'
+import supertest from 'supertest'
+import TestAgent from 'supertest/lib/agent'
+import { beforeAll, describe, expect, test } from 'vitest'
+import { defaults } from '../config/defaults.mjs'
+import { Controller } from './controller.mjs'
+
+describe.sequential('Controller', () => {
+  let app: Express
+  let request: TestAgent
+  let controller: Controller
+
+  beforeAll(() => {
+    app = express()
+    request = supertest(app)
+  })
+
+  test('should be instantiated (with default configuration)', () => {
+    controller = new Controller(defaults)
+    expect(controller.router).toBeTruthy()
+    app.use(controller.router)
+  })
+
+  // All `attach<Verb>` methods are aliases for the private `attach`. Hence let
+  // us assume that by testing one (i.e. `attachGet`), all of them are tested
+  // sufficiently.
+  test('should be able to attach handlers', () => {
+    // can't attach to undefined endpoints, but can trick TS into see them as defined
+    controller.attachGet('/test-get' as '/mirror', (req, res) => {
+      controller.respond(res, '<ok>', '<debug>')
+    })
+    controller.attachGet('/test-request-error' as '/mirror', (req, res) => {
+      controller.requestError(res, { text: 'request error' })
+    })
+    controller.attachGet('/test-auth-error' as '/mirror', (req, res) => {
+      controller.unauthorizedError(res, { text: 'auth error' })
+    })
+    controller.attachGet('/test-server-error' as '/mirror', (req, res) => {
+      controller.serverError(res, { text: 'server error' })
+    })
+    // test fallback error handler with exception
+    controller.attachGet('/test-catch' as '/mirror', (_req, _res) => {
+      throw 'error'
+    })
+    // test fallback error handler with undefined handler response
+    controller.attachGet('/test-undefined' as '/mirror', (_req, _res) => {
+      /* */
+    })
+    // test for routes presence
+    const routes = getControllerRoutes(controller)
+    expect(routes).toContain('/test-get')
+    expect(routes).toContain('/test-request-error')
+    expect(routes).toContain('/test-auth-error')
+    expect(routes).toContain('/test-server-error')
+    expect(routes).toContain('/test-catch')
+    expect(routes).toContain('/test-undefined')
+    // no more than the the explicitely attached routes should be present
+    expect(routes).toHaveLength(6)
+  })
+
+  test.each([
+    {
+      route: '/test-get',
+      expects: { status: 200, response: { body: '<ok>', dbg: '<debug>' } },
+    },
+    {
+      route: '/test-request-error',
+      expects: { status: 400, response: { error: { text: 'request error' } } },
+    },
+    {
+      route: '/test-auth-error',
+      expects: { status: 401, response: { error: { text: 'auth error' } } },
+    },
+    {
+      route: '/test-server-error',
+      expects: { status: 500, response: { error: { text: 'server error' } } },
+    },
+    // fallback error handler does not set a response body
+    {
+      route: '/test-catch',
+      expects: { status: 500, response: {} },
+    },
+    {
+      route: '/test-undefined',
+      expects: { status: 500, response: {} },
+    },
+  ])('should answer $route with $expects', async (testCase) => {
+    const res = await request.get(testCase.route)
+    expect(res.statusCode).toBe(testCase.expects.status)
+    if (typeof testCase.expects.response !== 'undefined') {
+      expect(res.body).toEqual(testCase.expects.response)
+    }
+  })
+})
+
+function getControllerRoutes(controller: Controller): string[] {
+  return controller.router.stack.filter((r) => r.route).map((r) => r.route!.path)
+}