Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the SO versionModel API #150149

Merged
merged 16 commits into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/core/saved-objects/core-saved-objects-server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,18 @@ export {
SECURITY_EXTENSION_ID,
SPACES_EXTENSION_ID,
} from './src/extensions/extensions';
export type {
SavedObjectsModelVersion,
SavedObjectsModelVersionMap,
Comment on lines +92 to +94
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this long name

Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, using a descriptive name is great!

SavedObjectsModelVersionMapProvider,
SavedObjectsModelChange,
SavedObjectsModelExpansionChange,
SavedObjectModelMigrationDoc,
SavedObjectModelMigrationContext,
SavedObjectModelMigrationFn,
SavedObjectModelBidirectionalMigration,
SavedObjectModelMigrationResult,
} from './src/model_version';

export type {
SavedObject,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export type {
SavedObjectModelMigrationDoc,
SavedObjectModelMigrationContext,
SavedObjectModelMigrationFn,
SavedObjectModelBidirectionalMigration,
SavedObjectModelMigrationResult,
} from './migration';

export type {
SavedObjectsModelChange,
SavedObjectsModelExpansionChange,
SavedObjectsModelVersion,
SavedObjectsModelVersionMap,
SavedObjectsModelVersionMapProvider,
} from './model_version';
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { SavedObjectSanitizedDoc } from '../serialization';
import type { SavedObjectsMigrationLogger } from '../migration';

/**
* Document type used during model migration.
*
* @public
*/
export type SavedObjectModelMigrationDoc<T = unknown> = SavedObjectSanitizedDoc<T>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Context passed down to {@link SavedObjectModelMigrationFn | migration functions}.
*
* @public
*/
export interface SavedObjectModelMigrationContext {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* logger instance to be used by the migration handler
*/
readonly log: SavedObjectsMigrationLogger;
/**
* The model version this migration is registered for
*/
readonly modelVersion: number;
}

/**
* Return type for the {@link SavedObjectModelMigrationFn | migration functions}
*
* @public
*/
export interface SavedObjectModelMigrationResult<DocAttrs = unknown> {
document: SavedObjectModelMigrationDoc<DocAttrs>;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Migration function for the model version API.
*
* Similar to the old migration system, model version migrations take the document to migrate
* and a context object as input and must return the transformed document in its return value.
*
* @public
*/
export type SavedObjectModelMigrationFn<InputAttributes = unknown, OutputAttributes = unknown> = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

document: SavedObjectModelMigrationDoc<InputAttributes>,
context: SavedObjectModelMigrationContext
) => SavedObjectModelMigrationResult<OutputAttributes>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* A bidirectional migration.
*
* Bidirectional migrations define migration functions that can be used to
* migrate from the lower version to the higher one (`up`), and the other way around,
* from the higher version to the lower one (`down`)
*
* @public
*/
export interface SavedObjectModelBidirectionalMigration<
PreviousAttributes = unknown,
NewAttributes = unknown
> {
/**
* The upward (previous=>next) migration.
*/
up: SavedObjectModelMigrationFn<PreviousAttributes, NewAttributes>;
/**
* The downward (next=>previous) migration.
*/
down: SavedObjectModelMigrationFn<NewAttributes, PreviousAttributes>;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { SavedObjectsMappingProperties } from '../mapping_definition';
import type { SavedObjectModelBidirectionalMigration } from './migration';

/**
* 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;
}
Comment on lines +12 to +25
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* {@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) A bidirectional migration to migrate the data from and/or to the previous model version.
*/
migration?: SavedObjectModelBidirectionalMigration<PreviousAttributes, NewAttributes>;
/**
* (optional) The new mappings introduced in this version.
*/
addedMappings?: SavedObjectsMappingProperties;
/**
* (optional) A list of paths to mappings to flag as deprecated. Deprecated mappings should no longer be used and will
* eventually be deleted later.
*/
deprecatedMappings?: string[];
}

/**
* Identify the model change associated with a given {@link SavedObjectsModelVersion}.
*
* At the moment, Only one type of change is supported: {@link SavedObjectsModelExpansionChange | expansions}.
*
* @public
*/
export type SavedObjectsModelChange = SavedObjectsModelExpansionChange;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* A record of {@link SavedObjectsModelVersion | model versions} for a given savedObjects type.
* The record's keys must be integers, starting with 1 for the first entry, and there shouldn't be gaps.
*
* @example
* ```typescript
* const modelVersionMap: SavedObjectsModelVersionMap = {
* '1': modelVersion1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pgayvallet pgayvallet Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* '2': modelVersion2,
* '3': modelVersion3,
* }
* ```
*
* @public
*/
export interface SavedObjectsModelVersionMap {
[modelVersion: string]: SavedObjectsModelVersion;
}

/**
* A function returning a {@link SavedObjectsModelVersionMap | model version map}
*
* Ensured to be called after all plugins executed their `setup` phase.
* Similar to what was done with migrations, can be used to defer resolving the model versions
* associated to a type to after all plugins have been set up.
*
* @public
*/
export type SavedObjectsModelVersionMapProvider = () => SavedObjectsModelVersionMap;
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import type { SavedObjectsTypeManagementDefinition } from './saved_objects_manag
import type { SavedObjectsValidationMap } from './validation';
import type { SavedObjectMigrationMap } from './migration';
import type { SavedObjectsTypeMappingDefinition } from './mapping_definition';
import type {
SavedObjectsModelVersionMap,
SavedObjectsModelVersionMapProvider,
} from './model_version';

/**
* @public
Expand Down Expand Up @@ -128,6 +132,102 @@ export interface SavedObjectsType<Attributes = any> {
* An optional {@link SavedObjectsTypeManagementDefinition | saved objects management section} definition for the type.
*/
management?: SavedObjectsTypeManagementDefinition<Attributes>;

/**
* A map of model versions associated with this 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 the type.
*
* Model versioning is decoupled from Kibana versioning, and isolated between types.
* Model versions are identified by a single numeric value, starting at `1` and without gaps.
*
* Please refer to {@link SavedObjectsModelVersion} for details on the API's usages.
*
* A **valid** versioning would be:
*
* ```ts
* {
* name: 'foo',
* // other mandatory attributes...
* modelVersions: {
* '1': modelVersion1,
* '2': modelVersion2,
* '3': modelVersion3,
* }
* }
* ```
*
* A **invalid** versioning would be:
*
* ```ts
* {
* name: 'foo',
* // other mandatory attributes...
* modelVersions: {
* '1': modelVersion1,
* '3': modelVersion3, // ERROR, no model version 2
* '3.1': modelVersion31, // ERROR, model version is a single numeric value
* }
* }
* ```
*
* @alpha experimental and subject to change.
*/
modelVersions?: SavedObjectsModelVersionMap | SavedObjectsModelVersionMapProvider;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that the map guarantees unique keys

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Allows to opt-in to the new model version API.
*
* Must be a valid semver version (with the patch version being necessarily 0)
*
* When specified, the type will switch from using the {@link SavedObjectsType.migrations | legacy migration API}
* to use the {@link SavedObjectsType.modelVersions | modelVersion API} after the specified version.
*
* When opted in, it will no longer be possible to use the legacy migration API after the specified version.
*
* A **valid** usage example would be:
*
* ```ts
* {
* name: 'foo',
* // other mandatory attributes...
* switchToModelVersionAt: '8.8.0',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* switchToModelVersionAt: '8.8.0',
* switchToModelVersionAfter: '8.8.0',

Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* migrations: {
* '8.1.0': migrateTo810,
* '8.7.0': migrateTo870,
* },
* modelVersions: {
* '1': modelVersion1
* }
* }
* ```
*
* An **invalid** usage example would be:
*
* ```ts
* {
* name: 'foo',
* // other mandatory attributes...
* switchToModelVersionAt: '8.9.0',
* migrations: {
* '8.1.0': migrateTo8_1,
* '8.9.0': migrateTo8_9, // error: migration registered for the switch version
* '8.10.0': migrateTo8_10, // error: migration registered for after the switch version
* },
* modelVersions: {
* '1': modelVersion1
* }
* }
* ```
*
* Please refer to the {@link SavedObjectsType.modelVersions | modelVersion API} for more documentation on
* the new API.
*
* @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.
*/
switchToModelVersionAt?: string;
}

/**
Expand Down
Loading