From 88c431f5309f37632740504328420d22e3b57b75 Mon Sep 17 00:00:00 2001
From: Thomas Boutell <tom@apostrophecms.com>
Date: Mon, 11 Sep 2023 16:17:02 -0400
Subject: [PATCH] hotfix for logging non-Error exceptions from api routes

---
 CHANGELOG.md                           |  8 +++
 modules/@apostrophecms/module/index.js |  2 +-
 package.json                           |  2 +-
 test/weird-api-errors.js               | 81 ++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 2 deletions(-)
 create mode 100644 test/weird-api-errors.js

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cb3c642ddf..35e6fbc0f7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # Changelog
 
+## 3.55.1
+
+### Fixes
+
+* The structured logging for API routes now responds properly if an API route throws a `string` as an exception, rather than
+a politely `Error`-derived object with a `stack` property. Previously this resulted in an error message about the logging
+system itself, which was not useful for debugging the original exception.
+
 ## 3.55.0
 
 ### Adds
diff --git a/modules/@apostrophecms/module/index.js b/modules/@apostrophecms/module/index.js
index ee9c829e16..9b07cce3f4 100644
--- a/modules/@apostrophecms/module/index.js
+++ b/modules/@apostrophecms/module/index.js
@@ -258,7 +258,7 @@ module.exports = {
             self.logError(req, `api-error${typeTrail}`, msg, {
               name: response.name,
               status: response.code,
-              stack: error.stack.split('\n').slice(1).map(line => line.trim()),
+              stack: (error.stack || '').split('\n').slice(1).map(line => line.trim()),
               errorPath: response.path,
               data: response.data
             });
diff --git a/package.json b/package.json
index 07fbe121ca..cfee4efcbc 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "apostrophe",
-  "version": "3.55.0",
+  "version": "3.55.1",
   "description": "The Apostrophe Content Management System.",
   "main": "index.js",
   "scripts": {
diff --git a/test/weird-api-errors.js b/test/weird-api-errors.js
new file mode 100644
index 0000000000..2963ede0ee
--- /dev/null
+++ b/test/weird-api-errors.js
@@ -0,0 +1,81 @@
+const t = require('../test-lib/test.js');
+const assert = require('assert');
+
+describe('Don\'t crash on weird API errors', function() {
+
+  after(async function() {
+    return t.destroy(apos);
+  });
+
+  this.timeout(t.timeout);
+
+  let apos;
+
+  it('should initialize apos', async function() {
+    apos = await t.create({
+      root: module,
+      modules: {
+        'api-test': {
+          apiRoutes(self) {
+            return {
+              get: {
+                fetchItFine(req) {
+                  return {
+                    nifty: true
+                  };
+                },
+                fetchItFailWeird(req) {
+                  throw 'not-an-error-object';
+                },
+                fetchItFailNormal(req) {
+                  throw new Error('normal error');
+                }
+              }
+            };
+          }
+        }
+      }
+    });
+  });
+  it('should fetch fine in the normal case', async function() {
+    const body = await apos.http.get('/api/v1/api-test/fetch-it-fine', {});
+    assert(typeof body === 'object');
+    assert.strictEqual(body.nifty, true);
+  });
+  it('should fail politely in the weird case of a non-Error exception', async function() {
+    let msgWas;
+    const consoleError = console.error;
+    console.error = msg => {
+      msgWas = msg;
+    };
+    try {
+      await apos.http.get('/api/v1/api-test/fetch-it-fail-weird', {});
+      // Should not get here
+      assert(false);
+    } catch (e) {
+      // Make sure the logging system itself is not at fault
+      assert(!msgWas.toString().includes('Structured logging error'));
+    } finally {
+      console.error = consoleError;
+      console.error(msgWas);
+    }
+  });
+  it('should fail politely in the normal case of an Error exception', async function() {
+    let msgWas;
+    const consoleError = console.error;
+    console.error = msg => {
+      msgWas = msg;
+    };
+    try {
+      await apos.http.get('/api/v1/api-test/fetch-it-fail-normal', {});
+      // Should not get here
+      assert(false);
+    } catch (e) {
+      // Make sure the logging system itself is not at fault
+      assert(!msgWas.toString().includes('Structured logging error'));
+    } finally {
+      console.error = consoleError;
+      console.error(msgWas);
+    }
+  });
+});