[8.0] Prevent endless loop for saved object migrations (#120146) (#120350)

* Prevent endless loop for saved object migrations (#120146)

* Fix invalid saved objects

* Fix more invalid saved objects

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 <DocLink id="kibDevTutorialTestingPlugins" section="saved-objects-migrations" text="Saved Object migrations section in the plugin testing guide"/>.

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

src/core/server/saved_objects/migrations/core/document_migrator.test.ts

@@ -664,39 +664,6 @@ describe('DocumentMigrator', () => {
       );
     });
 
-    it('allows updating a migrationVersion prop to a later version', () => {
-      const migrator = new DocumentMigrator({
-        ...testOpts(),
-        typeRegistry: createRegistry({
-          name: 'cat',
-          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'),
-          },
-        }),
-      });
-      migrator.prepareMigrations();
-      const actual = migrator.migrate({
-        id: 'smelly',
-        type: 'cat',
-        attributes: { name: 'Boo' },
-        migrationVersion: { cat: '0.5.6' },
-      });
-      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(),
@@ -1072,7 +1039,8 @@ describe('DocumentMigrator', () => {
               name: 'dog',
               namespaceType: 'single',
               migrations: {
-                '1.0.0': setAttr('migrationVersion.dog', '2.0.0'),
+                '1.1.0': setAttr('attributes.age', '12'),
+                '1.5.0': setAttr('attributes.color', 'tri-color'),
                 '2.0.0': (doc) => doc, // noop
               },
             },
@@ -1083,9 +1051,10 @@ describe('DocumentMigrator', () => {
         const obj = {
           id: 'sleepy',
           type: 'dog',
-          attributes: { name: 'Patches' },
-          migrationVersion: {},
+          attributes: { name: 'Patches', age: '11' },
+          migrationVersion: { dog: '1.1.0' }, // skip the first migration transform, only apply the second and third
           references: [{ id: 'favorite', type: 'toy', name: 'BALL!' }],
+          coreMigrationVersion: undefined, // this is intentional
         };
 
         it('in the default space', () => {
@@ -1095,7 +1064,7 @@ describe('DocumentMigrator', () => {
             {
               id: 'sleepy',
               type: 'dog',
-              attributes: { name: 'Patches' },
+              attributes: { name: 'Patches', age: '11', color: 'tri-color' },
               migrationVersion: { dog: '2.0.0' },
               references: [{ id: 'favorite', type: 'toy', name: 'BALL!' }], // no change
               coreMigrationVersion: kibanaVersion,
@@ -1111,7 +1080,7 @@ describe('DocumentMigrator', () => {
             {
               id: 'sleepy',
               type: 'dog',
-              attributes: { name: 'Patches' },
+              attributes: { name: 'Patches', age: '11', color: 'tri-color' },
               migrationVersion: { dog: '2.0.0' },
               references: [{ id: 'uuidv5', type: 'toy', name: 'BALL!' }], // changed
               coreMigrationVersion: kibanaVersion,

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)) {
-      // 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);

src/core/server/saved_objects/service/lib/repository.test.ts

@@ -976,8 +976,9 @@ describe('SavedObjectsRepository', () => {
     describe('migration', () => {
       it(`migrates the docs and serializes the migrated docs`, async () => {
         migrator.migrateDocument.mockImplementation(mockMigrateDocument);
-        await bulkCreateSuccess([obj1, obj2]);
-        const docs = [obj1, obj2].map((x) => ({ ...x, ...mockTimestampFields }));
+        const modifiedObj1 = { ...obj1, coreMigrationVersion: '8.0.0' };
+        await bulkCreateSuccess([modifiedObj1, obj2]);
+        const docs = [modifiedObj1, obj2].map((x) => ({ ...x, ...mockTimestampFields }));
         expectMigrationArgs(docs[0], true, 1);
         expectMigrationArgs(docs[1], true, 2);
 
@@ -2556,8 +2557,22 @@ describe('SavedObjectsRepository', () => {
 
       it(`migrates a document and serializes the migrated doc`, async () => {
         const migrationVersion = mockMigrationVersion;
-        await createSuccess(type, attributes, { id, references, migrationVersion });
-        const doc = { type, id, attributes, references, migrationVersion, ...mockTimestampFields };
+        const coreMigrationVersion = '8.0.0';
+        await createSuccess(type, attributes, {
+          id,
+          references,
+          migrationVersion,
+          coreMigrationVersion,
+        });
+        const doc = {
+          type,
+          id,
+          attributes,
+          references,
+          migrationVersion,
+          coreMigrationVersion,
+          ...mockTimestampFields,
+        };
         expectMigrationArgs(doc);
 
         const migratedDoc = migrator.migrateDocument(doc);

src/core/server/saved_objects/service/lib/repository.ts

@@ -305,6 +305,7 @@ export class SavedObjectsRepository {
     const {
       id = SavedObjectsUtils.generateId(),
       migrationVersion,
+      coreMigrationVersion,
       overwrite = false,
       references = [],
       refresh = DEFAULT_REFRESH_SETTING,
@@ -359,6 +360,7 @@ export class SavedObjectsRepository {
       originId,
       attributes,
       migrationVersion,
+      coreMigrationVersion,
       updated_at: time,
       ...(Array.isArray(references) && { references }),
     });
@@ -523,6 +525,7 @@ export class SavedObjectsRepository {
             type: object.type,
             attributes: object.attributes,
             migrationVersion: object.migrationVersion,
+            coreMigrationVersion: object.coreMigrationVersion,
             ...(savedObjectNamespace && { namespace: savedObjectNamespace }),
             ...(savedObjectNamespaces && { namespaces: savedObjectNamespaces }),
             updated_at: time,

x-pack/test/functional/fixtures/kbn_archiver/maps.json

@@ -752,7 +752,7 @@
     "title":"MVT geotile grid (style meta from local - count)",
     "uiStateJSON":"{\"isLayerTOCOpen\":true,\"openTOCDetails\":[\"g1xkv\"]}"
   },
-  "coreMigrationVersion":"8.1.0",
+  "coreMigrationVersion":"8.0.0",
   "id":"943443a0-3b48-11ec-8a0d-af01166a5cc3",
   "migrationVersion": {
     "map":"8.0.0"
@@ -777,7 +777,7 @@
     "title":"MVT geotile grid (style meta from local - metric)",
     "uiStateJSON":"{\"isLayerTOCOpen\":true,\"openTOCDetails\":[\"g1xkv\"]}"
   },
-  "coreMigrationVersion":"8.1.0",
+  "coreMigrationVersion":"8.0.0",
   "id":"9ff6f170-3b56-11ec-9cfb-57b0ede90800",
   "migrationVersion": {
     "map":"8.0.0"
@@ -919,7 +919,7 @@
     "title": "MVT documents",
     "uiStateJSON": "{\"isLayerTOCOpen\":true,\"openTOCDetails\":[]}"
   },
-  "coreMigrationVersion": "8.1.0",
+  "coreMigrationVersion": "8.0.0",
   "id": "2aff3160-3d78-11ec-9b35-f52e723e8a71",
   "migrationVersion": {
     "map": "8.0.0"

x-pack/test/functional/fixtures/kbn_archiver/reporting/logs.json

@@ -429,7 +429,7 @@
     "variables": [],
     "width": 1080
   },
-  "coreMigrationVersion": "8.1.0",
+  "coreMigrationVersion": "8.0.0",
   "id": "workpad-e7464259-0b75-4b8c-81c8-8422b15ff201",
   "migrationVersion": {
     "canvas-workpad": "8.0.0"