[Content Management] Maps onboard by sebelga · Pull Request #153304 · elastic/kibana

sebelga · 2023-03-20T15:00:08Z

This PR migrates the "map" Saved object type to content management.

Included in the PR

removed any usage of the public saved object client
registered the map content type in the contentManagement registries (browser and server)
created a v1 CM services definition for BWC support
created a MapsStorage class on the server to communicate with the saved object client

Not included in this PR

The main focus of the PR was to remove the use of the savedObject public client in the browser. I didn't change other dependency on the @kbn/saved-objects-plugin package like the <SavedObjectSaveModalOrigin /> component.

Notes for reviewer

I've re-written the history of commits to better show how the migration has been done. I would recommend to review "by commit" (read the description of the commit wherever I've added one).

Documentation

In #154453 we have the documentation on how to onboard content into CM.

Fixes #153256

x-pack/plugins/maps/common/content_management/types.ts

elasticmachine · 2023-03-29T12:38:40Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

elasticmachine · 2023-03-29T12:39:50Z

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

sebelga · 2023-03-29T14:50:31Z

@elasticmachine merge upstream

sebelga · 2023-03-29T15:02:11Z

Sorry for the ping to the reviewers. I need to put this back in draft as we are going to make a change to the API before merging this one.

mattkime · 2023-03-30T02:44:58Z

x-pack/plugins/maps/common/content_management/v1/cm_services.ts

Aside from mapAttributesSchema, this looks like a generic saved objects schema

mattkime · 2023-03-30T03:18:41Z

x-pack/plugins/maps/server/content_management/maps_storage.ts

This has me a bit confused. As best I can tell its changing some object keys from snake case to camel case.

It is a function that guarantees that we explicitly return the fields for our MapItem instead of doing { attributes: ...savedObject.attributes }

We declare clear TS types for each CRUD (get, create, update, delete) and search methods inside a versioned folder ("v1"). Once that is done we can create the schema validation for the different object and expose a "v1" serviceDefinition. A service definition is an object where every objects that comes "in" (e.g. data to be saved or a query to execute) can have a schema validation and up & down transform handler for BWC. The same applies for the result object that comes "out".

The MapsStorage class is where the CRUD + search methods are declared. This is where calls to the Saved Object client apis are made

MapSavedObjectAttributes is going to be versioned and might evolve across different versions of the map content. For that we will expose under the "v1" folder. It is then re-exposed by the "latest.ts" file which is exposed by the barrel file.

…kibana into content-management/maps-onboard

…maps-onboard

ppisljar · 2023-04-11T10:54:03Z

x-pack/plugins/maps/common/content_management/v1/types.ts

+  uiStateJSON?: string;
+};
+
+export interface MapItem {


this seems like saved object type that could be abstracted as it will be the same for all our saved objects ? SavedObject ?

I think that the goal is to decouple from the SavedObject naming (exposing the persistence layer). There is indeed an opportunity to create a generic somewhere that SO content could extend but the idea in this PR is to decouple the specific content type from the storage layer (SO) and creating generic is out of scope.

Where this generic should live, I don't know.

I do think it is important to not want to abstract too early to save a few LOC (types anyway that will go away). If teams/owners of multiple SO types want to create a package for the generic they are free to do so 😊

x-pack/plugins/maps/server/content_management/maps_storage.ts

mattkime · 2023-04-12T03:11:59Z

x-pack/plugins/maps/server/content_management/maps_storage.ts

+
+    // Save data in DB
+    const soClient = await savedObjectClientFromRequest(ctx);
+    const partialSavedObject = await soClient.update<MapAttributes>(


Why does this work with partial saved objects? Is there something about the SO client that makes this necessary?

That's what you get from soClient.update : https://github.com/elastic/kibana/blob/main/packages/core/saved-objects/core-saved-objects-api-server/src/apis/update.ts#L44

mattkime · 2023-04-12T03:36:49Z

x-pack/plugins/maps/common/content_management/v1/types.ts

+  uiStateJSON?: string;
+};
+
+export interface MapItem {


This is really the SO data and meta data. The name item confused me a bit.

MapItem,MapSavedObject, Map... It could be named as the team understand better.

With that said, adding the SavedObject suffix is what we want to get away from as we don't want to expose databases specifics (e.g. we would not call this MapPostgreSQL)

mattkime · 2023-04-12T04:15:40Z

I put together a PR against this branch which simplifies types for any SO using content management. I've made a number of assumptions, hopefully I got more right than wrong - https://github.com/sebelga/kibana/pull/19/files

The abstraction - https://github.com/sebelga/kibana/pull/19/files#diff-1fcd1885f36b55f3d124588ce363d99feffa17cdc3a2f53ff7e611a3f353305a

The result - https://github.com/sebelga/kibana/blob/aabf1c9da2bfedb6f67e04c83cd5d7bfb2557fd6/x-pack/plugins/maps/common/content_management/v1/types.ts

sebelga · 2023-04-12T13:05:06Z

I put together a PR against this branch which simplifies types for any SO using content management.

@mattkime Those abstraction look good to me. They are out of scope for this PR, you probably want to create a package and expose them for other plugins to consume. And then open a PR to refactor the types here in map 👍

This reverts commit 0725ebb.

…maps-onboard

sebelga · 2023-04-12T15:34:43Z

@nreese I reverted the change to return the full object back to only returning the "title" and "description". Can you have another look? Cheers! 👍

kibana-ci · 2023-04-12T16:31:52Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: e01635a

Failed CI Steps

FTR Configs #40

Test Failures

[job] [logs] FTR Configs #40 / Lists plugin Lists API "after all" hook for "should return a 400 if endpoint exception entry has disallowed field"
[job] [logs] FTR Configs #40 / Lists plugin Lists API "before all" hook for "should return a 400 if an endpoint exception item with a list-based entry is provided"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`maps`	976	982	+6

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.7MB	2.7MB	+966.0B

Page load bundle

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

id	before	after	diff
`contentManagement`	7.5KB	7.5KB	+3.0B
`maps`	57.0KB	57.1KB	+86.0B
total			+89.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`maps`	28	27	-1

ESLint disabled line counts

id	before	after	diff
`maps`	46	47	+1
`securitySolution`	433	436	+3
total			+4

References to deprecated APIs

id	before	after	diff
`maps`	41	27	-14

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💚 Build #119399 succeeded 8e60c53
💚 Build #118909 succeeded ce0eeaf
💛 Build #118655 was flaky 3f60fd1
💔 Build #118623 failed 2627b08
💛 Build #118032 was flaky f5ea8ad
💚 Build #117972 succeeded e286a5f5b7a67cc80dca858dad7178bfcf499976

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

cc @sebelga

nreese

LGTM
code review, tested in chrome

thomasneirynck

hi @sebelga

this proposal looks really straightforward. Only some nits and small Qs thx!

src/plugins/visualizations/public/vis_types/vis_type_alias_registry.ts

thomasneirynck · 2023-04-12T19:43:16Z

x-pack/plugins/maps/public/kibana_services.ts

 export const getExecutionContextService = () => coreStart.executionContext;
 export const getTimeFilter = () => pluginsStart.data.query.timefilter.timefilter;
 export const getToasts = () => coreStart.notifications.toasts;
-export const getSavedObjectsClient = () => coreStart.savedObjects.client;


x-pack/plugins/maps/public/content_management/maps_client.ts

x-pack/plugins/maps/server/content_management/maps_storage.ts

sebelga · 2023-04-13T09:32:20Z

Thanks for the review @nreese and @thomasneirynck 👍 !

thomasneirynck · 2023-05-08T19:50:38Z

Part of #157043

## Summary Changes to the `down` method type from #153304, missed passing the generic input type. Also fixes types on schema utility functions from `@kbn/content-management-utils`. I created #231191 and a child issues for CO teams to track and fix. _Pulling this out of a larger PR, #229534, to ease CO review._ ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

## Summary Changes to the `down` method type from elastic#153304, missed passing the generic input type. Also fixes types on schema utility functions from `@kbn/content-management-utils`. I created elastic#231191 and a child issues for CO teams to track and fix. _Pulling this out of a larger PR, elastic#229534, to ease CO review._ ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit 7acaf61)

## Summary Changes to the `down` method type from elastic#153304, missed passing the generic input type. Also fixes types on schema utility functions from `@kbn/content-management-utils`. I created elastic#231191 and a child issues for CO teams to track and fix. _Pulling this out of a larger PR, elastic#229534, to ease CO review._ ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

sebelga force-pushed the content-management/maps-onboard branch from ebde438 to 6de3442 Compare March 22, 2023 17:07

mattkime reviewed Mar 27, 2023

View reviewed changes

x-pack/plugins/maps/common/content_management/types.ts Outdated Show resolved Hide resolved

sebelga force-pushed the content-management/maps-onboard branch 2 times, most recently from 10e5f14 to 3bffab5 Compare March 29, 2023 12:18

sebelga marked this pull request as ready for review March 29, 2023 12:36

sebelga requested review from a team as code owners March 29, 2023 12:36

sebelga added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// Feature:Maps labels Mar 29, 2023

sebelga marked this pull request as draft March 29, 2023 15:00

mattkime reviewed Mar 30, 2023

View reviewed changes

sebelga force-pushed the content-management/maps-onboard branch from 67eb74c to abd68a4 Compare April 5, 2023 10:18

sebelga added 7 commits April 5, 2023 15:50

Fix object versioning and contentManagement types & schemas

6c1babf

Add plugin dependency to contentManagement

0dac967

Create MapsStorage class

131345d

The MapsStorage class is where the CRUD + search methods are declared. This is where calls to the Saved Object client apis are made

[server] Register the map content in CM registry

babe23b

[public] Register the map content in CM registry

18e6f23

sebelga added 3 commits April 11, 2023 10:49

Limit search to one item when checking if there are maps

9d8a30a

Merge branch 'content-management/maps-onboard' of github.com:sebelga/…

1c69603

…kibana into content-management/maps-onboard

Merge remote-tracking branch 'upstream/main' into content-management/…

ce0eeaf

…maps-onboard

ppisljar reviewed Apr 11, 2023

View reviewed changes

x-pack/plugins/maps/server/content_management/maps_storage.ts Show resolved Hide resolved

mattkime reviewed Apr 12, 2023

View reviewed changes

sebelga and others added 3 commits April 12, 2023 14:06

Merge branch 'main' into content-management/maps-onboard

8e60c53

Revert "Return full map object from search"

5adbb64

This reverts commit 0725ebb.

Merge remote-tracking branch 'upstream/main' into content-management/…

e01635a

…maps-onboard

sebelga requested a review from nreese April 12, 2023 15:33

nreese approved these changes Apr 12, 2023

View reviewed changes

thomasneirynck reviewed Apr 12, 2023

View reviewed changes

sebelga merged commit 42a893d into elastic:main Apr 13, 2023

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

sebelga deleted the content-management/maps-onboard branch April 13, 2023 10:46

thomasneirynck mentioned this pull request May 8, 2023

[META][AppEx - AnalyticsXP] Remove dependencies on the saved object client in the browser #157043

Closed

12 tasks

nickofthyme added a commit to nickofthyme/kibana that referenced this pull request Aug 5, 2025

fix: content management types from elastic#153304

fdfcad1

nickofthyme mentioned this pull request Aug 5, 2025

[Content Management] fix down method types #230656

Merged

1 task

Conversation

sebelga commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Included in the PR

Not included in this PR

Notes for reviewer

Documentation

Uh oh!

Uh oh!

elasticmachine commented Mar 29, 2023

Uh oh!

elasticmachine commented Mar 29, 2023

Uh oh!

sebelga commented Mar 29, 2023

Uh oh!

sebelga commented Mar 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Apr 12, 2023

Uh oh!

sebelga commented Apr 12, 2023

Uh oh!

sebelga commented Apr 12, 2023

Uh oh!

kibana-ci commented Apr 12, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled in files

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebelga commented Apr 13, 2023

Uh oh!

thomasneirynck commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

sebelga commented Mar 20, 2023 •

edited

Loading