From 8011281070f65929241526911c8dcfda79b0e8fe Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Tue, 29 Jun 2021 10:15:18 +0200 Subject: [PATCH] review comments --- .../actions/check_for_unknown_docs.test.ts | 132 ++++++++++++++++++ .../actions/check_for_unknown_docs.ts | 22 +-- .../migrationsv2/actions/index.ts | 6 +- .../migration_7_13_0_unknown_types.test.ts | 2 +- .../migrationsv2/model/extract_errors.test.ts | 4 +- .../migrationsv2/model/extract_errors.ts | 8 +- .../migrationsv2/model/model.test.ts | 11 +- .../saved_objects/migrationsv2/model/model.ts | 45 +++--- 8 files changed, 185 insertions(+), 45 deletions(-) create mode 100644 src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.test.ts diff --git a/src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.test.ts b/src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.test.ts new file mode 100644 index 0000000000000..15e3bc14a392e --- /dev/null +++ b/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).left).toEqual({ + type: 'unknown_docs_found', + unknownDocs: [ + { id: '12', type: 'foo' }, + { id: '14', type: 'bar' }, + ], + }); + }); +}); diff --git a/src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.ts b/src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.ts index e38a820b89846..8c908ba6f8f57 100644 --- a/src/core/server/saved_objects/migrationsv2/actions/check_for_unknown_docs.ts +++ b/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); }; diff --git a/src/core/server/saved_objects/migrationsv2/actions/index.ts b/src/core/server/saved_objects/migrationsv2/actions/index.ts index a2ddfebbda262..8e4584970f138 100644 --- a/src/core/server/saved_objects/migrationsv2/actions/index.ts +++ b/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; } /** diff --git a/src/core/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts b/src/core/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts index 8e4316f6ce86d..c5e302adbe903 100644 --- a/src/core/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts +++ b/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(); diff --git a/src/core/server/saved_objects/migrationsv2/model/extract_errors.test.ts b/src/core/server/saved_objects/migrationsv2/model/extract_errors.test.ts index 52847c4cc7f69..a028c40ca6597 100644 --- a/src/core/server/saved_objects/migrationsv2/model/extract_errors.test.ts +++ b/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: diff --git a/src/core/server/saved_objects/migrationsv2/model/extract_errors.ts b/src/core/server/saved_objects/migrationsv2/model/extract_errors.ts index 6b64f260e4ab4..cc6fe7bad3ca7 100644 --- a/src/core/server/saved_objects/migrationsv2/model/extract_errors.ts +++ b/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` + diff --git a/src/core/server/saved_objects/migrationsv2/model/model.test.ts b/src/core/server/saved_objects/migrationsv2/model/model.test.ts index 13840230ff54d..174459d04d9ee 100644 --- a/src/core/server/saved_objects/migrationsv2/model/model.test.ts +++ b/src/core/server/saved_objects/migrationsv2/model/model.test.ts @@ -715,7 +715,7 @@ 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', @@ -723,7 +723,7 @@ describe('migrations v2 model', () => { 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,7 +760,7 @@ 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', @@ -768,7 +768,8 @@ describe('migrations v2 model', () => { 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' ), }); }); diff --git a/src/core/server/saved_objects/migrationsv2/model/model.ts b/src/core/server/saved_objects/migrationsv2/model/model.ts index 8577ec6489e92..e7d6b8ed175e5 100644 --- a/src/core/server/saved_objects/migrationsv2/model/model.ts +++ b/src/core/server/saved_objects/migrationsv2/model/model.ts @@ -319,35 +319,34 @@ export const model = (currentState: State, resW: ResponseType): } else if (stateP.controlState === 'CHECK_UNKNOWN_DOCUMENTS') { const res = resW as ExcludeRetryableEsError>; 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([ + { 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([ - { 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>;