Remove try/catch for short url so the appropriate errors will be propagated to the UI (#13004)

* Remove try/catch for short url so the appropriate errors will be propagated to the UI

* Simply ensure the error is a Boom error by wrapping it, but keep the original error details.

* Boom.wrap can't handle non Error instances, as exist in some of the tests.

* Only support Error objects, and check both status and statusCode

* fix test

* Fix test errors for reals this time

* Break out status and statusCode short url error tests

src/server/http/__tests__/short_url_error.js

@@ -2,27 +2,49 @@ import expect from 'expect.js';
 import _ from 'lodash';
 import { handleShortUrlError } from '../short_url_error';
 
+function createErrorWithStatus(status) {
+  const error = new Error();
+  error.status = status;
+  return error;
+}
+
+function createErrorWithStatusCode(statusCode) {
+  const error = new Error();
+  error.statusCode = statusCode;
+  return error;
+}
+
 describe('handleShortUrlError()', () => {
-  const caughtErrors = [{
-    status: 401
-  }, {
-    status: 403
-  }, {
-    status: 404
-  }];
+  const caughtErrorsWithStatus = [
+    createErrorWithStatus(401),
+    createErrorWithStatus(403),
+    createErrorWithStatus(404),
+  ];
 
-  const uncaughtErrors = [{
-    status: null
-  }, {
-    status: 500
-  }];
+  const caughtErrorsWithStatusCode = [
+    createErrorWithStatusCode(401),
+    createErrorWithStatusCode(403),
+    createErrorWithStatusCode(404),
+  ];
 
-  caughtErrors.forEach((err) => {
-    it(`should handle ${err.status} errors`, function () {
+  const uncaughtErrors = [
+    new Error(),
+    createErrorWithStatus(500),
+    createErrorWithStatusCode(500)
+  ];
+
+  caughtErrorsWithStatus.forEach((err) => {
+    it(`should handle errors with status of ${err.status}`, function () {
       expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status);
     });
   });
 
+  caughtErrorsWithStatusCode.forEach((err) => {
+    it(`should handle errors with statusCode of ${err.statusCode}`, function () {
+      expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.statusCode);
+    });
+  });
+
   uncaughtErrors.forEach((err) => {
     it(`should not handle unknown errors`, function () {
       expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(500);

src/server/http/short_url_error.js

@@ -1,9 +1,5 @@
 import Boom from 'boom';
 
-export function handleShortUrlError(err) {
-  if (err.isBoom) return err;
-  if (err.status === 401) return Boom.unauthorized();
-  if (err.status === 403) return Boom.forbidden();
-  if (err.status === 404) return Boom.notFound();
-  return Boom.badImplementation();
+export function handleShortUrlError(error) {
+  return Boom.wrap(error, error.statusCode || error.status || 500);
 }

src/server/http/short_url_lookup.js

@@ -37,14 +37,10 @@ export default function (server) {
     },
 
     async getUrl(id, req) {
-      try {
-        const doc = await req.getSavedObjectsClient().get('url', id);
-        updateMetadata(doc, req);
+      const doc = await req.getSavedObjectsClient().get('url', id);
+      updateMetadata(doc, req);
 
-        return doc.attributes.url;
-      } catch (err) {
-        return '/';
-      }
+      return doc.attributes.url;
     }
   };
 }