[data / saved query] BWCA all the routes #158790

mattkime · 2023-05-31T23:34:19Z

Summary

All routes

Use versioned router
Moved to internal path
Validate responses
All responses are typed with response types, separate from internal api types. This is to help prevent unacknowledged changes to the api.
Version is included in all requests

mattkime · 2023-06-03T04:13:28Z

src/plugins/data/server/query/route_types.ts

+ * These types are used to define the shape of the response from the saved query API
+ */
+
+// @ts-expect-error


I don't know why this is necessary - this type is essentially copied from MatchAllFilterMeta where it works.

Bizarrely, this seems to be caused by Filter being an interface instead of a type. It looks like that's why packages/kbn-es-query/src/filters/build_filters/types.ts uses a type with // eslint-disable-next-line @typescript-eslint/consistent-type-definitions instead:

kibana/packages/kbn-es-query/src/filters/build_filters/types.ts

Lines 87 to 94 in ef524bb

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions

export type Filter = {

$state?: {

store: FilterStateStore;

};

meta: FilterMeta;

query?: Record<string, any>;

};

I have no idea why the compiler doesn't like this though 🤔

I'm mostly confused because I made sure my copied Filter is also a type and yet the compiler still complains.

elasticmachine · 2023-06-03T05:06:40Z

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

davismcphee

The changes are looking good overall, just a couple of validation issues to fix up and a few comments.

src/plugins/data/server/query/routes.ts

davismcphee · 2023-06-06T23:58:08Z

src/plugins/data/server/query/route_types.ts

+ * These types are used to define the shape of the response from the saved query API
+ */
+
+// @ts-expect-error


Bizarrely, this seems to be caused by Filter being an interface instead of a type. It looks like that's why packages/kbn-es-query/src/filters/build_filters/types.ts uses a type with // eslint-disable-next-line @typescript-eslint/consistent-type-definitions instead:

kibana/packages/kbn-es-query/src/filters/build_filters/types.ts

Lines 87 to 94 in ef524bb

// eslint-disable-next-line @typescript-eslint/consistent-type-definitions

export type Filter = {

$state?: {

store: FilterStateStore;

};

meta: FilterMeta;

query?: Record<string, any>;

};

I have no idea why the compiler doesn't like this though 🤔

src/plugins/data/server/query/route_types.ts

src/plugins/data/server/query/routes.ts

…kime/kibana into saved_query_better_route_definition

mattkime · 2023-06-13T17:20:50Z

@davismcphee I overlooked this for a few days, its ready for another looks.

davismcphee

Tested again locally and all is working as expected now 👍 thanks for the changes, LGTM!

stratoula

Changes LGTM!

kertal · 2023-06-19T12:59:10Z

@elasticmachine merge upstream

logeekal

Looks great 🚀 and works well as well. Left a comment about something I was not sure.

logeekal · 2023-06-19T13:13:01Z

x-pack/plugins/security_solution/cypress/tasks/api_calls/saved_queries.ts

@@ -15,7 +17,7 @@ export const createSavedQuery = (
 ) =>
  rootRequest<SavedQuery>({
    method: 'POST',
-    url: '/api/saved_query/_create',
+    url: `${SAVED_QUERY_BASE_URL}/_create`,


I am not sure so I am writing here for your consideration if you think it makes sense for security solution to use createQuery function instead of calling this API directly.

Although, if we go down that way, createQuery would need to expose version as well so that users can easily pass it.

Either the REST api should be used or the plugin api contract. This looks like a test setup - REST api usage looks fine to me but your team might also decide something different.

kibana-ci · 2023-06-19T14:34:58Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 5f43361

Failed CI Steps

Test Failures

[job] [logs] Security Solution Tests #2 / Detection response view Open in timeline "after each" hook for "opens timeline with correct query count for hosts by alert severity table"
[job] [logs] Investigations - Security Solution Tests #2 / Detection response view Open in timeline "after each" hook for "opens timeline with correct query count for open alerts by rule table"
[job] [logs] Security Solution Tests #2 / Detection response view Open in timeline opens timeline with correct query count for hosts by alert severity table
[job] [logs] Investigations - Security Solution Tests #2 / Detection response view Open in timeline opens timeline with correct query count for open alerts by rule table

Metrics [docs]

Page load bundle

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

id	before	after	diff
`data`	404.7KB	404.8KB	+89.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`data`	52	54	+2
`enterpriseSearch`	13	15	+2
`securitySolution`	411	415	+4
total			+8

Total ESLint disabled count

id	before	after	diff
`data`	54	56	+2
`enterpriseSearch`	14	16	+2
`securitySolution`	494	498	+4
total			+8

History

💛 Build #135582 was flaky 86eca58
💔 Build #135451 failed 941fd24
💛 Build #135199 was flaky 728f3cb
💚 Build #134952 succeeded 277881a
💛 Build #134827 was flaky edbf2bf
💛 Build #134785 was flaky 1f54546

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

cc @mattkime

mattkime added 4 commits May 31, 2023 18:33

add versioning to all routes

a678e2e

create response validation

bb027c3

Merge branch 'main' into saved_query_better_route_definition

ef6a77b

response schema

d614259

mattkime changed the title ~~add versioning to all routes~~ [data / saved query] BWCA all the routes Jun 3, 2023

mattkime added release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jun 3, 2023

mattkime self-assigned this Jun 3, 2023

mattkime added 3 commits June 2, 2023 21:27

move some config to vars

9d5748f

add response types

de81382

pass version

53823dd

mattkime commented Jun 3, 2023

View reviewed changes

fix jest test

cf0dfde

mattkime marked this pull request as ready for review June 3, 2023 05:06

mattkime requested review from a team as code owners June 3, 2023 05:06

Merge branch 'main' into saved_query_better_route_definition

24584aa

kertal mentioned this pull request Jun 5, 2023

[Saved query] Routes must satisfy bwca #157105

Closed

1 task

davismcphee reviewed Jun 7, 2023

View reviewed changes

mattkime added 5 commits June 6, 2023 22:53

misc fixes

5702fb5

Merge branch 'main' into saved_query_better_route_definition

cbbba19

Merge branch 'main' into saved_query_better_route_definition

9df8d63

Merge branch 'saved_query_better_route_definition' of github.com:matt…

f7371a5

…kime/kibana into saved_query_better_route_definition

add versioning to security solution test request

a6f631a

mattkime requested a review from a team as a code owner June 13, 2023 10:08

mattkime added 2 commits June 13, 2023 08:27

fix version header

1f54546

better schema name

edbf2bf

mattkime mentioned this pull request Jun 13, 2023

Ensure saved queries use content-management and are not loaded through the saved object client. #158574

Closed

mattkime added 2 commits June 13, 2023 13:46

change route to internal

277881a

Merge branch 'main' into saved_query_better_route_definition

728f3cb

davismcphee approved these changes Jun 15, 2023

View reviewed changes

Merge branch 'main' into saved_query_better_route_definition

941fd24

stratoula approved these changes Jun 15, 2023

View reviewed changes

Merge branch 'main' into saved_query_better_route_definition

86eca58

Merge branch 'main' into saved_query_better_route_definition

5f43361

logeekal approved these changes Jun 19, 2023

View reviewed changes

mattkime enabled auto-merge (squash) June 19, 2023 13:47

mattkime merged commit 9e74fcf into elastic:main Jun 19, 2023

kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data / saved query] BWCA all the routes #158790

[data / saved query] BWCA all the routes #158790

mattkime commented May 31, 2023 •

edited

Loading

mattkime Jun 3, 2023

davismcphee Jun 6, 2023

mattkime Jun 7, 2023

elasticmachine commented Jun 3, 2023

davismcphee left a comment

davismcphee Jun 6, 2023

mattkime commented Jun 13, 2023

davismcphee left a comment

stratoula left a comment

kertal commented Jun 19, 2023

logeekal left a comment

logeekal Jun 19, 2023

mattkime Jun 19, 2023

kibana-ci commented Jun 19, 2023

ESLint disabled line counts

Total ESLint disabled count

	// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
	export type Filter = {
	$state?: {
	store: FilterStateStore;
	};
	meta: FilterMeta;
	query?: Record<string, any>;
	};

[data / saved query] BWCA all the routes #158790

[data / saved query] BWCA all the routes #158790

Conversation

mattkime commented May 31, 2023 • edited Loading

Summary

mattkime Jun 3, 2023

Choose a reason for hiding this comment

davismcphee Jun 6, 2023

Choose a reason for hiding this comment

mattkime Jun 7, 2023

Choose a reason for hiding this comment

elasticmachine commented Jun 3, 2023

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Jun 6, 2023

Choose a reason for hiding this comment

mattkime commented Jun 13, 2023

davismcphee left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

kertal commented Jun 19, 2023

logeekal left a comment

Choose a reason for hiding this comment

logeekal Jun 19, 2023

Choose a reason for hiding this comment

mattkime Jun 19, 2023

Choose a reason for hiding this comment

kibana-ci commented Jun 19, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

mattkime commented May 31, 2023 •

edited

Loading