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

Invalid searchSourceJSON causes saved object migration to fail #78535

Merged
merged 5 commits into from
Sep 30, 2020
Merged

Invalid searchSourceJSON causes saved object migration to fail #78535

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e6475c
Invalid `searchSourceJSON` causes saved object migration to fail
alexwizp Sep 25, 2020
97e2201
Merge remote-tracking branch 'upstream/master' into #78530
alexwizp Sep 28, 2020
b0e1ac7
7.8.2 -> 7.9.3
alexwizp Sep 28, 2020
e4da267
return migration into 6.7.2
alexwizp Sep 30, 2020
00e625f
Merge remote-tracking branch 'upstream/master' into #78530
alexwizp Sep 30, 2020
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
12 changes: 1 addition & 11 deletions src/plugins/dashboard/server/saved_objects/dashboard_migrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,7 @@ const migrations700: SavedObjectMigrationFn<any, any> = (doc): DashboardDoc700To
};

export const dashboardSavedObjectTypeMigrations = {
/**
* We need to have this migration twice, once with a version prior to 7.0.0 once with a version
* after it. The reason for that is, that this migration has been introduced once 7.0.0 was already
* released. Thus a user who already had 7.0.0 installed already got the 7.0.0 migrations below running,
* so we need a version higher than that. But this fix was backported to the 6.7 release, meaning if we
* would only have the 7.0.1 migration in here a user on the 6.7 release will migrate their saved objects
* to the 7.0.1 state, and thus when updating their Kibana to 7.0, will never run the 7.0.0 migrations introduced
* in that version. So we apply this twice, once with 6.7.2 and once with 7.0.1 while the backport to 6.7
* only contained the 6.7.2 migration and not the 7.0.1 migration.
*/
'6.7.2': flow(migrateMatchAllQuery),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we "can" remove (or should) old migrations. The problem is, that depending on from which version you then came, you'll either ran in 6.7.2 over that migration or not. E.g. you were updating from 6.6.0 to 7.6.0 and then 7.10.0, will give you different migrations then upgrading from 6.6.0 to 7.10.0. @rudolf Do you know if this is at all possible? If it's possible I still would like to hear your opinion on if we should do something like that, because this sounds like a horrible source for errors that we'll never be able to find later, since we removed old migrations from the code and won't see it when debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timroes you highlighted a good question in general. It seems to me that wherever we add this migration script, there will still be questions.
Also, analyzing this particular script, I see no problems if it is executed in 7.8. On the other hand, it seems strange to me to add in 7.8 something that is valid for an upgrade from version 5 to 6

From my point of view we should keep version as it (6.7.2) and just a fix reason of an exception:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that this migration fixes a bug introduced in v5 (or a bug in the v6 migration). The version the data bug was introduced is irrelevant, the migration version captures when this migration was applied and the data was transformed and fixed. So a search document with a migrationVersion of 7.9.3 means that this data has been migrated and fixed in Kibana 7.9.3. And then we know that any kibana > 7.9.3 can safely consume this document knowing it has the expected shape.

What complicates matters here is that we first released this migration in v7.8.0 as "6.7.2". As Joe pointed out in the original PR this means that this migration would not have been applied to documents which for instance had already been upgraded to v7.4.0 because we only apply newer migrations. So if someone created a v5 document with this bug (or however you reproduce the original issue) and then upgraded through some path to v7.4.0 this transformation will not be applied when they later upgrade to > v7.8.0

So there are two issues which should be addressed, one is correctly handling invalid JSON, the other is making sure that this migration is applied to all documents. To get this migration applied to all versions we need to set it's version to the oldest Kibana this will get released in (e.g. 7.9.3).

Since the "new" 7.9.3 migration is the same as the 6.7.2 migration, anyone running Kibana > 7.9.3 will always have this migration applied (perhaps it's applied twice, but that's fine).

However removing the migration might still be a bad idea because it makes it harder to find and debug because it's no longer in the master codebase. So I can see some benefit in keeping the 6.7.2 migration just as a historic reference so that if someone ever wonders why a document had a migration applied with v6.7.2 they'll be able to find that migration in the codebase. We would then have two identical migration scripts with different versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other option is to not have duplicate migrations but just have a big comment block in the code above the 7.9.3 migration stating that at some point this was released as a 6.7.2 migration and later changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "traceability" here would outrule that small performance drawback of running that other (same) migration in 6.7.2 and as far as I understand there'll be no other drawback to it, why I'd suggest we're not removing it, but instead keeping it for the old version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I also prefer the better traceability of duplicate migrations.

'7.0.0': flow(migrations700),
'7.3.0': flow(migrations730),
'7.8.2': flow(migrateMatchAllQuery),
rudolf marked this conversation as resolved.
Show resolved Hide resolved
};
17 changes: 17 additions & 0 deletions src/plugins/dashboard/server/saved_objects/migrate_match_all_query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,21 @@ describe('migrate match_all query', () => {
},
});
});

it('should return original doc if searchSourceJSON cannot be parsed', () => {
const migratedDoc = migrateMatchAllQuery(
{
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
} as Parameters<SavedObjectMigrationFn>[0],
savedObjectMigrationContext
);

expect(migratedDoc).toEqual({
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
});
});
});
7 changes: 7 additions & 0 deletions src/plugins/dashboard/server/saved_objects/migrate_match_all_query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ import { SavedObjectMigrationFn } from 'kibana/server';
import { get } from 'lodash';
import { DEFAULT_QUERY_LANGUAGE } from '../../../data/common';

/**
* This migration script is related to:
* @link https://github.com/elastic/kibana/pull/62194
* @link https://github.com/elastic/kibana/pull/14644
* This is only a problem when you import an object from 5.x into 6.x but to be sure that all saved objects migrated that script was added into 7.8.2
*/
export const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
const searchSourceJSON = get(doc, 'attributes.kibanaSavedObjectMeta.searchSourceJSON');

Expand All @@ -31,6 +37,7 @@ export const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
searchSource = JSON.parse(searchSourceJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
return doc;
alexwizp marked this conversation as resolved.
Show resolved Hide resolved
}

if (searchSource.query?.match_all) {
Expand Down
83 changes: 51 additions & 32 deletions src/plugins/discover/server/saved_objects/search_migrations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,38 +23,6 @@ import { searchMigrations } from './search_migrations';
const savedObjectMigrationContext = (null as unknown) as SavedObjectMigrationContext;

describe('migration search', () => {
describe('6.7.2', () => {
const migrationFn = searchMigrations['6.7.2'];

it('should migrate obsolete match_all query', () => {
const migratedDoc = migrationFn(
{
type: 'search',
attributes: {
kibanaSavedObjectMeta: {
searchSourceJSON: JSON.stringify({
query: {
match_all: {},
},
}),
},
},
},
savedObjectMigrationContext
);
const migratedSearchSource = JSON.parse(
migratedDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON
);

expect(migratedSearchSource).toEqual({
query: {
query: '',
language: 'kuery',
},
});
});
});

describe('7.0.0', () => {
const migrationFn = searchMigrations['7.0.0'];

Expand Down Expand Up @@ -328,4 +296,55 @@ Object {
expect(migratedDoc).toEqual(doc);
});
});

describe('7.8.2', () => {
const migrationFn = searchMigrations['7.8.2'];

it('should migrate obsolete match_all query', () => {
const migratedDoc = migrationFn(
{
type: 'search',
attributes: {
kibanaSavedObjectMeta: {
searchSourceJSON: JSON.stringify({
query: {
match_all: {},
},
}),
},
},
},
savedObjectMigrationContext
);
const migratedSearchSource = JSON.parse(
migratedDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON
);

expect(migratedSearchSource).toEqual({
query: {
query: '',
language: 'kuery',
},
});
});

it('should return original doc if searchSourceJSON cannot be parsed', () => {
const migratedDoc = migrationFn(
{
type: 'search',
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
},
savedObjectMigrationContext
);

expect(migratedDoc).toEqual({
type: 'search',
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
});
});
});
});
9 changes: 8 additions & 1 deletion src/plugins/discover/server/saved_objects/search_migrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ import { flow, get } from 'lodash';
import { SavedObjectMigrationFn } from 'kibana/server';
import { DEFAULT_QUERY_LANGUAGE } from '../../../data/common';

/**
* This migration script is related to:
* @link https://github.com/elastic/kibana/pull/62194
* @link https://github.com/elastic/kibana/pull/14644
* This is only a problem when you import an object from 5.x into 6.x but to be sure that all saved objects migrated that script was added into 7.8.2
*/
const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
const searchSourceJSON = get(doc, 'attributes.kibanaSavedObjectMeta.searchSourceJSON');

Expand All @@ -31,6 +37,7 @@ const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
searchSource = JSON.parse(searchSourceJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
return doc;
}

if (searchSource.query?.match_all) {
Expand Down Expand Up @@ -122,7 +129,7 @@ const migrateSearchSortToNestedArray: SavedObjectMigrationFn<any, any> = (doc) =
};

export const searchMigrations = {
'6.7.2': flow(migrateMatchAllQuery),
'7.0.0': flow(setNewReferences),
'7.4.0': flow(migrateSearchSortToNestedArray),
'7.8.2': flow(migrateMatchAllQuery),
};
77 changes: 51 additions & 26 deletions src/plugins/visualizations/server/saved_objects/visualization_migrations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,32 +150,6 @@ describe('migration visualization', () => {
expect(aggs[3]).not.toHaveProperty('params.customBucket.params.time_zone');
expect(aggs[2]).not.toHaveProperty('params.time_zone');
});

it('should migrate obsolete match_all query', () => {
const migratedDoc = migrate({
...doc,
attributes: {
...doc.attributes,
kibanaSavedObjectMeta: {
searchSourceJSON: JSON.stringify({
query: {
match_all: {},
},
}),
},
},
});
const migratedSearchSource = JSON.parse(
migratedDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON
);

expect(migratedSearchSource).toEqual({
query: {
query: '',
language: 'kuery',
},
});
});
});
});

Expand Down Expand Up @@ -1487,6 +1461,57 @@ describe('migration visualization', () => {
});
});

describe('7.8.2', () => {
const migrationFn = visualizationSavedObjectTypeMigrations['7.8.2'];

it('should migrate obsolete match_all query', () => {
const migratedDoc = migrationFn(
{
type: 'area',
attributes: {
kibanaSavedObjectMeta: {
searchSourceJSON: JSON.stringify({
query: {
match_all: {},
},
}),
},
},
},
savedObjectMigrationContext
);
const migratedSearchSource = JSON.parse(
migratedDoc.attributes.kibanaSavedObjectMeta.searchSourceJSON
);

expect(migratedSearchSource).toEqual({
query: {
query: '',
language: 'kuery',
},
});
});

it('should return original doc if searchSourceJSON cannot be parsed', () => {
const migratedDoc = migrationFn(
{
type: 'area',
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
},
savedObjectMigrationContext
);

expect(migratedDoc).toEqual({
type: 'area',
attributes: {
kibanaSavedObjectMeta: 'kibanaSavedObjectMeta',
},
});
});
});

describe('7.8.0 tsvb split_color_mode', () => {
const migrate = (doc: any) =>
visualizationSavedObjectTypeMigrations['7.8.0'](
Expand Down
10 changes: 9 additions & 1 deletion src/plugins/visualizations/server/saved_objects/visualization_migrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,12 @@ const migrateTableSplits: SavedObjectMigrationFn<any, any> = (doc) => {
}
};

/**
* This migration script is related to:
* @link https://github.com/elastic/kibana/pull/62194
* @link https://github.com/elastic/kibana/pull/14644
* This is only a problem when you import an object from 5.x into 6.x but to be sure that all saved objects migrated that script was added into 7.8.2
*/
const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
const searchSourceJSON = get(doc, 'attributes.kibanaSavedObjectMeta.searchSourceJSON');

Expand All @@ -665,6 +671,7 @@ const migrateMatchAllQuery: SavedObjectMigrationFn<any, any> = (doc) => {
searchSource = JSON.parse(searchSourceJSON);
} catch (e) {
// Let it go, the data is invalid and we'll leave it as is
return doc;
}

if (searchSource.query?.match_all) {
Expand Down Expand Up @@ -761,7 +768,7 @@ export const visualizationSavedObjectTypeMigrations = {
* in that version. So we apply this twice, once with 6.7.2 and once with 7.0.1 while the backport to 6.7
* only contained the 6.7.2 migration and not the 7.0.1 migration.
*/
'6.7.2': flow(migrateMatchAllQuery, removeDateHistogramTimeZones),
'6.7.2': flow(removeDateHistogramTimeZones),
'7.0.0': flow(
addDocReferences,
migrateIndexPattern,
Expand All @@ -781,5 +788,6 @@ export const visualizationSavedObjectTypeMigrations = {
'7.4.2': flow(transformSplitFiltersStringToQueryObject),
'7.7.0': flow(migrateOperatorKeyTypo, migrateSplitByChartRow),
'7.8.0': flow(migrateTsvbDefaultColorPalettes),
'7.8.2': flow(migrateMatchAllQuery),
'7.10.0': flow(migrateFilterRatioQuery, removeTSVBSearchSource),
};