[HTTP] Expose versioned router #153858

jloleysens · 2023-03-28T11:35:10Z

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:

// 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

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

elasticmachine · 2023-03-28T11:35:14Z

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

… version via header and actually pass version via header

… case the header

…fix'

TinaHeiligers · 2023-04-02T06:48:04Z

src/core/server/integration_tests/http/versioned_router.test.ts

+    ).resolves.toEqual(
+      expect.objectContaining({
+        message:
+          'Version expected at [get] [/my-path]. Please specify a version using the "elastic-api-version" header. Available versions are: "1,2"',


Oh so polite 😉

TinaHeiligers · 2023-04-02T07:00:39Z

packages/core/http/core-http-versioned-router-server-mocks/README.md

@@ -0,0 +1,3 @@
+# @kbn/core-http-versioned-router-server-mocks
+
+This package contains the mocks and test utils for core's server-side `http` service


nit:

Suggested change

This package contains the mocks and test utils for core's server-side `http` service

This package contains the mocks and test utils for core's server-side `http` versioned router server.

TinaHeiligers

Great work!
I added a couple of comments but nothing that blocks merging.

TinaHeiligers · 2023-04-02T07:27:14Z

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

+ * Side Public License, v 1.
+ */
+
+export function isValidRouteVersion(version: string): boolean {


Validation won't prevent duplicate versions from being defined. Should we consider adding an option to ensure that each version defined is unique or are we ok with leaving that up to consumers?
Ok, nevermind, we do

TinaHeiligers · 2023-04-02T07:45:07Z

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

@@ -13,10 +13,19 @@ import { Method, VersionedRouterRoute } from './types';

 export class CoreVersionedRouter implements VersionedRouter {
  private readonly routes = new Set<CoreVersionedRoute>();
-  public static from({ router }: { router: IRouter }) {
-    return new CoreVersionedRouter(router);
+  public static from({


Sneaky...I get where the inspiration comes from 😉

TinaHeiligers · 2023-04-02T08:00:57Z

packages/core/http/core-http-router-server-mocks/src/router.mock.ts

@@ -34,6 +35,7 @@ function createRouterMock({ routerPath = '' }: { routerPath?: string } = {}): Ro
    patch: jest.fn(),
    getRoutes: jest.fn(),
    handleLegacyErrors: jest.fn().mockImplementation((handler) => handler),
+    versioned: createVersionedRouterMock(),


In the interest of reducing core domains packages footprint, I'd prefer we move the versioned router mock into the router server mocks now rather than piling on yet more tech debt.

TinaHeiligers · 2023-04-02T08:05:51Z

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

+  private versionedRouter: undefined | VersionedRouter<Context> = undefined;
+  public get versioned(): VersionedRouter<Context> {
+    if (this.versionedRouter === undefined) {
+      this.versionedRouter = CoreVersionedRouter.from({ router: this });
+    }
+    return this.versionedRouter;
+  }


Based on recent discussions around architectural complexity specifically related to core's packages footprint, perhaps we should move the versioned router into the main core-http-router-server-internal package.

TinaHeiligers · 2023-04-02T08:07:07Z

packages/core/http/core-http-route-validator-internal/index.ts

+ * Side Public License, v 1.
+ */
+
+export { RouteValidator } from './src/validator';


Do we really need another package just for the validator?

This enabled us to use this same validation across the router and versioned router packages (otherwise we'd have a circular dep). Collapsing them into one removes this 🙌🏻

…fix'

kibana-ci · 2023-04-03T16:13:12Z

💚 Build Succeeded

Buildkite Build
Commit: 2c5c840

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	415	417	+2

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-browser`	35	36	+1
`@kbn/core-http-router-server-mocks`	7	8	+1
`@kbn/core-http-server`	176	177	+1
`@kbn/core-http-versioned-router-server-internal`	15	-	-15
total			-12

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

Page load bundle

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

id	before	after	diff
`core`	352.4KB	352.5KB	+38.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-browser`	109	110	+1
`@kbn/core-http-common`	4	5	+1
`@kbn/core-http-router-server-mocks`	7	8	+1
`@kbn/core-http-server`	439	441	+2
`@kbn/core-http-versioned-router-server-internal`	15	-	-15
total			-10

ESLint disabled line counts

id	before	after	diff
`securitySolution`	432	435	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	512	515	+3

History

💔 Build #117360 failed df0211a
💔 Build #117325 failed e723e06
💛 Build #117059 was flaky 42ed016
💚 Build #116638 succeeded 047dfce
💚 Build #116358 succeeded 435b26f

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

cc @jloleysens

jloleysens added Feature:http 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 28, 2023

jloleysens self-assigned this Mar 28, 2023

jloleysens requested a review from a team as a code owner March 28, 2023 11:35

jloleysens added 18 commits March 28, 2023 13:42

moved ApiVersion type to common http package

cfd23a7

runtime check for version number

5a41847

rename shared version header constant, added test for passing through…

52b25b9

… version via header and actually pass version via header

move route validator to own package so that it can be shared

ffa9f66

ran yarn kbn bootstrap

ba1a3a1

import route validator from new package

9d7506d

remove dependence on core internal router

2db39e1

pointless change to tsconfig.json

8af51c5

remove dependence on core internal router

f937521

fix mock response factory

6f01b5c

added versioned router mocks

25d6249

ran yarn kbn bootstrap

983596b

added versioned router mock

ce98092

merge imports from same package

b8c694e

expose the versioned router on the IRouter interface

1364c7a

added an example and removed auto return type inference

b98044b

fix output validation by actually passing in the response 🤦 and lower…

02f330d

… case the header

just use the const

7b88ded

jloleysens force-pushed the expose-versioned-router branch from 705b8eb to 7b88ded Compare March 28, 2023 11:43

kibanamachine and others added 3 commits March 28, 2023 11:49

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

f67e2d5

…fix'

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

a27e49f

remove circular dep

3ad27c0

jloleysens added 3 commits March 31, 2023 14:27

added test and fixed logic

03ab1b9

added integration test case for missing version header

9a3e7c7

fix typo

42ed016

jloleysens requested a review from TinaHeiligers March 31, 2023 12:59

jloleysens mentioned this pull request Mar 31, 2023

[HTTP] Versioned browser client designs #152091

Closed

TinaHeiligers reviewed Apr 2, 2023

View reviewed changes

TinaHeiligers approved these changes Apr 2, 2023

View reviewed changes

jloleysens and others added 9 commits April 3, 2023 11:42

Merge branch 'main' into expose-versioned-router

6a67877

move versioned router mock

fa60493

update exports and use the versioned router mock

f2a532b

move versioned router implementation

3ddc635

remove the validator package

3489693

update a few import paths and ts config references

ca0c431

remove ts project reference

3ea8c48

go back to using actual CoreKibanaRequest!

41df80c

ran yarn kbn bootstrap

154821a

jloleysens enabled auto-merge (squash) April 3, 2023 10:22

jloleysens and others added 6 commits April 3, 2023 12:23

remove 3 unnecessary code owners entries

40fd5e1

remove unnecessary default

3d51eec

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

5388234

…fix'

change the version missing response

e723e06

Merge branch 'main' into expose-versioned-router

df0211a

Merge branch 'main' into expose-versioned-router

2c5c840

jloleysens merged commit ed56403 into elastic:main Apr 3, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 3, 2023

jloleysens deleted the expose-versioned-router branch April 5, 2023 08:37

weltenwort mentioned this pull request May 10, 2023

[Logs UI] Version the logs-related HTTP routes #157324

Closed

maximpn mentioned this pull request May 9, 2024

[Security Solution] Collect the first feedback on documentation bundling approaches #183019

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP] Expose versioned router #153858

[HTTP] Expose versioned router #153858

jloleysens commented Mar 28, 2023 •

edited

Loading

elasticmachine commented Mar 28, 2023

TinaHeiligers Apr 2, 2023

TinaHeiligers Apr 2, 2023

TinaHeiligers left a comment

TinaHeiligers Apr 2, 2023

TinaHeiligers Apr 2, 2023

TinaHeiligers Apr 2, 2023

TinaHeiligers Apr 2, 2023

TinaHeiligers Apr 2, 2023

jloleysens Apr 3, 2023

kibana-ci commented Apr 3, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

		@@ -0,0 +1,3 @@
		# @kbn/core-http-versioned-router-server-mocks

		This package contains the mocks and test utils for core's server-side `http` service

[HTTP] Expose versioned router #153858

[HTTP] Expose versioned router #153858

Conversation

jloleysens commented Mar 28, 2023 • edited Loading

Summary

TODO

Future work

elasticmachine commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Apr 3, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

jloleysens commented Mar 28, 2023 •

edited

Loading