Remove special handling of savedObjectId in embeddables and containers

[stacey-gammon]

This PR removes the special handling of embeddables backed by saved objects vs those that are not backed by saved objects.

From the description of the PR:

• Changes container PanelState from

 { 
  type: string
  savedObjectId?: string
  explicitInput: {...}
 }

to

 { 
  type: string
  explicitInput: {
   savedObjectId?: string
  }
 }

• Removes addSavedObjectEmbeddable from the container interface. Now the function addNewEmbeddable can be used for all cases.

A follow up will be to remove the factory.createFromSavedObject function as that should not be needed anymore.

---

I believe that whether or not an embeddable is backed by a saved object should be an implementation detail of that embeddable and not require any external knowledge or special handling. As shown in the PR there is no need for this.

I think code like:

     embeddable = panel.savedObjectId	? 
        await factory.createFromSavedObject(panel.savedObjectId, input, this)
      : await factory.create(input, this);

Should be replaced by:

  embeddable = await factory.create(input, this);

@ppisljar has expressed concerns about this approach, filing here so we can discuss.

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[ppisljar]

i partly agree with Stacey here. i think if embeddable is backed by saved object, then indeed the consumer should not care about that and should just provide the right input.

HOWEVER: i think in general embeddables should not be backed by saved objects, for example visualize embeddable should receive visState on the input and not savedObjectId. Embeddable should not care where this visState comes from, so you can instantiate it from saved object, from some state in memory or whatever else. Embeddable internally should not use savedObjectLoader, but just work with the state given. To make the case where we actually have saved objects that back embeddables (saved visualization) the associated embeddable factory should provide you with easy way of loading the saved object and giving you back an embeddable which state is based on the saved object. In this case then the consumer needs to know if it wants to create embeddable from some state it already has or if it wants to load some saved object to get the state it needs to create the embeddable. (the consumer in our case being dashboard)

this keeps embeddables (visualize for example) simple(r), (no need to do loading of saved objects and conditional handling of input) and with less dependencies (no dependency on saved object client for one).

this does require some additional handling in consumer (thats how it works today by the way), as consumer needs to be aware if its panel state contains the embeddable state or saved object id .... but imo thats the correct way of looking at it, its the dashboard that has the capability to have panels which link to other saved objects and panels which contain state by value.

[majagrubic]

I've already approved Stacey's PR here and I don't have much to add. The approach Stacey is taking seems perfectly fine to me - there is already a layer of abstraction, with the consumer calling just addNewEmbeddable and finally, getting rid of createFromSavedObject.

Changing this to a way Peter describes seems like a massive change in the way embeddables work, but I trust you have more context on this. I would be very curious to understand further reasons for considering this approach. From what I can tell, the changes that will required in the dashboard code to support this outweighs any benefits you may get from this "cleaner", more abstract approach. (Eg. is there a plan to add other embeddables not backed up by a saved object any time soon? To my knowledge, this will only be the case for visualizations in the dashboard for the forseeable future).

[ppisljar]

actually, if i am not missing something what i am proposing is closer to the current way embeddables work (or pretty much what we have at the moment)

[ThomThomson]

As I understood from the meeting, we have two main approaches for allowing embeddables to be created via savedobjectid(by reference) or by value, and the discussion is mainly around where to keep this logic.

1: the logic can stay within the domain of the dashboard / container, which means that the embeddable / factory does not care where its state comes from, with the disadvantage that all embeddables will be treated the same way. Additionally, transferring an embeddable between being stored as a reference and a value will have to be re-thought.

2: the logic can be stored in the embeddable factory. This has the advantage of removing the special treatment of savedobjectid by the container, and the advantage of allowing each embeddable factory to decide its own logic for instantiation. With this approach we would have to be careful to avoid duplication of effort, and we would have to be aware of special cases like when a savedobjectid is changed via updateinput.

I am leaning towards option 2 because it gives embeddable authors more freedom to decide what to do with the input provided to them.

Please let me know if I've misrepresented or misunderstood anything!

[stacey-gammon]

Going to summarize my response/feelings from this mornings meeting.

What are we deciding between?

Option 1: Saved objects as a formal concept in the Embeddable system, with dedicated APIs and function calls. This is how it is today.
vs
Option 2: The Embeddable system having no knowledge or requirements about where Embeddables get their state from. This is left as an implementation detail.

Special considerations for each approach.

Containers deserializing children

Generic or specific "save to library" action

Option 1:

• Would only be supported on containers as savedObjectId is not part of the embeddable, but stored on the container. No embeddable rendered individually would be able to make use of a generic "save to library" function. I implemented an example here and you can see that the top example supports "edit and update library reference" and it is not rendered inside a container:

Option 1 code would look something like:

// Generic "save to library" function with saved objects as an formal
// concept on embeddable factories.
createSaveToLibraryAction = ({ factories, savedObjectClient }) => createAction<{ embeddable }> => ({
  displayName: 'Save To Library'

  isCompatible: ({ embeddable }) => (
    // This factory supports saved objects
    factories.get(embeddable.type).savedObjectMetadata &&
    
    // We need to check if this embeddable is already stored as "by reference"
    // on it's container. Going this route, only embeddables rendered inside containers
    // can be "saved to library".  Embeddables rendered individually, outside of a container,
    // could not support this functionality because the container holds the knowledge of
    // where state came from.
    embeddable.getRoot().getPanelForChild(embeddable.id).savedObjectId === undefined
  ),
  execute: ({ embeddable }) => {
    // ?? How do we ensure there is an exact mapping of input => saved object attributes?
    // Other ideas here?
    const savedObjectAttributes = embeddable.getInput();
    const id = savedObjectClient.create(embeddable.type, savedObjectAttributes);

    embeddable.getRoot().getPanelForChild(embeddable.id).savedObjectId = id;
    
    // ?? This wouldn't really work...
    // Not sure what to do here in a generic fashion.  Some data here could be
    // "overrides" that should stay, like if this panel had a "per panel time range"
    // set on it.  But other data could have been retrieved from the user during the
    // `factory.getExplicitInput` function and stored here, like if this was a
    // SayHelloEmbeddable that asked for the users name, and rendered "Hello ${name}",
    // this name value would need to be wiped out to ensure the embeddable was rendering
    // the attributes all from the savedObjectId. The container doesn't know the difference
    // 
    // I think this is pointing to the reason that the container should not be responsible
    // for handling this generically.
    embeddable.getRoot().getPanelForChild(embeddable.id).explicitInput = {};
  }
});

This would be a difficult feature to implement for saved searches, maps, in one go, because there is no generic way to get the savedObjectAttributes from an embeddable in a generic fashion. We'd have to probably make sure we excluded them from supporting this initially.

Option 2
Option 2 doesn't preclude us from using a generic action, though I still think it would be a premature abstraction at this point.

// Generic "save to library" function with saved objects as an internal
// detail on embeddables, not factories.
createSaveToLibraryAction = ({ factories }) => createAction<{ embeddable }> => ({
  displayName: 'Save To Library'

  isCompatible: ({ embeddable }) => (
    // This factory supports saved objects
    factories.get(embeddable.type).savedObjectMetadata &&
    // This embeddable is not already backed by a saved object.
    embeddable.getInput().savedObjectId === undefined,
  execute({ embeddable }) => {
    // Again if we are trying to support this generically, we need to somehow make sure
    // we can extract savedObjectAttribtues from input somehow. 
    const savedObjectAttributes = embeddable.getInput().attributes;
    const id = savedObjectClient.create(embeddable.type, savedObjectAttributes);
    embeddable.updateInput({ savedObjectId: id, attributes: undefined });

    // Pros here to above version: embeddable doesn't care whether it's being rendered
    // inside a container or by itself. Action still works.
  }
})

However, if we didn't want to make this generic at all in the first go, the kibana app team could register their own action specific to visualize embeddables:

// Specific "save to library" implementation.
createSaveVisToLibraryAction = ({ factories }) => createAction<{ embeddable: VisEmbeddable }> => ({
  displayName: 'Save To Library'

  isCompatible: ({ embeddable }) => (
    // This embeddable is not already backed by a saved object.
    // Note this is specific to this embeddable so we can use whatever name here we like.
    embeddable.getInput().visObjectId === undefined,
  execute({ embeddable }) => {
    // I wrote this embeddable, I know I put the attributes here, I can do this:
    const attributes = embeddable.getInput().attributes;
    const id = savedObjectClient.create(embeddable.type, attributes);
    embeddable.updateInput({ visObjectId: id, attributes: undefined });

    // Very similar to above version but I have more flexibility with storage.
    // I don't _have_ to store saved object data in input.attributes, I could
    // derive it and clear out what I want.
    // Pros: More flexible to the author of the embeddable.
    // Cons: I have to write and maintain this code.

    // I personally think this is the best way right now and that
    // creating an abstraction right now would be premature.
  }
})

Add panel flow

option 1 continues as it is now, conditionally calling either create or createFromSavedObject on the factory.

pseudocode:

// Today
function addPanelFlyout() {
  const addPanelFromSavedObject = (savedObjectId: string, type: string) => {
    // Forcing a 1:0-1: mapping of factory to saved object type.
    const factory = findFactorySupportingType(type);
    const panel = factory.createBySavedObjectId(savedObjectId);
  }

  return (
    <SavedObjectFinder addFunction={addPanelFromSavedObject} />
    <CreateNewButton />
  );
}

Option2

Still not my favorite implementation, but improved to use only the single create function on the factory:

function addPanelFlyout() {
  const addPanelFromSavedObject = (savedObjectId: string, type: string) => {
    // Forcing a 1:0-1: mapping of factory to saved object type.
    const factory = findFactorySupportingType(type);
    // I don't like this either!  How do we know this embeddable doesn't need any
    // additional information along with `savedObjectId`?  We are still treating it
    // like a special concept here.
    const panel = factory.create({ savedObjectId });
  }

  return (
    <SavedObjectFinder addFunction={addPanelFromSavedObject} />
    <CreateNewButton />
  );
}

However if we were to truly decouple embeddables from saved objects and completely make them an implementation detail, it would look like this:

// In an ideal world
function addPanelFlyout() {
  const renderFactoryOption(factory => <FactoryOption onClick={() => {
    // This function on the factory is in charge of requesting whatever data
    // it needs from the user to get input data - in the "backed by saved object world", that
    // would be showing the user a list of saved objects to choose from. In the "by value" world,
    // that would be an editor.
    // Note that this could be a totally different registry of functions. 
    // This would be tough to implement though because we'd have to change the design and not use
    // the SavedObjectFinder anymore.
    //
    // Bonus - multiple factories could support the same saved object type.
    //
    // You could do this still today, but the UI flow is awkward, it's through the "create new"
    // button.
    const input = factory.getExplicitInput();

    container.addChild(factory.create(input));
  }} />
  })

  return factories.forEach(renderFactoryOption);
}

There are no longer two paths in the add panel flyout - create from saved object, or create new, simply every factory exposes the ability to create new and handles requesting the necessary data from the user.

We could also use a separate registry that is responsible for asking the user for the data necessary to create an instance of an embeddable. However, right now this is captured with the Factory.getExplicitInput function.

This change would require a change to the UX design however. I think we will want to change this anyway, since the "create new embeddable" flow is hidden, especially with the latest dashboard changes. There are three Create new buttons, one which exposes totally different things than the others!

Create news hooked into the visualize extension point:

Create new hooked into embeddable factory creation:

Summary

The goal of this PR is to make savedObjectId an implementation detail of Embeddables, removing it from storage on ContainerState. I believe it:

• Factories should be lightweight functions to create an Embeddable. Doing

  create(input) => new MyEmbeddable(input)

is perfectly acceptable (and how almost all of the example embeddables work).

• Containers should not be responsible for retrieving state from a saved object for an embeddable, nor required to understand this difference in order to conditionally call different factory.create methods.

• Data fetching should be an internal implementation detail of embeddable.

• Push more logic to embeddable authors.

• Simpler generic Embeddable system.

[stacey-gammon]

I'd like to call out another issue with factories converting savedObjectId -> input and returning a pure "by value" embeddable.

If the savedObjectId is not persisted in EmbeddableInput, but only on the container, this means the only way an action can grab the savedObjectId is from the container, via:

embeddable.getRoot()?.getPanelForChild(embeddable.id).savedObjectId 

which means these actions only work on embeddables inside a container.

Dashboard is an embeddable that is not rendered inside a container. The SIEM team wants to render a dashboard in it's Overview page and do so via reference.

They want functionality to support editing the dashboard (by reference) inside the SIEM app and saving edits. (#52680 (comment)).

How would a save action work for a "dashboard by reference", when only the factory has the saved object id upon creation time, and the instance of the embeddable does not?

[peterschretlen]

I appreciate the discussion here, and that there is no absolute correct answer. There’s a trade off between keeping Embeddables simpler with no savedObject dependency, and giving authors leeway.

Stacey laid out the case and rationale, it makes sense to me I think we should align and proceed with pushing responsibility of getting state to the embeddable author. The arguments of simplicity and reduced dependencies are valid but I'm not seeing a huge downside. Unless I'm missing something big can we align and close this off @ppisljar @stacey-gammon

[ppisljar]

i didn't read whole stacey's response yet, but i want to raise this again:
what does #61692 solve, that we cannot do otherwise ?
if its nothing, then we are having a pretty long discussions around refactoring that would be mostly aesthetically and where we have very different opinions on which of the options is prettier. so could we just say to not do this for now and move on ?

[majagrubic]

I agree with the conclusion here and the approach seems perfectly reasonable to me.

I just have a general suggestion for these discussions in the future:
Can we have a detailed write-up like this with options and pros and cons of each approach laid out before the meeting? It's very hard to catch-up on context on the fly with 7 people and 8 different opinions in the same room.

[stacey-gammon]

It's not just aesthetic, it's needed to support functionality like save and edit on embedded dashboards by reference.

I'm going to move forward with the PR, as a decision needs to be made here, and Peter S. weighed in, in support.

@majagrubic - agree. I was waiting on Peter P.s response to help me understand what exactly he didn't like in order to form a response. This came the morning of the meeting so there was time for offline discussion in prep. I agree it's better to get the sides of the argument out before a meeting so there is time for everyone to form an opinion with full knowledge.

Thanks everyone for weighing in and contributing to this discussion!

[elasticmachine]

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