Introduce the SO versionModel API #150149

pgayvallet · 2023-02-02T10:41:46Z

Summary

First step toward managed upgrades

This PR adds the types that will be used for the new SO model version API, and adds the new properties to the SavedObjectsType type.

The implementation is outside of scope of the PR and will be implemented in a future PR.

The PR also adapt the check_registered_types test to trigger a review when the attributes of SavedObjectsType introduced in this PR are changed.

…lVersion-API

pgayvallet

Self-review

pgayvallet · 2023-02-06T07:34:02Z

packages/core/saved-objects/core-saved-objects-server/index.ts

+export type {
+  SavedObjectsModelVersion,
+  SavedObjectsModelVersionMap,


So, I used a SavedObjectsModel prefix for all the model / model version types. It leads to quite long names, but I couldn't find something sorted that stayed isolated and explicit...

If anyone has a better idea, I'll gladly take it.

I prefer this long name

I agree, using a descriptive name is great!

pgayvallet · 2023-02-06T07:35:58Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+import type { SavedObjectsMigrationLogger } from '../migration';
+
+// alias to more easily adapt later
+export type SavedObjectModelMigrationDoc<T = unknown> = SavedObjectSanitizedDoc<T>;


We may need later to use specific document types for the purpose of the lossless migration, depending on how we decide to propagate the migration state to the migration functions, so I aliased this to be future-proof.

Sounds good. We already use the somethingDoc type-name strategy, so at least it's consistent with that.

pgayvallet · 2023-02-06T07:56:56Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+ *
+ * @public
+ */
+export interface SavedObjectModelMigrationContext {


Re-used the concept of context that was already used for the current migration system, but I created a distinct type for proper isolation, given the two will evolve differently. Also only added the properties that are really useful, so it was an opportunity to cleanup the thing.

pgayvallet · 2023-02-06T08:01:04Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+export interface SavedObjectModelMigrationResult<DocAttrs = unknown> {
+  document: SavedObjectModelMigrationDoc<DocAttrs>;
+}


Instead of having the migration functions only returning the migrated document, I introduced a result type containing it.

This adds a lot of flexibility regarding how we will be able to make the API evolve.

For example, we could imagine adding a polymorphic return type with a type to identify different migration outcomes, or support mutating the lossless migration state by returning the mutated version.

Overall, I'm just taking this opportunity to cleanup and improve the old migration APIs / types.

pgayvallet · 2023-02-06T08:01:43Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+export type SavedObjectModelMigrationFn<InputAttributes = unknown, OutputAttributes = unknown> = (
+  document: SavedObjectModelMigrationDoc<InputAttributes>,
+  context: SavedObjectModelMigrationContext
+) => SavedObjectModelMigrationResult<OutputAttributes>;


Not much to say here, very similar to the existing migration functions, only using the new types for input and output.

pgayvallet · 2023-02-06T08:20:06Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+export interface SavedObjectModelBidirectionalMigration<
+  PreviousAttributes = unknown,
+  NewAttributes = unknown
+> {
+  /**
+   * The upward (old=>new) migration.
+   */
+  up: SavedObjectModelMigrationFn<PreviousAttributes, NewAttributes>;
+  /**
+   * The downward (new=>old) migration.
+   */
+  down: SavedObjectModelMigrationFn<NewAttributes, PreviousAttributes>;


The bidirectional migration structure.

Are we fine using up and down as the property names, or do we want something more explicit?

I'd be +1 for up/down as this is quite common in other frameworks

I'm ++ with up and down. My only nit is in the comment:

Instead of old=>new, should we use something like previous => current

Instead of new=>old, should we use something like current => previous

I'm not fully convinced with my suggestions either, but I'm sure we can even find better wording here to provide a clear mental model that this applies to "the current migration jump"...

My mind is at: the user upgrades 2 versions in a go (v1 -> v2 -> v3). IIUC, when running the migrations that apply to v2, new is v2 and not the actual final version of the distributable that it's running v3.

Disclaimer: it's a super-nit and shouldn't block this PR.

WDYT?

++ to up and down as property names. As for the type and comments, wouldn't previous => next and next => previous work?
Naming's hard! These are nits though and TBH, I don't believe make much difference in the long term.

pgayvallet · 2023-02-06T08:28:39Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/model_version.ts

+/**
+ * Represents a model version of a given savedObjects type.
+ *
+ * Model versions supersede the {@link SavedObjectsType.migrations | migrations} (and {@link SavedObjectsType.schemas | schemas}) APIs
+ * by exposing an unified way of describing the changes of shape or data of a type.
+ *
+ * @public
+ */
+export interface SavedObjectsModelVersion {
+  /**
+   * The {@link SavedObjectsModelChange | changes} associated with this version.
+   */
+  modelChange: SavedObjectsModelChange;
+}


So, compared to the design in the doc, i decided to dissociate the model version and the model change. Some properties, not implemented yet, such as the schema validation for create/update, will be part of the model version, where the things related to the upgrade are contained within the model change

At first, I didn't quite get what the modelChange represents after reading the type description comments. Then seeing the type definition made it clear.

wouldn't validation have to be modelVersion specific? In next a field might be required but in prev it wasn't and even before we introduce contract you might still want to reject any values from being written into deprecated fields.

oh I realise it would still be part of the model version... it's one level up, not two levels up.

pgayvallet · 2023-02-06T08:30:23Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/model_version.ts

+/**
+ * {@link SavedObjectsModelChange | model change} representing an expansion.
+ *
+ * A model expansion can do either, or both, or those:
+ * - add new mappings
+ * - migrate data in a backward-compatible way
+ *
+ * @remark when adding mappings, {@link SavedObjectsType.mappings | the type mappings} must also be updated accordingly.
+ *         Overall, the type's mapping represents the latests version of the mappings, where the model changes
+ *         represent the changes of mappings between two versions.
+ *
+ * @public
+ */
+export interface SavedObjectsModelExpansionChange<
+  PreviousAttributes = unknown,
+  NewAttributes = unknown
+> {
+  /**
+   * The type of {@link SavedObjectsModelChange | change}, used to identify them internally.
+   */
+  type: 'expansion';
+  /**
+   * (optional) The new mappings introduced in this version.
+   */
+  addedMappings?: SavedObjectsMappingProperties;
+  /**
+   * (optional) A bidirectional migration to migrate the data from and/or to the previous model version.
+   */
+  migration?: SavedObjectModelBidirectionalMigration<PreviousAttributes, NewAttributes>;
+}


This represents an expand type change. As defined in the design doc, expand migration supports either, or both, of two actions: adding compatible mappings, and/or registering a bidirectional migration to migrate the data between the versions.

pgayvallet · 2023-02-06T08:33:56Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   * @remarks All types will be forced to switch to use the new API in a later version. This switch is
+   *          allowing types owners to switch their types before the milestone.
+   */
+  switchToModelVersionAfter?: string;


All types will be forced to switch to the new API after a given version.

However, we will allow type owners to opt-in early to the new system via this switchToModelVersionAfter property.

The main use case of manually opt-in to the model version API will be for testing: we will need to be able to define our types as using the new system for our different layers of tests.

Since swithToModelVersionAfter actually means changing to the new API, how about using switchToModelVersionMigrationStrategyAfter rather? It's a much longer name but does clearly distinguish what the property refers to.

pgayvallet · 2023-02-06T09:14:02Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   *
+   * @alpha experimental and subject to change.
+   */
+  modelVersions?: SavedObjectsModelVersionMap | SavedObjectsModelVersionMapProvider;


The modelVersion API, as exposed on the SavedObjectsType type.

Are we fine with using a map based definition as we did for the old migration registration? It's kinda inelegant given versions are only integers, but I couldn't find a better idea.

We could imagine using a list with the version number being included in the entries, but I wasn't convinced if was necessarily better, especially given internally we'll likely have to convert it to a map anyway?

I like that the map guarantees unique keys

A map is fine. It may not be elegant but it works for now.

elasticmachine · 2023-02-06T09:29:54Z

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

Bamieh · 2023-02-06T12:04:31Z

packages/core/saved-objects/core-saved-objects-server/index.ts

+export type {
+  SavedObjectsModelVersion,
+  SavedObjectsModelVersionMap,


I prefer this long name

packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts

afharo · 2023-02-06T13:50:09Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+export interface SavedObjectModelBidirectionalMigration<
+  PreviousAttributes = unknown,
+  NewAttributes = unknown
+> {
+  /**
+   * The upward (old=>new) migration.
+   */
+  up: SavedObjectModelMigrationFn<PreviousAttributes, NewAttributes>;
+  /**
+   * The downward (new=>old) migration.
+   */
+  down: SavedObjectModelMigrationFn<NewAttributes, PreviousAttributes>;


I'm ++ with up and down. My only nit is in the comment:

Instead of old=>new, should we use something like previous => current

Instead of new=>old, should we use something like current => previous

I'm not fully convinced with my suggestions either, but I'm sure we can even find better wording here to provide a clear mental model that this applies to "the current migration jump"...

My mind is at: the user upgrades 2 versions in a go (v1 -> v2 -> v3). IIUC, when running the migrations that apply to v2, new is v2 and not the actual final version of the distributable that it's running v3.

Disclaimer: it's a super-nit and shouldn't block this PR.

WDYT?

afharo · 2023-02-06T14:39:50Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   * {
+   *   name: 'foo',
+   *   // other mandatory attributes...
+   *   switchToModelVersionAt: '8.8.0',


Suggested change

* switchToModelVersionAt: '8.8.0',

* switchToModelVersionAfter: '8.8.0',

I prefer switchToModelVersionAt over using after. To me, "after" is ambiguous, I don't know if the change should be in 8.9 or 8.8.1 or at the version given.
Besides, there's already a property for swithToModelVersionAfter.

Yeah, you're probably right, technically at is more correct here.

afharo · 2023-02-06T14:44:23Z

...es/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts

@@ -158,4 +158,108 @@ describe('extractMigrationInfo', () => {
      });
    });
  });
+
+  describe('modelVersions', () => {


nit: should we add a test for a mixed migration (legacy and new one)?

a mixed migration

Is that even supported? How would that work? I can't imagine supporting BWC in that scenario would be easy.

Is that even supported?

Yeah it is supported, as we need it for on-prem (see #150301). It's just that you won't be able to down-migrate lower than model versions, but API consumers won't be able to ask for such operation anyway.

should we add a test for a mixed migration (legacy and new one)?

Yeah, probably worth it, will add

TinaHeiligers

I only added a few comments.

The types look great! Nice work!

TinaHeiligers · 2023-02-06T23:51:58Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/model_version.ts

+ *
+ * @public
+ */
+export type SavedObjectsModelChange = SavedObjectsModelExpansionChange;


We will one day add to this list, hopefully with a SavedObjectsModelReductionChange 🤞 . Should we bite the bullet and introduce a savedObjectModelChange option from the get-go?
Something like:

export type SavedObjectsModelChange = SavedObjectsModelChangeOption.SavedObjectsModelExpansionChange;

We could use an enum (if that's what you mean by option) for the type attribute, but not for the polymorphic TS type, AFAIK

TinaHeiligers · 2023-02-06T23:58:22Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/model_version.ts

+ * @example
+ * ```typescript
+ * const modelVersionMap: SavedObjectsModelVersionMap = {
+ *   '1': modelVersion1,


I missed where we decided using integers was the best approach. Sure, it doesn't leave wiggle room to squeeze a xx.5 in sometime in the future that could spiral but how would we easily track when and why model versions changed?
OTOH, nothing's preventing folks from documenting changes to a SO model either, so it doesn't really matter.

There's no minor/patch versions because model migrations are decoupled from the stack version, and just executed in order as soon as they're deployed. Patch versions wouldn't make any sense, given you wouldn't be able to run a patch from a prior version anyway.

The only use case I see for 'minor' model version would be introductions that aren't considered changes to the model. Could be mapping removal, or data bug fixes (bi-migrations for which the down one is a no-op).

We could support that eventually later by allowing minor versions (e.g 1.1: modelVersion11), but I don't want to focus on that until we have a better vision on how frequent it could be used.

Ihmo starting with only 'majors' would be fine

TinaHeiligers · 2023-02-07T00:05:07Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   *
+   * @alpha experimental and subject to change.
+   */
+  modelVersions?: SavedObjectsModelVersionMap | SavedObjectsModelVersionMapProvider;


A map is fine. It may not be elegant but it works for now.

TinaHeiligers · 2023-02-07T00:06:45Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   * {
+   *   name: 'foo',
+   *   // other mandatory attributes...
+   *   switchToModelVersionAt: '8.8.0',


I prefer switchToModelVersionAt over using after. To me, "after" is ambiguous, I don't know if the change should be in 8.9 or 8.8.1 or at the version given.
Besides, there's already a property for swithToModelVersionAfter.

TinaHeiligers · 2023-02-07T00:28:20Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   * @remarks All types will be forced to switch to use the new API in a later version. This switch is
+   *          allowing types owners to switch their types before the milestone.
+   */
+  switchToModelVersionAfter?: string;


Since swithToModelVersionAfter actually means changing to the new API, how about using switchToModelVersionMigrationStrategyAfter rather? It's a much longer name but does clearly distinguish what the property refers to.

TinaHeiligers · 2023-02-07T00:29:16Z

...es/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.test.ts

@@ -158,4 +158,108 @@ describe('extractMigrationInfo', () => {
      });
    });
  });
+
+  describe('modelVersions', () => {


a mixed migration

Is that even supported? How would that work? I can't imagine supporting BWC in that scenario would be easy.

rudolf · 2023-02-06T12:52:41Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+export interface SavedObjectModelBidirectionalMigration<
+  PreviousAttributes = unknown,
+  NewAttributes = unknown
+> {
+  /**
+   * The upward (old=>new) migration.
+   */
+  up: SavedObjectModelMigrationFn<PreviousAttributes, NewAttributes>;
+  /**
+   * The downward (new=>old) migration.
+   */
+  down: SavedObjectModelMigrationFn<NewAttributes, PreviousAttributes>;


I'd be +1 for up/down as this is quite common in other frameworks

rudolf · 2023-02-06T13:19:29Z

packages/core/saved-objects/core-saved-objects-server/src/saved_objects_type.ts

+   *
+   * @alpha experimental and subject to change.
+   */
+  modelVersions?: SavedObjectsModelVersionMap | SavedObjectsModelVersionMapProvider;


I like that the map guarantees unique keys

rudolf · 2023-02-07T10:17:26Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/migration.ts

+ *
+ * @public
+ */
+export type SavedObjectModelMigrationFn<InputAttributes = unknown, OutputAttributes = unknown> = (


we use "migration" in a lot of places and it means different things. A "migration" could also mean the whole process of going from one modelVersion to the next (mappings + transforms). What you think about calling these transformation functions like SavedObjectModelTransformationFn

rudolf · 2023-02-07T10:21:35Z

packages/core/saved-objects/core-saved-objects-server/src/model_version/model_version.ts

+/**
+ * Represents a model version of a given savedObjects type.
+ *
+ * Model versions supersede the {@link SavedObjectsType.migrations | migrations} (and {@link SavedObjectsType.schemas | schemas}) APIs
+ * by exposing an unified way of describing the changes of shape or data of a type.
+ *
+ * @public
+ */
+export interface SavedObjectsModelVersion {
+  /**
+   * The {@link SavedObjectsModelChange | changes} associated with this version.
+   */
+  modelChange: SavedObjectsModelChange;
+}


wouldn't validation have to be modelVersion specific? In next a field might be required but in prev it wasn't and even before we introduce contract you might still want to reject any values from being written into deprecated fields.

…lVersion-API

kibana-ci · 2023-02-07T16:14:28Z

💚 Build Succeeded

Buildkite Build
Commit: 790897f

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-server`	96	104	+8
`@kbn/core-test-helpers-so-type-serializer`	12	14	+2
total			+10

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-test-helpers-so-type-serializer`	0	1	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-server`	329	358	+29
`@kbn/core-test-helpers-so-type-serializer`	13	15	+2
`core`	2846	2848	+2
total			+33

History

💔 Build #106112 failed 888636b
💔 Build #106100 failed 131fab8
💚 Build #105959 succeeded b7c3421
💚 Build #105501 succeeded e37f874
💚 Build #105105 succeeded 9ab9110

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

cc @pgayvallet

pgayvallet added 4 commits February 2, 2023 10:17

Add the new types

5aa9983

update migration hash helpers

fdb7811

updates the registered types test snapshots

56e07ed

Merge remote-tracking branch 'upstream/main' into kbn-serverless-mode…

9ab9110

…lVersion-API

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.7.0 release_note:skip Skip the PR/issue when compiling release notes labels Feb 2, 2023

pgayvallet added 4 commits February 6, 2023 08:23

Merge remote-tracking branch 'upstream/main' into kbn-serverless-mode…

ba87f7c

…lVersion-API

updating snapshots again

cd3ae20

add TS doc

70bdfb6

add doc for bimig

e37f874

pgayvallet commented Feb 6, 2023

View reviewed changes

pgayvallet marked this pull request as ready for review February 6, 2023 09:29

pgayvallet requested a review from a team as a code owner February 6, 2023 09:29

Bamieh reviewed Feb 6, 2023

View reviewed changes

afharo approved these changes Feb 6, 2023

View reviewed changes

afharo assigned pgayvallet Feb 6, 2023

TinaHeiligers approved these changes Feb 7, 2023

View reviewed changes

pgayvallet added 3 commits February 7, 2023 08:26

add deprecatedMappings to model change

bd48331

rename attribute

b7c3421

add test

5c556bd

rudolf reviewed Feb 7, 2023

View reviewed changes

rudolf approved these changes Feb 7, 2023

View reviewed changes

pgayvallet added 3 commits February 7, 2023 15:33

change migrations to transformations

59d8bab

Merge remote-tracking branch 'upstream/main' into kbn-serverless-mode…

fb3666b

…lVersion-API

update snapshot

131fab8

pgayvallet enabled auto-merge (squash) February 7, 2023 14:43

pgayvallet added 2 commits February 7, 2023 16:00

fix tests

888636b

fix more types

790897f

Bamieh approved these changes Feb 7, 2023

View reviewed changes

pgayvallet merged commit 7136039 into elastic:main Feb 7, 2023

kibanamachine added the backport:skip This commit does not require backporting label Feb 7, 2023

	* switchToModelVersionAt: '8.8.0',
	* switchToModelVersionAfter: '8.8.0',

Introduce the SO versionModel API #150149

Introduce the SO versionModel API #150149

Conversation

pgayvallet commented Feb 2, 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

TinaHeiligers Feb 6, 2023 • edited Loading

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

TinaHeiligers Feb 6, 2023 • edited Loading

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 Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Feb 7, 2023 • edited Loading

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 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers 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

pgayvallet Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Feb 7, 2023 • edited Loading

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

kibana-ci commented Feb 7, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

pgayvallet commented Feb 2, 2023 •

edited

Loading

TinaHeiligers Feb 6, 2023 •

edited

Loading

TinaHeiligers Feb 6, 2023 •

edited

Loading

TinaHeiligers Feb 7, 2023 •

edited

Loading

pgayvallet Feb 7, 2023 •

edited

Loading

pgayvallet Feb 7, 2023 •

edited

Loading

TinaHeiligers Feb 7, 2023 •

edited

Loading