Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZDT] Pickup updated types only #161123

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ jest.mock('../core/build_active_mappings');

const getUpdatedHashesMock = getUpdatedHashes as jest.MockedFn<typeof getUpdatedHashes>;

const indexTypes = ['type1', 'type2'];

const properties: SavedObjectsMappingProperties = {
type1: { type: 'long' },
type2: { type: 'long' },
Expand All @@ -48,7 +46,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,
});

Expand All @@ -58,7 +55,6 @@ describe('checkTargetMappings', () => {

it("returns 'actual_mappings_incomplete' if actual mappings do not define _meta", async () => {
const task = checkTargetMappings({
indexTypes,
expectedMappings,
actualMappings: {
properties,
Expand All @@ -72,7 +68,6 @@ describe('checkTargetMappings', () => {

it("returns 'actual_mappings_incomplete' if actual mappings do not define migrationMappingPropertyHashes", async () => {
const task = checkTargetMappings({
indexTypes,
expectedMappings,
actualMappings: {
properties,
Expand All @@ -87,7 +82,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,
Expand All @@ -105,7 +99,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,
});
Expand All @@ -115,8 +108,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));
});
Expand All @@ -125,7 +117,6 @@ describe('checkTargetMappings', () => {
describe('and mappings match', () => {
it('returns a compared_mappings_match response', async () => {
const task = checkTargetMappings({
indexTypes,
expectedMappings,
actualMappings: expectedMappings,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { getUpdatedHashes } from '../core/build_active_mappings';

/** @internal */
export interface CheckTargetMappingsParams {
indexTypes: string[];
actualMappings?: IndexMapping;
expectedMappings: IndexMapping;
}
Expand All @@ -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<
Expand All @@ -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 });
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
Original file line number Diff line number Diff line change
@@ -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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Why passing rootFields as parameters? any value compared to just using Object.keys(getBaseMappings().properties) internally in this function? Is this only for testability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was mainly thinking of testability, but I don't have a strong opinion.
I will simplify and build the rootFields internally.

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 } })),
},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<AllActionStates>): State => {
// The action response `resW` is weakly typed, the type includes all action
Expand Down Expand Up @@ -1439,18 +1441,24 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
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}.`,
},
],
};
Expand All @@ -1460,16 +1468,12 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
...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}.`,
},
],
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof nextActionMap>;

Expand Down Expand Up @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
SetDocMigrationStartedState,
UpdateMappingModelVersionState,
UpdateDocumentModelVersionsState,
UpdateIndexMappingsState,
} from './state';

describe('actions', () => {
Expand Down Expand Up @@ -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,
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof nextActionMap>;

Expand Down Expand Up @@ -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({
Expand Down