Handling saved object references in expressions

[timroes]

Via #59598 and the resulting meetings we've figured out, that we will definitely run into situations where ids of saved objects might change and saving ids of saved objects inside any saved object part can break. Therefore saved objects have a references concept where they can reference other saved objects. Other fields within the saved objects should now only contain the reference name and the consumer of the saved object need to take care about looking that name up in the references.

We have some places we need references in expression, e.g. the index pattern id in esaggs, the saved object id in canvas embeddable functions. Currently we store the id of the saved object directly in that. We need to make sure this is using references as well, so the changes in id of the references saved objects will be handled correctly by the saved object system.

I see currently two main methods of handling this:

• We keep storing ids in the expression, and need to explicitly update them whenever the referenced object's id change.
• We keep only reference names in the expression and thus the system already takes care about updating the references correctly.

For both of those cases, it makes sense to mark references to other saved object as such. The suggestion we talked about so far would be, to introduce a reference function via the core expressions plugin for that. This one will be used whenever you need to reference a saved object, so that a (simplified) correct expression would look like follows:

esaggs id={reference type="index-pattern" id="12345-67890"} | xy_chart

If we don't mark those explicitly, we would need to have implicit knowledge about what inside an expression could be a saved object id and what just a regular string, which sounds like a very unsafe solution.

Since we still need to write references correctly into the saved object (since otherwise export/import also breaks), the expressions plugin itself could also export a method to retrieve now all references from an expression:

extractReferences(expression: string | Ast) => SavedObjectReference[];

This can be called by the application when it saves the SavedObject to extract references from the expression.

Every saved object that currently still has ids inside expressions in it, will need to write a migration to properly extract them into the references array.

Explicitly update references

For this approach to happen, we'd assume that we have a way to let the core saved object system notify us about id changes. So we could plug in a callback, that will be called on our "saved object", whenever the id of one of its references change, e.g. onReferencesIdsChanged(changedId: Record<{ type: string; id: string }, string>) => void (mapping old types/ids to new ones).

An implementation of that method, would now replace the ids in the reference function parameters with the new ones. The expressions plugin should offer a utility that does that (updateReferences(expression: string | Ast, changedIds: Record<{ type: string; id: string}, string>) => Ast).

Advantages

• We can save the expression at is it in the saved object (see the other option for more explanation).
• Very explicit about what happens.

Disadvantages

• We would need a new callback in the saved object infrastructure to notify us about those id changes.
• There is currently no SavedObject class that a plugin would need to implement, so we currently don't have a clear place where this method would need to go, so most likely it would be a new registry mapping saved object ids, to onReferencesIdsChanged callbacks.

Only keep reference names in expressions

This approach assumes, we're never actually saving ids in the expression, but instead storing expressions only with reference names. This means the extractReferences method would not only return us a the references inside the expression, but also a rewritten expression, that will now only have reference names in it, e.g.:

const ex = `esaggs id={references type="index-pattern" id="12345-67890"} | xy_chart`;
const extract = extractReferences(ex);
console.log(extract.references);
// [ { type: 'index-pattern', id: '12345-67890', name: 'ref0' } ]
console.log(extract.expression);
// esaggs id={references type="index-pattern" ref="ref0"} | xy_chart

Whenever we load that saved object now, we basically would use a reverse (insert) method in the expressions plugin to convert that saved expression and an array of references back into the original expression.

insertReferences(ex: string | Ast, references: SavedObjectReference[]) => Ast;

Advantages

• Requires no modifications of the saved object core system, since the reference array is already updated correctly.
• We don't store duplicate information (in the above approach we store the same id once inside the expression and once inside the references array).

Disadvantages

• We store a different expression in the saved object, than what is actually used inside Kibana (after insertion).
• Slightly worse performance since we need to inject on every load, but I don't think this would be any notable difference at all (unless you're having reaaaally large expression ASTs).

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[timroes]

Plan

We'll go with Option 2 (Only keep reference names in expressions) and made the following decisions:

The reference function will return a custom reference type (instead of a string with just the id), so the AST parser can already assure, that a function that requires a reference only accepts the reference function and cannot be used wrongly by forgetting using the reference function.

The phasing is the following

1. Provide reference function [Team: AppArch]
2. Provide extract/inject utility [Team: AppArch]
3. Refactor kibana_context [Team: AppArch]
4. [Canvas] Convert savedMap/savedVisualization [Team: Canvas]
   a) making it accept only the reference type for it's id
   b) migrate saved objects to use the reference function
5. [Lens] esaggs in Lens [Team: App]
   a) making it's id accept only the reference type
   b) migrate lens saved objects to use reference in esaggs/kibana_context
   c) build_pipeline.ts -> use reference in the generated expression

Optional (in the sense of the migration):

• Create savedObject function and limit saved object API access to it

[ppisljar]

i think we need to recognize the difference between having an expression inside kibana at runtime and between persisting the expression. When we are not persisting the expression (saved object, url, ...) there is no problem with keeping references inside the expressions. The only time we need to extract them is when we are persisting them.

This is pretty much the same as with any state we want to persist, thats why we are thinking about implementing a global service to tackle this issue #62950

with this approach expressions can keep the references for as long as they they are not persisted. the persister needs to ask expression service to convert the expression into a version that can be persisted. At this point expression service will extract all the references and possibly to other changes to expression, return the new expression and the references array.

when loading them we need to ask expression service to inject the references.

in general, any plugin exposing any persistable state (state that can be persisted) would need to register with our service its state id together with migration, reference extraction and reference injection functions.

with expressions specifically this is gonna be a two layer implementation. Expression service will register its injectReferences function, which will:

• walk the ast tree
• for every function in the tree, check if the function has a injectReferences function registered (in this same registry, possibly with a prefix. .... expr_fn_esaggs) and run it
  same with extracting references or doing state migration.

[legrego]

@ppisljar based on what you're saying above, is #63358 going to supersede this issue, or is #63358 just the underlying infrastructure that will enable us to resolve this issue?

I'm trying to get a sense of a timeline for this, as it's necessary in order for virtually any of our existing saved objects to become sharable between spaces. The security team isn't blocked on this yet, but at some point we will have all of our pieces in place to migrate these existing objects, and that's something we'd love to do in 7.x if possible.

[ppisljar]

underlying infrastructure was provided in #72438

remaining work is now providing extract and inject functons for each of expression functions accepting references to saved objects as arguments (listed above).

kibana_context refactoring is planned for 7.11/7.12

we will most likely also provide extract/inject functions for our loadIndexPAttern expression function during 7.11/7.12

there are also some canvas expression functions which will need to provide those.

[legrego]

Thanks for the clarification and update, @ppisljar! If you could keep this issue updated with the team's progress over the next couple of minors, that would be greatly appreciated

[jportner]

@ppisljar Do you have any updates on this?

[ppisljar]

This continues to be pushed back due to other things getting a higher priority. Work on kibana_context function is planned again for 7.13 hopefully it does happen this time.