Skip to content

Commit

Permalink
[HTTP/OAS] Lazy response schemas (#181622)
Browse files Browse the repository at this point in the history
## Summary

Based on the introduction of new response schemas for OAS generation we
are going to start the long tail of introducing missing response (`joi`)
schemas. We have roughly 520 known public APIs, most of which do not
have response schemas defined. We expected a fairly large increase in
`@kbn/config-schema` definitions in the coming weeks/months. Regardless
of actual outcome and given how slow schema instantiation is, this
presents a slight concern for startup time.

## Proposed changes

Give consumers guidance and a way to pass in validation lazily. Under
the hood we make sure that the lazy schemas only get called once.

```ts

/**
 * A validation schema factory.
 *
 * @note Used to lazily create schemas that are otherwise not needed
 * @note Assume this function will only be called once
 *
 * @return A @kbn/config-schema schema
 * @public
 */
export type LazyValidator = () => Type<unknown>;

/** @public */
export interface VersionedRouteCustomResponseBodyValidation {
  /** A custom validation function */
  custom: RouteValidationFunction<unknown>;
}

/** @public */
export type VersionedResponseBodyValidation =
  | LazyValidator
  | VersionedRouteCustomResponseBodyValidation;

/**
 * Map of response status codes to response schemas
 *
 * @note Instantiating response schemas is expensive, especially when it is
 *       not needed in most cases. See example below to ensure this is lazily
 *       provided.
 *
 * @note The {@link TypeOf} type utility from @kbn/config-schema can extract
 *       types from lazily created schemas
 *
 * @example
 * ```ts
 * // Avoid this:
 * const badResponseSchema = schema.object({ foo: foo.string() });
 * // Do this:
 * const goodResponseSchema = () => schema.object({ foo: foo.string() });
 *
 * type ResponseType = TypeOf<typeof goodResponseSchema>;
 * ...
 * .addVersion(
 *  { ... validation: { response: { 200: { body: goodResponseSchema } } } },
 *  handlerFn
 * )
 * ...
 * ```
 * @public
 */
export interface VersionedRouteResponseValidation {
  [statusCode: number]: {
    body: VersionedResponseBodyValidation;
  };
  unsafe?: { body?: boolean };
}
```

## Notes

* Expected (worst case) in low resource environments is an additional
1.5 seconds to start up time and additional ~70MB to memory pressure
which is not a great trade-off for functionality that is only used when
OAS generation is on.

Related #181277
  • Loading branch information
jloleysens authored Apr 29, 2024
1 parent c96a560 commit 97e1d9f
Show file tree
Hide file tree
Showing 56 changed files with 713 additions and 248 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export class CoreAppsService {
body: schema.recordOf(schema.string(), schema.any()),
},
response: {
'200': { body: schema.object({ ok: schema.boolean() }) },
'200': { body: () => schema.object({ ok: schema.boolean() }) },
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export {
versionHandlerResolvers,
CoreVersionedRouter,
ALLOWED_PUBLIC_VERSION,
unwrapVersionedResponseBodyValidation,
type VersionedRouterRoute,
type HandlerResolutionStrategy,
} from './src/versioned_router';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@

import { Router, type RouterOptions } from './router';
import { loggingSystemMock } from '@kbn/core-logging-server-mocks';
import { schema } from '@kbn/config-schema';
import { isConfigSchema, schema } from '@kbn/config-schema';
import { createFooValidation } from './router.test.util';
import { createRequestMock } from '@kbn/hapi-mocks/src/request';
import type { RouteValidatorRequestAndResponses } from '@kbn/core-http-server';

const mockResponse: any = {
code: jest.fn().mockImplementation(() => mockResponse),
Expand All @@ -32,6 +33,10 @@ const routerOptions: RouterOptions = {
};

describe('Router', () => {
let testValidation: ReturnType<typeof createFooValidation>;
beforeEach(() => {
testValidation = createFooValidation();
});
afterEach(() => jest.clearAllMocks());
describe('#getRoutes', () => {
it('returns expected route metadata', () => {
Expand Down Expand Up @@ -81,12 +86,10 @@ describe('Router', () => {
});
});

const { fooValidation, validateBodyFn, validateOutputFn, validateParamsFn, validateQueryFn } =
createFooValidation();

it.each([['static' as const], ['lazy' as const]])(
'runs %s route validations',
async (staticOrLazy) => {
const { fooValidation } = testValidation;
const router = new Router('', logger, enhanceWithContext, routerOptions);
router.post(
{
Expand All @@ -104,6 +107,8 @@ describe('Router', () => {
}),
mockResponseToolkit
);
const { validateBodyFn, validateParamsFn, validateQueryFn, validateOutputFn } =
testValidation;
expect(validateBodyFn).toHaveBeenCalledTimes(1);
expect(validateParamsFn).toHaveBeenCalledTimes(1);
expect(validateQueryFn).toHaveBeenCalledTimes(1);
Expand All @@ -113,6 +118,16 @@ describe('Router', () => {

it('constructs lazily provided validations once (idempotency)', async () => {
const router = new Router('', logger, enhanceWithContext, routerOptions);
const { fooValidation } = testValidation;

const response200 = fooValidation.response[200].body;
const lazyResponse200 = jest.fn(() => response200());
fooValidation.response[200].body = lazyResponse200;

const response404 = fooValidation.response[404].body;
const lazyResponse404 = jest.fn(() => response404());
fooValidation.response[404].body = lazyResponse404;

const lazyValidation = jest.fn(() => fooValidation);
router.post(
{
Expand All @@ -121,7 +136,7 @@ describe('Router', () => {
},
(context, req, res) => res.ok()
);
const [{ handler }] = router.getRoutes();
const [{ handler, validationSchemas }] = router.getRoutes();
for (let i = 0; i < 10; i++) {
await handler(
createRequestMock({
Expand All @@ -131,8 +146,25 @@ describe('Router', () => {
}),
mockResponseToolkit
);

expect(
isConfigSchema(
(
validationSchemas as () => RouteValidatorRequestAndResponses<unknown, unknown, unknown>
)().response![200].body()
)
).toBe(true);
expect(
isConfigSchema(
(
validationSchemas as () => RouteValidatorRequestAndResponses<unknown, unknown, unknown>
)().response![404].body()
)
).toBe(true);
}
expect(lazyValidation).toHaveBeenCalledTimes(1);
expect(lazyResponse200).toHaveBeenCalledTimes(1);
expect(lazyResponse404).toHaveBeenCalledTimes(1);
});

it('registers pluginId if provided', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,18 @@ export function createFooValidation() {
},
response: {
200: {
body: schema.object({
foo: schema.number({
validate: validateOutputFn,
body: () =>
schema.object({
foo: schema.number({
validate: validateOutputFn,
}),
}),
},
404: {
body: () =>
schema.object({
error: schema.string(),
}),
}),
},
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
*/

import type { Request, ResponseToolkit } from '@hapi/hapi';
import { once } from 'lodash';
import apm from 'elastic-apm-node';
import { isConfigSchema } from '@kbn/config-schema';
import type { Logger } from '@kbn/logging';
Expand Down Expand Up @@ -35,6 +34,7 @@ import { kibanaResponseFactory } from './response';
import { HapiResponseAdapter } from './response_adapter';
import { wrapErrors } from './error_wrapper';
import { Method } from './versioned_router/types';
import { prepareRouteConfigValidation } from './util';

export type ContextEnhancer<
P,
Expand Down Expand Up @@ -187,9 +187,7 @@ export class Router<Context extends RequestHandlerContextBase = RequestHandlerCo
handler: RequestHandler<P, Q, B, Context, Method>,
internalOptions: { isVersioned: boolean } = { isVersioned: false }
) => {
if (typeof route.validate === 'function') {
route = { ...route, validate: once(route.validate) };
}
route = prepareRouteConfigValidation(route);
const routeSchemas = routeSchemasFromRouteConfig(route, method);

this.routes.push({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { schema } from '@kbn/config-schema';
import { RouteValidator } from '@kbn/core-http-server';
import { prepareResponseValidation } from './util';

describe('prepareResponseValidation', () => {
it('wraps only expected values in "once"', () => {
const validation: RouteValidator<unknown, unknown, unknown> = {
request: {},
response: {
200: {
body: jest.fn(() => schema.string()),
},
404: {
body: jest.fn(() => schema.string()),
},
unsafe: {
body: true,
},
},
};

const prepared = prepareResponseValidation(validation.response!);

expect(prepared).toEqual({
200: { body: expect.any(Function) },
404: { body: expect.any(Function) },
unsafe: { body: true },
});

[1, 2, 3].forEach(() => prepared[200].body());
[1, 2, 3].forEach(() => prepared[404].body());

expect(validation.response![200].body).toHaveBeenCalledTimes(1);
expect(validation.response![404].body).toHaveBeenCalledTimes(1);
});
});
66 changes: 66 additions & 0 deletions packages/core/http/core-http-router-server-internal/src/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { once } from 'lodash';
import {
isFullValidatorContainer,
type RouteConfig,
type RouteMethod,
type RouteValidator,
} from '@kbn/core-http-server';
import type { ObjectType, Type } from '@kbn/config-schema';

function isStatusCode(key: string) {
return !isNaN(parseInt(key, 10));
}

interface ResponseValidation {
[statusCode: number]: { body: () => ObjectType | Type<unknown> };
}

export function prepareResponseValidation(validation: ResponseValidation): ResponseValidation {
const responses = Object.entries(validation).map(([key, value]) => {
if (isStatusCode(key)) {
return [key, { body: once(value.body) }];
}
return [key, value];
});

return Object.fromEntries(responses);
}

function prepareValidation<P, Q, B>(validator: RouteValidator<P, Q, B>) {
if (isFullValidatorContainer(validator) && validator.response) {
return {
...validator,
response: prepareResponseValidation(validator.response),
};
}
return validator;
}

// Integration tested in ./routes.test.ts
export function prepareRouteConfigValidation<P, Q, B>(
config: RouteConfig<P, Q, B, RouteMethod>
): RouteConfig<P, Q, B, RouteMethod> {
// Calculating schema validation can be expensive so when it is provided lazily
// we only want to instantiate it once. This also provides idempotency guarantees
if (typeof config.validate === 'function') {
const validate = config.validate;
return {
...config,
validate: once(() => prepareValidation(validate())),
};
} else if (typeof config.validate === 'object' && typeof config.validate !== null) {
return {
...config,
validate: prepareValidation(config.validate),
};
}
return config;
}
Loading

0 comments on commit 97e1d9f

Please sign in to comment.