[HTTP] Versioned router implementation #153543

jloleysens · 2023-03-23T13:12:09Z

Summary

Implements the designs from #151596

Move packages/versioning/* into packages/core/http to follow existing structure more closely
Implements the first iteration of the versioned router as a wrapper/layer around the existing router
Adds some integration tests
Future work needed! Once we have a the versioned spec we should implement it in this wrapper layer
Validation is a little bit tricky because of when the CoreKibanaResponse object is instantiated, the approach taken here is to replace body, params, query on the route-level's request object

Closes #149286

Next steps:

Expose this versioned router on the IRouter interface
Pass through flag for dev-only checks
Implement the client side changes

…level config, was in "options"

…tests and functionality though

packages/core/http/core-http-router-server-internal/src/request.ts

...ges/core/versioning/core-versioned-http-server-internal/src/internal_versioned_route.test.ts

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_router.ts

jloleysens · 2023-03-23T13:18:47Z

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_route.ts

+// This validation is a pass-through so that we can apply our version-specific validation later
+const passThroughValidation = { body: schema.any(), params: schema.any(), query: schema.any() };
+
+export class InternalVersionedRoute implements VersionedRoute {


I expect most of the code in here will be implementing the actual API specification and making sure versioned routes adhere to it.

jloleysens · 2023-03-23T13:19:31Z

packages/core/versioning/core-versioned-http-server/src/types.ts

- *
- * @experimental
- */
-export interface VersionHTTPToolkit {


I decided to remove the concept of "Toolkit" as I don't think it adds anything.

packages/core/versioning/core-versioned-http-server/src/types.ts

…fix'

pgayvallet

Nothing major to report, but a bunch of NITs, remarks and questions

packages/core/versioning/core-versioned-http-server/src/types.ts

packages/core/versioning/core-versioned-http-server-internal/src/types.ts

pgayvallet · 2023-03-23T15:08:25Z

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_route.ts

+    if (this.validateResponses && handler.options.validate && handler.options.validate.response) {
+      const { response } = handler.options.validate;
+      try {
+        validate(
+          req,
+          { body: response.body, unsafe: { body: response.unsafe } },
+          handler.options.version
+        );
+      } catch (e) {
+        return res.custom({
+          statusCode: 500,
+          body: `Failed output validation: ${e.message}`,
+        });
+      }
+    }


:oh_no_you_didnt:

😄 gotta love structural typing

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_route.ts

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_router.ts

.github/CODEOWNERS

packages/core/versioning/core-versioned-http-server-internal/src/internal_versioned_router.ts

elasticmachine · 2023-03-23T17:15:57Z

Pinging @elastic/kibana-core (Team:Core)

jloleysens · 2023-03-24T15:39:17Z

packages/core/http/core-http-versioned-router-server-internal/src/core_versioned_route.ts

+
+    const validation = handler.options.validate || undefined;
+
+    const mutableCoreKibanaRequest = req as Mutable<CoreKibanaRequest>;


(moving comment from previous location)

This is so that we can hide/remove unvalidated inputs after CoreKibanaRequest was constructed. Not super happy about this, but it seemed like the least trouble without doing one of:

(1) Exposing more knowledge of Router internals so that we can dynamically set validation schema (right now we just use IRouter)
(2) Changing our approach, integrating the versioned router functionality into the Router implementation more directly

jloleysens · 2023-03-24T17:13:04Z

packages/core/http/core-http-server/src/versioning/types.ts

 }

 /** @experimental */
 export type RequestValidation<P, Q, B> = RouteValidatorFullConfig<P, Q, B>;

 /** @experimental */
 export interface ResponseValidation<R> {
-  body: RouteValidationFunction<R> | Type<R>;
+  [statusCode: number]: { body: RouteValidationFunction<R> | Type<R> };


Following OAS's example here. For the responses we added validation for specific status codes. At the moment this is used to scope validation to a specific status code, but we could go further and throw if an unknown status code was provided.

Note: output validation is intended to run in dev only.

…t validation defined

pgayvallet

Implementation looks good to me.

What are the next steps / what are we missing to effectively expose this feature to our consumers via Core APIs?

pgayvallet · 2023-03-28T06:10:58Z

packages/core/http/core-http-server/index.ts

+export type {
+  AddVersionOpts,
+  RequestValidation,
+  ResponseValidation,
+  Version,
+  VersionedRoute,
+  VersionedRouteConfig,
+  VersionedRouteRegistrar,
+  VersionedRouter,
+} from './src/versioning';


NIT: I wonder if we shouldn't add a more explicit prefix to some of those types, just to make it more understandable that they're bound to the versioned router. Mostly thinking about:

Version

RequestValidation

ResponseValidation

Maybe (I know, those are long names...):

RouteVersion (or ApiVersion?)

VersionedRouteRequestValidation

VersionedRouteResponseValidation

pgayvallet · 2023-03-28T06:12:04Z

packages/core/http/core-http-server/src/versioning/example.ts

-const router = {} as unknown as IRouter<MyCustomContext>;
-
-const versionedRouter = vtk.createVersionedRouter({ router });
+const versionedRouter = {} as unknown as VersionedRouter<MyCustomContext>;


Now that we have an implementation, should we get rid of this file, or convert it to documentation or TSDoc?

pgayvallet · 2023-03-28T06:15:57Z

packages/core/http/core-http-versioned-router-server-internal/src/core_versioned_route.ts

+    // TODO: Make "true" dev-only
+    private readonly validateResponses: boolean = true


Is it a TODO for now or a TODO for later 😄?

I was planning on addressing this when exposing the versioned router on our http router instance :thu:

jloleysens · 2023-03-28T08:59:37Z

What are the next steps / what are we missing to effectively expose this feature to our consumers via Core APIs?

I'll update the PR description with this info @pgayvallet

kibana-ci · 2023-03-28T10:11:37Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 7abb5da

Failed CI Steps

FTR Configs #12

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-http-server`	161	176	+15
`@kbn/core-http-versioned-router-server-internal`	-	15	+15
total			+30

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-http-versioned-router-server-internal`	-	2	+2

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-server`	413	439	+26
`@kbn/core-http-versioned-router-server-internal`	-	15	+15
total			+41

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💚 Build #115593 succeeded 0cf0319
💚 Build #115556 succeeded ff4c755
💚 Build #115473 succeeded 4ebba29
💚 Build #115389 succeeded 991984d
💚 Build #115361 succeeded ae1a918
💚 Build #115127 succeeded 91d54bf

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

afharo

What a great effort! This is exciting! LGTM!

I added 2 comments for your consideration. But feel free to tackle them in the upcoming follow ups.

afharo · 2023-03-28T10:14:59Z

packages/core/http/core-http-versioned-router-server-internal/src/core_versioned_route.ts

+  AddVersionOpts,
+  VersionedRoute,
+  VersionedRouteConfig,
+} from '@kbn/core-http-server';


Super-nit: 2 imports from the same package can be merged

afharo · 2023-03-28T10:25:01Z

packages/core/http/core-http-versioned-router-server-internal/src/core_versioned_route.ts

+          validation.request,
+          handler.options.version
+        );
+        mutableCoreKibanaRequest.body = body;


I think this will also mutate req. Do we want to use clone?

## Summary Now that we merged #153543, this PR exposes the versioned router for teams to start using. The versioned router will be available on `IRouter` under a new `versioned` property. Primary benefit of this approach is that plugin developers will not need to do anything other than "get" the `versioned` property to get a versioned router. Drawback is that this precludes us from passing in additional configuration, like a version, to scope the versioned router instance. For that we would need some kind of `createVersionedRouter({ version: ... })`. At this point it is not clear this is necessary, we could revisit this decision based on actual usage. Plugin developers could also do something like: ```ts // common const const MY_API_VERSION: ApiVersion = '1'; // in routes import {MY_API_VERSION} from '../from/common'; router.versioned.get({ path: ... }) .addVersion({ version: MY_API_VERSION }); ``` In this way they could get many of the same benefits of a version-scoped version router, with the drawback that they need to pass this in for every route. ### TODO - [x] Add an integration test for the versioned router ### Future work * We still need to consider revisiting some of the router design to better support internal cases like adding support for registering a handler for a version range and adding a default version to continue supporting on-prem where introducing versions will be a breaking change --------- Co-authored-by: kibanamachine <[email protected]>

jloleysens added 14 commits March 22, 2023 13:48

move types to "core-versioned-http-server"

30c31e1

rename "version" to "versioned" in all files

52aff31

export types

e5a80bc

remove the concept of versioned http toolkit

1e88fac

remove concept of versioned toolkit from code

1d28474

added getRoutes function and moved required "access" property to top-…

b191455

…level config, was in "options"

Added VersionedRouterRoute type

3b74e08

one small step for versioned http

d720fdf

added some check for "not found" 406 responses

2124cae

move to "from" pattern for internal versioned router

5a76452

update index exports

dc6b41b

update index exports

1890a96

updated types a bit more

51f63db

got first iteration of the versioned router working, need a lot more …

256a47f

…tests and functionality though

jloleysens added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes Epic:VersionedAPIs Kibana Versioned APIs v8.8.0 labels Mar 23, 2023

jloleysens commented Mar 23, 2023

View reviewed changes

jloleysens and others added 4 commits March 23, 2023 14:23

ran yarn kbn bootstrap

78a9e59

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

02f7eef

…fix'

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

a2601b2

remove export of non-existent types

929b3ef

pgayvallet reviewed Mar 23, 2023

View reviewed changes

jloleysens added 6 commits March 23, 2023 17:41

moved types to core-http-server, bye bye boilerplate

7422334

move implementation to core/http

9a751f2

run yarn kbn bootstrap and moved some internal types to internal package

f9241f6

remove unhelpful comment

879575b

remove unnecessary type cast

f76be86

fix jest config and rename Internal... to Core...

c83fdea

jloleysens requested a review from a team as a code owner March 23, 2023 17:15

jloleysens added 3 commits March 23, 2023 18:29

added some more tests

2bbc777

updated test message

91d54bf

clean making the core request mutable

82a2147

jloleysens commented Mar 24, 2023

View reviewed changes

jloleysens and others added 3 commits March 24, 2023 16:40

Merge branch 'main' into versioned-router-impl

3e79036

add "type" to type only import

ae1a918

added status code to body result

991984d

jloleysens commented Mar 24, 2023

View reviewed changes

jloleysens and others added 6 commits March 24, 2023 23:06

Pass in the mutated request

e7f3b6d

improve if statement to run validation if there is at least some inpu…

d0363a3

…t validation defined

use mutable kibana request

4ebba29

Merge branch 'main' into versioned-router-impl

ff4c755

slight update to types

2970540

added internal comment and updated test

0cf0319

pgayvallet approved these changes Mar 28, 2023

View reviewed changes

jloleysens and others added 3 commits March 28, 2023 10:50

Merge branch 'main' into versioned-router-impl

9e2e542

remove example.ts file, move to doc comment

9ae403e

refactor names of some versioning specific types

abb110d

jloleysens enabled auto-merge (squash) March 28, 2023 08:59

fix ts issue

7abb5da

jloleysens merged commit e8055e8 into elastic:main Mar 28, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 28, 2023

afharo approved these changes Mar 28, 2023

View reviewed changes

jloleysens deleted the versioned-router-impl branch March 28, 2023 10:35

jloleysens mentioned this pull request Mar 28, 2023

[HTTP] Expose versioned router #153858

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP] Versioned router implementation #153543

[HTTP] Versioned router implementation #153543

jloleysens commented Mar 23, 2023 •

edited

Loading

jloleysens Mar 23, 2023

jloleysens Mar 23, 2023

pgayvallet left a comment

pgayvallet Mar 23, 2023

jloleysens Mar 23, 2023

elasticmachine commented Mar 23, 2023

jloleysens Mar 24, 2023

jloleysens Mar 24, 2023 •

edited

Loading

pgayvallet left a comment

pgayvallet Mar 28, 2023

pgayvallet Mar 28, 2023

pgayvallet Mar 28, 2023

jloleysens Mar 28, 2023

jloleysens commented Mar 28, 2023

kibana-ci commented Mar 28, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

afharo left a comment

afharo Mar 28, 2023

afharo Mar 28, 2023


		const validation = handler.options.validate \|\| undefined;

		const mutableCoreKibanaRequest = req as Mutable<CoreKibanaRequest>;

		// TODO: Make "true" dev-only
		private readonly validateResponses: boolean = true

[HTTP] Versioned router implementation #153543

[HTTP] Versioned router implementation #153543

Conversation

jloleysens commented Mar 23, 2023 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 23, 2023

Choose a reason for hiding this comment

jloleysens Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Mar 28, 2023

kibana-ci commented Mar 28, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloleysens commented Mar 23, 2023 •

edited

Loading

jloleysens Mar 24, 2023 •

edited

Loading