From 86d6ce48650bae0f0825c357943f082fdd9a2e12 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 23 Apr 2020 09:01:04 +0200 Subject: [PATCH 01/21] initial file import --- rfcs/text/0011_global_search.md | 284 ++++++++++++++++++++++++++++++++ 1 file changed, 284 insertions(+) create mode 100644 rfcs/text/0011_global_search.md diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md new file mode 100644 index 0000000000000..c50f9262b25b2 --- /dev/null +++ b/rfcs/text/0011_global_search.md @@ -0,0 +1,284 @@ +- Start Date: 2020-04-19 +- RFC PR: (leave this empty) +- Kibana Issue: [#61657](https://github.com/elastic/kibana/issues/61657) + +# Summary + +A core API exposed on both public and server sides, allowing to search for arbitrary objects and +register result providers. + +# Basic example + +- registering a result provider + +```ts +coreSetup.globalSearch.registerResultProvider({ + id: 'my_provider', + search: (term, options, context) => { + const resultPromise = myService.search(term, context.core.savedObjects.client); + return from(resultPromise); + }, +}); +``` + +- using the `find` API + +```ts +coreStart.globalSearch.find('some term', { type: ['dashboard', 'application'] }); +``` + +# Motivation + +The main goal of this feature is to power the global search bar [#57576](https://github.com/elastic/kibana/issues/57576). + +While this first point remains the priority, this API should also provide a solution for alternative needs regarding +searching for arbitrary objects from a Kibana instance. + +# Detailed design + +## Functional behavior + +### Summary + +- `core` exposes an setup API to be able to register result providers (`GlobalSearchResultProvider`). These providers + can be registered from either public or server side, even if the interface for each sides is not exactly the same. +- `core` exposes a start API to be able to search for objects. This API is available from both public and server sides. +- when a search is triggered, the `GS` service will query all registered providers and then returns an aggregated result + +### result provider registration + +Due to the fact that some kind of results (i.e `application`) can currently only be retrieved from the public side of Kibana, +the `registerResultProvider` API will be available both from the public and the server counterpart of the `GlobalSearchService`. +However, as results from providers registered from the client-side will not be available from the server's `find` API, +registration of result providers from the client will be discouraged with proper documentation stating that it should +only be used when it is not technically possible to expose it from the server side instead. + +### publicUrl + +To be usable from external services (such as ChatOps), results returned from the GlobalSearch public HTTP API should all contains absolute urls, as +relative paths would not be usable from outside of the instance. + +However, given the fact that some Kibana deployments can have complex architecture, there is currently +no reliable way to know for sure what the public address used to access kibana is. + +A new `server.publicAddress` property will be added to the kibana configuration. + +When not manually defined in the configuration, this property will be constructed using the known `server` configuration values: + +```ts +const publicAddress = removeTrailingSlash( + `${getServerInfo().protocol}://${httpConfig.host}:${httpConfig.port}/${httpConfig.basePath}` +); +``` + +// TODO: example + +### searching from the server side + +When calling `GlobalSearchServiceStart.search` from the server-side service: + +- the service will call `find` on each server-side registered result provider and collect the resulting result observables + +- then, the service will combine every result observable and trigger the next step on every emission until either + - the predefined timeout duration is reached + - Every providers result observable are completed + +- on every emission of the combined observable, the results will be aggregated and sorted following +the logic defined in the [results aggregation](#results-aggregation) section + +A very naive implementation of this behavior would be: + +```ts +search( + term: string, + options: GlobalSearchOptions, + request: KibanaRequest +): Observable { + const fromProviders$ = this.providers.map(p => + p.find(term, options, contextFromRequest(request)) + ); + + return combineLatest([...fromProviders$]).pipe( + takeUntil(timeout$) + map(resultLists => { + return mergeAndOrder(resultLists); + }) + ); +} +``` + +### searching from the client side + +When calling `GlobalSearchServiceStart.search` from the public-side service: + +- the service will call: + - the server-side API via an http call to fetch results from the server-side result providers + - `find` on each client-side registered result provider and collect the resulting observables + +- then, the service will combine every result observable and trigger the next step on every emission until either + - the predefined timeout duration is reached + - Every providers result observable are completed + +- on every emission of the combined observable, the results will be aggregated and sorted following +the logic defined in the [results aggregation](#results-aggregation) section + +A very naive implementation of this behavior would be: + +``` +search( + term: string, + options: GlobalSearchOptions, +): Observable { + const fromProviders$ = this.providers.map(p => + p.find(term, options) + ); + const fromServer$ = of(this.fetchServerResults(term, options)) + + return combineLatest([...fromProviders$, fromServer$]).pipe( + takeUntil(timeout$) + map(resultLists => { + return mergeAndOrder(resultLists); + }) + ); +} +``` + +Note: due to the complexity of the process, the initial implementation will not be streaming results from the server, +meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' http call to the GS endpoint). +The observable-based API architecture is ready for this however, and the enhancement could be added at a later time. + +### results aggregation + +On every emission of an underlying provider, the service will aggregate and sort the results following this logic before emitting them: + +- results from all providers will be merged in a single list. +- results will be sorted by ascending `type` ordinal value. +- results of a same `type` will then be sorted by descending `score` value. + +This is an equivalent of the following lodash call: + +```ts +const sorted = _.sortBy(unsorted, [r => SEARCH_TYPE[r.type], 'score'], ['asc', 'desc']); +``` + +For example, given this list of unsorted results: + +```ts +const unsorted = [ + { id: 'viz-1', type: 'visualization', score: 100 }, + { id: 'dash-2', type: 'dashboard', score: 25 }, + { id: 'app-1', type: 'application', score: 50 }, + { id: 'dash-1', type: 'dashboard', score: 50 }, + { id: 'app-1', type: 'application', score: 100 }, +]; +``` + +the resulting sorted results would be: + +```ts +const sorted = [ + { id: 'app-1', type: 'application', score: 100 }, + { id: 'app-1', type: 'application', score: 50 }, + { id: 'dash-1', type: 'dashboard', score: 50 }, + { id: 'dash-2', type: 'dashboard', score: 25 }, + { id: 'viz-1', type: 'visualization', score: 100 }, +]; +``` + +#### Note on score value + +Due to the fact that the results will be coming from various providers, from distinct ES queries or even not from ES, +using a centralized scoring mechanism is not possible. + +the `GlobalSearchResult` contains a `score` field, with an expected value from 1 (lowest) to 100 (highest). +How this field is populated from each individual provider is considered an implementation detail. + +### Search cancellation + +Once triggered, a given call to `GlobalSearchServiceStart.find` (and underlying search provider's `find` method) +cannot be canceled, neither from the public nor server API. + +## API Design + +### Types + +```ts +type GlobalSearchResultType = 'dashboard' | 'visualization' | 'search' | 'application'; + +/** + * Representation + */ +interface GlobalSearchResult { + /* the title of the result */ + title: string; + /* the type of result / object */ + type: GlobalSearchResultType; + /* an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ + icon?: string; + /* The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ + url: string; + /* the score of the result, from 1 to 100. used for ordering individual results of a same type */ + score: number; +} + +/** + * Response returned from a {@link GlobalSearchResultProvider | result provider} + */ +type GlobalSearchResultProviderResponse = + | { + success: true; + /** list of matching results */ + results: GlobalSearchResult[]; + } + | { + success: false; + /** error message of the failure cause */ + error?: string; + }; + +/** + * Options provided to {@link GlobalSearchResultProvider | result providers} + * Currently empty and only present for keeping the API future-proof. + */ +interface GlobalSearchOptions {} + +/** + * GlobalSearch result provider, to be registered using the {@link GlobalSearchSetup | global search API} + */ +type GlobalSearchResultProvider = { + search(term: string, options: GlobalSearchOptions): Promise; +}; +``` + +### Plugin API + +# Drawbacks + +- The fact that some result providers must be on the client-side... + +# Alternatives + +- could only allow to register result providers from the server-side for the public API +- could have a struct instead of a url for internal results. +- use plain string instead of enum for result `type` +- should we pass a signal or a `canceled$` observable to the providers to allow search cancellation? + +# Adoption strategy + +The `globalSearch` service is a new feature provided by the `core` API. Also, the base providers +used to search for saved objects and applications will be implemented by the platform team, meaning +that by default, plugin developers won't have to do anything. + +Plugins that wish to expose more detail about their availability will easily be able to do so, +including providing detailed information such as links to documentation to resolve the problem. + +# How we teach this + +This follows the same patterns we have used for other Core APIs: Observables subscriptions, etc. + +This should be taught using the same channels we've leveraged for other Kibana Platform APIs, API documentation mostly. + +# Unresolved questions + +Optional, but suggested for first drafts. What parts of the design are still +TBD? From 2aa6e80a2aa12aea1e38b893e5dfc8736a2d17dc Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 23 Apr 2020 10:05:58 +0200 Subject: [PATCH 02/21] update types --- rfcs/text/0011_global_search.md | 265 ++++++++++++++++++++++++-------- 1 file changed, 205 insertions(+), 60 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index c50f9262b25b2..96177ecc1ad51 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -9,22 +9,25 @@ register result providers. # Basic example -- registering a result provider +- registering a result provider: ```ts coreSetup.globalSearch.registerResultProvider({ id: 'my_provider', - search: (term, options, context) => { + find: (term, options, context) => { const resultPromise = myService.search(term, context.core.savedObjects.client); return from(resultPromise); }, }); ``` -- using the `find` API +- using the `find` API from the client-side: ```ts -coreStart.globalSearch.find('some term', { type: ['dashboard', 'application'] }); +coreStart.globalSearch.find('some term').subscribe(({ results, complete }) => { + showAsyncSearchIndicator(!complete); + updateResults(results); +}); ``` # Motivation @@ -36,6 +39,192 @@ searching for arbitrary objects from a Kibana instance. # Detailed design +## API Design + +### Types + +#### common + +```ts +/** + * Static list of all the possible types of results. + * Ordinal value matter here, as it will used to sort results of different types. + */ +enum SEARCH_TYPE { + // non-exhaustive + applications = 10, + dashboard = 20, + visualization = 30, + search = 40, +} + +/** @public */ +type GlobalSearchResultType = keyof typeof SEARCH_TYPE; + +/** + * Options provided to {@link GlobalSearchResultProvider | result providers} `find` method + * Currently empty and only present for keeping the API future-proof. + */ +interface GlobalSearchOptions {} + +/** + * Representation of a result returned by a {@linlkGlobalSearchResultProvider | result provider} + */ +interface GlobalSearchResult { + /* an arbitrary id that should be unique for an individual provider's results */ + id: string; + /* the title/label of the result */ + title: string; + /* the type of result / object */ + type: GlobalSearchResultType; + /* an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ + icon?: string; + /* The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ + url: string; + /* the score of the result, used for ordering individual results of a same type */ + score: number; +} +``` + +#### server + +```ts +/** + * Context for the server-side result providers. + */ +export interface GlobalSearchProviderContext { + core: { + savedObjects: { + client: SavedObjectsClientContract; + typeRegistry: ISavedObjectTypeRegistry; + }; + elasticsearch: { + legacy: { + client: IScopedClusterClient; + }; + }; + uiSettings: { + client: IUiSettingsClient; + }; + }; +} + +/** + * GlobalSearch result provider, to be registered using the {@link GlobalSearchSetup | global search API} + */ +type GlobalSearchResultProvider = { + id: string; + find( + term: string, + options: GlobalSearchOptions, + context: GlobalSearchProviderContext + ): Observable; +}; +``` + +Notes: +- even if initial implementation will provide a static version of `GlobalSearchProviderContext`, +it could be possible to allow plugins to register contexts as it's currently done for `RequestHandlerContext`. +This will not be done until the need arises. + +#### public + +```ts +/** + * GlobalSearch result provider, to be registered using the {@link GlobalSearchSetup | global search API} + */ +type GlobalSearchResultProvider = { + id: string; + find( + term: string, + options: GlobalSearchOptions, + ): Observable; +}; + +/** + * Enhanced {@link GlobalSearchResult | result type} for the client-side, + * to allow navigating to a given result. + */ +interface NavigableGlobalSearchResult extends GlobalSearchResult { + navigateTo: () => void; +} +``` + +Notes: +- The client-side version of `GlobalSearchResultProvider` is slightly difference than the +server one, as there is no `context` parameter passed to `find`. +- `NavigableGlobalSearchResult` is here to enhanced results with a `navigateTo` method to +allow `core` to handle the navigation mechanism for the results, which is non-trivial. +See the [Redirecting to a result](#redirecting-to-a-result) section for more info. + +### Plugin API + +#### common types + +```ts +/** + * Response returned from a {@link GlobalSearchResultProvider | result provider} + */ +type GlobalSearchResponse = { + /** + * Current results fetched from the providers. + */ + results: ResultType[]; + /** + * Is the search complete. Will only be true during the last emission of the `GlobalSearchServiceStart.search` observable. + * A search is considered complete either when all underlying providers completed their observable, or when the timeout is reached. + */ + complete: boolean; +}; +``` + +#### server API + +```ts +/** @public */ +interface GlobalSearchServiceSetup { + registerResultProvider(provider: GlobalSearchResultProvider); +} + +/** @public */ +interface GlobalSearchServiceStart { + find( + term: string, + options: GlobalSearchOptions, + request: KibanaRequest + ): Observable; +} +``` + +#### public API + +```ts +/** @public */ +interface GlobalSearchServiceSetup { + registerResultProvider(provider: GlobalSearchResultProvider); +} + +/** @public */ +interface GlobalSearchServiceStart { + find( + term: string, + options: GlobalSearchOptions, + ): Observable>; +} +``` + +Notes: the public API looks quite similar to the server one. The differences are: +- the `registerResultProvider` setup API got the same signature, however the `GlobalSearchResultProvider` types is different +than the server one +- the `find` start API signature got a `KibanaRequest` for `server`, where this parameter is not present for `public`. + +#### http API + +// TODO + +Note: this is the API that will consumed by the client-side `GlobalSearchService` to retrieve server-side results. It should +not be considered a public and supported API until the need to do so arises. + ## Functional behavior ### Summary @@ -73,6 +262,10 @@ const publicAddress = removeTrailingSlash( // TODO: example +### Redirecting to a result + +// TODO + ### searching from the server side When calling `GlobalSearchServiceStart.search` from the server-side service: @@ -198,60 +391,6 @@ How this field is populated from each individual provider is considered an imple Once triggered, a given call to `GlobalSearchServiceStart.find` (and underlying search provider's `find` method) cannot be canceled, neither from the public nor server API. -## API Design - -### Types - -```ts -type GlobalSearchResultType = 'dashboard' | 'visualization' | 'search' | 'application'; - -/** - * Representation - */ -interface GlobalSearchResult { - /* the title of the result */ - title: string; - /* the type of result / object */ - type: GlobalSearchResultType; - /* an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ - icon?: string; - /* The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ - url: string; - /* the score of the result, from 1 to 100. used for ordering individual results of a same type */ - score: number; -} - -/** - * Response returned from a {@link GlobalSearchResultProvider | result provider} - */ -type GlobalSearchResultProviderResponse = - | { - success: true; - /** list of matching results */ - results: GlobalSearchResult[]; - } - | { - success: false; - /** error message of the failure cause */ - error?: string; - }; - -/** - * Options provided to {@link GlobalSearchResultProvider | result providers} - * Currently empty and only present for keeping the API future-proof. - */ -interface GlobalSearchOptions {} - -/** - * GlobalSearch result provider, to be registered using the {@link GlobalSearchSetup | global search API} - */ -type GlobalSearchResultProvider = { - search(term: string, options: GlobalSearchOptions): Promise; -}; -``` - -### Plugin API - # Drawbacks - The fact that some result providers must be on the client-side... @@ -280,5 +419,11 @@ This should be taught using the same channels we've leveraged for other Kibana P # Unresolved questions -Optional, but suggested for first drafts. What parts of the design are still -TBD? +## terminology / naming + +Are the current types, services and api names acceptable: + - `GlobalSearch` ts prefix + - `core.globalSearch` / `GlobalSearchService` + - `GlobalSearchResultProvider` + + \ No newline at end of file From 6bdc15f8d03071d930dc4a50bdb5dd67eb1cb6e9 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 23 Apr 2020 10:13:46 +0200 Subject: [PATCH 03/21] update RFC PR number --- rfcs/text/0011_global_search.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 96177ecc1ad51..972811d58d3a1 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -1,5 +1,5 @@ - Start Date: 2020-04-19 -- RFC PR: (leave this empty) +- RFC PR: [#64284](https://github.com/elastic/kibana/pull/64284) - Kibana Issue: [#61657](https://github.com/elastic/kibana/issues/61657) # Summary @@ -425,5 +425,6 @@ Are the current types, services and api names acceptable: - `GlobalSearch` ts prefix - `core.globalSearch` / `GlobalSearchService` - `GlobalSearchResultProvider` + - `core.globalSearch.find` \ No newline at end of file From b3a31fa2732f4e4bea1b3067a1415eae26745918 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 23 Apr 2020 16:39:15 +0200 Subject: [PATCH 04/21] first draft complete --- rfcs/text/0011_global_search.md | 145 ++++++++++++++++++++++++++++---- 1 file changed, 128 insertions(+), 17 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 972811d58d3a1..f9f8b50ff23ec 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -4,7 +4,7 @@ # Summary -A core API exposed on both public and server sides, allowing to search for arbitrary objects and +A core API exposed on both public and server sides, allowing to search for various objects and register result providers. # Basic example @@ -71,18 +71,20 @@ interface GlobalSearchOptions {} * Representation of a result returned by a {@linlkGlobalSearchResultProvider | result provider} */ interface GlobalSearchResult { - /* an arbitrary id that should be unique for an individual provider's results */ + /** an id that should be unique for an individual provider's results */ id: string; - /* the title/label of the result */ + /** the title/label of the result */ title: string; - /* the type of result / object */ + /** the type of result / object */ type: GlobalSearchResultType; - /* an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ + /** an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ icon?: string; - /* The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ + /** The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ url: string; - /* the score of the result, used for ordering individual results of a same type */ + /** the score of the result, used for ordering individual results of a same type */ score: number; + /** an optional record of metadata for this result */ + meta?: Record; } ``` @@ -123,9 +125,9 @@ type GlobalSearchResultProvider = { ``` Notes: -- even if initial implementation will provide a static version of `GlobalSearchProviderContext`, -it could be possible to allow plugins to register contexts as it's currently done for `RequestHandlerContext`. -This will not be done until the need arises. +- initial implementation will only provide a static / non extensible version of `GlobalSearchProviderContext`. +It could be possible to allow plugins to register their own contexts as it's done for `RequestHandlerContext`, +but this will not be done until the need arises. #### public @@ -255,16 +257,49 @@ A new `server.publicAddress` property will be added to the kibana configuration. When not manually defined in the configuration, this property will be constructed using the known `server` configuration values: ```ts -const publicAddress = removeTrailingSlash( +const defaultPublicAddress = removeTrailingSlash( `${getServerInfo().protocol}://${httpConfig.host}:${httpConfig.port}/${httpConfig.basePath}` ); + +const getPublicAddress = () => httpConfig.publicAddress ?? defaultPublicAddress; +``` + +then a new `getAbsoluteUrl` api would be added to the core `http` service. + +```ts +const getAbsoluteUrl = (path: string, request: KibanaRequest) => { + const publicUrl = getPublicAddress(); + const absoluteUrl = joinRemovingDuplicateAndTrailingSlash( + publicUrl, + serverContract.basePath.get(request), + path + ); +} ``` -// TODO: example +Search results will then be consolidated before being returned to convert relative urls to absolute ones. + +```ts +const consolidateResult(result: GlobalSearchResult, request: KibanaRequest) { + if(isUriPath(result.url)) { + result.url = http.getAbsoluteUrl(result.url, request) + } +} +``` ### Redirecting to a result -// TODO +from the client-side, `NavigableGlobalSearchResult.navigateTo` would follow this logic: + +If all 3 of these criteria are true for `result.url`: + +- The domain of the URL (if present) matches the domain of the `publicUrl` +- The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) +- The pathname segment after the basePath matches an application route (eg. /app//) + +Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment + +Otherwise, do a full page navigation (window.location.assign()) ### searching from the server side @@ -393,14 +428,90 @@ cannot be canceled, neither from the public nor server API. # Drawbacks -- The fact that some result providers must be on the client-side... +- The fact that some result providers must be on the client-side complexify the API. # Alternatives - could only allow to register result providers from the server-side for the public API -- could have a struct instead of a url for internal results. -- use plain string instead of enum for result `type` -- should we pass a signal or a `canceled$` observable to the providers to allow search cancellation? + - would ensure + + +## `GlobalSearchResult.url` could be a struct instead of a url for internal results. + +One of the initial proposal was to have + +```ts +url: { absUrl: string } | { application: string; path?: string }; +``` + +That was making it way easier to redirect to an internal result from the UI, as we could directly call +`application.navigateTo(application, { path })`. + +However, that didn't answer for need for the (future) need to be able to search for and redirect to object in +different spaces. We could have then changed to + +```ts +url: { absUrl: string } | { application: string; path?: string, space?: string }; +``` + +But this had some issues: +- `space` is an xpack plugin, adding this property in the oss implementation is problematic +- `space` API is not callable from core or oss, meaning that we would have to 'forge' the url to this space anyway +- this is really not generic. If another plugin was to alter the basepath, we would have needed to add it another property + +So even if the 'parsable absolute url' approach seems fragile, it's probably still better than this alternative. + +## We could use plain string instead of an enum for `GlobalSearchResult.type` + +The current static enum used for type + +```ts +enum SEARCH_TYPE { + // non-exhaustive + applications = 10, + dashboard = 20, + visualization = 30, + search = 40, +} + +/** @public */ +type GlobalSearchResultType = keyof typeof SEARCH_TYPE; + +interface GlobalSearchResult { + // [...] + type: GlobalSearchResultType; +} +``` + +has some limitations: +- it forces the enum to be modified every time a new type is added +- 3rd party plugins cannot introduce new types + +We could change the API to accept plain strings for `GlobalSearchResult.type`, however, atm this enum approach +is needed as the ordinal values of the entries is used in results sorting. Changing to plain strings forces to find +another sorting approach. + +## triggered searches could be cancelable + +In current specifications, once a search has been triggered, it's not possible to cancel it (see [Search cancellation](#search-cancellation)) + +Main drawback of this decision is that if a `search` consumer + +We could add an optional signal or observable in `GlobalSearchOptions`. That way a consumer knowing that a `find` call may be aborted could use +this option. + +However result providers from plugins would still have to manually handles this signal to cancel any http call or +other asynchronous task that could be pending. + +Note that as this can be implemented with an additional option, this can be done at a later time. + +## The GlobalSearch service could be provided as a plugin instead of a core service + +- could be provided as a plugin instead of a core service + - However as GS is going to be used in the header, what still would mean a bridge of some kind to be able to register it + to core. + - As the platform team is going to provide the base result providers for search results and application, that would mean + create yet another plugin for these providers # Adoption strategy From 697159799d66fd2c671c0942ca934ecbe2404d7b Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 24 Apr 2020 10:47:13 +0200 Subject: [PATCH 05/21] draft v1.1.0 --- rfcs/text/0011_global_search.md | 373 ++++++++++++++++++++++---------- 1 file changed, 258 insertions(+), 115 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index f9f8b50ff23ec..02744fe3e90c8 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -37,18 +37,20 @@ The main goal of this feature is to power the global search bar [#57576](https:/ While this first point remains the priority, this API should also provide a solution for alternative needs regarding searching for arbitrary objects from a Kibana instance. +// TODO: develop probably + # Detailed design ## API Design -### Types +### Result provider API -#### common +#### common types ```ts /** - * Static list of all the possible types of results. - * Ordinal value matter here, as it will used to sort results of different types. + * Static list of all the possible result types. + * Ordinal value matters here, as it will be used to sort results of different types. */ enum SEARCH_TYPE { // non-exhaustive @@ -62,8 +64,8 @@ enum SEARCH_TYPE { type GlobalSearchResultType = keyof typeof SEARCH_TYPE; /** - * Options provided to {@link GlobalSearchResultProvider | result providers} `find` method - * Currently empty and only present for keeping the API future-proof. + * Options provided to {@link GlobalSearchResultProvider | result providers} `find` method. + * Currently empty and only present to keep the API future-proof. */ interface GlobalSearchOptions {} @@ -75,13 +77,13 @@ interface GlobalSearchResult { id: string; /** the title/label of the result */ title: string; - /** the type of result / object */ + /** the type of result */ type: GlobalSearchResultType; /** an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ icon?: string; - /** The url to navigate to this result. This can be either an absolute url, or a path relative to the server's publishUrl */ + /** The url to navigate to this result. This can be either an absolute url, or a path relative to the performing request's basePath */ url: string; - /** the score of the result, used for ordering individual results of a same type */ + /** the score of the result, from 1 (lowest) to 100 (highest) */ score: number; /** an optional record of metadata for this result */ meta?: Record; @@ -92,9 +94,10 @@ interface GlobalSearchResult { ```ts /** - * Context for the server-side result providers. + * Context passed to server-side {@GlobalSearchResultProvider | result provider}'s `find` method. */ export interface GlobalSearchProviderContext { + request: KibanaRequest; core: { savedObjects: { client: SavedObjectsClientContract; @@ -125,9 +128,11 @@ type GlobalSearchResultProvider = { ``` Notes: -- initial implementation will only provide a static / non extensible version of `GlobalSearchProviderContext`. -It could be possible to allow plugins to register their own contexts as it's done for `RequestHandlerContext`, +- initial implementation will only provide a static / non extensible `GlobalSearchProviderContext` context. +It would be possible to allow plugins to register their own context providers as it's done for `RequestHandlerContext`, but this will not be done until the need arises. +- Because of the previous point, the performing `request` object is also exposed in the context to allow result providers +to scope their services if needed. #### public @@ -142,22 +147,11 @@ type GlobalSearchResultProvider = { options: GlobalSearchOptions, ): Observable; }; - -/** - * Enhanced {@link GlobalSearchResult | result type} for the client-side, - * to allow navigating to a given result. - */ -interface NavigableGlobalSearchResult extends GlobalSearchResult { - navigateTo: () => void; -} ``` Notes: - The client-side version of `GlobalSearchResultProvider` is slightly difference than the -server one, as there is no `context` parameter passed to `find`. -- `NavigableGlobalSearchResult` is here to enhanced results with a `navigateTo` method to -allow `core` to handle the navigation mechanism for the results, which is non-trivial. -See the [Redirecting to a result](#redirecting-to-a-result) section for more info. +server one, as there is no `context` parameter on the `find` signature. ### Plugin API @@ -201,6 +195,14 @@ interface GlobalSearchServiceStart { #### public API ```ts +/** + * Enhanced {@link GlobalSearchResult | result type} for the client-side, + * to allow navigating to a given result. + */ +interface NavigableGlobalSearchResult extends GlobalSearchResult { + navigateTo: () => void; +} + /** @public */ interface GlobalSearchServiceSetup { registerResultProvider(provider: GlobalSearchResultProvider); @@ -215,46 +217,118 @@ interface GlobalSearchServiceStart { } ``` -Notes: the public API looks quite similar to the server one. The differences are: -- the `registerResultProvider` setup API got the same signature, however the `GlobalSearchResultProvider` types is different -than the server one -- the `find` start API signature got a `KibanaRequest` for `server`, where this parameter is not present for `public`. +Notes: +- The public API looks quite similar to the server one. The differences are: + - The `registerResultProvider` setup API share the same signature, however the input `GlobalSearchResultProvider` + types are different on the client and server. + - The `find` start API signature got a `KibanaRequest` for `server`, where this parameter is not present for `public`. +- The `find` API returns a observable of `NavigableGlobalSearchResult` instead of plain `GlobalSearchResult`. This type +is here to enhance results with a `navigateTo` method to let `core` handle the navigation mechanism, which is +non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section for more info. #### http API -// TODO +An HTTP API will be exposed on `/api/core/global_search/find` to allow the client-side `GlobalSearchService` +to fetch results from the server-side result providers. + +```ts +router.post( + { + path: '/api/core/global_search/find', + validate: { + body: schema.object({ + term: schema.string(), + options: schema.maybe(schema.object({})), + }), + }, + }, + async (ctx, req, res) => { + const { term, options } = req.body; + const results = await ctx.core.globalSearch + .find(term, options) + .pipe(last()) + .toPromise(); + return res.ok({ + body: { + results, + }, + }); + } +); +``` -Note: this is the API that will consumed by the client-side `GlobalSearchService` to retrieve server-side results. It should -not be considered a public and supported API until the need to do so arises. +// TODO: expose on request handler context +Notes: +- This API should be considered internal until we'll officially need to expose it for external consumers such as ChatOps. +- Initial implementation will await for all results and then return them as a single response. As it's supported by + the service, it could theoretically be possible to stream the results instead, however that makes the consumption of + the API more difficult. If this become important at some point, a new `/api/core/global_search/find/async` endpoint + will be added. + ## Functional behavior -### Summary - -- `core` exposes an setup API to be able to register result providers (`GlobalSearchResultProvider`). These providers - can be registered from either public or server side, even if the interface for each sides is not exactly the same. -- `core` exposes a start API to be able to search for objects. This API is available from both public and server sides. -- when a search is triggered, the `GS` service will query all registered providers and then returns an aggregated result +### summary + +- `coreSetup.globalService` exposes an API to be able to register result providers (`GlobalSearchResultProvider`). + These providers can be registered from either public or server side, even if the interface for each side is not + exactly the same. +- `coreStart.globalService` exposes an API to be able to find objects. This API is available from both public + and server sides. + - When using the server `find` API, only results from providers registered from the server will be returned. + - When using the public `find` API, results from provider registered from either server and public sides will be returned. +- During a `find` call, the service will call all the registered result providers and collect their result observables. + Every time a result provider emits some new results, the `globalSearch` service will: + - Consolidate/enhance them + - Merge them with the already present results + - Sort and order the new aggregated results + - Emit this up to date list of results ### result provider registration -Due to the fact that some kind of results (i.e `application`) can currently only be retrieved from the public side of Kibana, -the `registerResultProvider` API will be available both from the public and the server counterpart of the `GlobalSearchService`. +Due to the fact that some kind of results (i.e `application`, and maybe later `management_section`) only exists on +the public side of Kibana and therefor are not known on the server side, the `registerResultProvider` API will be +available both from the public and the server counterpart of the `GlobalSearchService`. + However, as results from providers registered from the client-side will not be available from the server's `find` API, -registration of result providers from the client will be discouraged with proper documentation stating that it should -only be used when it is not technically possible to expose it from the server side instead. +registering result providers from the client should only be done to answer this specific use case and will be +discouraged, by providing appropriated jsdoc and documentation explaining that it should only +be used when it is not technically possible to register it from the server side instead. + +### results url consolidation + +When retrieving results from providers, the GS service will always start by consolidating them. The most notable (and +currently only) step is to convert the result url to an absolute one. + +#### absolute url conversion logic + +Results returned from the GlobalSearch's `find` programmatic and HTTP APIs will all contains absolute urls for the following +reasons: +- It would not be usable by external consumers otherwise. +- Some result providers are supposed to be returning results from outside of the current `basePath`, or even from outside of the + Kibana instance (ie: results from another space, results from another Kibana instance in a cloud cluster) + +However, as forging absolute urls can be a tedious process for the plugins, the `url` property of results returned by +a result provider can be either an absolute url or an url path relative to the executing request's `basePath`. + +I.E are considered valid: +- `https://my-other-kibana-instance/some/result` +- `/app/kibana#/dashboard/some-id` -### publicUrl +when consolidating the results, the logic regarding the `url` property is: +- if the `url` is absolute, return it unchanged +- if the `url` is considered relative (starts with `/`), it will be converted to an absolute url by prepending the Kibana +instance's newly introduced `publicAddress`. -To be usable from external services (such as ChatOps), results returned from the GlobalSearch public HTTP API should all contains absolute urls, as -relative paths would not be usable from outside of the instance. +#### server.publicAddress -However, given the fact that some Kibana deployments can have complex architecture, there is currently -no reliable way to know for sure what the public address used to access kibana is. +Given the fact that some Kibana deployments have complex architecture (proxies, rewrite rules...), there is currently +no reliable way to know for sure what the public address used to access kibana is (at least from the server-side). -A new `server.publicAddress` property will be added to the kibana configuration. +A new `server.publicAddress` property will be added to the kibana configuration, allowing ops to explicitly define the public +address to instance is accessible from. -When not manually defined in the configuration, this property will be constructed using the known `server` configuration values: +When not explicitly defined, this property will be constructed using the known `server` configuration values: ```ts const defaultPublicAddress = removeTrailingSlash( @@ -264,54 +338,63 @@ const defaultPublicAddress = removeTrailingSlash( const getPublicAddress = () => httpConfig.publicAddress ?? defaultPublicAddress; ``` -then a new `getAbsoluteUrl` api would be added to the core `http` service. +A new `getAbsoluteUrl` api would also be added to the core `http` service contract: ```ts const getAbsoluteUrl = (path: string, request: KibanaRequest) => { const publicUrl = getPublicAddress(); const absoluteUrl = joinRemovingDuplicateAndTrailingSlash( publicUrl, - serverContract.basePath.get(request), + // note: this is actually wrong. We would need the `requestScopePath` here + // as this currently returns `${this.serverBasePath}${requestScopePath}` and the basePath + // is already included in `publicAddress` + serverContract.basePath.get(request), path ); } ``` -Search results will then be consolidated before being returned to convert relative urls to absolute ones. +Search results will then be consolidated before being returned to convert relative urls to absolute ones: ```ts const consolidateResult(result: GlobalSearchResult, request: KibanaRequest) { - if(isUriPath(result.url)) { + if(isUrlPath(result.url)) { result.url = http.getAbsoluteUrl(result.url, request) } } ``` -### Redirecting to a result +#### Redirecting to a result -from the client-side, `NavigableGlobalSearchResult.navigateTo` would follow this logic: +Having absolute urls in our results is a necessity for external consumers, and makes the API more consistent than mixing +relative and absolute urls, however this makes it less trivial for UI consumers to redirect to a given result in a SPA +friendly way (using `application.navigateTo` instead of triggering a full page refresh). -If all 3 of these criteria are true for `result.url`: +This is why `NavigableGlobalSearchResult.navigateTo` has been introduced, to let `core` handles this navigation logic. + +When using `navigateTo` from a result instance, the following logic will be executed: -- The domain of the URL (if present) matches the domain of the `publicUrl` +If all 3 of these criteria are true for `result.url`: +- The domain of the URL matches the domain of the `publicUrl` - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches an application route (eg. /app//) -Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment +Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using +the remaining pathname segment -Otherwise, do a full page navigation (window.location.assign()) +Otherwise: do a full page navigation (window.location.assign()) ### searching from the server side -When calling `GlobalSearchServiceStart.search` from the server-side service: +When calling `GlobalSearchServiceStart.find` from the server-side service: - the service will call `find` on each server-side registered result provider and collect the resulting result observables -- then, the service will combine every result observable and trigger the next step on every emission until either - - the predefined timeout duration is reached - - Every providers result observable are completed +- then, the service will merge every result observable and trigger the next step on every emission until either + - A predefined timeout duration is reached + - All result observables are completed -- on every emission of the combined observable, the results will be aggregated and sorted following +- on every emission of the merged observable, the results will be aggregated to the existing list and sorted following the logic defined in the [results aggregation](#results-aggregation) section A very naive implementation of this behavior would be: @@ -325,12 +408,13 @@ search( const fromProviders$ = this.providers.map(p => p.find(term, options, contextFromRequest(request)) ); - - return combineLatest([...fromProviders$]).pipe( - takeUntil(timeout$) - map(resultLists => { - return mergeAndOrder(resultLists); - }) + const results: GlobalSearchResult[] = []; + return merge([...fromProviders$]).pipe( + takeUntil(timeout$), + map(newResults => { + results.push(...newResults); + return order(results); + }), ); } ``` @@ -339,15 +423,15 @@ search( When calling `GlobalSearchServiceStart.search` from the public-side service: -- the service will call: +- The service will call: - the server-side API via an http call to fetch results from the server-side result providers - `find` on each client-side registered result provider and collect the resulting observables -- then, the service will combine every result observable and trigger the next step on every emission until either - - the predefined timeout duration is reached - - Every providers result observable are completed +- Then, the service will merge every result observable and trigger the next step on every emission until either + - A predefined timeout duration is reached + - All result observables are completed -- on every emission of the combined observable, the results will be aggregated and sorted following +- On every emission of the merged observable, the results will be aggregated to the existing list and sorted following the logic defined in the [results aggregation](#results-aggregation) section A very naive implementation of this behavior would be: @@ -362,26 +446,30 @@ search( ); const fromServer$ = of(this.fetchServerResults(term, options)) - return combineLatest([...fromProviders$, fromServer$]).pipe( - takeUntil(timeout$) - map(resultLists => { - return mergeAndOrder(resultLists); - }) + const results: GlobalSearchResult[] = []; + return merge([...fromProviders$, fromServer$]).pipe( + takeUntil(timeout$), + map(newResults => { + results.push(...newResults); + return order(results); + }), ); } ``` -Note: due to the complexity of the process, the initial implementation will not be streaming results from the server, -meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' http call to the GS endpoint). -The observable-based API architecture is ready for this however, and the enhancement could be added at a later time. +Notes: +- Due to the complexity of the process, the initial implementation will not be streaming results from the server, +meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' +http call to the GS endpoint). The observable-based API architecture is ready for this however, and the enhancement +could be added at a later time. ### results aggregation -On every emission of an underlying provider, the service will aggregate and sort the results following this logic before emitting them: +On every emission of an underlying provider, the service will aggregate and sort the results following this logic +before emitting them: -- results from all providers will be merged in a single list. -- results will be sorted by ascending `type` ordinal value. -- results of a same `type` will then be sorted by descending `score` value. +- Results will be sorted by ascending `type` ordinal value. +- Results of a same `type` will then be sorted by descending `score` value. This is an equivalent of the following lodash call: @@ -415,10 +503,10 @@ const sorted = [ #### Note on score value -Due to the fact that the results will be coming from various providers, from distinct ES queries or even not from ES, +Due to the fact that the results will be coming from various providers, from multiple ES queries or even not from ES, using a centralized scoring mechanism is not possible. -the `GlobalSearchResult` contains a `score` field, with an expected value from 1 (lowest) to 100 (highest). +the `GlobalSearchResult` contains a `score` field, with an expected value going from 1 (lowest) to 100 (highest). How this field is populated from each individual provider is considered an implementation detail. ### Search cancellation @@ -428,17 +516,59 @@ cannot be canceled, neither from the public nor server API. # Drawbacks -- The fact that some result providers must be on the client-side complexify the API. +See alternatives. # Alternatives -- could only allow to register result providers from the server-side for the public API - - would ensure - +## Result providers could only be registrable from the server-side API + +The fact that some kind of results, and therefor some result providers must be on the client-side complexifies the API, +while making these results not available from the server-side and HTTP APIs. + +We could decide to only allow providers registration from the server-side. It would reduce API exposure, while simplifying +the service implementation. However to do that, we would need to find a solution for existing needs regarding `application` +(and later `management_section`) type provider. + +I will directly exclude here the option to move the `application` registration from client to server-side, as it's +going the opposite way of the KP philosophy while being an heavy breaking change. + +### AST parsing + +One option to make the `application` type 'visible' from the server-side would be to parse the client code at build time +using AST, and generates a server file containing the applications. The server-side `application` result provider would +then just read this file and uses it to return application results. + +However +- At the parsing would be done at build time, we would not be able to generate entries for any 3rd party plugins +- As entries for every existing applications would be generated, the search provider would to be able to know which +applications are actually enabled/accessible at runtime to filter them, which is all but easy +- It will also not contains test plugin apps, making it really hard to FTR. +- AST parsing is a complex mechanism for an already unsatisfactory alternative + +### Duplicated server-side `application.register` API + +One other option would be to duplicate the `application.register` API on the server side, with a subset of the +client-side metadata. + +```ts +core.application.register({ + id: 'app_status', + title: 'App Status', + euiIconType: 'snowflake' +}); +``` + +This way, the applications could be searchable from the server using this server-side `applications` registry. + +However +- It forces plugin developers to add this API call. In addition to be a very poor developer experience, it can also + very easily be forgotten, making a given app non searchable +- client-side only plugins would need to add a server-side part to their plugin just to register their application on + the server side ## `GlobalSearchResult.url` could be a struct instead of a url for internal results. -One of the initial proposal was to have +The initial idea for `GlobalSearchResult.url` was to have a polymorphic structure instead of just a string: ```ts url: { absUrl: string } | { application: string; path?: string }; @@ -447,19 +577,19 @@ url: { absUrl: string } | { application: string; path?: string }; That was making it way easier to redirect to an internal result from the UI, as we could directly call `application.navigateTo(application, { path })`. -However, that didn't answer for need for the (future) need to be able to search for and redirect to object in -different spaces. We could have then changed to +However, that didn't bring answer for the (future) need to be able to search for and redirect to object in +different spaces. We could then have changed the structure to ```ts url: { absUrl: string } | { application: string; path?: string, space?: string }; ``` -But this had some issues: -- `space` is an xpack plugin, adding this property in the oss implementation is problematic -- `space` API is not callable from core or oss, meaning that we would have to 'forge' the url to this space anyway -- this is really not generic. If another plugin was to alter the basepath, we would have needed to add it another property +But this had some caveats: +- `space` is an xpack plugin, adding this `space` property in an oss part of the code is problematic +- The `space` API is not callable from core or oss, meaning that we would have to 'forge' the url to this space anyway +- this is really not generic. If another plugin was to alter the basepath in another way, we would have needed to add it another property -So even if the 'parsable absolute url' approach seems fragile, it's probably still better than this alternative. +So even if the 'parsable absolute url' approach seems fragile, it still felt than this alternative. ## We could use plain string instead of an enum for `GlobalSearchResult.type` @@ -484,34 +614,48 @@ interface GlobalSearchResult { ``` has some limitations: -- it forces the enum to be modified every time a new type is added +- It forces the enum to be modified every time a new type is added - 3rd party plugins cannot introduce new types -We could change the API to accept plain strings for `GlobalSearchResult.type`, however, atm this enum approach +We could change the API to accept plain strings for `GlobalSearchResult.type`. However, atm this enum approach is needed as the ordinal values of the entries is used in results sorting. Changing to plain strings forces to find -another sorting approach. +an alternative sorting logic. ## triggered searches could be cancelable In current specifications, once a search has been triggered, it's not possible to cancel it (see [Search cancellation](#search-cancellation)) -Main drawback of this decision is that if a `search` consumer +Main drawback of this decision is that if a `find` API consumer is going to perform multiple `find` requests and +only uses the last one (For example in the UI, a debounced `find` triggered on end-user keypress: On every new keypress, +a new search is performed and the previous, potentially still executing, `find` call will still be executed) -We could add an optional signal or observable in `GlobalSearchOptions`. That way a consumer knowing that a `find` call may be aborted could use -this option. +We could add an optional cancellation signal or observable in `GlobalSearchOptions` and propagate it to result providers. +That way `find` calls could be aborted. -However result providers from plugins would still have to manually handles this signal to cancel any http call or +However result providers would still have to manually handles this signal to cancel any http call or other asynchronous task that could be pending. -Note that as this can be implemented with an additional option, this can be done at a later time. +Notes: +- As this can be implemented with an additional option, this would be a non-breaking change and so could be done at + a later time (even if it means that existing result provider would not handles this option until they implements it). ## The GlobalSearch service could be provided as a plugin instead of a core service -- could be provided as a plugin instead of a core service - - However as GS is going to be used in the header, what still would mean a bridge of some kind to be able to register it - to core. - - As the platform team is going to provide the base result providers for search results and application, that would mean - create yet another plugin for these providers +The GlobalSearch API could be provided and exposed from a plugin instead of `core`. + +Pros: +- Less `core` API exposure + +Cons: +- We know our initial consumer of this API is going to use it from/for the chrome header component, and because this component + is in `core`, that would mean creating a bridge of some kind to be able to use the service from core, as plugin APIs + are not usable here. +- The platform team is going to provide the base result providers for SO and application search results, so having the GS + service in a plugin would mean creating yet another plugin for these providers instead of having them self contained + in core. +- We are probably going to be adding internal SO API for the SO result provider, as current APIs are not sufficient. It + would be better if these APIs where kept internal, which would not be possible if the SO provider is in a plugin + instead of core. # Adoption strategy @@ -519,8 +663,7 @@ The `globalSearch` service is a new feature provided by the `core` API. Also, th used to search for saved objects and applications will be implemented by the platform team, meaning that by default, plugin developers won't have to do anything. -Plugins that wish to expose more detail about their availability will easily be able to do so, -including providing detailed information such as links to documentation to resolve the problem. +Plugins that wish to expose additional result providers will easily be able to do so by using the exposed APIs. # How we teach this From 2e1c0066bc0169e936bc3e8434bf0660b1c3666d Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 24 Apr 2020 15:14:51 +0200 Subject: [PATCH 06/21] initial self review --- rfcs/text/0011_global_search.md | 99 ++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 45 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 02744fe3e90c8..cd8c51c6fd0bb 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -32,12 +32,12 @@ coreStart.globalSearch.find('some term').subscribe(({ results, complete }) => { # Motivation -The main goal of this feature is to power the global search bar [#57576](https://github.com/elastic/kibana/issues/57576). +Kibana should do it's best to assist users searching for and navigating to the various objects present on the Kibana platform. -While this first point remains the priority, this API should also provide a solution for alternative needs regarding -searching for arbitrary objects from a Kibana instance. +We should expose an API to make it possible for plugins to search for the various objects present on a Kibana instance. -// TODO: develop probably +The first consumer of this API will be the global search bar [#57576](https://github.com/elastic/kibana/issues/57576). This API +should still be generic to answer similar needs from any other consumer, either client or server side. # Detailed design @@ -131,8 +131,8 @@ Notes: - initial implementation will only provide a static / non extensible `GlobalSearchProviderContext` context. It would be possible to allow plugins to register their own context providers as it's done for `RequestHandlerContext`, but this will not be done until the need arises. -- Because of the previous point, the performing `request` object is also exposed in the context to allow result providers -to scope their services if needed. +- Because of the previous point, the performing `request` object is also exposed on the context to allow result providers +to scope their custom services if needed. #### public @@ -159,7 +159,7 @@ server one, as there is no `context` parameter on the `find` signature. ```ts /** - * Response returned from a {@link GlobalSearchResultProvider | result provider} + * Response returned from the {@link GlobalSearchServiceStart | global search service}'s `find` API */ type GlobalSearchResponse = { /** @@ -167,7 +167,7 @@ type GlobalSearchResponse void; } @@ -218,12 +222,12 @@ interface GlobalSearchServiceStart { ``` Notes: -- The public API looks quite similar to the server one. The differences are: - - The `registerResultProvider` setup API share the same signature, however the input `GlobalSearchResultProvider` +- The public API is very similar to its server counterpart. The differences are: + - The `registerResultProvider` setup APIs share the same signature, however the input `GlobalSearchResultProvider` types are different on the client and server. - - The `find` start API signature got a `KibanaRequest` for `server`, where this parameter is not present for `public`. + - The `find` start API signature got a `KibanaRequest` for `server`, when this parameter is not present for `public`. - The `find` API returns a observable of `NavigableGlobalSearchResult` instead of plain `GlobalSearchResult`. This type -is here to enhance results with a `navigateTo` method to let `core` handle the navigation mechanism, which is +is here to enhance results with a `navigateTo` method to let `core` handle the navigation logic, which is non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section for more info. #### http API @@ -231,6 +235,8 @@ non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section An HTTP API will be exposed on `/api/core/global_search/find` to allow the client-side `GlobalSearchService` to fetch results from the server-side result providers. +It should be very close to: + ```ts router.post( { @@ -257,14 +263,13 @@ router.post( ); ``` -// TODO: expose on request handler context - Notes: +- A new `globalSearch` service will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. - This API should be considered internal until we'll officially need to expose it for external consumers such as ChatOps. - Initial implementation will await for all results and then return them as a single response. As it's supported by the service, it could theoretically be possible to stream the results instead, however that makes the consumption of - the API more difficult. If this become important at some point, a new `/api/core/global_search/find/async` endpoint - will be added. + the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` + endpoint could be added. ## Functional behavior @@ -273,7 +278,7 @@ Notes: - `coreSetup.globalService` exposes an API to be able to register result providers (`GlobalSearchResultProvider`). These providers can be registered from either public or server side, even if the interface for each side is not exactly the same. -- `coreStart.globalService` exposes an API to be able to find objects. This API is available from both public +- `coreStart.globalService` exposes an API to be able to search for objects. This API is available from both public and server sides. - When using the server `find` API, only results from providers registered from the server will be returned. - When using the public `find` API, results from provider registered from either server and public sides will be returned. @@ -315,9 +320,9 @@ I.E are considered valid: - `https://my-other-kibana-instance/some/result` - `/app/kibana#/dashboard/some-id` -when consolidating the results, the logic regarding the `url` property is: +When consolidating the result, the logic regarding the `url` property is: - if the `url` is absolute, return it unchanged -- if the `url` is considered relative (starts with `/`), it will be converted to an absolute url by prepending the Kibana +- if the `url` is a relative path (starts with `/`), it will be converted to an absolute url by prepending the Kibana instance's newly introduced `publicAddress`. #### server.publicAddress @@ -326,9 +331,10 @@ Given the fact that some Kibana deployments have complex architecture (proxies, no reliable way to know for sure what the public address used to access kibana is (at least from the server-side). A new `server.publicAddress` property will be added to the kibana configuration, allowing ops to explicitly define the public -address to instance is accessible from. +address to instance is accessible from. This property is only meant to be used to generate 'links' to arbitrary resources +and will not be used to configure the http server in any way. -When not explicitly defined, this property will be constructed using the known `server` configuration values: +When not explicitly defined, this property will be computed using the known `server` configuration values: ```ts const defaultPublicAddress = removeTrailingSlash( @@ -338,7 +344,7 @@ const defaultPublicAddress = removeTrailingSlash( const getPublicAddress = () => httpConfig.publicAddress ?? defaultPublicAddress; ``` -A new `getAbsoluteUrl` api would also be added to the core `http` service contract: +A new `getAbsoluteUrl` api will also be added to the core `http` service contract: ```ts const getAbsoluteUrl = (path: string, request: KibanaRequest) => { @@ -366,7 +372,7 @@ const consolidateResult(result: GlobalSearchResult, request: KibanaRequest) { #### Redirecting to a result -Having absolute urls in our results is a necessity for external consumers, and makes the API more consistent than mixing +Having absolute urls in GS results is a necessity for external consumers, and makes the API more consistent than mixing relative and absolute urls, however this makes it less trivial for UI consumers to redirect to a given result in a SPA friendly way (using `application.navigateTo` instead of triggering a full page refresh). @@ -382,7 +388,7 @@ If all 3 of these criteria are true for `result.url`: Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment -Otherwise: do a full page navigation (window.location.assign()) +Otherwise: do a full page navigation (`window.location.assign()`) ### searching from the server side @@ -421,7 +427,7 @@ search( ### searching from the client side -When calling `GlobalSearchServiceStart.search` from the public-side service: +When calling `GlobalSearchServiceStart.find` from the public-side service: - The service will call: - the server-side API via an http call to fetch results from the server-side result providers @@ -465,8 +471,8 @@ could be added at a later time. ### results aggregation -On every emission of an underlying provider, the service will aggregate and sort the results following this logic -before emitting them: +On every emission of an underlying provider, the service will aggregate the new results with the existing list and +sort the results following this logic before emitting them: - Results will be sorted by ascending `type` ordinal value. - Results of a same `type` will then be sorted by descending `score` value. @@ -511,8 +517,8 @@ How this field is populated from each individual provider is considered an imple ### Search cancellation -Once triggered, a given call to `GlobalSearchServiceStart.find` (and underlying search provider's `find` method) -cannot be canceled, neither from the public nor server API. +In initial implementation, once triggered, a given call to `GlobalSearchServiceStart.find` +(and underlying search provider's `find` method) cannot be canceled, neither from the public nor server API. # Drawbacks @@ -520,29 +526,30 @@ See alternatives. # Alternatives -## Result providers could only be registrable from the server-side API +## Result providers could be only registrable from the server-side API -The fact that some kind of results, and therefor some result providers must be on the client-side complexifies the API, +The fact that some kind of results, and therefor some result providers, must be on the client-side complexifies the API, while making these results not available from the server-side and HTTP APIs. We could decide to only allow providers registration from the server-side. It would reduce API exposure, while simplifying -the service implementation. However to do that, we would need to find a solution for existing needs regarding `application` -(and later `management_section`) type provider. +the service implementation. However to do that, we would need to find a solution to be able to implement a server-side + result provider for `application` (and later `management_section`) type provider. -I will directly exclude here the option to move the `application` registration from client to server-side, as it's -going the opposite way of the KP philosophy while being an heavy breaking change. +I will directly exclude the option to move the `application` registration (`core.application.register`) from client +to server-side, as it's going the opposite way of the KP philosophy while also being an heavy breaking change. ### AST parsing -One option to make the `application` type 'visible' from the server-side would be to parse the client code at build time -using AST, and generates a server file containing the applications. The server-side `application` result provider would -then just read this file and uses it to return application results. +One option to make the `application` results 'visible' from the server-side would be to parse the client code at build time +using AST to find all usages to `application.register` inspect the parameters, and generates a server file +containing the applications. The server-side `application` result provider would then just read this file and uses it +to return application results. However - At the parsing would be done at build time, we would not be able to generate entries for any 3rd party plugins - As entries for every existing applications would be generated, the search provider would to be able to know which applications are actually enabled/accessible at runtime to filter them, which is all but easy -- It will also not contains test plugin apps, making it really hard to FTR. +- It will also not contains test plugin apps, making it really hard to FTR - AST parsing is a complex mechanism for an already unsatisfactory alternative ### Duplicated server-side `application.register` API @@ -589,7 +596,7 @@ But this had some caveats: - The `space` API is not callable from core or oss, meaning that we would have to 'forge' the url to this space anyway - this is really not generic. If another plugin was to alter the basepath in another way, we would have needed to add it another property -So even if the 'parsable absolute url' approach seems fragile, it still felt than this alternative. +So even if the 'parsable absolute url' approach seems fragile, it still felt better than this alternative. ## We could use plain string instead of an enum for `GlobalSearchResult.type` @@ -627,7 +634,8 @@ In current specifications, once a search has been triggered, it's not possible t Main drawback of this decision is that if a `find` API consumer is going to perform multiple `find` requests and only uses the last one (For example in the UI, a debounced `find` triggered on end-user keypress: On every new keypress, -a new search is performed and the previous, potentially still executing, `find` call will still be executed) +a new search is performed and the previous, potentially still executing, `find` call will still be executed), it can't +cancel the previous active search, resulting on potential useless http calls and resources consumption. We could add an optional cancellation signal or observable in `GlobalSearchOptions` and propagate it to result providers. That way `find` calls could be aborted. @@ -653,9 +661,9 @@ Cons: - The platform team is going to provide the base result providers for SO and application search results, so having the GS service in a plugin would mean creating yet another plugin for these providers instead of having them self contained in core. -- We are probably going to be adding internal SO API for the SO result provider, as current APIs are not sufficient. It - would be better if these APIs where kept internal, which would not be possible if the SO provider is in a plugin - instead of core. +- We are probably going to be adding internal SO APIs for the SO result provider, as current APIs are not sufficient to + search for multiple type of SO at the same time. It would be better if these APIs were kept internal, which would + not be possible if the SO provider is in a plugin instead of core. # Adoption strategy @@ -663,7 +671,8 @@ The `globalSearch` service is a new feature provided by the `core` API. Also, th used to search for saved objects and applications will be implemented by the platform team, meaning that by default, plugin developers won't have to do anything. -Plugins that wish to expose additional result providers will easily be able to do so by using the exposed APIs. +Plugins that wish to expose additional result providers will easily be able to do so by using the exposed APIs and +documentation. # How we teach this From cc473feba038c65532d3c610d375168b4628716e Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Wed, 29 Apr 2020 22:56:57 +0200 Subject: [PATCH 07/21] nits and comments --- rfcs/text/0011_global_search.md | 63 ++++++++++++++++----------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index cd8c51c6fd0bb..8a29c502b1fff 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -4,7 +4,7 @@ # Summary -A core API exposed on both public and server sides, allowing to search for various objects and +A Kibana API exposed on both public and server side, allowing consumers to search for various objects and register result providers. # Basic example @@ -24,15 +24,16 @@ coreSetup.globalSearch.registerResultProvider({ - using the `find` API from the client-side: ```ts -coreStart.globalSearch.find('some term').subscribe(({ results, complete }) => { - showAsyncSearchIndicator(!complete); +coreStart.globalSearch.find('some term').subscribe(({ results }) => { updateResults(results); +}, () => {}, () => { + showAsyncSearchIndicator(false); }); ``` # Motivation -Kibana should do it's best to assist users searching for and navigating to the various objects present on the Kibana platform. +Kibana should do its best to assist users searching for and navigating to the various objects present on the Kibana platform. We should expose an API to make it possible for plugins to search for the various objects present on a Kibana instance. @@ -70,7 +71,7 @@ type GlobalSearchResultType = keyof typeof SEARCH_TYPE; interface GlobalSearchOptions {} /** - * Representation of a result returned by a {@linlkGlobalSearchResultProvider | result provider} + * Representation of a result returned by a {@link GlobalSearchResultProvider | result provider} */ interface GlobalSearchResult { /** an id that should be unique for an individual provider's results */ @@ -79,7 +80,7 @@ interface GlobalSearchResult { title: string; /** the type of result */ type: GlobalSearchResultType; - /** an optional EUI icon name to associate with the search result. If not specified, will use a default icon */ + /** an optional EUI icon name to associate with the search result */ icon?: string; /** The url to navigate to this result. This can be either an absolute url, or a path relative to the performing request's basePath */ url: string; @@ -150,7 +151,7 @@ type GlobalSearchResultProvider = { ``` Notes: -- The client-side version of `GlobalSearchResultProvider` is slightly difference than the +- The client-side version of `GlobalSearchResultProvider` is slightly different than the server one, as there is no `context` parameter on the `find` signature. ### Plugin API @@ -166,11 +167,6 @@ type GlobalSearchResponse void; + navigate: () => Promise; } /** @public */ @@ -227,12 +223,12 @@ Notes: types are different on the client and server. - The `find` start API signature got a `KibanaRequest` for `server`, when this parameter is not present for `public`. - The `find` API returns a observable of `NavigableGlobalSearchResult` instead of plain `GlobalSearchResult`. This type -is here to enhance results with a `navigateTo` method to let `core` handle the navigation logic, which is +is here to enhance results with a `navigate` method to let `core` handle the navigation logic, which is non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section for more info. #### http API -An HTTP API will be exposed on `/api/core/global_search/find` to allow the client-side `GlobalSearchService` +An internal HTTP API will be exposed on `/internal/core/global_search/find` to allow the client-side `GlobalSearchService` to fetch results from the server-side result providers. It should be very close to: @@ -264,8 +260,9 @@ router.post( ``` Notes: +- This API is only for internal use and communication between the client and the server parts of the `GS` API. When +the need to expose an API for external consumers will appear, a new public API will be exposed for that. - A new `globalSearch` service will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. -- This API should be considered internal until we'll officially need to expose it for external consumers such as ChatOps. - Initial implementation will await for all results and then return them as a single response. As it's supported by the service, it could theoretically be possible to stream the results instead, however that makes the consumption of the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` @@ -281,7 +278,7 @@ Notes: - `coreStart.globalService` exposes an API to be able to search for objects. This API is available from both public and server sides. - When using the server `find` API, only results from providers registered from the server will be returned. - - When using the public `find` API, results from provider registered from either server and public sides will be returned. + - When using the public `find` API, results from provider registered from both server and public sides will be returned. - During a `find` call, the service will call all the registered result providers and collect their result observables. Every time a result provider emits some new results, the `globalSearch` service will: - Consolidate/enhance them @@ -300,9 +297,9 @@ registering result providers from the client should only be done to answer this discouraged, by providing appropriated jsdoc and documentation explaining that it should only be used when it is not technically possible to register it from the server side instead. -### results url consolidation +### results url processing -When retrieving results from providers, the GS service will always start by consolidating them. The most notable (and +When retrieving results from providers, the GS service will always start by processing them. The most notable (and currently only) step is to convert the result url to an absolute one. #### absolute url conversion logic @@ -320,7 +317,7 @@ I.E are considered valid: - `https://my-other-kibana-instance/some/result` - `/app/kibana#/dashboard/some-id` -When consolidating the result, the logic regarding the `url` property is: +When processing the result, the logic regarding the `url` property is: - if the `url` is absolute, return it unchanged - if the `url` is a relative path (starts with `/`), it will be converted to an absolute url by prepending the Kibana instance's newly introduced `publicAddress`. @@ -360,10 +357,10 @@ const getAbsoluteUrl = (path: string, request: KibanaRequest) => { } ``` -Search results will then be consolidated before being returned to convert relative urls to absolute ones: +Search results will then be processed before being returned to convert relative urls to absolute ones: ```ts -const consolidateResult(result: GlobalSearchResult, request: KibanaRequest) { +const processResult(result: GlobalSearchResult, request: KibanaRequest) { if(isUrlPath(result.url)) { result.url = http.getAbsoluteUrl(result.url, request) } @@ -374,16 +371,16 @@ const consolidateResult(result: GlobalSearchResult, request: KibanaRequest) { Having absolute urls in GS results is a necessity for external consumers, and makes the API more consistent than mixing relative and absolute urls, however this makes it less trivial for UI consumers to redirect to a given result in a SPA -friendly way (using `application.navigateTo` instead of triggering a full page refresh). +friendly way (using `application.navigateToApp` instead of triggering a full page refresh). -This is why `NavigableGlobalSearchResult.navigateTo` has been introduced, to let `core` handles this navigation logic. +This is why `NavigableGlobalSearchResult.navigate` has been introduced, to let `core` handle this navigation logic. -When using `navigateTo` from a result instance, the following logic will be executed: +When using `navigate` from a result instance, the following logic will be executed: If all 3 of these criteria are true for `result.url`: -- The domain of the URL matches the domain of the `publicUrl` +- The origin of the URL matches the origin of the `publicUrl` - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) -- The pathname segment after the basePath matches an application route (eg. /app//) +- The pathname segment after the basePath matches any known application route (eg. /app// or any application's `appRoute` configuration) Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using the remaining pathname segment @@ -528,7 +525,7 @@ See alternatives. ## Result providers could be only registrable from the server-side API -The fact that some kind of results, and therefor some result providers, must be on the client-side complexifies the API, +The fact that some kinds of results, and therefore some result providers, must be on the client-side makes the API more complex, while making these results not available from the server-side and HTTP APIs. We could decide to only allow providers registration from the server-side. It would reduce API exposure, while simplifying @@ -536,7 +533,8 @@ the service implementation. However to do that, we would need to find a solution result provider for `application` (and later `management_section`) type provider. I will directly exclude the option to move the `application` registration (`core.application.register`) from client -to server-side, as it's going the opposite way of the KP philosophy while also being an heavy breaking change. +to server-side, as it's a very heavy impacting (and breaking) change to `core` APIs that would requires more reasons +than just this RFC/API to consider. ### AST parsing @@ -582,7 +580,7 @@ url: { absUrl: string } | { application: string; path?: string }; ``` That was making it way easier to redirect to an internal result from the UI, as we could directly call -`application.navigateTo(application, { path })`. +`application.navigateToApp(application, { path })`. However, that didn't bring answer for the (future) need to be able to search for and redirect to object in different spaces. We could then have changed the structure to @@ -678,7 +676,8 @@ documentation. This follows the same patterns we have used for other Core APIs: Observables subscriptions, etc. -This should be taught using the same channels we've leveraged for other Kibana Platform APIs, API documentation mostly. +This should be taught using the same channels we've leveraged for other Kibana Platform APIs, API documentation and +example plugins. # Unresolved questions From cbdc44ce111f8353b77237dbeb6b0c7dd420f9ba Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 08:58:06 +0200 Subject: [PATCH 08/21] add 'preference' option --- rfcs/text/0011_global_search.md | 47 ++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 8a29c502b1fff..60f2b3c6bec73 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -66,9 +66,15 @@ type GlobalSearchResultType = keyof typeof SEARCH_TYPE; /** * Options provided to {@link GlobalSearchResultProvider | result providers} `find` method. - * Currently empty and only present to keep the API future-proof. */ -interface GlobalSearchOptions {} +interface GlobalSearchProviderFindOptions { + /** + * A custom preference token associated with a search 'session' that should be used to get consistent scoring + * when performing calls to ES. Can also be used as a 'session' token for providers returning data from elsewhere + * than an elasticsearch cluster. + */ + preference: string; +} /** * Representation of a result returned by a {@link GlobalSearchResultProvider | result provider} @@ -122,7 +128,7 @@ type GlobalSearchResultProvider = { id: string; find( term: string, - options: GlobalSearchOptions, + options: GlobalSearchProviderFindOptions, context: GlobalSearchProviderContext ): Observable; }; @@ -145,7 +151,7 @@ type GlobalSearchResultProvider = { id: string; find( term: string, - options: GlobalSearchOptions, + options: GlobalSearchProviderFindOptions, ): Observable; }; ``` @@ -173,6 +179,19 @@ type GlobalSearchResponse; } @@ -191,6 +210,12 @@ interface GlobalSearchServiceStart { #### public API ```ts +/** + * Options for the {@link GlobalSearchServiceStart.find | find API} + * Currently empty and only present for keeping the API future-proof. + */ +interface GlobalSearchFindOptions {} + /** * Enhanced {@link GlobalSearchResult | result type} for the client-side, * to allow navigating to a given result. @@ -212,7 +237,7 @@ interface GlobalSearchServiceSetup { interface GlobalSearchServiceStart { find( term: string, - options: GlobalSearchOptions, + options: GlobalSearchFindOptions, ): Observable>; } ``` @@ -240,7 +265,9 @@ router.post( validate: { body: schema.object({ term: schema.string(), - options: schema.maybe(schema.object({})), + options: schema.maybe(schema.object({ + preference: schema.maybe(schema.string()), + })), }), }, }, @@ -405,7 +432,7 @@ A very naive implementation of this behavior would be: ```ts search( term: string, - options: GlobalSearchOptions, + options: GlobalSearchFindOptions, request: KibanaRequest ): Observable { const fromProviders$ = this.providers.map(p => @@ -442,7 +469,7 @@ A very naive implementation of this behavior would be: ``` search( term: string, - options: GlobalSearchOptions, + options: GlobalSearchFindOptions, ): Observable { const fromProviders$ = this.providers.map(p => p.find(term, options) @@ -635,7 +662,7 @@ only uses the last one (For example in the UI, a debounced `find` triggered on e a new search is performed and the previous, potentially still executing, `find` call will still be executed), it can't cancel the previous active search, resulting on potential useless http calls and resources consumption. -We could add an optional cancellation signal or observable in `GlobalSearchOptions` and propagate it to result providers. +We could add an optional cancellation signal or observable in `GlobalSearchFindOptions` and propagate it to result providers. That way `find` calls could be aborted. However result providers would still have to manually handles this signal to cancel any http call or From 5d6e4cbbb48a8ec2155fcd51f08b8198a3aee9bc Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 09:13:31 +0200 Subject: [PATCH 09/21] change meta type to Record --- rfcs/text/0011_global_search.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 60f2b3c6bec73..60e505c8d5323 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -93,10 +93,19 @@ interface GlobalSearchResult { /** the score of the result, from 1 (lowest) to 100 (highest) */ score: number; /** an optional record of metadata for this result */ - meta?: Record; + meta?: Record; } ``` +Notes: +- The `Serializable` type should be implemented and exposed from `core`. A base implementation could be: + +```ts +type Serializable = string | number | boolean | PrimitiveArray | PrimitiveRecord; +interface PrimitiveArray extends Array {} +interface PrimitiveRecord extends Record {} +``` + #### server ```ts From 3a094aae6bf8b96100ed1e77d64314ca1e4ef646 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 09:44:47 +0200 Subject: [PATCH 10/21] specify cancellation --- rfcs/text/0011_global_search.md | 66 +++++++++++++++++---------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 60e505c8d5323..7671a74fe94ce 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -74,6 +74,12 @@ interface GlobalSearchProviderFindOptions { * than an elasticsearch cluster. */ preference: string; + /** + * Observable that emit once if and when the `find` call has been aborted by the consumer, or when the timeout period as been reached. + * When a `find` request is aborted, the service will stop emitting any new result to the consumer anyway, but + * this can (and should) be used to cancel any pending asynchronous task and complete the result observable. + */ + aborted$: Observable; } /** @@ -189,7 +195,7 @@ type GlobalSearchResponse; } /** @public */ @@ -220,10 +231,15 @@ interface GlobalSearchServiceStart { ```ts /** - * Options for the {@link GlobalSearchServiceStart.find | find API} - * Currently empty and only present for keeping the API future-proof. + * Options for the client-side {@link GlobalSearchServiceStart.find | find API} */ -interface GlobalSearchFindOptions {} +interface GlobalSearchFindOptions { + /** + * Optional observable to notify that the associated `find` call should be canceled. + * If/when provided and emitting, the result observable will be completed and no further result emission will be performed. + */ + aborted$?: Observable; +} /** * Enhanced {@link GlobalSearchResult | result type} for the client-side, @@ -283,7 +299,7 @@ router.post( async (ctx, req, res) => { const { term, options } = req.body; const results = await ctx.core.globalSearch - .find(term, options) + .find(term, { ...options, $aborted: req.events.aborted$ }) .pipe(last()) .toPromise(); return res.ok({ @@ -444,12 +460,13 @@ search( options: GlobalSearchFindOptions, request: KibanaRequest ): Observable { + const aborted$ = merge(timeout$, options.$aborted).pipe(first()) const fromProviders$ = this.providers.map(p => - p.find(term, options, contextFromRequest(request)) + p.find(term, { ...options, aborted$ }, contextFromRequest(request)) ); const results: GlobalSearchResult[] = []; return merge([...fromProviders$]).pipe( - takeUntil(timeout$), + takeUntil(aborted$), map(newResults => { results.push(...newResults); return order(results); @@ -480,14 +497,15 @@ search( term: string, options: GlobalSearchFindOptions, ): Observable { + const aborted$ = merge(timeout$, options.$aborted).pipe(first()) const fromProviders$ = this.providers.map(p => - p.find(term, options) + p.find(term, { ...options, aborted$ }) ); - const fromServer$ = of(this.fetchServerResults(term, options)) + const fromServer$ = of(this.fetchServerResults(term, options, aborted$)) const results: GlobalSearchResult[] = []; return merge([...fromProviders$, fromServer$]).pipe( - takeUntil(timeout$), + takeUntil(aborted$), map(newResults => { results.push(...newResults); return order(results); @@ -550,8 +568,13 @@ How this field is populated from each individual provider is considered an imple ### Search cancellation -In initial implementation, once triggered, a given call to `GlobalSearchServiceStart.find` -(and underlying search provider's `find` method) cannot be canceled, neither from the public nor server API. +Consumers can cancel a `find` call at any time by providing a cancellation observable with +the `GlobalSearchFindOptions.aborted$` option and then emitting from it. + +When this observable is provided and emitting, the GS service will complete the result observable. + +This observable will also be passed down to the underlying result providers, that can leverage it to cancel any pending +asynchronous task and perform cleanup if necessary. # Drawbacks @@ -662,25 +685,6 @@ We could change the API to accept plain strings for `GlobalSearchResult.type`. H is needed as the ordinal values of the entries is used in results sorting. Changing to plain strings forces to find an alternative sorting logic. -## triggered searches could be cancelable - -In current specifications, once a search has been triggered, it's not possible to cancel it (see [Search cancellation](#search-cancellation)) - -Main drawback of this decision is that if a `find` API consumer is going to perform multiple `find` requests and -only uses the last one (For example in the UI, a debounced `find` triggered on end-user keypress: On every new keypress, -a new search is performed and the previous, potentially still executing, `find` call will still be executed), it can't -cancel the previous active search, resulting on potential useless http calls and resources consumption. - -We could add an optional cancellation signal or observable in `GlobalSearchFindOptions` and propagate it to result providers. -That way `find` calls could be aborted. - -However result providers would still have to manually handles this signal to cancel any http call or -other asynchronous task that could be pending. - -Notes: -- As this can be implemented with an additional option, this would be a non-breaking change and so could be done at - a later time (even if it means that existing result provider would not handles this option until they implements it). - ## The GlobalSearch service could be provided as a plugin instead of a core service The GlobalSearch API could be provided and exposed from a plugin instead of `core`. From 5d750e072f4606634ff7e7c3c234c00fc0fe46ba Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 09:54:02 +0200 Subject: [PATCH 11/21] remove generic for GlobalSearchResponse, use distinct type on client and server --- rfcs/text/0011_global_search.md | 36 +++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 7671a74fe94ce..5dba62d5c92de 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -177,20 +177,6 @@ server one, as there is no `context` parameter on the `find` signature. ### Plugin API -#### common types - -```ts -/** - * Response returned from the {@link GlobalSearchServiceStart | global search service}'s `find` API - */ -type GlobalSearchResponse = { - /** - * Current results fetched from the providers. - */ - results: ResultType[]; -}; -``` - #### server API ```ts @@ -212,6 +198,16 @@ interface GlobalSearchFindOptions { aborted$?: Observable; } +/** + * Response returned from the server-side {@link GlobalSearchServiceStart | global search service}'s `find` API + */ +type GlobalSearchResponse = { + /** + * Current results fetched from the providers. + */ + results: GlobalSearchResult[]; +}; + /** @public */ interface GlobalSearchServiceSetup { registerResultProvider(provider: GlobalSearchResultProvider); @@ -253,6 +249,16 @@ interface NavigableGlobalSearchResult extends GlobalSearchResult { navigate: () => Promise; } +/** + * Response returned from the client-side {@link GlobalSearchServiceStart | global search service}'s `find` API + */ +type GlobalSearchResponse = { + /** + * Current results fetched from the providers. + */ + results: NavigableGlobalSearchResult[]; +}; + /** @public */ interface GlobalSearchServiceSetup { registerResultProvider(provider: GlobalSearchResultProvider); @@ -263,7 +269,7 @@ interface GlobalSearchServiceStart { find( term: string, options: GlobalSearchFindOptions, - ): Observable>; + ): Observable; } ``` From 24b1ec3bc4e7a1041f64f1c7a8ecc34ce60aed29 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 10:44:46 +0200 Subject: [PATCH 12/21] use plain string instead of enum for GlobalSearchResult.type --- rfcs/text/0011_global_search.md | 61 ++++++++------------------------- 1 file changed, 14 insertions(+), 47 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 5dba62d5c92de..d5ff87b8196e1 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -50,20 +50,16 @@ should still be generic to answer similar needs from any other consumer, either ```ts /** - * Static list of all the possible result types. - * Ordinal value matters here, as it will be used to sort results of different types. + * Static, non exhaustive list of the common search types. + * Only present to allow consumers and result providers to have aliases to the most common types. */ -enum SEARCH_TYPE { - // non-exhaustive - applications = 10, - dashboard = 20, - visualization = 30, - search = 40, +enum GlobalSearchCommonResultTypes { + application = 'application, + dashboard = 'dashboard', + visualization = 'visualization', + search = 'search', } -/** @public */ -type GlobalSearchResultType = keyof typeof SEARCH_TYPE; - /** * Options provided to {@link GlobalSearchResultProvider | result providers} `find` method. */ @@ -91,7 +87,7 @@ interface GlobalSearchResult { /** the title/label of the result */ title: string; /** the type of result */ - type: GlobalSearchResultType; + type: string; /** an optional EUI icon name to associate with the search result */ icon?: string; /** The url to navigate to this result. This can be either an absolute url, or a path relative to the performing request's basePath */ @@ -104,7 +100,7 @@ interface GlobalSearchResult { ``` Notes: -- The `Serializable` type should be implemented and exposed from `core`. A base implementation could be: +- The `Serializable` type should be implemented and exposed from `core`. A basic implementation could be: ```ts type Serializable = string | number | boolean | PrimitiveArray | PrimitiveRecord; @@ -531,13 +527,14 @@ could be added at a later time. On every emission of an underlying provider, the service will aggregate the new results with the existing list and sort the results following this logic before emitting them: -- Results will be sorted by ascending `type` ordinal value. -- Results of a same `type` will then be sorted by descending `score` value. +- Results will be sorted by descending `score` value. +- Results with the same score will then be sorted by ascending `id` value. This arbitrary sort is only performed to + ensure consistent order when multiple results have the exact same score This is an equivalent of the following lodash call: ```ts -const sorted = _.sortBy(unsorted, [r => SEARCH_TYPE[r.type], 'score'], ['asc', 'desc']); +const sorted = _.sortBy(unsorted, ['score', 'id'], ['desc', 'asc']); ``` For example, given this list of unsorted results: @@ -557,10 +554,10 @@ the resulting sorted results would be: ```ts const sorted = [ { id: 'app-1', type: 'application', score: 100 }, + { id: 'viz-1', type: 'visualization', score: 100 }, { id: 'app-1', type: 'application', score: 50 }, { id: 'dash-1', type: 'dashboard', score: 50 }, { id: 'dash-2', type: 'dashboard', score: 25 }, - { id: 'viz-1', type: 'visualization', score: 100 }, ]; ``` @@ -661,36 +658,6 @@ But this had some caveats: So even if the 'parsable absolute url' approach seems fragile, it still felt better than this alternative. -## We could use plain string instead of an enum for `GlobalSearchResult.type` - -The current static enum used for type - -```ts -enum SEARCH_TYPE { - // non-exhaustive - applications = 10, - dashboard = 20, - visualization = 30, - search = 40, -} - -/** @public */ -type GlobalSearchResultType = keyof typeof SEARCH_TYPE; - -interface GlobalSearchResult { - // [...] - type: GlobalSearchResultType; -} -``` - -has some limitations: -- It forces the enum to be modified every time a new type is added -- 3rd party plugins cannot introduce new types - -We could change the API to accept plain strings for `GlobalSearchResult.type`. However, atm this enum approach -is needed as the ordinal values of the entries is used in results sorting. Changing to plain strings forces to find -an alternative sorting logic. - ## The GlobalSearch service could be provided as a plugin instead of a core service The GlobalSearch API could be provided and exposed from a plugin instead of `core`. From 15ece401dd8bdf8c20828bfc496aa42827883731 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 10:49:02 +0200 Subject: [PATCH 13/21] remove terminology from unresolved questions --- rfcs/text/0011_global_search.md | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index d5ff87b8196e1..22d7917fcf768 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -694,12 +694,6 @@ example plugins. # Unresolved questions -## terminology / naming - -Are the current types, services and api names acceptable: - - `GlobalSearch` ts prefix - - `core.globalSearch` / `GlobalSearchService` - - `GlobalSearchResultProvider` - - `core.globalSearch.find` +N/A \ No newline at end of file From 0987e0dce2df0a1b3af1d4132c831e3cf498b533 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Thu, 30 Apr 2020 11:21:15 +0200 Subject: [PATCH 14/21] update pros/cons of core vs plugin --- rfcs/text/0011_global_search.md | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 22d7917fcf768..6efd0f0dc9e6a 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -663,18 +663,28 @@ So even if the 'parsable absolute url' approach seems fragile, it still felt bet The GlobalSearch API could be provided and exposed from a plugin instead of `core`. Pros: -- Less `core` API exposure +- Reduced `core` API exposure. +- Could leverage other plugins APIs (such as `bfetch` and yet-to-come `KibanaURL` - if not provided by core), which would + not be possible from within `core`. +- The consensus seems to be that this API is not a 'base' or 'core' API, so it probably make sense to have it in a plugin. +- `core` is by nature only under OSS license. If some or all the features of `GS` needs to be under the ES license, it cannot be in core. + If only parts of `GS` should be OSS, we could imagine an OSS plugin for exposing the GS API, and xpack plugin(s) to register the licensed + result providers. Cons: -- We know our initial consumer of this API is going to use it from/for the chrome header component, and because this component - is in `core`, that would mean creating a bridge of some kind to be able to use the service from core, as plugin APIs - are not usable here. -- The platform team is going to provide the base result providers for SO and application search results, so having the GS - service in a plugin would mean creating yet another plugin for these providers instead of having them self contained - in core. -- We are probably going to be adding internal SO APIs for the SO result provider, as current APIs are not sufficient to - search for multiple type of SO at the same time. It would be better if these APIs were kept internal, which would - not be possible if the SO provider is in a plugin instead of core. +- We know that our initial consumer of this API is going to be the `searchbar` header component developed by the `core-ui` team. + As the header is currently in `core`, and as plugin APIs are not usable from within `core`, having the GS API in + a plugin would mean additional development of some kind of bridge API to allow a plugin to register to the `chrome` service + the component that will be providing results to the search bar. Also, as the `searchbar` would not be able to directly + import / use the GS `NavigableGlobalSearchResult` type (which would be in a plugin, so not importable within core), + there would be a question regarding type compatibility between the `GS` result types, and the actual result types used + from within core in the searchbar. Duplicating the type is always an option, even if something that would ideally be avoided. + One alternative to that would be to provide expose an API in the chrome service to allow registering a `MountPoint` that + would be used to populate the `searchbar` placeholder in the header. That way, the searchbar implementation could live + in a plugin and use the GS API directly. +- As our current SO `find` API may not be sufficient to answer the SO result provider needs, we might be adding specific + SO API(s) for it. If such APIs were to be added, it would be preferable to keep them internal, which + is not possible if the SO result provider is outside of `core`. # Adoption strategy From 8180c3af0369acbc4b29b1809bf3f72f2093d568 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 09:24:10 +0200 Subject: [PATCH 15/21] GS is exposed from a plugin instead of core --- rfcs/text/0011_global_search.md | 62 ++++++++++----------------------- 1 file changed, 18 insertions(+), 44 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 6efd0f0dc9e6a..cec25bac73675 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -4,15 +4,17 @@ # Summary -A Kibana API exposed on both public and server side, allowing consumers to search for various objects and +A new Kibana plugin exposing an API on both public and server side, to allow consumers to search for various objects and register result providers. +Note: whether this will be an oss or xpack plugin still depends on https://github.com/elastic/dev/issues/1404. + # Basic example - registering a result provider: ```ts -coreSetup.globalSearch.registerResultProvider({ +setupDeps.globalSearch.registerResultProvider({ id: 'my_provider', find: (term, options, context) => { const resultPromise = myService.search(term, context.core.savedObjects.client); @@ -24,7 +26,7 @@ coreSetup.globalSearch.registerResultProvider({ - using the `find` API from the client-side: ```ts -coreStart.globalSearch.find('some term').subscribe(({ results }) => { +startDeps.globalSearch.find('some term').subscribe(({ results }) => { updateResults(results); }, () => {}, () => { showAsyncSearchIndicator(false); @@ -205,12 +207,12 @@ type GlobalSearchResponse = { }; /** @public */ -interface GlobalSearchServiceSetup { +interface GlobalSearchPluginSetup { registerResultProvider(provider: GlobalSearchResultProvider); } /** @public */ -interface GlobalSearchServiceStart { +interface GlobalSearchPluginStart { find( term: string, options: GlobalSearchFindOptions, @@ -256,12 +258,12 @@ type GlobalSearchResponse = { }; /** @public */ -interface GlobalSearchServiceSetup { +interface GlobalSearchPluginSetup { registerResultProvider(provider: GlobalSearchResultProvider); } /** @public */ -interface GlobalSearchServiceStart { +interface GlobalSearchPluginStart { find( term: string, options: GlobalSearchFindOptions, @@ -280,7 +282,7 @@ non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section #### http API -An internal HTTP API will be exposed on `/internal/core/global_search/find` to allow the client-side `GlobalSearchService` +An internal HTTP API will be exposed on `/internal/global_search/find` to allow the client-side `GlobalSearch` plugin to fetch results from the server-side result providers. It should be very close to: @@ -288,7 +290,7 @@ It should be very close to: ```ts router.post( { - path: '/api/core/global_search/find', + path: '/internal/global_search/find', validate: { body: schema.object({ term: schema.string(), @@ -300,7 +302,7 @@ router.post( }, async (ctx, req, res) => { const { term, options } = req.body; - const results = await ctx.core.globalSearch + const results = await ctx.globalSearch .find(term, { ...options, $aborted: req.events.aborted$ }) .pipe(last()) .toPromise(); @@ -316,7 +318,7 @@ router.post( Notes: - This API is only for internal use and communication between the client and the server parts of the `GS` API. When the need to expose an API for external consumers will appear, a new public API will be exposed for that. -- A new `globalSearch` service will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. +- A new `globalSearch` context will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. - Initial implementation will await for all results and then return them as a single response. As it's supported by the service, it could theoretically be possible to stream the results instead, however that makes the consumption of the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` @@ -326,10 +328,10 @@ the need to expose an API for external consumers will appear, a new public API w ### summary -- `coreSetup.globalService` exposes an API to be able to register result providers (`GlobalSearchResultProvider`). +- the `GlobalSearch` plugin setup contract exposes an API to be able to register result providers (`GlobalSearchResultProvider`). These providers can be registered from either public or server side, even if the interface for each side is not exactly the same. -- `coreStart.globalService` exposes an API to be able to search for objects. This API is available from both public +- the `GlobalSearch` plugin start contract exposes an API to be able to search for objects. This API is available from both public and server sides. - When using the server `find` API, only results from providers registered from the server will be returned. - When using the public `find` API, results from provider registered from both server and public sides will be returned. @@ -344,7 +346,7 @@ the need to expose an API for external consumers will appear, a new public API w Due to the fact that some kind of results (i.e `application`, and maybe later `management_section`) only exists on the public side of Kibana and therefor are not known on the server side, the `registerResultProvider` API will be -available both from the public and the server counterpart of the `GlobalSearchService`. +available both from the public and the server counterpart of the `GlobalSearchPluginSetup` contract. However, as results from providers registered from the client-side will not be available from the server's `find` API, registering result providers from the client should only be done to answer this specific use case and will be @@ -443,7 +445,7 @@ Otherwise: do a full page navigation (`window.location.assign()`) ### searching from the server side -When calling `GlobalSearchServiceStart.find` from the server-side service: +When calling `GlobalSearchPluginStart.find` from the server-side service: - the service will call `find` on each server-side registered result provider and collect the resulting result observables @@ -479,7 +481,7 @@ search( ### searching from the client side -When calling `GlobalSearchServiceStart.find` from the public-side service: +When calling `GlobalSearchPluginStart.find` from the public-side service: - The service will call: - the server-side API via an http call to fetch results from the server-side result providers @@ -658,34 +660,6 @@ But this had some caveats: So even if the 'parsable absolute url' approach seems fragile, it still felt better than this alternative. -## The GlobalSearch service could be provided as a plugin instead of a core service - -The GlobalSearch API could be provided and exposed from a plugin instead of `core`. - -Pros: -- Reduced `core` API exposure. -- Could leverage other plugins APIs (such as `bfetch` and yet-to-come `KibanaURL` - if not provided by core), which would - not be possible from within `core`. -- The consensus seems to be that this API is not a 'base' or 'core' API, so it probably make sense to have it in a plugin. -- `core` is by nature only under OSS license. If some or all the features of `GS` needs to be under the ES license, it cannot be in core. - If only parts of `GS` should be OSS, we could imagine an OSS plugin for exposing the GS API, and xpack plugin(s) to register the licensed - result providers. - -Cons: -- We know that our initial consumer of this API is going to be the `searchbar` header component developed by the `core-ui` team. - As the header is currently in `core`, and as plugin APIs are not usable from within `core`, having the GS API in - a plugin would mean additional development of some kind of bridge API to allow a plugin to register to the `chrome` service - the component that will be providing results to the search bar. Also, as the `searchbar` would not be able to directly - import / use the GS `NavigableGlobalSearchResult` type (which would be in a plugin, so not importable within core), - there would be a question regarding type compatibility between the `GS` result types, and the actual result types used - from within core in the searchbar. Duplicating the type is always an option, even if something that would ideally be avoided. - One alternative to that would be to provide expose an API in the chrome service to allow registering a `MountPoint` that - would be used to populate the `searchbar` placeholder in the header. That way, the searchbar implementation could live - in a plugin and use the GS API directly. -- As our current SO `find` API may not be sufficient to answer the SO result provider needs, we might be adding specific - SO API(s) for it. If such APIs were to be added, it would be preferable to keep them internal, which - is not possible if the SO result provider is outside of `core`. - # Adoption strategy The `globalSearch` service is a new feature provided by the `core` API. Also, the base providers From aed714fd8664ae56ae9e46878e4bdd2c6f05ec23 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 09:43:39 +0200 Subject: [PATCH 16/21] remove the server.publicAddress proposal, return mixed abs url / rel paths instead --- rfcs/text/0011_global_search.md | 90 ++++++--------------------------- 1 file changed, 16 insertions(+), 74 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index cec25bac73675..030456244c2ff 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -355,93 +355,35 @@ be used when it is not technically possible to register it from the server side ### results url processing -When retrieving results from providers, the GS service will always start by processing them. The most notable (and -currently only) step is to convert the result url to an absolute one. +When retrieving results from providers, the GS service will convert them from the provider's `GlobalSearchProviderResult` +result type to `GlobalSeachResult`, which is the structure returned from the `GlobalSearchPluginStart.find` observable. -#### absolute url conversion logic +In current specification, the only conversion step is to transform the `result.url` property following this logic: -Results returned from the GlobalSearch's `find` programmatic and HTTP APIs will all contains absolute urls for the following -reasons: -- It would not be usable by external consumers otherwise. -- Some result providers are supposed to be returning results from outside of the current `basePath`, or even from outside of the - Kibana instance (ie: results from another space, results from another Kibana instance in a cloud cluster) +- if `url` is an absolute url, it will not be modified +- if `url` is a relative path, the basePath will be prepended using `basePath.prepend` +- if `url` is a `{ path: string; prependBasePath: boolean }` structure: + - if `prependBasePath` is true, the basePath will be prepended to the given `path` using `basePath.prepend` + - if `prependBasePath` is false, the given `path` will be returned unmodified -However, as forging absolute urls can be a tedious process for the plugins, the `url` property of results returned by -a result provider can be either an absolute url or an url path relative to the executing request's `basePath`. +#### redirecting to a result -I.E are considered valid: -- `https://my-other-kibana-instance/some/result` -- `/app/kibana#/dashboard/some-id` +Parsing a relative or absolute result url to perform SPA navigation can be non trivial, and should remains the responsibility +of the GlobalSearch plugin API. -When processing the result, the logic regarding the `url` property is: -- if the `url` is absolute, return it unchanged -- if the `url` is a relative path (starts with `/`), it will be converted to an absolute url by prepending the Kibana -instance's newly introduced `publicAddress`. - -#### server.publicAddress - -Given the fact that some Kibana deployments have complex architecture (proxies, rewrite rules...), there is currently -no reliable way to know for sure what the public address used to access kibana is (at least from the server-side). - -A new `server.publicAddress` property will be added to the kibana configuration, allowing ops to explicitly define the public -address to instance is accessible from. This property is only meant to be used to generate 'links' to arbitrary resources -and will not be used to configure the http server in any way. - -When not explicitly defined, this property will be computed using the known `server` configuration values: - -```ts -const defaultPublicAddress = removeTrailingSlash( - `${getServerInfo().protocol}://${httpConfig.host}:${httpConfig.port}/${httpConfig.basePath}` -); - -const getPublicAddress = () => httpConfig.publicAddress ?? defaultPublicAddress; -``` - -A new `getAbsoluteUrl` api will also be added to the core `http` service contract: - -```ts -const getAbsoluteUrl = (path: string, request: KibanaRequest) => { - const publicUrl = getPublicAddress(); - const absoluteUrl = joinRemovingDuplicateAndTrailingSlash( - publicUrl, - // note: this is actually wrong. We would need the `requestScopePath` here - // as this currently returns `${this.serverBasePath}${requestScopePath}` and the basePath - // is already included in `publicAddress` - serverContract.basePath.get(request), - path - ); -} -``` - -Search results will then be processed before being returned to convert relative urls to absolute ones: - -```ts -const processResult(result: GlobalSearchResult, request: KibanaRequest) { - if(isUrlPath(result.url)) { - result.url = http.getAbsoluteUrl(result.url, request) - } -} -``` - -#### Redirecting to a result - -Having absolute urls in GS results is a necessity for external consumers, and makes the API more consistent than mixing -relative and absolute urls, however this makes it less trivial for UI consumers to redirect to a given result in a SPA -friendly way (using `application.navigateToApp` instead of triggering a full page refresh). - -This is why `NavigableGlobalSearchResult.navigate` has been introduced, to let `core` handle this navigation logic. +This is why `NavigableGlobalSearchResult.navigate` has been introduced on the client-side version of the `find` API When using `navigate` from a result instance, the following logic will be executed: -If all 3 of these criteria are true for `result.url`: -- The origin of the URL matches the origin of the `publicUrl` +If all these criteria are true for `result.url`: +- (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app// or any application's `appRoute` configuration) Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using -the remaining pathname segment +`application.navigateToApp` using the remaining pathname segment for the `path` option. -Otherwise: do a full page navigation (`window.location.assign()`) +Otherwise: do a full page navigation using `window.location.assign` ### searching from the server side From 9bf9231c090ea5ef322b7626efaa6063491b5399 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 09:59:58 +0200 Subject: [PATCH 17/21] distinguish result type between GS api and result providers --- rfcs/text/0011_global_search.md | 198 ++++++++++++++++---------------- 1 file changed, 101 insertions(+), 97 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 030456244c2ff..c26757638ca06 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -26,11 +26,15 @@ setupDeps.globalSearch.registerResultProvider({ - using the `find` API from the client-side: ```ts -startDeps.globalSearch.find('some term').subscribe(({ results }) => { +startDeps.globalSearch.find('some term').subscribe( + ({ results }) => { updateResults(results); -}, () => {}, () => { + }, + () => {}, + () => { showAsyncSearchIndicator(false); -}); + } +); ``` # Motivation @@ -83,7 +87,7 @@ interface GlobalSearchProviderFindOptions { /** * Representation of a result returned by a {@link GlobalSearchResultProvider | result provider} */ -interface GlobalSearchResult { +interface GlobalSearchProviderResult { /** an id that should be unique for an individual provider's results */ id: string; /** the title/label of the result */ @@ -92,8 +96,16 @@ interface GlobalSearchResult { type: string; /** an optional EUI icon name to associate with the search result */ icon?: string; - /** The url to navigate to this result. This can be either an absolute url, or a path relative to the performing request's basePath */ - url: string; + /** + * The url associated with this result. + * This can be either an absolute url, a path relative to the basePath, or a structure specifying if the basePath should be prepended. + * + * @example + * `result.url = 'https://kibana-instance:8080/base-path/app/my-app/my-result-type/id';` + * `result.url = '/app/my-app/my-result-type/id';` + * `result.url = { path: '/base-path/app/my-app/my-result-type/id', prependBasePath: false };` + */ + url: string | { path: string; prependBasePath: boolean }; /** the score of the result, from 1 (lowest) to 100 (highest) */ score: number; /** an optional record of metadata for this result */ @@ -102,6 +114,7 @@ interface GlobalSearchResult { ``` Notes: + - The `Serializable` type should be implemented and exposed from `core`. A basic implementation could be: ```ts @@ -143,16 +156,17 @@ type GlobalSearchResultProvider = { term: string, options: GlobalSearchProviderFindOptions, context: GlobalSearchProviderContext - ): Observable; + ): Observable; }; ``` -Notes: +Notes: + - initial implementation will only provide a static / non extensible `GlobalSearchProviderContext` context. -It would be possible to allow plugins to register their own context providers as it's done for `RequestHandlerContext`, -but this will not be done until the need arises. + It would be possible to allow plugins to register their own context providers as it's done for `RequestHandlerContext`, + but this will not be done until the need arises. - Because of the previous point, the performing `request` object is also exposed on the context to allow result providers -to scope their custom services if needed. + to scope their custom services if needed. #### public @@ -164,20 +178,32 @@ type GlobalSearchResultProvider = { id: string; find( term: string, - options: GlobalSearchProviderFindOptions, - ): Observable; + options: GlobalSearchProviderFindOptions + ): Observable; }; ``` -Notes: -- The client-side version of `GlobalSearchResultProvider` is slightly different than the -server one, as there is no `context` parameter on the `find` signature. +Notes: + +- The client-side version of `GlobalSearchResultProvider` is slightly different than the + server one, as there is no `context` parameter on the `find` signature. ### Plugin API #### server API ```ts +/** + * Representation of a result returned by the {@link GlobalSearchPluginStart.find | `find` API} + */ +type GlobalSearchResult = Omit & { + /** + * The url associated with this result. + * This can be either an absolute url, or a relative path including the basePath + */ + url: string; +}; + /** * Options for the server-side {@link GlobalSearchServiceStart.find | find API} */ @@ -241,7 +267,7 @@ interface GlobalSearchFindOptions { */ interface NavigableGlobalSearchResult extends GlobalSearchResult { /** - * Navigate to this result's associated url. If the result is on this kibana instance, user will be redirected to it + * Navigate to this result's associated url. If the result is on this kibana instance, user will be redirected to it * in a SPA friendly way using `application.navigateToApp`, else, a full page refresh will be performed. */ navigate: () => Promise; @@ -264,25 +290,23 @@ interface GlobalSearchPluginSetup { /** @public */ interface GlobalSearchPluginStart { - find( - term: string, - options: GlobalSearchFindOptions, - ): Observable; + find(term: string, options: GlobalSearchFindOptions): Observable; } ``` -Notes: +Notes: + - The public API is very similar to its server counterpart. The differences are: - - The `registerResultProvider` setup APIs share the same signature, however the input `GlobalSearchResultProvider` - types are different on the client and server. + - The `registerResultProvider` setup APIs share the same signature, however the input `GlobalSearchResultProvider` + types are different on the client and server. - The `find` start API signature got a `KibanaRequest` for `server`, when this parameter is not present for `public`. -- The `find` API returns a observable of `NavigableGlobalSearchResult` instead of plain `GlobalSearchResult`. This type -is here to enhance results with a `navigate` method to let `core` handle the navigation logic, which is -non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section for more info. +- The `find` API returns a observable of `NavigableGlobalSearchResult` instead of plain `GlobalSearchResult`. This type + is here to enhance results with a `navigate` method to let the `GlobalSearch` plugin handle the navigation logic, which is + non-trivial. See the [Redirecting to a result](#redirecting-to-a-result) section for more info. #### http API -An internal HTTP API will be exposed on `/internal/global_search/find` to allow the client-side `GlobalSearch` plugin +An internal HTTP API will be exposed on `/internal/global_search/find` to allow the client-side `GlobalSearch` plugin to fetch results from the server-side result providers. It should be very close to: @@ -294,9 +318,11 @@ router.post( validate: { body: schema.object({ term: schema.string(), - options: schema.maybe(schema.object({ - preference: schema.maybe(schema.string()), - })), + options: schema.maybe( + schema.object({ + preference: schema.maybe(schema.string()), + }) + ), }), }, }, @@ -315,25 +341,26 @@ router.post( ); ``` -Notes: +Notes: + - This API is only for internal use and communication between the client and the server parts of the `GS` API. When -the need to expose an API for external consumers will appear, a new public API will be exposed for that. + the need to expose an API for external consumers will appear, a new public API will be exposed for that. - A new `globalSearch` context will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. -- Initial implementation will await for all results and then return them as a single response. As it's supported by +- Initial implementation will await for all results and then return them as a single response. As it's supported by the service, it could theoretically be possible to stream the results instead, however that makes the consumption of - the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` + the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` endpoint could be added. - + ## Functional behavior ### summary -- the `GlobalSearch` plugin setup contract exposes an API to be able to register result providers (`GlobalSearchResultProvider`). - These providers can be registered from either public or server side, even if the interface for each side is not +- the `GlobalSearch` plugin setup contract exposes an API to be able to register result providers (`GlobalSearchResultProvider`). + These providers can be registered from either public or server side, even if the interface for each side is not exactly the same. - the `GlobalSearch` plugin start contract exposes an API to be able to search for objects. This API is available from both public - and server sides. - - When using the server `find` API, only results from providers registered from the server will be returned. + and server sides. + - When using the server `find` API, only results from providers registered from the server will be returned. - When using the public `find` API, results from provider registered from both server and public sides will be returned. - During a `find` call, the service will call all the registered result providers and collect their result observables. Every time a result provider emits some new results, the `globalSearch` service will: @@ -344,12 +371,12 @@ the need to expose an API for external consumers will appear, a new public API w ### result provider registration -Due to the fact that some kind of results (i.e `application`, and maybe later `management_section`) only exists on -the public side of Kibana and therefor are not known on the server side, the `registerResultProvider` API will be +Due to the fact that some kind of results (i.e `application`, and maybe later `management_section`) only exists on +the public side of Kibana and therefor are not known on the server side, the `registerResultProvider` API will be available both from the public and the server counterpart of the `GlobalSearchPluginSetup` contract. -However, as results from providers registered from the client-side will not be available from the server's `find` API, -registering result providers from the client should only be done to answer this specific use case and will be +However, as results from providers registered from the client-side will not be available from the server's `find` API, +registering result providers from the client should only be done to answer this specific use case and will be discouraged, by providing appropriated jsdoc and documentation explaining that it should only be used when it is not technically possible to register it from the server side instead. @@ -376,11 +403,12 @@ This is why `NavigableGlobalSearchResult.navigate` has been introduced on the cl When using `navigate` from a result instance, the following logic will be executed: If all these criteria are true for `result.url`: + - (only for absolute URLs) The origin of the URL matches the origin of the browser's current location - The pathname of the URL starts with the current basePath (eg. /mybasepath/s/my-space) - The pathname segment after the basePath matches any known application route (eg. /app// or any application's `appRoute` configuration) -Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using +Then: match the pathname segment to the corresponding application and do the SPA navigation to that application using `application.navigateToApp` using the remaining pathname segment for the `path` option. Otherwise: do a full page navigation using `window.location.assign` @@ -392,11 +420,11 @@ When calling `GlobalSearchPluginStart.find` from the server-side service: - the service will call `find` on each server-side registered result provider and collect the resulting result observables - then, the service will merge every result observable and trigger the next step on every emission until either - - A predefined timeout duration is reached - - All result observables are completed - -- on every emission of the merged observable, the results will be aggregated to the existing list and sorted following -the logic defined in the [results aggregation](#results-aggregation) section + - A predefined timeout duration is reached + - All result observables are completed + +- on every emission of the merged observable, the results will be aggregated to the existing list and sorted following + the logic defined in the [results aggregation](#results-aggregation) section A very naive implementation of this behavior would be: @@ -432,9 +460,9 @@ When calling `GlobalSearchPluginStart.find` from the public-side service: - Then, the service will merge every result observable and trigger the next step on every emission until either - A predefined timeout duration is reached - All result observables are completed - -- On every emission of the merged observable, the results will be aggregated to the existing list and sorted following -the logic defined in the [results aggregation](#results-aggregation) section + +- On every emission of the merged observable, the results will be aggregated to the existing list and sorted following + the logic defined in the [results aggregation](#results-aggregation) section A very naive implementation of this behavior would be: @@ -460,19 +488,20 @@ search( } ``` -Notes: +Notes: + - Due to the complexity of the process, the initial implementation will not be streaming results from the server, -meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' -http call to the GS endpoint). The observable-based API architecture is ready for this however, and the enhancement -could be added at a later time. + meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' + http call to the GS endpoint). The observable-based API architecture is ready for this however, and the enhancement + could be added at a later time. ### results aggregation -On every emission of an underlying provider, the service will aggregate the new results with the existing list and +On every emission of an underlying provider, the service will aggregate the new results with the existing list and sort the results following this logic before emitting them: - Results will be sorted by descending `score` value. -- Results with the same score will then be sorted by ascending `id` value. This arbitrary sort is only performed to +- Results with the same score will then be sorted by ascending `id` value. This arbitrary sort is only performed to ensure consistent order when multiple results have the exact same score This is an equivalent of the following lodash call: @@ -515,7 +544,7 @@ How this field is populated from each individual provider is considered an imple ### Search cancellation -Consumers can cancel a `find` call at any time by providing a cancellation observable with +Consumers can cancel a `find` call at any time by providing a cancellation observable with the `GlobalSearchFindOptions.aborted$` option and then emitting from it. When this observable is provided and emitting, the GS service will complete the result observable. @@ -536,71 +565,48 @@ while making these results not available from the server-side and HTTP APIs. We could decide to only allow providers registration from the server-side. It would reduce API exposure, while simplifying the service implementation. However to do that, we would need to find a solution to be able to implement a server-side - result provider for `application` (and later `management_section`) type provider. +result provider for `application` (and later `management_section`) type provider. -I will directly exclude the option to move the `application` registration (`core.application.register`) from client +I will directly exclude the option to move the `application` registration (`core.application.register`) from client to server-side, as it's a very heavy impacting (and breaking) change to `core` APIs that would requires more reasons than just this RFC/API to consider. ### AST parsing One option to make the `application` results 'visible' from the server-side would be to parse the client code at build time -using AST to find all usages to `application.register` inspect the parameters, and generates a server file -containing the applications. The server-side `application` result provider would then just read this file and uses it +using AST to find all usages to `application.register` inspect the parameters, and generates a server file +containing the applications. The server-side `application` result provider would then just read this file and uses it to return application results. However + - At the parsing would be done at build time, we would not be able to generate entries for any 3rd party plugins - As entries for every existing applications would be generated, the search provider would to be able to know which -applications are actually enabled/accessible at runtime to filter them, which is all but easy + applications are actually enabled/accessible at runtime to filter them, which is all but easy - It will also not contains test plugin apps, making it really hard to FTR - AST parsing is a complex mechanism for an already unsatisfactory alternative - -### Duplicated server-side `application.register` API - -One other option would be to duplicate the `application.register` API on the server side, with a subset of the + +### Duplicated server-side `application.register` API + +One other option would be to duplicate the `application.register` API on the server side, with a subset of the client-side metadata. ```ts core.application.register({ id: 'app_status', title: 'App Status', - euiIconType: 'snowflake' + euiIconType: 'snowflake', }); ``` This way, the applications could be searchable from the server using this server-side `applications` registry. However + - It forces plugin developers to add this API call. In addition to be a very poor developer experience, it can also very easily be forgotten, making a given app non searchable - client-side only plugins would need to add a server-side part to their plugin just to register their application on the server side - -## `GlobalSearchResult.url` could be a struct instead of a url for internal results. - -The initial idea for `GlobalSearchResult.url` was to have a polymorphic structure instead of just a string: - -```ts -url: { absUrl: string } | { application: string; path?: string }; -``` - -That was making it way easier to redirect to an internal result from the UI, as we could directly call -`application.navigateToApp(application, { path })`. - -However, that didn't bring answer for the (future) need to be able to search for and redirect to object in -different spaces. We could then have changed the structure to - -```ts -url: { absUrl: string } | { application: string; path?: string, space?: string }; -``` - -But this had some caveats: -- `space` is an xpack plugin, adding this `space` property in an oss part of the code is problematic -- The `space` API is not callable from core or oss, meaning that we would have to 'forge' the url to this space anyway -- this is really not generic. If another plugin was to alter the basepath in another way, we would have needed to add it another property - -So even if the 'parsable absolute url' approach seems fragile, it still felt better than this alternative. # Adoption strategy @@ -621,5 +627,3 @@ example plugins. # Unresolved questions N/A - - \ No newline at end of file From d2289f79e44ac6d042a049ee307395b25df5913b Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 14:24:55 +0200 Subject: [PATCH 18/21] return batched results, no longer sort --- rfcs/text/0011_global_search.md | 96 ++++++++++----------------------- 1 file changed, 27 insertions(+), 69 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index c26757638ca06..ceb2f0e828102 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -225,9 +225,9 @@ interface GlobalSearchFindOptions { /** * Response returned from the server-side {@link GlobalSearchServiceStart | global search service}'s `find` API */ -type GlobalSearchResponse = { +type GlobalSearchBatchedResults = { /** - * Current results fetched from the providers. + * Results for this batch */ results: GlobalSearchResult[]; }; @@ -243,7 +243,7 @@ interface GlobalSearchPluginStart { term: string, options: GlobalSearchFindOptions, request: KibanaRequest - ): Observable; + ): Observable; } ``` @@ -276,9 +276,9 @@ interface NavigableGlobalSearchResult extends GlobalSearchResult { /** * Response returned from the client-side {@link GlobalSearchServiceStart | global search service}'s `find` API */ -type GlobalSearchResponse = { +type GlobalSearchBatchedResults = { /** - * Current results fetched from the providers. + * Results for this batch */ results: NavigableGlobalSearchResult[]; }; @@ -290,7 +290,7 @@ interface GlobalSearchPluginSetup { /** @public */ interface GlobalSearchPluginStart { - find(term: string, options: GlobalSearchFindOptions): Observable; + find(term: string, options: GlobalSearchFindOptions): Observable; } ``` @@ -330,7 +330,7 @@ router.post( const { term, options } = req.body; const results = await ctx.globalSearch .find(term, { ...options, $aborted: req.events.aborted$ }) - .pipe(last()) + .pipe(reduce((acc, results) => [...acc, ...results])) .toPromise(); return res.ok({ body: { @@ -346,10 +346,8 @@ Notes: - This API is only for internal use and communication between the client and the server parts of the `GS` API. When the need to expose an API for external consumers will appear, a new public API will be exposed for that. - A new `globalSearch` context will be exposed on core's `RequestHandlerContext` to wrap a `find` call with current request. -- Initial implementation will await for all results and then return them as a single response. As it's supported by - the service, it could theoretically be possible to stream the results instead, however that makes the consumption of - the API from the client more difficult. If this become important at some point, a new `/api/core/global_search/find/async` - endpoint could be added. +- Example implementation is awaiting for all results and then returns them as a single response. Ideally, we would + leverage the `bfetch` plugin to stream the results to the client instead. ## Functional behavior @@ -364,10 +362,8 @@ Notes: - When using the public `find` API, results from provider registered from both server and public sides will be returned. - During a `find` call, the service will call all the registered result providers and collect their result observables. Every time a result provider emits some new results, the `globalSearch` service will: - - Consolidate/enhance them - - Merge them with the already present results - - Sort and order the new aggregated results - - Emit this up to date list of results + - process them to convert their url to the expected output format + - emit the processed results ### result provider registration @@ -422,9 +418,8 @@ When calling `GlobalSearchPluginStart.find` from the server-side service: - then, the service will merge every result observable and trigger the next step on every emission until either - A predefined timeout duration is reached - All result observables are completed - -- on every emission of the merged observable, the results will be aggregated to the existing list and sorted following - the logic defined in the [results aggregation](#results-aggregation) section + +- on every emission of the merged observable, the results will be processed then emitted. A very naive implementation of this behavior would be: @@ -438,12 +433,10 @@ search( const fromProviders$ = this.providers.map(p => p.find(term, { ...options, aborted$ }, contextFromRequest(request)) ); - const results: GlobalSearchResult[] = []; return merge([...fromProviders$]).pipe( takeUntil(aborted$), map(newResults => { - results.push(...newResults); - return order(results); + return process(newResults); }), ); } @@ -454,15 +447,16 @@ search( When calling `GlobalSearchPluginStart.find` from the public-side service: - The service will call: + - the server-side API via an http call to fetch results from the server-side result providers - `find` on each client-side registered result provider and collect the resulting observables - Then, the service will merge every result observable and trigger the next step on every emission until either + - A predefined timeout duration is reached - All result observables are completed - -- On every emission of the merged observable, the results will be aggregated to the existing list and sorted following - the logic defined in the [results aggregation](#results-aggregation) section + +- on every emission of the merged observable, the results will be processed then emitted. A very naive implementation of this behavior would be: @@ -476,13 +470,10 @@ search( p.find(term, { ...options, aborted$ }) ); const fromServer$ = of(this.fetchServerResults(term, options, aborted$)) - - const results: GlobalSearchResult[] = []; return merge([...fromProviders$, fromServer$]).pipe( takeUntil(aborted$), map(newResults => { - results.push(...newResults); - return order(results); + return process(newResults); }), ); } @@ -490,49 +481,16 @@ search( Notes: -- Due to the complexity of the process, the initial implementation will not be streaming results from the server, - meaning that all results from server-side registered providers will all be fetched at the same time (via a 'classic' - http call to the GS endpoint). The observable-based API architecture is ready for this however, and the enhancement - could be added at a later time. - -### results aggregation - -On every emission of an underlying provider, the service will aggregate the new results with the existing list and -sort the results following this logic before emitting them: - -- Results will be sorted by descending `score` value. -- Results with the same score will then be sorted by ascending `id` value. This arbitrary sort is only performed to - ensure consistent order when multiple results have the exact same score - -This is an equivalent of the following lodash call: +- The example implementation is not streaming results from the server, meaning that all results from server-side + registered providers will all be fetched and emitted in a single batch. Ideally, we would leverage the `bfetch` plugin + to stream the results to the client instead. -```ts -const sorted = _.sortBy(unsorted, ['score', 'id'], ['desc', 'asc']); -``` - -For example, given this list of unsorted results: +### results sorting -```ts -const unsorted = [ - { id: 'viz-1', type: 'visualization', score: 100 }, - { id: 'dash-2', type: 'dashboard', score: 25 }, - { id: 'app-1', type: 'application', score: 50 }, - { id: 'dash-1', type: 'dashboard', score: 50 }, - { id: 'app-1', type: 'application', score: 100 }, -]; -``` - -the resulting sorted results would be: - -```ts -const sorted = [ - { id: 'app-1', type: 'application', score: 100 }, - { id: 'viz-1', type: 'visualization', score: 100 }, - { id: 'app-1', type: 'application', score: 50 }, - { id: 'dash-1', type: 'dashboard', score: 50 }, - { id: 'dash-2', type: 'dashboard', score: 25 }, -]; -``` +As the GS `find` API is 'streaming' the results from the result providers by emitting the results in batches, sorting results in +each individual batch, even if technically possible, wouldn't provide much value as the consumer will need to sort the +aggregated results on each emission anyway. This is why the results emitted by the `find` API should be considered as +unsorted. Consumers should implement sorting themselves, using either the `score` attribute, or any other arbitrary logic. #### Note on score value From 66d6db9f9cbdfce04647dd47b9fc02f55966e69d Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 14:27:31 +0200 Subject: [PATCH 19/21] remove request from GlobalSearchProviderContext --- rfcs/text/0011_global_search.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index ceb2f0e828102..34a840db0238e 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -130,7 +130,6 @@ interface PrimitiveRecord extends Record {} * Context passed to server-side {@GlobalSearchResultProvider | result provider}'s `find` method. */ export interface GlobalSearchProviderContext { - request: KibanaRequest; core: { savedObjects: { client: SavedObjectsClientContract; @@ -162,11 +161,11 @@ type GlobalSearchResultProvider = { Notes: -- initial implementation will only provide a static / non extensible `GlobalSearchProviderContext` context. +- Initial implementation will only provide a static / non extensible `GlobalSearchProviderContext` context. It would be possible to allow plugins to register their own context providers as it's done for `RequestHandlerContext`, but this will not be done until the need arises. -- Because of the previous point, the performing `request` object is also exposed on the context to allow result providers - to scope their custom services if needed. +- The performing `request` object could also be exposed on the context to allow result providers + to scope their custom services if needed. However as the previous option, this should only be done once needed. #### public From 3de606aa047a272d426ecfefa8d50ce44bb9978b Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Fri, 8 May 2020 14:32:35 +0200 Subject: [PATCH 20/21] add maxResults to GlobalSearchProviderFindOptions --- rfcs/text/0011_global_search.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 34a840db0238e..593ed3a0939bb 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -82,6 +82,11 @@ interface GlobalSearchProviderFindOptions { * this can (and should) be used to cancel any pending asynchronous task and complete the result observable. */ aborted$: Observable; + /** + * The total maximum number of results (including all batches / emissions) that should be returned by the provider for a given `find` request. + * Any result emitted exceeding this quota will be ignored by the service and not emitted to the consumer. + */ + maxResults: number; } /** From fc6bf132978672232648907288a37d4427914b20 Mon Sep 17 00:00:00 2001 From: pgayvallet Date: Mon, 11 May 2020 08:33:44 +0200 Subject: [PATCH 21/21] nit/typo --- rfcs/text/0011_global_search.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/text/0011_global_search.md b/rfcs/text/0011_global_search.md index 593ed3a0939bb..5ec368a1c2f02 100644 --- a/rfcs/text/0011_global_search.md +++ b/rfcs/text/0011_global_search.md @@ -60,7 +60,7 @@ should still be generic to answer similar needs from any other consumer, either * Only present to allow consumers and result providers to have aliases to the most common types. */ enum GlobalSearchCommonResultTypes { - application = 'application, + application = 'application', dashboard = 'dashboard', visualization = 'visualization', search = 'search',