From ba4ca8979088d0e257fa01fb47bd88587b445549 Mon Sep 17 00:00:00 2001 From: Joe Portner Date: Tue, 28 Jun 2022 14:48:21 -0400 Subject: [PATCH 1/4] Preserve originId when overwriting existing objects If an existing object is being overwritten (via create/bulkCreate), Kibana now preserves its originId field if it has one. This behavior can be overridden by specifying `originId: undefined`. I also added validation to ensure that originId is only set for multi-namespace object types (it would basically have no effect for single-namespace object types, but we shouldn't allow it). --- .../service/lib/preflight_check_for_create.ts | 2 +- .../service/lib/repository.test.ts | 184 +++++++++++++++--- .../saved_objects/service/lib/repository.ts | 31 ++- 3 files changed, 190 insertions(+), 27 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/preflight_check_for_create.ts b/src/core/server/saved_objects/service/lib/preflight_check_for_create.ts index 036a0e417386b1..55eb80dbc48741 100644 --- a/src/core/server/saved_objects/service/lib/preflight_check_for_create.ts +++ b/src/core/server/saved_objects/service/lib/preflight_check_for_create.ts @@ -255,7 +255,7 @@ async function bulkGetObjectsAndAliases( docsToBulkGet.push({ _id: serializer.generateRawId(undefined, type, id), // namespace is intentionally undefined because multi-namespace objects don't have a namespace in their raw ID _index: getIndexForType(type), - _source: ['type', 'namespaces'], + _source: ['type', 'namespaces', 'originId'], }); if (checkAliases) { for (const space of spaces) { diff --git a/src/core/server/saved_objects/service/lib/repository.test.ts b/src/core/server/saved_objects/service/lib/repository.test.ts index d9fa313ca7c0ca..199ad514c28edb 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.ts +++ b/src/core/server/saved_objects/service/lib/repository.test.ts @@ -46,6 +46,7 @@ import { import { ALL_NAMESPACES_STRING } from './utils'; import { loggerMock } from '@kbn/logging-mocks'; import { + SavedObjectsRawDoc, SavedObjectsRawDocSource, SavedObjectsSerializer, SavedObjectUnsanitizedDoc, @@ -431,7 +432,6 @@ describe('SavedObjectsRepository', () => { id: '6.0.0-alpha1', attributes: { title: 'Test One' }, references: [{ name: 'ref_0', type: 'test', id: '1' }], - originId: 'some-origin-id', // only one of the object args has an originId, this is intentional to test both a positive and negative case }; const obj2 = { type: 'index-pattern', @@ -613,6 +613,96 @@ describe('SavedObjectsRepository', () => { ); }); + describe('originId', () => { + it(`returns error if originId is set for non-multi-namespace type`, async () => { + const result = await savedObjectsRepository.bulkCreate([ + { ...obj1, originId: 'some-originId' }, + { ...obj2, type: NAMESPACE_AGNOSTIC_TYPE, originId: 'some-originId' }, + ]); + expect(result.saved_objects).toEqual([ + expect.objectContaining({ id: obj1.id, type: obj1.type, error: expect.anything() }), + expect.objectContaining({ + id: obj2.id, + type: NAMESPACE_AGNOSTIC_TYPE, + error: expect.anything(), + }), + ]); + expect(client.bulk).not.toHaveBeenCalled(); + }); + + it(`defaults to no originId`, async () => { + const objects = [ + { ...obj1, type: MULTI_NAMESPACE_TYPE }, + { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }, + ]; + + await bulkCreateSuccess(objects); + const expected = expect.not.objectContaining({ originId: expect.anything() }); + const body = [expect.any(Object), expected, expect.any(Object), expected]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + + describe('with existing originId', () => { + beforeEach(() => { + mockPreflightCheckForCreate.mockImplementation(({ objects }) => { + const existingDocument = { + _source: { originId: 'existing-originId' }, + } as SavedObjectsRawDoc; + return Promise.resolve( + objects.map(({ type, id }) => ({ type, id, existingDocument })) + ); + }); + }); + + it(`accepts custom originId for multi-namespace type`, async () => { + // The preflight result has `existing-originId`, but that is discarded + const objects = [ + { ...obj1, type: MULTI_NAMESPACE_TYPE, originId: 'some-originId' }, + { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE, originId: 'some-originId' }, + ]; + await bulkCreateSuccess(objects); + const expected = expect.objectContaining({ originId: 'some-originId' }); + const body = [expect.any(Object), expected, expect.any(Object), expected]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + + it(`accepts undefined originId`, async () => { + // The preflight result has `existing-originId`, but that is discarded + const objects = [ + { ...obj1, type: MULTI_NAMESPACE_TYPE, originId: undefined }, + { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE, originId: undefined }, + ]; + await bulkCreateSuccess(objects); + const expected = expect.not.objectContaining({ originId: expect.anything() }); + const body = [expect.any(Object), expected, expect.any(Object), expected]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + + it(`preserves existing originId if originId option is not set`, async () => { + const objects = [ + { ...obj1, type: MULTI_NAMESPACE_TYPE }, + { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }, + ]; + await bulkCreateSuccess(objects); + const expected = expect.objectContaining({ originId: 'existing-originId' }); + const body = [expect.any(Object), expected, expect.any(Object), expected]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + }); + }); + it(`adds namespace to request body for any types that are single-namespace`, async () => { await bulkCreateSuccess([obj1, obj2], { namespace }); const expected = expect.objectContaining({ namespace }); @@ -2116,7 +2206,6 @@ describe('SavedObjectsRepository', () => { const attributes = { title: 'Logstash' }; const id = 'logstash-*'; const namespace = 'foo-namespace'; - const originId = 'some-origin-id'; const references = [ { name: 'ref_0', @@ -2226,24 +2315,73 @@ describe('SavedObjectsRepository', () => { await test(null); }); - it(`defaults to no originId`, async () => { - await createSuccess(type, attributes, { id }); - expect(client.create).toHaveBeenCalledWith( - expect.objectContaining({ - body: expect.not.objectContaining({ originId: expect.anything() }), - }), - expect.anything() - ); - }); + describe('originId', () => { + for (const objType of [type, NAMESPACE_AGNOSTIC_TYPE]) { + it(`throws an error if originId is set for non-multi-namespace type`, async () => { + await expect( + savedObjectsRepository.create(objType, attributes, { originId: 'some-originId' }) + ).rejects.toThrowError( + createBadRequestError('"originId" can only be set for multi-namespace object types') + ); + }); + } - it(`accepts custom originId`, async () => { - await createSuccess(type, attributes, { id, originId }); - expect(client.create).toHaveBeenCalledWith( - expect.objectContaining({ - body: expect.objectContaining({ originId }), - }), - expect.anything() - ); + for (const objType of [MULTI_NAMESPACE_TYPE, MULTI_NAMESPACE_ISOLATED_TYPE]) { + it(`${objType} defaults to no originId`, async () => { + await createSuccess(objType, attributes, { id }); + expect(client.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.not.objectContaining({ originId: expect.anything() }), + }), + expect.anything() + ); + }); + + describe(`${objType} with existing originId`, () => { + beforeEach(() => { + mockPreflightCheckForCreate.mockImplementation(({ objects }) => { + const existingDocument = { + _source: { originId: 'existing-originId' }, + } as SavedObjectsRawDoc; + return Promise.resolve( + objects.map(({ type, id }) => ({ type, id, existingDocument })) + ); + }); + }); + + it(`accepts custom originId for multi-namespace type`, async () => { + // The preflight result has `existing-originId`, but that is discarded + await createSuccess(objType, attributes, { id, originId: 'some-originId' }); + expect(client.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.objectContaining({ originId: 'some-originId' }), + }), + expect.anything() + ); + }); + + it(`accepts undefined originId`, async () => { + // The preflight result has `existing-originId`, but that is discarded + await createSuccess(objType, attributes, { id, originId: undefined }); + expect(client.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.not.objectContaining({ originId: expect.anything() }), + }), + expect.anything() + ); + }); + + it(`preserves existing originId if originId option is not set`, async () => { + await createSuccess(objType, attributes, { id }); + expect(client.create).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.objectContaining({ originId: 'existing-originId' }), + }), + expect.anything() + ); + }); + }); + } }); it(`defaults to a refresh setting of wait_for`, async () => { @@ -2638,22 +2776,20 @@ describe('SavedObjectsRepository', () => { describe('returns', () => { it(`formats the ES response`, async () => { - const result = await createSuccess(type, attributes, { + const result = await createSuccess(MULTI_NAMESPACE_TYPE, attributes, { id, namespace, references, - originId, }); expect(result).toEqual({ - type, + type: MULTI_NAMESPACE_TYPE, id, - originId, ...mockTimestampFields, version: mockVersion, attributes, references, namespaces: [namespace ?? 'default'], - migrationVersion: { [type]: '1.1.1' }, + migrationVersion: { [MULTI_NAMESPACE_TYPE]: '1.1.1' }, coreMigrationVersion: KIBANA_VERSION, }); }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 2827b7f5ffea6e..679d3fbdb498f9 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -315,7 +315,6 @@ export class SavedObjectsRepository { overwrite = false, references = [], refresh = DEFAULT_REFRESH_SETTING, - originId, initialNamespaces, version, } = options; @@ -323,6 +322,7 @@ export class SavedObjectsRepository { const namespace = normalizeNamespace(options.namespace); this.validateInitialNamespaces(type, initialNamespaces); + this.validateOriginId(type, options); if (!this._allowedTypes.includes(type)) { throw SavedObjectsErrorHelpers.createUnsupportedTypeError(type); @@ -331,6 +331,7 @@ export class SavedObjectsRepository { const time = getCurrentTime(); let savedObjectNamespace: string | undefined; let savedObjectNamespaces: string[] | undefined; + let existingOriginId: string | undefined; if (this._registry.isSingleNamespace(type)) { savedObjectNamespace = initialNamespaces @@ -354,11 +355,17 @@ export class SavedObjectsRepository { } savedObjectNamespaces = initialNamespaces || getSavedObjectNamespaces(namespace, existingDocument); + existingOriginId = existingDocument?._source?.originId; } else { savedObjectNamespaces = initialNamespaces || getSavedObjectNamespaces(namespace); } } + // 1. If the originId has been *explicitly set* in the options (defined or undefined), respect that. + // 2. Otherwise, preserve the originId of the existing object that is being overwritten, if any. + const originId = Object.keys(options).includes('originId') + ? options.originId + : existingOriginId; const migrated = this._migrator.migrateDocument({ id, type, @@ -442,6 +449,7 @@ export class SavedObjectsRepository { } else { try { this.validateInitialNamespaces(type, initialNamespaces); + this.validateOriginId(type, object); } catch (e) { error = e; } @@ -499,6 +507,7 @@ export class SavedObjectsRepository { let savedObjectNamespace: string | undefined; let savedObjectNamespaces: string[] | undefined; + let existingOriginId: string | undefined; let versionProperties; const { preflightCheckIndex, @@ -525,6 +534,7 @@ export class SavedObjectsRepository { savedObjectNamespaces = initialNamespaces || getSavedObjectNamespaces(namespace, existingDocument); versionProperties = getExpectedVersionProperties(version); + existingOriginId = existingDocument?._source?.originId; } else { if (this._registry.isSingleNamespace(object.type)) { savedObjectNamespace = initialNamespaces @@ -536,6 +546,11 @@ export class SavedObjectsRepository { versionProperties = getExpectedVersionProperties(version); } + // 1. If the originId has been *explicitly set* for the object (defined or undefined), respect that. + // 2. Otherwise, preserve the originId of the existing object that is being overwritten, if any. + const originId = Object.keys(object).includes('originId') + ? object.originId + : existingOriginId; const migrated = this._migrator.migrateDocument({ id: object.id, type: object.type, @@ -546,7 +561,7 @@ export class SavedObjectsRepository { ...(savedObjectNamespaces && { namespaces: savedObjectNamespaces }), updated_at: time, references: object.references || [], - originId: object.originId, + originId, }) as SavedObjectSanitizedDoc; /** @@ -2335,6 +2350,18 @@ export class SavedObjectsRepository { throw SavedObjectsErrorHelpers.createBadRequestError(error.message); } } + + /** This is used when objects are created. */ + private validateOriginId(type: string, objectOrOptions: { originId?: string }) { + if ( + Object.keys(objectOrOptions).includes('originId') && + !this._registry.isMultiNamespace(type) + ) { + throw SavedObjectsErrorHelpers.createBadRequestError( + '"originId" can only be set for multi-namespace object types' + ); + } + } } /** From 6b5c8270becd79a4ed5d1865586bcce53e663c90 Mon Sep 17 00:00:00 2001 From: Joe Portner Date: Tue, 28 Jun 2022 16:58:29 -0400 Subject: [PATCH 2/4] Fix import integration test --- .../import/lib/create_saved_objects.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/core/server/saved_objects/import/lib/create_saved_objects.ts b/src/core/server/saved_objects/import/lib/create_saved_objects.ts index d6c7cbe934b51b..8839d01ade2bf9 100644 --- a/src/core/server/saved_objects/import/lib/create_saved_objects.ts +++ b/src/core/server/saved_objects/import/lib/create_saved_objects.ts @@ -56,8 +56,8 @@ export const createSavedObjects = async ({ new Map>() ); - // filter out the 'version' field of each object, if it exists - const objectsToCreate = filteredObjects.map(({ version, ...object }) => { + // filter out the 'version' field of each object, if it exists, and set the originId appropriately + const objectsToCreate = filteredObjects.map(({ version, originId, ...object }) => { // use the import ID map to ensure that each reference is being created with the correct ID const references = object.references?.map((reference) => { const { type, id } = reference; @@ -72,15 +72,18 @@ export const createSavedObjects = async ({ const importStateValue = importStateMap.get(`${object.type}:${object.id}`); if (importStateValue?.destinationId) { objectIdMap.set(`${object.type}:${importStateValue.destinationId}`, object); - const originId = importStateValue.omitOriginId ? undefined : object.originId ?? object.id; return { ...object, id: importStateValue.destinationId, - originId, ...(references && { references }), + // Do not set originId, not even to undefined, if omitOriginId is true. + // When omitOriginId is true, we are trying to create a brand new object without setting the originId at all. + // Semantically, setting `originId: undefined` is used to clear out an existing object's originId when overwriting it + // (and if you attempt to do that on a non-multi-namespace object type, it will result in a 400 Bad Request error). + ...(!importStateValue.omitOriginId && { originId: originId ?? object.id }), }; } - return { ...object, ...(references && { references }) }; + return { ...object, originId, ...(references && { references }) }; }); const resolvableErrors = ['conflict', 'ambiguous_conflict', 'missing_references']; From 8e8fe84554dacbf1797d89533d2c4445001fa20b Mon Sep 17 00:00:00 2001 From: Joe Portner Date: Tue, 28 Jun 2022 17:05:56 -0400 Subject: [PATCH 3/4] Third time's the charm --- .../server/saved_objects/import/lib/create_saved_objects.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/server/saved_objects/import/lib/create_saved_objects.ts b/src/core/server/saved_objects/import/lib/create_saved_objects.ts index 8839d01ade2bf9..f3260e4968d00c 100644 --- a/src/core/server/saved_objects/import/lib/create_saved_objects.ts +++ b/src/core/server/saved_objects/import/lib/create_saved_objects.ts @@ -83,7 +83,7 @@ export const createSavedObjects = async ({ ...(!importStateValue.omitOriginId && { originId: originId ?? object.id }), }; } - return { ...object, originId, ...(references && { references }) }; + return { ...object, ...(references && { references }), ...(originId && { originId }) }; }); const resolvableErrors = ['conflict', 'ambiguous_conflict', 'missing_references']; From f31c2ad142fdbb7db80b32dda15d1c490046d2eb Mon Sep 17 00:00:00 2001 From: Joe Portner Date: Tue, 28 Jun 2022 21:55:43 -0400 Subject: [PATCH 4/4] Fix jest_integration tests --- .../saved_objects/routes/integration_tests/import.test.ts | 2 -- .../routes/integration_tests/resolve_import_errors.test.ts | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/core/server/saved_objects/routes/integration_tests/import.test.ts b/src/core/server/saved_objects/routes/integration_tests/import.test.ts index d36b8b07f78f0f..2f6f2063667666 100644 --- a/src/core/server/saved_objects/routes/integration_tests/import.test.ts +++ b/src/core/server/saved_objects/routes/integration_tests/import.test.ts @@ -544,13 +544,11 @@ describe(`POST ${URL}`, () => { type: 'visualization', id: 'new-id-1', references: [{ name: 'ref_0', type: 'index-pattern', id: 'my-pattern' }], - originId: undefined, }), expect.objectContaining({ type: 'dashboard', id: 'new-id-2', references: [{ name: 'ref_0', type: 'visualization', id: 'new-id-1' }], - originId: undefined, }), ], expect.any(Object) // options diff --git a/src/core/server/saved_objects/routes/integration_tests/resolve_import_errors.test.ts b/src/core/server/saved_objects/routes/integration_tests/resolve_import_errors.test.ts index 956da9aa135246..dfc69edaff4208 100644 --- a/src/core/server/saved_objects/routes/integration_tests/resolve_import_errors.test.ts +++ b/src/core/server/saved_objects/routes/integration_tests/resolve_import_errors.test.ts @@ -396,13 +396,11 @@ describe(`POST ${URL}`, () => { type: 'visualization', id: 'new-id-1', references: [{ name: 'ref_0', type: 'index-pattern', id: 'existing' }], - originId: undefined, }), expect.objectContaining({ type: 'dashboard', id: 'new-id-2', references: [{ name: 'ref_0', type: 'visualization', id: 'new-id-1' }], - originId: undefined, }), ], expect.any(Object) // options