[content mgmt / maps] Saved Object wrapper for Content Management API

[mattkime]

Summary

Abstract class for implementing content storage for saved objects.

Follow up to #155342

The maps content management storage class for maps has been reduced to -

export class MapsStorage extends SOContentStorage<MapCrudTypes> {
  constructor() {
    super({
      savedObjectType: CONTENT_ID,
      cmServicesDefinition,
      searchArgsToSOFindOptions, // converts map search args to saved object api args
      enableMSearch: true,
    });
  }
}

[mattkime]

This class is where the work happens.

[mattkime]

This is the result. Expressing your SO / content mgmt api integration in a very concise number of lines of code. Easier to see if you look at the file view - https://github.com/elastic/kibana/blob/861b8af7545082b2aae79205be6c3e98ad480d55/x-pack/plugins/maps/server/content_management/maps_storage.ts

[mattkime]

@Dosant We should find a better api for this. I'd prefer to use some sort of boolean to determine whether mSearch is enabled. As best I can tell this is largely a reconfiguration of code used elsewhere

[Dosant]

will improve. #155718
hopefully non blocking

[elasticmachine]

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

[sebelga]

Thanks for putting this PR together. I keep having some concerns about trying to fit it all under one package in terms of interfaces and schema.

At the very least I think that the utility package should follow the same versioning folder mechanism that the one I put for maps. It will be versioned based on change in the underlying SO server client.

We need to be able to version and up/down grade all those common interfaces (options, SO meta...) and schema somehow.

Let's take an example: in the SavedObjectSearchOptions interface there is an namespaces field which is a string[]

• How can we make it evolve to { name: string; group: string }?
• How is the up transform going to occur when the UI will still send a list of string and the server has been upgraded to accept an object?

It seems that we will have to also send the request version to the utility that will in turn return the correct schema... This starts to get complex (in my head at least 😄 )

[mattkime]

@sebelga

At the very least I think that the utility package should follow the same versioning folder mechanism that the one I put for maps. It will be versioned based on change in the underlying SO server client.

This confuses me a bit. If there's an underlying change in the SO server client then all SO CM integrations will need to work with it. I don't think the callee's (in this case the SO server client) need to be versioned in the same way that the arguments need to. If there's something I'm missing here, I suspect the code in this PR would have the same weaknesses as the maps CM integration code.

We need to be able to version and up/down grade all those common interfaces

Oh, perhaps I'm misunderstanding and this is a restatement of a previous concern - that the various Option types are too closely tied to the actual SO api. If thats the concern, then I think this is addressed by the existing transform code. I think its okay to have arguments that map to SO api arguments as long as they're well defined AND we could transform them. If a map of arguments was being tossed into the SO api without further definition that would be clearly wrong.

Let's take an example...

We have two options. Either we can inspect the items in the namespace array and convert the strings to objects OR we could add an namespaceObjects argument and we could handle the two types appropriately. Perhaps I'm repeating a familiar debate regarding how to transform apis.

A big part of my reason for arguing against using args that are purposefully different from SO args is that I suspect it creates unnecessary work. Implementing the CM api can be very easy, thinking about how to abstract every call into the SO api is much more work. I do think it may eventually be necessary to change these calls but its possible that we could delay it
a very long time.

---

I'd like to state any outstanding concerns as plainly as possible -

1. The cm options map too directly to SO apis.
2. Versioning of the whole SOContentManagement class.

Is this correct?

[sebelga]

@mattkime

If you are 100% sure you got a solution for the concerns I raised about being able to version any object that comes in for any SO, then I trust your judgment.

From where I am standing I see it even more complex now than before to handle it. But with that said... we still don't have the final verdict if we even have to worry about it (e.g. if we have a blue/green upgrade we won't have to do any kind of transform in the storage class).

Otherwise your abstraction might start looking like:

async create(ctx, data, options) {
    const transforms = ctx.utils.getTransforms(this.cmServicesDefinition);
    const soClient = await savedObjectClientFromRequest(ctx);
   
    // NEW: we need a new definition for the shared schemas
    const transformsForAbstraction = ctx.utils.getTransforms(<some-definition>);

    // Validate data & UP transform them to the latest version
    const { value: dataToLatest, error: dataError } = transforms.create.in.data.up<
      Types['Attributes'],
      Types['Attributes']
    >(data);
    if (dataError) {
      throw Boom.badRequest(`Invalid data. ${dataError.message}`);
    }
    
    // NEW: Add this block to handle BWC for **shared** option schema
    // But do we really need it, what if the options are completely different for one specific SO type??
    const { value: optionsToLatest, error: optionsError } = transformsForAbstraction.create.in.options.up(options);
    if (optionsError) {
      throw Boom.badRequest(`Invalid options. ${optionsError.message}`);
    }
    
    //  What do we do here?? Do we do it or is it behind a if condition?
    const { value: optionsToLatest, error: optionsError } = transforms.create.in.options.up<
      Types['CreateOptions'],
      Types['CreateOptions']
    >(optionsToLatest1);
    if (optionsError) {
      throw Boom.badRequest(`Invalid options. ${optionsError.message}`);
    }

    const createOptions = this.createArgsToSoCreateOptions(optionsToLatest);
}

As you can see it starts to get very complex and IMO pretty hard to maintain.

[sebelga]

Approving to unblock the work. It is still unclear to me how we will migrate the shared options object in the abstraction as mentioned in my previous comments.

As we don't have yet a way to test a server upgrade and a UI doing request from an older version we can revisit later. But I do think at some point we'll have to address this problem 😊 and my personal take is that abstracting too early until all unknown are known might create more problem down the road.

[jughosta]

Code changes LGTM 👍

[jughosta]

Can we use tagsToFindOptions here too?

[nreese]

kibana-gis changes LGTM
code review only

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: b2282fc

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/content-management-utils	57	112	+55

Unknown metric groups

API count

id	before	after	diff
@kbn/content-management-utils	106	177	+71

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	19	21	+2
securitySolution	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
securitySolution	480	484	+4
total			+6

History

• 💚 Build #127767 succeeded 9e09aed
• 💚 Build #126171 succeeded 770327d
• 💔 Build #124340 failed aa728d9
• 💛 Build #124166 was flaky 5685fd4
• 💚 Build #123272 succeeded b885823
• 💚 Build #123240 succeeded 5742da2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @mattkime