[SOR] update: don't deep merge flattened fields (#168986)

Fix #168960

Utilize the type's mappings when performing the merge-for-update
operation to restore the behavior ES has with `flattened` fields.

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/update.ts

@@ -83,7 +83,7 @@ export const executeUpdate = async <T>(
     validation: validationHelper,
   } = helpers;
   const { securityExtension } = extensions;
-
+  const typeDefinition = registry.getType(type)!;
   const {
     version,
     references,
@@ -246,10 +246,18 @@ 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(
-      { ...migrated!.attributes },
-      await encryptionHelper.optionallyEncryptAttributes(type, id, namespace, attributes)
-    );
+    const updatedAttributes = mergeForUpdate({
+      targetAttributes: {
+        ...migrated!.attributes,
+      },
+      updatedAttributes: await encryptionHelper.optionallyEncryptAttributes(
+        type,
+        id,
+        namespace,
+        attributes
+      ),
+      typeMappings: typeDefinition.mappings,
+    });
     const migratedUpdatedSavedObjectDoc = migrationHelper.migrateInputDocument({
       ...migrated!,
       id,

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/merge_for_update.test.ts

@@ -6,27 +6,50 @@
  * Side Public License, v 1.
  */
 
+import type { SavedObjectsTypeMappingDefinition } from '@kbn/core-saved-objects-server';
 import { mergeForUpdate } from './merge_for_update';
 
+const defaultMappings: SavedObjectsTypeMappingDefinition = {
+  properties: {},
+};
+
 describe('mergeForUpdate', () => {
   it('merges top level properties', () => {
-    expect(mergeForUpdate({ foo: 'bar', hello: 'dolly' }, { baz: 42 })).toEqual({
+    expect(
+      mergeForUpdate({
+        targetAttributes: { foo: 'bar', hello: 'dolly' },
+        updatedAttributes: { baz: 42 },
+        typeMappings: defaultMappings,
+      })
+    ).toEqual({
       foo: 'bar',
       hello: 'dolly',
       baz: 42,
     });
   });
 
   it('overrides top level properties', () => {
-    expect(mergeForUpdate({ foo: 'bar', hello: 'dolly' }, { baz: 42, foo: '9000' })).toEqual({
+    expect(
+      mergeForUpdate({
+        targetAttributes: { foo: 'bar', hello: 'dolly' },
+        updatedAttributes: { baz: 42, foo: '9000' },
+        typeMappings: defaultMappings,
+      })
+    ).toEqual({
       foo: '9000',
       hello: 'dolly',
       baz: 42,
     });
   });
 
   it('ignores undefined top level properties', () => {
-    expect(mergeForUpdate({ foo: 'bar', hello: 'dolly' }, { baz: 42, foo: undefined })).toEqual({
+    expect(
+      mergeForUpdate({
+        targetAttributes: { foo: 'bar', hello: 'dolly' },
+        updatedAttributes: { baz: 42, foo: undefined },
+        typeMappings: defaultMappings,
+      })
+    ).toEqual({
       foo: 'bar',
       hello: 'dolly',
       baz: 42,
@@ -35,7 +58,11 @@ describe('mergeForUpdate', () => {
 
   it('merges nested properties', () => {
     expect(
-      mergeForUpdate({ nested: { foo: 'bar', hello: 'dolly' } }, { nested: { baz: 42 } })
+      mergeForUpdate({
+        targetAttributes: { nested: { foo: 'bar', hello: 'dolly' } },
+        updatedAttributes: { nested: { baz: 42 } },
+        typeMappings: defaultMappings,
+      })
     ).toEqual({
       nested: {
         foo: 'bar',
@@ -47,10 +74,11 @@ describe('mergeForUpdate', () => {
 
   it('overrides nested properties', () => {
     expect(
-      mergeForUpdate(
-        { nested: { foo: 'bar', hello: 'dolly' } },
-        { nested: { baz: 42, foo: '9000' } }
-      )
+      mergeForUpdate({
+        targetAttributes: { nested: { foo: 'bar', hello: 'dolly' } },
+        updatedAttributes: { nested: { baz: 42, foo: '9000' } },
+        typeMappings: defaultMappings,
+      })
     ).toEqual({
       nested: {
         foo: '9000',
@@ -62,10 +90,11 @@ describe('mergeForUpdate', () => {
 
   it('ignores undefined nested properties', () => {
     expect(
-      mergeForUpdate(
-        { nested: { foo: 'bar', hello: 'dolly' } },
-        { nested: { baz: 42, foo: undefined } }
-      )
+      mergeForUpdate({
+        targetAttributes: { nested: { foo: 'bar', hello: 'dolly' } },
+        updatedAttributes: { nested: { baz: 42, foo: undefined } },
+        typeMappings: defaultMappings,
+      })
     ).toEqual({
       nested: {
         foo: 'bar',
@@ -77,10 +106,17 @@ describe('mergeForUpdate', () => {
 
   it('functions with mixed levels of properties', () => {
     expect(
-      mergeForUpdate(
-        { rootPropA: 'A', nested: { foo: 'bar', hello: 'dolly', deep: { deeper: 'we need' } } },
-        { rootPropB: 'B', nested: { baz: 42, foo: '9000', deep: { deeper: 'we are' } } }
-      )
+      mergeForUpdate({
+        targetAttributes: {
+          rootPropA: 'A',
+          nested: { foo: 'bar', hello: 'dolly', deep: { deeper: 'we need' } },
+        },
+        updatedAttributes: {
+          rootPropB: 'B',
+          nested: { baz: 42, foo: '9000', deep: { deeper: 'we are' } },
+        },
+        typeMappings: defaultMappings,
+      })
     ).toEqual({
       rootPropA: 'A',
       rootPropB: 'B',
@@ -94,4 +130,45 @@ describe('mergeForUpdate', () => {
       },
     });
   });
+
+  describe('with flattened fields', () => {
+    const mappingsWithFlattened: SavedObjectsTypeMappingDefinition = {
+      properties: {
+        flattened: {
+          type: 'flattened',
+        },
+        nested: {
+          properties: {
+            deepFlat: {
+              type: 'flattened',
+            },
+          },
+        },
+      },
+    };
+
+    it('replaces top level flattened properties', () => {
+      expect(
+        mergeForUpdate({
+          targetAttributes: { flattened: { before: 42 }, notFlattened: { before: 42 } },
+          updatedAttributes: { flattened: { after: 9000 }, notFlattened: { after: 9000 } },
+          typeMappings: mappingsWithFlattened,
+        })
+      ).toEqual({ flattened: { after: 9000 }, notFlattened: { before: 42, after: 9000 } });
+    });
+
+    it('replaces nested flattened properties', () => {
+      expect(
+        mergeForUpdate({
+          targetAttributes: { nested: { deepFlat: { before: 42 }, notFlattened: { before: 42 } } },
+          updatedAttributes: {
+            nested: { deepFlat: { after: 9000 }, notFlattened: { after: 9000 } },
+          },
+          typeMappings: mappingsWithFlattened,
+        })
+      ).toEqual({
+        nested: { deepFlat: { after: 9000 }, notFlattened: { before: 42, after: 9000 } },
+      });
+    });
+  });
 });

packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/merge_for_update.ts

@@ -8,22 +8,59 @@
 
 import { isPlainObject } from 'lodash';
 import { set } from '@kbn/safer-lodash-set';
+import type { MappingProperty as EsMappingProperty } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
+import type {
+  SavedObjectsTypeMappingDefinition,
+  SavedObjectsFieldMapping,
+} from '@kbn/core-saved-objects-server';
 
-export const mergeForUpdate = (
-  targetAttributes: Record<string, any>,
-  updatedAttributes: any
-): Record<string, any> => {
-  return recursiveMerge(targetAttributes, updatedAttributes, []);
+type MaybeMappings = SavedObjectsFieldMapping | EsMappingProperty | undefined;
+
+export const mergeForUpdate = ({
+  targetAttributes,
+  updatedAttributes,
+  typeMappings,
+}: {
+  targetAttributes: Record<string, any>;
+  updatedAttributes: any;
+  typeMappings: SavedObjectsTypeMappingDefinition;
+}): Record<string, any> => {
+  const rootMappings: SavedObjectsFieldMapping = {
+    properties: typeMappings.properties,
+  };
+  return recursiveMerge(targetAttributes, updatedAttributes, [], rootMappings);
 };
 
-const recursiveMerge = (target: Record<string, any>, value: any, keys: string[] = []) => {
-  if (isPlainObject(value) && Object.keys(value).length > 0) {
+const recursiveMerge = (
+  target: Record<string, any>,
+  value: any,
+  keys: string[],
+  mappings: MaybeMappings
+) => {
+  if (shouldRecursiveMerge(value, mappings)) {
     for (const [subKey, subVal] of Object.entries(value)) {
-      recursiveMerge(target, subVal, [...keys, subKey]);
+      recursiveMerge(target, subVal, [...keys, subKey], getFieldMapping(mappings, subKey));
     }
   } else if (keys.length > 0 && value !== undefined) {
     set(target, keys, value);
   }
 
   return target;
 };
+
+const getFieldMapping = (parentMapping: MaybeMappings, fieldName: string): MaybeMappings => {
+  if (parentMapping && 'properties' in parentMapping) {
+    return parentMapping.properties?.[fieldName];
+  }
+  return undefined;
+};
+
+const shouldRecursiveMerge = (value: any, mappings: MaybeMappings): boolean => {
+  if (mappings && 'type' in mappings && mappings.type === 'flattened') {
+    return false;
+  }
+  if (isPlainObject(value) && Object.keys(value).length > 0) {
+    return true;
+  }
+  return false;
+};