DocumentMigration: implement downward transformation of documents #153921

pgayvallet · 2023-03-29T08:34:01Z

Summary

Fix #150301

Implements DocumentMigrator.transformDown and the associated transformation pipeline.

Note: The feature is currently unused, and will only be when we'll start #150312

… pipeline

…-ref HEAD~1..HEAD --fix'

…own-migrations

pgayvallet

Self-review

pgayvallet · 2023-03-30T10:00:28Z

...core/saved-objects/core-saved-objects-migration-server-internal/src/core/migrate_raw_docs.ts

 import { TransformSavedObjectDocumentError } from '.';

+type MigrateAndConvertFn = VersionedTransformer['migrateAndConvert'];


This was the only place where this function type was used, so I inlined it in VersionedTransformer instead

pgayvallet · 2023-03-30T10:02:33Z

...core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.test.ts

@@ -1329,6 +1342,175 @@ describe('DocumentMigrator', () => {
      });
    });
  });
+
+  describe('down transformation', () => {


Most of the unit tests are directly for downgrade_pipeline. In theory, I should have stubbed the pipeline for the doc migrator tests, but

we're not doing it for other unit tests in the file and the 'normal' pipeline

this part is so touchy that including the underlying implementation in the testing makes some sense

So I added some 'integration' smoke tests here.

pgayvallet · 2023-03-30T10:03:50Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

-/*
- * This file contains logic for transforming / migrating a saved object document.
- *
- * At first, it may seem as if this could be a simple filter + reduce operation,
- * running the document through a linear set of transform functions until it is


This long wall of text is mostly outdated, and doesn't really bring anything, so I decided to finally get rid of it.

Speak now or forever hold your peace.

pgayvallet · 2023-03-30T10:04:42Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

-  migrationVersion: SavedObjectsMigrationVersion;
-  migrate: MigrateFn;
-  migrateAndConvert: MigrateAndConvertFn;


VersionedTransformer was 'leaking' migrationVersion and prepareMigrations that shouldn't be present on that kind of publish-ish interfaces, so I cleaned that up

pgayvallet · 2023-03-30T10:06:28Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

+  transformDown: (
+    doc: SavedObjectUnsanitizedDoc,
+    options: { targetTypeVersion: string }
+  ) => SavedObjectUnsanitizedDoc;


Note: I highly suspect that given what we're intending to use the feature for, we will need some kind of getAtVersion API that either convert up or down depending on the doc and target version. However it will likely just delegates to the existing APIs, and I didn't want to implement it before having a better understanding of how the SOR will use the feature in #150312

pgayvallet · 2023-03-30T10:11:58Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

+import type { MigrationPipeline, MigrationPipelineResult } from './types';
+import { applyVersion, assertValidCoreVersion } from './utils';
+
+export class DocumentDowngradePipeline implements MigrationPipeline {


I spent a lot of time trying to figure out if it was better to have the logic for both up and down conversions at the same place or them being dissociated, and at the end, I think this is better:

The up migration has a lot of legacy logic because of the Reference and Converting migrations that down migrations don't have to care about

The logic and sanity check are fully different (e.g assertUpgrade in the up pipeline)

Even the look and version-applying logic are somewhat different

So I went with the 2 distinct logics approach, and I still think that's the correct decision.

pgayvallet · 2023-03-30T10:15:47Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

+      case TransformType.Core:
+        return (
+          this.targetCoreVersion &&
+          (coreMigrationVersion == null ||
+            Semver.gt(coreMigrationVersion, this.targetCoreVersion)) &&
+          Semver.gt(version, this.targetCoreVersion)
+        );


I implemented support for downward core transforms, even if it's not planned to use it atm.

pgayvallet · 2023-03-30T10:16:46Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

+      typeMigrationVersion: this.targetTypeVersion,
+      ...(coreMigrationVersion ? { coreMigrationVersion } : {}),


Different behavior than the up migrations, here we always apply/set the target version to the document before returning it.

pgayvallet · 2023-03-30T10:18:02Z

...-saved-objects-migration-server-internal/src/document_migrator/pipelines/upgrade_pipeline.ts

+    migrations: ActiveMigrations;
+    kibanaVersion: string;
+    convertNamespaceTypes: boolean;
+    targetTypeVersion?: string;


Added the targetTypeVersion option to the up pipeline, as I suspect we will need it. Note that atm, the option is not exposed or used by the document migrator itself.

pgayvallet · 2023-03-30T10:18:44Z

...-saved-objects-migration-server-internal/src/document_migrator/pipelines/upgrade_pipeline.ts

+        if (this.document.type !== this.originalDoc.type) {
+          throw new Error(`Changing a document's type during a migration is not supported.`);


This is (finally) no longer possible, as it would be a real problem for ZDT.

Note that no one ever used this functionality anyway.

elasticmachine · 2023-03-30T10:20:31Z

Pinging @elastic/kibana-core (Team:Core)

afharo

Great job! LGTM! I added a few nits for your consideration.

afharo · 2023-03-30T12:21:25Z

...core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.test.ts

+
+    beforeEach(() => {
+      const migrate1 = createTransformFn((doc) => {
+        doc.attributes.wentThoughDown1 = true;


nit: wentThroughDown1 (missing r)

afharo · 2023-03-30T12:24:37Z

...core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.test.ts

+            1: {
+              modelChange: {
+                type: 'expansion',
+                transformation: { up: jest.fn(), down: migrate1 },


NIT: Are we planning to add an FTR that checks up & down migrations are symmetric? I guess this is something we'll do in follow-up PRs, right?

P.S.: This comment doesn't belong here... but this line reminded me that...

afharo · 2023-03-30T13:27:26Z

...cts/core-saved-objects-migration-server-internal/src/document_migrator/model_version.test.ts

+      expect(result).toEqual({
+        transformedDoc: resultDoc,
+        additionalDocs: [],
+      });


nit: should we check that downTransform has been called once?

it's covered in the previous test

jloleysens

Great job @pgayvallet ! Left a few comments (mostly for my education).

Note: The feature is currently unused, and will only be when we'll start #150312

👍🏻

jloleysens · 2023-03-31T10:14:17Z

...core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.test.ts

+          id: 'smelly',
+          type: 'dog',
+          attributes: { name: 'Callie' },


Smelly Callie 😄

jloleysens · 2023-03-31T10:20:58Z

...ects/core-saved-objects-migration-server-internal/src/document_migrator/document_migrator.ts

+      kibanaVersion: this.options.kibanaVersion,
+      targetTypeVersion: options.targetTypeVersion,
+    });
+    const { document } = pipeline.run();


Is it possible for a down migration to create additional docs?

In practice, no as the higher-level API does not allow it. I've been reusing the same internal types that the up migration for simplicity's sake.

jloleysens · 2023-03-31T10:46:23Z

...-objects/core-saved-objects-migration-server-internal/src/document_migrator/model_version.ts

-  return transforms;
+  return Object.entries(modelVersionMap).map<Transform>(([rawModelVersion, definition]) => {
+    const modelVersion = assertValidModelVersion(rawModelVersion);
+    const virtualVersion = modelVersionToVirtualVersion(modelVersion);


A little late to the party on this, but would it be possible to get a little bit more context on the exact purpose of a virtual version in the doc comment of modelVersionToVirtualVersion or modelVersionVirtualMajor? I understand what it is (thanks to your examples) but I don't fully understand the role it plays, specifically the "virtual" part.

To avoid breaking change in the way

the documentMigration handles versions

the way versions are stored in typeModelVersion in documents

We're considering that model versions are just version 10. E.g MV 1 = version 10.1.0. This is just a trick to avoid greatly reduce the internal changes

jloleysens · 2023-03-31T10:56:34Z

...aved-objects-migration-server-internal/src/document_migrator/pipelines/downgrade_pipeline.ts

+    const latestVersion = this.typeTransforms.latestVersion.migrate;
+
+    if (currentVersion && Semver.gt(currentVersion, latestVersion)) {
+      throw Boom.badData(


Just curious: is the intention to continue using/proliferating use of Boom? My understanding is that it is legacy, but that may be mistaken.

We need to get rid of those atrocities. I just didn't want to do it in the current PR as I suspect some snapshots are based on those. But you're right, for the new pipeline, I can remove them right now. Will do.

…own-migrations

pgayvallet · 2023-04-03T07:13:18Z

@elasticmachine merge upstream

kibana-ci · 2023-04-03T08:02:39Z

💚 Build Succeeded

Buildkite Build
Commit: 32764a0

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	81	85	+4

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	116	120	+4

ESLint disabled in files

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	0	2	+2

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	0	2	+2
`securitySolution`	512	515	+3
total			+5

History

💔 Build #117268 failed 8cfd326
💚 Build #116629 succeeded a5a43bf
💔 Build #116619 failed 2e629de
💔 Build #116492 failed 2b17e30
💔 Build #116308 failed 74cf5f8

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

plug downward transform from model versions and add some tests around…

8c79e4f

… pipeline

pgayvallet mentioned this pull request Mar 29, 2023

DocumentMigrator: support downward migrations #150301

Closed

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

74cf5f8

…-ref HEAD~1..HEAD --fix'

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Migrations release_note:skip Skip the PR/issue when compiling release notes labels Mar 29, 2023

pgayvallet added 12 commits March 29, 2023 14:17

add targetTypeVersion option to pipeline

3f3699a

trying to find the best approach

67cdaa2

implement downgrade pipeline, move stuff around

2b17e30

adapt model version conversion unit test

29d1d36

Merge remote-tracking branch 'upstream/main' into kbn-150301-docmig-d…

e11e21d

…own-migrations

remove export

651c34f

removing outdated comment

6e22e60

some cleanup

2e629de

more cleanup

f3738f8

implements transformDown

79231b5

add more test coverage

a615064

one more smoke test

a5a43bf

pgayvallet commented Mar 30, 2023

View reviewed changes

pgayvallet marked this pull request as ready for review March 30, 2023 10:20

pgayvallet requested a review from a team as a code owner March 30, 2023 10:20

afharo approved these changes Mar 30, 2023

View reviewed changes

jloleysens approved these changes Mar 31, 2023

View reviewed changes

pgayvallet added 2 commits April 3, 2023 07:41

Merge remote-tracking branch 'upstream/main' into kbn-150301-docmig-d…

abd80ef

…own-migrations

review comments

8cfd326

pgayvallet enabled auto-merge (squash) April 3, 2023 05:50

Merge branch 'main' into kbn-150301-docmig-down-migrations

32764a0

pgayvallet merged commit 54df909 into elastic:main Apr 3, 2023

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DocumentMigration: implement downward transformation of documents #153921

DocumentMigration: implement downward transformation of documents #153921

pgayvallet commented Mar 29, 2023 •

edited

Loading

pgayvallet left a comment

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

pgayvallet Mar 30, 2023

elasticmachine commented Mar 30, 2023

afharo left a comment

afharo Mar 30, 2023

afharo Mar 30, 2023

afharo Mar 30, 2023

pgayvallet Apr 3, 2023

jloleysens left a comment

jloleysens Mar 31, 2023

jloleysens Mar 31, 2023

pgayvallet Apr 3, 2023

jloleysens Mar 31, 2023

pgayvallet Apr 3, 2023 •

edited

Loading

jloleysens Mar 31, 2023

pgayvallet Apr 3, 2023

pgayvallet commented Apr 3, 2023

kibana-ci commented Apr 3, 2023

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

		import { TransformSavedObjectDocumentError } from '.';

		type MigrateAndConvertFn = VersionedTransformer['migrateAndConvert'];

		typeMigrationVersion: this.targetTypeVersion,
		...(coreMigrationVersion ? { coreMigrationVersion } : {}),

		if (this.document.type !== this.originalDoc.type) {
		throw new Error(`Changing a document's type during a migration is not supported.`);

DocumentMigration: implement downward transformation of documents #153921

DocumentMigration: implement downward transformation of documents #153921

Conversation

pgayvallet commented Mar 29, 2023 • edited Loading

Summary

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 30, 2023

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Apr 3, 2023

kibana-ci commented Apr 3, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented Mar 29, 2023 •

edited

Loading

pgayvallet Apr 3, 2023 •

edited

Loading