From b62b8794911e60664f38c42c760cfdc25009876c Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 20 Jul 2017 11:56:02 -0400 Subject: [PATCH 1/7] Remove try/catch for short url so the appropriate errors will be propagated to the UI --- src/server/http/short_url_lookup.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/server/http/short_url_lookup.js b/src/server/http/short_url_lookup.js index 60d7cd5b1cc69c..5514bd4023b188 100644 --- a/src/server/http/short_url_lookup.js +++ b/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; } }; } From 2295921eddb41935ba5dd031077f8d7930022aed Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 20 Jul 2017 13:18:31 -0400 Subject: [PATCH 2/7] Simply ensure the error is a Boom error by wrapping it, but keep the original error details. --- src/server/http/short_url_error.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/server/http/short_url_error.js b/src/server/http/short_url_error.js index 8c617a5fbf5fd2..c37a359861b7f8 100644 --- a/src/server/http/short_url_error.js +++ b/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.status || 500); } From d5f622fe65dacb1a78f2db7224d5dd55f0398270 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Mon, 24 Jul 2017 10:56:35 -0400 Subject: [PATCH 3/7] Boom.wrap can't handle non Error instances, as exist in some of the tests. --- src/server/http/short_url_error.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/server/http/short_url_error.js b/src/server/http/short_url_error.js index c37a359861b7f8..dd66d3db2ac8a4 100644 --- a/src/server/http/short_url_error.js +++ b/src/server/http/short_url_error.js @@ -1,5 +1,8 @@ import Boom from 'boom'; export function handleShortUrlError(error) { + if (!error.isBoom && !(error instanceof Error)) { + return Boom.create(error.status || 500); + } return Boom.wrap(error, error.status || 500); } From 109a67f9442790cbadb767b4153b729181f1156a Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 25 Jul 2017 16:07:25 -0400 Subject: [PATCH 4/7] Only support Error objects, and check both status and statusCode --- src/server/http/__tests__/short_url_error.js | 35 +++++++++++++------- src/server/http/short_url_error.js | 5 +-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/server/http/__tests__/short_url_error.js b/src/server/http/__tests__/short_url_error.js index 2cb6e34b99fb3a..60626d671f4007 100644 --- a/src/server/http/__tests__/short_url_error.js +++ b/src/server/http/__tests__/short_url_error.js @@ -2,20 +2,31 @@ import expect from 'expect.js'; import _ from 'lodash'; import { handleShortUrlError } from '../short_url_error'; +function createErrorWithStatus(status) { + const error = new Error(); + error.status = status; +} + +function createErrorWithStatusCode(statusCode) { + const error = new Error(); + error.statusCode = statusCode; +} + describe('handleShortUrlError()', () => { - const caughtErrors = [{ - status: 401 - }, { - status: 403 - }, { - status: 404 - }]; + const caughtErrors = [ + createErrorWithStatus(401), + createErrorWithStatus(403), + createErrorWithStatus(404), + createErrorWithStatusCode(401), + createErrorWithStatusCode(403), + createErrorWithStatusCode(404), + ]; - const uncaughtErrors = [{ - status: null - }, { - status: 500 - }]; + const uncaughtErrors = [ + new Error(), + createErrorWithStatus(500), + createErrorWithStatusCode(500) + ]; caughtErrors.forEach((err) => { it(`should handle ${err.status} errors`, function () { diff --git a/src/server/http/short_url_error.js b/src/server/http/short_url_error.js index dd66d3db2ac8a4..a79eedbe2c0ecf 100644 --- a/src/server/http/short_url_error.js +++ b/src/server/http/short_url_error.js @@ -1,8 +1,5 @@ import Boom from 'boom'; export function handleShortUrlError(error) { - if (!error.isBoom && !(error instanceof Error)) { - return Boom.create(error.status || 500); - } - return Boom.wrap(error, error.status || 500); + return Boom.wrap(error, error.statusCode || error.status || 500); } From a7d36dcb6886d9453f56c526bd261498aee0cddd Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 25 Jul 2017 16:35:59 -0400 Subject: [PATCH 5/7] fix test --- src/server/http/__tests__/short_url_error.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/http/__tests__/short_url_error.js b/src/server/http/__tests__/short_url_error.js index 60626d671f4007..fe5b82dc158086 100644 --- a/src/server/http/__tests__/short_url_error.js +++ b/src/server/http/__tests__/short_url_error.js @@ -29,8 +29,8 @@ describe('handleShortUrlError()', () => { ]; caughtErrors.forEach((err) => { - it(`should handle ${err.status} errors`, function () { - expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status); + it(`should handle ${err.status || err.statusCode} errors`, function () { + expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status || err.statusCode); }); }); From b7870ab18eab257d9bfdbea60916330aa0d6c4aa Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Tue, 25 Jul 2017 17:29:07 -0400 Subject: [PATCH 6/7] Fix test errors for reals this time --- src/server/http/__tests__/short_url_error.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/server/http/__tests__/short_url_error.js b/src/server/http/__tests__/short_url_error.js index fe5b82dc158086..a4ee1fbd7df4b2 100644 --- a/src/server/http/__tests__/short_url_error.js +++ b/src/server/http/__tests__/short_url_error.js @@ -5,11 +5,13 @@ 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()', () => { From a2e864e1ffe550065c2ee0c7479e7e25abafa5d9 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 27 Jul 2017 10:17:30 -0400 Subject: [PATCH 7/7] Break out status and statusCode short url error tests --- src/server/http/__tests__/short_url_error.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/server/http/__tests__/short_url_error.js b/src/server/http/__tests__/short_url_error.js index a4ee1fbd7df4b2..e344107075e193 100644 --- a/src/server/http/__tests__/short_url_error.js +++ b/src/server/http/__tests__/short_url_error.js @@ -15,10 +15,13 @@ function createErrorWithStatusCode(statusCode) { } describe('handleShortUrlError()', () => { - const caughtErrors = [ + const caughtErrorsWithStatus = [ createErrorWithStatus(401), createErrorWithStatus(403), createErrorWithStatus(404), + ]; + + const caughtErrorsWithStatusCode = [ createErrorWithStatusCode(401), createErrorWithStatusCode(403), createErrorWithStatusCode(404), @@ -30,9 +33,15 @@ describe('handleShortUrlError()', () => { createErrorWithStatusCode(500) ]; - caughtErrors.forEach((err) => { - it(`should handle ${err.status || err.statusCode} errors`, function () { - expect(_.get(handleShortUrlError(err), 'output.statusCode')).to.be(err.status || err.statusCode); + 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); }); });