Prevent endless loop for saved object migrations

[jportner]

Resolves #120129.

This changes the DocumentMigrator behavior so that reference transforms are not accidentally skipped when an object has a higher migrationVersion than its coreMigrationVersion.

I'm labeling this as release_note:skip because it's fixing an unreleased bug.

[jportner]

This fixes the bug at hand, but maybe we want to change more than this.

I mentioned in the linked issue and I will mention again here, we had a conversation about this last year: #80945 (comment)

The comment chain on that PR is very long, but here's a screenshot of the discussion in question:

And here is the commit that my comment linked to: e27e6be

---

SO:

1. Technically you can define a "migration" transform that skips your object's "conversion" transform. It's not a problem that we realistically have to worry about, but maybe we want to prevent that from happening?
2. While we're at it, maybe we want to reconsider this whole idea of allowing "leapfrog" migrations. AFAIK it was supported in the original DocumentMigrator implementation but it has never been used by consumers. We already had unit tests for it, true, but in hindsight it seems to add unnecessary complexity for a feature that no consumers are using (that we are aware of).

So, maybe we should just rip out these 3 lines of code and modify the tests accordingly. WDYT?

Edit: To make it fully clear, I think we should rip this out completely. I just wanted to submit the PR with the minimal fix first so we all have the right context.

[rudolf]

I agree, this code is really hard to follow so any complexity we can remove would be worth it.

[jportner]

OK, I will make additional changes tomorrow to that effect

[jportner]

Changed in 060dff5.

I noticed some comments at the top of the document that mention that migration functions can increase the migrationVersion, but cannot decrease it. I changed those comments accordingly and added an explicit disclaimer in our dev docs that changing migrationVersion, type, and id fields is not supported.

I noticed there is already an assertNoDowngrades validation function that checks to see if a consumer has decreased the migrationVersion and throws an error:

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

Lines 812 to 834 in 060dff5

	function assertNoDowngrades(
	doc: SavedObjectUnsanitizedDoc,
	migrationVersion: SavedObjectsMigrationVersion,
	prop: string,
	version: string
	) {
	const docVersion = doc.migrationVersion;
	if (!docVersion) {
	return;
	}
	const downgrade = Object.keys(migrationVersion).find(
	(k) => !docVersion.hasOwnProperty(k) || Semver.lt(docVersion[k], migrationVersion[k])
	);
	if (downgrade) {
	throw new Error(
	`Migration "${prop} v ${version}" attempted to ` +
	`downgrade "migrationVersion.${downgrade}" from ${migrationVersion[downgrade]} ` +
	`to ${docVersion[downgrade]}.`
	);
	}
	}

This is good to keep, because decreasing the migrationVersion could cause an infinite loop.

I didn't add validation to check for an increased migrationVersion, my reasoning is:

• The impact is low -- a migration function that increases the migrationVersion simply won't work as intended, e.g., it won't "leapfrog" other transforms
• I want to reduce complexity and testing burden
• We never explicitly stated in our dev docs that we supported increasing the migrationVersion, and now we are explicitly stating that we don't support it

But if you think I should add validation anyway, I'll do it, just let me know.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 060dff5
• Storybooks Preview

Metrics [docs]

✅ unchanged

History

• 💚 Build #10487 succeeded 82e0963

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[rudolf]

Thanks for taking this one! ❤️