review comments

src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.test.ts

@@ -0,0 +1,132 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import * as Either from 'fp-ts/lib/Either';
+import { catchRetryableEsClientErrors } from './catch_retryable_es_client_errors';
+import { errors as EsErrors, estypes } from '@elastic/elasticsearch';
+import { elasticsearchClientMock } from '../../../elasticsearch/client/mocks';
+import { checkForUnknownDocs } from './check_for_unknown_docs';
+
+jest.mock('./catch_retryable_es_client_errors');
+
+describe('checkForUnknownDocs', () => {
+  const unusedTypesQuery: estypes.QueryDslQueryContainer = {
+    bool: { must: [{ term: { hello: 'dolly' } }] },
+  };
+  const knownTypes = ['foo', 'bar'];
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('calls catchRetryableEsClientErrors when the promise rejects', async () => {
+    // Create a mock client that rejects all methods with a 503 status code response.
+    const retryableError = new EsErrors.ResponseError(
+      elasticsearchClientMock.createApiResponse({
+        statusCode: 503,
+        body: { error: { type: 'es_type', reason: 'es_reason' } },
+      })
+    );
+    const client = elasticsearchClientMock.createInternalClient(
+      elasticsearchClientMock.createErrorTransportRequestPromise(retryableError)
+    );
+
+    const task = checkForUnknownDocs({
+      client,
+      indexName: '.kibana_8.0.0',
+      knownTypes,
+      unusedTypesQuery,
+    });
+    try {
+      await task();
+    } catch (e) {
+      /** ignore */
+    }
+    expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError);
+  });
+
+  it('calls `client.search` with the correct parameters', async () => {
+    const client = elasticsearchClientMock.createInternalClient(
+      elasticsearchClientMock.createSuccessTransportRequestPromise({ hits: { hits: [] } })
+    );
+
+    const task = checkForUnknownDocs({
+      client,
+      indexName: '.kibana_8.0.0',
+      knownTypes,
+      unusedTypesQuery,
+    });
+
+    await task();
+
+    expect(client.search).toHaveBeenCalledTimes(1);
+    expect(client.search).toHaveBeenCalledWith({
+      index: '.kibana_8.0.0',
+      body: {
+        query: {
+          bool: {
+            must: unusedTypesQuery,
+            must_not: knownTypes.map((type) => ({
+              term: {
+                type,
+              },
+            })),
+          },
+        },
+      },
+    });
+  });
+
+  it('resolves with `Either.right` when no unknown docs are found', async () => {
+    const client = elasticsearchClientMock.createInternalClient(
+      elasticsearchClientMock.createSuccessTransportRequestPromise({ hits: { hits: [] } })
+    );
+
+    const task = checkForUnknownDocs({
+      client,
+      indexName: '.kibana_8.0.0',
+      knownTypes,
+      unusedTypesQuery,
+    });
+
+    const result = await task();
+
+    expect(Either.isRight(result)).toBe(true);
+  });
+
+  it('resolves with `Either.left` when unknown docs are found', async () => {
+    const client = elasticsearchClientMock.createInternalClient(
+      elasticsearchClientMock.createSuccessTransportRequestPromise({
+        hits: {
+          hits: [
+            { _id: '12', _source: { type: 'foo' } },
+            { _id: '14', _source: { type: 'bar' } },
+          ],
+        },
+      })
+    );
+
+    const task = checkForUnknownDocs({
+      client,
+      indexName: '.kibana_8.0.0',
+      knownTypes,
+      unusedTypesQuery,
+    });
+
+    const result = await task();
+
+    expect(Either.isLeft(result)).toBe(true);
+    expect((result as Either.Left<any>).left).toEqual({
+      type: 'unknown_docs_found',
+      unknownDocs: [
+        { id: '12', type: 'foo' },
+        { id: '14', type: 'bar' },
+      ],
+    });
+  });
+});

src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.ts

@@ -25,14 +25,15 @@ export interface CheckForUnknownDocsParams {
 }
 
 /** @internal */
-export interface CheckForUnknownDocsResponseDoc {
+export interface CheckForUnknownDocsFoundDoc {
   id: string;
   type: string;
 }
 
 /** @internal */
-export interface CheckForUnknownDocsResponse {
-  unknownDocs: CheckForUnknownDocsResponseDoc[];
+export interface UnknownDocsFound {
+  type: 'unknown_docs_found';
+  unknownDocs: CheckForUnknownDocsFoundDoc[];
 }
 
 export const checkForUnknownDocs = ({
@@ -41,8 +42,8 @@ export const checkForUnknownDocs = ({
   unusedTypesQuery,
   knownTypes,
 }: CheckForUnknownDocsParams): TaskEither.TaskEither<
-  RetryableEsClientError,
-  CheckForUnknownDocsResponse
+  RetryableEsClientError | UnknownDocsFound,
+  {}
 > => () => {
   const query = createUnknownDocQuery(unusedTypesQuery, knownTypes);
 
@@ -55,9 +56,14 @@ export const checkForUnknownDocs = ({
     })
     .then((response) => {
       const { hits } = response.body.hits;
-      return Either.right({
-        unknownDocs: hits.map((hit) => ({ id: hit._id, type: hit._source?.type ?? 'undefined' })),
-      });
+      if (hits.length) {
+        return Either.left({
+          type: 'unknown_docs_found' as const,
+          unknownDocs: hits.map((hit) => ({ id: hit._id, type: hit._source?.type ?? 'undefined' })),
+        });
+      } else {
+        return Either.right({});
+      }
     })
     .catch(catchRetryableEsClientErrors);
 };

src/core/server/saved_objects/migrationsv2/actions/index.ts

@@ -80,10 +80,11 @@ export type {
 } from './update_and_pickup_mappings';
 export { updateAndPickupMappings } from './update_and_pickup_mappings';
 
+import type { UnknownDocsFound } from './check_for_unknown_docs';
 export type {
   CheckForUnknownDocsParams,
-  CheckForUnknownDocsResponse,
-  CheckForUnknownDocsResponseDoc,
+  UnknownDocsFound,
+  CheckForUnknownDocsFoundDoc,
 } from './check_for_unknown_docs';
 export { checkForUnknownDocs } from './check_for_unknown_docs';
 
@@ -130,6 +131,7 @@ export interface ActionErrorTypeMap {
   alias_not_found_exception: AliasNotFound;
   remove_index_not_a_concrete_index: RemoveIndexNotAConcreteIndex;
   documents_transform_failed: DocumentsTransformFailed;
+  unknown_docs_found: UnknownDocsFound;
 }
 
 /**

src/core/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts

@@ -68,7 +68,7 @@ describe('migration v2', () => {
       expect(
         errorMessage.startsWith(
           'Unable to complete saved object migrations for the [.kibana] index: Migration failed because documents ' +
-            'from unknown types were found. To proceed with the migration, please delete these documents from the ' +
+            'were found for unknown saved object types. To proceed with the migration, please delete these documents from the ' +
             '".kibana_7.13.0_001" index.'
         )
       ).toBeTruthy();

src/core/server/saved_objects/migrationsv2/model/extract_errors.test.ts

@@ -25,8 +25,8 @@ describe('extractUnknownDocFailureReason', () => {
         '.kibana_15'
       )
     ).toMatchInlineSnapshot(`
-      "Migration failed because documents from unknown types were found. To proceed with the migration, please delete these documents from the \\".kibana_15\\" index.
-      The unknown documents were:
+      "Migration failed because documents were found for unknown saved object types. To proceed with the migration, please delete these documents from the \\".kibana_15\\" index.
+      The documents with unknown types are:
       - \\"unknownType:12\\" (type: \\"unknownType\\")
       - \\"anotherUnknownType:42\\" (type: \\"anotherUnknownType\\")
       You can delete them using the following command:

src/core/server/saved_objects/migrationsv2/model/extract_errors.ts

@@ -7,7 +7,7 @@
  */
 
 import { TransformErrorObjects } from '../../migrations/core';
-import { CheckForUnknownDocsResponseDoc } from '../actions';
+import { CheckForUnknownDocsFoundDoc } from '../actions';
 
 /**
  * Constructs migration failure message strings from corrupt document ids and document transformation errors
@@ -37,13 +37,13 @@ export function extractTransformFailuresReason(
 }
 
 export function extractUnknownDocFailureReason(
-  unknownDocs: CheckForUnknownDocsResponseDoc[],
+  unknownDocs: CheckForUnknownDocsFoundDoc[],
   sourceIndex: string
 ): string {
   return (
-    `Migration failed because documents from unknown types were found. ` +
+    `Migration failed because documents were found for unknown saved object types. ` +
     `To proceed with the migration, please delete these documents from the "${sourceIndex}" index.\n` +
-    `The unknown documents were:\n` +
+    `The documents with unknown types are:\n` +
     unknownDocs.map((doc) => `- "${doc.id}" (type: "${doc.type}")\n`).join('') +
     `You can delete them using the following command:\n` +
     `curl -X POST "{elasticsearch}/${sourceIndex}/_bulk?pretty" -H 'Content-Type: application/json' -d'\n` +

src/core/server/saved_objects/migrationsv2/model/model.test.ts

@@ -715,15 +715,15 @@ describe('migrations v2 model', () => {
         },
       } as const;
 
-      test('CHECK_UNKNOWN_DOCUMENTS -> SET_SOURCE_WRITE_BLOCK if action succeeds and no unknown docs were found', () => {
+      test('CHECK_UNKNOWN_DOCUMENTS -> SET_SOURCE_WRITE_BLOCK if action succeeds', () => {
         const checkUnknownDocumentsSourceState: CheckUnknownDocumentsState = {
           ...baseState,
           controlState: 'CHECK_UNKNOWN_DOCUMENTS',
           sourceIndex: Option.some('.kibana_3') as Option.Some<string>,
           sourceIndexMappings: mappingsWithUnknownType,
         };
 
-        const res: ResponseType<'CHECK_UNKNOWN_DOCUMENTS'> = Either.right({ unknownDocs: [] });
+        const res: ResponseType<'CHECK_UNKNOWN_DOCUMENTS'> = Either.right({});
         const newState = model(checkUnknownDocumentsSourceState, res);
         expect(newState.controlState).toEqual('SET_SOURCE_WRITE_BLOCK');
 
@@ -760,15 +760,16 @@ describe('migrations v2 model', () => {
         `);
       });
 
-      test('CHECK_UNKNOWN_DOCUMENTS -> FATAL if action succeeds but unknown docs were found', () => {
+      test('CHECK_UNKNOWN_DOCUMENTS -> FATAL if action fails and unknown docs were found', () => {
         const checkUnknownDocumentsSourceState: CheckUnknownDocumentsState = {
           ...baseState,
           controlState: 'CHECK_UNKNOWN_DOCUMENTS',
           sourceIndex: Option.some('.kibana_3') as Option.Some<string>,
           sourceIndexMappings: mappingsWithUnknownType,
         };
 
-        const res: ResponseType<'CHECK_UNKNOWN_DOCUMENTS'> = Either.right({
+        const res: ResponseType<'CHECK_UNKNOWN_DOCUMENTS'> = Either.left({
+          type: 'unknown_docs_found',
           unknownDocs: [
             { id: 'dashboard:12', type: 'dashboard' },
             { id: 'foo:17', type: 'foo' },
@@ -780,7 +781,7 @@ describe('migrations v2 model', () => {
         expect(newState).toMatchObject({
           controlState: 'FATAL',
           reason: expect.stringContaining(
-            'Migration failed because documents from unknown types were found'
+            'Migration failed because documents were found for unknown saved object types'
           ),
         });
       });

src/core/server/saved_objects/migrationsv2/model/model.ts

@@ -319,35 +319,34 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
   } else if (stateP.controlState === 'CHECK_UNKNOWN_DOCUMENTS') {
     const res = resW as ExcludeRetryableEsError<ResponseType<typeof stateP.controlState>>;
     if (Either.isRight(res)) {
-      const { unknownDocs } = res.right;
-      if (unknownDocs.length) {
+      const source = stateP.sourceIndex;
+      const target = stateP.versionIndex;
+      return {
+        ...stateP,
+        controlState: 'SET_SOURCE_WRITE_BLOCK',
+        sourceIndex: source,
+        targetIndex: target,
+        targetIndexMappings: disableUnknownTypeMappingFields(
+          stateP.targetIndexMappings,
+          stateP.sourceIndexMappings
+        ),
+        versionIndexReadyActions: Option.some<AliasAction[]>([
+          { remove: { index: source.value, alias: stateP.currentAlias, must_exist: true } },
+          { add: { index: target, alias: stateP.currentAlias } },
+          { add: { index: target, alias: stateP.versionAlias } },
+          { remove_index: { index: stateP.tempIndex } },
+        ]),
+      };
+    } else {
+      if (isLeftTypeof(res.left, 'unknown_docs_found')) {
         return {
           ...stateP,
           controlState: 'FATAL',
-          reason: extractUnknownDocFailureReason(unknownDocs, stateP.sourceIndex.value),
+          reason: extractUnknownDocFailureReason(res.left.unknownDocs, stateP.sourceIndex.value),
         };
       } else {
-        const source = stateP.sourceIndex;
-        const target = stateP.versionIndex;
-        return {
-          ...stateP,
-          controlState: 'SET_SOURCE_WRITE_BLOCK',
-          sourceIndex: source,
-          targetIndex: target,
-          targetIndexMappings: disableUnknownTypeMappingFields(
-            stateP.targetIndexMappings,
-            stateP.sourceIndexMappings
-          ),
-          versionIndexReadyActions: Option.some<AliasAction[]>([
-            { remove: { index: source.value, alias: stateP.currentAlias, must_exist: true } },
-            { add: { index: target, alias: stateP.currentAlias } },
-            { add: { index: target, alias: stateP.versionAlias } },
-            { remove_index: { index: stateP.tempIndex } },
-          ]),
-        };
+        return throwBadResponse(stateP, res.left);
       }
-    } else {
-      return throwBadResponse(stateP, res);
     }
   } else if (stateP.controlState === 'SET_SOURCE_WRITE_BLOCK') {
     const res = resW as ExcludeRetryableEsError<ResponseType<typeof stateP.controlState>>;