[data views] Use versioned router for REST routes by mattkime · Pull Request #158608 · elastic/kibana

mattkime · 2023-05-27T03:12:24Z

Summary

Version alllll the data view routes.

Best viewed with whitespace hidden - https://github.com/elastic/kibana/pull/158608/files?diff=unified&w=1

In this PR:

All REST (public and internal) routes are versioned
Internal routes are called with version specified
Internal and public routes are now stored in directories labeled as such
All routes have a response schema
All responses are typed with response types, separate from internal api types. This is to help prevent unacknowledged changes to the api.
Moves some functional tests from js => ts

For follow up PRs:

Move to internal path for internal routes
Proper typing and schema for fields_for_wildcard filter

Closes #157099
Closes #157100

elasticmachine · 2023-05-28T02:14:21Z

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

jughosta

Looking good! Have some questions about the scope:

Is it required or not to pass INITIAL_REST_VERSION or 1 where that API is called?
Is it required or not to type API responses?

src/plugins/data_views/server/rest_api_routes/default_data_view.ts

jughosta · 2023-05-30T07:27:34Z

src/plugins/data_views/server/rest_api_routes/has_user_data_view.ts

      {
-        path,
-        validate: {},
+        version: '1', // INITIAL_REST_VERSION,


Suggested change

version: '1', // INITIAL_REST_VERSION,

version: '1',

jughosta · 2023-05-30T07:29:41Z

src/plugins/data_views/server/routes/fields_for.ts


 const path = '/api/index_patterns/_fields_for_wildcard';
+const version = '1';
+const access = 'internal';


Is it okay to keep /api in the path if it's an internal route?

I think ideally all internal routes should be prefixed with /internal, but I don't think we included this one in the audit either.

cc @thomasneirynck looks like /api/index_patterns/_fields_for_wildcard is another candidate for making internal.

…ana into data_views_versioned_router

…w.ts Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

…ana into data_views_versioned_router

davismcphee

Did another round of reviewing and testing. The latest code changes seem good to me, but I'm still running into some issues hitting some endpoints.

The data view update endpoint returns 500:

I'm still getting a strange response from the data view delete endpoint:

And the update runtime field endpoint returns 400:

davismcphee · 2023-06-07T01:17:02Z

src/plugins/data_views/server/rest_api_routes/internal/fields_for.ts

+  timeSeriesDimension: schema.maybe(schema.boolean()),
+});
+
+const validate: FullValidationConfig<any, any, any> = {


Just curious what the types marked any are for?

export interface RouteValidatorConfig<P, Q, B> { /** * Validation logic for the URL params * @public */ params?: RouteValidationSpec<P>; /** * Validation logic for the Query params * @public */ query?: RouteValidationSpec<Q>; /** * Validation logic for the body payload * @public */ body?: RouteValidationSpec<B>; }

...I could probably do a better job using these

mattkime · 2023-06-07T04:20:08Z

@davismcphee I must have fixed those and then unfixed them. At any rate, they should be working now. One item -

And the update runtime field endpoint returns 400:

The problem is a lack of type value. As best I can tell that poor error message is a bug in the schema validation error messages. Supply "type": "keyword", and it should work.

mattkime · 2023-06-07T05:19:11Z

test/api_integration/apis/data_views/data_views_crud/delete_data_view/main.ts

@@ -41,21 +41,6 @@ export default function ({ getService }: FtrProviderContext) {

          expect(response4.status).to.be(404);
        });


I removed this test because there was a weird relationship between it at and the versioned router. The versioned router would provide a weird message even though the task completed successfully and this is the circumstance under which this test would pass. Now the api returns no body and I'm no longer testing for its emptiness - which wasn't registering as emptiness in this test for some reason.

What was it registering as instead?

res.ok() on the sender side results in an empty object for the body

davismcphee

@davismcphee I must have fixed those and then unfixed them. At any rate, they should be working now. One item -

Tested again and looks like the issues are fixed now 👍 thanks! Just a couple of questions to answer first, then this will be good to merge on my end.

And the update runtime field endpoint returns 400:

The problem is a lack of type value. As best I can tell that poor error message is a bug in the schema validation error messages. Supply "type": "keyword", and it should work.

Yup, looks like that fixed it, thanks! Although the docs I pulled this example from show an update without the type prop: https://www.elastic.co/guide/en/kibana/master/data-views-runtime-field-api-update.html#data-views-runtime-field-update-example. Is it the docs that are wrong and need to be updated or did we used to allow updates without specifying the type?

davismcphee · 2023-06-12T02:21:59Z

test/api_integration/apis/data_views/data_views_crud/delete_data_view/main.ts

@@ -41,21 +41,6 @@ export default function ({ getService }: FtrProviderContext) {

          expect(response4.status).to.be(404);
        });


What was it registering as instead?

mattkime · 2023-06-12T04:32:11Z

What was it registering as instead?

An empty object - I restored the test and it now checks for an empty object instead of an empty value.

mattkime · 2023-06-12T04:37:07Z

@davismcphee You're right and the docs are right - I added validation that didn't allow updating a runtime field without specifying the type. I'll address this.

kibana-ci · 2023-06-12T23:13:52Z

💚 Build Succeeded

Buildkite Build
Commit: ed710db

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
`dataViews`	44.6KB	44.6KB	+38.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server`	444	447	+3

ESLint disabled in files

id	before	after	diff
`dataViews`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	410	414	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`dataViews`	13	14	+1
`enterpriseSearch`	20	22	+2
`securitySolution`	491	495	+4
total			+7

History

💚 Build #134569 succeeded d7698ba
💔 Build #134550 failed 83dc781
💔 Build #134533 failed ceda6a4
💔 Build #134497 failed d325913
💔 Build #134205 failed f49f8cd
💚 Build #133166 succeeded ff77f04

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

cc @mattkime

mattkime · 2023-06-12T23:39:45Z

@davismcphee I added a test for partial runtime field updates and fixed the corresponding schema issues.

davismcphee

Latest code changes look good and the update runtime field endpoint works without type now. Thanks for updating/adding tests too! LGTM 👍

* main: (199 commits) [Lens] Add custom formatter within the Lens editor (elastic#158468) [Uptime] Hide app if no data is available (elastic#159118) [CodeEditor] Add grok highlighting (elastic#159334) Expose decoded cloudId components from the cloud plugin's contract (elastic#159442) [Profiling] Use collector and symbolizer integrations in the add data page (elastic#159481) [Infrastructure UI] Hosts View: Unified Search bar with auto-refresh enabled (elastic#157011) [APM] Add feature flag for not available apm schema (elastic#158911) [Lens] Remove deprecated componentWillReceiveProps usage (elastic#159502) [api-docs] 2023-06-13 Daily api_docs build (elastic#159536) [data views] Use versioned router for REST routes (elastic#158608) [maps] fix geo line source not loaded unless maps application is opened (elastic#159432) [Enterprise Search][Search application]Fix Create Api key url (elastic#159519) [Security Solution] Increase timeout for indexing hosts (elastic#159518) dashboard Reset button disable (elastic#159430) [Security Solution] Unskip Endpoint API tests after package fix (elastic#159484) [Synthetics] adjust alert timing (elastic#159511) [ResponseOps][rule registry] Remove usages of `refresh: true` (elastic#159252) Revert "Remove unused package (elastic#158597)" [Serverless] Adding config to disable authentication on task manager background worker utilization API (elastic#159505) Remove unused package (elastic#158597) ...

…#159758) ## Summary Response validation is failing when there are field conflicts. Disable response validation until a complete schema can be created. Problem was introduced in #158608

mattkime added 4 commits May 26, 2023 21:59

version create data view

3d659b6

change version to date

29bbb0c

partial progress

bc46a64

version all rest routes

588e47c

mattkime changed the title ~~version create data view~~ [data views] Use versioned router for REST routes May 27, 2023

mattkime added 3 commits May 27, 2023 11:07

version all rest routes

c9100b5

fix internal version

96a43df

version alllll the things

3f436ba

mattkime self-assigned this May 28, 2023

mattkime added Feature:Data Views Data Views code and UI - index patterns before 8.0 Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// release_note:skip Skip the PR/issue when compiling release notes labels May 28, 2023

mattkime marked this pull request as ready for review May 28, 2023 02:14

mattkime requested a review from a team as a code owner May 28, 2023 02:14

mattkime added 5 commits May 27, 2023 21:51

fix fields_for_wildcard

0b64be4

fix test, create type problem

6426e9a

fix type

bcd8c30

remove comment

5f87326

Merge branch 'main' into data_views_versioned_router

cca761d

jughosta reviewed May 30, 2023

View reviewed changes

mattkime and others added 9 commits May 30, 2023 05:22

cleanup

594080b

Merge branch 'data_views_versioned_router' of github.com:mattkime/kib…

27a81f7

…ana into data_views_versioned_router

Update src/plugins/data_views/server/rest_api_routes/default_data_vie…

4483f98

…w.ts Co-authored-by: Julia Rechkunova <julia.rechkunova@gmail.com>

Merge branch 'main' into data_views_versioned_router

efef50c

add response validation to create data view

068cf4c

Merge branch 'data_views_versioned_router' of github.com:mattkime/kib…

a905277

…ana into data_views_versioned_router

add response validation to create data view

0c71ac0

response schemas and stronger typing for REST endpoints

41029cf

response schemas and stronger typing for REST endpoints

1ccb4de

davismcphee reviewed Jun 7, 2023

View reviewed changes

api fixes

cce8311

remove unneeded test

ff77f04

mattkime commented Jun 7, 2023

View reviewed changes

davismcphee reviewed Jun 12, 2023

View reviewed changes

mattkime added 2 commits June 11, 2023 22:39

Merge branch 'main' into data_views_versioned_router

2b2bc05

restore test, check for empty body

f49f8cd

mattkime added 6 commits June 12, 2023 12:36

fix api functional tests

d325913

functional test fixes

ceda6a4

move some functional tests to ts

6b282b4

fix functional tests

83dc781

fix filter functional test

d7698ba

add test for runtime partial update

ed710db

davismcphee approved these changes Jun 13, 2023

View reviewed changes

mattkime merged commit 26d4ba5 into elastic:main Jun 13, 2023

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

mattkime mentioned this pull request Jun 13, 2023

[IndexPattern] routes must satisfy bwca #157108

Closed

7 tasks

mattkime mentioned this pull request Jun 14, 2023

[data views] fields_for_wildcard - disable schema response validation #159758

Merged

jeramysoucy mentioned this pull request Jun 22, 2023

Conform Security HTTP APIs to Versioned HTTP API roadmap #158334

Open

14 tasks

		@@ -41,21 +41,6 @@ export default function ({ getService }: FtrProviderContext) {

		expect(response4.status).to.be(404);
		});

Conversation

mattkime commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented May 28, 2023

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkime commented Jun 12, 2023

Uh oh!

mattkime commented Jun 12, 2023

Uh oh!

kibana-ci commented Jun 12, 2023

💚 Build Succeeded

Metrics [docs]

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

mattkime commented Jun 12, 2023

Uh oh!

davismcphee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattkime commented May 27, 2023 •

edited

Loading

mattkime Jun 7, 2023 •

edited

Loading