From 2e7d9f7693bb4966c1ccfdd9c376ac7f84e5d9a3 Mon Sep 17 00:00:00 2001 From: Tyler Smalley Date: Thu, 13 Jul 2017 15:08:19 -0400 Subject: [PATCH 1/2] Remove entire compatibility layer and updates id generation When creating an id, we will always prefix the type to either an id which was provided or one we generate. This alows us to simplify all actions for managing documents. Signed-off-by: Tyler Smalley --- .../client/__tests__/saved_objects_client.js | 228 +++++++++--------- .../client/lib/create_id_query.js | 45 ---- .../client/lib/handle_es_error.js | 11 - src/server/saved_objects/client/lib/index.js | 3 +- .../client/saved_objects_client.js | 63 +++-- 5 files changed, 141 insertions(+), 209 deletions(-) delete mode 100644 src/server/saved_objects/client/lib/create_id_query.js diff --git a/src/server/saved_objects/client/__tests__/saved_objects_client.js b/src/server/saved_objects/client/__tests__/saved_objects_client.js index 5b72c409349e7..8b9852468a5a1 100644 --- a/src/server/saved_objects/client/__tests__/saved_objects_client.js +++ b/src/server/saved_objects/client/__tests__/saved_objects_client.js @@ -1,7 +1,6 @@ import expect from 'expect.js'; import sinon from 'sinon'; import { SavedObjectsClient } from '../saved_objects_client'; -import { createIdQuery } from '../lib/create_id_query'; describe('SavedObjectsClient', () => { const sandbox = sinon.sandbox.create(); @@ -13,32 +12,41 @@ describe('SavedObjectsClient', () => { total: 3, hits: [{ _index: '.kibana', - _type: 'index-pattern', - _id: 'logstash-*', + _type: 'doc', + _id: 'index-pattern:logstash-*', _score: 1, _source: { - title: 'logstash-*', - timeFieldName: '@timestamp', - notExpandable: true + type: 'index-pattern', + 'index-pattern': { + title: 'logstash-*', + timeFieldName: '@timestamp', + notExpandable: true + } } }, { _index: '.kibana', - _type: 'config', - _id: '6.0.0-alpha1', + _type: 'doc', + _id: 'config:6.0.0-alpha1', _score: 1, _source: { - buildNum: 8467, - defaultIndex: 'logstash-*' + type: 'config', + config: { + buildNum: 8467, + defaultIndex: 'logstash-*' + } } }, { _index: '.kibana', - _type: 'index-pattern', - _id: 'stocks-*', + _type: 'doc', + _id: 'index-pattern:stocks-*', _score: 1, _source: { - title: 'stocks-*', - timeFieldName: '@timestamp', - notExpandable: true + type: 'index-pattern', + 'index-pattern': { + title: 'stocks-*', + timeFieldName: '@timestamp', + notExpandable: true + } } }] } @@ -69,9 +77,15 @@ describe('SavedObjectsClient', () => { describe('#create', () => { - it('formats Elasticsearch response', async () => { - callAdminCluster.returns({ _type: 'index-pattern', _id: 'logstash-*', _version: 2 }); + beforeEach(() => { + callAdminCluster.returns(Promise.resolve({ + _type: 'doc', + _id: 'index-pattern:logstash-*', + _version: 2 + })); + }); + it('formats Elasticsearch response', async () => { const response = await savedObjectsClient.create('index-pattern', { title: 'Logstash' }); @@ -87,8 +101,6 @@ describe('SavedObjectsClient', () => { }); it('should use ES index action', async () => { - callAdminCluster.returns({ _type: 'index-pattern', _id: 'logstash-*', _version: 2 }); - await savedObjectsClient.create('index-pattern', { id: 'logstash-*', title: 'Logstash' @@ -101,8 +113,6 @@ describe('SavedObjectsClient', () => { }); it('should use create action if ID defined and overwrite=false', async () => { - callAdminCluster.returns({ _type: 'index-pattern', _id: 'logstash-*', _version: 2 }); - await savedObjectsClient.create('index-pattern', { title: 'Logstash' }, { @@ -116,8 +126,6 @@ describe('SavedObjectsClient', () => { }); it('allows for id to be provided', async () => { - callAdminCluster.returns({ _type: 'index-pattern', _id: 'logstash-*', _version: 2 }); - await savedObjectsClient.create('index-pattern', { title: 'Logstash' }, { id: 'logstash-*' }); @@ -127,6 +135,17 @@ describe('SavedObjectsClient', () => { const args = callAdminCluster.getCall(0).args; expect(args[1].id).to.be('index-pattern:logstash-*'); }); + + it('self-generates an ID', async () => { + await savedObjectsClient.create('index-pattern', { + title: 'Logstash' + }); + + expect(callAdminCluster.calledOnce).to.be(true); + + 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}/); + }); }); describe('#bulkCreate', () => { @@ -173,16 +192,16 @@ describe('SavedObjectsClient', () => { errors: false, items: [{ create: { - _type: 'config', - _id: 'one', + _type: 'doc', + _id: 'config:one', error: { reason: 'type[config] missing' } } }, { create: { - _type: 'index-pattern', - _id: 'two', + _type: 'doc', + _id: 'index-pattern:two', _version: 2 } }] @@ -215,14 +234,14 @@ describe('SavedObjectsClient', () => { errors: false, items: [{ create: { - _type: 'config', - _id: 'one', + _type: 'doc', + _id: 'config:one', _version: 2 } }, { create: { - _type: 'index-pattern', - _id: 'two', + _type: 'doc', + _id: 'index-pattern:two', _version: 2 } }] @@ -252,17 +271,16 @@ describe('SavedObjectsClient', () => { }); describe('#delete', () => { - it('throws notFound when ES is unable to find the document', (done) => { - callAdminCluster.returns(Promise.resolve({ - deleted: 0 - })); + it('throws notFound when ES is unable to find the document', async () => { + callAdminCluster.returns(Promise.resolve({ found: false })); + + try { + await savedObjectsClient.delete('index-pattern', 'logstash-*'); + expect().fail('should throw error'); + } catch(e) { + expect(e.output.statusCode).to.eql(404); + } - savedObjectsClient.delete('index-pattern', 'logstash-*').then(() => { - done('failed'); - }).catch(e => { - expect(e.output.statusCode).to.be(404); - done(); - }); }); it('passes the parameters to callAdminCluster', async () => { @@ -270,10 +288,11 @@ describe('SavedObjectsClient', () => { expect(callAdminCluster.calledOnce).to.be(true); - const args = callAdminCluster.getCall(0).args; - expect(args[0]).to.be('deleteByQuery'); - expect(args[1]).to.eql({ - body: createIdQuery({ type: 'index-pattern', id: 'logstash-*' }), + const [method, args] = callAdminCluster.getCall(0).args; + expect(method).to.be('delete'); + expect(args).to.eql({ + type: 'doc', + id: 'index-pattern:logstash-*', refresh: 'wait_for', index: '.kibana-test' }); @@ -289,12 +308,13 @@ describe('SavedObjectsClient', () => { expect(response.total).to.be(count); expect(response.saved_objects).to.have.length(count); + docs.hits.hits.forEach((doc, i) => { expect(response.saved_objects[i]).to.eql({ - id: doc._id, - type: doc._type, + id: doc._id.replace(/(index-pattern|config)\:/, ''), + type: doc._source.type, version: doc._version, - attributes: doc._source + attributes: doc._source[doc._source.type] }); }); }); @@ -396,17 +416,14 @@ describe('SavedObjectsClient', () => { describe('#get', () => { it('formats Elasticsearch response', async () => { callAdminCluster.returns(Promise.resolve({ - hits: { - hits: [ - { - _id: 'logstash-*', - _type: 'index-pattern', - _version: 2, - _source: { - title: 'Testing' - } - } - ] + _id: 'index-pattern:logstash-*', + _type: 'doc', + _version: 2, + _source: { + type: 'index-pattern', + 'index-pattern': { + title: 'Testing' + } } })); @@ -420,10 +437,19 @@ describe('SavedObjectsClient', () => { } }); }); + + it('prepends type to the id', async () => { + callAdminCluster.returns(Promise.resolve({})); + await savedObjectsClient.get('index-pattern', 'logstash-*'); + + const [, args] = callAdminCluster.getCall(0).args; + expect(args.id).to.eql('index-pattern:logstash-*'); + expect(args.type).to.eql('doc'); + }); }); describe('#bulkGet', () => { - it('accepts an array of mixed type and ids', async () => { + it('accepts a array of mixed type and ids', async () => { await savedObjectsClient.bulkGet([ { id: 'one', type: 'config' }, { id: 'two', type: 'index-pattern' } @@ -432,11 +458,9 @@ describe('SavedObjectsClient', () => { expect(callAdminCluster.calledOnce).to.be(true); const options = callAdminCluster.getCall(0).args[1]; - expect(options.body).to.eql([ - {}, - createIdQuery({ type: 'config', id: 'one' }), - {}, - createIdQuery({ type: 'index-pattern', id: 'two' }) + expect(options.body.docs).to.eql([ + { _type: 'doc', _id: 'config:one' }, + { _type: 'doc', _id: 'index-pattern:two' } ]); }); @@ -449,38 +473,35 @@ describe('SavedObjectsClient', () => { it('reports error on missed objects', async () => { callAdminCluster.returns(Promise.resolve({ - responses: [ - { - hits: { - hits: [ - { - _id: 'good', - _type: 'doc', - _version: 2, - _source: { - type: 'config', - config: { - title: 'Test' - } - } - } - ] - } - } - ] + docs:[{ + _type: 'doc', + _id: 'config:good', + found: true, + _version: 2, + _source: { config: { title: 'Test' } } + }, { + _type: 'doc', + _id: 'config:bad', + found: false + }] })); const { saved_objects: savedObjects } = await savedObjectsClient.bulkGet( [{ id: 'good', type: 'config' }, { id: 'bad', type: 'config' }] ); - expect(savedObjects).to.have.length(1); + expect(savedObjects).to.have.length(2); expect(savedObjects[0]).to.eql({ id: 'good', type: 'config', version: 2, attributes: { title: 'Test' } }); + expect(savedObjects[1]).to.eql({ + id: 'bad', + type: 'config', + error: { statusCode: 404, message: 'Not found' } + }); }); }); @@ -491,12 +512,9 @@ describe('SavedObjectsClient', () => { const version = 2; const attributes = { title: 'Testing' }; - callAdminCluster.withArgs('search', sinon.match.any).returns(Promise.resolve({ - hits: { hits: [ { _id: 'index-pattern:logstash-*' } ] } - })); - callAdminCluster.withArgs('update', sinon.match.any).returns(Promise.resolve({ - _id: id, - _type: type, + callAdminCluster.returns(Promise.resolve({ + _id: `${type}:${id}`, + _type: 'doc', _version: version, result: 'updated' })); @@ -511,10 +529,6 @@ describe('SavedObjectsClient', () => { }); it('accepts version', async () => { - callAdminCluster.withArgs('search', sinon.match.any).returns(Promise.resolve({ - hits: { hits: [ { _id: 'index-pattern:logstash-*' } ] } - })); - await savedObjectsClient.update( 'index-pattern', 'logstash-*', @@ -522,20 +536,16 @@ describe('SavedObjectsClient', () => { { version: 1 } ); - const esParams = callAdminCluster.getCall(1).args[1]; + const esParams = callAdminCluster.getCall(0).args[1]; expect(esParams.version).to.be(1); }); it('passes the parameters to callAdminCluster', async () => { - callAdminCluster.withArgs('search', sinon.match.any).returns(Promise.resolve({ - hits: { hits: [ { _id: 'index-pattern:logstash-*' } ] } - })); - await savedObjectsClient.update('index-pattern', 'logstash-*', { title: 'Testing' }); - expect(callAdminCluster.calledTwice).to.be(true); + expect(callAdminCluster.calledOnce).to.be(true); - const args = callAdminCluster.getCall(1).args; + const args = callAdminCluster.getCall(0).args; expect(args[0]).to.be('update'); expect(args[1]).to.eql({ @@ -547,19 +557,5 @@ describe('SavedObjectsClient', () => { index: '.kibana-test' }); }); - - it('throws 404 if document does not exist', async () => { - callAdminCluster.withArgs('search', sinon.match.any).returns(Promise.resolve({ - hits: { hits: [] } - })); - - try { - await savedObjectsClient.update('index-pattern', 'logstash-*', { title: 'Testing' }); - expect().fail('should have throw'); - } catch (e) { - expect(callAdminCluster.calledOnce).to.be(true); - expect(e.message).to.eql('Not Found'); - } - }); }); }); diff --git a/src/server/saved_objects/client/lib/create_id_query.js b/src/server/saved_objects/client/lib/create_id_query.js deleted file mode 100644 index 359adb39ccb64..0000000000000 --- a/src/server/saved_objects/client/lib/create_id_query.js +++ /dev/null @@ -1,45 +0,0 @@ -/** - * Finds a document by either its v5 or v6 format - * - * @param type The documents type - * @param id The documents id or legacy id -**/ -export function createIdQuery({ type, id }) { - return { - version: true, - size: 1, - query: { - bool: { - should: [ - // v5 document - { - bool: { - must: [ - { term: { _id: id } }, - { term: { _type: type } } - ] - } - }, - // migrated v5 document - { - bool: { - must: [ - { term: { _id: `${type}:${id}` } }, - { term: { type: type } } - ] - } - }, - // v6 document - { - bool: { - must: [ - { term: { _id: id } }, - { term: { type: type } } - ] - } - }, - ] - } - } - }; -} diff --git a/src/server/saved_objects/client/lib/handle_es_error.js b/src/server/saved_objects/client/lib/handle_es_error.js index 725d954a1ac61..87bdcf44841b0 100644 --- a/src/server/saved_objects/client/lib/handle_es_error.js +++ b/src/server/saved_objects/client/lib/handle_es_error.js @@ -13,13 +13,6 @@ const { BadRequest } = elasticsearch.errors; -export function isSingleTypeError(error) { - if (!error) return; - - return error.type === 'illegal_argument_exception' && - error.reason.match(/the final mapping would have more than 1 type/); -} - export function handleEsError(error) { if (!(error instanceof Error)) { throw new Error('Expected an instance of Error'); @@ -50,10 +43,6 @@ export function handleEsError(error) { } if (error instanceof BadRequest) { - if (isSingleTypeError(get(error, 'body.error'))) { - details.type = 'is_single_type'; - } - throw Boom.badRequest(reason, details); } diff --git a/src/server/saved_objects/client/lib/index.js b/src/server/saved_objects/client/lib/index.js index 8644b2bacdce7..62fd63889aab8 100644 --- a/src/server/saved_objects/client/lib/index.js +++ b/src/server/saved_objects/client/lib/index.js @@ -1,5 +1,4 @@ export { createFindQuery } from './create_find_query'; -export { createIdQuery } from './create_id_query'; -export { handleEsError, isSingleTypeError } from './handle_es_error'; +export { handleEsError } from './handle_es_error'; export { normalizeEsDoc } from './normalize_es_doc'; export { includedFields } from './included_fields'; diff --git a/src/server/saved_objects/client/saved_objects_client.js b/src/server/saved_objects/client/saved_objects_client.js index 8afbd11593dee..1189d9a4cf178 100644 --- a/src/server/saved_objects/client/saved_objects_client.js +++ b/src/server/saved_objects/client/saved_objects_client.js @@ -1,9 +1,9 @@ import Boom from 'boom'; +import uuid from 'uuid'; import { get } from 'lodash'; import { createFindQuery, - createIdQuery, handleEsError, normalizeEsDoc, includedFields @@ -32,7 +32,7 @@ export class SavedObjectsClient { const method = options.id && !options.overwrite ? 'create' : 'index'; const response = await this._withKibanaIndex(method, { type: V6_TYPE, - id: options.id ? `${type}:${options.id}` : undefined, + id: this.generateEsId(type, options.id), body: { type, [type]: attributes @@ -57,7 +57,7 @@ export class SavedObjectsClient { acc.push({ [method]: { _type: V6_TYPE, - _id: object.id ? `${object.type}:${object.id}` : undefined + _id: this.generateEsId(object.type, object.id) } }); acc.push(Object.assign({}, @@ -94,12 +94,13 @@ export class SavedObjectsClient { * @returns {promise} */ async delete(type, id) { - const response = await this._withKibanaIndex('deleteByQuery', { - body: createIdQuery({ type, id }), + const response = await this._withKibanaIndex('delete', { + type: V6_TYPE, + id: `${type}:${id}`, refresh: 'wait_for' }); - if (get(response, 'deleted') === 0) { + if (get(response, 'found') === false) { throw Boom.notFound(); } } @@ -166,24 +167,22 @@ export class SavedObjectsClient { return { saved_objects: [] }; } - const docs = objects.reduce((acc, { type, id }) => { - return [...acc, {}, createIdQuery({ type, id })]; - }, []); + const docs = objects.map(doc => { + return { _type: V6_TYPE, _id: `${doc.type}:${doc.id}` }; + }); - const response = await this._withKibanaIndex('msearch', { body: docs }); - const responses = get(response, 'responses', []); + const response = await this._withKibanaIndex('mget', { body: { docs } }) + .then(resp => get(resp, 'docs', [])); return { - saved_objects: responses.map((r, i) => { - const [hit] = get(r, 'hits.hits', []); - - if (!hit) { + saved_objects: response.map((r, i) => { + if (r.found === false) { return Object.assign({}, objects[i], { error: { statusCode: 404, message: 'Not found' } }); } - return normalizeEsDoc(hit, objects[i]); + return normalizeEsDoc(r, objects[i]); }) }; } @@ -196,14 +195,12 @@ export class SavedObjectsClient { * @returns {promise} - { id, type, version, attributes } */ async get(type, id) { - const response = await this._withKibanaIndex('search', { body: createIdQuery({ type, id }) }); - const [hit] = get(response, 'hits.hits', []); - - if (!hit) { - throw Boom.notFound(); - } + const response = await this._withKibanaIndex('get', { + type: V6_TYPE, + id: `${type}:${id}` + }); - return normalizeEsDoc(hit); + return normalizeEsDoc(response); } /** @@ -216,27 +213,19 @@ export class SavedObjectsClient { * @returns {promise} */ async update(type, id, attributes, options = {}) { - // handles documents with an _id of `${type}:${id}` - const docResponse = await this._withKibanaIndex('search', { body: createIdQuery({ type, id }) }); - const [hit] = get(docResponse, 'hits.hits', []); - - if (!hit) { - throw Boom.notFound(); - } - const response = await this._withKibanaIndex('update', { - id: hit._id, type: V6_TYPE, + id: `${type}:${id}`, version: options.version, - refresh: 'wait_for', body: { doc: { [type]: attributes } - } + }, + refresh: 'wait_for' }); - return normalizeEsDoc(response, { id, type, attributes }); + return normalizeEsDoc(response, { type, id, attributes }); } async _withKibanaIndex(method, params) { @@ -249,4 +238,8 @@ export class SavedObjectsClient { throw handleEsError(err); } } + + generateEsId(type, id) { + return `${type}:${id || uuid.v1()}`; + } } From 986b39bc0dbfefbb28fdaabbb1b2d492c1a5b161 Mon Sep 17 00:00:00 2001 From: Tyler Smalley Date: Thu, 13 Jul 2017 15:43:55 -0400 Subject: [PATCH 2/2] Use generateEsId and prefix with _ Signed-off-by: Tyler Smalley --- .../saved_objects/client/saved_objects_client.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/server/saved_objects/client/saved_objects_client.js b/src/server/saved_objects/client/saved_objects_client.js index 1189d9a4cf178..0de5b959418ac 100644 --- a/src/server/saved_objects/client/saved_objects_client.js +++ b/src/server/saved_objects/client/saved_objects_client.js @@ -32,7 +32,7 @@ export class SavedObjectsClient { const method = options.id && !options.overwrite ? 'create' : 'index'; const response = await this._withKibanaIndex(method, { type: V6_TYPE, - id: this.generateEsId(type, options.id), + id: this._generateEsId(type, options.id), body: { type, [type]: attributes @@ -57,7 +57,7 @@ export class SavedObjectsClient { acc.push({ [method]: { _type: V6_TYPE, - _id: this.generateEsId(object.type, object.id) + _id: this._generateEsId(object.type, object.id) } }); acc.push(Object.assign({}, @@ -96,7 +96,7 @@ export class SavedObjectsClient { async delete(type, id) { const response = await this._withKibanaIndex('delete', { type: V6_TYPE, - id: `${type}:${id}`, + id: this._generateEsId(type, id), refresh: 'wait_for' }); @@ -168,7 +168,7 @@ export class SavedObjectsClient { } const docs = objects.map(doc => { - return { _type: V6_TYPE, _id: `${doc.type}:${doc.id}` }; + return { _type: V6_TYPE, _id: this._generateEsId(doc.type, doc.id) }; }); const response = await this._withKibanaIndex('mget', { body: { docs } }) @@ -197,7 +197,7 @@ export class SavedObjectsClient { async get(type, id) { const response = await this._withKibanaIndex('get', { type: V6_TYPE, - id: `${type}:${id}` + id: this._generateEsId(type, id) }); return normalizeEsDoc(response); @@ -215,7 +215,7 @@ export class SavedObjectsClient { async update(type, id, attributes, options = {}) { const response = await this._withKibanaIndex('update', { type: V6_TYPE, - id: `${type}:${id}`, + id: this._generateEsId(type, id), version: options.version, body: { doc: { @@ -239,7 +239,7 @@ export class SavedObjectsClient { } } - generateEsId(type, id) { + _generateEsId(type, id) { return `${type}:${id || uuid.v1()}`; } }