Restructure savedObject types and declarations

In the legacy world, declaring an actual SavedObject type is done in multiple steps. Schemas, mappings, migrations, and validations for every types are declared in separate properties of the plugin's uiExports property.

new kibana.Plugin({
  uiExports: {
    mappings, // map of every types mappings
    savedObjectSchemas, // map of every types schemas
    migrations // map of every types migrations
  },
})

In #55825, #50311 and #50313, we are going to mimic this behaviour by adding specific API to register each of these 'parts' of savedObjects.

All the features are not migrated yet, but this will be something like:

import mappings from './mappings.json';
import { schemas } from './schemas';
import { migrations } from './migrations';

export class MyPlugin implements Plugin {
  setup({ savedObjects }) {
    savedObjects.registerMappings(mappings as SavedObjectsTypeMappingDefinitions);
    savedObjects.registerSchemas(schemas);
    savedObjects.registerMigrations(migrations);
  }

This doesn't feel right.

I'm not sure why this distinction has been done in legacy in the first place, but my feeling is that is was caused by the fact these various features around savedObject were not added at the same time, therefor adding a new uiExport property every time to avoid refactoring the whole thing.

Now that we have the Kibana Platform, and typescript to back us up, I think it may be a good time to refactor all this to regroup everything related to a single savedObject type at the same place.

I think that registering everything related to a type should be done in a single call.

The upsides I see:

The description of your types is done in a single place. You don't need to check 3 or 4 files to see it's schema, it's mapping, it's migrations and it's validations
ATM these 4 groups of properties are propagated alongs the savedObject service's nested components (such as the migrator), this would really clean this part of core to have a better model.
easier for plugins to isolate a specific type for functional/unit test
just feel like a more logical structure

What I see is something like that:

export interface SavedObjectsType {
  type: string;
  schema: SavedObjectsSchema;
  mappings: SavedObjectsTypeMappingDefinition;
  migrations: SavedObjectsMigrations;
  validations: SavedObjectsValidations
}

and actual registration would be done like that:

import { mySoType, myOtherSoType } from './saved_objects';

export class MyPlugin implements Plugin {
  setup({ savedObjects }) {
    savedObjects.registerType(mySoType);
    savedObjects.registerType(myOtherSoType);
  }

This is a breaking change, and we will have to support the legacy/currently implemented in NP APIs for a time, but we would still be able use the new model internally by constructing the types from the various 'legacy' API calls, until we finally remove the old APIs and only expose registerType

WDYT?

Pinging @elastic/kibana-platform (Team:Platform)

I agree that these should be consolidated. I think SavedObjectsSchema could be collapsed into the top-level SavedObjectsType

export interface SavedObjectsType {
  type: string;
  hidden: boolean;
  isNamespaceAgnostic: boolean;
  indexPattern: (() => string) | string
  mappings: SavedObjectsTypeMappingDefinition;
  migrations: SavedObjectsMigrations;
  validations: SavedObjectsValidations
}

I think migrating from uiExports properties in legacy to a single API could would be trivial and won't add a big migration burden for plugins. So I think now is the right time to make this change.

I think SavedObjectsSchema could be collapsed into the top-level SavedObjectsType

Fine with me. the schema name didn't make much sense anyway imho (I think I would name schema what we currently name mappings, but not let's do that kind of terminology change now, would be to hard to follow for plugin developers)

I think migrating from uiExports properties in legacy to a single API could would be trivial and won't add a big migration burden for plugins. So I think now is the right time to make this change.

If we go in that direction, I guess we should do all #55825, #50311 and #50313 in the same PR (or at least issue) as they will kinda be 'merged' into the same API?

If we go in that direction, I guess we should do all #55825, #50311 and #50313 in the same PR (or at least issue) as they will kinda be 'merged' into the same API?

It makes sense to me to only expose this API once everything is done, but perhaps we can break up the underlying work in different PR's but keep the API internal. If the PR's are roughly the same size as #55825 it could become quite a lot of changes.

It makes sense to me to only expose this API once everything is done, but perhaps we can break up the underlying work in different PR's but keep the API internal. If the PR's are roughly the same size as #55825 it could become quite a lot of changes.

I think #55825 will be significantly bigger than the other PRs, as the prerequisites to perform SO features registrations in NP are included in it. (also the mappings types seems more of a mess than the others, and needed more changes)

If we agree to do this work now (which I'm strongly for, as it would be harder to introduce a breaking change in NP api later), I can prepare the SavedObjectsType structure we discussed previously of in #55825. The others PR should be significantly smaller then.

I could even do a distinct PR by cherry picking the needed bits from #55825 to introduce our new structure, and flag it a prerequisite for #55825, #50311 and #50313

I'm in support of doing this change now. I've always found this confusing and have to look each of these different concepts up each time I work on these.

If we agree to do this work now (which I'm strongly for, as it would be harder to introduce a breaking change in NP api later), I can prepare the SavedObjectsType structure we discussed previously of in #55825. The others PR should be significantly smaller then.

I could even do a distinct PR by cherry picking the needed bits from #55825 to introduce our new structure, and flag it a prerequisite for #55825, #50311 and #50313

I think I'm a bit unclear on what would be exposed when. Are you proposing we add an API in #55825 that accepts the entire type for all of these components, but some of them are non-functional until the other PR(s) are done?

I think I'm a bit unclear on what would be exposed when. Are you proposing we add an API in #55825 that accepts the entire type for all of these components, but some of them are non-functional until the other PR(s) are done?

After thinking on the order / process, I came to the conclusion that this should be done in two steps:

Change the internal model, and adapt the uiExport SO registration to convert to this new model (this is the heaviest change, and can only be done in a unitary PR imho). This would probably start from a cherry pick from Add savedObjects mappings API to core #55825 where I remove the added NP APIs.
Once this is done, implementing savedObjects.registerType in new platform should be rather trivial, as the internal mechanism would already be done in previous PR, and could be done in a single PR fixing all three of Add savedObjects mappings API to core #55825, [Platform] SavedObject Migrations #50311 and [Platform] SavedObject Schemas #50313

If we adopt the suggestion I've posted in #56166 (comment) to allow plugins to migrate types owned by another plugins, we will need a way for a plugin to update it's type definition at later stages in the setup lifecycle.

One way to do this while keeping a single API call per type would be to allow multiple calls to savedObjects.registerType, calling it more than once will override the previous definition so that we only use the last call. The disadvantage is that this opens up the possibility for a plugin to redefine any type that's been previously registered since we can no longer "close" the type after it's been registered.

If we adopt the suggestion I've posted in #56166 (comment) to allow plugins to migrate types owned by another plugins

Note: I don't thing this even works in legacy right now, as I think the uiExport merger merges at the type level, meaning that multiple plugins registering migrations for a same version would erase some of them.

One way to do this while keeping a single API call per type would be to allow multiple calls to savedObjects.registerType, calling it more than once will override the previous definition so that we only use the last call.

What is the 'last call' exactly ? This would just depends on the order we initialise the plugins?
That means the plugin registering migrations for another plugin's type need to know all about the plugin, as it should re-define mappings, validations and schema for the other plugin's type.
That seems way too permissive imho.

If we really want that, I think I would:

Add a safety flag on the SavedObjectType, something like allowExternalMigrations that would defaults to false. A plugin would need to explicitly allow migrations from other plugins.
Keep the registerType(type: SavedObjectType) API, and add an additional registerMigrations(type: string, migrations: SavedObjectMigrationMap). We would find a way to merge/aggregate the migrations at the end of the setup phase

This way, we ensure that the only thing other plugins can add to an existing type is additional migrations, and not alter the actual structure/mapping of the type.

WDYT? Does that answer the need?

Navigation Menu

Repository navigation

Restructure savedObject types and declarations #55857

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Restructure savedObject types and declarations #55857

Description

Activity

elasticmachine commented on Jan 24, 2020

rudolf commented on Jan 24, 2020

pgayvallet commented on Jan 27, 2020

rudolf commented on Jan 27, 2020

pgayvallet commented on Jan 27, 2020

joshdover commented on Jan 27, 2020

pgayvallet commented on Jan 28, 2020

rudolf commented on Jan 30, 2020

pgayvallet commented on Jan 30, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions