Skip to content

Commit

Permalink
[savedObjects] fix error handling when Kibana index is missing (elast…
Browse files Browse the repository at this point in the history 
…ic#14141)

* [savedObjects/delete+bulk_get] add failing tests

* [savedObjects/delete+bulk_get] improve 404 handling

* [savedObjects/client] fix mocha tests

* [savedObjects/tests] remove extra test wrapper

* [apiIntegration/kbnServer] basically disable es healthcheck

* [savedObjects/create] add integration test

* [savedObjects/find] add failing integration tests

* [savedObjects/find] fix failing test

* [savedObjects/client] explain reason for generic 404s

* [savedObjects/get] add integration tests

* [savedObjects/find] test request with unkown type

* [savedObjects/find] add some more weird param tests

* [savedObjects/find] test that weird params pass when no index

* [savedObjects/update] use generic 404

* fix typos

* [savedObjects/update] add integration tests

* remove debugging uncomment

* [savedObjects/tests] move backup kibana index delete out of tests

* [savedObjects/tests/esArchives] remove logstash data

* [savedObjects] update test

* [uiSettings] remove detailed previously leaked from API

* [functional/dashboard] wrap check that is only failing on Jenkins

* [savedObjects/error] replace decorateNotFound with createGenericNotFound

* fix typo

* [savedObjectsClient/errors] fix decorateEsError() test

* [savedObjectsClient] fix typos

* [savedObjects/tests/functional] delete document that would normally exist

* [savedObjectsClient/tests] use sinon assertions

* [savedObjects/apiTests] create without index responds with 503 after elastic#14202
  • Loading branch information
@spalger @chrisronline
spalger authored and chrisronline committed Nov 20, 2017
1 parent 104ffe7 commit 354528f
Show file tree
Hide file tree
Showing 20 changed files with 1,076 additions and 131 deletions.
147 changes: 77 additions & 70 deletions src/server/saved_objects/client/__tests__/saved_objects_client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,9 @@ describe('SavedObjectsClient', () => {
title: 'Logstash'
});

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWith(callAdminCluster, 'index');
sinon.assert.calledOnce(onBeforeWrite);

const args = callAdminCluster.getCall(0).args;
expect(args[0]).to.be('index');
});

it('should use create action if ID defined and overwrite=false', async () => {
Expand All @@ -141,35 +139,35 @@ describe('SavedObjectsClient', () => {
id: 'logstash-*',
});

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWith(callAdminCluster, 'create');
sinon.assert.calledOnce(onBeforeWrite);

const args = callAdminCluster.getCall(0).args;
expect(args[0]).to.be('create');
});

it('allows for id to be provided', async () => {
await savedObjectsClient.create('index-pattern', {
title: 'Logstash'
}, { id: 'logstash-*' });

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(onBeforeWrite);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
id: 'index-pattern:logstash-*'
}));

const args = callAdminCluster.getCall(0).args;
expect(args[1].id).to.be('index-pattern:logstash-*');
sinon.assert.calledOnce(onBeforeWrite);
});

it('self-generates an ID', async () => {
await savedObjectsClient.create('index-pattern', {
title: 'Logstash'
});

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(onBeforeWrite);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
id: sinon.match(/index-pattern:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/)
}));

const args = callAdminCluster.getCall(0).args;
expect(args[1].id).to.match(/index-pattern:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/);
sinon.assert.calledOnce(onBeforeWrite);
});
});

Expand All @@ -182,26 +180,24 @@ describe('SavedObjectsClient', () => {
{ type: 'index-pattern', id: 'two', attributes: { title: 'Test Two' } }
]);

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(onBeforeWrite);

const args = callAdminCluster.getCall(0).args;
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, 'bulk', sinon.match({
body: [
{ create: { _type: 'doc', _id: 'config:one' } },
{ type: 'config', ...mockTimestampFields, config: { title: 'Test One' } },
{ create: { _type: 'doc', _id: 'index-pattern:two' } },
{ type: 'index-pattern', ...mockTimestampFields, 'index-pattern': { title: 'Test Two' } }
]
}));

expect(args[0]).to.be('bulk');
expect(args[1].body).to.eql([
{ create: { _type: 'doc', _id: 'config:one' } },
{ type: 'config', ...mockTimestampFields, config: { title: 'Test One' } },
{ create: { _type: 'doc', _id: 'index-pattern:two' } },
{ type: 'index-pattern', ...mockTimestampFields, 'index-pattern': { title: 'Test Two' } }
]);
sinon.assert.calledOnce(onBeforeWrite);
});

it('should overwrite objects if overwrite is truthy', async () => {
callAdminCluster.returns({ items: [] });

await savedObjectsClient.bulkCreate([{ type: 'foo', id: 'bar', attributes: {} }], { overwrite: false });
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledOnce(onBeforeWrite);
sinon.assert.calledWithExactly(callAdminCluster, 'bulk', sinon.match({
body: [
// uses create because overwriting is not allowed
Expand All @@ -210,12 +206,13 @@ describe('SavedObjectsClient', () => {
]
}));

sinon.assert.calledOnce(onBeforeWrite);

callAdminCluster.reset();
onBeforeWrite.reset();

await savedObjectsClient.bulkCreate([{ type: 'foo', id: 'bar', attributes: {} }], { overwrite: true });
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledOnce(onBeforeWrite);
sinon.assert.calledWithExactly(callAdminCluster, 'bulk', sinon.match({
body: [
// uses index because overwriting is allowed
Expand All @@ -224,7 +221,7 @@ describe('SavedObjectsClient', () => {
]
}));


sinon.assert.calledOnce(onBeforeWrite);
});

it('returns document errors', async () => {
Expand Down Expand Up @@ -310,7 +307,9 @@ describe('SavedObjectsClient', () => {

describe('#delete', () => {
it('throws notFound when ES is unable to find the document', async () => {
callAdminCluster.returns(Promise.resolve({ found: false }));
callAdminCluster.returns(Promise.resolve({
result: 'not_found'
}));

try {
await savedObjectsClient.delete('index-pattern', 'logstash-*');
Expand All @@ -323,20 +322,21 @@ describe('SavedObjectsClient', () => {
});

it('passes the parameters to callAdminCluster', async () => {
callAdminCluster.returns({});
callAdminCluster.returns({
result: 'deleted'
});
await savedObjectsClient.delete('index-pattern', 'logstash-*');

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(onBeforeWrite);

const [method, args] = callAdminCluster.getCall(0).args;
expect(method).to.be('delete');
expect(args).to.eql({
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, 'delete', {
type: 'doc',
id: 'index-pattern:logstash-*',
refresh: 'wait_for',
index: '.kibana-test'
index: '.kibana-test',
ignore: [404],
});

sinon.assert.calledOnce(onBeforeWrite);
});
});

Expand Down Expand Up @@ -416,24 +416,26 @@ describe('SavedObjectsClient', () => {
it('accepts per_page/page', async () => {
await savedObjectsClient.find({ perPage: 10, page: 6 });

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.notCalled(onBeforeWrite);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
size: 10,
from: 50
}));

const options = callAdminCluster.getCall(0).args[1];
expect(options.size).to.be(10);
expect(options.from).to.be(50);
sinon.assert.notCalled(onBeforeWrite);
});

it('can filter by fields', async () => {
await savedObjectsClient.find({ fields: ['title'] });

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.notCalled(onBeforeWrite);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
_source: [
'*.title', 'type', 'title'
]
}));

const options = callAdminCluster.getCall(0).args[1];
expect(options._source).to.eql([
'*.title', 'type', 'title'
]);
sinon.assert.notCalled(onBeforeWrite);
});
});

Expand Down Expand Up @@ -471,9 +473,11 @@ describe('SavedObjectsClient', () => {
await savedObjectsClient.get('index-pattern', 'logstash-*');

sinon.assert.notCalled(onBeforeWrite);
const [, args] = callAdminCluster.getCall(0).args;
expect(args.id).to.eql('index-pattern:logstash-*');
expect(args.type).to.eql('doc');
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
id: 'index-pattern:logstash-*',
type: 'doc'
}));
});
});

Expand All @@ -486,14 +490,17 @@ describe('SavedObjectsClient', () => {
{ id: 'two', type: 'index-pattern' }
]);

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.notCalled(onBeforeWrite);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
body: {
docs: [
{ _type: 'doc', _id: 'config:one' },
{ _type: 'doc', _id: 'index-pattern:two' }
]
}
}));

const options = callAdminCluster.getCall(0).args[1];
expect(options.body.docs).to.eql([
{ _type: 'doc', _id: 'config:one' },
{ _type: 'doc', _id: 'index-pattern:two' }
]);
sinon.assert.notCalled(onBeforeWrite);
});

it('returns early for empty objects argument', async () => {
Expand All @@ -502,7 +509,7 @@ describe('SavedObjectsClient', () => {
const response = await savedObjectsClient.bulkGet([]);

expect(response.saved_objects).to.have.length(0);
expect(callAdminCluster.notCalled).to.be(true);
sinon.assert.notCalled(callAdminCluster);
sinon.assert.notCalled(onBeforeWrite);
});

Expand Down Expand Up @@ -578,29 +585,29 @@ describe('SavedObjectsClient', () => {
{ version: newVersion - 1 }
);

const esParams = callAdminCluster.getCall(0).args[1];
expect(esParams.version).to.be(newVersion - 1);
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, sinon.match.string, sinon.match({
version: newVersion - 1
}));
});

it('passes the parameters to callAdminCluster', async () => {
await savedObjectsClient.update('index-pattern', 'logstash-*', { title: 'Testing' });

expect(callAdminCluster.calledOnce).to.be(true);
sinon.assert.calledOnce(onBeforeWrite);

const args = callAdminCluster.getCall(0).args;

expect(args[0]).to.be('update');
expect(args[1]).to.eql({
sinon.assert.calledOnce(callAdminCluster);
sinon.assert.calledWithExactly(callAdminCluster, 'update', {
type: 'doc',
id: 'index-pattern:logstash-*',
version: undefined,
body: {
doc: { updated_at: mockTimestamp, 'index-pattern': { title: 'Testing' } }
},
ignore: [404],
refresh: 'wait_for',
index: '.kibana-test'
});

sinon.assert.calledOnce(onBeforeWrite);
});
});

Expand Down
8 changes: 5 additions & 3 deletions src/server/saved_objects/client/lib/__tests__/decorate_es_error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@ describe('savedObjectsClient/decorateEsError', () => {
expect(isForbiddenError(error)).to.be(true);
});

it('makes es.NotFound a SavedObjectsClient/NotFound error', () => {
it('discards es.NotFound errors and returns a generic 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);
const genericError = decorateEsError(error);
expect(genericError).to.not.be(error);
expect(isNotFoundError(error)).to.be(false);
expect(isNotFoundError(genericError)).to.be(true);
});

it('makes es.BadRequest a SavedObjectsClient/BadRequest error', () => {
Expand Down
38 changes: 11 additions & 27 deletions src/server/saved_objects/client/lib/__tests__/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
isNotAuthorizedError,
decorateForbiddenError,
isForbiddenError,
decorateNotFoundError,
createGenericNotFoundError,
isNotFoundError,
decorateConflictError,
isConflictError,
Expand Down Expand Up @@ -145,42 +145,26 @@ describe('savedObjectsClient/errorTypes', () => {
});
});
describe('NotFound error', () => {
describe('decorateNotFoundError', () => {
it('returns original object', () => {
const error = new 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);
decorateNotFoundError(error);
describe('createGenericNotFoundError', () => {
it('makes an error identifiable as a NotFound error', () => {
const error = createGenericNotFoundError();
expect(isNotFoundError(error)).to.be(true);
});

it('adds boom properties', () => {
const error = decorateNotFoundError(new Error());
it('is a boom error, has boom properties', () => {
const error = createGenericNotFoundError();
expect(error).to.have.property('isBoom', true);
expect(error.output).to.be.an('object');
expect(error.output.statusCode).to.be(404);
});

it('preserves boom properties of input', () => {
const error = Boom.forbidden();
decorateNotFoundError(error);
expect(error.output.statusCode).to.be(403);
});

describe('error.output', () => {
it('defaults to message of erorr', () => {
const error = decorateNotFoundError(new Error('foobar'));
expect(error.output.payload).to.have.property('message', 'foobar');
});
it('prefixes message with passed reason', () => {
const error = decorateNotFoundError(new Error('foobar'), 'biz');
expect(error.output.payload).to.have.property('message', 'biz: foobar');
it('Uses "Not Found" message', () => {
const error = createGenericNotFoundError();
expect(error.output.payload).to.have.property('message', 'Not Found');
});
it('sets statusCode to 404', () => {
const error = decorateNotFoundError(new Error('foo'));
const error = createGenericNotFoundError();
expect(error.output).to.have.property('statusCode', 404);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/server/saved_objects/client/lib/decorate_es_error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
decorateBadRequestError,
decorateNotAuthorizedError,
decorateForbiddenError,
decorateNotFoundError,
createGenericNotFoundError,
decorateConflictError,
decorateEsUnavailableError,
decorateGeneralError,
Expand Down Expand Up @@ -51,7 +51,7 @@ export function decorateEsError(error) {
}

if (error instanceof NotFound) {
return decorateNotFoundError(error, reason);
return createGenericNotFoundError();
}

if (error instanceof BadRequest) {
Expand Down
4 changes: 2 additions & 2 deletions src/server/saved_objects/client/lib/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ export function isForbiddenError(error) {

// 404 - Not Found
const CODE_NOT_FOUND = 'SavedObjectsClient/notFound';
export function decorateNotFoundError(error, reason) {
return decorate(error, CODE_NOT_FOUND, 404, reason);
export function createGenericNotFoundError() {
return decorate(Boom.notFound(), CODE_NOT_FOUND, 404);
}
export function isNotFoundError(error) {
return error && error[code] === CODE_NOT_FOUND;
Expand Down
Loading

0 comments on commit 354528f

Please sign in to comment.