Address PR feedback

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts

@@ -8,37 +8,35 @@
 
 import * as Either from 'fp-ts/lib/Either';
 import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
-import { checkTargetMappings } from './check_target_mappings';
-import { diffMappings, getUpdatedHashes } from '../core/build_active_mappings';
+import type { SavedObjectsMappingProperties } from '@kbn/core-saved-objects-server';
+import {
+  checkTargetMappings,
+  type ComparedMappingsChanged,
+  type ComparedMappingsMatch,
+} from './check_target_mappings';
+import { getUpdatedHashes } from '../core/build_active_mappings';
 
 jest.mock('../core/build_active_mappings');
 
-const diffMappingsMock = diffMappings as jest.MockedFn<typeof diffMappings>;
 const getUpdatedHashesMock = getUpdatedHashes as jest.MockedFn<typeof getUpdatedHashes>;
 
-const actualMappings: IndexMapping = {
-  properties: {
-    type1: { type: 'integer' },
-    type2: { type: 'integer' },
-  },
-  _meta: {
-    migrationMappingPropertyHashes: {
-      type1: 'type1OldHash',
-      type2: 'type2OldHash',
-    },
-  },
+const indexTypes = ['type1', 'type2'];
+
+const properties: SavedObjectsMappingProperties = {
+  type1: { type: 'long' },
+  type2: { type: 'long' },
+};
+
+const migrationMappingPropertyHashes = {
+  type1: 'type1Hash',
+  type2: 'type2Hash',
 };
 
 const expectedMappings: IndexMapping = {
-  properties: {
-    type1: { type: 'long' },
-    type2: { type: 'long' },
-  },
+  properties,
+  dynamic: 'strict',
   _meta: {
-    migrationMappingPropertyHashes: {
-      type1: 'type1NewHash',
-      type2: 'type2NewHash',
-    },
+    migrationMappingPropertyHashes,
   },
 };
 
@@ -47,49 +45,99 @@ describe('checkTargetMappings', () => {
     jest.clearAllMocks();
   });
 
-  it('returns match=false if source mappings are not defined', async () => {
-    const task = checkTargetMappings({
-      expectedMappings,
-    });
-
-    const result = await task();
-    expect(diffMappings).not.toHaveBeenCalled();
-    expect(result).toEqual(Either.right({ match: false }));
-  });
+  describe('when actual mappings are incomplete', () => {
+    it("returns 'actual_mappings_incomplete' if actual mappings are not defined", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+      });
 
-  it('calls diffMappings() with the source and target mappings', async () => {
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
     });
 
-    await task();
-    expect(diffMappings).toHaveBeenCalledTimes(1);
-    expect(diffMappings).toHaveBeenCalledWith(actualMappings, expectedMappings);
-  });
-
-  it('returns match=true if diffMappings() match', async () => {
-    diffMappingsMock.mockReturnValueOnce(undefined);
+    it("returns 'actual_mappings_incomplete' if actual mappings do not define _meta", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: 'strict',
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
+    });
 
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+    it("returns 'actual_mappings_incomplete' if actual mappings do not define migrationMappingPropertyHashes", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: 'strict',
+          _meta: {},
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
     });
 
-    const result = await task();
-    expect(result).toEqual(Either.right({ match: true }));
+    it("returns 'actual_mappings_incomplete' if actual mappings define a different value for 'dynamic' property", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: false,
+          _meta: { migrationMappingPropertyHashes },
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
+    });
   });
 
-  it('returns match=false if diffMappings() finds differences', async () => {
-    diffMappingsMock.mockReturnValueOnce({ changedProp: 'type1' });
-    getUpdatedHashesMock.mockReturnValueOnce(['type1', 'type2']);
-
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+  describe('when actual mappings are complete', () => {
+    describe('and mappings do not match', () => {
+      it('returns the lists of changed root fields and types', async () => {
+        const task = checkTargetMappings({
+          indexTypes,
+          expectedMappings,
+          actualMappings: expectedMappings,
+        });
+
+        getUpdatedHashesMock.mockReturnValueOnce(['type1', 'type2', 'someRootField']);
+
+        const result = await task();
+        const expected: ComparedMappingsChanged = {
+          type: 'compared_mappings_changed' as const,
+          updatedRootFields: ['someRootField'],
+          updatedTypes: ['type1', 'type2'],
+        };
+        expect(result).toEqual(Either.left(expected));
+      });
     });
 
-    const result = await task();
-    expect(result).toEqual(Either.right({ match: false, updatedHashes: ['type1', 'type2'] }));
+    describe('and mappings match', () => {
+      it('returns a compared_mappings_match response', async () => {
+        const task = checkTargetMappings({
+          indexTypes,
+          expectedMappings,
+          actualMappings: expectedMappings,
+        });
+
+        getUpdatedHashesMock.mockReturnValueOnce([]);
+
+        const result = await task();
+        const expected: ComparedMappingsMatch = {
+          type: 'compared_mappings_match' as const,
+        };
+        expect(result).toEqual(Either.right(expected));
+      });
+    });
   });
 });

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts

@@ -9,30 +9,61 @@ import * as Either from 'fp-ts/lib/Either';
 import * as TaskEither from 'fp-ts/lib/TaskEither';
 
 import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
-import { diffMappings, getUpdatedHashes } from '../core/build_active_mappings';
+import { getUpdatedHashes } from '../core/build_active_mappings';
 
 /** @internal */
 export interface CheckTargetMappingsParams {
+  indexTypes: string[];
   actualMappings?: IndexMapping;
   expectedMappings: IndexMapping;
 }
 
 /** @internal */
-export interface TargetMappingsCompareResult {
-  match: boolean;
-  updatedHashes?: string[];
+export interface ComparedMappingsMatch {
+  type: 'compared_mappings_match';
+}
+
+export interface ActualMappingsIncomplete {
+  type: 'actual_mappings_incomplete';
+}
+
+export interface ComparedMappingsChanged {
+  type: 'compared_mappings_changed';
+  updatedRootFields: string[];
+  updatedTypes: string[];
 }
 
 export const checkTargetMappings =
   ({
+    indexTypes,
     actualMappings,
     expectedMappings,
-  }: CheckTargetMappingsParams): TaskEither.TaskEither<never, TargetMappingsCompareResult> =>
+  }: CheckTargetMappingsParams): TaskEither.TaskEither<
+    ActualMappingsIncomplete | ComparedMappingsChanged,
+    ComparedMappingsMatch
+  > =>
   async () => {
-    if (!actualMappings) {
-      return Either.right({ match: false });
+    if (
+      !actualMappings?._meta?.migrationMappingPropertyHashes ||
+      actualMappings.dynamic !== expectedMappings.dynamic
+    ) {
+      return Either.left({ type: 'actual_mappings_incomplete' as const });
+    }
+
+    const updatedHashes = getUpdatedHashes({
+      actual: actualMappings,
+      expected: expectedMappings,
+    });
+
+    if (updatedHashes.length) {
+      const updatedTypes = updatedHashes.filter((field) => indexTypes.includes(field));
+      const updatedRootFields = updatedHashes.filter((field) => !indexTypes.includes(field));
+      return Either.left({
+        type: 'compared_mappings_changed' as const,
+        updatedRootFields,
+        updatedTypes,
+      });
+    } else {
+      return Either.right({ type: 'compared_mappings_match' as const });
     }
-    const diff = diffMappings(actualMappings, expectedMappings);
-    const updatedHashes = getUpdatedHashes(actualMappings, expectedMappings);
-    return Either.right({ match: !diff, updatedHashes });
   };

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts

@@ -108,6 +108,7 @@ import type { UnknownDocsFound } from './check_for_unknown_docs';
 import type { IncompatibleClusterRoutingAllocation } from './initialize_action';
 import type { ClusterShardLimitExceeded } from './create_index';
 import type { SynchronizationFailed } from './synchronize_migrators';
+import type { ActualMappingsIncomplete, ComparedMappingsChanged } from './check_target_mappings';
 
 export type {
   CheckForUnknownDocsParams,
@@ -176,6 +177,8 @@ export interface ActionErrorTypeMap {
   cluster_shard_limit_exceeded: ClusterShardLimitExceeded;
   es_response_too_large: EsResponseTooLargeError;
   synchronization_failed: SynchronizationFailed;
+  actual_mappings_incomplete: ActualMappingsIncomplete;
+  compared_mappings_changed: ComparedMappingsChanged;
 }
 
 /**

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts

@@ -21,7 +21,7 @@ export interface UpdateByQueryResponse {
 
 /**
  * Pickup updated mappings by performing an update by query operation on all
- * documents in the index. Returns a task ID which can be
+ * documents matching the passed in query. Returns a task ID which can be
  * tracked for progress.
  *
  * @remarks When mappings are updated to add a field which previously wasn't

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts

@@ -210,37 +210,37 @@ describe('diffMappings', () => {
 });
 
 describe('getUpdatedHashes', () => {
-  test('is undefined if _meta is missing from actual', () => {
+  test('gives all hashes if _meta is missing from actual', () => {
     const actual: IndexMapping = {
       dynamic: 'strict',
       properties: {},
     };
     const expected: IndexMapping = {
       _meta: {
-        migrationMappingPropertyHashes: { foo: 'bar' },
+        migrationMappingPropertyHashes: { foo: 'bar', bar: 'baz' },
       },
       dynamic: 'strict',
       properties: {},
     };
 
-    expect(getUpdatedHashes(actual, expected)).toBeUndefined();
+    expect(getUpdatedHashes({ actual, expected })).toEqual(['foo', 'bar']);
   });
 
-  test('is undefined if migrationMappingPropertyHashes is missing from actual', () => {
+  test('gives all hashes if migrationMappingPropertyHashes is missing from actual', () => {
     const actual: IndexMapping = {
       dynamic: 'strict',
       properties: {},
       _meta: {},
     };
     const expected: IndexMapping = {
       _meta: {
-        migrationMappingPropertyHashes: { foo: 'bar' },
+        migrationMappingPropertyHashes: { foo: 'bar', bar: 'baz' },
       },
       dynamic: 'strict',
       properties: {},
     };
 
-    expect(getUpdatedHashes(actual, expected)).toBeUndefined();
+    expect(getUpdatedHashes({ actual, expected })).toEqual(['foo', 'bar']);
   });
 
   test('gives a list of the types with updated hashes', () => {
@@ -267,6 +267,6 @@ describe('getUpdatedHashes', () => {
       },
     };
 
-    expect(getUpdatedHashes(actual, expected)).toEqual(['type2', 'type4']);
+    expect(getUpdatedHashes({ actual, expected })).toEqual(['type2', 'type4']);
   });
 });

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.ts

@@ -67,18 +67,21 @@ export function diffMappings(actual: IndexMapping, expected: IndexMapping) {
  * Compares the actual vs expected mappings' hashes.
  * Returns a list with all the hashes that have been updated.
  */
-export const getUpdatedHashes = (
-  current: IndexMapping,
-  updated: IndexMapping
-): string[] | undefined => {
-  if (!current._meta?.migrationMappingPropertyHashes) {
-    return undefined;
+export const getUpdatedHashes = ({
+  actual,
+  expected,
+}: {
+  actual: IndexMapping;
+  expected: IndexMapping;
+}): string[] => {
+  if (!actual._meta?.migrationMappingPropertyHashes) {
+    return Object.keys(expected._meta!.migrationMappingPropertyHashes!);
   }
 
-  const updatedHashes = Object.keys(updated._meta!.migrationMappingPropertyHashes!).filter(
+  const updatedHashes = Object.keys(expected._meta!.migrationMappingPropertyHashes!).filter(
     (key) =>
-      current._meta!.migrationMappingPropertyHashes![key] !==
-      updated._meta!.migrationMappingPropertyHashes![key]
+      actual._meta!.migrationMappingPropertyHashes![key] !==
+      expected._meta!.migrationMappingPropertyHashes![key]
   );
 
   return updatedHashes;