[content management / maps] Create abstract types for saved object usage with content management api #154985

mattkime · 2023-04-15T04:32:31Z

Summary

Abstract types for using Saved Objects with the content management api. This should significantly reduce the amount of code to use additional saved object types.

…fix'

…om:mattkime/kibana into content_management_api_so_type_abstraction

elasticmachine · 2023-04-15T16:05:33Z

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

sebelga · 2023-04-17T11:43:55Z

packages/kbn-content-management-utils/types.ts

+  name: string;
+}
+
+export interface CreateOptions {


I wonder if it is a good idea to lock the same Options interface for all content type. The idea was to allow each content to pass different options to the storage layer and type those interface + add a schema for them.

Maybe you are right that all call to create/update/delete/search will have the exact same options object but from where I am standing I can't make such a call. WDYT?

This is specifically aimed at Content Management items backed by Saved Objects. I'm familiar with the Saved Objects api and I don't see a benefit to creating specific Option types for CRUD operations. As best I can tell, only the plugin owner of a particular type should be using the CM api, the plugin can provide whatever API it wants.

This intends to be more of a starting point for consolidating Options rather than what should definitely work for everything. Perhaps we'll find a need to extend the Options and can do so at that time.

Let me see if I understand correctly.

I only added the references prop to create because that's the only SavedObjectCreateOptions that map uses. They are more fields in that interface but "map" does not use them so I limited the interface.

Here you are making this decision generic for all SO CM types..

Now if "lens" needs another option to be passed we will allow all other types to provide that other option, even though it is not related to they need and it does not do anything.

It seems to me that we are back in tying different content together and it might be hard to make evolve interfaces with time and version them separately.

How will we know if we need to up/down transform an option object?

How will you provide a different validation schema if "lens" need to pass this options object: { myLensOptionField: true, references: ['123'] }. Will now all CM types have to up/down transform this specific myLensOptionField, what will be the code line documentation for that "specific to lens" option?

Here you are making this decision generic for all SO CM types..

If you'd like, we could provide functionality to extend the Option types.

How will we know if we need to up/down transform an option object?

I've ignored that since we don't yet know how the Saved Object versioning will work, but perhaps it can be considered separately.

Should we consider Options versioning a necessary part of the current content management api? If so, I can revise this PR.

We should not have any well defined "options" across all CM types. If there is some field that would be useful for all CM types, it should not be an option, but built into the CM CRUD API directly.

Options should ideally never be used, they are an escape hatch, that can be utilised when some type has some extra behavior that does not fill well into the starndartized CM CURD API.

Having said that, I don't think any database related concern should be standartized in the CM layer. For example, references are a database concern (some database have them, some don't, some content types use them, some don't), the CM system should not know anything about the Saved Object references.

Also, with regards to TypeScript types: in CM plugin and packages there should be no types that deal with SavedObjects, i.e. all names that contain *SO*, savedObject*, etc.. do not belong to CM code. Those should be handled on the content provider level.

For example, if AX group thinks it is useful for their content types to standartize some "options" or SO functionality, you are free to do that, but that code should live outside of CM.

We want it to be unique to each content.

I understand what you're saying, but as someone considering how I'd apply the Content Management API to a number of saved objects, it doesn't make sense.

Why don't we want to reuse code in this circumstance?

Applying what I learned from the Maps implementation to the Data Views implementation was a big copy and paste project.

What I'm saying if you feel like you want to standartize "options" across your types, that is all good. But that standartized code should not live in CM, it could be some "AX storage helpers" code.

@vadimkibana

This is in a new content-management-utils package - is that good?

This is in a new content-management-utils package - is that good?

Yes, if that package is not expected to be owned by SharedUx.

I should clarify, I'm fine defaulting options to an empty object although I'd still want to provide something more detailed that a dev could easily grab.

@vadimkibana @sebelga

I think we should find some time to talk. Its unclear why some decisions have been made regarding the CM api and I'd like clarity before I continue work on this.

sebelga

Overall looks good. I just have some concerns about having a single Option interface for all the CM types and how BWC will be handle for those.
Maybe I am being overly cautious, I'd like to hear other people thoughts on this. 👍

sebelga · 2023-04-17T13:51:06Z

packages/kbn-content-management-utils/types.ts

+  references?: Reference[];
+}
+
+export type GetResultSO<Item extends object> = GetResult<


Can we keep the same generic name and default? It makes it easier to follow IMO

export type GetResultSO<T extends unknown> = GetResult<T, ...>

mattkime · 2023-04-19T03:00:55Z

@vadimkibana @sebelga

I moved the option types outside of the main interface as to make them easier to modify. I've taken a guess at which options are most likely to be used. We can encourage devs to only use the option attrs they need.

I think it might make more sense if the arguments for a given method call were treated as a single item for upgrade / downgrade purposes. Treating them separately means being unable to alter one based on the content of the other. While its possible that we may never come across a case where its necessary, it would be better to have the option.

Its clear that CM needs to be able to do versioning transforms even if the SO api does some of the work. After all, other persistence layers might not take responsibility for this.

Next I'd like to put together a PR that focuses on code rather than types, providing necessary separation of concerns and BWCA logic.

Hopefully we're better aligned based on the recent changes in this PR and my statements above.

…fix'

…om:mattkime/kibana into content_management_api_so_type_abstraction

sebelga

LGTM! Having the possibility to make the options evolve independently from one SO type to another looks much better. thanks 👍

nreese

kibana-gis changes LGTM

kibana-ci · 2023-04-24T17:58:50Z

💚 Build Succeeded

Buildkite Build
Commit: dd9d980

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`	-	15	+15

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`@kbn/content-management-utils`	-	1	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`maps`	2.8MB	2.8MB	+7.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`maps`	45.0KB	45.1KB	+131.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/content-management-utils`	-	64	+64

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	17	19	+2
`securitySolution`	395	398	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	18	20	+2
`securitySolution`	475	478	+3
total			+5

History

💚 Build #121839 succeeded 48756d5
💔 Build #121782 failed 907ce41
💚 Build #121565 succeeded f32ecb4
💚 Build #120931 succeeded 530c23d
💚 Build #120249 succeeded ac841b5
💛 Build #120132 was flaky 055679a

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

cc @mattkime

…abstraction (#155342) ## Summary Abstract schema definitions for using Saved Objects with the content management api. For most schema types, this will reduce creation to only the attributes specific to a saved object. For Option types (create options, update options, search options) the saved object api is more complex and its likely that most SO types will only need to use a portion of it. In these cases we recommend using the provided schema definitions as a pattern for creating simpler schemas. Follow up to - #154985 - expresses types in schema form --------- Co-authored-by: kibanamachine <[email protected]>

abstract types

d601dbd

mattkime changed the title ~~abstract types~~ [content management / maps] Create abstract types for saved object usage with content management api Apr 15, 2023

kibanamachine and others added 5 commits April 15, 2023 04:39

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

9e06b50

…fix'

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

83b1ff5

fix types

86e3a85

Merge branch 'content_management_api_so_type_abstraction' of github.c…

8daec60

…om:mattkime/kibana into content_management_api_so_type_abstraction

remove comment

aea2173

mattkime marked this pull request as ready for review April 15, 2023 16:04

mattkime requested a review from a team as a code owner April 15, 2023 16:04

mattkime requested a review from sebelga April 15, 2023 16:05

mattkime added Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Feature:Maps labels Apr 15, 2023

mattkime added Feature:Content Management User generated content (saved objects) management release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2023

mattkime added 3 commits April 15, 2023 11:15

fix partial types

28548f7

add readme content

44ca8ee

Merge branch 'main' into content_management_api_so_type_abstraction

5b8eb84

mattkime mentioned this pull request Apr 16, 2023

[data views] Implement content management api #153733

Closed

mattkime added 3 commits April 16, 2023 23:10

simplify type export

3ab8198

simplify type export

e10b297

simplify type export

055679a

sebelga reviewed Apr 17, 2023

View reviewed changes

Merge branch 'main' into content_management_api_so_type_abstraction

ac841b5

sebelga reviewed Apr 17, 2023

View reviewed changes

mattkime and others added 3 commits April 18, 2023 18:30

Merge branch 'main' into content_management_api_so_type_abstraction

df3fd85

move Option types external to main interface

eee1744

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

530c23d

Update CODEOWNERS

b3da60a

mattkime and others added 5 commits April 20, 2023 08:18

Update kibana.jsonc

f32ecb4

more complete types for SO Options

64c4733

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

907ce41

…fix'

fix SO types for config

c3d7211

Merge branch 'content_management_api_so_type_abstraction' of github.c…

48756d5

…om:mattkime/kibana into content_management_api_so_type_abstraction

kertal assigned mattkime Apr 24, 2023

sebelga approved these changes Apr 24, 2023

View reviewed changes

nreese approved these changes Apr 24, 2023

View reviewed changes

Merge branch 'main' into content_management_api_so_type_abstraction

dd9d980

mattkime merged commit 273eec0 into elastic:main Apr 25, 2023

kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 25, 2023

mattkime mentioned this pull request Apr 25, 2023

[content management / maps] Content management / Saved object schema abstraction #155342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[content management / maps] Create abstract types for saved object usage with content management api #154985

[content management / maps] Create abstract types for saved object usage with content management api #154985

mattkime commented Apr 15, 2023 •

edited

Loading

elasticmachine commented Apr 15, 2023

sebelga Apr 17, 2023

mattkime Apr 17, 2023 •

edited

Loading

sebelga Apr 17, 2023

mattkime Apr 17, 2023 •

edited

Loading

vadimkibana Apr 17, 2023 •

edited

Loading

mattkime Apr 17, 2023

vadimkibana Apr 17, 2023

mattkime Apr 17, 2023

vadimkibana Apr 17, 2023

mattkime Apr 17, 2023 •

edited

Loading

sebelga left a comment

sebelga Apr 17, 2023

mattkime commented Apr 19, 2023 •

edited

Loading

sebelga left a comment

nreese left a comment

kibana-ci commented Apr 24, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[content management / maps] Create abstract types for saved object usage with content management api #154985

[content management / maps] Create abstract types for saved object usage with content management api #154985

Conversation

mattkime commented Apr 15, 2023 • edited Loading

Summary

elasticmachine commented Apr 15, 2023

sebelga Apr 17, 2023

Choose a reason for hiding this comment

mattkime Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

sebelga Apr 17, 2023

Choose a reason for hiding this comment

mattkime Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

vadimkibana Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

mattkime Apr 17, 2023

Choose a reason for hiding this comment

vadimkibana Apr 17, 2023

Choose a reason for hiding this comment

mattkime Apr 17, 2023

Choose a reason for hiding this comment

vadimkibana Apr 17, 2023

Choose a reason for hiding this comment

mattkime Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

sebelga left a comment

Choose a reason for hiding this comment

sebelga Apr 17, 2023

Choose a reason for hiding this comment

mattkime commented Apr 19, 2023 • edited Loading

sebelga left a comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 24, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Any counts in public APIs

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

mattkime commented Apr 15, 2023 •

edited

Loading

mattkime Apr 17, 2023 •

edited

Loading

mattkime Apr 17, 2023 •

edited

Loading

vadimkibana Apr 17, 2023 •

edited

Loading

mattkime Apr 17, 2023 •

edited

Loading

mattkime commented Apr 19, 2023 •

edited

Loading