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.

Sign up for GitHub

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

Jump to bottom

[Cases] Mark lens migrations as deferred #159291

Merged
merged 5 commits into from
Jun 9, 2023
Merged

[Cases] Mark lens migrations as deferred #159291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5143120
Mark lens migrations as deferred
cnasikas Jun 8, 2023
a103f85
Merge branch 'main' into lens_deferred_migrations
kibanamachine Jun 9, 2023
638f4ad
PR feedback
cnasikas Jun 9, 2023
23e265e
Merge branch 'lens_deferred_migrations' of github.com:cnasikas/kibana…
cnasikas Jun 9, 2023
1bba41a
Fix
cnasikas Jun 9, 2023
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
68 changes: 63 additions & 5 deletions x-pack/plugins/cases/server/saved_object_types/migrations/comments.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ import type { MigrateFunction, MigrateFunctionsObject } from '@kbn/kibana-utils-
import type { SerializableRecord } from '@kbn/utility-types';
import { GENERATED_ALERT, SUB_CASE_SAVED_OBJECT } from './constants';
import { PersistableStateAttachmentTypeRegistry } from '../../attachment_framework/persistable_state_registry';
import { omit, partition } from 'lodash';
import type {
SavedObjectMigrationFn,
SavedObjectMigrationMap,
SavedObjectMigrationParams,
} from '@kbn/core-saved-objects-server';
import { gte } from 'semver';

describe('comments migrations', () => {
const contextMock = savedObjectsServiceMock.createMigrationContext();
Expand Down Expand Up @@ -108,7 +115,7 @@ describe('comments migrations', () => {
jest.clearAllMocks();
});

describe('lens embeddable migrations for by value panels', () => {
describe('lens migrations', () => {
describe('7.14.0 remove time zone from Lens visualization date histogram', () => {
const expectedLensVisualizationMigrated = {
title: 'MyRenamedOps',
Expand Down Expand Up @@ -271,6 +278,57 @@ describe('comments migrations', () => {
expect((columns[2] as { params: {} }).params).toEqual({ timeZone: 'do not delete' });
});
});

it('should marked lens migrations as deferred correctly', () => {
const lensEmbeddableFactory = makeLensEmbeddableFactory(
() => ({}),
() => ({}),
{}
);

const lensMigrations = lensEmbeddableFactory().migrations;
const lensMigrationObject =
typeof lensMigrations === 'function' ? lensMigrations() : lensMigrations || {};
const lensMigrationObjectWithFakeMigration = {
...lensMigrationObject,
'8.9.0': jest.fn(),
'8.10.0': jest.fn(),
};

const lensVersions = Object.keys(lensMigrationObjectWithFakeMigration);
const [lensVersionToBeDeferred, lensVersionToNotBeDeferred] = partition(
lensVersions,
(version) => gte(version, '8.9.0')
);

const migrations = createCommentsMigrations({
persistableStateAttachmentTypeRegistry: new PersistableStateAttachmentTypeRegistry(),
lensEmbeddableFactory: () => ({
id: 'test',
migrations: lensMigrationObjectWithFakeMigration,
}),
});

for (const version of lensVersionToBeDeferred) {
const migration = migrations[version] as SavedObjectMigrationParams;
expect(migration.deferred).toBe(true);
cnasikas marked this conversation as resolved.
Show resolved Hide resolved
expect(migration.transform).toEqual(expect.any(Function));
}

for (const version of lensVersionToNotBeDeferred) {
const migration = migrations[version] as SavedObjectMigrationParams;
expect(migration.deferred).toBe(false);
expect(migration.transform).toEqual(expect.any(Function));
}

const migrationsWithoutLens = omit<SavedObjectMigrationMap>(migrations, lensVersions);

for (const version of Object.keys(migrationsWithoutLens)) {
const migration = migrationsWithoutLens[version] as SavedObjectMigrationFn;

expect(migration).toEqual(expect.any(Function));
}
});
});

describe('handles errors', () => {
Expand Down Expand Up @@ -298,9 +356,9 @@ describe('comments migrations', () => {
};

it('logs an error when it fails to parse invalid json', () => {
const commentMigrationFunction = migrateByValueLensVisualizations(migrationFunction);
const commentMigrationFunction = migrateByValueLensVisualizations(migrationFunction, '0.0.0');

const result = commentMigrationFunction(caseComment, contextMock);
const result = commentMigrationFunction.transform(caseComment, contextMock);
// the comment should remain unchanged when there is an error
expect(result.attributes.comment).toEqual(comment);

Expand All @@ -322,7 +380,7 @@ describe('comments migrations', () => {
describe('mergeSavedObjectMigrationMaps', () => {
it('logs an error when the passed migration functions fails', () => {
const migrationObj1 = {
'1.0.0': migrateByValueLensVisualizations(migrationFunction),
'1.0.0': migrateByValueLensVisualizations(migrationFunction, '1.0.0'),
} as unknown as MigrateFunctionsObject;

const migrationObj2 = {
Expand Down Expand Up @@ -351,7 +409,7 @@ describe('comments migrations', () => {

it('it does not log an error when the migration function does not use the context', () => {
const migrationObj1 = {
'1.0.0': migrateByValueLensVisualizations(migrationFunction),
'1.0.0': migrateByValueLensVisualizations(migrationFunction, '1.0.0'),
} as unknown as MigrateFunctionsObject;

const migrationObj2 = {
Expand Down
85 changes: 50 additions & 35 deletions x-pack/plugins/cases/server/saved_object_types/migrations/comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import type { MigrateFunction, MigrateFunctionsObject } from '@kbn/kibana-utils-
import type {
SavedObjectUnsanitizedDoc,
SavedObjectSanitizedDoc,
SavedObjectMigrationFn,
SavedObjectMigrationMap,
SavedObjectMigrationContext,
} from '@kbn/core/server';
import { mergeSavedObjectMigrationMaps } from '@kbn/core/server';
import type { LensServerPluginSetup } from '@kbn/lens-plugin/server';
import type { SavedObjectMigrationParams } from '@kbn/core-saved-objects-server';
import { CommentType } from '../../../common/api';
import type { LensMarkdownNode, MarkdownNode } from '../../../common/utils/markdown_plugins/utils';
import {
Expand All @@ -26,8 +26,8 @@ import {
} from '../../../common/utils/markdown_plugins/utils';
import type { SanitizedCaseOwner } from '.';
import { addOwnerToSO } from '.';
import { logError } from './utils';
import { GENERATED_ALERT, SUB_CASE_SAVED_OBJECT } from './constants';
import { isDeferredMigration, logError } from './utils';
import { GENERATED_ALERT, MIN_DEFERRED_KIBANA_VERSION, SUB_CASE_SAVED_OBJECT } from './constants';
import type { PersistableStateAttachmentTypeRegistry } from '../../attachment_framework/persistable_state_registry';

interface UnsanitizedComment {
Expand Down Expand Up @@ -63,8 +63,8 @@ export const createCommentsMigrations = (

const embeddableMigrations = mapValues<
MigrateFunctionsObject,
SavedObjectMigrationFn<{ comment?: string }>
>(lensMigrationObject, migrateByValueLensVisualizations) as MigrateFunctionsObject;
SavedObjectMigrationParams<{ comment?: string }, { comment?: string }>
>(lensMigrationObject, migrateByValueLensVisualizations);

const commentsMigrations = {
'7.11.0': (
Expand Down Expand Up @@ -119,39 +119,54 @@ export const createCommentsMigrations = (
return mergeSavedObjectMigrationMaps(commentsMigrations, embeddableMigrations);
};

export const migrateByValueLensVisualizations =
(migrate: MigrateFunction): SavedObjectMigrationFn<{ comment?: string }, { comment?: string }> =>
(doc: SavedObjectUnsanitizedDoc<{ comment?: string }>, context: SavedObjectMigrationContext) => {
if (doc.attributes.comment == null) {
return doc;
}
export const migrateByValueLensVisualizations = (
migrate: MigrateFunction,
migrationVersion: string
): SavedObjectMigrationParams<{ comment?: string }, { comment?: string }> => {
const deferred = isDeferredMigration(MIN_DEFERRED_KIBANA_VERSION, migrationVersion);

try {
const parsedComment = parseCommentString(doc.attributes.comment);
const migratedComment = parsedComment.children.map((comment) => {
if (isLensMarkdownNode(comment)) {
// casting here because ts complains that comment isn't serializable because LensMarkdownNode
// extends Node which has fields that conflict with SerializableRecord even though it is serializable
return migrate(comment as SerializableRecord) as LensMarkdownNode;
}

return comment;
});

const migratedMarkdown = { ...parsedComment, children: migratedComment };
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

We now change "normal" lens migrations into deferred ones. This would work, but I think it'd be more explicit if the actual lens migration was also forced to set deferred: true.

To do this we'd need to handle migrate being migrate: MigrateFunction | SavedObjectMigrationParams

And then do something like:

if(deferred) { // nit: I would call this variable shouldDefer 
  if(typeof(migrate) == "function" || migrate.deferred === false) { // just for illustration, typescript won't like this
   throw new Error("Cases only allows deferred migrations starting in 8.9.0")
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this with @cnasikas and @stratoula

This would require a small refactor in embeddable migration framework to allow for MigrateFunction | SavedObjectMigrationParams instead of just MigrateFunction. While we ultimately want all embeddable migrations to be deferred we're likely going to start moving to a client-side embeddable migration framework soon #158677

So for now we decided we won't force the lens migration to be deferred: true but only apply that when we migrate the case comments.

// @ts-expect-error: remove when core changes the types
Copy link
Member Author

Choose a reason for hiding this comment

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

@rudolf Is it ok to set it as true when needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, until #152807 is merged there's a caveat: if there's partial updates against this saved object it could cause migrations to run twice (update comment without updating typeVersion) or never run (updating typeVersion without updating comment).

Looking at https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/common/api/cases/comment/index.ts#L250C5-L261 it doesn't seem like we allow partial updates?

Otherwise we're blocked on #152807

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is correct. We do not allow partial updates for comments.

deferred,
transform: (
Copy link
Member Author

Choose a reason for hiding this comment

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

Same function as before. Just set it to the transform attribute.

doc: SavedObjectUnsanitizedDoc<{ comment?: string }>,
context: SavedObjectMigrationContext
) => {
if (doc.attributes.comment == null) {
return doc;
}

return {
...doc,
attributes: {
...doc.attributes,
comment: stringifyCommentWithoutTrailingNewline(doc.attributes.comment, migratedMarkdown),
},
};
} catch (error) {
logError({ id: doc.id, context, error, docType: 'comment', docKey: 'comment' });
return doc;
}
try {
const parsedComment = parseCommentString(doc.attributes.comment);
const migratedComment = parsedComment.children.map((comment) => {
if (isLensMarkdownNode(comment)) {
// casting here because ts complains that comment isn't serializable because LensMarkdownNode
// extends Node which has fields that conflict with SerializableRecord even though it is serializable
return migrate(comment as SerializableRecord) as LensMarkdownNode;
}

return comment;
});

const migratedMarkdown = { ...parsedComment, children: migratedComment };

return {
...doc,
attributes: {
...doc.attributes,
comment: stringifyCommentWithoutTrailingNewline(
doc.attributes.comment,
migratedMarkdown
),
},
};
} catch (error) {
logError({ id: doc.id, context, error, docType: 'comment', docKey: 'comment' });
return doc;
}
},
};
};

export const stringifyCommentWithoutTrailingNewline = (
originalComment: string,
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/cases/server/saved_object_types/migrations/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@ export const GENERATED_ALERT = 'generated_alert';
export const COMMENT_ASSOCIATION_TYPE = 'case';
export const CASE_TYPE_INDIVIDUAL = 'individual';
export const SUB_CASE_SAVED_OBJECT = 'cases-sub-case';

export const MIN_DEFERRED_KIBANA_VERSION = '8.9.0';
47 changes: 36 additions & 11 deletions x-pack/plugins/cases/server/saved_object_types/migrations/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

import type { SavedObjectsMigrationLogger } from '@kbn/core/server';
import { migrationMocks } from '@kbn/core/server/mocks';
import { logError } from './utils';
import { MIN_DEFERRED_KIBANA_VERSION } from './constants';
import { isDeferredMigration, logError } from './utils';

describe('migration utils', () => {
const context = migrationMocks.createContext();
Expand All @@ -16,18 +17,19 @@ describe('migration utils', () => {
jest.clearAllMocks();
});

it('logs an error', () => {
const log = context.log as jest.Mocked<SavedObjectsMigrationLogger>;
describe('logError', () => {
it('logs an error', () => {
const log = context.log as jest.Mocked<SavedObjectsMigrationLogger>;

logError({
id: '1',
context,
error: new Error('an error'),
docType: 'a document',
docKey: 'key',
});
logError({
id: '1',
context,
error: new Error('an error'),
docType: 'a document',
docKey: 'key',
});

expect(log.error.mock.calls[0]).toMatchInlineSnapshot(`
expect(log.error.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Failed to migrate a document with doc id: 1 version: 8.0.0 error: an error",
Object {
Expand All @@ -39,5 +41,28 @@ describe('migration utils', () => {
},
]
`);
});
});

describe('isDeferredMigration', () => {
it('should mark the migration as deferred if the migration version is greater than the Kibana version', () => {
expect(isDeferredMigration(MIN_DEFERRED_KIBANA_VERSION, '8.10.0')).toBe(true);
});

it('should mark the migration as not deferred if the migration version is smaller than the Kibana version', () => {
expect(isDeferredMigration(MIN_DEFERRED_KIBANA_VERSION, '8.8.0')).toBe(false);
});

it('should mark the migration as deferred if the migration version is equal to the Kibana version', () => {
expect(isDeferredMigration(MIN_DEFERRED_KIBANA_VERSION, '8.9.0')).toBe(true);
});

it('should return false if the Kibana version is not a valid semver', () => {
expect(isDeferredMigration('invalid', '8.8.0')).toBe(false);
});

it('should return false if the migration version is not a valid semver', () => {
expect(isDeferredMigration(MIN_DEFERRED_KIBANA_VERSION, 'invalid')).toBe(false);
});
});
});
13 changes: 13 additions & 0 deletions x-pack/plugins/cases/server/saved_object_types/migrations/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* 2.0.
*/

import valid from 'semver/functions/valid';
import gte from 'semver/functions/gte';

import type {
LogMeta,
SavedObjectMigrationContext,
Expand Down Expand Up @@ -50,3 +53,13 @@ export function pipeMigrations<T>(...migrations: Array<CaseMigration<T>>): CaseM
return (doc: SavedObjectUnsanitizedDoc<T>) =>
migrations.reduce((migratedDoc, nextMigration) => nextMigration(migratedDoc), doc);
}

export const isDeferredMigration = (
minDeferredKibanaVersion: string,
migrationVersion: string
): boolean =>
Boolean(
valid(migrationVersion) &&
valid(minDeferredKibanaVersion) &&
gte(migrationVersion, minDeferredKibanaVersion)
);