Preserve originId when overwriting existing objects (#135358)

src/core/server/saved_objects/import/lib/create_saved_objects.ts

@@ -56,8 +56,8 @@ export const createSavedObjects = async <T>({
     new Map<string, SavedObject<T>>()
   );
 
-  // filter out the 'version' field of each object, if it exists
+  // filter out the 'version' field of each object, if it exists, and set the originId appropriately
-  const objectsToCreate = filteredObjects.map(({ version, ...object }) => {
+  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 <T>({
     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, ...(references && { references }), ...(originId && { originId }) };
   });
 
   const resolvableErrors = ['conflict', 'ambiguous_conflict', 'missing_references'];

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

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

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) {

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 () => {
+      describe('originId', () => {
-        await createSuccess(type, attributes, { id });
+        for (const objType of [type, NAMESPACE_AGNOSTIC_TYPE]) {
-        expect(client.create).toHaveBeenCalledWith(
+          it(`throws an error if originId is set for non-multi-namespace type`, async () => {
-          expect.objectContaining({
+            await expect(
-            body: expect.not.objectContaining({ originId: expect.anything() }),
+              savedObjectsRepository.create(objType, attributes, { originId: 'some-originId' })
-          }),
+            ).rejects.toThrowError(
-          expect.anything()
+              createBadRequestError('"originId" can only be set for multi-namespace object types')
-        );
+            );
-      });
+          });
+        }
 
-      it(`accepts custom originId`, async () => {
+        for (const objType of [MULTI_NAMESPACE_TYPE, MULTI_NAMESPACE_ISOLATED_TYPE]) {
-        await createSuccess(type, attributes, { id, originId });
+          it(`${objType} defaults to no originId`, async () => {
-        expect(client.create).toHaveBeenCalledWith(
+            await createSuccess(objType, attributes, { id });
-          expect.objectContaining({
+            expect(client.create).toHaveBeenCalledWith(
-            body: expect.objectContaining({ originId }),
+              expect.objectContaining({
-          }),
+                body: expect.not.objectContaining({ originId: expect.anything() }),
-          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,
         });
       });

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<T>;
 
       /**
@@ -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'
+      );
+    }
+  }
 }
 
 /**