From 8871956e8bb1e7fefe6284a79e840dced50b343c Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 17:46:37 -0400 Subject: [PATCH 01/20] [SavedObjectClient] emit detectable errors --- package.json | 2 +- .../kibana/server/lib/handle_es_error.js | 2 +- .../client/lib/__tests__/decorate_es_error.js | 89 +++++ .../client/lib/__tests__/error_types.js | 304 ++++++++++++++++++ .../client/lib/decorate_es_error.js | 62 ++++ .../client/lib/error_type_helpers.js | 17 + .../saved_objects/client/lib/error_types.js | 81 +++++ .../client/lib/handle_es_error.js | 50 --- src/server/saved_objects/client/lib/index.js | 3 +- .../client/saved_objects_client.js | 10 +- 10 files changed, 564 insertions(+), 56 deletions(-) create mode 100644 src/server/saved_objects/client/lib/__tests__/decorate_es_error.js create mode 100644 src/server/saved_objects/client/lib/__tests__/error_types.js create mode 100644 src/server/saved_objects/client/lib/decorate_es_error.js create mode 100644 src/server/saved_objects/client/lib/error_type_helpers.js create mode 100644 src/server/saved_objects/client/lib/error_types.js delete mode 100644 src/server/saved_objects/client/lib/handle_es_error.js diff --git a/package.json b/package.json index 0df6485f28c4f..229468bfea7f9 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ "babel-register": "6.18.0", "bluebird": "2.9.34", "body-parser": "1.12.0", - "boom": "2.8.0", + "boom": "^5.2.0", "brace": "0.5.1", "bunyan": "1.7.1", "check-hash": "1.0.1", diff --git a/src/core_plugins/kibana/server/lib/handle_es_error.js b/src/core_plugins/kibana/server/lib/handle_es_error.js index d6670ba44442f..dfa2179a12a40 100644 --- a/src/core_plugins/kibana/server/lib/handle_es_error.js +++ b/src/core_plugins/kibana/server/lib/handle_es_error.js @@ -11,7 +11,7 @@ export default function handleESError(error) { error instanceof esErrors.ServiceUnavailable || error instanceof esErrors.NoConnections || error instanceof esErrors.RequestTimeout) { - return Boom.serverTimeout(error); + return Boom.serverUnavailable(error); } else if (error instanceof esErrors.Conflict || _.contains(error.message, 'index_template_already_exists')) { return Boom.conflict(error); } else if (error instanceof esErrors[403]) { diff --git a/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js b/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js new file mode 100644 index 0000000000000..77687c2f676b3 --- /dev/null +++ b/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js @@ -0,0 +1,89 @@ +import expect from 'expect.js'; +import { errors as esErrors } from 'elasticsearch'; + +import { decorateEsError } from '../decorate_es_error'; +import { + isEsUnavailableError, + isConflictError, + isNotAuthorizedError, + isForbiddenError, + isNotFoundError, + isBadRequestError, +} from '../error_types'; + +describe('savedObjectsClient/decorateEsError', () => { + it('always returns the same error it receives', () => { + const error = new Error(); + expect(decorateEsError(error)).to.be(error); + }); + + it('makes es.ConnectionFault a SavedObjectsClient/EsUnavailable error', () => { + const error = new esErrors.ConnectionFault(); + expect(isEsUnavailableError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isEsUnavailableError(error)).to.be(true); + }); + + it('makes es.ServiceUnavailable a SavedObjectsClient/EsUnavailable error', () => { + const error = new esErrors.ServiceUnavailable(); + expect(isEsUnavailableError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isEsUnavailableError(error)).to.be(true); + }); + + it('makes es.NoConnections a SavedObjectsClient/EsUnavailable error', () => { + const error = new esErrors.NoConnections(); + expect(isEsUnavailableError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isEsUnavailableError(error)).to.be(true); + }); + + it('makes es.RequestTimeout a SavedObjectsClient/EsUnavailable error', () => { + const error = new esErrors.RequestTimeout(); + expect(isEsUnavailableError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isEsUnavailableError(error)).to.be(true); + }); + + it('makes es.Conflict a SavedObjectsClient/Conflict error', () => { + const error = new esErrors.Conflict(); + expect(isConflictError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isConflictError(error)).to.be(true); + }); + + it('makes es.AuthenticationException a SavedObjectsClient/NotAuthorized error', () => { + const error = new esErrors.AuthenticationException(); + expect(isNotAuthorizedError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isNotAuthorizedError(error)).to.be(true); + }); + + it('makes es.Forbidden a SavedObjectsClient/Forbidden error', () => { + const error = new esErrors.Forbidden(); + expect(isForbiddenError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isForbiddenError(error)).to.be(true); + }); + + it('makes es.NotFound a SavedObjectsClient/NotFound error', () => { + const error = new esErrors.NotFound(); + expect(isNotFoundError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isNotFoundError(error)).to.be(true); + }); + + it('makes es.BadRequest a SavedObjectsClient/BadRequest error', () => { + const error = new esErrors.BadRequest(); + expect(isBadRequestError(error)).to.be(false); + expect(decorateEsError(error)).to.be(error); + expect(isBadRequestError(error)).to.be(true); + }); + + it('returns other errors as Boom errors', () => { + const error = new Error(); + expect(error).to.not.have.property('isBoom'); + expect(decorateEsError(error)).to.be(error); + expect(error).to.have.property('isBoom'); + }); +}); diff --git a/src/server/saved_objects/client/lib/__tests__/error_types.js b/src/server/saved_objects/client/lib/__tests__/error_types.js new file mode 100644 index 0000000000000..a0c0d73813e80 --- /dev/null +++ b/src/server/saved_objects/client/lib/__tests__/error_types.js @@ -0,0 +1,304 @@ +import expect from 'expect.js'; +import Boom from 'boom'; + +import { + wrapBadRequestError, + isBadRequestError, + wrapNotAuthorizedError, + isNotAuthorizedError, + wrapForbiddenError, + isForbiddenError, + wrapNotFoundError, + isNotFoundError, + wrapConflictError, + isConflictError, + wrapEsUnavailableError, + isEsUnavailableError, + wrapGeneralError, +} from '../error_types'; + +describe('savedObjectsClient/errorTypes', () => { + describe('BadRequest error', () => { + describe('wrapBadRequestError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapBadRequestError(error)).to.be(error); + }); + + it('makes the error identifiable as a BadRequest error', () => { + const error = new Error(); + expect(isBadRequestError(error)).to.be(false); + wrapBadRequestError(error); + expect(isBadRequestError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapBadRequestError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(400); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapBadRequestError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapBadRequestError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapBadRequestError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 400', () => { + const error = wrapBadRequestError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 400); + }); + }); + }); + }); + describe('NotAuthorized error', () => { + describe('wrapNotAuthorizedError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapNotAuthorizedError(error)).to.be(error); + }); + + it('makes the error identifiable as a NotAuthorized error', () => { + const error = new Error(); + expect(isNotAuthorizedError(error)).to.be(false); + wrapNotAuthorizedError(error); + expect(isNotAuthorizedError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapNotAuthorizedError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(401); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapNotAuthorizedError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapNotAuthorizedError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapNotAuthorizedError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 401', () => { + const error = wrapNotAuthorizedError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 401); + }); + }); + }); + }); + describe('Forbidden error', () => { + describe('wrapForbiddenError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapForbiddenError(error)).to.be(error); + }); + + it('makes the error identifiable as a Forbidden error', () => { + const error = new Error(); + expect(isForbiddenError(error)).to.be(false); + wrapForbiddenError(error); + expect(isForbiddenError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapForbiddenError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(403); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapForbiddenError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapForbiddenError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapForbiddenError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 403', () => { + const error = wrapForbiddenError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 403); + }); + }); + }); + }); + describe('NotFound error', () => { + describe('wrapNotFoundError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapNotFoundError(error)).to.be(error); + }); + + it('makes the error identifiable as a NotFound error', () => { + const error = new Error(); + expect(isNotFoundError(error)).to.be(false); + wrapNotFoundError(error); + expect(isNotFoundError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapNotFoundError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(404); + }); + + it('preserves boom properties of input', () => { + const error = Boom.forbidden(); + wrapNotFoundError(error); + expect(error.output.statusCode).to.be(403); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapNotFoundError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapNotFoundError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 404', () => { + const error = wrapNotFoundError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 404); + }); + }); + }); + }); + describe('Conflict error', () => { + describe('wrapConflictError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapConflictError(error)).to.be(error); + }); + + it('makes the error identifiable as a Conflict error', () => { + const error = new Error(); + expect(isConflictError(error)).to.be(false); + wrapConflictError(error); + expect(isConflictError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapConflictError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(409); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapConflictError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapConflictError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapConflictError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 409', () => { + const error = wrapConflictError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 409); + }); + }); + }); + }); + describe('EsUnavailable error', () => { + describe('wrapEsUnavailableError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapEsUnavailableError(error)).to.be(error); + }); + + it('makes the error identifiable as a EsUnavailable error', () => { + const error = new Error(); + expect(isEsUnavailableError(error)).to.be(false); + wrapEsUnavailableError(error); + expect(isEsUnavailableError(error)).to.be(true); + }); + + it('adds boom properties', () => { + const error = wrapEsUnavailableError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(503); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapEsUnavailableError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('defaults to message of erorr', () => { + const error = wrapEsUnavailableError(new Error('foobar')); + expect(error.output.payload).to.have.property('message', 'foobar'); + }); + it('prefixes message with passed reason', () => { + const error = wrapEsUnavailableError(new Error('foobar'), 'biz'); + expect(error.output.payload).to.have.property('message', 'biz: foobar'); + }); + it('sets statusCode to 503', () => { + const error = wrapEsUnavailableError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 503); + }); + }); + }); + }); + describe('General error', () => { + describe('wrapGeneralError', () => { + it('returns original object', () => { + const error = new Error(); + expect(wrapGeneralError(error)).to.be(error); + }); + + it('adds boom properties', () => { + const error = wrapGeneralError(new Error()); + expect(error.output).to.be.an('object'); + expect(error.output.statusCode).to.be(500); + }); + + it('preserves boom properties of input', () => { + const error = Boom.notFound(); + wrapGeneralError(error); + expect(error.output.statusCode).to.be(404); + }); + + describe('error.output', () => { + it('ignores error message', () => { + const error = wrapGeneralError(new Error('foobar')); + expect(error.output.payload).to.have.property('message').match(/internal server error/i); + }); + it('sets statusCode to 500', () => { + const error = wrapGeneralError(new Error('foo')); + expect(error.output).to.have.property('statusCode', 500); + }); + }); + }); + }); +}); diff --git a/src/server/saved_objects/client/lib/decorate_es_error.js b/src/server/saved_objects/client/lib/decorate_es_error.js new file mode 100644 index 0000000000000..518c8badb55c9 --- /dev/null +++ b/src/server/saved_objects/client/lib/decorate_es_error.js @@ -0,0 +1,62 @@ +import elasticsearch from 'elasticsearch'; +import { get } from 'lodash'; + +const { + ConnectionFault, + ServiceUnavailable, + NoConnections, + RequestTimeout, + Conflict, + 401: NotAuthorized, + 403: Forbidden, + NotFound, + BadRequest +} = elasticsearch.errors; + +import { + wrapBadRequestError, + wrapNotAuthorizedError, + wrapForbiddenError, + wrapNotFoundError, + wrapConflictError, + wrapEsUnavailableError, + wrapGeneralError, +} from './error_types'; + +export function decorateEsError(error) { + if (!(error instanceof Error)) { + throw new Error('Expected an instance of Error'); + } + + const { reason } = get(error, 'body.error', {}); + if ( + error instanceof ConnectionFault || + error instanceof ServiceUnavailable || + error instanceof NoConnections || + error instanceof RequestTimeout + ) { + return wrapEsUnavailableError(error, reason); + } + + if (error instanceof Conflict) { + return wrapConflictError(error, reason); + } + + if (error instanceof NotAuthorized) { + return wrapNotAuthorizedError(error, reason); + } + + if (error instanceof Forbidden) { + return wrapForbiddenError(error, reason); + } + + if (error instanceof NotFound) { + return wrapNotFoundError(error, reason); + } + + if (error instanceof BadRequest) { + return wrapBadRequestError(error, reason); + } + + return wrapGeneralError(error, reason); +} diff --git a/src/server/saved_objects/client/lib/error_type_helpers.js b/src/server/saved_objects/client/lib/error_type_helpers.js new file mode 100644 index 0000000000000..cd69d90557046 --- /dev/null +++ b/src/server/saved_objects/client/lib/error_type_helpers.js @@ -0,0 +1,17 @@ +import { + isBadRequestError, + isNotAuthorizedError, + isForbiddenError, + isNotFoundError, + isConflictError, + isEsUnavailableError +} from './error_types'; + +export const errorTypeHelpers = { + isBadRequestError, + isNotAuthorizedError, + isForbiddenError, + isNotFoundError, + isConflictError, + isEsUnavailableError, +}; diff --git a/src/server/saved_objects/client/lib/error_types.js b/src/server/saved_objects/client/lib/error_types.js new file mode 100644 index 0000000000000..3ce494073fb99 --- /dev/null +++ b/src/server/saved_objects/client/lib/error_types.js @@ -0,0 +1,81 @@ +import Boom from 'boom'; + +const code = Symbol('SavedObjectsClientErrorCode'); + +function wrap(error, errorCode, statusCode, message) { + const boom = Boom.boomify(error, { + statusCode, + message, + override: false, + }); + + boom[code] = errorCode; + + return boom; +} + +// 400 - badRequest +const CODE_BAD_REQUEST = 'SavedObjectsClient/badRequest'; +export function wrapBadRequestError(error, reason) { + return wrap(error, CODE_BAD_REQUEST, 400, reason); +} +export function isBadRequestError(error) { + return error && error[code] === CODE_BAD_REQUEST; +} + + +// 401 - Not Authorized +const CODE_NOT_AUTHORIZED = 'SavedObjectsClient/notAuthorized'; +export function wrapNotAuthorizedError(error, reason) { + return wrap(error, CODE_NOT_AUTHORIZED, 401, reason); +} +export function isNotAuthorizedError(error) { + return error && error[code] === CODE_NOT_AUTHORIZED; +} + + +// 403 - Forbidden +const CODE_FORBIDDEN = 'SavedObjectsClient/forbidden'; +export function wrapForbiddenError(error, reason) { + return wrap(error, CODE_FORBIDDEN, 403, reason); +} +export function isForbiddenError(error) { + return error && error[code] === CODE_FORBIDDEN; +} + + +// 404 - Not Found +const CODE_NOT_FOUND = 'SavedObjectsClient/notFound'; +export function wrapNotFoundError(error, reason) { + return wrap(error, CODE_NOT_FOUND, 404, reason); +} +export function isNotFoundError(error) { + return error && error[code] === CODE_NOT_FOUND; +} + + +// 409 - Conflict +const CODE_CONFLICT = 'SavedObjectsClient/conflict'; +export function wrapConflictError(error, reason) { + return wrap(error, CODE_CONFLICT, 409, reason); +} +export function isConflictError(error) { + return error && error[code] === CODE_CONFLICT; +} + + +// 500 - Es Unavailable +const CODE_ES_UNAVAILABLE = 'SavedObjectsClient/esUnavailable'; +export function wrapEsUnavailableError(error, reason) { + return wrap(error, CODE_ES_UNAVAILABLE, 503, reason); +} +export function isEsUnavailableError(error) { + return error && error[code] === CODE_ES_UNAVAILABLE; +} + + +// 500 - General Error +const CODE_GENERAL_ERROR = 'SavedObjectsClient/generalError'; +export function wrapGeneralError(error, reason) { + return wrap(error, CODE_GENERAL_ERROR, 500, reason); +} diff --git a/src/server/saved_objects/client/lib/handle_es_error.js b/src/server/saved_objects/client/lib/handle_es_error.js deleted file mode 100644 index 87bdcf44841b0..0000000000000 --- a/src/server/saved_objects/client/lib/handle_es_error.js +++ /dev/null @@ -1,50 +0,0 @@ -import elasticsearch from 'elasticsearch'; -import Boom from 'boom'; -import { get } from 'lodash'; - -const { - ConnectionFault, - ServiceUnavailable, - NoConnections, - RequestTimeout, - Conflict, - 403: Forbidden, - NotFound, - BadRequest -} = elasticsearch.errors; - -export function handleEsError(error) { - if (!(error instanceof Error)) { - throw new Error('Expected an instance of Error'); - } - - const { reason, type } = get(error, 'body.error', {}); - const details = { type }; - - if ( - error instanceof ConnectionFault || - error instanceof ServiceUnavailable || - error instanceof NoConnections || - error instanceof RequestTimeout - ) { - throw Boom.serverTimeout(); - } - - if (error instanceof Conflict) { - throw Boom.conflict(reason, details); - } - - if (error instanceof Forbidden) { - throw Boom.forbidden(reason, details); - } - - if (error instanceof NotFound) { - throw Boom.notFound(reason, details); - } - - if (error instanceof BadRequest) { - throw Boom.badRequest(reason, details); - } - - throw error; -} diff --git a/src/server/saved_objects/client/lib/index.js b/src/server/saved_objects/client/lib/index.js index 3410337dfd35b..c44e7936d992d 100644 --- a/src/server/saved_objects/client/lib/index.js +++ b/src/server/saved_objects/client/lib/index.js @@ -1,4 +1,5 @@ export { getSearchDsl } from './search_dsl'; -export { handleEsError } from './handle_es_error'; export { trimIdPrefix } from './trim_id_prefix'; export { includedFields } from './included_fields'; +export { decorateEsError } from './decorate_es_error'; +export { errorTypeHelpers } from './error_type_helpers'; diff --git a/src/server/saved_objects/client/saved_objects_client.js b/src/server/saved_objects/client/saved_objects_client.js index 09d41a7cea47a..9d1b785a516ba 100644 --- a/src/server/saved_objects/client/saved_objects_client.js +++ b/src/server/saved_objects/client/saved_objects_client.js @@ -5,12 +5,16 @@ import { getRootType } from '../../mappings'; import { getSearchDsl, - handleEsError, trimIdPrefix, - includedFields + includedFields, + decorateEsError, + errorTypeHelpers, } from './lib'; export class SavedObjectsClient { + static errors = errorTypeHelpers + errors = errorTypeHelpers + constructor(kibanaIndex, mappings, callAdminCluster) { this._kibanaIndex = kibanaIndex; this._mappings = mappings; @@ -316,7 +320,7 @@ export class SavedObjectsClient { index: this._kibanaIndex, }); } catch (err) { - throw handleEsError(err); + throw decorateEsError(err); } } From a5892fcbde51c37741d88df54c7596fdab4e3a43 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 17:51:07 -0400 Subject: [PATCH 02/20] [uiSettingsService] consume new SavedObjectsClient errors --- package.json | 2 +- src/ui/ui_settings/ui_settings_service.js | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 229468bfea7f9..a724a404a3e8b 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ "babel-register": "6.18.0", "bluebird": "2.9.34", "body-parser": "1.12.0", - "boom": "^5.2.0", + "boom": "5.2.0", "brace": "0.5.1", "bunyan": "1.7.1", "check-hash": "1.0.1", diff --git a/src/ui/ui_settings/ui_settings_service.js b/src/ui/ui_settings/ui_settings_service.js index d966d4a0e9683..b694d5352bd48 100644 --- a/src/ui/ui_settings/ui_settings_service.js +++ b/src/ui/ui_settings/ui_settings_service.js @@ -1,5 +1,4 @@ import { defaultsDeep, noop } from 'lodash'; -import { errors as esErrors } from 'elasticsearch'; function hydrateUserSettings(userSettings) { return Object.keys(userSettings) @@ -105,16 +104,17 @@ export class UiSettingsService { const { ignore401Errors = false } = options; + const savedObjectsClient = this._savedObjectsClient; const isIgnorableError = error => ( - error instanceof esErrors[404] || - error instanceof esErrors[403] || - error instanceof esErrors.NoConnections || - (ignore401Errors && error instanceof esErrors[401]) + savedObjectsClient.errors.isNotFound(error) || + savedObjectsClient.errors.isForbidden(error) || + savedObjectsClient.errors.isEsUnavailable(error) || + (ignore401Errors && savedObjectsClient.errors.isNotAuthorized(error)) ); try { - const resp = await this._savedObjectsClient.get(this._type, this._id); + const resp = await savedObjectsClient.get(this._type, this._id); return resp.attributes; } catch (error) { if (isIgnorableError(error)) { From e4a7f8102cf12707e95678007fa1595f73ef9506 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 20:35:27 -0400 Subject: [PATCH 03/20] [SavedObjectsClient] expose errorTypeHelpers as such --- .../client/saved_objects_client.js | 5 ++--- src/ui/ui_settings/ui_settings_service.js | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/server/saved_objects/client/saved_objects_client.js b/src/server/saved_objects/client/saved_objects_client.js index 9d1b785a516ba..39cd93227e22f 100644 --- a/src/server/saved_objects/client/saved_objects_client.js +++ b/src/server/saved_objects/client/saved_objects_client.js @@ -12,9 +12,6 @@ import { } from './lib'; export class SavedObjectsClient { - static errors = errorTypeHelpers - errors = errorTypeHelpers - constructor(kibanaIndex, mappings, callAdminCluster) { this._kibanaIndex = kibanaIndex; this._mappings = mappings; @@ -22,6 +19,8 @@ export class SavedObjectsClient { this._callAdminCluster = callAdminCluster; } + errorTypeHelpers = errorTypeHelpers + /** * Persists an object * diff --git a/src/ui/ui_settings/ui_settings_service.js b/src/ui/ui_settings/ui_settings_service.js index b694d5352bd48..349add006f389 100644 --- a/src/ui/ui_settings/ui_settings_service.js +++ b/src/ui/ui_settings/ui_settings_service.js @@ -104,17 +104,23 @@ export class UiSettingsService { const { ignore401Errors = false } = options; - const savedObjectsClient = this._savedObjectsClient; + + const { + isNotFoundError, + isForbiddenError, + isEsUnavailableError, + isNotAuthorizedError + } = this._savedObjectsClient.errorTypeHelpers; const isIgnorableError = error => ( - savedObjectsClient.errors.isNotFound(error) || - savedObjectsClient.errors.isForbidden(error) || - savedObjectsClient.errors.isEsUnavailable(error) || - (ignore401Errors && savedObjectsClient.errors.isNotAuthorized(error)) + isNotFoundError(error) || + isForbiddenError(error) || + isEsUnavailableError(error) || + (ignore401Errors && isNotAuthorizedError(error)) ); try { - const resp = await savedObjectsClient.get(this._type, this._id); + const resp = await this._savedObjectsClient.get(this._type, this._id); return resp.attributes; } catch (error) { if (isIgnorableError(error)) { From b299ec59b9cd5b2e84b2433261c9109475d0c7aa Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 23:22:54 -0400 Subject: [PATCH 04/20] [elasticsearch/tests] recreate error for each test --- src/core_plugins/elasticsearch/lib/__tests__/cluster.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core_plugins/elasticsearch/lib/__tests__/cluster.js b/src/core_plugins/elasticsearch/lib/__tests__/cluster.js index e0a729c70b65e..e4afa75d7477c 100644 --- a/src/core_plugins/elasticsearch/lib/__tests__/cluster.js +++ b/src/core_plugins/elasticsearch/lib/__tests__/cluster.js @@ -104,10 +104,12 @@ describe('plugins/elasticsearch', function () { describe('wrap401Errors', () => { let handler; - const error = new Error('Authentication required'); - error.statusCode = 401; + let error; beforeEach(() => { + error = new Error('Authentication required'); + error.statusCode = 401; + handler = sinon.stub(); }); From fce591613fa5eead908048c777024c80a051ebd8 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 23:25:10 -0400 Subject: [PATCH 05/20] [http] wait for elasticsearch plugin to be ready --- src/server/http/__tests__/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/server/http/__tests__/index.js b/src/server/http/__tests__/index.js index 89914c9ff60ed..90e20f40d13b6 100644 --- a/src/server/http/__tests__/index.js +++ b/src/server/http/__tests__/index.js @@ -6,9 +6,10 @@ describe('routes', function () { this.timeout(60000); let kbnServer; - beforeEach(function () { + beforeEach(async function () { kbnServer = kbnTestServer.createServerWithCorePlugins(); - return kbnServer.ready(); + await kbnServer.ready(); + await kbnServer.server.plugins.elasticsearch.waitUntilReady(); }); afterEach(function () { return kbnServer.close(); From cbdbe2ac0047cb1d97bf6a2c8dac4c3503810334 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 23:26:04 -0400 Subject: [PATCH 06/20] [shortUrl/tests] ensure that create request responds with 200 --- src/server/http/__tests__/index.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/server/http/__tests__/index.js b/src/server/http/__tests__/index.js index 90e20f40d13b6..7790771fb3350 100644 --- a/src/server/http/__tests__/index.js +++ b/src/server/http/__tests__/index.js @@ -64,6 +64,8 @@ describe('routes', function () { it('redirects shortened urls', (done) => { kbnTestServer.makeRequest(kbnServer, shortenOptions, (res) => { + expect(res).to.have.property('statusCode', 200); + const gotoOptions = { method: 'GET', url: '/goto/' + res.payload From e1e835affb3a08c85298552afe131e7dcd49d0a5 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 23:27:14 -0400 Subject: [PATCH 07/20] [shortUrl] use errorTypeHelpers to filter errors --- src/server/http/short_url_lookup.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/server/http/short_url_lookup.js b/src/server/http/short_url_lookup.js index 60d7cd5b1cc69..7a172085a0837 100644 --- a/src/server/http/short_url_lookup.js +++ b/src/server/http/short_url_lookup.js @@ -17,9 +17,11 @@ export default function (server) { return { async generateUrlId(url, req) { const id = crypto.createHash('md5').update(url).digest('hex'); + const savedObjectsClient = req.getSavedObjectsClient(); + const { isConflictError } = savedObjectsClient.errorTypeHelpers; try { - const doc = await req.getSavedObjectsClient().create('url', { + const doc = await savedObjectsClient.create('url', { url, accessCount: 0, createDate: new Date(), @@ -27,12 +29,12 @@ export default function (server) { }, { id }); return doc.id; - } catch(e) { - if (get(e, 'data.type') === 'version_conflict_engine_exception') { + } catch (error) { + if (isConflictError(error)) { return id; } - throw e; + throw error; } }, From f378cd12383d8b591e6066d926a1b64bb0851a36 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 24 Jul 2017 23:27:55 -0400 Subject: [PATCH 08/20] [uiSettings/savedObjectsClientStub] stub errorTypeHelpers --- .../__tests__/lib/create_objects_client_stub.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js index 5b6c38b8a5cf5..3a82c240aecc8 100644 --- a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js +++ b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js @@ -4,7 +4,13 @@ import expect from 'expect.js'; export function createObjectsClientStub(type, id, esDocSource = {}) { const savedObjectsClient = { update: sinon.stub().returns(Promise.resolve()), - get: sinon.stub().returns({ attributes: esDocSource }) + get: sinon.stub().returns({ attributes: esDocSource }), + errorTypeHelpers: { + isNotFoundError: sinon.stub().returns(false), + isForbiddenError: sinon.stub().returns(false), + isEsUnavailableError: sinon.stub().returns(false), + isNotAuthorizedError: sinon.stub().returns(false), + } }; savedObjectsClient.assertGetQuery = () => { From 5431bdca65b2e31902824d90d1eca50271319b68 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 25 Jul 2017 00:42:55 -0400 Subject: [PATCH 09/20] [SavedObjectsClient/errors] expose error module so tests can make errors --- src/server/http/short_url_lookup.js | 2 +- .../client/lib/decorate_es_error.js | 2 +- .../client/lib/error_type_helpers.js | 17 ----------------- .../client/lib/{error_types.js => errors.js} | 0 src/server/saved_objects/client/lib/index.js | 4 +++- .../client/saved_objects_client.js | 5 +++-- .../__tests__/lib/create_objects_client_stub.js | 2 +- src/ui/ui_settings/ui_settings_service.js | 2 +- 8 files changed, 10 insertions(+), 24 deletions(-) delete mode 100644 src/server/saved_objects/client/lib/error_type_helpers.js rename src/server/saved_objects/client/lib/{error_types.js => errors.js} (100%) diff --git a/src/server/http/short_url_lookup.js b/src/server/http/short_url_lookup.js index 7a172085a0837..b79b7063afeba 100644 --- a/src/server/http/short_url_lookup.js +++ b/src/server/http/short_url_lookup.js @@ -18,7 +18,7 @@ export default function (server) { async generateUrlId(url, req) { const id = crypto.createHash('md5').update(url).digest('hex'); const savedObjectsClient = req.getSavedObjectsClient(); - const { isConflictError } = savedObjectsClient.errorTypeHelpers; + const { isConflictError } = savedObjectsClient.errors; try { const doc = await savedObjectsClient.create('url', { diff --git a/src/server/saved_objects/client/lib/decorate_es_error.js b/src/server/saved_objects/client/lib/decorate_es_error.js index 518c8badb55c9..f516ad6c2c60f 100644 --- a/src/server/saved_objects/client/lib/decorate_es_error.js +++ b/src/server/saved_objects/client/lib/decorate_es_error.js @@ -21,7 +21,7 @@ import { wrapConflictError, wrapEsUnavailableError, wrapGeneralError, -} from './error_types'; +} from './errors'; export function decorateEsError(error) { if (!(error instanceof Error)) { diff --git a/src/server/saved_objects/client/lib/error_type_helpers.js b/src/server/saved_objects/client/lib/error_type_helpers.js deleted file mode 100644 index cd69d90557046..0000000000000 --- a/src/server/saved_objects/client/lib/error_type_helpers.js +++ /dev/null @@ -1,17 +0,0 @@ -import { - isBadRequestError, - isNotAuthorizedError, - isForbiddenError, - isNotFoundError, - isConflictError, - isEsUnavailableError -} from './error_types'; - -export const errorTypeHelpers = { - isBadRequestError, - isNotAuthorizedError, - isForbiddenError, - isNotFoundError, - isConflictError, - isEsUnavailableError, -}; diff --git a/src/server/saved_objects/client/lib/error_types.js b/src/server/saved_objects/client/lib/errors.js similarity index 100% rename from src/server/saved_objects/client/lib/error_types.js rename to src/server/saved_objects/client/lib/errors.js diff --git a/src/server/saved_objects/client/lib/index.js b/src/server/saved_objects/client/lib/index.js index c44e7936d992d..3e90b4820f9c7 100644 --- a/src/server/saved_objects/client/lib/index.js +++ b/src/server/saved_objects/client/lib/index.js @@ -2,4 +2,6 @@ export { getSearchDsl } from './search_dsl'; export { trimIdPrefix } from './trim_id_prefix'; export { includedFields } from './included_fields'; export { decorateEsError } from './decorate_es_error'; -export { errorTypeHelpers } from './error_type_helpers'; + +import * as errors from './errors'; +export { errors }; diff --git a/src/server/saved_objects/client/saved_objects_client.js b/src/server/saved_objects/client/saved_objects_client.js index 39cd93227e22f..953480ec1f49c 100644 --- a/src/server/saved_objects/client/saved_objects_client.js +++ b/src/server/saved_objects/client/saved_objects_client.js @@ -8,7 +8,7 @@ import { trimIdPrefix, includedFields, decorateEsError, - errorTypeHelpers, + errors, } from './lib'; export class SavedObjectsClient { @@ -19,7 +19,8 @@ export class SavedObjectsClient { this._callAdminCluster = callAdminCluster; } - errorTypeHelpers = errorTypeHelpers + static errors = errors + errors = errors /** * Persists an object diff --git a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js index 3a82c240aecc8..340b84c40c861 100644 --- a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js +++ b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js @@ -5,7 +5,7 @@ export function createObjectsClientStub(type, id, esDocSource = {}) { const savedObjectsClient = { update: sinon.stub().returns(Promise.resolve()), get: sinon.stub().returns({ attributes: esDocSource }), - errorTypeHelpers: { + errors: { isNotFoundError: sinon.stub().returns(false), isForbiddenError: sinon.stub().returns(false), isEsUnavailableError: sinon.stub().returns(false), diff --git a/src/ui/ui_settings/ui_settings_service.js b/src/ui/ui_settings/ui_settings_service.js index 349add006f389..559134f90191a 100644 --- a/src/ui/ui_settings/ui_settings_service.js +++ b/src/ui/ui_settings/ui_settings_service.js @@ -110,7 +110,7 @@ export class UiSettingsService { isForbiddenError, isEsUnavailableError, isNotAuthorizedError - } = this._savedObjectsClient.errorTypeHelpers; + } = this._savedObjectsClient.errors; const isIgnorableError = error => ( isNotFoundError(error) || From 7c81e7a3504f323721eb11f631e9c731b338a9c5 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 25 Jul 2017 00:43:35 -0400 Subject: [PATCH 10/20] [shortUrl/tests] use actual SavedObjectsClient errors --- src/server/http/__tests__/short_url_lookup.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/http/__tests__/short_url_lookup.js b/src/server/http/__tests__/short_url_lookup.js index eac9dee0f43a8..94f8aa6ae7dca 100644 --- a/src/server/http/__tests__/short_url_lookup.js +++ b/src/server/http/__tests__/short_url_lookup.js @@ -1,6 +1,7 @@ import expect from 'expect.js'; import sinon from 'sinon'; import shortUrlLookupProvider from '../short_url_lookup'; +import { SavedObjectsClient } from '../../saved_objects/client'; describe('shortUrlLookupProvider', () => { const ID = 'bf00ad16941fc51420f91a93428b27a0'; @@ -17,7 +18,8 @@ describe('shortUrlLookupProvider', () => { savedObjectsClient = { get: sandbox.stub(), create: sandbox.stub().returns(Promise.resolve({ id: ID })), - update: sandbox.stub() + update: sandbox.stub(), + errors: SavedObjectsClient.errors }; req = { getSavedObjectsClient: () => savedObjectsClient }; @@ -58,10 +60,8 @@ describe('shortUrlLookupProvider', () => { }); it('gracefully handles version conflict', async () => { - const error = new Error('version conflict'); - error.data = { type: 'version_conflict_engine_exception' }; + const error = savedObjectsClient.errors.wrapConflictError(new Error()); savedObjectsClient.create.throws(error); - const id = await shortUrl.generateUrlId(URL, req); expect(id).to.eql(ID); }); From 304fd9e8ab1613c69e6e752d22df017cb257a46c Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 25 Jul 2017 00:45:12 -0400 Subject: [PATCH 11/20] [uiSettings/savedObjectsClientStub] use actual errors lib --- .../client/lib/__tests__/decorate_es_error.js | 2 +- .../__tests__/{error_types.js => errors.js} | 2 +- .../lib/create_objects_client_stub.js | 10 ++++---- src/ui/ui_settings/__tests__/lib/index.js | 5 +++- .../__tests__/ui_settings_service.js | 23 +++++++++++++------ 5 files changed, 26 insertions(+), 16 deletions(-) rename src/server/saved_objects/client/lib/__tests__/{error_types.js => errors.js} (99%) diff --git a/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js b/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js index 77687c2f676b3..4a6c09495c331 100644 --- a/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js +++ b/src/server/saved_objects/client/lib/__tests__/decorate_es_error.js @@ -9,7 +9,7 @@ import { isForbiddenError, isNotFoundError, isBadRequestError, -} from '../error_types'; +} from '../errors'; describe('savedObjectsClient/decorateEsError', () => { it('always returns the same error it receives', () => { diff --git a/src/server/saved_objects/client/lib/__tests__/error_types.js b/src/server/saved_objects/client/lib/__tests__/errors.js similarity index 99% rename from src/server/saved_objects/client/lib/__tests__/error_types.js rename to src/server/saved_objects/client/lib/__tests__/errors.js index a0c0d73813e80..40cbbb15ded31 100644 --- a/src/server/saved_objects/client/lib/__tests__/error_types.js +++ b/src/server/saved_objects/client/lib/__tests__/errors.js @@ -15,7 +15,7 @@ import { wrapEsUnavailableError, isEsUnavailableError, wrapGeneralError, -} from '../error_types'; +} from '../errors'; describe('savedObjectsClient/errorTypes', () => { describe('BadRequest error', () => { diff --git a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js index 340b84c40c861..63ff8b5363b64 100644 --- a/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js +++ b/src/ui/ui_settings/__tests__/lib/create_objects_client_stub.js @@ -1,16 +1,14 @@ import sinon from 'sinon'; import expect from 'expect.js'; +import { SavedObjectsClient } from '../../../../server/saved_objects/client'; + +export const savedObjectsClientErrors = SavedObjectsClient.errors; export function createObjectsClientStub(type, id, esDocSource = {}) { const savedObjectsClient = { update: sinon.stub().returns(Promise.resolve()), get: sinon.stub().returns({ attributes: esDocSource }), - errors: { - isNotFoundError: sinon.stub().returns(false), - isForbiddenError: sinon.stub().returns(false), - isEsUnavailableError: sinon.stub().returns(false), - isNotAuthorizedError: sinon.stub().returns(false), - } + errors: savedObjectsClientErrors }; savedObjectsClient.assertGetQuery = () => { diff --git a/src/ui/ui_settings/__tests__/lib/index.js b/src/ui/ui_settings/__tests__/lib/index.js index 14c9b5af0c610..0ca35ea49fb7d 100644 --- a/src/ui/ui_settings/__tests__/lib/index.js +++ b/src/ui/ui_settings/__tests__/lib/index.js @@ -1 +1,4 @@ -export { createObjectsClientStub } from './create_objects_client_stub'; +export { + createObjectsClientStub, + savedObjectsClientErrors, +} from './create_objects_client_stub'; diff --git a/src/ui/ui_settings/__tests__/ui_settings_service.js b/src/ui/ui_settings/__tests__/ui_settings_service.js index aad976cb98d81..c7ab164c5945e 100644 --- a/src/ui/ui_settings/__tests__/ui_settings_service.js +++ b/src/ui/ui_settings/__tests__/ui_settings_service.js @@ -5,7 +5,10 @@ import Chance from 'chance'; import { UiSettingsService } from '../ui_settings_service'; -import { createObjectsClientStub } from './lib'; +import { + createObjectsClientStub, + savedObjectsClientErrors, +} from './lib'; const TYPE = 'config'; const ID = 'kibana-version'; @@ -197,9 +200,12 @@ describe('ui settings', () => { it('throws 401 errors', async () => { const { uiSettings } = setup({ - savedObjectsClient: { async get() { - throw new esErrors[401](); - } } + savedObjectsClient: { + errors: savedObjectsClientErrors, + async get() { + throw new esErrors[401](); + } + } }); try { @@ -214,9 +220,12 @@ describe('ui settings', () => { const expectedUnexpectedError = new Error('unexpected'); const { uiSettings } = setup({ - savedObjectsClient: { async get() { - throw expectedUnexpectedError; - } } + savedObjectsClient: { + errors: savedObjectsClientErrors, + async get() { + throw expectedUnexpectedError; + } + } }); try { From 2e1fec204b1fdccf5cb7f4a8ea6d4213e357278f Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 25 Jul 2017 16:02:40 -0400 Subject: [PATCH 12/20] [SavedObjectsClient] use decorate instead of "wrap" --- src/server/http/__tests__/short_url_lookup.js | 2 +- .../client/lib/__tests__/errors.js | 122 +++++++++--------- .../client/lib/decorate_es_error.js | 28 ++-- src/server/saved_objects/client/lib/errors.js | 30 ++--- 4 files changed, 91 insertions(+), 91 deletions(-) diff --git a/src/server/http/__tests__/short_url_lookup.js b/src/server/http/__tests__/short_url_lookup.js index 94f8aa6ae7dca..fa9ecb0357a0a 100644 --- a/src/server/http/__tests__/short_url_lookup.js +++ b/src/server/http/__tests__/short_url_lookup.js @@ -60,7 +60,7 @@ describe('shortUrlLookupProvider', () => { }); it('gracefully handles version conflict', async () => { - const error = savedObjectsClient.errors.wrapConflictError(new Error()); + const error = savedObjectsClient.errors.decorateConflictError(new Error()); savedObjectsClient.create.throws(error); const id = await shortUrl.generateUrlId(URL, req); expect(id).to.eql(ID); diff --git a/src/server/saved_objects/client/lib/__tests__/errors.js b/src/server/saved_objects/client/lib/__tests__/errors.js index 40cbbb15ded31..f912d20944850 100644 --- a/src/server/saved_objects/client/lib/__tests__/errors.js +++ b/src/server/saved_objects/client/lib/__tests__/errors.js @@ -2,300 +2,300 @@ import expect from 'expect.js'; import Boom from 'boom'; import { - wrapBadRequestError, + decorateBadRequestError, isBadRequestError, - wrapNotAuthorizedError, + decorateNotAuthorizedError, isNotAuthorizedError, - wrapForbiddenError, + decorateForbiddenError, isForbiddenError, - wrapNotFoundError, + decorateNotFoundError, isNotFoundError, - wrapConflictError, + decorateConflictError, isConflictError, - wrapEsUnavailableError, + decorateEsUnavailableError, isEsUnavailableError, - wrapGeneralError, + decorateGeneralError, } from '../errors'; describe('savedObjectsClient/errorTypes', () => { describe('BadRequest error', () => { - describe('wrapBadRequestError', () => { + describe('decorateBadRequestError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapBadRequestError(error)).to.be(error); + expect(decorateBadRequestError(error)).to.be(error); }); it('makes the error identifiable as a BadRequest error', () => { const error = new Error(); expect(isBadRequestError(error)).to.be(false); - wrapBadRequestError(error); + decorateBadRequestError(error); expect(isBadRequestError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapBadRequestError(new Error()); + const error = decorateBadRequestError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(400); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapBadRequestError(error); + decorateBadRequestError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapBadRequestError(new Error('foobar')); + const error = decorateBadRequestError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapBadRequestError(new Error('foobar'), 'biz'); + const error = decorateBadRequestError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 400', () => { - const error = wrapBadRequestError(new Error('foo')); + const error = decorateBadRequestError(new Error('foo')); expect(error.output).to.have.property('statusCode', 400); }); }); }); }); describe('NotAuthorized error', () => { - describe('wrapNotAuthorizedError', () => { + describe('decorateNotAuthorizedError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapNotAuthorizedError(error)).to.be(error); + expect(decorateNotAuthorizedError(error)).to.be(error); }); it('makes the error identifiable as a NotAuthorized error', () => { const error = new Error(); expect(isNotAuthorizedError(error)).to.be(false); - wrapNotAuthorizedError(error); + decorateNotAuthorizedError(error); expect(isNotAuthorizedError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapNotAuthorizedError(new Error()); + const error = decorateNotAuthorizedError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(401); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapNotAuthorizedError(error); + decorateNotAuthorizedError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapNotAuthorizedError(new Error('foobar')); + const error = decorateNotAuthorizedError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapNotAuthorizedError(new Error('foobar'), 'biz'); + const error = decorateNotAuthorizedError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 401', () => { - const error = wrapNotAuthorizedError(new Error('foo')); + const error = decorateNotAuthorizedError(new Error('foo')); expect(error.output).to.have.property('statusCode', 401); }); }); }); }); describe('Forbidden error', () => { - describe('wrapForbiddenError', () => { + describe('decorateForbiddenError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapForbiddenError(error)).to.be(error); + expect(decorateForbiddenError(error)).to.be(error); }); it('makes the error identifiable as a Forbidden error', () => { const error = new Error(); expect(isForbiddenError(error)).to.be(false); - wrapForbiddenError(error); + decorateForbiddenError(error); expect(isForbiddenError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapForbiddenError(new Error()); + const error = decorateForbiddenError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(403); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapForbiddenError(error); + decorateForbiddenError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapForbiddenError(new Error('foobar')); + const error = decorateForbiddenError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapForbiddenError(new Error('foobar'), 'biz'); + const error = decorateForbiddenError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 403', () => { - const error = wrapForbiddenError(new Error('foo')); + const error = decorateForbiddenError(new Error('foo')); expect(error.output).to.have.property('statusCode', 403); }); }); }); }); describe('NotFound error', () => { - describe('wrapNotFoundError', () => { + describe('decorateNotFoundError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapNotFoundError(error)).to.be(error); + expect(decorateNotFoundError(error)).to.be(error); }); it('makes the error identifiable as a NotFound error', () => { const error = new Error(); expect(isNotFoundError(error)).to.be(false); - wrapNotFoundError(error); + decorateNotFoundError(error); expect(isNotFoundError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapNotFoundError(new Error()); + const error = decorateNotFoundError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(404); }); it('preserves boom properties of input', () => { const error = Boom.forbidden(); - wrapNotFoundError(error); + decorateNotFoundError(error); expect(error.output.statusCode).to.be(403); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapNotFoundError(new Error('foobar')); + const error = decorateNotFoundError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapNotFoundError(new Error('foobar'), 'biz'); + const error = decorateNotFoundError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 404', () => { - const error = wrapNotFoundError(new Error('foo')); + const error = decorateNotFoundError(new Error('foo')); expect(error.output).to.have.property('statusCode', 404); }); }); }); }); describe('Conflict error', () => { - describe('wrapConflictError', () => { + describe('decorateConflictError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapConflictError(error)).to.be(error); + expect(decorateConflictError(error)).to.be(error); }); it('makes the error identifiable as a Conflict error', () => { const error = new Error(); expect(isConflictError(error)).to.be(false); - wrapConflictError(error); + decorateConflictError(error); expect(isConflictError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapConflictError(new Error()); + const error = decorateConflictError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(409); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapConflictError(error); + decorateConflictError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapConflictError(new Error('foobar')); + const error = decorateConflictError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapConflictError(new Error('foobar'), 'biz'); + const error = decorateConflictError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 409', () => { - const error = wrapConflictError(new Error('foo')); + const error = decorateConflictError(new Error('foo')); expect(error.output).to.have.property('statusCode', 409); }); }); }); }); describe('EsUnavailable error', () => { - describe('wrapEsUnavailableError', () => { + describe('decorateEsUnavailableError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapEsUnavailableError(error)).to.be(error); + expect(decorateEsUnavailableError(error)).to.be(error); }); it('makes the error identifiable as a EsUnavailable error', () => { const error = new Error(); expect(isEsUnavailableError(error)).to.be(false); - wrapEsUnavailableError(error); + decorateEsUnavailableError(error); expect(isEsUnavailableError(error)).to.be(true); }); it('adds boom properties', () => { - const error = wrapEsUnavailableError(new Error()); + const error = decorateEsUnavailableError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(503); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapEsUnavailableError(error); + decorateEsUnavailableError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('defaults to message of erorr', () => { - const error = wrapEsUnavailableError(new Error('foobar')); + const error = decorateEsUnavailableError(new Error('foobar')); expect(error.output.payload).to.have.property('message', 'foobar'); }); it('prefixes message with passed reason', () => { - const error = wrapEsUnavailableError(new Error('foobar'), 'biz'); + const error = decorateEsUnavailableError(new Error('foobar'), 'biz'); expect(error.output.payload).to.have.property('message', 'biz: foobar'); }); it('sets statusCode to 503', () => { - const error = wrapEsUnavailableError(new Error('foo')); + const error = decorateEsUnavailableError(new Error('foo')); expect(error.output).to.have.property('statusCode', 503); }); }); }); }); describe('General error', () => { - describe('wrapGeneralError', () => { + describe('decorateGeneralError', () => { it('returns original object', () => { const error = new Error(); - expect(wrapGeneralError(error)).to.be(error); + expect(decorateGeneralError(error)).to.be(error); }); it('adds boom properties', () => { - const error = wrapGeneralError(new Error()); + const error = decorateGeneralError(new Error()); expect(error.output).to.be.an('object'); expect(error.output.statusCode).to.be(500); }); it('preserves boom properties of input', () => { const error = Boom.notFound(); - wrapGeneralError(error); + decorateGeneralError(error); expect(error.output.statusCode).to.be(404); }); describe('error.output', () => { it('ignores error message', () => { - const error = wrapGeneralError(new Error('foobar')); + const error = decorateGeneralError(new Error('foobar')); expect(error.output.payload).to.have.property('message').match(/internal server error/i); }); it('sets statusCode to 500', () => { - const error = wrapGeneralError(new Error('foo')); + const error = decorateGeneralError(new Error('foo')); expect(error.output).to.have.property('statusCode', 500); }); }); diff --git a/src/server/saved_objects/client/lib/decorate_es_error.js b/src/server/saved_objects/client/lib/decorate_es_error.js index f516ad6c2c60f..9f721e25eb6ba 100644 --- a/src/server/saved_objects/client/lib/decorate_es_error.js +++ b/src/server/saved_objects/client/lib/decorate_es_error.js @@ -14,13 +14,13 @@ const { } = elasticsearch.errors; import { - wrapBadRequestError, - wrapNotAuthorizedError, - wrapForbiddenError, - wrapNotFoundError, - wrapConflictError, - wrapEsUnavailableError, - wrapGeneralError, + decorateBadRequestError, + decorateNotAuthorizedError, + decorateForbiddenError, + decorateNotFoundError, + decorateConflictError, + decorateEsUnavailableError, + decorateGeneralError, } from './errors'; export function decorateEsError(error) { @@ -35,28 +35,28 @@ export function decorateEsError(error) { error instanceof NoConnections || error instanceof RequestTimeout ) { - return wrapEsUnavailableError(error, reason); + return decorateEsUnavailableError(error, reason); } if (error instanceof Conflict) { - return wrapConflictError(error, reason); + return decorateConflictError(error, reason); } if (error instanceof NotAuthorized) { - return wrapNotAuthorizedError(error, reason); + return decorateNotAuthorizedError(error, reason); } if (error instanceof Forbidden) { - return wrapForbiddenError(error, reason); + return decorateForbiddenError(error, reason); } if (error instanceof NotFound) { - return wrapNotFoundError(error, reason); + return decorateNotFoundError(error, reason); } if (error instanceof BadRequest) { - return wrapBadRequestError(error, reason); + return decorateBadRequestError(error, reason); } - return wrapGeneralError(error, reason); + return decorateGeneralError(error, reason); } diff --git a/src/server/saved_objects/client/lib/errors.js b/src/server/saved_objects/client/lib/errors.js index 3ce494073fb99..cbeb4921ffe70 100644 --- a/src/server/saved_objects/client/lib/errors.js +++ b/src/server/saved_objects/client/lib/errors.js @@ -2,7 +2,7 @@ import Boom from 'boom'; const code = Symbol('SavedObjectsClientErrorCode'); -function wrap(error, errorCode, statusCode, message) { +function decorate(error, errorCode, statusCode, message) { const boom = Boom.boomify(error, { statusCode, message, @@ -16,8 +16,8 @@ function wrap(error, errorCode, statusCode, message) { // 400 - badRequest const CODE_BAD_REQUEST = 'SavedObjectsClient/badRequest'; -export function wrapBadRequestError(error, reason) { - return wrap(error, CODE_BAD_REQUEST, 400, reason); +export function decorateBadRequestError(error, reason) { + return decorate(error, CODE_BAD_REQUEST, 400, reason); } export function isBadRequestError(error) { return error && error[code] === CODE_BAD_REQUEST; @@ -26,8 +26,8 @@ export function isBadRequestError(error) { // 401 - Not Authorized const CODE_NOT_AUTHORIZED = 'SavedObjectsClient/notAuthorized'; -export function wrapNotAuthorizedError(error, reason) { - return wrap(error, CODE_NOT_AUTHORIZED, 401, reason); +export function decorateNotAuthorizedError(error, reason) { + return decorate(error, CODE_NOT_AUTHORIZED, 401, reason); } export function isNotAuthorizedError(error) { return error && error[code] === CODE_NOT_AUTHORIZED; @@ -36,8 +36,8 @@ export function isNotAuthorizedError(error) { // 403 - Forbidden const CODE_FORBIDDEN = 'SavedObjectsClient/forbidden'; -export function wrapForbiddenError(error, reason) { - return wrap(error, CODE_FORBIDDEN, 403, reason); +export function decorateForbiddenError(error, reason) { + return decorate(error, CODE_FORBIDDEN, 403, reason); } export function isForbiddenError(error) { return error && error[code] === CODE_FORBIDDEN; @@ -46,8 +46,8 @@ export function isForbiddenError(error) { // 404 - Not Found const CODE_NOT_FOUND = 'SavedObjectsClient/notFound'; -export function wrapNotFoundError(error, reason) { - return wrap(error, CODE_NOT_FOUND, 404, reason); +export function decorateNotFoundError(error, reason) { + return decorate(error, CODE_NOT_FOUND, 404, reason); } export function isNotFoundError(error) { return error && error[code] === CODE_NOT_FOUND; @@ -56,8 +56,8 @@ export function isNotFoundError(error) { // 409 - Conflict const CODE_CONFLICT = 'SavedObjectsClient/conflict'; -export function wrapConflictError(error, reason) { - return wrap(error, CODE_CONFLICT, 409, reason); +export function decorateConflictError(error, reason) { + return decorate(error, CODE_CONFLICT, 409, reason); } export function isConflictError(error) { return error && error[code] === CODE_CONFLICT; @@ -66,8 +66,8 @@ export function isConflictError(error) { // 500 - Es Unavailable const CODE_ES_UNAVAILABLE = 'SavedObjectsClient/esUnavailable'; -export function wrapEsUnavailableError(error, reason) { - return wrap(error, CODE_ES_UNAVAILABLE, 503, reason); +export function decorateEsUnavailableError(error, reason) { + return decorate(error, CODE_ES_UNAVAILABLE, 503, reason); } export function isEsUnavailableError(error) { return error && error[code] === CODE_ES_UNAVAILABLE; @@ -76,6 +76,6 @@ export function isEsUnavailableError(error) { // 500 - General Error const CODE_GENERAL_ERROR = 'SavedObjectsClient/generalError'; -export function wrapGeneralError(error, reason) { - return wrap(error, CODE_GENERAL_ERROR, 500, reason); +export function decorateGeneralError(error, reason) { + return decorate(error, CODE_GENERAL_ERROR, 500, reason); } From fbb96391402c2c06e4be63931eccfb670835921b Mon Sep 17 00:00:00 2001 From: spalger Date: Thu, 27 Jul 2017 12:51:10 -0400 Subject: [PATCH 13/20] [server/routes/uiSettings] refactor routes to forward Boom errors from uiSettings --- .../routes/api/settings/register_delete.js | 26 ++++++++--------- .../routes/api/settings/register_get.js | 18 ++++++------ .../routes/api/settings/register_set.js | 28 +++++++++---------- .../routes/api/settings/register_set_many.js | 26 ++++++++--------- 4 files changed, 50 insertions(+), 48 deletions(-) diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js b/src/core_plugins/kibana/server/routes/api/settings/register_delete.js index 1b78e83b5eac6..2b2d5470182f0 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_delete.js @@ -1,20 +1,20 @@ -import Boom from 'boom'; - export default function registerDelete(server) { + + async function handleRequest(request) { + const { key } = request.params; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.remove(key); + return { + settings: await uiSettings.getUserProvided() + }; + } + server.route({ path: '/api/kibana/settings/{key}', method: 'DELETE', - handler: function (req, reply) { - const { key } = req.params; - const uiSettings = req.getUiSettingsService(); - - uiSettings - .remove(key) - .then(() => uiSettings - .getUserProvided() - .then(settings => reply({ settings }).type('application/json')) - ) - .catch(err => reply(Boom.wrap(err, err.statusCode))); + handler(request, reply) { + reply(handleRequest(request)); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_get.js b/src/core_plugins/kibana/server/routes/api/settings/register_get.js index 123f869ef0672..51deffc88f53c 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_get.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_get.js @@ -1,15 +1,17 @@ -import Boom from 'boom'; - export default function registerGet(server) { + + async function handleRequest(request) { + const uiSettings = request.getUiSettingsService(); + return { + settings: await uiSettings.getUserProvided() + }; + } + server.route({ path: '/api/kibana/settings', method: 'GET', - handler: function (req, reply) { - req - .getUiSettingsService() - .getUserProvided() - .then(settings => reply({ settings }).type('application/json')) - .catch(err => reply(Boom.wrap(err, err.statusCode))); + handler: function (request, reply) { + reply(handleRequest(request)); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set.js b/src/core_plugins/kibana/server/routes/api/settings/register_set.js index f82cafef43324..10dd2788581c7 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_set.js @@ -1,21 +1,21 @@ -import Boom from 'boom'; - export default function registerSet(server) { + + async function handleRequest(request) { + const { key } = request.params; + const { value } = request.payload; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.set(key, value); + return { + settings: await uiSettings.getUserProvided() + }; + } + server.route({ path: '/api/kibana/settings/{key}', method: 'POST', - handler: function (req, reply) { - const { key } = req.params; - const { value } = req.payload; - const uiSettings = req.getUiSettingsService(); - - uiSettings - .set(key, value) - .then(() => uiSettings - .getUserProvided() - .then(settings => reply({ settings }).type('application/json')) - ) - .catch(err => reply(Boom.wrap(err, err.statusCode))); + handler(request, reply) { + reply(handleRequest(request)); } }); } diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js b/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js index 71e873bd80d79..d831704aef4ba 100644 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js +++ b/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js @@ -1,20 +1,20 @@ -import Boom from 'boom'; - export default function registerSet(server) { + + async function handleRequest(request) { + const { changes } = request.payload; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.setMany(changes); + return { + settings: await uiSettings.getUserProvided() + }; + } + server.route({ path: '/api/kibana/settings', method: 'POST', - handler: function (req, reply) { - const { changes } = req.payload; - const uiSettings = req.getUiSettingsService(); - - uiSettings - .setMany(changes) - .then(() => uiSettings - .getUserProvided() - .then(settings => reply({ settings }).type('application/json')) - ) - .catch(err => reply(Boom.wrap(err, err.statusCode))); + handler: function (request, reply) { + reply(handleRequest(request)); } }); } From a8e59b32dc30f1195f6b8f161cc9d0172062d5a0 Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 11:21:15 -0700 Subject: [PATCH 14/20] [uiSettings] colocate routes and service --- src/core_plugins/kibana/index.js | 2 -- .../server/routes/api/settings/index.js | 6 ------ .../routes/api/settings/register_delete.js | 20 ------------------ .../routes/api/settings/register_get.js | 17 --------------- .../routes/api/settings/register_set.js | 21 ------------------- .../routes/api/settings/register_set_many.js | 20 ------------------ src/ui/ui_settings/routes/delete.js | 17 +++++++++++++++ src/ui/ui_settings/routes/get.js | 14 +++++++++++++ src/ui/ui_settings/routes/index.js | 4 ++++ src/ui/ui_settings/routes/set.js | 18 ++++++++++++++++ src/ui/ui_settings/routes/set_many.js | 17 +++++++++++++++ src/ui/ui_settings/ui_settings_mixin.js | 11 ++++++++++ 12 files changed, 81 insertions(+), 86 deletions(-) delete mode 100644 src/core_plugins/kibana/server/routes/api/settings/index.js delete mode 100644 src/core_plugins/kibana/server/routes/api/settings/register_delete.js delete mode 100644 src/core_plugins/kibana/server/routes/api/settings/register_get.js delete mode 100644 src/core_plugins/kibana/server/routes/api/settings/register_set.js delete mode 100644 src/core_plugins/kibana/server/routes/api/settings/register_set_many.js create mode 100644 src/ui/ui_settings/routes/delete.js create mode 100644 src/ui/ui_settings/routes/get.js create mode 100644 src/ui/ui_settings/routes/index.js create mode 100644 src/ui/ui_settings/routes/set.js create mode 100644 src/ui/ui_settings/routes/set_many.js diff --git a/src/core_plugins/kibana/index.js b/src/core_plugins/kibana/index.js index b237df06da533..a1e3fe0e353bc 100644 --- a/src/core_plugins/kibana/index.js +++ b/src/core_plugins/kibana/index.js @@ -5,7 +5,6 @@ import { mkdirp as mkdirpNode } from 'mkdirp'; import manageUuid from './server/lib/manage_uuid'; import search from './server/routes/api/search'; -import settings from './server/routes/api/settings'; import { scrollSearchApi } from './server/routes/api/scroll_search'; import { importApi } from './server/routes/api/import'; import { exportApi } from './server/routes/api/export'; @@ -142,7 +141,6 @@ export default function (kibana) { manageUuid(server); // routes search(server); - settings(server); scripts(server); scrollSearchApi(server); importApi(server); diff --git a/src/core_plugins/kibana/server/routes/api/settings/index.js b/src/core_plugins/kibana/server/routes/api/settings/index.js deleted file mode 100644 index d6c0df42ff44f..0000000000000 --- a/src/core_plugins/kibana/server/routes/api/settings/index.js +++ /dev/null @@ -1,6 +0,0 @@ -export default function (server) { - require('./register_get')(server); - require('./register_set')(server); - require('./register_set_many')(server); - require('./register_delete')(server); -} diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js b/src/core_plugins/kibana/server/routes/api/settings/register_delete.js deleted file mode 100644 index 2b2d5470182f0..0000000000000 --- a/src/core_plugins/kibana/server/routes/api/settings/register_delete.js +++ /dev/null @@ -1,20 +0,0 @@ -export default function registerDelete(server) { - - async function handleRequest(request) { - const { key } = request.params; - const uiSettings = request.getUiSettingsService(); - - await uiSettings.remove(key); - return { - settings: await uiSettings.getUserProvided() - }; - } - - server.route({ - path: '/api/kibana/settings/{key}', - method: 'DELETE', - handler(request, reply) { - reply(handleRequest(request)); - } - }); -} diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_get.js b/src/core_plugins/kibana/server/routes/api/settings/register_get.js deleted file mode 100644 index 51deffc88f53c..0000000000000 --- a/src/core_plugins/kibana/server/routes/api/settings/register_get.js +++ /dev/null @@ -1,17 +0,0 @@ -export default function registerGet(server) { - - async function handleRequest(request) { - const uiSettings = request.getUiSettingsService(); - return { - settings: await uiSettings.getUserProvided() - }; - } - - server.route({ - path: '/api/kibana/settings', - method: 'GET', - handler: function (request, reply) { - reply(handleRequest(request)); - } - }); -} diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set.js b/src/core_plugins/kibana/server/routes/api/settings/register_set.js deleted file mode 100644 index 10dd2788581c7..0000000000000 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set.js +++ /dev/null @@ -1,21 +0,0 @@ -export default function registerSet(server) { - - async function handleRequest(request) { - const { key } = request.params; - const { value } = request.payload; - const uiSettings = request.getUiSettingsService(); - - await uiSettings.set(key, value); - return { - settings: await uiSettings.getUserProvided() - }; - } - - server.route({ - path: '/api/kibana/settings/{key}', - method: 'POST', - handler(request, reply) { - reply(handleRequest(request)); - } - }); -} diff --git a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js b/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js deleted file mode 100644 index d831704aef4ba..0000000000000 --- a/src/core_plugins/kibana/server/routes/api/settings/register_set_many.js +++ /dev/null @@ -1,20 +0,0 @@ -export default function registerSet(server) { - - async function handleRequest(request) { - const { changes } = request.payload; - const uiSettings = request.getUiSettingsService(); - - await uiSettings.setMany(changes); - return { - settings: await uiSettings.getUserProvided() - }; - } - - server.route({ - path: '/api/kibana/settings', - method: 'POST', - handler: function (request, reply) { - reply(handleRequest(request)); - } - }); -} diff --git a/src/ui/ui_settings/routes/delete.js b/src/ui/ui_settings/routes/delete.js new file mode 100644 index 0000000000000..ccd3418101b52 --- /dev/null +++ b/src/ui/ui_settings/routes/delete.js @@ -0,0 +1,17 @@ +async function handleRequest(request) { + const { key } = request.params; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.remove(key); + return { + settings: await uiSettings.getUserProvided() + }; +} + +export const deleteRoute = { + path: '/api/kibana/settings/{key}', + method: 'DELETE', + handler(request, reply) { + reply(handleRequest(request)); + } +}; diff --git a/src/ui/ui_settings/routes/get.js b/src/ui/ui_settings/routes/get.js new file mode 100644 index 0000000000000..813c6b8a1fe2d --- /dev/null +++ b/src/ui/ui_settings/routes/get.js @@ -0,0 +1,14 @@ +async function handleRequest(request) { + const uiSettings = request.getUiSettingsService(); + return { + settings: await uiSettings.getUserProvided() + }; +} + +export const getRoute = { + path: '/api/kibana/settings', + method: 'GET', + handler: function (request, reply) { + reply(handleRequest(request)); + } +}; diff --git a/src/ui/ui_settings/routes/index.js b/src/ui/ui_settings/routes/index.js new file mode 100644 index 0000000000000..9100e0107ac35 --- /dev/null +++ b/src/ui/ui_settings/routes/index.js @@ -0,0 +1,4 @@ +export { deleteRoute } from './delete'; +export { getRoute } from './get'; +export { setManyRoute } from './set_many'; +export { setRoute } from './set'; diff --git a/src/ui/ui_settings/routes/set.js b/src/ui/ui_settings/routes/set.js new file mode 100644 index 0000000000000..cd8b16c678349 --- /dev/null +++ b/src/ui/ui_settings/routes/set.js @@ -0,0 +1,18 @@ +async function handleRequest(request) { + const { key } = request.params; + const { value } = request.payload; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.set(key, value); + return { + settings: await uiSettings.getUserProvided() + }; +} + +export const setRoute = { + path: '/api/kibana/settings/{key}', + method: 'POST', + handler(request, reply) { + reply(handleRequest(request)); + } +}; diff --git a/src/ui/ui_settings/routes/set_many.js b/src/ui/ui_settings/routes/set_many.js new file mode 100644 index 0000000000000..560aef40c10a2 --- /dev/null +++ b/src/ui/ui_settings/routes/set_many.js @@ -0,0 +1,17 @@ +async function handleRequest(request) { + const { changes } = request.payload; + const uiSettings = request.getUiSettingsService(); + + await uiSettings.setMany(changes); + return { + settings: await uiSettings.getUserProvided() + }; +} + +export const setManyRoute = { + path: '/api/kibana/settings', + method: 'POST', + handler: function (request, reply) { + reply(handleRequest(request)); + } +}; diff --git a/src/ui/ui_settings/ui_settings_mixin.js b/src/ui/ui_settings/ui_settings_mixin.js index 9c8468e759560..131219e7a421f 100644 --- a/src/ui/ui_settings/ui_settings_mixin.js +++ b/src/ui/ui_settings/ui_settings_mixin.js @@ -2,6 +2,12 @@ import { uiSettingsServiceFactory } from './ui_settings_service_factory'; import { getUiSettingsServiceForRequest } from './ui_settings_service_for_request'; import { mirrorStatus } from './mirror_status'; import { UiExportsConsumer } from './ui_exports_consumer'; +import { + deleteRoute, + getRoute, + setManyRoute, + setRoute, +} from './routes'; export function uiSettingsMixin(kbnServer, server, config) { const status = kbnServer.status.create('ui settings'); @@ -56,4 +62,9 @@ export function uiSettingsMixin(kbnServer, server, config) { server.uiSettings has been removed, see https://github.com/elastic/kibana/pull/12243. `); }); + + server.route(deleteRoute); + server.route(getRoute); + server.route(setManyRoute); + server.route(setRoute); } From 66316d93980ebf6117ebb96a92816aa228705cfa Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 13:59:17 -0700 Subject: [PATCH 15/20] [testUtils/esTestCluster] use more standard api style --- .../elasticsearch/lib/__tests__/routes.js | 4 +- .../__tests__/integration/with_es_archiver.js | 4 +- src/server/http/__tests__/index.js | 4 +- src/test_utils/es/es_test_cluster.js | 150 +++++++++--------- src/test_utils/es/index.js | 2 +- 5 files changed, 80 insertions(+), 84 deletions(-) diff --git a/src/core_plugins/elasticsearch/lib/__tests__/routes.js b/src/core_plugins/elasticsearch/lib/__tests__/routes.js index 73b60561eae01..25ce5a2d8000c 100644 --- a/src/core_plugins/elasticsearch/lib/__tests__/routes.js +++ b/src/core_plugins/elasticsearch/lib/__tests__/routes.js @@ -1,12 +1,12 @@ import { format } from 'util'; import * as kbnTestServer from '../../../../test_utils/kbn_server'; -import { esTestCluster } from '../../../../test_utils/es'; +import { createEsTestCluster } from '../../../../test_utils/es'; describe('plugins/elasticsearch', function () { describe('routes', function () { let kbnServer; - const es = esTestCluster.use({ + const es = createEsTestCluster({ name: 'core_plugins/es/routes', }); diff --git a/src/functional_test_runner/__tests__/integration/with_es_archiver.js b/src/functional_test_runner/__tests__/integration/with_es_archiver.js index 5bfd1a9383832..48a1d168d0f00 100644 --- a/src/functional_test_runner/__tests__/integration/with_es_archiver.js +++ b/src/functional_test_runner/__tests__/integration/with_es_archiver.js @@ -4,7 +4,7 @@ import { format as formatUrl } from 'url'; import { readConfigFile } from '../../lib'; import { createToolingLog, createReduceStream } from '../../../utils'; -import { esTestCluster } from '../../../test_utils/es'; +import { createEsTestCluster } from '../../../test_utils/es'; import { startupKibana } from '../lib'; const SCRIPT = resolve(__dirname, '../../../../scripts/functional_test_runner.js'); @@ -24,7 +24,7 @@ describe('single test that uses esArchiver', () => { log.info('starting elasticsearch'); log.indent(2); - const es = esTestCluster.use({ + const es = createEsTestCluster({ log: msg => log.debug(msg), name: 'ftr/withEsArchiver', port: config.get('servers.elasticsearch.port') diff --git a/src/server/http/__tests__/index.js b/src/server/http/__tests__/index.js index e51d2e8634194..f6909114698e4 100644 --- a/src/server/http/__tests__/index.js +++ b/src/server/http/__tests__/index.js @@ -1,10 +1,10 @@ import expect from 'expect.js'; import * as kbnTestServer from '../../../test_utils/kbn_server'; -import { esTestCluster } from '../../../test_utils/es'; +import { createEsTestCluster } from '../../../test_utils/es'; describe('routes', () => { let kbnServer; - const es = esTestCluster.use({ + const es = createEsTestCluster({ name: 'server/http', }); diff --git a/src/test_utils/es/es_test_cluster.js b/src/test_utils/es/es_test_cluster.js index 9e96554a12e83..97ec418e1d831 100644 --- a/src/test_utils/es/es_test_cluster.js +++ b/src/test_utils/es/es_test_cluster.js @@ -1,103 +1,99 @@ import { resolve } from 'path'; -import { esTestConfig } from './es_test_config'; import libesvm from 'libesvm'; +import { esTestConfig } from './es_test_config'; + const ESVM_DIR = resolve(__dirname, '../../../esvm/test_utils/es_test_cluster'); +const BRANCHES_DOWNLOADED = []; -export class EsTestCluster { - _branchesDownloaded = []; +function isDownloadNeeded(branch) { + if (process.env.ESVM_NO_FRESH || process.argv.includes('--esvm-no-fresh')) { + return false; + } - use(options = {}) { - const { - name, - log = console.log, - port = esTestConfig.getPort(), - branch = esTestConfig.getBranch(), - } = options; + if (BRANCHES_DOWNLOADED.includes(branch)) { + return false; + } - if (!name) { - throw new Error('esTestCluster.use() requires { name }'); - } + return true; +} - // assigned in use.start(), reassigned in use.stop() - let cluster; +export function createEsTestCluster(options = {}) { + const { + name, + log = console.log, + port = esTestConfig.getPort(), + branch = esTestConfig.getBranch(), + } = options; - return { - getStartTimeout: () => { - return esTestConfig.getLibesvmStartTimeout(); - }, + if (!name) { + throw new Error('createEsTestCluster() requires { name }'); + } - start: async () => { - const download = this._isDownloadNeeded(branch); + // assigned in use.start(), reassigned in use.stop() + let cluster; - if (cluster) { - throw new Error(` - EsTestCluster[${name}] is already started, call and await es.stop() - before calling es.start() again. - `); - } + return new class EsTestCluster { + getStartTimeout() { + return esTestConfig.getLibesvmStartTimeout(); + } + + async start() { + const download = isDownloadNeeded(branch); - cluster = libesvm.createCluster({ - fresh: download, - purge: !download, - directory: ESVM_DIR, - branch, - config: { - http: { - port, - }, - cluster: { - name, - }, - discovery: { - zen: { - ping: { - unicast: { - hosts: [ `localhost:${port}` ] - } + if (cluster) { + throw new Error(` + EsTestCluster[${name}] is already started, call and await es.stop() + before calling es.start() again. + `); + } + + cluster = libesvm.createCluster({ + fresh: download, + purge: !download, + directory: ESVM_DIR, + branch, + config: { + http: { + port, + }, + cluster: { + name, + }, + discovery: { + zen: { + ping: { + unicast: { + hosts: [ `localhost:${port}` ] } } } } - }); - - cluster.on('log', (event) => { - log(`EsTestCluster[${name}]: ${event.type} - ${event.message}`); - }); - - await cluster.install(); - - if (download) { - // track the branches that have successfully downloaded - // after cluster.install() resolves - this._branchesDownloaded.push(branch); } + }); - await cluster.start(); - }, + cluster.on('log', (event) => { + log(`EsTestCluster[${name}]: ${event.type} - ${event.message}`); + }); - stop: async () => { - if (cluster) { - const c = cluster; - cluster = null; - await c.shutdown(); - } + await cluster.install(); + + if (download) { + // track the branches that have successfully downloaded + // after cluster.install() resolves + BRANCHES_DOWNLOADED.push(branch); } - }; - } - _isDownloadNeeded(branch) { - if (process.env.ESVM_NO_FRESH || process.argv.includes('--esvm-no-fresh')) { - return false; + await cluster.start(); } - if (this._branchesDownloaded.includes(branch)) { - return false; + async stop() { + if (cluster) { + const c = cluster; + cluster = null; + await c.shutdown(); + } } - - return true; - } + }; } - -export const esTestCluster = new EsTestCluster(); diff --git a/src/test_utils/es/index.js b/src/test_utils/es/index.js index 7230b751960c6..0409e204608eb 100644 --- a/src/test_utils/es/index.js +++ b/src/test_utils/es/index.js @@ -1,2 +1,2 @@ -export { esTestCluster } from './es_test_cluster'; export { esTestConfig } from './es_test_config'; +export { createEsTestCluster } from './es_test_cluster'; From e1a91e039be4f2e7a36b83264376ada4a63b0234 Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 14:27:08 -0700 Subject: [PATCH 16/20] [testUtils/es] add createCallCluster util --- src/test_utils/es/create_call_cluster.js | 21 +++++++++++++++++++ src/test_utils/es/index.js | 1 + .../services/kibana_server/ui_settings.js | 16 +------------- 3 files changed, 23 insertions(+), 15 deletions(-) create mode 100644 src/test_utils/es/create_call_cluster.js diff --git a/src/test_utils/es/create_call_cluster.js b/src/test_utils/es/create_call_cluster.js new file mode 100644 index 0000000000000..2801b466d3fa8 --- /dev/null +++ b/src/test_utils/es/create_call_cluster.js @@ -0,0 +1,21 @@ +import { get } from 'lodash'; +import toPath from 'lodash/internal/toPath'; + +/** + * Create a callCluster function that properly executes methods on an + * elasticsearch-js client + * + * @param {elasticsearch.Client} esClient + * @return {Function} + */ +export function createCallCluster(esClient) { + return function callCluster(method, params) { + const path = toPath(method); + const contextPath = path.slice(0, -1); + + const action = get(esClient, path); + const context = contextPath.length ? get(esClient, contextPath) : esClient; + + return action.call(context, params); + }; +} diff --git a/src/test_utils/es/index.js b/src/test_utils/es/index.js index 0409e204608eb..0b4aa85c802a0 100644 --- a/src/test_utils/es/index.js +++ b/src/test_utils/es/index.js @@ -1,2 +1,3 @@ export { esTestConfig } from './es_test_config'; export { createEsTestCluster } from './es_test_cluster'; +export { createCallCluster } from './create_call_cluster'; diff --git a/test/common/services/kibana_server/ui_settings.js b/test/common/services/kibana_server/ui_settings.js index 7c6439e0783b1..0d45eaa361cc0 100644 --- a/test/common/services/kibana_server/ui_settings.js +++ b/test/common/services/kibana_server/ui_settings.js @@ -1,20 +1,6 @@ -import { get } from 'lodash'; -import toPath from 'lodash/internal/toPath'; - +import { createCallCluster } from '../../../../src/test_utils/es'; import { SavedObjectsClient } from '../../../../src/server/saved_objects'; -function createCallCluster(es) { - return function callCluster(method, params) { - const path = toPath(method); - const contextPath = path.slice(0, -1); - - const action = get(es, path); - const context = contextPath.length ? get(es, contextPath) : es; - - return action.call(context, params); - }; -} - export class KibanaServerUiSettings { constructor(log, es, kibanaIndex, kibanaVersion) { this._log = log; From 4c8bb8b46e067818128d5e58e32f621c6c578f15 Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 14:27:55 -0700 Subject: [PATCH 17/20] [testUtils/esTestCluster] add getters for client/callCluster --- src/test_utils/es/es_test_cluster.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/test_utils/es/es_test_cluster.js b/src/test_utils/es/es_test_cluster.js index 97ec418e1d831..0f08ff994cd90 100644 --- a/src/test_utils/es/es_test_cluster.js +++ b/src/test_utils/es/es_test_cluster.js @@ -1,8 +1,10 @@ import { resolve } from 'path'; import libesvm from 'libesvm'; +import elasticsearch from 'elasticsearch'; import { esTestConfig } from './es_test_config'; +import { createCallCluster } from './create_call_cluster'; const ESVM_DIR = resolve(__dirname, '../../../esvm/test_utils/es_test_cluster'); const BRANCHES_DOWNLOADED = []; @@ -33,12 +35,27 @@ export function createEsTestCluster(options = {}) { // assigned in use.start(), reassigned in use.stop() let cluster; + let client; return new class EsTestCluster { getStartTimeout() { return esTestConfig.getLibesvmStartTimeout(); } + getClient() { + if (!client) { + client = new elasticsearch.Client({ + host: esTestConfig.getUrl() + }); + } + + return client; + } + + getCallCluster() { + return createCallCluster(this.getClient()); + } + async start() { const download = isDownloadNeeded(branch); @@ -89,6 +106,12 @@ export function createEsTestCluster(options = {}) { } async stop() { + if (client) { + const c = client; + client = null; + await c.close(); + } + if (cluster) { const c = cluster; cluster = null; From 22056976f60efb99bf6daa6ba7df5618f82cb790 Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 15:49:54 -0700 Subject: [PATCH 18/20] [es/healthcheck] ensure that healtcheck stops when server is stopped --- .../lib/__tests__/health_check.js | 32 +++++++++++++++++-- .../elasticsearch/lib/health_check.js | 5 +++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/core_plugins/elasticsearch/lib/__tests__/health_check.js b/src/core_plugins/elasticsearch/lib/__tests__/health_check.js index 6e315c08e5069..01e024072b195 100644 --- a/src/core_plugins/elasticsearch/lib/__tests__/health_check.js +++ b/src/core_plugins/elasticsearch/lib/__tests__/health_check.js @@ -21,8 +21,13 @@ describe('plugins/elasticsearch', () => { let health; let plugin; let cluster; + let server; const sandbox = sinon.sandbox.create(); + function getTimerCount() { + return Object.keys(sandbox.clock.timers || {}); + } + beforeEach(() => { const COMPATIBLE_VERSION_NUMBER = '5.0.0'; @@ -66,7 +71,7 @@ describe('plugins/elasticsearch', () => { const set = sinon.stub(); // Setup the server mock - const server = { + server = { log: sinon.stub(), info: { port: 5601 }, config: function () { return { get, set }; }, @@ -77,7 +82,8 @@ describe('plugins/elasticsearch', () => { }, getKibanaIndexMappingsDsl() { return mappings; - } + }, + ext: sinon.stub() }; health = healthCheck(plugin, server); @@ -85,6 +91,28 @@ describe('plugins/elasticsearch', () => { afterEach(() => sandbox.restore()); + it('should stop when cluster is shutdown', () => { + sandbox.useFakeTimers(); + + // ensure that health.start() is responsible for the timer we are observing + expect(getTimerCount()).to.be(0); + health.start(); + expect(getTimerCount()).to.be(1); + + // ensure that a server extension was registered + sinon.assert.calledOnce(server.ext); + sinon.assert.calledWithExactly(server.ext, sinon.match.string, sinon.match.func); + + // call the server extension + const reply = sinon.stub(); + const [,handler] = server.ext.firstCall.args; + handler({}, reply); + + // ensure that the handler called reply and unregistered the time + sinon.assert.calledOnce(reply); + expect(getTimerCount()).to.be(0); + }); + it('should set the cluster green if everything is ready', function () { cluster.callWithInternalUser.withArgs('ping').returns(Promise.resolve()); cluster.callWithInternalUser.withArgs('cluster.health', sinon.match.any).returns( diff --git a/src/core_plugins/elasticsearch/lib/health_check.js b/src/core_plugins/elasticsearch/lib/health_check.js index 1874ededd3bb0..2cc952e1b8830 100644 --- a/src/core_plugins/elasticsearch/lib/health_check.js +++ b/src/core_plugins/elasticsearch/lib/health_check.js @@ -145,6 +145,11 @@ export default function (plugin, server) { return true; } + server.ext('onPreStop', (request, reply) => { + stopChecking(); + reply(); + }); + return { waitUntilReady: waitUntilReady, run: check, From e902cd6507810d48005378f975e2980b31ebe85a Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 23:44:38 -0700 Subject: [PATCH 19/20] [uiSettings/routes] add param/payload validation --- src/ui/ui_settings/routes/set.js | 17 +++++++++++++++-- src/ui/ui_settings/routes/set_many.js | 13 +++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/ui/ui_settings/routes/set.js b/src/ui/ui_settings/routes/set.js index cd8b16c678349..5070304d4eb00 100644 --- a/src/ui/ui_settings/routes/set.js +++ b/src/ui/ui_settings/routes/set.js @@ -1,3 +1,5 @@ +import Joi from 'joi'; + async function handleRequest(request) { const { key } = request.params; const { value } = request.payload; @@ -12,7 +14,18 @@ async function handleRequest(request) { export const setRoute = { path: '/api/kibana/settings/{key}', method: 'POST', - handler(request, reply) { - reply(handleRequest(request)); + config: { + validate: { + params: Joi.object().keys({ + key: Joi.string().required(), + }).default(), + + payload: Joi.object().keys({ + value: Joi.any().required() + }).required() + }, + handler(request, reply) { + reply(handleRequest(request)); + } } }; diff --git a/src/ui/ui_settings/routes/set_many.js b/src/ui/ui_settings/routes/set_many.js index 560aef40c10a2..c43aa6f72ba5c 100644 --- a/src/ui/ui_settings/routes/set_many.js +++ b/src/ui/ui_settings/routes/set_many.js @@ -1,3 +1,5 @@ +import Joi from 'joi'; + async function handleRequest(request) { const { changes } = request.payload; const uiSettings = request.getUiSettingsService(); @@ -11,7 +13,14 @@ async function handleRequest(request) { export const setManyRoute = { path: '/api/kibana/settings', method: 'POST', - handler: function (request, reply) { - reply(handleRequest(request)); + config: { + validate: { + payload: Joi.object().keys({ + changes: Joi.object().unknown(true).required() + }).required() + }, + handler(request, reply) { + reply(handleRequest(request)); + } } }; From 256a0b21a90e8b9ba15f797ff7205d71470e6e10 Mon Sep 17 00:00:00 2001 From: spalger Date: Fri, 28 Jul 2017 23:47:56 -0700 Subject: [PATCH 20/20] [uiSettings/routes] add tests that verify error behaviors --- .../lib/__tests__/health_check.js | 2 +- .../ui_settings_mixin_integration.js | 1 + .../routes/__tests__/doc_exists.js | 134 ++++++++++++++++++ .../routes/__tests__/doc_missing.js | 77 ++++++++++ src/ui/ui_settings/routes/__tests__/index.js | 23 +++ .../routes/__tests__/index_missing.js | 116 +++++++++++++++ .../routes/__tests__/lib/assert.js | 16 +++ .../routes/__tests__/lib/chance.js | 3 + .../ui_settings/routes/__tests__/lib/index.js | 14 ++ .../routes/__tests__/lib/servers.js | 53 +++++++ 10 files changed, 438 insertions(+), 1 deletion(-) create mode 100644 src/ui/ui_settings/routes/__tests__/doc_exists.js create mode 100644 src/ui/ui_settings/routes/__tests__/doc_missing.js create mode 100644 src/ui/ui_settings/routes/__tests__/index.js create mode 100644 src/ui/ui_settings/routes/__tests__/index_missing.js create mode 100644 src/ui/ui_settings/routes/__tests__/lib/assert.js create mode 100644 src/ui/ui_settings/routes/__tests__/lib/chance.js create mode 100644 src/ui/ui_settings/routes/__tests__/lib/index.js create mode 100644 src/ui/ui_settings/routes/__tests__/lib/servers.js diff --git a/src/core_plugins/elasticsearch/lib/__tests__/health_check.js b/src/core_plugins/elasticsearch/lib/__tests__/health_check.js index 01e024072b195..af6d8df377758 100644 --- a/src/core_plugins/elasticsearch/lib/__tests__/health_check.js +++ b/src/core_plugins/elasticsearch/lib/__tests__/health_check.js @@ -25,7 +25,7 @@ describe('plugins/elasticsearch', () => { const sandbox = sinon.sandbox.create(); function getTimerCount() { - return Object.keys(sandbox.clock.timers || {}); + return Object.keys(sandbox.clock.timers || {}).length; } beforeEach(() => { diff --git a/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js b/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js index cdc7a2b48ebcd..67dcb9cfd5751 100644 --- a/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js +++ b/src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js @@ -37,6 +37,7 @@ describe('uiSettingsMixin()', () => { // mock hapi server const server = { log: sinon.stub(), + route: sinon.stub(), config: () => config, addMemoizedFactoryToRequest(name, factory) { this.decorate('request', name, function () { diff --git a/src/ui/ui_settings/routes/__tests__/doc_exists.js b/src/ui/ui_settings/routes/__tests__/doc_exists.js new file mode 100644 index 0000000000000..13b445089a1d2 --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/doc_exists.js @@ -0,0 +1,134 @@ +import expect from 'expect.js'; +import sinon from 'sinon'; + +import { + getServices, + chance, + assertSinonMatch, +} from './lib'; + +export function docExistsSuite() { + async function setup(options = {}) { + const { + initialSettings + } = options; + + const { kbnServer, uiSettings } = getServices(); + + if (initialSettings) { + await uiSettings.setMany(initialSettings); + } + + return { kbnServer, uiSettings }; + } + + describe('get route', () => { + it('returns a 200 and includes userValues', async () => { + const defaultIndex = chance.word({ length: 10 }); + const { kbnServer } = await setup({ + initialSettings: { + defaultIndex + } + }); + + const { statusCode, result } = await kbnServer.inject({ + method: 'GET', + url: '/api/kibana/settings' + }); + + expect(statusCode).to.be(200); + assertSinonMatch(result, { + settings: { + buildNum: { + userValue: sinon.match.number + }, + defaultIndex: { + userValue: defaultIndex + } + } + }); + }); + }); + + describe('set route', () => { + it('returns a 200 and all values including update', async () => { + const { kbnServer } = await setup(); + + const defaultIndex = chance.word(); + const { statusCode, result } = await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings/defaultIndex', + payload: { + value: defaultIndex + } + }); + + expect(statusCode).to.be(200); + assertSinonMatch(result, { + settings: { + buildNum: { + userValue: sinon.match.number + }, + defaultIndex: { + userValue: defaultIndex + } + } + }); + }); + }); + + describe('setMany route', () => { + it('returns a 200 and all values including updates', async () => { + const { kbnServer } = await setup(); + + const defaultIndex = chance.word(); + const { statusCode, result } = await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings', + payload: { + changes: { + defaultIndex + } + } + }); + + expect(statusCode).to.be(200); + assertSinonMatch(result, { + settings: { + buildNum: { + userValue: sinon.match.number + }, + defaultIndex: { + userValue: defaultIndex + } + } + }); + }); + }); + + describe('delete route', () => { + it('returns a 200 and deletes the setting', async () => { + const defaultIndex = chance.word({ length: 10 }); + + const { kbnServer, uiSettings } = await setup({ + initialSettings: { defaultIndex } + }); + + expect(await uiSettings.get('defaultIndex')).to.be(defaultIndex); + + const { statusCode, result } = await kbnServer.inject({ + method: 'DELETE', + url: '/api/kibana/settings/defaultIndex' + }); + + expect(statusCode).to.be(200); + assertSinonMatch(result, { + settings: { + buildNum: { + userValue: sinon.match.number + } + } + }); + }); + }); +} diff --git a/src/ui/ui_settings/routes/__tests__/doc_missing.js b/src/ui/ui_settings/routes/__tests__/doc_missing.js new file mode 100644 index 0000000000000..8c46aaa113dcf --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/doc_missing.js @@ -0,0 +1,77 @@ +import expect from 'expect.js'; + +import { + getServices, + chance, + assertDocMissingResponse +} from './lib'; + +export function docMissingSuite() { + async function setup() { + const { kbnServer, savedObjectsClient } = getServices(); + + // delete all config docs + const { saved_objects: objs } = await savedObjectsClient.find({ type: 'config' }); + + for (const obj of objs) { + await savedObjectsClient.delete(obj.type, obj.id); + } + + return { kbnServer }; + } + + describe('get route', () => { + it('returns a 200 with empty values', async () => { + const { kbnServer } = await setup(); + + const { statusCode, result } = await kbnServer.inject({ + method: 'GET', + url: '/api/kibana/settings' + }); + + expect(statusCode).to.be(200); + expect(result).to.eql({ settings: {} }); + }); + }); + + describe('set route', () => { + it('returns a 404', async () => { + const { kbnServer } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings/defaultIndex', + payload: { + value: chance.word() + } + })); + }); + }); + + describe('setMany route', () => { + it('returns a 404', async () => { + const { kbnServer } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings', + payload: { + changes: { + defaultIndex: chance.word() + } + } + })); + }); + }); + + describe('delete route', () => { + it('returns a 404', async () => { + const { kbnServer } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'DELETE', + url: '/api/kibana/settings/defaultIndex' + })); + }); + }); +} diff --git a/src/ui/ui_settings/routes/__tests__/index.js b/src/ui/ui_settings/routes/__tests__/index.js new file mode 100644 index 0000000000000..d2936052a94e4 --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/index.js @@ -0,0 +1,23 @@ +import { + startServers, + stopServers, +} from './lib'; + +import { docExistsSuite } from './doc_exists'; +import { docMissingSuite } from './doc_missing'; +import { indexMissingSuite } from './index_missing'; + +describe('uiSettings/routes', function () { + this.slow(2000); + this.timeout(10000); + + // these tests rely on getting sort of lucky with + // the healthcheck, so we retry if they fail + this.retries(3); + + before(startServers); + describe('doc exists', docExistsSuite); + describe('doc missing', docMissingSuite); + describe('index missing', indexMissingSuite); + after(stopServers); +}); diff --git a/src/ui/ui_settings/routes/__tests__/index_missing.js b/src/ui/ui_settings/routes/__tests__/index_missing.js new file mode 100644 index 0000000000000..790ba02207f4f --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/index_missing.js @@ -0,0 +1,116 @@ +import expect from 'expect.js'; + +import { + getServices, + chance, + assertDocMissingResponse +} from './lib'; + +export function indexMissingSuite() { + beforeEach(async function () { + const { kbnServer } = getServices(); + await kbnServer.server.plugins.elasticsearch.waitUntilReady(); + }); + + function getNumberOfShards(index) { + return parseInt(Object.values(index)[0].settings.index.number_of_shards, 10); + } + + async function getIndex(callCluster, indexName) { + return await callCluster('indices.get', { + index: indexName, + }); + } + + async function setup() { + const { callCluster, kbnServer } = getServices(); + const indexName = kbnServer.config.get('kibana.index'); + const initialIndex = await getIndex(callCluster, indexName); + + await callCluster('indices.delete', { + index: indexName, + }); + + return { + kbnServer, + + // an incorrect number of shards is how we determine when the index was not created by Kibana, + // but automatically by writing to es when index didn't exist + async assertInvalidKibanaIndex() { + const index = await getIndex(callCluster, indexName); + + expect(getNumberOfShards(index)) + .to.not.be(getNumberOfShards(initialIndex)); + } + }; + } + + afterEach(async () => { + const { kbnServer, callCluster } = getServices(); + await callCluster('indices.delete', { + index: kbnServer.config.get('kibana.index'), + ignore: 404 + }); + }); + + describe('get route', () => { + it('returns a 200 and with empty values', async () => { + const { kbnServer } = await setup(); + + const { statusCode, result } = await kbnServer.inject({ + method: 'GET', + url: '/api/kibana/settings' + }); + + expect(statusCode).to.be(200); + expect(result).to.eql({ settings: {} }); + }); + }); + + describe('set route', () => { + it('creates an invalid Kibana index and returns a 404 document missing error', async () => { + const { kbnServer, assertInvalidKibanaIndex } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings/defaultIndex', + payload: { + value: chance.word() + } + })); + + await assertInvalidKibanaIndex(); + }); + }); + + describe('setMany route', () => { + it('creates an invalid Kibana index and returns a 404 document missing error', async () => { + const { kbnServer, assertInvalidKibanaIndex } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'POST', + url: '/api/kibana/settings', + payload: { + changes: { + defaultIndex: chance.word() + } + } + })); + + await assertInvalidKibanaIndex(); + }); + }); + + describe('delete route', () => { + it('creates an invalid Kibana index and returns a 404 document missing error', async () => { + const { kbnServer, assertInvalidKibanaIndex } = await setup(); + + assertDocMissingResponse(await kbnServer.inject({ + method: 'DELETE', + url: '/api/kibana/settings/defaultIndex' + })); + + await assertInvalidKibanaIndex(); + }); + }); +} diff --git a/src/ui/ui_settings/routes/__tests__/lib/assert.js b/src/ui/ui_settings/routes/__tests__/lib/assert.js new file mode 100644 index 0000000000000..36de9a0e1e090 --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/lib/assert.js @@ -0,0 +1,16 @@ +import sinon from 'sinon'; + +export function assertSinonMatch(value, match) { + const stub = sinon.stub(); + stub(value); + sinon.assert.calledWithExactly(stub, match); +} + +export function assertDocMissingResponse({ result }) { + assertSinonMatch(result, { + statusCode: 404, + error: 'Not Found', + message: sinon.match('document_missing_exception') + .and(sinon.match('document missing')) + }); +} diff --git a/src/ui/ui_settings/routes/__tests__/lib/chance.js b/src/ui/ui_settings/routes/__tests__/lib/chance.js new file mode 100644 index 0000000000000..f3b9840f57537 --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/lib/chance.js @@ -0,0 +1,3 @@ +import Chance from 'chance'; + +export const chance = new Chance(); diff --git a/src/ui/ui_settings/routes/__tests__/lib/index.js b/src/ui/ui_settings/routes/__tests__/lib/index.js new file mode 100644 index 0000000000000..3bb32ca01ce8f --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/lib/index.js @@ -0,0 +1,14 @@ +export { + startServers, + getServices, + stopServers +} from './servers'; + +export { + chance +} from './chance'; + +export { + assertSinonMatch, + assertDocMissingResponse +} from './assert'; diff --git a/src/ui/ui_settings/routes/__tests__/lib/servers.js b/src/ui/ui_settings/routes/__tests__/lib/servers.js new file mode 100644 index 0000000000000..2cc5c7a29c78d --- /dev/null +++ b/src/ui/ui_settings/routes/__tests__/lib/servers.js @@ -0,0 +1,53 @@ +import { createEsTestCluster } from '../../../../../test_utils/es'; +import * as kbnTestServer from '../../../../../test_utils/kbn_server'; + +let kbnServer; +let services; +const es = createEsTestCluster({ + name: 'ui_settings/routes' +}); + +export async function startServers() { + this.timeout(es.getStartTimeout()); + await es.start(); + + kbnServer = kbnTestServer.createServerWithCorePlugins(); + await kbnServer.ready(); + await kbnServer.server.plugins.elasticsearch.waitUntilReady(); +} + +export function getServices() { + if (services) { + return services; + } + + const callCluster = es.getCallCluster(); + + const savedObjectsClient = kbnServer.server.savedObjectsClientFactory({ + callCluster, + }); + + const uiSettings = kbnServer.server.uiSettingsServiceFactory({ + savedObjectsClient, + }); + + services = { + kbnServer, + callCluster, + savedObjectsClient, + uiSettings + }; + + return services; +} + +export async function stopServers() { + services = null; + + if (kbnServer) { + await kbnServer.close(); + kbnServer = null; + } + + await es.stop(); +}