diff --git a/src/middleware/middleware.builders.js b/src/middleware/middleware.builders.js index 49fcebd2..1afb3d0b 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 0725cdd1..6c79a767 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,20 @@ 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) + 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) }) @@ -136,6 +142,47 @@ 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) + 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'") + }) }) describe('handleRejections', async () => {