SharingSavedObjectProps Injected into Dashboard Saved Object

[ThomThomson]

Kibana version: 7.16.0

Describe the bug:
When a user unlinks a Map or Lens embeddable from the library, the sharingSavedObjectProps is injected into the attributes. This causes an unexpected unsaved changes badge on the dashboard, but also means that sharingSavedObjectProps is saved into the dashboard saved object.

Steps to reproduce:

1. Load a dashboard
2. Add a Lens or Map by reference
3. Unlink the Lens or Map from the library
4. Save the Dashboard

Expected behavior:
sharingSavedObjectProps should never be saved into the saved object

Cause:
The Maps & Lens attribute service unwrap methods were mistakenly modified in #112606 and #110059. This is because the saved object is loaded in order to unwrap it as part of the transformation to a by value saved object. When the saved object is loaded it also returns the outcome and alias. These are mistakenly injected into the by value attributes of the panel. By Value panels should not have sharingSavedObjectProps because they are not related to any saved object.

Screenshots (if relevant):
Dashboard saved object with savedObjectProps:

This should be fixed by reverting changes to x-pack/plugins/lens/public/lens_attribute_service.ts and x-pack/plugins/maps/public/map_attribute_service.ts from the above PRs.

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[nreese]

This should be fixed by reverting changes to x-pack/plugins/lens/public/lens_attribute_service.ts and x-pack/plugins/maps/public/map_attribute_service.ts from the above PRs.

Reverting changes to x-pack/plugins/lens/public/lens_attribute_service.ts and x-pack/plugins/maps/public/map_attribute_service.ts will cause problems with surfacing saved object loading conflicts. Both lens and maps use sharingSavedObjectProps to later handle conflicts and this state still needs to be passed along with saved object

[ThomThomson]

Hey Nathan, I originally thought that even after removing those changes, the saved object conflicts will still be surfaced, because the unwrap method is used for unlinking from the library and when the embeddable is by value that information isn't relevant.

I know that the unwrap function is also used when loading an embeddable by reference, so it could be possible that simply removing those changes isn't enough.

If that is the case, do you have any ideas on how to handle this? Maybe we need to handle unwrap differently depending on by value or by reference

[nreese]

If that is the case, do you have any ideas on how to handle this? Maybe we need to handle unwrap differently depending on by value or by reference

There are a couple of places where this issue can be resolved

1. In the MapEmbeddable.getInputAsValueType and LensEmbeddable.getInputAsValueType methods, remove sharingSavedObjectProps from results of AttributeService.getInputAsValueType. This may be brittle if AttrbitueService.getInputAsValueType is called in other locations, you would have to be careful to remove sharingSavedObjectProps any time AttrbitueService.getInputAsValueType is called.
2. Update signature of AttrbitueService.unwrapAttributes method to return { attributes, attributesMeta }. Then AttrbitueService.getInputAsValueType can just spread attributes into the results. MapsAttributeService and LensAttributeService can put sharingSavedObjectProps into attributesMeta and use as needed.

[nreese]

I am also concerned this issue was not identified by typescript compile failures. AttributeService.getInputAsValueType returns type Promise<ValType> while AttributeService.unwrapAttributes returns type Promise<SavedObjectAttributes>.

For MapAttributeService,

SavedObjectAttributes is typed as

type MapDoc = MapSavedObjectAttributes & {
  sharingSavedObjectProps?: SharingSavedObjectProps;
  references?: SavedObjectReference[];
};

ValType is typed as

export type MapByValueInput = {
  attributes: MapSavedObjectAttributes;
} & EmbeddableInput & { filterByMapExtent?: boolean } & MapEmbeddableState;

How come there are no typing errors when returning SavedObjectAttributes as MapByValueInput since MapByValueInput does not contain sharingSavedObjectProps?

[kobelb]

@ThomThomson and @nreese - what is the impact of sharingSavedObjectProps being persisted in the saved object? Will it prevent Dashboards from being viewed?

[ThomThomson]

It doesn't prevent dashboards from being viewed. The only tangible issue from the user's point of view is dashboards still showing the unsaved changes badge after being saved any time after a panel has been unlinked.

In terms of seriousness though, I'm generally very wary about issues that accidentally affect the structure of saved objects. That said, I am not sure this warrants a blocker. @jportner may also be interested in the discussion going on here.

@nreese, great point about the typings not catching this, I will start there when I begin working on this. I think this is also intertwined with our earlier discussion about the mapBuffer key causing unsaved changes, and I'm thinking that the presentation team should eventually build a more structured fix to this, including some changes to the embeddable infrastructure to move the responsibility for what parts of input is diffed / persisted into individual embeddables.

[kobelb]

@ThomThomson I share your generalized concern but given the concrete impact, it doesn't seem like it should be a blocker unless we're putting ourselves in a really bad position long-term. Do we have the option of using a saved-object migration to fix this behavior in a future version?

[ThomThomson]

Yes we do have that option. In that case, I also support removing this as a blocker.

[kobelb]

Awesome, thanks @ThomThomson