Repository: jsonata-js/jsonata (js)

Issue:
`$base64decode` failure gives an incorrect error message
If I run

```js
jsonata('$base64decode("not base64")').evaluate()
```

then a seemingly unrelated exception is thrown:

```
Uncaught TypeError: Cannot assign to read only property 'message' of object ''
```

This happens because internally the `$base64decode` method uses [`atob`](https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/atob). When `atob` fails, it throws a [`DOMException`](https://developer.mozilla.org/en-US/docs/Web/API/DOMException). Later, `jsonata` attempts to reassign the `message` property of the `DOMException`:

```js
} catch (err) {
    // insert error message into structure
    err.message = lookupMessage(err);
    throw err;
}
```

However, while this is allowed for most `Error` objects, [the `message` property of a `DOMException` is read-only](https://developer.mozilla.org/en-US/docs/Web/API/DOMException/message). Attempting to assign to it causes the unrelated `TypeError` to be thrown instead, which is what we see output.

To solve this specific case, we could modify the `$base64decode` function to catch the `DOMException` and throw a good, honest `Error` instead. But to solve the general case, we could consider finding all the places where thrown exceptions are being mutated, and altering the code to create a new `Error` and throw that instead. Something like:

```js
} catch (err) {
  throw Error(lookupMessage(err))
}
```

Unfortunately this approach trashes the stack trace... so, perhaps some thought is required... hmm...

Pull request:
Handle DOMExceptions
Fixes #261.

I fixed this by modifying (simplifying) `lookupMessage`.

* `lookupMessage` is now named `populateMessage`. Rather than return a string, it now populates `err.message` as a side-effect (or leaves it as is, if appropriate).
* Generally speaking, `populateMessage` now explicitly expects `err` to be either
  1. a plain old JavaScript object with an error `code`, thrown by `jsonata` itself, whose `message` should be populated using the appropriate template, or
  2. an actual `Error` object or similar (such as a `DOMException`) with no `code`, but with an existing `message` property which should not be touched.

Note that logic which automatically reified an error message of `"Unknown error"` was untested and therefore apparently unused, and so has been removed. The only way to hit this case was for a user to deliberately define a function which threw a non-`Error`, which I think is a rare case and not worth adding a test for.

Patch:
diff --git a/src/jsonata.js b/src/jsonata.js
index 4b7e2ff..c79df1c 100644
--- a/src/jsonata.js
+++ b/src/jsonata.js
@@ -4938,28 +4938,27 @@ var jsonata = (function() {
     };
 
     /**
-     * lookup a message template from the catalog and substitute the inserts
+     * lookup a message template from the catalog and substitute the inserts.
+     * Populates `err.message` with the substituted message. Leaves `err.message`
+     * untouched if code lookup fails.
      * @param {string} err - error code to lookup
-     * @returns {string} message
+     * @returns {undefined} - `err` is modified in place
      */
-    function lookupMessage(err) {
-        var message = 'Unknown error';
-        if(typeof err.message !== 'undefined') {
-            message = err.message;
-        }
+    function populateMessage(err) {
         var template = errorCodes[err.code];
         if(typeof template !== 'undefined') {
             // if there are any handlebars, replace them with the field references
             // triple braces - replace with value
             // double braces - replace with json stringified value
-            message = template.replace(/\{\{\{([^}]+)}}}/g, function() {
+            var message = template.replace(/\{\{\{([^}]+)}}}/g, function() {
                 return err[arguments[1]];
             });
             message = message.replace(/\{\{([^}]+)}}/g, function() {
                 return JSON.stringify(err[arguments[1]]);
             });
+            err.message = message;
         }
-        return message;
+        // Otherwise retain the original `err.message`
     }
 
     /**
@@ -4977,7 +4976,7 @@ var jsonata = (function() {
             delete ast.errors;
         } catch(err) {
             // insert error message into structure
-            err.message = lookupMessage(err);
+            populateMessage(err); // possible side-effects on `err`
             throw err;
         }
         var environment = createFrame(staticFrame);
@@ -4998,7 +4997,7 @@ var jsonata = (function() {
                         code: 'S0500',
                         position: 0
                     };
-                    err.message = lookupMessage(err);
+                    populateMessage(err); // possible side-effects on `err`
                     throw err;
                 }
 
@@ -5024,7 +5023,7 @@ var jsonata = (function() {
                 if(typeof callback === 'function') {
                     exec_env.bind('__jsonata_async', true);
                     var catchHandler = function (err) {
-                        err.message = lookupMessage(err);
+                        populateMessage(err); // possible side-effects on `err`
                         callback(err, null);
                     };
                     var thenHandler = function (response) {
@@ -5049,7 +5048,7 @@ var jsonata = (function() {
                         return result.value;
                     } catch (err) {
                         // insert error message into structure
-                        err.message = lookupMessage(err);
+                        populateMessage(err); // possible side-effects on `err`
                         throw err;
                     }
                 }


Test patch:
diff --git a/test/implementation-tests.js b/test/implementation-tests.js
index 90e3835..b75ba76 100644
--- a/test/implementation-tests.js
+++ b/test/implementation-tests.js
@@ -296,6 +296,43 @@ describe("Tests that bind Javascript functions", () => {
         });
     });
 
+    // Issue #261. Previously we would attempt to assign to the read-only `message` property,
+    // causing an unrelated `TypeError` to be thrown instead
+    describe("function throws a `DOMException` with a read-only `message` property", function() {
+        /**
+         * `DOMException` is not available in our testing environment. Additionally, we can't
+         * just import the `domexception` module since it doesn't work on Node.js v4, which
+         * we still support. So, here's a fake skeleton implementation which has the relevant
+         * qualities we need to reproduce the bug, most importantly a read-only `message`
+         * property
+         * @param {string} message - Error message
+         * @constructor
+         */
+        function DOMException (message) {
+            Object.defineProperty(this, "message", {
+                get() {
+                    return message;
+                },
+                enumerable: true,
+                configurable: true
+            });
+        }
+
+        Object.setPrototypeOf(DOMException.prototype, Error.prototype);
+
+        it("rethrows correctly", function() {
+            var expr = jsonata("$throwDomEx()");
+            expr.registerFunction("throwDomEx", function() {
+                throw new DOMException('Here is my message');
+            });
+            expect(function () {
+                expr.evaluate({});
+            })
+                .to.throw(DOMException)
+                .to.deep.contain({ message: 'Here is my message', position: 12, token: 'throwDomEx' });
+        });
+    });
+
     describe("map a user-defined Javascript function with signature", function() {
         it("should return result object", function() {
             var expr = jsonata("$map([1,4,9,16], $squareroot)");