Improve route error handling consistency from TinyPEN changes (#111)

* Improve route error handling consistency

* Add RERUM fetch helper; improve route errors

Introduce rerum.js with fetchRerum (timeout/abort handling) and helpers for network/timeout errors. Update create, update, overwrite, delete and query routes to use fetchRerum, add Origin header and unified error handling, validate request bodies/params more strictly, and surface RERUM textual error bodies as 502 responses. Also handle id/_id normalization, reject empty queries, and specially return 409 conflict bodies from overwrite.

* Improve error handling and validation

Add explicit error body handling and streamline error propagation. error-messenger now detects ReadableStream bodies and uses an explicitBody field (fixing a typo in the default message). rest.httpError sets error.errorBody when a body is provided so original bodies are preserved. Routes enforce request body presence before checking @id and now throw a 400 error instead of directly sending a response (overwrite.js, update.js). query.js clarifies the limit/skip validation message to require non-negative integers. Tests updated to reflect changed status codes/messages and the query tests now mount the messenger middleware.

* Use httpError helper and payload for errors

Standardize error handling by introducing/using httpError and a `payload` property for error bodies. rest.js now sets error.payload instead of errorBody/body. error-messenger.js was simplified to send rerum_error_res.payload and dropped the ReadableStream/body detection logic. Routes (overwrite, query, update) now import and throw httpError for validation and upstream RERUM errors instead of creating Error objects and manually setting status codes. This centralizes HTTP error construction and response payload handling.

* Rename httpError parameter to payload

Change httpError signature from (message, status, body) to (message, status, payload) and set error.payload = payload. Note: the conditional still checks 'body !== undefined', which appears to be an inconsistent leftover and may prevent payload from being attached; update the check to 'payload !== undefined' as a follow-up.

* fixes all tests

---------

Co-authored-by: Bryan Haberberger <bryan.j.haberberger@slu.edu>

Author: cubap

UPSTREAM_CANDIDATES.md

@@ -15,7 +15,7 @@ This document tracks candidate updates identified from TinyPen that may be upstr
 | C-001 | Content-Type validation middleware for JSON endpoints | upstream-now | PR #39 | `routes/query.js`, `routes/create.js`, `routes/update.js`, `routes/delete.js`, `routes/overwrite.js` | Detect malformed or unsupported media types and return 415. |
 | C-002 | Query body validation for empty object/array | upstream-now | PR #33 | `routes/query.js`, `routes/__tests__/query.test.js` | Reject empty query payloads with 400. |
 | C-003 | Query pagination validation for `limit` and `skip` | upstream-now | PR #33 | `routes/query.js`, `routes/__tests__/query.test.js` | Require non-negative integers when provided. |
-| C-004 | Route-level error semantic consistency | investigate-first | PR #39 and related | `routes/*.js`, `error-messenger.js` | Normalize error objects/status handling before broad refactor. |
+| C-004 | Route-level error semantic consistency | upstream-now | PR #39 and related | `routes/*.js`, `error-messenger.js` | Partial implementation landed: explicit 502 network failures, structured JSON passthrough, and fixed upstream propagation defects. Remaining route-specific parity can follow later. |
 | C-005 | Dependency cleanup after Node upgrade | investigate-first | TinyPen dependency diffs | `package.json` | Verify runtime paths before removing deps (`morgan`, `dotenv-expand`, `jsonld`). |
 | C-006 | Optimistic locking parity improvements | investigate-first | PR #20 | `routes/overwrite.js`, `routes/__tests__/overwrite.test.js` | Keep current behavior, evaluate if more parity is needed. |
 | C-007 | Temporary test alignment imports from TinyPen | upstream-now | PR #35 context | `routes/__tests__/*` | Include useful generated coverage temporarily while test strategy evolves. |
@@ -42,3 +42,13 @@ This document tracks candidate updates identified from TinyPen that may be upstr
 3. Full test suite passes before merge.
 4. Dependency changes include runtime verification notes.
 5. Uncertain candidates remain blocked until evidence is upgraded from investigate-first to upstream-now.
+
+## Deferred Test Cases
+
+These scenarios are worth adding as follow-up coverage, but do not block the current implementation slice.
+
+1. `create`, `update`, `delete`, and `overwrite` should each preserve upstream text error bodies and status codes.
+2. `update`, `delete`, and `overwrite` should each map network failures to `502 Bad Gateway`.
+3. `overwrite` should preserve its special `409` JSON conflict response when `If-Overwritten-Version` negotiation fails.
+4. `delete/:id` should verify upstream non-2xx responses are propagated through the shared messenger instead of failing locally.
+5. Shared error handling should verify fallback behavior for generic thrown `Error` objects without upstream response metadata.

error-messenger.js

@@ -1,6 +1,7 @@
 /**
  * Errors from RERUM are a response code with a text body (except those handled specifically upstream)
  * We want to send the same error code and message through.  It is assumed to be RESTful and useful.
+ * This is a fallback for unhandled errors from RERUM, and should not be used for expected errors that are handled upstream (e.g. 409 conflict for version mismatch on overwrite).  It will also handle network errors that occur when trying to reach RERUM, which will be mapped to a 502 Bad Gateway with a message indicating a RERUM network error.
  * This will also handle generic (500) app level errors, as well as app level 404 errors.
  *
  * @param rerum_error_res A Fetch API Response object from a fetch() to RERUM that encountered an error.  Explanatory text is in .text().  In some cases it is a unhandled generic (500) app level Error.
@@ -9,26 +10,40 @@
  * @param next The Express next() operator, unused here but required to support the middleware chain.
  */
 export async function messenger(rerum_error_res, req, res, next) {
-        if (res.headersSent) {
-                return
-        }
-        let error = {}
-        let rerum_err_text
-        try {
-                // Unless already handled upstream the rerum_error_res is an error Response with details as a textual body.
-                rerum_err_text = await rerum_error_res.text()
-        }
-        catch (err) {
-                // It is some 500
-                rerum_err_text = undefined
+    if (res.headersSent) {
+        return
+    }
+
+    const error = {
+        message: rerum_error_res.statusMessage ?? rerum_error_res.message ?? "A server error has occurred",
+        status: rerum_error_res.statusCode ?? rerum_error_res.status ?? 500,
+    }
+
+    if (rerum_error_res.payload !== undefined) {
+        console.error(error)
+        res.status(error.status).json(rerum_error_res.payload)
+        return
+    }
+
+    try {
+        const contentType = rerum_error_res.headers?.get?.("Content-Type")?.toLowerCase?.() ?? ""
+        if (contentType.includes("json")) {
+            error.body = await rerum_error_res.json()
+            console.error(error)
+            res.status(error.status).json(error.body)
+            return
         }
-        if (rerum_err_text) error.message = rerum_err_text
-        else { 
-                // Perhaps this is a more generic 500 from the app, perhaps involving RERUM, and there is no good rerum_error_res
-                error.message = rerum_error_res.statusMessage ?? rerum_error_res.message ?? `A server error has occurred` 
+
+        const rerumErrText = await rerum_error_res.text()
+        if (rerumErrText) {
+            error.message = rerumErrText
         }
-        error.status = rerum_error_res.statusCode ?? rerum_error_res.status ?? 500
-        console.error(error)
-        res.set("Content-Type", "text/plain; charset=utf-8")
-        res.status(error.status).send(error.message)
+    }
+    catch (err) {
+        // Fall back to the status/message values already collected above.
+    }
+
+    console.error(error)
+    res.set("Content-Type", "text/plain; charset=utf-8")
+    res.status(error.status).send(error.message)
 }

rerum.js

@@ -0,0 +1,62 @@
+const DEFAULT_RERUM_TIMEOUT_MS = 30000
+
+const getRerumTimeoutMs = () => {
+  const timeoutMs = Number.parseInt(process.env.RERUM_FETCH_TIMEOUT_MS ?? `${DEFAULT_RERUM_TIMEOUT_MS}`, 10)
+  return Number.isFinite(timeoutMs) && timeoutMs > 0 ? timeoutMs : DEFAULT_RERUM_TIMEOUT_MS
+}
+
+/**
+ * Build the generic upstream error used when RERUM cannot be reached or returns invalid data.
+ *
+ * @param {string} url - The RERUM URL being requested
+ * @returns {Error}
+ */
+const createRerumNetworkError = (url) => {
+  const err = new Error(`500: ${url} - A RERUM error occurred`)
+  err.status = 502
+  return err
+}
+
+/**
+ * Build an upstream timeout error for requests that exceed the configured wait time.
+ *
+ * @param {string} url - The RERUM URL being requested
+ * @param {number} timeoutMs - The timeout in milliseconds
+ * @returns {Error}
+ */
+const createRerumTimeoutError = (url, timeoutMs) => {
+  const err = new Error(`504: ${url} - RERUM did not respond within ${timeoutMs}ms`)
+  err.status = 504
+  return err
+}
+
+/**
+ * Execute a fetch to RERUM with a bounded wait time so workers do not block indefinitely.
+ *
+ * @param {string} url - The RERUM URL being requested
+ * @param {RequestInit} [options={}] - Fetch options
+ * @returns {Promise<Response>}
+ */
+async function fetchRerum(url, options = {}) {
+  const timeoutMs = getRerumTimeoutMs()
+  const timeoutController = new AbortController()
+  const timeoutId = setTimeout(() => timeoutController.abort(), timeoutMs)
+  const signal = options.signal
+    ? AbortSignal.any([options.signal, timeoutController.signal])
+    : timeoutController.signal
+
+  try {
+    return await fetch(url, { ...options, signal })
+  }
+  catch (err) {
+    if (err?.name === "AbortError" && timeoutController.signal.aborted) {
+      throw createRerumTimeoutError(url, timeoutMs)
+    }
+    throw createRerumNetworkError(url)
+  }
+  finally {
+    clearTimeout(timeoutId)
+  }
+}
+
+export { createRerumNetworkError, fetchRerum }

rest.js

@@ -3,6 +3,15 @@ const acceptedJsonContentTypes = new Set([
   "application/ld+json"
 ])
 
+export function httpError(message, status = 500, payload) {
+  const error = new Error(message)
+  error.status = status
+  if (payload !== undefined) {
+    error.payload = payload
+  }
+  return error
+}
+
 function getContentType(req) {
   const rawContentType = req.headers?.["content-type"]
   if (!rawContentType) {

routes/__tests__/create.test.js

@@ -3,13 +3,15 @@ import request from "supertest"
 import { jest } from "@jest/globals"
 
 import createRoute from "../create.js"
+import { messenger } from "../../error-messenger.js"
 //import app from "../../app.js"
 
 const routeTester = new express()
 routeTester.use(express.json())
 routeTester.use(express.urlencoded({ extended: false }))
 routeTester.use("/create", createRoute)
 routeTester.use("/app/create", createRoute)
+routeTester.use(messenger)
 
 const rerum_uri = `${process.env.RERUM_ID_PATTERN}_not_`
 
@@ -108,6 +110,41 @@ describe("Check that incorrect TinyNode create route usage results in expected R
   })
 })
 
+describe("Check that TinyNode create route propagates upstream and network errors predictably.  __rest __core", () => {
+  it("Preserves upstream text errors and maps network failures to 502.", async () => {
+    global.fetch = jest.fn(() =>
+      Promise.resolve({
+        ok: false,
+        status: 503,
+        headers: {
+          get: () => "text/plain; charset=utf-8"
+        },
+        text: () => Promise.resolve("Upstream create failure")
+      })
+    )
+
+    let response = await request(routeTester)
+      .post("/create")
+      .set("Content-Type", "application/json")
+      .send({ "test": "item" })
+      .then(resp => resp)
+      .catch(err => err)
+    expect(response.statusCode).toBe(502)
+    expect(response.text).toContain("Upstream create failure")
+
+    global.fetch = jest.fn(() => Promise.reject(new Error("socket hang up")))
+
+    response = await request(routeTester)
+      .post("/create")
+      .set("Content-Type", "application/json")
+      .send({ "test": "item" })
+      .then(resp => resp)
+      .catch(err => err)
+    expect(response.statusCode).toBe(502)
+    expect(response.text).toContain("A RERUM error occurred")
+  })
+})
+
 /**
  * Full integration test.  Checks the TinyNode app create endpoint functionality and RERUM connection.
  * 

routes/__tests__/error-messenger.test.js

@@ -0,0 +1,30 @@
+import express from "express"
+import request from "supertest"
+import { messenger } from "../../error-messenger.js"
+
+const app = express()
+
+app.get("/json-error", (req, res, next) => {
+  next({
+    status: 422,
+    headers: {
+      get: () => "application/json"
+    },
+    json: async () => ({ message: "Invalid payload", field: "name" })
+  })
+})
+
+app.use(messenger)
+
+describe("Check shared error messenger behavior.  __rest __core", () => {
+  it("Returns structured JSON error bodies when upstream responds with JSON.", async () => {
+    const response = await request(app)
+      .get("/json-error")
+      .then(resp => resp)
+      .catch(err => err)
+
+    expect(response.statusCode).toBe(422)
+    expect(response.body.message).toBe("Invalid payload")
+    expect(response.body.field).toBe("name")
+  })
+})

routes/__tests__/mount.test.js

@@ -32,23 +32,32 @@ afterEach(() => {
  * @returns {boolean} - True if all routes are found
  */
 function routeExists(routes) {
-  const stack = app.router.stack
   const foundRoutes = new Set()
-  for (const layer of stack) {
-    if (layer.matchers && layer.matchers[0]) {
-      const matcher = layer.matchers[0]
-      // Test each route against this layer's matcher
-      for (const route of routes) {
-        if (matcher(route)) foundRoutes.add(route)
+  const scanStack = (stack = []) => {
+    for (const layer of stack) {
+      if (layer.matchers && layer.matchers[0]) {
+        const matcher = layer.matchers[0]
+        for (const route of routes) {
+          if (matcher(route)) foundRoutes.add(route)
+        }
       }
-    }
-    // Also check route.path directly if it exists
-    if (layer.route && layer.route.path) {
-      for (const route of routes) {
-        if (layer.route.path === route) foundRoutes.add(route)
+      if (layer.regexp) {
+        for (const route of routes) {
+          if (layer.regexp.test(route)) foundRoutes.add(route)
+        }
+      }
+      if (layer.route && layer.route.path) {
+        for (const route of routes) {
+          if (layer.route.path === route) foundRoutes.add(route)
+        }
+      }
+      if (layer.handle?.stack) {
+        scanStack(layer.handle.stack)
       }
     }
   }
+
+  scanStack(app._router?.stack)
   // Check if all expected routes were found
   return routes.every(route => foundRoutes.has(route))
 }

routes/__tests__/query.test.js

@@ -2,13 +2,15 @@ import express from "express"
 import request from "supertest"
 import { jest } from "@jest/globals"
 import queryRoute from "../query.js"
+import { messenger } from "../../error-messenger.js"
 //import app from "../../app.js"
 
 const routeTester = new express()
 routeTester.use(express.json())
 routeTester.use(express.urlencoded({ extended: false }))
 routeTester.use("/query", queryRoute)
 routeTester.use("/app/query", queryRoute)
+routeTester.use(messenger)
 
 const rerum_uri = `${process.env.RERUM_ID_PATTERN}_not_`
 

routes/create.js

@@ -1,44 +1,55 @@
 import express from "express"
 import checkAccessToken from "../tokens.js"
 import { verifyJsonContentType } from "../rest.js"
+import { createRerumNetworkError, fetchRerum } from "../rerum.js"
 const router = express.Router()
 
 /* POST a create to the thing. */
 router.post('/', verifyJsonContentType, checkAccessToken, async (req, res, next) => {
 
   try {
+    // if an id is passed in, pop off the end to make it an _id
+    if (req.body.id) {
+      req.body._id = req.body.id.split('/').pop()
+    }
+
     // check body for JSON
-    const body = JSON.stringify(req.body)
+    const createBody = JSON.stringify(req.body)
     const createOptions = {
       method: 'POST',
-      body,
+      body: createBody,
       headers: {
         'user-agent': 'Tiny-Things/1.0',
+        'Origin': process.env.ORIGIN,
         'Authorization': `Bearer ${process.env.ACCESS_TOKEN}`,
         'Content-Type' : "application/json;charset=utf-8"
       }
     }
     const createURL = `${process.env.RERUM_API_ADDR}create`
-    let errored = false
-    const result = await fetch(createURL, createOptions).then(res=>{
-      if (res.ok) return res.json()
-      errored = true
-      return res
-    })
-    .catch(err => {
+    const rerumResponse = await fetchRerum(createURL, createOptions)
+    .then(async (resp) => {
+      if (resp.ok) return resp.json()
+      // The response from RERUM indicates a failure, likely with a specific code and textual body
+      let rerumErrorMessage
+      try {
+        rerumErrorMessage = `${resp.status ?? 500}: ${createURL} - ${await resp.text()}`
+      } catch (e) {
+        rerumErrorMessage = `500: ${createURL} - A RERUM error occurred`
+      }
+      const err = new Error(rerumErrorMessage)
+      err.status = 502
       throw err
     })
-    // Send RERUM error responses to error-messenger.js
-    if (errored) return next(result)
-    const location = result?.["@id"] ?? result?.id
-    const responseBody = { ...req.body, ...(result ?? {}) }
-    if (location) {
-      res.setHeader("Location", location)
+    if (!(rerumResponse.id || rerumResponse["@id"])) {
+      // A 200 with garbled data, call it a fail
+      throw createRerumNetworkError(createURL)
     }
-    res.status(201).json(responseBody)
+    res.setHeader("Location", rerumResponse["@id"] ?? rerumResponse.id)
+    res.status(201).json(rerumResponse)
   }
   catch (err) {
-    next(err)
+    console.error(err)
+    res.status(err.status ?? 500).type('text/plain').send(err.message ?? 'An error occurred')
   }
 })