From 5a919efb3e4e6341946522e82132edc08b9d8eaf Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 2 Jun 2020 02:50:48 -0700 Subject: [PATCH 01/11] initial commit --- rfcs/text/0011_persistable_state_service.md | 100 ++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 rfcs/text/0011_persistable_state_service.md diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md new file mode 100644 index 0000000000000..341a7855515ea --- /dev/null +++ b/rfcs/text/0011_persistable_state_service.md @@ -0,0 +1,100 @@ +- Start Date: 2020-06-02 +- RFC PR: (leave this empty) +- Kibana Issue: (leave this empty) + +# Summary + +Pluggable state from registry items can often end up stored inside saved objects, for instance: +Embeddables, Expression strings, Url generators, ... + +When plugin A (persister) stores some state that belongs to another plugin B a few issues arise: +- How does persister know if plugin B state contains references to saved objects +- How does persister migrate the saved object when it contains state that belongs to plugin B +- How does persister know if state that belongs to plugin B contains state that belongs to plugin C + +# Basic example + +```ts + +const MY_STATE_ID = 'MyState'; + +interface MyStateV77 extends Serializable { + objectId: string, + val: number, +} + +interface MyState extends Serializable { + objectId: string, + value: number, +} + +const migrate = (state: MyState, version: number) => { + let newState: MyState, + if (version < 7.8) { + const oldState = state as MyStateV77; + newState = { objectId: oldState.objectId, value: oldState.val }; + } else { + newState = state; + } + + return newState; +} + +const inject = (state: MyState, savedObjectReferences: SavedObjectReference[]) => { + return { + ...state, + objectId: savedObjectReferences.find(ref => ref.name = state.objectId)?.id; + } +} + +const extract = (state: MyState) => { + const references = [{ name: 'objectId', id: state.objectId, type: 'savedObject' }]; + return [{ ...state, objectId: 'objectId' }, references]; +} + +persistableStateService.register(MY_STATE_ID, { migrate, inject, extract }); +``` + +```ts + const stateReadyForPersisting = persistableStateService.get(MY_STATE_ID),beforeSave(myState); + const stateReadyForConsuming = persistableStateService.get(MY_STATE_ID).afterLoad(stateReadyForPersisting); +``` + +# Motivation + +We need a formal way for authors of registry implementations to add migrations for this data. Since they may not own the saved object that their implementation ends up in, we can't rely on saved object migrations as it exists today (not to mention data that ends up in URLs that may need to be migrated). + +We also need to make sure that all the persited state containing references to saved objects extracts those before being saved and injects them later. + +# Detailed design + +... + +# Drawbacks + +- teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state. + +# Alternatives + +Instead of having a central registry we could have each plugin export functions for migration, reference extraction and reference injection. +Plugin consuming its state would need to import them from correct place. + +# Adoption strategy + +First app arch team will add persistable state definitions for saved visualizations and saved searches (vis state, search source), expressions and base embeddables. Adoption by other teams can happen gradually. + +# How we teach this + +What names and terminology work best for these concepts and why? How is this +idea best presented? As a continuation of existing Kibana patterns? + +Would the acceptance of this proposal mean the Kibana documentation must be +re-organized or altered? Does it change how Kibana is taught to new developers +at any level? + +How should this feature be taught to existing Kibana developers? + +# Unresolved questions + +Optional, but suggested for first drafts. What parts of the design are still +TBD? \ No newline at end of file From db20ddeec460ce94fc4bb07a5b66060540d2f5e6 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 2 Jun 2020 03:09:30 -0700 Subject: [PATCH 02/11] ... --- rfcs/text/0011_persistable_state_service.md | 119 +++++++++++++++++++- 1 file changed, 116 insertions(+), 3 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 341a7855515ea..128e79d183ccd 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -18,12 +18,12 @@ When plugin A (persister) stores some state that belongs to another plugin B a f const MY_STATE_ID = 'MyState'; -interface MyStateV77 extends Serializable { +interface MyStateV77 extends PersistableState { objectId: string, val: number, } -interface MyState extends Serializable { +interface MyState extends PersistableState { objectId: string, value: number, } @@ -68,7 +68,120 @@ We also need to make sure that all the persited state containing references to s # Detailed design -... +```ts +interface PersistableState extends Serializable {} + +interface PersistableStateDefinition { + id: string, + // migrate function receives state and version string and should return latest state version + // default is identity function + migrate: (state: PersistableState, version: string) => PersistableState, + // inject function receives state and a list of references and should return state with references injected + // default is identity function + inject: (state: PersistableState, references: SavedObjectReference[]) => PersistableState, + // extract function receives state and should return state with references extracted and array of references + // default returns same state with empty reference array + extract: (state: PersistableState) => [PersistableState, SavedObjectReference[]] +} + +class PersistableStatePlugin { + setup(core, plugins) { + return { + // if one of the functions is not provided default is used + // so we can always assume that all of the functions are available for every persistable state + register: (id: string, definition: Partial) => void + } + } + start(core, plugins) { + return { + // get will always return. If requested persistable state is not defined it will return default set of functions. + get: (id: string) => ({ + // takes the state, extracts the references and returns them + version string + beforeSave: (state: PersistableState) => [PersistableState, SavedObjectReference[], string], + // takes the state, references and version and returns latest state version with references injected + afterLoad: (state: PersistableState, references: SavedObjectReference[], version: string) => PersistableState, + }) + } + } +} +``` + + +# Consuming and edge cases + +When plugin A wants to store some state that it does not own it should check with persistableStateRegistry for a registered persistable state definition and execute `beforeSave` function. + +```ts + const expressionReadyForSaving = persistableStatePlugin.get('expression').beforeSave(expressionString); + +``` + +## EnhacedDrilldownEmbeddableInput + +Drilldown plugin adds to the state of embeddable: + +```ts +interface EnhancedDrilldownEmbeddableInput extends EmbeddableInput { + enhancements: { + drillDowns: { events?: Events[]; } + } +} +``` +Embeddable is aware that it has an enhancements property on its state, where every key represents state of another plugin. + +```ts +const inject = (state, references) => { + Object.getKeys(state.enhancements).forEach(key => { + State.enhancements[key] = persistableStatePlugin.get(key).inject(state.enhancements[key], references); + } +} +``` + +And drilldown plugin registers its state with persistableStateService. + +## We have to avoid shared persistable state + +So we have to do two things: + +- Always call a migration function for nested state. +- Maintain state boundaries along plugin boundaries. Do not allow VisualizeEmbeddableInput to migrate data on EmbeddableInput. Don't even give it access to this state at all. +As long as state ownership is isolated, we can do something like this: + +```ts +const embeddableMigrator: PersistableStateMigrationFn (state: { type: string; input: unknown }, version: string): State { + return { + ...migrateInputToLatest(state.input), + enhancements: { + ...state.input.enhancements.map(enhancement => + persistableStateMigrations.get(enhancement).migrate(state.input.enhancements[extension], version)), + }, + specializedState: persistableStateMigrations.get(state.type).migrate(state.input.specializedState, version)), + } +} +``` + +The key here is that we have to separate out the state owned by VisualizeEmbeddableInput and that owned by EmbeddableInput. If we allow them both access to the full input (VisualizeEmbeddableInput extends EmbeddableInput), there is the possibility for key clashes. For instance: + +```ts +// We start out with the base class having an attribute of `title`. +interface EmbeddableInputV1 { + title: string; +} + +// `Title` is taken by the base class, so Visualize plugin adds `defaultTitle`. +interface VisualizeEmbeddableInputV1 implements EmbeddableInputV1 { + defaultTitle: string; +} + +// In a future version, EmbeddableInput author decides to rename `title` => `defaultTitle`. +// They don't know about `VisualizeEmbeddableInput` so don't realize there is going to be +// a key collision. +interface EmbeddableInputV2 { + defaultTitle: string; +} +``` + +It's probably a rare occurrence, but it's also very risky. EmbeddableInput thinks it owns defaultTitle. Let's say it decides to change the name yet again, so in V3 changes it to customPanelTitle and removes defaultTitle. Let's also say VisualizeEmbeddable author doesn't add a migration for V2 or V3. A user migrates to V3, VisualizeEmbeddable just had its defaultTitle state wiped out unknowingly (types will still say it's a required parameters). This has the potential to permanently break saved objects with no way to undo the migration. # Drawbacks From d725ea7f0b55fb753449e546e0e4304d6e6f64cc Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 2 Jun 2020 06:49:54 -0700 Subject: [PATCH 03/11] ... --- rfcs/text/0011_persistable_state_service.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 128e79d183ccd..c6f79501fd266 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -95,12 +95,11 @@ class PersistableStatePlugin { start(core, plugins) { return { // get will always return. If requested persistable state is not defined it will return default set of functions. - get: (id: string) => ({ - // takes the state, extracts the references and returns them + version string - beforeSave: (state: PersistableState) => [PersistableState, SavedObjectReference[], string], - // takes the state, references and version and returns latest state version with references injected - afterLoad: (state: PersistableState, references: SavedObjectReference[], version: string) => PersistableState, - }) + get: (id: string) => PersistableStateDefinition, + // takes the state, extracts the references and returns them + version string + beforeSave: (id: string, state: PersistableState) => [PersistableState, SavedObjectReference[], string], + // takes the state, references and version and returns latest state version with references injected + afterLoad: (id: string, state: PersistableState, references: SavedObjectReference[], version: string) => PersistableState, } } } @@ -112,7 +111,7 @@ class PersistableStatePlugin { When plugin A wants to store some state that it does not own it should check with persistableStateRegistry for a registered persistable state definition and execute `beforeSave` function. ```ts - const expressionReadyForSaving = persistableStatePlugin.get('expression').beforeSave(expressionString); + const expressionReadyForSaving = persistableStatePlugin.beforeSave('expression', expressionString); ``` From f3534487e8b81bf5cbb684e2b812db8ed87d875a Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 9 Jun 2020 04:00:58 -0700 Subject: [PATCH 04/11] review updates --- rfcs/text/0011_persistable_state_service.md | 47 ++++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index c6f79501fd266..dcaeb1fbf4179 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -18,6 +18,11 @@ When plugin A (persister) stores some state that belongs to another plugin B a f const MY_STATE_ID = 'MyState'; +interface MyStateV76 extends PersistableState { + object: string, + val: number, +} + interface MyStateV77 extends PersistableState { objectId: string, val: number, @@ -28,13 +33,15 @@ interface MyState extends PersistableState { value: number, } -const migrate = (state: MyState, version: number) => { - let newState: MyState, - if (version < 7.8) { - const oldState = state as MyStateV77; - newState = { objectId: oldState.objectId, value: oldState.val }; - } else { - newState = state; +const migrate = (state: unknown, version: string) => { + let v76 = version === '7.6' ? state as MyStateV76 : undefined; + let v77 = version === '7.7' ? state as MyStateV77 : undefined; + let v78 = version === '7.8' ? state as MyState : undefined; + if (v76) { + v77 = { objectId: v76.object, val: v76.val }; + } + if (v77) { + v78 = { objectId: v77.objectId, value: v77.val }; } return newState; @@ -81,7 +88,7 @@ interface PersistableStateDefinition { inject: (state: PersistableState, references: SavedObjectReference[]) => PersistableState, // extract function receives state and should return state with references extracted and array of references // default returns same state with empty reference array - extract: (state: PersistableState) => [PersistableState, SavedObjectReference[]] + extract: (state: PersistableState) => { state: PersistableState, references: SavedObjectReference[] } } class PersistableStatePlugin { @@ -98,7 +105,7 @@ class PersistableStatePlugin { get: (id: string) => PersistableStateDefinition, // takes the state, extracts the references and returns them + version string beforeSave: (id: string, state: PersistableState) => [PersistableState, SavedObjectReference[], string], - // takes the state, references and version and returns latest state version with references injected + // takes the state, references and version and returns latest (migrated) state with references injected afterLoad: (id: string, state: PersistableState, references: SavedObjectReference[], version: string) => PersistableState, } } @@ -111,10 +118,16 @@ class PersistableStatePlugin { When plugin A wants to store some state that it does not own it should check with persistableStateRegistry for a registered persistable state definition and execute `beforeSave` function. ```ts - const expressionReadyForSaving = persistableStatePlugin.beforeSave('expression', expressionString); + const stateForSaving = persistableStatePlugin.beforeSave(id, myState); ``` +WARNING: If state id is known at compile time we should rather import the correct utilities directly from expression plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. + +## Handling corrupt state + +This is up to the registrator, but we will try to come up with a good recomendation. + ## EnhacedDrilldownEmbeddableInput Drilldown plugin adds to the state of embeddable: @@ -138,6 +151,12 @@ const inject = (state, references) => { And drilldown plugin registers its state with persistableStateService. +## + +## Use a `{pluginId}+{stateType}` pattern for your ID + +To avoid clashes with other unknown plugins. + ## We have to avoid shared persistable state So we have to do two things: @@ -184,12 +203,8 @@ It's probably a rare occurrence, but it's also very risky. EmbeddableInput think # Drawbacks -- teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state. - -# Alternatives - -Instead of having a central registry we could have each plugin export functions for migration, reference extraction and reference injection. -Plugin consuming its state would need to import them from correct place. +- teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state. +- this proposal uses a different pattern than the current SO migration system which is having SOs register a migration function per version, where this system has users register a single function across all versions. That means consumer doesn't care what version his state is, he can assume he will always get the latest state out of the migrate function. # Adoption strategy From 79ae5aab5fc38490ec69615fb5c6dc89f20d0543 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Wed, 10 Jun 2020 01:45:08 -0700 Subject: [PATCH 05/11] review updates --- rfcs/text/0011_persistable_state_service.md | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index dcaeb1fbf4179..8e6c4de77bbc5 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -78,17 +78,17 @@ We also need to make sure that all the persited state containing references to s ```ts interface PersistableState extends Serializable {} -interface PersistableStateDefinition { +interface PersistableStateDefinition

{ id: string, // migrate function receives state and version string and should return latest state version // default is identity function - migrate: (state: PersistableState, version: string) => PersistableState, + migrate: (state: unknown, version: string) => P, // inject function receives state and a list of references and should return state with references injected // default is identity function - inject: (state: PersistableState, references: SavedObjectReference[]) => PersistableState, + inject: (state: P, references: SavedObjectReference[]) => P, // extract function receives state and should return state with references extracted and array of references // default returns same state with empty reference array - extract: (state: PersistableState) => { state: PersistableState, references: SavedObjectReference[] } + extract: (state: P) => { state: P, references: SavedObjectReference[] } } class PersistableStatePlugin { @@ -126,7 +126,7 @@ WARNING: If state id is known at compile time we should rather import the correc ## Handling corrupt state -This is up to the registrator, but we will try to come up with a good recomendation. +In current proposal handling of corrupt state is up to the implementator of PersistableStateDefinition. State incoming to migrate function could not match with version string provided so author should validate the incoming state first. ## EnhacedDrilldownEmbeddableInput @@ -212,16 +212,7 @@ First app arch team will add persistable state definitions for saved visualizati # How we teach this -What names and terminology work best for these concepts and why? How is this -idea best presented? As a continuation of existing Kibana patterns? - -Would the acceptance of this proposal mean the Kibana documentation must be -re-organized or altered? Does it change how Kibana is taught to new developers -at any level? - -How should this feature be taught to existing Kibana developers? +If state id is known at compile time we should rather import the correct utilities directly from expression plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. # Unresolved questions -Optional, but suggested for first drafts. What parts of the design are still -TBD? \ No newline at end of file From 3186dc2e08869ba44801275f2ca9875dbea307e4 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 30 Jun 2020 02:05:29 -0700 Subject: [PATCH 06/11] more updates --- rfcs/text/0011_persistable_state_service.md | 76 +++++++++++++++++---- 1 file changed, 62 insertions(+), 14 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 8e6c4de77bbc5..022af4e3300bc 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -7,16 +7,20 @@ Pluggable state from registry items can often end up stored inside saved objects, for instance: Embeddables, Expression strings, Url generators, ... -When plugin A (persister) stores some state that belongs to another plugin B a few issues arise: +When plugin A (persister) stores some state that belongs to another plugin B (registrator) a few issues arise: - How does persister know if plugin B state contains references to saved objects - How does persister migrate the saved object when it contains state that belongs to plugin B - How does persister know if state that belongs to plugin B contains state that belongs to plugin C +In order to solve the problem we are proposing implementing a `PersistableStateRegistry` to which registrators can register `PersistableStateDefinition` which persisters can then use to do the above tasks. + # Basic example +registrator plugin registers its `PersistableStateDefinition` + ```ts -const MY_STATE_ID = 'MyState'; +export const MY_STATE_ID = 'MyState'; interface MyStateV76 extends PersistableState { object: string, @@ -44,7 +48,7 @@ const migrate = (state: unknown, version: string) => { v78 = { objectId: v77.objectId, value: v77.val }; } - return newState; + return v78; } const inject = (state: MyState, savedObjectReferences: SavedObjectReference[]) => { @@ -62,9 +66,11 @@ const extract = (state: MyState) => { persistableStateService.register(MY_STATE_ID, { migrate, inject, extract }); ``` +Persister plugin can then use the service to prepare state for saving or get it ready after loading. + ```ts - const stateReadyForPersisting = persistableStateService.get(MY_STATE_ID),beforeSave(myState); - const stateReadyForConsuming = persistableStateService.get(MY_STATE_ID).afterLoad(stateReadyForPersisting); + const stateReadyForPersisting = persistableStateService.get(id),beforeSave(myState); + const stateReadyForConsuming = persistableStateService.get(id).afterLoad(await savedObjectsClient.get(...)); ``` # Motivation @@ -75,6 +81,8 @@ We also need to make sure that all the persited state containing references to s # Detailed design +We plan to implement `PersistableStateRegistry ` which will be exposed under `share` plugin + ```ts interface PersistableState extends Serializable {} @@ -91,7 +99,7 @@ interface PersistableStateDefinition

{ state: P, references: SavedObjectReference[] } } -class PersistableStatePlugin { +class PersistableStateRegistry { setup(core, plugins) { return { // if one of the functions is not provided default is used @@ -112,25 +120,38 @@ class PersistableStatePlugin { } ``` +# Server or Client ? + +Persistable state service will be available on both, server and client, under same interface. We suggest to register your persistable state definition on both client and server even if you are using just one of those as you cannot know where the consuming side is running. + # Consuming and edge cases When plugin A wants to store some state that it does not own it should check with persistableStateRegistry for a registered persistable state definition and execute `beforeSave` function. ```ts - const stateForSaving = persistableStatePlugin.beforeSave(id, myState); + const stateForSaving = persistableStateRegistry.beforeSave(id, myState); ``` -WARNING: If state id is known at compile time we should rather import the correct utilities directly from expression plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. +WARNING: If state id is known at compile time we should rather import the correct utilities directly from the registrator plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. + +```ts + import { extract } from 'src/plugins/pluginOwningState'; + const stateForSaving = extract(myState); +``` ## Handling corrupt state In current proposal handling of corrupt state is up to the implementator of PersistableStateDefinition. State incoming to migrate function could not match with version string provided so author should validate the incoming state first. -## EnhacedDrilldownEmbeddableInput +## Handling enhancements + +Related github issue: https://github.com/elastic/kibana/issues/68880 -Drilldown plugin adds to the state of embeddable: +The registrator needs to explicitly support enhancements. Its state should contain an `enhancement` property which is an object where every key represents a state belonging to enhancer. + +Drilldown plugin (enhancer) adds to the state of embeddable (registrator): ```ts interface EnhancedDrilldownEmbeddableInput extends EmbeddableInput { @@ -139,24 +160,51 @@ interface EnhancedDrilldownEmbeddableInput extends EmbeddableInput { } } ``` -Embeddable is aware that it has an enhancements property on its state, where every key represents state of another plugin. +Embeddable (registrator) is aware that it has an `enhancements` property on its state, where every key represents state of another plugin. ```ts const inject = (state, references) => { Object.getKeys(state.enhancements).forEach(key => { - State.enhancements[key] = persistableStatePlugin.get(key).inject(state.enhancements[key], references); + State.enhancements[key] = persistableStateRegistry.get(key).inject(state.enhancements[key], references); } } ``` And drilldown plugin registers its state with persistableStateService. -## +## what about state which extends some base state ? + +With embeddables we currently have `SpecializedState extends BaseState` scenario. If possible don't do this but rather use the enhancements pattern described above. + +Migrations can still work, but there are edgecases they can't handle. For example `embeddable-baseInput` migrator would do something like: + +```ts +const migrate = (state, version) => { + // migrate the base input properties + + // run the extended type migration which will migrate the rest of the properties + return persistableStateRegistry.get(state.type).migrate(state, version); +} + +``` + +or do it the other way around, first running the extending class migration and then the base one. + +However there could be clashes using this approach due to conflicts with property names, so special care needs to be taken to not run into them. For this reason we suggest migrating towards enhancements pattern. + +For example if base class has a property named `x` which it wants to migrate to `y` and extended class has a property named `y` which it wants to migrate to `z` this will only work if we do extending class migration first, the other way around we will lose information during migration. + ## Use a `{pluginId}+{stateType}` pattern for your ID To avoid clashes with other unknown plugins. +A plugin could have a single `stateType`, for example `visualizations` plugin has a `savedVis` state, so it use `visualizations-savedVis` as id. Other plugins might have multiple state types they want to register. Lets say that we introduce a new state type to `visualizations` plugin called `visState` we would use `visualizations-visState` as id. + +A good example of plugin exposing multiple state types is expressions plugin, where each of the functions it registers into the registry will also register a persistable state definition. + +Persistable state ID should be a const exported on plugin contract. + ## We have to avoid shared persistable state So we have to do two things: @@ -212,7 +260,7 @@ First app arch team will add persistable state definitions for saved visualizati # How we teach this -If state id is known at compile time we should rather import the correct utilities directly from expression plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. +If state id is known at compile time we should rather import the correct utilities directly from registrator plugin. As using the registry can hide dependencies between plugins when possible plugins should directly depend on plugins providing the state they want to store. # Unresolved questions From 09d361b6f522b6487df5994b6358008c69b8f515 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 30 Jun 2020 02:15:28 -0700 Subject: [PATCH 07/11] more updates --- rfcs/text/0011_persistable_state_service.md | 24 +++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 022af4e3300bc..f2b3ff6043560 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -32,7 +32,7 @@ interface MyStateV77 extends PersistableState { val: number, } -interface MyState extends PersistableState { +export interface MyState extends PersistableState { objectId: string, value: number, } @@ -63,7 +63,9 @@ const extract = (state: MyState) => { return [{ ...state, objectId: 'objectId' }, references]; } -persistableStateService.register(MY_STATE_ID, { migrate, inject, extract }); +export const persistableStateDefinition = { migrate, inject, extract }; + +persistableStateService.register(MY_STATE_ID, persistableStateDefinition); ``` Persister plugin can then use the service to prepare state for saving or get it ready after loading. @@ -84,9 +86,9 @@ We also need to make sure that all the persited state containing references to s We plan to implement `PersistableStateRegistry ` which will be exposed under `share` plugin ```ts -interface PersistableState extends Serializable {} +export interface PersistableState extends Serializable {} -interface PersistableStateDefinition

{ +export interface PersistableStateDefinition

{ id: string, // migrate function receives state and version string and should return latest state version // default is identity function @@ -120,6 +122,20 @@ class PersistableStateRegistry { } ``` +Every plugin that exposes its state should register its `PersistableStateDefinition`. In this definition he needs to expose `migrate` function which can take in any state version and migrate it to the latest state as well as `inject` and `extract` functions, which should be able to inject and extract saved object references from the latest state version. + +This same persitableStateDefinition should also be exposed on the plugin contract. On top of that plugin also needs to export type definition for the latest state version. Name of type definition should not include a version number but always be the same. (see the basic example) + +Plugins consuming state not owned by them should always use persistableStateDefinion of the registrator plugin. When loading state they need to: + +`const state = persistabelStateDefinition.inject(persistableStateDefinition.migrate(loadedState, version), references)` + +and when saving state they always need to: + +`const { state, references } = persistableStateDefinition.extract(runtimeState)` and store references separatly. + +This way consuming plugin can always be sure that its working with the latest state version. + # Server or Client ? Persistable state service will be available on both, server and client, under same interface. We suggest to register your persistable state definition on both client and server even if you are using just one of those as you cannot know where the consuming side is running. From 0a54e203cb01c753cd970c1162797eae43101745 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Mon, 13 Jul 2020 07:10:45 -0700 Subject: [PATCH 08/11] ... --- rfcs/text/0011_persistable_state_service.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index f2b3ff6043560..471e75ac7f4a7 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -54,13 +54,13 @@ const migrate = (state: unknown, version: string) => { const inject = (state: MyState, savedObjectReferences: SavedObjectReference[]) => { return { ...state, - objectId: savedObjectReferences.find(ref => ref.name = state.objectId)?.id; + objectId: savedObjectReferences.find(ref => ref.name = 'mystate.objectId')?.id; } } const extract = (state: MyState) => { const references = [{ name: 'objectId', id: state.objectId, type: 'savedObject' }]; - return [{ ...state, objectId: 'objectId' }, references]; + return [{ ...state, objectId: 'mystate.objectId' }, references]; } export const persistableStateDefinition = { migrate, inject, extract }; @@ -83,7 +83,7 @@ We also need to make sure that all the persited state containing references to s # Detailed design -We plan to implement `PersistableStateRegistry ` which will be exposed under `share` plugin +We plan to implement `PersistableStateRegistry ` which will be exposed under `PersitableState` plugin ```ts export interface PersistableState extends Serializable {} @@ -265,6 +265,13 @@ interface EmbeddableInputV2 { It's probably a rare occurrence, but it's also very risky. EmbeddableInput thinks it owns defaultTitle. Let's say it decides to change the name yet again, so in V3 changes it to customPanelTitle and removes defaultTitle. Let's also say VisualizeEmbeddable author doesn't add a migration for V2 or V3. A user migrates to V3, VisualizeEmbeddable just had its defaultTitle state wiped out unknowingly (types will still say it's a required parameters). This has the potential to permanently break saved objects with no way to undo the migration. +# What saved object migrations + +With saved object migrations (or any other migration of potentially high amount of state objects) currently we try to detect when a migration needs to be performed. For example if there was no change in specific saved object between minors we will not try to read or write those objects from/to elasticsearch. + +With extention points to which 3rd party developers might register their own items which expose state that is not possible. For example dashboard saved object might contain state from any embeddable. As we don't know about all those embeddables we need to always fetch all saved objects and run migrations on all of them. +If expecting to handle large amount of state objects you should always deep compare input and output state and filter out objects without changes to avoid to many updates in the database. + # Drawbacks - teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state. From 6d132665d0aaa5662e9efa5b040752b6114a6a6e Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 14 Jul 2020 05:59:31 -0700 Subject: [PATCH 09/11] ... --- rfcs/text/0011_persistable_state_service.md | 51 +++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 471e75ac7f4a7..542e07ac6d0fa 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -272,6 +272,57 @@ With saved object migrations (or any other migration of potentially high amount With extention points to which 3rd party developers might register their own items which expose state that is not possible. For example dashboard saved object might contain state from any embeddable. As we don't know about all those embeddables we need to always fetch all saved objects and run migrations on all of them. If expecting to handle large amount of state objects you should always deep compare input and output state and filter out objects without changes to avoid to many updates in the database. + +# Full look into a complex example: saved dashboards + +saved dashboard state looks something like this: +```json +{ + panels: [{ + ...embeddable, + enhancements: { + drilldowns: { + ....drilldownStateForThisEmbeddable + } + } +}], +} +``` + +where any plugin can add to enhancements a property name matching its state id with value being that state. +also any embeddable can extend the base embeddable input type. + +dashboard needs to register a persistable state service with migrate function looking something like this: +```ts +const migrate = (state: unknown, version: string) => { + // do the internal state migration like state.title etc (owned by dashboard) + + // and then migrate panels (state not owned by dashboard) + state.panels = state.panels.map(p => baseEmbeddablePesistableStateDefinition.migrate(p, version)); +} +``` + +all panels extend base embeddable, which would register its baseEmbeddablePErsistableState with migrate function like this: + +```ts +const migrate = (state: unknown, version: string) => { + // do the internal state migration like state.title etc (owned by base embeddable) + + // then migrate enhancements where every key represents a state not owned by embeddable + Object.keys(state.enhancements).forEach(k => { + state.enhancements[k] = persistableStateRegistry.get(k).migrate(state.enhancements[k], version); + } + + // migrate the actual embeddable type + // here we run into an issue as base embeddable input and specific embeddable input should not collide (thru any version) ... + // this is mentioned before "We have to avoid shared persistable state" topic + // we should make all embeddables provide their custom inputs under a base property to prevent that + return persistableStateRegistry.get(state.type).migrate(state, version), +} + +``` + + # Drawbacks - teaching impact: everyone storing state from belonging to another plugin will need to remember to use this service when saving and loading state. From 514e7342c9792f0900e2316ecb7b9d37df8ee653 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Tue, 14 Jul 2020 15:24:30 +0200 Subject: [PATCH 10/11] fix snippet format --- rfcs/text/0011_persistable_state_service.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 542e07ac6d0fa..ebb2b77946430 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -276,7 +276,7 @@ If expecting to handle large amount of state objects you should always deep comp # Full look into a complex example: saved dashboards saved dashboard state looks something like this: -```json +```ts { panels: [{ ...embeddable, From a7c1550252473dbeabc503ee02adbd5f359be309 Mon Sep 17 00:00:00 2001 From: ppisljar Date: Tue, 11 Aug 2020 00:37:12 -0700 Subject: [PATCH 11/11] ... --- rfcs/text/0011_persistable_state_service.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/rfcs/text/0011_persistable_state_service.md b/rfcs/text/0011_persistable_state_service.md index 542e07ac6d0fa..c468e2ed8f830 100644 --- a/rfcs/text/0011_persistable_state_service.md +++ b/rfcs/text/0011_persistable_state_service.md @@ -37,7 +37,9 @@ export interface MyState extends PersistableState { value: number, } -const migrate = (state: unknown, version: string) => { +type MyStateOld = MyStateV76 | MyStateV77 | MyState; + +const migrate = (state: MyStateOld, version: string) => { let v76 = version === '7.6' ? state as MyStateV76 : undefined; let v77 = version === '7.7' ? state as MyStateV77 : undefined; let v78 = version === '7.8' ? state as MyState : undefined; @@ -63,7 +65,9 @@ const extract = (state: MyState) => { return [{ ...state, objectId: 'mystate.objectId' }, references]; } -export const persistableStateDefinition = { migrate, inject, extract }; +export type MyStatePersistableStateDefinition = PersistableStateDefinition; + +export const persistableStateDefinition: MyStatePersistableStateDefinition = { migrate, inject, extract }; persistableStateService.register(MY_STATE_ID, persistableStateDefinition); ``` @@ -88,11 +92,14 @@ We plan to implement `PersistableStateRegistry ` which will be exposed under `Pe ```ts export interface PersistableState extends Serializable {} -export interface PersistableStateDefinition

{ +export interface PersistableStateDefinition< + Old extends PersistableState = PersistableState, + P extends PersistableState = PersistableState +> { id: string, // migrate function receives state and version string and should return latest state version // default is identity function - migrate: (state: unknown, version: string) => P, + migrate: (state: Old, version: string) => P, // inject function receives state and a list of references and should return state with references injected // default is identity function inject: (state: P, references: SavedObjectReference[]) => P,