Make SavedObjectFinder backward compatible
#162904
Conversation
Dosant
force-pushed
the
d/2023-07-28-saved-object-finder-msearch
branch
from
5b7e630 to
8f08e60
Compare
…ved-object-finder-msearch # Conflicts: # src/plugins/discover/tsconfig.json
## Summary These examples are outdated and don't show recent embeddable best practices. They also use client-side saved object client and block making `SavedObjectFinder` backward compatible #162904 as the `foobar` saved objects need to be added to content management. We decided that it is better to clean them up, as fixing them is not a small effort and it is not worth it on this point as a large embeddable refactor is coming.
|
@elasticmachine merge upstream |
Dosant
changed the title
SavedObjectFinder backward compatible
Dosant
added
release_note:skip
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
There was a problem hiding this comment.
Did a smoke test in Dashboard app and the everything worked as expected. I left a question about using TS interface but otherwise everything looks great!
Nice job @Dosant !
Discussed offline about not supporting the includeFields option at first, happy with the trade off that we may be sending more data over the wire in these searches -- seems to mitigated a bit by CM's processing of allowed fields already?
| })) as FindResponseHTTP<FinderAttributes>; | ||
| const types = visibleTypes ?? Object.keys(metaDataMap); | ||
|
|
||
| const response = await contentClient.mSearch<SavedObjectCommon<FinderAttributes>>({ |
There was a problem hiding this comment.
I thought the content management would not return the actual SO interface, perhaps we should rather use SOWithMetadata?
There was a problem hiding this comment.
(I see SavedObjectCommon is SavedObject)
There was a problem hiding this comment.
My feeling is indeed that we should have our own interface here with the minimum set of fields that are consumed by apps (for ex is coreMigrationVersion or typeMigrationVersion or namespaces actually needed/consumed in the UI?)
There was a problem hiding this comment.
@jloleysens, I changed from SavedObject to SOWithMetadata, still far from ideal, but better as we more decoupled form saved objects. thanks
My feeling is indeed that we should have our own interface here with the minimum set of fields that are consumed by apps
Agree, yes, if/when we move forward with cm this could be the direction.
But for now msearch is just a different way to query saved objects with intermediate bwc layer. If to do this typing technically correct right now, then seems we can only type this as generic object for now :(
Or we could do an intersection of all supported types and their interfaces
| @@ -45,8 +39,8 @@ export interface SavedObjectMetaData<T = unknown> { | |||
| getTooltipForSavedObject?(savedObject: SavedObjectCommon<T>): string; | |||
| showSavedObject?(savedObject: SavedObjectCommon<T>): boolean; | |||
| getSavedObjectSubType?(savedObject: SavedObjectCommon<T>): string; | |||
| /** @deprecated doesn't do anything, the full object is returned **/ | |||
There was a problem hiding this comment.
Why don't we remove it then?
There was a problem hiding this comment.
I'll try to spend some time and see if it will be easy and worth it adding this back #163043 , didn't want to delete, as there is a chance we can bring this functionality back without changing the component outside outside
| })) as FindResponseHTTP<FinderAttributes>; | ||
| const types = visibleTypes ?? Object.keys(metaDataMap); | ||
|
|
||
| const response = await contentClient.mSearch<SavedObjectCommon<FinderAttributes>>({ |
There was a problem hiding this comment.
My feeling is indeed that we should have our own interface here with the minimum set of fields that are consumed by apps (for ex is coreMigrationVersion or typeMigrationVersion or namespaces actually needed/consumed in the UI?)
| @@ -135,6 +135,7 @@ export interface SOContentStorageConstrutorParams<Types extends CMCrudTypes> { | |||
| updateArgsToSoUpdateOptions?: UpdateArgsToSoUpdateOptions<Types>; | |||
| searchArgsToSOFindOptions?: SearchArgsToSOFindOptions<Types>; | |||
| enableMSearch?: boolean; | |||
| mSearchAdditionalSearchFields?: string[]; | |||
There was a problem hiding this comment.
I am still a bit puzzeled why we allow any field name to be passed here instead of having a set of fixed fields to be added to the search (e.g. 'name' | 'description' )
There was a problem hiding this comment.
title and description are common, but only DataView has name and wants it searchable. This mSearchAdditionalSearchFields is for DataView to configure the name that belongs to them without hardcoding it
There was a problem hiding this comment.
So why not set this type as mSearchAdditionalSearchFields?: Array<'name'> ?
…:Dosant/kibana into d/2023-07-28-saved-object-finder-msearch
…ved-object-finder-msearch
|
@elasticmachine merge upstream |
…ved-object-finder-msearch
…:Dosant/kibana into d/2023-07-28-saved-object-finder-msearch
…ved-object-finder-msearch
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
References to deprecated APIs
History
To update your PR or re-run it, just comment with: |
kibanamachine
added
v8.10.0
backport:skip
## Summary close elastic#161545 close elastic#153257 This PR makes `SavedObjectFinder` component backward compatible. It is achieved by going through content- management layer, more technical details [here](https://docs.google.com/document/d/1ssYmqSEUPrsuCR4iz8DohkEWekoYrm2yL4QR_fVxXLg/edit) ### Testing `SavedObjectFinder` is this component that allows to pick a saved object (supports: `search` `index-pattern` `map` `visualization` `lens` `event-annotation-group`:  It is used in the following places: - Dashboard - Add panel - Replace panel - Discover - Open Search - Visualization - Select search as a source for new viz - Graph - select source - Cases - markdown editor add lens - ML (3 places) - Canvas - select embeddable panel - Transform - Lens > select event annotation ### Risks / Follow up The `SavedObjectFinder` should stay mostly the same, the only notable functional change is that now `SavedObjectFinder` doesn't support `includeFields` which allowed partial saved object returns, this was done to make the call backward-compatible without making the system even more complicated as otherwise we'll need a way to abstract `includeFields` from so attributes and allow to run migrations on it before making a search. follow up issue to bring it back elastic#163043 The risk with that is that some client that have a lot of large objects might run into performance issues when using `SavedObjectFinder`. This can be mitigated by changing listing limit in advanced setting from default 1000 to something lower
Summary
close #161545
close #153257
This PR makes
SavedObjectFindercomponent backward compatible. It is achieved by going through content- management layer, more technical details hereTesting
SavedObjectFinderis this component that allows to pick a saved object (supports:searchindex-patternmapvisualizationlensevent-annotation-group:It is used in the following places:
Risks / Follow up
The
SavedObjectFindershould stay mostly the same, the only notable functional change is that nowSavedObjectFinderdoesn't supportincludeFieldswhich allowed partial saved object returns, this was done to make the call backward-compatible without making the system even more complicated as otherwise we'll need a way to abstractincludeFieldsfrom so attributes and allow to run migrations on it before making a search. follow up issue to bring it back #163043The risk with that is that some client that have a lot of large objects might run into performance issues when using
SavedObjectFinder. This can be mitigated by changing listing limit in advanced setting from default 1000 to something lower