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

[Saved object migrations] Collect all documents that fail to transform before stopping the migration #96986

Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
db8feb8
Initial notes
TinaHeiligers Apr 6, 2021
afd4649
Refactoring to add new state and state transforms
TinaHeiligers Apr 9, 2021
96c2901
Converts migrateRawDocsNonThrowing into a TaskEither implementation
TinaHeiligers Apr 12, 2021
b0eb13f
Adds unit tests for model -> OUTDATED_DOCUMENTS_TRANSFORM and TRANSFO…
TinaHeiligers Apr 13, 2021
0b14388
Keeps track of transform scripts failing during migrations
TinaHeiligers Apr 19, 2021
8714263
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 21, 2021
1f7a389
Factors out common functionality in migrate_raw_docs, adds state type…
TinaHeiligers Apr 21, 2021
d7ba06c
Cleans up more code comments
TinaHeiligers Apr 22, 2021
bc949bb
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 22, 2021
6dd4d89
Formats transformation errors reason for migration failing better, sp…
TinaHeiligers Apr 22, 2021
f798053
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 23, 2021
2f109a6
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 26, 2021
e804e74
Works collecting document transform failures and issues into current …
TinaHeiligers Apr 26, 2021
beee25e
Refactors model.test.ts
TinaHeiligers Apr 26, 2021
9d7f824
Refactors unit and some integration tests with new control flow
TinaHeiligers Apr 27, 2021
738f5ae
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 27, 2021
5131c41
Code comment, update to run ci as an initial test
TinaHeiligers Apr 27, 2021
9b66353
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers Apr 29, 2021
b3ae792
Updates error message phrase
TinaHeiligers Apr 30, 2021
e87d0e9
Merge branch 'master' of github.com:elastic/kibana into so-migrations…
TinaHeiligers May 2, 2021
736df8c
Merge branch 'master' of github.com:elastic/kibana into so-migrations…
TinaHeiligers May 3, 2021
f697729
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers May 4, 2021
c909b3a
Adds check on left response condition for READINDEX_SOURCE_TO_TEMP_INDEX
TinaHeiligers May 4, 2021
d11fd0a
Updates model unit tests
TinaHeiligers May 4, 2021
c3081e3
Cleanup
TinaHeiligers May 4, 2021
8360a1b
changes type to interface
TinaHeiligers May 5, 2021
2a6675c
Addresses minor change requests
TinaHeiligers May 5, 2021
8d16852
Adds TransformationError stack trace to transformation errors migrati…
TinaHeiligers May 5, 2021
2b31763
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
TinaHeiligers May 6, 2021
1765513
Ensures model treats response from Actions.bulkOverwriteTransformedDo…
TinaHeiligers May 6, 2021
9f8e432
Adds the saved object id to the transform error object regardless of …
TinaHeiligers May 6, 2021
0f4c497
Addresses remaining smaler PR change requests
TinaHeiligers May 6, 2021
9bb9ec1
Deletes commented out code
TinaHeiligers May 6, 2021
22a6ca3
Error message format changes and updates to snapshots
TinaHeiligers May 6, 2021
c0f0ef6
Merge branch 'master' into so-migrations/collect-failing-docs-WIP
kibanamachine May 9, 2021
1814c87
Adds integration test for collecting document migration failures acro…
TinaHeiligers May 9, 2021
671b286
Updates expected test string
TinaHeiligers May 9, 2021
39f9f28
Addresses minor changes requested
TinaHeiligers May 10, 2021
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 @@ -11,6 +11,7 @@ import { set } from '@elastic/safer-lodash-set';
import _ from 'lodash';
import { SavedObjectUnsanitizedDoc } from '../../serialization';
import { DocumentMigrator } from './document_migrator';
import { TransformSavedObjectDocumentError } from './transform_saved_object_document_error';
import { loggingSystemMock } from '../../../logging/logging_system.mock';
import { SavedObjectsType } from '../../types';
import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry';
Expand Down Expand Up @@ -724,6 +725,12 @@ describe('DocumentMigrator', () => {

it('logs the original error and throws a transform error if a document transform fails', () => {
const log = mockLogger;
const failedDoc = {
id: 'smelly',
type: 'dog',
attributes: {},
migrationVersion: {},
};
const migrator = new DocumentMigrator({
...testOpts(),
typeRegistry: createRegistry({
Expand All @@ -737,12 +744,6 @@ describe('DocumentMigrator', () => {
log,
});
migrator.prepareMigrations();
const failedDoc = {
id: 'smelly',
type: 'dog',
attributes: {},
migrationVersion: {},
};
try {
migrator.migrate(_.cloneDeep(failedDoc));
expect('Did not throw').toEqual('But it should have!');
Expand All @@ -751,6 +752,7 @@ describe('DocumentMigrator', () => {
"Failed to transform document smelly. Transform: dog:1.2.3
Doc: {\\"id\\":\\"smelly\\",\\"type\\":\\"dog\\",\\"attributes\\":{},\\"migrationVersion\\":{}}"
`);
expect(error).toBeInstanceOf(TransformSavedObjectDocumentError);
expect(loggingSystemMock.collect(mockLoggerFactory).error[0][0]).toMatchInlineSnapshot(
`[Error: Dang diggity!]`
);
Expand Down
13 changes: 10 additions & 3 deletions src/core/server/saved_objects/migrations/core/document_migrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import {
SavedObjectsType,
} from '../../types';
import { MigrationLogger } from './migration_logger';
import { TransformSavedObjectDocumentError } from '.';
import { ISavedObjectTypeRegistry } from '../../saved_objects_type_registry';
import { SavedObjectMigrationFn, SavedObjectMigrationMap } from '../types';
import { DEFAULT_NAMESPACE_STRING } from '../../service/lib/utils';
Expand Down Expand Up @@ -679,9 +680,15 @@ function wrapWithTry(
const failedTransform = `${type.name}:${version}`;
const failedDoc = JSON.stringify(doc);
log.error(error);

throw new Error(
`Failed to transform document ${doc?.id}. Transform: ${failedTransform}\nDoc: ${failedDoc}`
// To make debugging failed migrations easier, we add items needed to convert the
// saved object id to the full raw id (the id only contains the uuid part) and the full error itself
throw new TransformSavedObjectDocumentError(
doc.id,
doc.type,
doc.namespace ?? undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why ?? undefined, is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A saved object might be multi-namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we need it: undefined will be used when doc.namespace is undefined. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You raise a very good point! 🤦‍♀️ I've fixed it.

failedTransform,
failedDoc,
error
);
}
};
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/saved_objects/migrations/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,9 @@ export type { MigrationResult, MigrationStatus } from './migration_coordinator';
export { createMigrationEsClient } from './migration_es_client';
export type { MigrationEsClient } from './migration_es_client';
export { excludeUnusedTypesQuery } from './elastic_index';
export { TransformSavedObjectDocumentError } from './transform_saved_object_document_error';
export type {
DocumentsTransformFailed,
DocumentsTransformSuccess,
TransformErrorObjects,
} from './migrate_raw_docs';
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import { set } from '@elastic/safer-lodash-set';
import _ from 'lodash';
import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry';
import { SavedObjectsSerializer } from '../../serialization';
import { migrateRawDocs } from './migrate_raw_docs';
import { migrateRawDocs, migrateRawDocsSafely } from './migrate_raw_docs';
import { TransformSavedObjectDocumentError } from './transform_saved_object_document_error';

describe('migrateRawDocs', () => {
test('converts raw docs to saved objects', async () => {
Expand Down Expand Up @@ -120,3 +121,156 @@ describe('migrateRawDocs', () => {
).rejects.toThrowErrorMatchingInlineSnapshot(`"error during transform"`);
});
});

describe('migrateRawDocsSafely', () => {
beforeEach(() => {
jest.clearAllMocks();
});

test('converts raw docs to saved objects', async () => {
let result: any;
const transform = jest.fn<any, any>((doc: any) => [
set(_.cloneDeep(doc), 'attributes.name', 'HOI!'),
]);
const task = migrateRawDocsSafely(
new SavedObjectsSerializer(new SavedObjectTypeRegistry()),
transform,
[
{ _id: 'a:b', _source: { type: 'a', a: { name: 'AAA' } } },
{ _id: 'c:d', _source: { type: 'c', c: { name: 'DDD' } } },
]
);
try {
result = await task();
} catch (e) {
/** ignore */
}
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
expect(result._tag).toEqual('Right');
expect(result.right.processedDocs).toEqual([
{
_id: 'a:b',
_source: { type: 'a', a: { name: 'HOI!' }, migrationVersion: {}, references: [] },
},
{
_id: 'c:d',
_source: { type: 'c', c: { name: 'HOI!' }, migrationVersion: {}, references: [] },
},
]);

const obj1 = {
id: 'b',
type: 'a',
attributes: { name: 'AAA' },
migrationVersion: {},
references: [],
};
const obj2 = {
id: 'd',
type: 'c',
attributes: { name: 'DDD' },
migrationVersion: {},
references: [],
};
expect(transform).toHaveBeenCalledTimes(2);
expect(transform).toHaveBeenNthCalledWith(1, obj1);
expect(transform).toHaveBeenNthCalledWith(2, obj2);
});

test('returns a `left` tag when encountering a corrupt saved object document', async () => {
let result: any;
const transform = jest.fn<any, any>((doc: any) => [
set(_.cloneDeep(doc), 'attributes.name', 'TADA'),
]);
const task = migrateRawDocsSafely(
new SavedObjectsSerializer(new SavedObjectTypeRegistry()),
transform,
[
{ _id: 'foo:b', _source: { type: 'a', a: { name: 'AAA' } } },
{ _id: 'c:d', _source: { type: 'c', c: { name: 'DDD' } } },
]
);
try {
result = await task();
} catch (e) {
result = e;
}
expect(transform).toHaveBeenCalledTimes(1);
expect(result._tag).toEqual('Left');
expect(Object.keys(result.left)).toEqual(['type', 'corruptDocumentIds', 'transformErrors']);
expect(result.left.corruptDocumentIds.length).toEqual(1);
expect(result.left.transformErrors.length).toEqual(0);
});

test('handles when one document is transformed into multiple documents', async () => {
let result: any;
const transform = jest.fn<any, any>((doc: any) => [
set(_.cloneDeep(doc), 'attributes.name', 'HOI!'),
{ id: 'bar', type: 'foo', attributes: { name: 'baz' } },
]);
const task = migrateRawDocsSafely(
new SavedObjectsSerializer(new SavedObjectTypeRegistry()),
transform,
[{ _id: 'a:b', _source: { type: 'a', a: { name: 'AAA' } } }]
);
try {
result = await task();
} catch (err) {
/** ignore */
}
expect(result._tag).toEqual('Right');
expect(result.right.processedDocs).toEqual([
{
_id: 'a:b',
_source: { type: 'a', a: { name: 'HOI!' }, migrationVersion: {}, references: [] },
},
{
_id: 'foo:bar',
_source: { type: 'foo', foo: { name: 'baz' }, references: [] },
},
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
]);

const obj = {
id: 'b',
type: 'a',
attributes: { name: 'AAA' },
migrationVersion: {},
references: [],
};
expect(transform).toHaveBeenCalledTimes(1);
expect(transform).toHaveBeenCalledWith(obj);
});

test('rejects when the transform function throws an error', async () => {
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
let result: any;
const transform = jest.fn<any, any>((doc: any) => {
throw new TransformSavedObjectDocumentError(
`${doc.id}`,
`${doc.type}`,
`${doc.namespace}`,
`${doc.type}1.2.3`,
JSON.stringify(doc),
new Error('error during transform')
);
});
const task = migrateRawDocsSafely(
new SavedObjectsSerializer(new SavedObjectTypeRegistry()),
transform,
[{ _id: 'a:b', _source: { type: 'a', a: { name: 'AAA' } } }] // this is the raw doc
);
try {
result = await task();
} catch (err) {
/* ignore */
}
expect(transform).toHaveBeenCalledTimes(1);
expect(result._tag).toEqual('Left');
expect(result.left.corruptDocumentIds.length).toEqual(0);
expect(result.left.transformErrors.length).toEqual(1);
expect(result.left.transformErrors[0].err.message).toMatchInlineSnapshot(
`
"Failed to transform document b. Transform: a1.2.3
Doc: {\\"type\\":\\"a\\",\\"id\\":\\"b\\",\\"attributes\\":{\\"name\\":\\"AAA\\"},\\"references\\":[],\\"migrationVersion\\":{}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

TransformSavedObjectDocumentError shows the problem SO, but not the source of the problem. Doesn't it? Let's refactor it to include the original error message as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the original error is included in the transformation error as originalError.

`
);
});
});
Loading