SOR: Add attr override option for update and bulkUpdate (#183267)

## Summary Fix https://github.com/elastic/kibana/issues/183112 Add a new `mergeAttributes` option for both `update` and `bulkUpdate`, which, when set to `false`, allows to perform a "full" update (fully replacing the current attributes of the document) instead of a "partial" one (merging the attributes). Technically, ES doesn't really support it, so it was only made possible due to the "client-side" update we implemented for ZDT / SO version BWC.
2025-04-24 09:48:58 -04:00 · 2024-05-15 11:02:32 +02:00 · 2024-05-15 11:02:32 +02:00 · 61e408c963
commit 61e408c963
parent 2786fb1a3a
8 changed files with 319 additions and 45 deletions
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.test.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.test.ts
@ -132,6 +132,22 @@ describe('#bulkUpdate', () => {
    const originId = 'some-origin-id';
    const namespace = 'foo-namespace';

+    const getBulkIndexEntry = (
+      method: string,
+      { type, id }: TypeIdTuple,
+      _index = expect.any(String),
+      getId: (type: string, id: string) => string = () => expect.any(String),
+      overrides: Record<string, unknown> = {}
+    ) => {
+      return {
+        [method]: {
+          _index,
+          _id: getId(type, id),
+          ...overrides,
+        },
+      };
+    };
+
    // bulk index calls have two objects for each source -- the action, and the source
    const expectClientCallArgsAction = (
      objects: TypeIdTuple[],
@ -148,14 +164,8 @@ describe('#bulkUpdate', () => {
      }
    ) => {
      const body = [];
-      for (const { type, id } of objects) {
-        body.push({
-          [method]: {
-            _index,
-            _id: getId(type, id),
-            ...overrides,
-          },
-        });
+      for (const object of objects) {
+        body.push(getBulkIndexEntry(method, object, _index, getId, overrides));
        body.push(expect.any(Object));
      }
      expect(client.bulk).toHaveBeenCalledWith(
@ -220,6 +230,55 @@ describe('#bulkUpdate', () => {
        expectClientCallArgsAction([obj1, _obj2], { method: 'index' });
      });

+      it('should use the ES bulk action with the merged attributes when mergeAttributes is not false', async () => {
+        const _obj1 = { ...obj1, attributes: { foo: 'bar' } };
+        const _obj2 = { ...obj2, attributes: { hello: 'dolly' }, mergeAttributes: true };
+        await bulkUpdateSuccess(client, repository, registry, [_obj1, _obj2]);
+
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expect(client.bulk).toHaveBeenCalledWith(
+          expect.objectContaining({
+            body: [
+              getBulkIndexEntry('index', _obj1),
+              expect.objectContaining({
+                [obj1.type]: {
+                  title: 'Testing',
+                  foo: 'bar',
+                },
+              }),
+              getBulkIndexEntry('index', _obj2),
+              expect.objectContaining({
+                [obj2.type]: {
+                  title: 'Testing',
+                  hello: 'dolly',
+                },
+              }),
+            ],
+          }),
+          expect.any(Object)
+        );
+      });
+
+      it('should use the ES bulk action only with the provided attributes when mergeAttributes is false', async () => {
+        const _obj1 = { ...obj1, attributes: { hello: 'dolly' }, mergeAttributes: false };
+        await bulkUpdateSuccess(client, repository, registry, [_obj1]);
+
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expect(client.bulk).toHaveBeenCalledWith(
+          expect.objectContaining({
+            body: [
+              getBulkIndexEntry('index', _obj1),
+              expect.objectContaining({
+                [obj1.type]: {
+                  hello: 'dolly',
+                },
+              }),
+            ],
+          }),
+          expect.any(Object)
+        );
+      });
+
      it(`doesnt call Elasticsearch if there are no valid objects to update`, async () => {
        const objects = [obj1, obj2].map((x) => ({ ...x, type: 'unknownType' }));
        await repository.bulkUpdate(objects);
@ -507,6 +566,7 @@ describe('#bulkUpdate', () => {
        await bulkUpdateError(obj, true, expectedErrorResult);
      });
    });
+
    describe('migration', () => {
      it('migrates the fetched documents from Mget', async () => {
        const modifiedObj2 = { ...obj2, coreMigrationVersion: '8.0.0' };
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/bulk_update.ts
@ -58,6 +58,7 @@ type ExpectedBulkGetResult = Either<
    objectNamespace?: string;
    esRequestIndex: number;
    migrationVersionCompatibility?: 'raw' | 'compatible';
+    mergeAttributes: boolean;
  }
 >;

@ -89,7 +90,15 @@ export const performBulkUpdate = async <T>(

  let bulkGetRequestIndexCounter = 0;
  const expectedBulkGetResults = objects.map<ExpectedBulkGetResult>((object) => {
-    const { type, id, attributes, references, version, namespace: objectNamespace } = object;
+    const {
+      type,
+      id,
+      attributes,
+      references,
+      version,
+      namespace: objectNamespace,
+      mergeAttributes = true,
+    } = object;
    let error: DecoratedError | undefined;

    if (!allowedTypes.includes(type)) {
@ -122,6 +131,7 @@ export const performBulkUpdate = async <T>(
      objectNamespace,
      esRequestIndex: bulkGetRequestIndexCounter++,
      migrationVersionCompatibility,
+      mergeAttributes,
    });
  });

@ -205,8 +215,15 @@ export const performBulkUpdate = async <T>(
        return expectedBulkGetResult;
      }

-      const { esRequestIndex, id, type, version, documentToSave, objectNamespace } =
-        expectedBulkGetResult.value;
+      const {
+        esRequestIndex,
+        id,
+        type,
+        version,
+        documentToSave,
+        objectNamespace,
+        mergeAttributes,
+      } = expectedBulkGetResult.value;

      let namespaces: string[] | undefined;
      const versionProperties = getExpectedVersionProperties(version);
@ -261,18 +278,23 @@ export const performBulkUpdate = async <T>(
      }

      const typeDefinition = registry.getType(type)!;
-      const updatedAttributes = mergeForUpdate({
-        targetAttributes: {
-          ...(migrated!.attributes as Record<string, unknown>),
-        },
-        updatedAttributes: await encryptionHelper.optionallyEncryptAttributes(
-          type,
-          id,
-          objectNamespace || namespace,
-          documentToSave[type]
-        ),
-        typeMappings: typeDefinition.mappings,
-      });
+
+      const encryptedUpdatedAttributes = await encryptionHelper.optionallyEncryptAttributes(
+        type,
+        id,
+        objectNamespace || namespace,
+        documentToSave[type]
+      );
+
+      const updatedAttributes = mergeAttributes
+        ? mergeForUpdate({
+            targetAttributes: {
+              ...(migrated!.attributes as Record<string, unknown>),
+            },
+            updatedAttributes: encryptedUpdatedAttributes,
+            typeMappings: typeDefinition.mappings,
+          })
+        : encryptedUpdatedAttributes;

      const migratedUpdatedSavedObjectDoc = migrationHelper.migrateInputDocument({
        ...migrated!,
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.test.ts
@ -178,6 +178,55 @@ describe('#update', () => {
        expect(client.index).toHaveBeenCalledTimes(1);
      });

+      it(`should use the ES index action with the merged attributes when mergeAttributes is not false`, async () => {
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+
+        await updateSuccess(client, repository, registry, NAMESPACE_AGNOSTIC_TYPE, id, {
+          foo: 'bar',
+        });
+
+        expect(client.index).toHaveBeenCalledTimes(1);
+        expect(client.index).toHaveBeenCalledWith(
+          expect.objectContaining({
+            body: expect.objectContaining({
+              globalType: {
+                foo: 'bar',
+                title: 'Testing',
+              },
+            }),
+          }),
+          expect.any(Object)
+        );
+      });
+
+      it(`should use the ES index action only with the provided attributes when mergeAttributes is false`, async () => {
+        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
+
+        await updateSuccess(
+          client,
+          repository,
+          registry,
+          NAMESPACE_AGNOSTIC_TYPE,
+          id,
+          {
+            foo: 'bar',
+          },
+          { mergeAttributes: false }
+        );
+
+        expect(client.index).toHaveBeenCalledTimes(1);
+        expect(client.index).toHaveBeenCalledWith(
+          expect.objectContaining({
+            body: expect.objectContaining({
+              globalType: {
+                foo: 'bar',
+              },
+            }),
+          }),
+          expect.any(Object)
+        );
+      });
+
      it(`should check for alias conflicts if a new multi-namespace object before create action would be created then create action to create the object`, async () => {
        migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
        await updateSuccess(
@ -218,6 +267,7 @@ describe('#update', () => {
        };
        await test(references);
      });
+
      it(`accepts custom references array 2`, async () => {
        const test = async (references: SavedObjectReference[]) => {
          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
@ -232,6 +282,7 @@ describe('#update', () => {
        };
        await test([{ type: 'foo', id: '42', name: 'some ref' }]);
      });
+
      it(`accepts custom references array 3`, async () => {
        const test = async (references: SavedObjectReference[]) => {
          migrator.migrateDocument.mockImplementationOnce((doc) => ({ ...doc, migrated: true }));
--- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.ts
@ -246,18 +246,24 @@ export const executeUpdate = async <T>(
    // at this point, we already know 1. the document exists 2. we're not doing an upsert
    // therefor we can safely process with the "standard" update sequence.

-    const updatedAttributes = mergeForUpdate({
-      targetAttributes: {
-        ...(migrated!.attributes as Record<string, unknown>),
-      },
-      updatedAttributes: await encryptionHelper.optionallyEncryptAttributes(
-        type,
-        id,
-        namespace,
-        attributes
-      ),
-      typeMappings: typeDefinition.mappings,
-    });
+    const mergeAttributes = options.mergeAttributes ?? true;
+    const encryptedUpdatedAttributes = await encryptionHelper.optionallyEncryptAttributes(
+      type,
+      id,
+      namespace,
+      attributes
+    );
+
+    const updatedAttributes = mergeAttributes
+      ? mergeForUpdate({
+          targetAttributes: {
+            ...(migrated!.attributes as Record<string, unknown>),
+          },
+          updatedAttributes: encryptedUpdatedAttributes,
+          typeMappings: typeDefinition.mappings,
+        })
+      : encryptedUpdatedAttributes;
+
    const migratedUpdatedSavedObjectDoc = migrationHelper.migrateInputDocument({
      ...migrated!,
      id,
--- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_update.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_update.ts
@ -29,6 +29,13 @@ export interface SavedObjectsBulkUpdateObject<T = unknown>
   * Note: the default namespace's string representation is `'default'`, and its ID representation is `undefined`.
   **/
  namespace?: string;
+  /**
+   * By default, update will merge the provided attributes with the ones present on the document
+   * (performing a standard partial update). Setting this option to `false` will change the behavior, performing
+   * a "full" update instead, where the provided attributes will fully replace the existing ones.
+   * Defaults to `true`.
+   */
+  mergeAttributes?: boolean;
 }

 /**
--- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/update.ts
+++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/update.ts
@ -34,6 +34,13 @@ export interface SavedObjectsUpdateOptions<Attributes = unknown> extends SavedOb
  retryOnConflict?: number;
  /** {@link SavedObjectsRawDocParseOptions.migrationVersionCompatibility} */
  migrationVersionCompatibility?: 'compatible' | 'raw';
+  /**
+   * By default, update will merge the provided attributes with the ones present on the document
+   * (performing a standard partial update). Setting this option to `false` will change the behavior, performing
+   * a "full" update instead, where the provided attributes will fully replace the existing ones.
+   * Defaults to `true`.
+   */
+  mergeAttributes?: boolean;
 }

 /**
--- a/src/core/server/integration_tests/saved_objects/service/lib/bulk_update.test.ts
+++ b/src/core/server/integration_tests/saved_objects/service/lib/bulk_update.test.ts
@ -30,7 +30,12 @@ describe('SOR - bulk_update API', () => {
    esServer = await startElasticsearch();
  });

-  const getType = (version: 'v1' | 'v2'): SavedObjectsType => {
+  afterAll(async () => {
+    await esServer?.stop();
+    await delay(10);
+  });
+
+  const getCrossVersionType = (version: 'v1' | 'v2'): SavedObjectsType => {
    const versionMap: SavedObjectsModelVersionMap = {
      1: {
        changes: [],
@ -117,16 +122,29 @@ describe('SOR - bulk_update API', () => {
      modelVersions: versionOtherMap,
    };
  };
-  afterAll(async () => {
-    await esServer?.stop();
-    await delay(10);
-  });
+
+  const getFullUpdateType = (): SavedObjectsType => {
+    return {
+      name: 'update-test-type',
+      hidden: false,
+      namespaceType: 'single',
+      mappings: {
+        dynamic: false,
+        properties: {},
+      },
+      management: {
+        importableAndExportable: true,
+      },
+      switchToModelVersionAt: '8.10.0',
+      modelVersions: {},
+    };
+  };

  const setup = async () => {
    const { runMigrations: runMigrationV1, savedObjectsRepository: repositoryV1 } =
      await getKibanaMigratorTestKit({
        ...getBaseMigratorParams(),
-        types: [getType('v1'), getOtherType('v1')],
+        types: [getCrossVersionType('v1'), getOtherType('v1')],
      });
    await runMigrationV1();

@ -136,7 +154,7 @@ describe('SOR - bulk_update API', () => {
      client: esClient,
    } = await getKibanaMigratorTestKit({
      ...getBaseMigratorParams(),
-      types: [getType('v2'), getOtherType('v2')],
+      types: [getCrossVersionType('v2'), getOtherType('v2'), getFullUpdateType()],
    });
    await runMigrationV2();

@ -225,6 +243,49 @@ describe('SOR - bulk_update API', () => {
    );
  });

+  it('supports update with attributes override', async () => {
+    const { repositoryV2: repository } = await setup();
+
+    await repository.create('update-test-type', { foo: 'bar' }, { id: 'my-id' });
+
+    let docs = await repository.bulkGet([{ type: 'update-test-type', id: 'my-id' }]);
+    const [doc] = docs.saved_objects;
+
+    expect(doc.attributes).toEqual({
+      foo: 'bar',
+    });
+
+    await repository.bulkUpdate([
+      { type: 'update-test-type', id: doc.id, attributes: { hello: 'dolly' } },
+    ]);
+
+    docs = await repository.bulkGet([{ type: 'update-test-type', id: 'my-id' }]);
+    const [doc1] = docs.saved_objects;
+
+    expect(doc1.attributes).toEqual({
+      foo: 'bar',
+      hello: 'dolly',
+    });
+
+    await repository.bulkUpdate([
+      {
+        type: 'update-test-type',
+        id: doc1.id,
+        attributes: {
+          over: '9000',
+        },
+        mergeAttributes: false,
+      },
+    ]);
+
+    docs = await repository.bulkGet([{ type: 'update-test-type', id: 'my-id' }]);
+    const [doc2] = docs.saved_objects;
+
+    expect(doc2.attributes).toEqual({
+      over: '9000',
+    });
+  });
+
  const fetchDoc = async (client: ElasticsearchClient, type: string, id: string) => {
    return await client.get({
      index: '.kibana',
--- a/src/core/server/integration_tests/saved_objects/service/lib/update.test.ts
+++ b/src/core/server/integration_tests/saved_objects/service/lib/update.test.ts
@ -30,7 +30,7 @@ describe('SOR - update API', () => {
    esServer = await startElasticsearch();
  });

-  const getType = (version: 'v1' | 'v2'): SavedObjectsType => {
+  const getCrossVersionType = (version: 'v1' | 'v2'): SavedObjectsType => {
    const versionMap: SavedObjectsModelVersionMap = {
      1: {
        changes: [],
@ -74,6 +74,23 @@ describe('SOR - update API', () => {
    };
  };

+  const getFullUpdateType = (): SavedObjectsType => {
+    return {
+      name: 'update-test-type',
+      hidden: false,
+      namespaceType: 'single',
+      mappings: {
+        dynamic: false,
+        properties: {},
+      },
+      management: {
+        importableAndExportable: true,
+      },
+      switchToModelVersionAt: '8.10.0',
+      modelVersions: {},
+    };
+  };
+
  afterAll(async () => {
    await esServer?.stop();
    await delay(10);
@ -83,7 +100,7 @@ describe('SOR - update API', () => {
    const { runMigrations: runMigrationV1, savedObjectsRepository: repositoryV1 } =
      await getKibanaMigratorTestKit({
        ...getBaseMigratorParams(),
-        types: [getType('v1')],
+        types: [getCrossVersionType('v1'), getFullUpdateType()],
      });
    await runMigrationV1();

@ -93,7 +110,7 @@ describe('SOR - update API', () => {
      client: esClient,
    } = await getKibanaMigratorTestKit({
      ...getBaseMigratorParams(),
-      types: [getType('v2')],
+      types: [getCrossVersionType('v2'), getFullUpdateType()],
    });
    await runMigrationV2();

@ -145,6 +162,49 @@ describe('SOR - update API', () => {
    );
  });

+  it('supports update with attributes override', async () => {
+    const { repositoryV2: repository } = await setup();
+
+    await repository.create('update-test-type', { foo: 'bar' }, { id: 'my-id' });
+
+    let document = await repository.get('update-test-type', 'my-id');
+
+    expect(document.attributes).toEqual({
+      foo: 'bar',
+    });
+
+    await repository.update(
+      'update-test-type',
+      'my-id',
+      {
+        hello: 'dolly',
+      },
+      { mergeAttributes: true }
+    );
+
+    document = await repository.get('update-test-type', 'my-id');
+
+    expect(document.attributes).toEqual({
+      foo: 'bar',
+      hello: 'dolly',
+    });
+
+    await repository.update(
+      'update-test-type',
+      'my-id',
+      {
+        over: '9000',
+      },
+      { mergeAttributes: false }
+    );
+
+    document = await repository.get('update-test-type', 'my-id');
+
+    expect(document.attributes).toEqual({
+      over: '9000',
+    });
+  });
+
  const fetchDoc = async (client: ElasticsearchClient, type: string, id: string) => {
    return await client.get({
      index: '.kibana',