From 5935fe3f5f1390e9520e7b3d0c69d4b2164f1c10 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Mon, 4 May 2020 21:07:27 +0200 Subject: [PATCH 1/9] Deserialize bulkCreate response to remove namespace type from id --- .../service/lib/repository.test.js | 39 ++++++++++++++++--- .../saved_objects/service/lib/repository.ts | 36 +++++++---------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 927171438ae996..aecfbbddd7dc90 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -424,9 +424,17 @@ describe('SavedObjectsRepository', () => { const getMockBulkCreateResponse = (objects, namespace) => { return { - items: objects.map(({ type, id }) => ({ + items: objects.map(({ type, id, attributes, references, migrationVersion }) => ({ create: { _id: `${namespace ? `${namespace}:` : ''}${type}:${id}`, + _source: { + [type]: attributes, + type, + namespace, + references, + ...mockTimestampFields, + migrationVersion: migrationVersion || { [type]: '1.1.1' }, + }, ...mockVersionProps, }, })), @@ -474,7 +482,7 @@ describe('SavedObjectsRepository', () => { const expectSuccessResult = obj => ({ ...obj, - migrationVersion: undefined, + migrationVersion: { [obj.type]: '1.1.1' }, version: mockVersion, ...mockTimestampFields, }); @@ -619,13 +627,16 @@ describe('SavedObjectsRepository', () => { }; const bulkCreateError = async (obj, esError, expectedError) => { - const objects = [obj1, obj, obj2]; - const response = getMockBulkCreateResponse(objects); + let response; if (esError) { + response = getMockBulkCreateResponse([obj1, obj, obj2]); response.items[1].create = { error: esError }; + } else { + response = getMockBulkCreateResponse([obj1, obj2]); } callAdminCluster.mockResolvedValue(response); // this._writeToCluster('bulk', ...) + const objects = [obj1, obj, obj2]; const result = await savedObjectsRepository.bulkCreate(objects); expectClusterCalls('bulk'); const objCall = esError ? expectObjArgs(obj) : []; @@ -781,7 +792,7 @@ describe('SavedObjectsRepository', () => { id: 'three', }; const objects = [obj1, obj, obj2]; - const response = getMockBulkCreateResponse(objects); + const response = getMockBulkCreateResponse([obj1, obj2]); callAdminCluster.mockResolvedValue(response); // this._writeToCluster('bulk', ...) const result = await savedObjectsRepository.bulkCreate(objects); expect(callAdminCluster).toHaveBeenCalledTimes(1); @@ -790,6 +801,24 @@ describe('SavedObjectsRepository', () => { }); }); }); + + it(`deserializes the raw ES response into a saved object`, async () => { + // Test for fix to https://github.com/elastic/kibana/issues/65088 where + // we returned raw ID's when an object without an id was created. + const response = getMockBulkCreateResponse([obj1, obj2]); + callAdminCluster.mockResolvedValueOnce(response); // this._writeToCluster('bulk', ...) + + // Bulk create one object with id unspecified, and one with id specified + const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2]); + + // Assert that both raw docs from the ES response are deserialized + expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, response.items[0].create); + expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create); + + // Assert that ID's are deserialized and doesn't include the namespace + // and type. + expect(result.saved_objects.map(so => so.id)).toEqual([obj1.id, obj2.id]); + }); }); describe('#bulkGet', () => { diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index bc8ad2cdb00582..5ae3d91678fa72 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -403,36 +403,26 @@ export class SavedObjectsRepository { return expectedResult.error as any; } - const { requestedId, rawMigratedDoc, esRequestIndex } = expectedResult.value; - const response = bulkResponse.items[esRequestIndex]; const { - error, - _id: responseId, - _seq_no: seqNo, - _primary_term: primaryTerm, - } = Object.values(response)[0] as any; - - const { - _source: { type, [type]: attributes, references = [], namespaces }, - } = rawMigratedDoc; + requestedId, + rawMigratedDoc: { + _source: { type }, + }, + esRequestIndex, + } = expectedResult.value; + const { error, ...rawResponse } = Object.values( + bulkResponse.items[esRequestIndex] + )[0] as any; - const id = requestedId || responseId; if (error) { return { - id, + id: requestedId, type, - error: getBulkOperationError(error, type, id), + error: getBulkOperationError(error, type, requestedId), }; } - return { - id, - type, - ...(namespaces && { namespaces }), - updated_at: time, - version: encodeVersion(seqNo, primaryTerm), - attributes, - references, - }; + + return this._serializer.rawToSavedObject(rawResponse); }), }; } From 9a41cd89c01e7cc71fb5abb5fa42d5a7881aefef Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 5 May 2020 13:00:56 +0200 Subject: [PATCH 2/9] Index operations don't return _source in response --- .../saved_objects/service/lib/repository.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 5ae3d91678fa72..f292c8cb2b6501 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -403,13 +403,7 @@ export class SavedObjectsRepository { return expectedResult.error as any; } - const { - requestedId, - rawMigratedDoc: { - _source: { type }, - }, - esRequestIndex, - } = expectedResult.value; + const { requestedId, rawMigratedDoc, esRequestIndex } = expectedResult.value; const { error, ...rawResponse } = Object.values( bulkResponse.items[esRequestIndex] )[0] as any; @@ -417,12 +411,15 @@ export class SavedObjectsRepository { if (error) { return { id: requestedId, - type, - error: getBulkOperationError(error, type, requestedId), + type: rawMigratedDoc._source.type, + error: getBulkOperationError(error, rawMigratedDoc._source.type, requestedId), }; } - return this._serializer.rawToSavedObject(rawResponse); + // When method == 'index' the bulkResponse doesn't include the indexed + // _source so we return rawMigratedDoc but have to spread rawResponse + // to return the latest _seq_no and _primary_term values from ES. + return this._serializer.rawToSavedObject({ ...rawMigratedDoc, ...rawResponse }); }), }; } From ecdaf68bd77d944dad7d545eaeb2ff09c2df9521 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 5 May 2020 21:52:56 +0200 Subject: [PATCH 3/9] Fix integration tests --- test/api_integration/apis/saved_objects/bulk_create.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/api_integration/apis/saved_objects/bulk_create.js b/test/api_integration/apis/saved_objects/bulk_create.js index 2d77fdf266793a..d51e55859cef5f 100644 --- a/test/api_integration/apis/saved_objects/bulk_create.js +++ b/test/api_integration/apis/saved_objects/bulk_create.js @@ -72,6 +72,9 @@ export default function({ getService }) { attributes: { title: 'A great new dashboard', }, + migrationVersion: { + dashboard: resp.body.saved_objects[1].migrationVersion.dashboard, + }, references: [], }, ], @@ -106,6 +109,9 @@ export default function({ getService }) { title: 'An existing visualization', }, references: [], + migrationVersion: { + visualization: resp.body.saved_objects[0].migrationVersion.visualization, + }, }, { type: 'dashboard', @@ -116,6 +122,9 @@ export default function({ getService }) { title: 'A great new dashboard', }, references: [], + migrationVersion: { + dashboard: resp.body.saved_objects[1].migrationVersion.dashboard, + }, }, ], }); From ccaa1c17dca80b922800110e99847c9992dafdb5 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 7 May 2020 14:16:54 +0200 Subject: [PATCH 4/9] repository: make id generation and seq_no/primary_term spreading more explicit --- .../service/lib/repository.test.js | 34 +++++++++++++++---- .../saved_objects/service/lib/repository.ts | 12 +++++-- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index aecfbbddd7dc90..c45f9c4880e791 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -23,6 +23,7 @@ import { SavedObjectsErrorHelpers } from './errors'; import { SavedObjectsSerializer } from '../../serialization'; import { encodeHitVersion } from '../../version'; import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry'; +import { DocumentMigrator } from '../../migrations/core/document_migrator'; jest.mock('./search_dsl/search_dsl', () => ({ getSearchDsl: jest.fn() })); @@ -115,6 +116,7 @@ describe('SavedObjectsRepository', () => { const createType = type => ({ name: type, mappings: { properties: mappings.properties[type].properties }, + migrations: { '1.1.1': doc => doc }, }); const registry = new SavedObjectTypeRegistry(); @@ -144,6 +146,13 @@ describe('SavedObjectsRepository', () => { namespaceType: 'agnostic', }); + const documentMigrator = new DocumentMigrator({ + typeRegistry: registry, + kibanaVersion: '2.0.0', + log: {}, + validateDoc: jest.fn(), + }); + const getMockGetResponse = ({ type, id, references, namespace }) => ({ // NOTE: Elasticsearch returns more fields (_index, _type) but the SavedObjectsRepository method ignores these found: true, @@ -207,7 +216,7 @@ describe('SavedObjectsRepository', () => { beforeEach(() => { callAdminCluster = jest.fn(); migrator = { - migrateDocument: jest.fn(doc => doc), + migrateDocument: jest.fn().mockImplementation(documentMigrator.migrate), runMigrations: async () => ({ status: 'skipped' }), }; @@ -805,19 +814,29 @@ describe('SavedObjectsRepository', () => { it(`deserializes the raw ES response into a saved object`, async () => { // Test for fix to https://github.com/elastic/kibana/issues/65088 where // we returned raw ID's when an object without an id was created. - const response = getMockBulkCreateResponse([obj1, obj2]); + const namespace = 'myspace'; + const response = getMockBulkCreateResponse([obj1, obj2], namespace); callAdminCluster.mockResolvedValueOnce(response); // this._writeToCluster('bulk', ...) // Bulk create one object with id unspecified, and one with id specified - const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2]); + const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2], { + namespace, + }); // Assert that both raw docs from the ES response are deserialized - expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, response.items[0].create); + expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, { + ...response.items[0].create, + _id: expect.stringMatching(/myspace:config:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/), + }); expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create); - // Assert that ID's are deserialized and doesn't include the namespace - // and type. - expect(result.saved_objects.map(so => so.id)).toEqual([obj1.id, obj2.id]); + // Assert that ID's are deserialized to remove the type and namespace + expect(result.saved_objects[0].id).toEqual( + expect.not.stringMatching(/config|index-pattern|myspace/) + ); + expect(result.saved_objects[1].id).toEqual( + expect.not.stringMatching(/config|index-pattern|myspace/) + ); }); }); @@ -1633,6 +1652,7 @@ describe('SavedObjectsRepository', () => { version: mockVersion, attributes, references, + migrationVersion: { [type]: '1.1.1' }, }); }); }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index f292c8cb2b6501..61027130e0eb73 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -18,6 +18,7 @@ */ import { omit } from 'lodash'; +import uuid from 'uuid'; import { retryCallCluster } from '../../../elasticsearch/retry_call_cluster'; import { APICaller } from '../../../elasticsearch/'; @@ -299,6 +300,8 @@ export class SavedObjectsRepository { const requiresNamespacesCheck = method === 'index' && this._registry.isMultiNamespace(object.type); + if (object.id == null) object.id = uuid.v1(); + return { tag: 'Right' as 'Right', value: { @@ -417,9 +420,12 @@ export class SavedObjectsRepository { } // When method == 'index' the bulkResponse doesn't include the indexed - // _source so we return rawMigratedDoc but have to spread rawResponse - // to return the latest _seq_no and _primary_term values from ES. - return this._serializer.rawToSavedObject({ ...rawMigratedDoc, ...rawResponse }); + // _source so we return rawMigratedDoc but have to spread the latest + // _seq_no and _primary_term values from the rawResponse. + return this._serializer.rawToSavedObject({ + ...rawMigratedDoc, + ...{ _seq_no: rawResponse._seq_no, _primary_term: rawResponse._primary_term }, + }); }), }; } From d20949ba1210b2ea22bc3dfc48c544469b881d65 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 7 May 2020 14:17:53 +0200 Subject: [PATCH 5/9] API Integration test for bulk create without ids --- .../apis/saved_objects/bulk_create.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/api_integration/apis/saved_objects/bulk_create.js b/test/api_integration/apis/saved_objects/bulk_create.js index d51e55859cef5f..a0d2717555150f 100644 --- a/test/api_integration/apis/saved_objects/bulk_create.js +++ b/test/api_integration/apis/saved_objects/bulk_create.js @@ -80,6 +80,18 @@ export default function({ getService }) { ], }); })); + + it('should not return raw id when object id is unspecified', async () => + await supertest + .post(`/api/saved_objects/_bulk_create`) + // eslint-disable-next-line no-unused-vars + .send(BULK_REQUESTS.map(({ id, ...rest }) => rest)) + .expect(200) + .then(resp => { + resp.body.saved_objects.map(({ id }) => + expect(id).not.match(/visualization|dashboard/) + ); + })); }); describe('without kibana index', () => { From 9c2b7433e3529236837c885108d0cc97d2e13169 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 7 May 2020 21:05:12 +0200 Subject: [PATCH 6/9] Fix copy_to_space snapshot --- .../spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts b/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts index d842f07cdb2057..d301d8a3fc4324 100644 --- a/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts +++ b/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts @@ -222,7 +222,6 @@ describe('copySavedObjectsToSpaces', () => { "_maxListeners": undefined, "_read": [Function], "_readableState": ReadableState { - "autoDestroy": false, "awaitDrain": 0, "buffer": BufferList { "head": null, @@ -287,7 +286,6 @@ describe('copySavedObjectsToSpaces', () => { "_maxListeners": undefined, "_read": [Function], "_readableState": ReadableState { - "autoDestroy": false, "awaitDrain": 0, "buffer": BufferList { "head": null, From e12c5427ef0e3140451a1f66babf2e5e80ac6991 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 7 May 2020 22:46:49 +0200 Subject: [PATCH 7/9] Revert "Fix copy_to_space snapshot" This reverts commit 9c2b7433e3529236837c885108d0cc97d2e13169. --- .../spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts b/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts index d301d8a3fc4324..d842f07cdb2057 100644 --- a/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts +++ b/x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.test.ts @@ -222,6 +222,7 @@ describe('copySavedObjectsToSpaces', () => { "_maxListeners": undefined, "_read": [Function], "_readableState": ReadableState { + "autoDestroy": false, "awaitDrain": 0, "buffer": BufferList { "head": null, @@ -286,6 +287,7 @@ describe('copySavedObjectsToSpaces', () => { "_maxListeners": undefined, "_read": [Function], "_readableState": ReadableState { + "autoDestroy": false, "awaitDrain": 0, "buffer": BufferList { "head": null, From 36356ab7f035e082a11492c8d5f7889ce6d2c27b Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 7 May 2020 22:48:54 +0200 Subject: [PATCH 8/9] Move test into returns block --- .../service/lib/repository.test.js | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index c45f9c4880e791..96f7bf1e7823af 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -809,34 +809,34 @@ describe('SavedObjectsRepository', () => { saved_objects: [expectSuccessResult(obj1), expectError(obj), expectSuccessResult(obj2)], }); }); - }); - it(`deserializes the raw ES response into a saved object`, async () => { - // Test for fix to https://github.com/elastic/kibana/issues/65088 where - // we returned raw ID's when an object without an id was created. - const namespace = 'myspace'; - const response = getMockBulkCreateResponse([obj1, obj2], namespace); - callAdminCluster.mockResolvedValueOnce(response); // this._writeToCluster('bulk', ...) + it(`a deserialized saved object`, async () => { + // Test for fix to https://github.com/elastic/kibana/issues/65088 where + // we returned raw ID's when an object without an id was created. + const namespace = 'myspace'; + const response = getMockBulkCreateResponse([obj1, obj2], namespace); + callAdminCluster.mockResolvedValueOnce(response); // this._writeToCluster('bulk', ...) - // Bulk create one object with id unspecified, and one with id specified - const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2], { - namespace, - }); + // Bulk create one object with id unspecified, and one with id specified + const result = await savedObjectsRepository.bulkCreate([{ ...obj1, id: undefined }, obj2], { + namespace, + }); - // Assert that both raw docs from the ES response are deserialized - expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, { - ...response.items[0].create, - _id: expect.stringMatching(/myspace:config:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/), - }); - expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create); + // Assert that both raw docs from the ES response are deserialized + expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, { + ...response.items[0].create, + _id: expect.stringMatching(/myspace:config:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/), + }); + expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create); - // Assert that ID's are deserialized to remove the type and namespace - expect(result.saved_objects[0].id).toEqual( - expect.not.stringMatching(/config|index-pattern|myspace/) - ); - expect(result.saved_objects[1].id).toEqual( - expect.not.stringMatching(/config|index-pattern|myspace/) - ); + // Assert that ID's are deserialized to remove the type and namespace + expect(result.saved_objects[0].id).toEqual( + expect.not.stringMatching(/config|index-pattern|myspace/) + ); + expect(result.saved_objects[1].id).toEqual( + expect.not.stringMatching(/config|index-pattern|myspace/) + ); + }); }); }); From 5b4aa222a78b2ae5709246be0b7c32f3885956f1 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 8 May 2020 10:06:10 +0200 Subject: [PATCH 9/9] repository.test.js stricter regexp matching --- .../server/saved_objects/service/lib/repository.test.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 96f7bf1e7823af..c46fcfbc6dbd74 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -825,17 +825,15 @@ describe('SavedObjectsRepository', () => { // Assert that both raw docs from the ES response are deserialized expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(1, { ...response.items[0].create, - _id: expect.stringMatching(/myspace:config:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}/), + _id: expect.stringMatching(/^myspace:config:[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/), }); expect(serializer.rawToSavedObject).toHaveBeenNthCalledWith(2, response.items[1].create); // Assert that ID's are deserialized to remove the type and namespace expect(result.saved_objects[0].id).toEqual( - expect.not.stringMatching(/config|index-pattern|myspace/) - ); - expect(result.saved_objects[1].id).toEqual( - expect.not.stringMatching(/config|index-pattern|myspace/) + expect.stringMatching(/^[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}$/) ); + expect(result.saved_objects[1].id).toEqual(obj2.id); }); }); });