Invalid searchSourceJSON causes saved object migration to fail
#78535
Conversation
alexwizp
added
v7.8.2
v7.9.3
v7.10.0
v8.0.0
Team:Visualizations
src/plugins/visualizations/server/saved_objects/visualization_migrations.ts
Outdated
Show resolved
Hide resolved
alexwizp
requested review from
stratoula,
flash1293,
VladLasitsa,
DianaDerevyankina and
sulemanof
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
| * 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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, I also prefer the better traceability of duplicate migrations.
src/plugins/dashboard/server/saved_objects/dashboard_migrations.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code LGTM, didn't run. Thanks for following up on this @rudolf
A nice addition to https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrations/README.md would be a guide with examples on how to write migrations (don't change old migrations, always add to the next version, ...)
I've created https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_writing_migrations and added some of the lessons we learned from this issue. I'll add something about never removing old migrations and if fixing a bug in an old migration, always release it as a duplicate in the current version. I should also link to the new docs from https://github.com/elastic/kibana/blob/master/src/core/server/saved_objects/migrations/README.md so help people find it. |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…stic#78535) * Invalid `searchSourceJSON` causes saved object migration to fail Closes: elastic#78530 * 7.8.2 -> 7.9.3 * return migration into 6.7.2
…stic#78535) * Invalid `searchSourceJSON` causes saved object migration to fail Closes: elastic#78530 * 7.8.2 -> 7.9.3 * return migration into 6.7.2 # Conflicts: # src/plugins/visualizations/server/saved_objects/visualization_migrations.ts
…aly-detection-partition-field * 'master' of github.com:elastic/kibana: (37 commits) [UiActions] Don't throw an error if there are no compatible actions to execute (elastic#78917) [Observability] Kibana nav when docked overlaps the content of the pages. (elastic#78593) Invalid `searchSourceJSON` causes saved object migration to fail (elastic#78535) update vega version (elastic#78390) Fix warning text doesn't get displayed on filters with custom filter name (elastic#78617) [ILM] Data tier notices should reflect tier preferences (elastic#78398) [APM] Service Map: `Not Defined` option doesn't work properly (elastic#77757) Improve invalid field error message at search.aggs.param_types.field (elastic#78587) Remove isDeprecated flag on visType (elastic#78820) Remove unused elasticsearch.preserverHost setting (elastic#78608) Fix condition and adjust tests (elastic#78898) [UX] Add percentile selector (elastic#78562) [ML] Replace use of rest_total_hits_as_int with track_total_hits (elastic#78423) expression service docs (elastic#78774) [Functional] Wait for the page to load and then click the new vis button (elastic#78725) [TSVB] No data in visualizations with annotations (elastic#78794) [kbn/ui-shared-deps] track asset sizes (elastic#78718) delete target before building (elastic#78665) Move indexPattern.popularizeField into discover (elastic#77668) [Security Solution][Resolver]Add backdrop to pills (elastic#78625) ...
Closes: #78530
Summary
Migration script (migrateMatchAllQuery) was added into
7.9.3Checklist
Delete any items that are not applicable to this PR.
For maintainers