Inspect view in saved objects management should show read-only JSON

[rudolf]

Unblocks #46435

Saved Objects can define an editUrl which will add an "inspect" item to the actions context menu.

Clicking that item brings up an ominous warning:

Proceed with caution!
Modifying objects is for advanced users only. Object properties are not validated and invalid objects could cause errors, data loss, or worse. Unless someone with intimate knowledge of the code told you to be in here, you probably shouldn’t be.

I believe we should deprecate this functionality for the following reasons:

• There's no validation on the edits, so, as the warning suggests, editing an object is very likely to break Kibana, or worse, cause a migration failure and downtime in a future Kibana version upgrade.
• Editing what's effectively raw JSON is a bad user experience and we should strive to provide all necessary functionality through the plugin's UI.
• Workflows that depend on editing objects requires giving users saved objects management permissions which ideally should be restricted to system administrators Deleting saved searches outside of objects management #57026
• This might have originally been important to update the index pattern references after importing visualizations, but this is now possible during the saved objects import process.

It's not possible to immediately remove these views as some edit/delete functionality would have to be exposed from the plugins that own these objects such as #57026 As a first step we can deprecate the API in Platform and then work with teams to transition any known workflows to the plugin's UI itself.

Related issues:

• Allow objects/flatten fields in saved objects management #59371
• Deleting saved searches outside of objects management #57026
• Management Saved Objects overhaul #13329 (comment)

[elasticmachine]

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

[rudolf]

@AlonaNadler What do you think about the suggestion? Would you be able to help create a list of common workflows that require editing saved objects directly? Then we can work through that list and see if we should create the UI to support these workflows or if it would be sufficient to tell users to use the Saved Objects API to edit their objects.

[AlonaNadler]

When you have an error for one of the existing visualizations, which can happen sometimes after upgrades or if you recreated the index pattern. There is no way to solve the error besides the inspect editing the JSON directly. This is far from ideal but that is the current situation.

[timroes]

@rudolf As talked besides this, I am all in favor for removing the editing support for saved objects. Instead as earlier mentioned we could just show the JSON as it is via an "inspect" button. That would then also allow to show ANY saved object and not just the "old" ones that are currently using the old client side saved object implementation (and thus solve #67607).

If we still want to give (advanced) users editing over saved objects, I'd still suggest, we simplify this screen significantly and just replace it by a single JSON editor. This way we could get rid of a lot of old code for that management section and in all the apps, that currently use the old legacy saved object classes (see above linked issue).

As an alternative we should make sure #67607 is addressed differently, and we actually decouple the old implementation from the management section. Since this sounds like quiet some effort, that imho does not outweigh the benefit of having separate input boxes on that management page (especially since in most of them the user anyway need to put JSON again...), I'd rather suggest we are going the "only JSON" route - and either only show it to users, or if we still really require, also have the user edit it (but as one JSON).

[pgayvallet]

I'm a simple man. I see 'that would allow to remove SO loaders from SO management', I upvote.

However, the loaders are not only used for the edit view, but also for the legacy import, see

kibana/src/plugins/saved_objects_management/public/management_section/objects_table/components/flyout.tsx

Lines 72 to 73 in 159369b

	export interface FlyoutProps {
	serviceRegistry: ISavedObjectsManagementServiceRegistry;

Hopefully we would be able to get rid of the legacy import in 8.0 (even if it's now one year from now)

Still, really in favor of @timroes proposal to replace the whole edition view with a single json editor for the whole content. That would indeed greatly simplify the SOM codebase, and is not really that different functionally.

[timroes]

@pgayvallet As far as my understanding goes, Platform team wanted to remove those APIs with 8.0 completely. I think it would be worthwhile already beforehand changing the management saved object section to the "JSON only" editor for all saved objects. Even if it won't help us to remove the client side saved object implementations before 8.0, we can a) already remove part of their code (that was responsible for the management screen, e.g. I think the clientside SO mapping that we have was only used for it), and b) can already offer editing/inspecting all saved objects beforehand (I'd really appreciate if we could look into those saved objects e.g. of Lens more easily :-)

[timroes]

I am not 100% sure if we decided to make this now a read-only view of the JSON or not? I wonder also if it's worth checking with some support engineers how commonly they might use the "edit" features on the saved object page. I'd still be in favor of making it a read-only JSON editor, and simplify the code a lot.

I think given that we'll need to work on this some time before 8.0 (or have to rewrite a lot of logic in the depending legacy saved object implementations to get the resolve functionality to work), we should make a decicion on that.

cc @lukeelmers @alexfrancoeur

[alexfrancoeur]

I'm not sure if we made a decision on this earlier. Adding @thesmallestduck for product related thoughts on this change.

[timroes]

I've pulled a PR up with a PoC how this will look like, in case you want to test it out: #106746

[timroes]

@thesmallestduck @alexfrancoeur do you think we could get to a decicion here? We have work depending on this coming up soon, for which it would be good if we'd know if a replacement to a read-only view would be fine or not.

[alexfrancoeur]

@timroes did you ever end up speaking with support on this topic or was that a suggestion? With limited data points on usage, and given the fact it's such an advanced use case, I'd be comfortable with converting to a read only view. The POC linked looks good.

I do have one suggestion. If folks truly are looking to edit the JSON, can we provide an appropriate CTA? For example, we could simply add another button for "Download" and potentially some supportive text. "While we don't recommend editing the JSON directly, you can do so by exporting and re-importing" or something along those lines (would definitely need Gails help 😉 )

@thesmallestduck @timroes what do you think?

[timroes]

@alexfrancoeur sorry that was meant as a suggestion. I did not plan to reach out to support :D

[lukeelmers]

If folks truly are looking to edit the JSON, can we provide an appropriate CTA? For example, we could simply add another button for "Download" and potentially some supportive text.

++ I'm supportive of the idea of going to read only mode, but agree a clear CTA for how to edit if you absolutely must would be useful.

We could also consider allowing editing in dev mode (this is something I have done many times when debugging or trying to reproduce a specific issue).

[rudolf]

We've had many upgrades fail because of corrupt saved objects which means downtime for our users. We should not expect our users to edit any JSON and I don't think we should encourage them with a CTA. If advanced users can't find any other way than to edit JSON directly they can use the documented API.

[alexfrancoeur]

I think that's a valid argument and am happy to approach conservatively and wait to see what type of feedback we get

[lukeelmers]

happy to approach conservatively and wait to see what type of feedback we get

SGTM. As long as the json is read-only, I think we'll be good here. I don't feel particularly strongly about having a CTA, and it would be easy to add later if we changed our minds.

[TinaHeiligers]

@thesmallestduck @alexfrancoeur do you think we could get to a decicion here? We have work depending on this coming up soon, for which it would be good if we'd know if a replacement to a read-only view would be fine or not.

@timroes Core is starting work on converting to a read-only view. Are you still blocked on this work?

[TinaHeiligers]

@lukeelmers did we decide to go directly to a read-only view or are we deprecating the edit view first? Considering that the deadline to 8 is looming, I assume that we're skipping directly to read-only.

[timroes]

@TinaHeiligers we're not blocked on that for working on our saved object tasks (removing the legacy SavedObject Loader infrastructure), but we'll require it to go in the same version with our changes, since we'd otherwise lose capabilities to inspect those saved objects for all types where it previously worked.

[pgayvallet]

but we'll require it to go in the same version with our changes, since we'd otherwise lose capabilities to inspect those saved objects for all types where it previously worked.

The inspect view changes must land before your work imho, else it will be a FTR test failure nightmare.

[lukeelmers]

did we decide to go directly to a read-only view or are we deprecating the edit view first?

Yep, I think the agreed-upon plan here is to:

• Make view read-only
• Ensure changes land in the same release as the app team's work (presumably 7.16)

[TinaHeiligers]

I've been working towards something similar to the POC, replacing the entire Form with a CodeEditor and the saved object (without meta).

I’m not entirely sure if we want to show the whole saved object ‘as-is’ in the json structure or do some sort of prettifying before showing it as json. I know @alexfrancoeur gave the POC a thumbs up but I'm not sure if anyone else took a look.

For some SO types, like config, rendering the plain object works just fine:

For others, such as visualizations, we end up with stringified json in the json tree:

To try and get around that, I've been fiddling with a modified version of createFieldList, where we don't use the loaders to extract the field mappings:

export function createFieldList(
  object: SimpleSavedObject | SavedObjectWithMetadata
  // service?: SavedObjectLoader
): ObjectField[] {
  let fields = Object.entries(object.attributes as Record<string, any>).reduce(
    (objFields, [key, value]) => {
      // creates fields from attributes
      return [...objFields, ...createFields(key, value)];
    },
    [] as ObjectField[]
  );
  // Special handling for references which isn't within "attributes"
  fields = [...fields, ...createFields('references', object.references)];
  // uses deprecated SO loader.
  // if (service && (service as any).Class) {
  //   addFieldsFromClass((service as any).Class, fields);
  // }

  return fields;
}

The question is, do we even need to worry about stringified values in the json tree?

It's taking some time and I'd really like a sanity check before spending too much more time on it if it's not needed.
@lukeelmers @rudolf @pgayvallet WDYT?

[timroes]

Ensure changes land in the same release as the app team's work (presumably 7.16)

We're not sure if we'll make 7.16 or 8.0 with them, but as long as you are "before us" I don't think there should be any problem with that.

[timroes]

The question is, do we even need to worry about stringified values in the json tree?

My two cents: This json as string converted should only be an issue with old saved objects (mainly Visualizations, Discover, Dashboard). New SO types should never use that way of storing information at all, but instead use unmapped object fields (or flattened if we need to read something at some point from deep within the saved object). See also #43673. Thus I would not worry around the stingified JSON values, and if we really consider that to be a problem in the presentation of the saved object, we should rather work on migrating the saved object format into the solution mentioned above instead.

cc @flash1293 @stratoula We originally thought about converting them at some point towards 8.0. I am not sure that is still realistic (since it's also a risky change to do), but maybe once Joe is back next week we can at least discuss this again, since it might get more relevant with this change here.

[pgayvallet]

This json as string converted should only be an issue with old saved objects (mainly Visualizations, Discover, Dashboard). New SO types should never use that way of storing information at all, but instead use unmapped object fields (or flattened if we need to read something at some point from deep within the saved object). See also #43673. Thus I would not worry around the stingified JSON values, and if we really consider that to be a problem in the presentation of the saved object, we should rather work on migrating the saved object format into the solution mentioned above instead.

Can't agree more on that. This whole stringified format is (well, one of) the root cause of the existence of the loaders in the first place (and imho doesn't make any sense compared to an unindexed nested field now), and trying to mimic the SO loaders to (properly) detect which fields are in JSON format may be complex and kinda go against one of the goal of this enhancement, which is the simplification of the edit page's code.

I'm +1 to just display all SOs without any kind of transformation.

[TinaHeiligers]

Thus I would not worry around the stingified JSON values...
I'm +1 to just display all SOs without any kind of transformation.

Thanks everyone, so glad I checked!

[flash1293]

Thought about this a bit and I think we shouldn't convert the existing saved objects and just keep them in their current shape because the risk / benefit trade off is not worth it. If everything goes according to plan, usage of the saved object types with stringified JSON should go down over time as well. I still agree with the sentiment here about not transforming the shown documents in any way - let's just show the JSON stored in a string, it's honest, it's what's happening with API integrations (or exporting and messing with the files) anyway and takes away some unnecessary complexity.