From 782e8358156d9ef3cfa1a692eb4dba02d5bd34e2 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Fri, 19 Jun 2026 13:55:34 +0100 Subject: [PATCH 1/3] temp refactor to recreate #1657 --- src/middleware/lpa-overview.middleware.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/lpa-overview.middleware.js b/src/middleware/lpa-overview.middleware.js index 43842e1f0..8d7e3f2bd 100644 --- a/src/middleware/lpa-overview.middleware.js +++ b/src/middleware/lpa-overview.middleware.js @@ -453,8 +453,8 @@ const fetchOutOfBoundsExpectations = expectationFetcher({ * Organisation (LPA) overview page middleware chain. */ export default [ - fetchOrgInfo, parallel([ + fetchOrgInfo, fetchLocalPlanningGroups, fetchEndpointSummary, fetchEntityIssueCountsPerformanceDb, From 92d2d3d9af87e19af8a9652e689aa940dfcb1613 Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Fri, 19 Jun 2026 14:30:31 +0100 Subject: [PATCH 2/3] Refactor middleware to improve error handling and response management --- src/middleware/lpa-overview.middleware.js | 2 +- src/middleware/middleware.builders.js | 8 +-- .../middleware/middleware.builders.test.js | 59 ++++++++++++++++--- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/middleware/lpa-overview.middleware.js b/src/middleware/lpa-overview.middleware.js index 8d7e3f2bd..43842e1f0 100644 --- a/src/middleware/lpa-overview.middleware.js +++ b/src/middleware/lpa-overview.middleware.js @@ -453,8 +453,8 @@ const fetchOutOfBoundsExpectations = expectationFetcher({ * Organisation (LPA) overview page middleware chain. */ export default [ + fetchOrgInfo, parallel([ - fetchOrgInfo, fetchLocalPlanningGroups, fetchEndpointSummary, fetchEntityIssueCountsPerformanceDb, diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 49fcebd25..1afb3d0b8 100644 --- a/src/middleware/middleware.builders.js +++ b/src/middleware/middleware.builders.js @@ -16,7 +16,7 @@ import { render } from '../utils/custom-renderer.js' import datasette from '../services/datasette.js' import platformApi from '../services/platformApi.js' import * as v from 'valibot' -import { errorTemplateContext, MiddlewareError } from '../utils/errors.js' +import { MiddlewareError } from '../utils/errors.js' import { getDataSubjects } from '../utils/utils.js' @@ -65,8 +65,7 @@ export const datasetOverride = (val, req) => { } const fetchOneFallbackPolicy = (req, res, next) => { - const err = new MiddlewareError('Not found', 404) - res.status(err.statusCode).render(err.template, { ...errorTemplateContext(), err }) + next(new MiddlewareError('Not found', 404)) } /** @@ -75,7 +74,7 @@ const fetchOneFallbackPolicy = (req, res, next) => { */ export const FetchOneFallbackPolicy = { /** - * Renders a 404 response. + * Passes a 404 response to the next error handler. */ 'not-found-error': fetchOneFallbackPolicy, @@ -115,6 +114,7 @@ async function fetchOneFn (req, res, next) { if (result.formattedData.length === 0) { // we can make the 404 more informative by informing the use what exactly was "not found" // Bind the context so the fallback policy can access `this.result` + req.handlerName = `fetching '${this.result}'` fallbackPolicy.call(this, req, res, next) } else { req[this.result] = result.formattedData[0] diff --git a/test/unit/middleware/middleware.builders.test.js b/test/unit/middleware/middleware.builders.test.js index 0725cdd14..e0141399c 100644 --- a/test/unit/middleware/middleware.builders.test.js +++ b/test/unit/middleware/middleware.builders.test.js @@ -1,6 +1,7 @@ import { describe, it, vi, expect, beforeEach } from 'vitest' import datasette from '../../../src/services/datasette.js' import * as middleware from '../../../src/middleware/middleware.builders.js' +import { MiddlewareError } from '../../../src/utils/errors.js' vi.mock('../../../src/services/datasette.js', () => ({ default: { @@ -18,6 +19,12 @@ describe('middleware.builders', () => { result: 'output' } + const mockResponse = () => { + const res = { status: vi.fn() } + res.status.mockReturnValue({ render: vi.fn() }) + return res + } + describe('fetchOne', () => { it('should not apply dataset when option not specified', async () => { const mw = middleware.fetchOne(baseMw) @@ -48,21 +55,17 @@ describe('middleware.builders', () => { expect(queryArgs[1]).toEqual('fun-dataset') }) - const mockResponse = () => { - const res = { status: vi.fn() } - res.status.mockReturnValue({ render: vi.fn() }) - return res - } - - it('should call next with error when no records found', async () => { + it('should call next with 404 error when no records found', async () => { const mw = middleware.fetchOne({ ...baseMw }) vi.mocked(datasette.runQuery).mockResolvedValue({ formattedData: [] }) const req = { params: {} } const res = mockResponse() const next = vi.fn() await mw(req, res, next) - expect(next).toBeCalledTimes(0) - expect(res.status).toHaveBeenCalledWith(404) + expect(next).toHaveBeenCalledTimes(1) + expect(next).toHaveBeenCalledWith(new MiddlewareError('Not found', 404)) + expect(res.status).not.toHaveBeenCalled() + expect(req.handlerName).toBe("fetching 'output'") expect(req).toBe(req) }) @@ -136,6 +139,44 @@ describe('middleware.builders', () => { expect(next).not.toHaveBeenCalledWith(expect.any(Error)) expect(fallbackPolicy).toHaveBeenCalledOnce() }) + + it('waits for slow sibling middleware before sending a fetchOne 404 to the final error handler', async () => { + let releaseSlowMiddleware + const slowMiddleware = vi.fn((_req, _res, next) => { + return new Promise(resolve => { + releaseSlowMiddleware = () => { + next() + resolve() + } + }) + }) + + const mw = middleware.parallel([ + middleware.fetchOne({ ...baseMw, result: 'r1' }), + slowMiddleware + ]) + + vi.mocked(datasette.runQuery).mockResolvedValueOnce({ formattedData: [] }) + + const req = { params: {} } + const res = mockResponse() + const next = vi.fn() + + const middlewarePromise = mw(req, res, next) + await Promise.resolve() + await Promise.resolve() + + expect(next).not.toHaveBeenCalled() + expect(res.status).not.toHaveBeenCalled() + + releaseSlowMiddleware() + await middlewarePromise + + expect(next).toHaveBeenCalledTimes(1) + expect(next).toHaveBeenCalledWith(new MiddlewareError('Not found', 404)) + expect(res.status).not.toHaveBeenCalled() + expect(req.handlerName).toBe("fetching 'r1'") + }) }) describe('handleRejections', async () => { From a06508fce5b67dd691936a78080b961534f01e5f Mon Sep 17 00:00:00 2001 From: Gibah Joseph Date: Mon, 22 Jun 2026 10:18:30 +0100 Subject: [PATCH 3/3] Enhance error handling in middleware tests to verify MiddlewareError properties --- test/unit/middleware/middleware.builders.test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/unit/middleware/middleware.builders.test.js b/test/unit/middleware/middleware.builders.test.js index e0141399c..6c79a7679 100644 --- a/test/unit/middleware/middleware.builders.test.js +++ b/test/unit/middleware/middleware.builders.test.js @@ -63,7 +63,10 @@ describe('middleware.builders', () => { const next = vi.fn() await mw(req, res, next) expect(next).toHaveBeenCalledTimes(1) - expect(next).toHaveBeenCalledWith(new MiddlewareError('Not found', 404)) + const err = next.mock.calls[0][0] + expect(err).toBeInstanceOf(MiddlewareError) + expect(err.message).toBe('Not found') + expect(err.statusCode).toBe(404) expect(res.status).not.toHaveBeenCalled() expect(req.handlerName).toBe("fetching 'output'") expect(req).toBe(req) @@ -173,7 +176,10 @@ describe('middleware.builders', () => { await middlewarePromise expect(next).toHaveBeenCalledTimes(1) - expect(next).toHaveBeenCalledWith(new MiddlewareError('Not found', 404)) + const err = next.mock.calls[0][0] + expect(err).toBeInstanceOf(MiddlewareError) + expect(err.message).toBe('Not found') + expect(err.statusCode).toBe(404) expect(res.status).not.toHaveBeenCalled() expect(req.handlerName).toBe("fetching 'r1'") })