Improve SearchSource SearchRequest type #186862

lukasolson · 2024-06-24T23:55:14Z

Summary

Improves the SearchRequest type exported from the data plugin. Prior to this PR, it was just a Record<string, any>. This is just one step in moving toward the correct type.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

lukasolson · 2024-06-24T23:55:33Z

/ci

lukasolson · 2024-06-25T15:57:17Z

/ci

lukasolson · 2024-06-25T19:19:09Z

/ci

elasticmachine · 2024-06-25T22:32:12Z

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

paul-tavares

Changes to files owned by elastic/security-defend-workflows look good 👍

…kibana into search_source_search_request

src/plugins/data/common/search/search_source/fetch/types.ts

davismcphee · 2024-07-10T19:55:59Z

@mattkime Requesting a review from you on this one since you've been working in SearchSource lately and are more familiar with the code.

mattkime · 2024-07-10T21:35:41Z

src/plugins/data/common/search/search_source/fetch/get_search_params.ts

@@ -37,12 +37,15 @@ export function getSearchParamsFromRequest(
  const searchParams = getSearchParams(getConfig);
  // eslint-disable-next-line @typescript-eslint/naming-convention
  const { track_total_hits, ...body } = searchRequest.body;
+  const dataView = typeof searchRequest.index !== 'string' ? searchRequest.index : undefined;
+  const index = dataView?.title ?? `${searchRequest.index}`;


It appears as though the template literal isn't needed. might leave a note in the code if it is doing something useful

searchRequest.index is optional and I was trying to get around the type checks while preserving the existing behavior here - that it will be treated as "undefined".

It might be more useful here to actually throw some sort of error instead - I think it is a required parameter here. I'll dig into it a little more.

mattkime · 2024-07-10T21:40:01Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -1157,14 +1166,15 @@ export class SearchSource {
  toExpressionAst({ asDatatable = true }: ExpressionAstOptions = {}): ExpressionAstExpression {
    const searchRequest = this.mergeProps();
    const { body, index, query } = searchRequest;
+    const dataView = typeof index !== 'string' ? index : undefined;


This check on index as to whether its a string or a data view happens enough that it might be abstracted into a function. I'll leave it up to you

Good point, I'll add this

mattkime · 2024-07-10T21:40:59Z

src/plugins/vis_types/vega/public/data_model/search_api.ts

@@ -72,7 +72,7 @@ export class SearchAPI {
        });

        return from(
-          extendSearchParamsWithRuntimeFields(indexPatterns, requestParams, request.index)
+          extendSearchParamsWithRuntimeFields(indexPatterns, requestParams, `${request.index}`)


Why the template literal?

Same as above

mattkime

I made some minor observations, I'll let you decide if you care to act upon them. Otherwise nice improvements that work well!

…kibana into search_source_search_request

lukasolson · 2024-07-17T21:52:24Z

/ci

kibana-ci · 2024-07-22T20:38:49Z

💚 Build Succeeded

Buildkite Build
Commit: 140b4a6

Metrics [docs]

Async chunks

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

id	before	after	diff
`visTypeVega`	1.8MB	1.8MB	+5.0B

Page load bundle

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

id	before	after	diff
`data`	417.7KB	418.1KB	+476.0B

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`data`	28	29	+1

History

💚 Build #218069 succeeded ba8353a
💚 Build #217746 succeeded 894c72c
💔 Build #217709 failed be1bc30
💔 Build #217555 failed 6b778cb

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

cc @lukasolson

dej611 · 2024-07-23T12:38:17Z

src/plugins/data/common/search/search_source/search_source.ts

@@ -1190,7 +1204,7 @@ export class SearchSource {
        buildExpressionFunction<EsdslExpressionFunctionDefinition>('esdsl', {
          size: body?.size,
          dsl: JSON.stringify({}),
-          index: index?.id,
+          index: typeof index === 'string' ? index : `${dataView?.id}`,


Just one question here: if dataView is undefined the index value would be set to "undefined". Is that ok?

dej611

Code review only.
Left a single code question.
Approve to unblock it.

* master: (3487 commits) `BedrockChat` & `GeminiChat` (elastic#186809) [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332) skip flaky suite (elastic#188997) [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580) [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608) [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943) Improve SearchSource SearchRequest type (elastic#186862) Deprecate Search Sessions config (elastic#188037) [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824) [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746) [Data Forge] Add `service.logs` dataset as a data stream (elastic#188786) [Console] Fix failing bulk requests (elastic#188552) Update dependency terser to ^5.31.2 (main) (elastic#188528) [APM][ECO] Telemetry (elastic#188627) [Fleet] Fix uninstall package validation accross space (elastic#188749) Update warning on `xpack.fleet.enableExperimental` (elastic#188917) [DOCS][Cases] Automate more screenshots for cases (elastic#188697) [Fleet] Fix get one agent when feature flag disabled (elastic#188953) chore(investigate): Add investigate-app plugin from poc (elastic#188122) [Monaco Editor] Add Search functionality (elastic#188337) ...

* master: (2400 commits) `BedrockChat` & `GeminiChat` (elastic#186809) [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332) skip flaky suite (elastic#188997) [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580) [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608) [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943) Improve SearchSource SearchRequest type (elastic#186862) Deprecate Search Sessions config (elastic#188037) [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824) [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746) [Data Forge] Add `service.logs` dataset as a data stream (elastic#188786) [Console] Fix failing bulk requests (elastic#188552) Update dependency terser to ^5.31.2 (main) (elastic#188528) [APM][ECO] Telemetry (elastic#188627) [Fleet] Fix uninstall package validation accross space (elastic#188749) Update warning on `xpack.fleet.enableExperimental` (elastic#188917) [DOCS][Cases] Automate more screenshots for cases (elastic#188697) [Fleet] Fix get one agent when feature flag disabled (elastic#188953) chore(investigate): Add investigate-app plugin from poc (elastic#188122) [Monaco Editor] Add Search functionality (elastic#188337) ...

Improve SearchSource SearchRequest type

6b778cb

lukasolson added Feature:Search Querying infrastructure in Kibana Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 24, 2024

lukasolson self-assigned this Jun 24, 2024

Fix data view mock

be1bc30

Update snapshots

894c72c

Merge branch 'main' into search_source_search_request

8372376

lukasolson marked this pull request as ready for review June 25, 2024 22:32

lukasolson requested review from a team as code owners June 25, 2024 22:32

lukasolson requested review from gergoabraham and paul-tavares June 25, 2024 22:32

lukasolson added the release_note:skip Skip the PR/issue when compiling release notes label Jun 25, 2024

sebelga approved these changes Jun 26, 2024

View reviewed changes

paul-tavares approved these changes Jun 26, 2024

View reviewed changes

lukasolson added 3 commits June 26, 2024 13:55

Update query_to_fields filters usage

f4bd2e9

Merge branch 'search_source_search_request' of github.com:lukasolson/…

c86d796

…kibana into search_source_search_request

Fix filters when they are functions

ba8353a

gergoabraham removed their request for review June 28, 2024 12:02

dej611 reviewed Jul 2, 2024

View reviewed changes

src/plugins/data/common/search/search_source/fetch/types.ts Outdated Show resolved Hide resolved

lukasolson added 2 commits July 8, 2024 15:55

Merge branch 'main' into search_source_search_request

5f41d2a

Review feedback & update one more reference to incorrect search request

6fd54cf

lukasolson requested a review from a team as a code owner July 8, 2024 23:30

Merge branch 'main' into search_source_search_request

f12ca4c

davismcphee requested a review from mattkime July 10, 2024 19:56

mattkime reviewed Jul 10, 2024

View reviewed changes

mattkime approved these changes Jul 10, 2024

View reviewed changes

lukasolson added 11 commits July 11, 2024 15:10

Merge branch 'main' into search_source_search_request

f8ef285

Review feedback

8097a77

Merge branch 'search_source_search_request' of github.com:lukasolson/…

411297f

…kibana into search_source_search_request

Merge branch 'main' into search_source_search_request

22fde50

Merge branch 'main' into search_source_search_request

704d5c0

Merge branch 'main' into search_source_search_request

0073ada

Merge branch 'main' into search_source_search_request

eb3e93b

Merge branch 'main' into search_source_search_request

02b31ad

Merge branch 'main' into search_source_search_request

c7fab54

Merge branch 'main' into search_source_search_request

e34304e

Merge branch 'search_source_search_request' of github.com:lukasolson/…

6ad68ab

…kibana into search_source_search_request

lukasolson requested a review from dej611 July 17, 2024 23:54

Merge branch 'main' into search_source_search_request

140b4a6

lukasolson removed the request for review from dej611 July 22, 2024 19:13

dej611 reviewed Jul 23, 2024

View reviewed changes

dej611 approved these changes Jul 23, 2024

View reviewed changes

lukasolson merged commit 375a699 into elastic:main Jul 23, 2024
42 checks passed

kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve SearchSource SearchRequest type #186862

Improve SearchSource SearchRequest type #186862

lukasolson commented Jun 24, 2024

lukasolson commented Jun 24, 2024

lukasolson commented Jun 25, 2024

lukasolson commented Jun 25, 2024

elasticmachine commented Jun 25, 2024

paul-tavares left a comment

davismcphee commented Jul 10, 2024

mattkime Jul 10, 2024 •

edited

Loading

lukasolson Jul 11, 2024

mattkime Jul 10, 2024

lukasolson Jul 11, 2024

mattkime Jul 10, 2024

lukasolson Jul 11, 2024

mattkime left a comment

lukasolson commented Jul 17, 2024

kibana-ci commented Jul 22, 2024

References to deprecated APIs

dej611 Jul 23, 2024

dej611 left a comment

Improve SearchSource SearchRequest type #186862

Improve SearchSource SearchRequest type #186862

Conversation

lukasolson commented Jun 24, 2024

Summary

Checklist

Risk Matrix

For maintainers

lukasolson commented Jun 24, 2024

lukasolson commented Jun 25, 2024

lukasolson commented Jun 25, 2024

elasticmachine commented Jun 25, 2024

paul-tavares left a comment

Choose a reason for hiding this comment

davismcphee commented Jul 10, 2024

mattkime Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

lukasolson Jul 11, 2024

Choose a reason for hiding this comment

mattkime Jul 10, 2024

Choose a reason for hiding this comment

lukasolson Jul 11, 2024

Choose a reason for hiding this comment

mattkime Jul 10, 2024

Choose a reason for hiding this comment

lukasolson Jul 11, 2024

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

lukasolson commented Jul 17, 2024

kibana-ci commented Jul 22, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

References to deprecated APIs

History

dej611 Jul 23, 2024

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

mattkime Jul 10, 2024 •

edited

Loading