[HTTP/OAS] Prepare router and versioned router for generating OAS #180275

jloleysens · 2024-04-08T11:48:23Z

Summary

Notes

Extracted and adapted from PoC: Derive OAS 3 from Kibana router #156357

jloleysens · 2024-04-08T11:49:16Z

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

@@ -7,6 +7,8 @@
 */

 export { filterHeaders } from './src/headers';
+export { versionHandlerResolvers } from './src/versioned_router';


Not used in this PR, just exporting in preparation for future PR, happy to hold off on this change

jloleysens · 2024-04-08T11:50:28Z

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

+        method: 'post',
+        path: '/',
+        validationSchemas: { body: validation, query: validation, params: validation },
+        isVersioned: false,


Mainly making sure that the above two fields are included as expected, will be used by OAS generation script

jloleysens · 2024-04-08T11:52:04Z

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

@@ -228,7 +228,7 @@ export interface AddVersionOpts<P, Q, B> {
   * Validation for this version of a route
   * @public
   */
-  validate: false | FullValidationConfig<P, Q, B>;
+  validate: false | FullValidationConfig<P, Q, B> | (() => FullValidationConfig<P, Q, B>); // Provide a way to lazily load validation schemas


This will enable ResponseOps and other plugins to provide route schemas lazily. Useful for registries that only know schema shapes after setup/start time.

I'm surprised to not see associated code changes to support passing a function now. Unless I missed it or it already worked and just wasn't documented (but traditional routes don't support that AFAIK, so...)

We have some new code in packages/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.ts to extract the validations.

But yeah, we should probably make IRouter symmetrical! Will update and add a couple of tests.

Pushed some updates that contain tests and update Router type and implementatio to support lazy validation

jloleysens · 2024-04-08T13:29:01Z

/ci

jloleysens · 2024-04-09T11:10:11Z

/ci

elasticmachine · 2024-04-09T11:13:03Z

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

jloleysens · 2024-04-10T06:43:17Z

One last try 😅

jloleysens · 2024-04-10T06:43:24Z

/ci

pgayvallet

Looking good overall.

Just asked for a few clarifications before approval:

pgayvallet · 2024-04-11T06:52:50Z

...core/http/core-http-router-server-internal/src/versioned_router/core_versioned_route.test.ts

@@ -44,7 +40,7 @@ const createRequest = (
  );

 describe('Versioned route', () => {
-  let router: IRouter;
+  let router: Router;


Why do we need to use the concrete implementation's as type here now? It doesn't seem necessary for the change on expected arguments you did below.

It's for CoreVersionedRoute to pass an additional argument "isVersioned" when registering a route.

pgayvallet · 2024-04-11T06:54:50Z

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

+        handler: RequestHandler<P, Q, B, Context, Method>,
+        internalOptions: { isVersioned: boolean } = { isVersioned: false }


So if I understand correctly, this new internalOptions third parameter doesn't leak on the public interface because we only specify it on the implementation via this new InternalRegistrar type on our method helpers than extends IRouter, is that correct?

Yes, exactly

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

pgayvallet · 2024-04-11T07:02:08Z

...ges/core/http/core-http-router-server-internal/src/versioned_router/core_versioned_router.ts


 /** @internal */
 export interface VersionedRouterArgs {
-  router: IRouter;
+  router: Router;


(thinking out loud) I get why we did that, I still can't avoid thinking it's a shame that we're now forced to use our concrete implementation directly here. Relation was pure before, it's no longer.

I agree, it is a :shame: moment. One alternative is that we can make something like an InternalIRouter interface to keep things more pure. Wdyt?

Probably don't need to add a type InternalIRouter = PublicMethodOf<Router>

pgayvallet · 2024-04-11T07:07:18Z

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

@@ -228,7 +228,7 @@ export interface AddVersionOpts<P, Q, B> {
   * Validation for this version of a route
   * @public
   */
-  validate: false | FullValidationConfig<P, Q, B>;
+  validate: false | FullValidationConfig<P, Q, B> | (() => FullValidationConfig<P, Q, B>); // Provide a way to lazily load validation schemas


I'm surprised to not see associated code changes to support passing a function now. Unless I missed it or it already worked and just wasn't documented (but traditional routes don't support that AFAIK, so...)

elasticmachine · 2024-04-11T08:54:52Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

jloleysens · 2024-04-11T09:06:04Z

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

@@ -198,7 +198,7 @@ export interface VersionedRouteResponseValidation {
 * Versioned route validation
 * @public
 */
-export interface FullValidationConfig<P, Q, B> {
+export interface VersionedRouteValidation<P, Q, B> {


Renamed for consistency, not necessary change for this PR

kibana-ci · 2024-04-11T10:09:58Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 5a35aa9

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #41 / apis transform basic license transform /internal/transform/reauthorize_transforms single transform reauthorize_transforms "after each" hook for "should reauthorize transform created by transform_viewer with new api key of poweruser and start the transform"
[job] [logs] FTR Configs #56 / transform /internal/transform/schedule_now_transforms bulk schedule "after each" hook for "should schedule multiple transforms by transformIds"

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-router-server-internal`	27	49	+22

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-router-server-internal`	2	5	+3

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-http-router-server-internal`	27	49	+22

ESLint disabled line counts

id	before	after	diff
`@kbn/core-http-router-server-internal`	0	1	+1

Total ESLint disabled count

id	before	after	diff
`@kbn/core-http-router-server-internal`	0	1	+1

History

💚 Build #202456 succeeded c19489c
💔 Build #202397 failed b5895eb
💔 Build #202177 failed b5895eb
💔 Build #202138 failed cf73ce0
💔 Build #202082 failed 5fb330e
💔 Build #201806 failed 98d2e01

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

pgayvallet

LGTM

smith

Synthetics changes look good

patrykkopycinski

Thank you @jloleysens

jloleysens added 6 commits April 5, 2024 16:37

updated router to expose route metadata

df4219a

export version resolvers

b7ef503

update types to allow lazy route schema registration

1142d49

update core versioned route to use the internal router

7aa141c

update core versioned router to use the internal router

ce59002

update tests

98d2e01

jloleysens commented Apr 8, 2024

View reviewed changes

Merge branch 'main' into http/update-core-routers-for-oas

5fb330e

jloleysens marked this pull request as ready for review April 9, 2024 11:12

jloleysens requested a review from a team as a code owner April 9, 2024 11:12

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 v8.14.0 labels Apr 9, 2024

fix types

cf73ce0

jloleysens requested review from a team as code owners April 9, 2024 13:33

Merge branch 'main' into http/update-core-routers-for-oas

b5895eb

Merge branch 'main' into http/update-core-routers-for-oas

c19489c

angorayc approved these changes Apr 10, 2024

View reviewed changes

pgayvallet reviewed Apr 11, 2024

View reviewed changes

jloleysens added 3 commits April 11, 2024 10:19

refactored and updated versioned route tests

9ea7d5c

rename versioned route validation for clarity

8155cd8

lazily load config on router

05ba2b7

update the core router to also take lazily provided validation

d9dc80e

jloleysens requested review from a team as code owners April 11, 2024 08:54

botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Apr 11, 2024

jest.Mocked

5a35aa9

jughosta approved these changes Apr 11, 2024

View reviewed changes

jloleysens commented Apr 11, 2024

View reviewed changes

pgayvallet approved these changes Apr 11, 2024

View reviewed changes

jloleysens enabled auto-merge (squash) April 11, 2024 12:59

smith approved these changes Apr 11, 2024

View reviewed changes

patrykkopycinski approved these changes Apr 11, 2024

View reviewed changes

jloleysens merged commit 8da3ec4 into elastic:main Apr 11, 2024
40 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Apr 11, 2024

jloleysens deleted the http/update-core-routers-for-oas branch April 11, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP/OAS] Prepare router and versioned router for generating OAS #180275

[HTTP/OAS] Prepare router and versioned router for generating OAS #180275

jloleysens commented Apr 8, 2024

jloleysens Apr 8, 2024

jloleysens Apr 8, 2024

jloleysens Apr 8, 2024

pgayvallet Apr 11, 2024

jloleysens Apr 11, 2024

jloleysens Apr 11, 2024

jloleysens commented Apr 8, 2024

jloleysens commented Apr 9, 2024

elasticmachine commented Apr 9, 2024

jloleysens commented Apr 10, 2024

jloleysens commented Apr 10, 2024

pgayvallet left a comment

pgayvallet Apr 11, 2024

jloleysens Apr 11, 2024

pgayvallet Apr 11, 2024

jloleysens Apr 11, 2024

pgayvallet Apr 11, 2024

jloleysens Apr 11, 2024 •

edited

Loading

pgayvallet Apr 11, 2024

pgayvallet Apr 11, 2024

elasticmachine commented Apr 11, 2024

jloleysens Apr 11, 2024

kibana-ci commented Apr 11, 2024

API count

ESLint disabled line counts

Total ESLint disabled count

pgayvallet left a comment

smith left a comment

patrykkopycinski left a comment

		handler: RequestHandler<P, Q, B, Context, Method>,
		internalOptions: { isVersioned: boolean } = { isVersioned: false }

[HTTP/OAS] Prepare router and versioned router for generating OAS #180275

[HTTP/OAS] Prepare router and versioned router for generating OAS #180275

Conversation

jloleysens commented Apr 8, 2024

Summary

Notes

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

jloleysens commented Apr 8, 2024

jloleysens commented Apr 9, 2024

elasticmachine commented Apr 9, 2024

jloleysens commented Apr 10, 2024

jloleysens commented Apr 10, 2024

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

Choose a reason for hiding this comment

jloleysens Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 11, 2024

Choose a reason for hiding this comment

kibana-ci commented Apr 11, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet left a comment

Choose a reason for hiding this comment

smith left a comment

Choose a reason for hiding this comment

patrykkopycinski left a comment

Choose a reason for hiding this comment

jloleysens Apr 11, 2024 •

edited

Loading