1657 permanent fix for parallel organisation lookup

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]

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 () => {