From 060dff5348324368f4d1cde6bcd80289a1b48cf5 Mon Sep 17 00:00:00 2001 From: Joe Portner Date: Thu, 2 Dec 2021 14:36:46 -0500 Subject: [PATCH] Remove support for migrationVersion leapfrogging --- dev_docs/tutorials/saved_objects.mdx | 2 + .../core/saved-objects-service.asciidoc | 3 ++ .../migrations/core/document_migrator.test.ts | 52 ------------------- .../migrations/core/document_migrator.ts | 16 +----- 4 files changed, 6 insertions(+), 67 deletions(-) diff --git a/dev_docs/tutorials/saved_objects.mdx b/dev_docs/tutorials/saved_objects.mdx index 9583e195d1c82..a9d8cd7c6ec1c 100644 --- a/dev_docs/tutorials/saved_objects.mdx +++ b/dev_docs/tutorials/saved_objects.mdx @@ -252,6 +252,8 @@ Having said that, if a document is encountered that is not in the expected shape fail an upgrade than to silently ignore a corrupt document which can cause unexpected behaviour at some future point in time. When such a scenario is encountered, the error should be verbose and informative so that the corrupt document can be corrected, if possible. +**WARNING:** Do not attempt to change the `migrationVersion`, `id`, or `type` fields within a migration function, this is not supported. + ### Testing Migrations Bugs in a migration function cause downtime for our users and therefore have a very high impact. Follow the . diff --git a/docs/developer/architecture/core/saved-objects-service.asciidoc b/docs/developer/architecture/core/saved-objects-service.asciidoc index a7ce86ea46359..54a5c319c6222 100644 --- a/docs/developer/architecture/core/saved-objects-service.asciidoc +++ b/docs/developer/architecture/core/saved-objects-service.asciidoc @@ -259,6 +259,9 @@ upgrade. In most scenarios, it is better to fail an upgrade than to silently ignore a corrupt document which can cause unexpected behaviour at some future point in time. +WARNING: Do not attempt to change the `migrationVersion`, `id`, or `type` fields +within a migration function, this is not supported. + It is critical that you have extensive tests to ensure that migrations behave as expected with all possible input documents. Given how simple it is to test all the branch conditions in a migration function and the high impact of a bug diff --git a/src/core/server/saved_objects/migrations/core/document_migrator.test.ts b/src/core/server/saved_objects/migrations/core/document_migrator.test.ts index 58bda057d7ab9..f92d505c058ed 100644 --- a/src/core/server/saved_objects/migrations/core/document_migrator.test.ts +++ b/src/core/server/saved_objects/migrations/core/document_migrator.test.ts @@ -664,58 +664,6 @@ describe('DocumentMigrator', () => { ); }); - it('allows updating a migrationVersion prop to a later version', () => { - const migrator = new DocumentMigrator({ - ...testOpts(), - typeRegistry: createRegistry({ - name: 'cat', - namespaceType: 'multiple', - migrations: { - '1.0.0': setAttr('migrationVersion.cat', '2.9.1'), - '2.0.0': () => { - throw new Error('POW!'); - }, - '2.9.1': () => { - throw new Error('BANG!'); - }, - '3.0.0': setAttr('attributes.name', 'Shiny'), - }, - convertToMultiNamespaceTypeVersion: '2.9.0', // intentionally less than 2.9.1 - }), - }); - migrator.prepareMigrations(); - const actual = migrator.migrate({ - id: 'smelly', - type: 'cat', - attributes: { name: 'Boo' }, - migrationVersion: { cat: '0.5.6' }, - coreMigrationVersion: undefined, // this is intentional - }); - // Transforms include, in order: - // 1. migration [1.0.0] - // 2. migration [2.0.0] - // 3. reference [2.9.0] - // 4. conversion [2.9.0] - // 5. migration [2.9.1] - // 6. migration [3.0.0] - // The outer loop (`applyMigrations`) enumerates all properties on the object to see if the object is outdated, and which transforms - // need to be applied for each property. - // Initially, all transforms are applicable. An inner loop starts to apply each of them. Transform (1) gets applied, which increases - // the document's migrationVersion. We detect this before applying transform (2) and break out of the inner loop. - // The outer loop continues, and it sees that the object is still outdated and that transforms (3) and (6) need to be applied. Another - // inner loop starts to apply each of these. Transform (3) is correctly applied (there are no references to update, but the - // object's coreMigrationVersion is set to 2.9.0). Then transform (6) is applied, and the inner loop ends. The outer loop detects that - // the object is no longer outdated, and the outer loop ends, which sets the object's coreMigrationVersion to the current Kibana - // version (3.0.0). - expect(actual).toEqual({ - id: 'smelly', - type: 'cat', - attributes: { name: 'Shiny' }, - migrationVersion: { cat: '3.0.0' }, - coreMigrationVersion: kibanaVersion, - }); - }); - it('allows adding props to migrationVersion', () => { const migrator = new DocumentMigrator({ ...testOpts(), diff --git a/src/core/server/saved_objects/migrations/core/document_migrator.ts b/src/core/server/saved_objects/migrations/core/document_migrator.ts index b405497eeed74..5f2870fb6e244 100644 --- a/src/core/server/saved_objects/migrations/core/document_migrator.ts +++ b/src/core/server/saved_objects/migrations/core/document_migrator.ts @@ -27,15 +27,7 @@ * handle property addition / deletion / renaming. * * A caveat is that this means we must restrict what a migration can do to the doc's - * migrationVersion itself. We allow only these kinds of changes: - * - * - Add a new property to migrationVersion - * - Move a migrationVersion property forward to a later version - * - * Migrations *cannot* move a migrationVersion property backwards (e.g. from 2.0.0 to 1.0.0), and they - * cannot clear a migrationVersion property, as allowing either of these could produce infinite loops. - * However, we do wish to allow migrations to modify migrationVersion if they wish, so that - * they could transform a type from "foo 1.0.0" to "bar 3.0.0". + * migrationVersion itself. Migrations should *not* make any changes to the migrationVersion property. * * One last gotcha is that any docs which have no migrationVersion are assumed to be up-to-date. * This is because Kibana UI and other clients really can't be expected build the migrationVersion @@ -753,12 +745,6 @@ function migrateProp( let additionalDocs: SavedObjectUnsanitizedDoc[] = []; for (const { version, transform, transformType } of applicableTransforms(migrations, doc, prop)) { - const currentVersion = propVersion(doc, prop); - if (currentVersion && Semver.gt(currentVersion, version) && transformType !== 'reference') { - // the previous transform function increased the object's migrationVersion; break out of the loop - break; - } - if (convertNamespaceTypes || (transformType !== 'convert' && transformType !== 'reference')) { // migrate transforms are always applied, but conversion transforms and reference transforms are only applied during index migrations const result = transform(doc);