From 397c14c9aca017954c7e985afdeb7895250bd852 Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Mon, 3 Jul 2023 18:19:05 +0200 Subject: [PATCH 1/3] [ZDT] Pickup updated types only --- .../src/actions/check_target_mappings.test.ts | 9 +-- .../src/actions/check_target_mappings.ts | 10 +-- .../core/build_pickup_mappings_query.test.ts | 37 +++++++++++ .../src/core/build_pickup_mappings_query.ts | 31 +++++++++ .../src/model/model.test.ts | 6 +- .../src/model/model.ts | 22 ++++--- .../src/next.ts | 2 - .../src/zdt/next.test.ts | 64 +++++++++++++++++++ .../src/zdt/next.ts | 6 ++ 9 files changed, 156 insertions(+), 31 deletions(-) create mode 100644 packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts create mode 100644 packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts index 17c55102a3cc7..8f31a46da8c27 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts @@ -48,7 +48,6 @@ describe('checkTargetMappings', () => { describe('when actual mappings are incomplete', () => { it("returns 'actual_mappings_incomplete' if actual mappings are not defined", async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, }); @@ -58,7 +57,6 @@ describe('checkTargetMappings', () => { it("returns 'actual_mappings_incomplete' if actual mappings do not define _meta", async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, actualMappings: { properties, @@ -72,7 +70,6 @@ describe('checkTargetMappings', () => { it("returns 'actual_mappings_incomplete' if actual mappings do not define migrationMappingPropertyHashes", async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, actualMappings: { properties, @@ -87,7 +84,6 @@ describe('checkTargetMappings', () => { it("returns 'actual_mappings_incomplete' if actual mappings define a different value for 'dynamic' property", async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, actualMappings: { properties, @@ -105,7 +101,6 @@ describe('checkTargetMappings', () => { describe('and mappings do not match', () => { it('returns the lists of changed root fields and types', async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, actualMappings: expectedMappings, }); @@ -115,8 +110,7 @@ describe('checkTargetMappings', () => { const result = await task(); const expected: ComparedMappingsChanged = { type: 'compared_mappings_changed' as const, - updatedRootFields: ['someRootField'], - updatedTypes: ['type1', 'type2'], + updatedHashes: ['type1', 'type2', 'someRootField'], }; expect(result).toEqual(Either.left(expected)); }); @@ -125,7 +119,6 @@ describe('checkTargetMappings', () => { describe('and mappings match', () => { it('returns a compared_mappings_match response', async () => { const task = checkTargetMappings({ - indexTypes, expectedMappings, actualMappings: expectedMappings, }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts index 26e0f074f43cb..576459beadd74 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts @@ -13,7 +13,6 @@ import { getUpdatedHashes } from '../core/build_active_mappings'; /** @internal */ export interface CheckTargetMappingsParams { - indexTypes: string[]; actualMappings?: IndexMapping; expectedMappings: IndexMapping; } @@ -29,13 +28,11 @@ export interface ActualMappingsIncomplete { export interface ComparedMappingsChanged { type: 'compared_mappings_changed'; - updatedRootFields: string[]; - updatedTypes: string[]; + updatedHashes: string[]; } export const checkTargetMappings = ({ - indexTypes, actualMappings, expectedMappings, }: CheckTargetMappingsParams): TaskEither.TaskEither< @@ -56,12 +53,9 @@ export const checkTargetMappings = }); 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, + updatedHashes, }); } else { return Either.right({ type: 'compared_mappings_match' as const }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts new file mode 100644 index 0000000000000..700a026b2b0f7 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts @@ -0,0 +1,37 @@ +/* + * 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 { buildPickupMappingsQuery } from './build_pickup_mappings_query'; + +describe('buildPickupMappingsQuery', () => { + describe('when no root fields have been updated', () => { + it('builds a boolean query to select the updated types', () => { + const query = buildPickupMappingsQuery({ + rootFields: ['someRootField'], + updatedFields: ['type1', 'type2'], + }); + + expect(query).toEqual({ + bool: { + should: [{ term: { type: 'type1' } }, { term: { type: 'type2' } }], + }, + }); + }); + }); + + describe('when some root fields have been updated', () => { + it('returns undefined', () => { + const query = buildPickupMappingsQuery({ + rootFields: ['someRootField'], + updatedFields: ['type1', 'type2', 'someRootField'], + }); + + expect(query).toBeUndefined(); + }); + }); +}); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts new file mode 100644 index 0000000000000..158660a75bb20 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts @@ -0,0 +1,31 @@ +/* + * 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 { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types'; + +export const buildPickupMappingsQuery = ({ + rootFields, + updatedFields, +}: { + rootFields: string[]; + updatedFields: string[]; +}): QueryDslQueryContainer | undefined => { + if (updatedFields.some((field) => rootFields.includes(field))) { + // we are updating some root fields, update ALL documents (no filter query) + return undefined; + } + + // at this point, all updated fields correspond to SO types + const updatedTypes = updatedFields; + + return { + bool: { + should: updatedTypes.map((type) => ({ term: { type } })), + }, + }; +}; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts index 39a03e1e28d47..7039ce1cd1afa 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.test.ts @@ -2615,8 +2615,7 @@ describe('migrations v2 model', () => { it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if core fields have been updated', () => { const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.left({ type: 'compared_mappings_changed' as const, - updatedRootFields: ['namespaces'], - updatedTypes: ['dashboard', 'lens'], + updatedHashes: ['dashboard', 'lens', 'namespaces'], }); const newState = model( checkTargetMappingsState, @@ -2631,8 +2630,7 @@ describe('migrations v2 model', () => { it('CHECK_TARGET_MAPPINGS -> UPDATE_TARGET_MAPPINGS_PROPERTIES if only SO types have changed', () => { const res: ResponseType<'CHECK_TARGET_MAPPINGS'> = Either.left({ type: 'compared_mappings_changed' as const, - updatedRootFields: [], - updatedTypes: ['dashboard', 'lens'], + updatedHashes: ['dashboard', 'lens'], }); const newState = model( checkTargetMappingsState, diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts index 2505581c6bc3d..ebe447e835439 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts @@ -54,6 +54,8 @@ import { CLUSTER_SHARD_LIMIT_EXCEEDED_REASON, FATAL_REASON_REQUEST_ENTITY_TOO_LARGE, } from '../common/constants'; +import { getBaseMappings } from '../core'; +import { buildPickupMappingsQuery } from '../core/build_pickup_mappings_query'; export const model = (currentState: State, resW: ResponseType): State => { // The action response `resW` is weakly typed, the type includes all action @@ -1439,18 +1441,24 @@ export const model = (currentState: State, resW: ResponseType): updatedTypesQuery: Option.none, }; } else if (isTypeof(left, 'compared_mappings_changed')) { - if (left.updatedRootFields.length) { + const rootFields = Object.keys(getBaseMappings().properties); + const updatedRootFields = left.updatedHashes.filter((field) => rootFields.includes(field)); + const updatedTypesQuery = Option.fromNullable( + buildPickupMappingsQuery({ rootFields, updatedFields: left.updatedHashes }) + ); + + if (updatedRootFields.length) { // compatible migration: some core fields have been updated return { ...stateP, controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', // we must "pick-up" all documents on the index (by not providing a query) - updatedTypesQuery: Option.none, + updatedTypesQuery, logs: [ ...stateP.logs, { level: 'info', - message: `Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: ${left.updatedRootFields}.`, + message: `Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: ${updatedRootFields}.`, }, ], }; @@ -1460,16 +1468,12 @@ export const model = (currentState: State, resW: ResponseType): ...stateP, controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES', // we can "pick-up" only the SO types that have changed - updatedTypesQuery: Option.some({ - bool: { - should: left.updatedTypes.map((type) => ({ term: { type } })), - }, - }), + updatedTypesQuery, logs: [ ...stateP.logs, { level: 'info', - message: `Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings: ${left.updatedTypes}.`, + message: `Kibana is performing a compatible upgrade and NO root fields have been udpated. Kibana will update the following SO types so that ES can pickup the updated mappings: ${left.updatedHashes}.`, }, ], }; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts index df7e0c23fbc20..b8d4afc55631d 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts @@ -58,7 +58,6 @@ import { createDelayFn } from './common/utils'; import type { TransformRawDocs } from './types'; import * as Actions from './actions'; import { REMOVED_TYPES } from './core'; -import { getIndexTypes } from './model/helpers'; type ActionMap = ReturnType; @@ -202,7 +201,6 @@ export const nextActionMap = ( Actions.checkTargetMappings({ actualMappings: Option.toUndefined(state.sourceIndexMappings), expectedMappings: state.targetIndexMappings, - indexTypes: getIndexTypes(state), }), UPDATE_TARGET_MAPPINGS_PROPERTIES: (state: UpdateTargetMappingsPropertiesState) => Actions.updateAndPickupMappings({ diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.test.ts index 00684ec46f85c..6c2f90155ac76 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.test.ts @@ -22,6 +22,7 @@ import type { SetDocMigrationStartedState, UpdateMappingModelVersionState, UpdateDocumentModelVersionsState, + UpdateIndexMappingsState, } from './state'; describe('actions', () => { @@ -147,4 +148,67 @@ describe('actions', () => { }); }); }); + + describe('UPDATE_INDEX_MAPPINGS', () => { + describe('when only SO types have been updated', () => { + it('calls updateAndPickupMappings with the correct parameters', () => { + const state: UpdateIndexMappingsState = { + ...createPostDocInitState(), + controlState: 'UPDATE_INDEX_MAPPINGS', + additiveMappingChanges: { + someToken: {}, + }, + }; + const action = actionMap.UPDATE_INDEX_MAPPINGS; + + action(state); + + expect(ActionMocks.updateAndPickupMappings).toHaveBeenCalledTimes(1); + expect(ActionMocks.updateAndPickupMappings).toHaveBeenCalledWith({ + client: context.elasticsearchClient, + index: state.currentIndex, + mappings: { + properties: { + someToken: {}, + }, + }, + batchSize: context.batchSize, + query: { + bool: { + should: [{ term: { type: 'someToken' } }], + }, + }, + }); + }); + }); + + describe('when core properties have been updated', () => { + it('calls updateAndPickupMappings with the correct parameters', () => { + const state: UpdateIndexMappingsState = { + ...createPostDocInitState(), + controlState: 'UPDATE_INDEX_MAPPINGS', + additiveMappingChanges: { + managed: {}, // this is a root field + someToken: {}, + }, + }; + const action = actionMap.UPDATE_INDEX_MAPPINGS; + + action(state); + + expect(ActionMocks.updateAndPickupMappings).toHaveBeenCalledTimes(1); + expect(ActionMocks.updateAndPickupMappings).toHaveBeenCalledWith({ + client: context.elasticsearchClient, + index: state.currentIndex, + mappings: { + properties: { + managed: {}, + someToken: {}, + }, + }, + batchSize: context.batchSize, + }); + }); + }); + }); }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts index 17749104d18fe..9d1e5b07fdd80 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts @@ -39,6 +39,8 @@ import { setMetaDocMigrationComplete, setMetaDocMigrationStarted, } from './utils'; +import { buildPickupMappingsQuery } from '../core/build_pickup_mappings_query'; +import { getBaseMappings } from '../core'; export type ActionMap = ReturnType; @@ -72,6 +74,10 @@ export const nextActionMap = (context: MigratorContext) => { index: state.currentIndex, mappings: { properties: state.additiveMappingChanges }, batchSize: context.batchSize, + query: buildPickupMappingsQuery({ + rootFields: Object.keys(getBaseMappings().properties), + updatedFields: Object.keys(state.additiveMappingChanges), + }), }), UPDATE_INDEX_MAPPINGS_WAIT_FOR_TASK: (state: UpdateIndexMappingsWaitForTaskState) => Actions.waitForPickupUpdatedMappingsTask({ From dfe83161695956f5770b01c69651e6391716a972 Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Mon, 3 Jul 2023 18:32:08 +0200 Subject: [PATCH 2/3] Fix type errors --- .../src/actions/check_target_mappings.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts index 8f31a46da8c27..4e04cc2a70539 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts @@ -20,8 +20,6 @@ jest.mock('../core/build_active_mappings'); const getUpdatedHashesMock = getUpdatedHashes as jest.MockedFn; -const indexTypes = ['type1', 'type2']; - const properties: SavedObjectsMappingProperties = { type1: { type: 'long' }, type2: { type: 'long' }, From 3ba6b03170263cabcda933b4956d35b518222e7f Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Tue, 4 Jul 2023 15:50:20 +0200 Subject: [PATCH 3/3] Address PR feedback --- .../src/core/build_pickup_mappings_query.test.ts | 10 ++-------- .../src/core/build_pickup_mappings_query.ts | 13 ++++++------- .../src/model/model.ts | 4 +--- .../src/zdt/next.ts | 6 +----- 4 files changed, 10 insertions(+), 23 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts index 700a026b2b0f7..3895bd9314df3 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.test.ts @@ -11,10 +11,7 @@ import { buildPickupMappingsQuery } from './build_pickup_mappings_query'; describe('buildPickupMappingsQuery', () => { describe('when no root fields have been updated', () => { it('builds a boolean query to select the updated types', () => { - const query = buildPickupMappingsQuery({ - rootFields: ['someRootField'], - updatedFields: ['type1', 'type2'], - }); + const query = buildPickupMappingsQuery(['type1', 'type2']); expect(query).toEqual({ bool: { @@ -26,10 +23,7 @@ describe('buildPickupMappingsQuery', () => { describe('when some root fields have been updated', () => { it('returns undefined', () => { - const query = buildPickupMappingsQuery({ - rootFields: ['someRootField'], - updatedFields: ['type1', 'type2', 'someRootField'], - }); + const query = buildPickupMappingsQuery(['type1', 'type2', 'namespaces']); expect(query).toBeUndefined(); }); diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts index 158660a75bb20..ce110f72f66c1 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/build_pickup_mappings_query.ts @@ -7,14 +7,13 @@ */ import { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types'; +import { getBaseMappings } from './build_active_mappings'; + +export const buildPickupMappingsQuery = ( + updatedFields: string[] +): QueryDslQueryContainer | undefined => { + const rootFields = Object.keys(getBaseMappings().properties); -export const buildPickupMappingsQuery = ({ - rootFields, - updatedFields, -}: { - rootFields: string[]; - updatedFields: string[]; -}): QueryDslQueryContainer | undefined => { if (updatedFields.some((field) => rootFields.includes(field))) { // we are updating some root fields, update ALL documents (no filter query) return undefined; diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts index ebe447e835439..2264ca388c975 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts @@ -1443,9 +1443,7 @@ export const model = (currentState: State, resW: ResponseType): } else if (isTypeof(left, 'compared_mappings_changed')) { const rootFields = Object.keys(getBaseMappings().properties); const updatedRootFields = left.updatedHashes.filter((field) => rootFields.includes(field)); - const updatedTypesQuery = Option.fromNullable( - buildPickupMappingsQuery({ rootFields, updatedFields: left.updatedHashes }) - ); + const updatedTypesQuery = Option.fromNullable(buildPickupMappingsQuery(left.updatedHashes)); if (updatedRootFields.length) { // compatible migration: some core fields have been updated diff --git a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts index 9d1e5b07fdd80..fe1093b0f4978 100644 --- a/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts +++ b/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/next.ts @@ -40,7 +40,6 @@ import { setMetaDocMigrationStarted, } from './utils'; import { buildPickupMappingsQuery } from '../core/build_pickup_mappings_query'; -import { getBaseMappings } from '../core'; export type ActionMap = ReturnType; @@ -74,10 +73,7 @@ export const nextActionMap = (context: MigratorContext) => { index: state.currentIndex, mappings: { properties: state.additiveMappingChanges }, batchSize: context.batchSize, - query: buildPickupMappingsQuery({ - rootFields: Object.keys(getBaseMappings().properties), - updatedFields: Object.keys(state.additiveMappingChanges), - }), + query: buildPickupMappingsQuery(Object.keys(state.additiveMappingChanges)), }), UPDATE_INDEX_MAPPINGS_WAIT_FOR_TASK: (state: UpdateIndexMappingsWaitForTaskState) => Actions.waitForPickupUpdatedMappingsTask({