Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context type parameter to IRouter and createRouter #57674

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Provides ability to declare a handler function for a particular path and HTTP re
<b>Signature:</b>

```typescript
createRouter: () => IRouter;
createRouter: <Context extends RequestHandlerContext = RequestHandlerContext>() => IRouter<Context>;
```

## Remarks
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ async (context, request, response) => {
| [auth](./kibana-plugin-server.httpservicesetup.auth.md) | <code>{</code><br/><code> get: GetAuthState;</code><br/><code> isAuthenticated: IsAuthenticated;</code><br/><code> }</code> | |
| [basePath](./kibana-plugin-server.httpservicesetup.basepath.md) | <code>IBasePath</code> | Access or manipulate the Kibana base path See [IBasePath](./kibana-plugin-server.ibasepath.md)<!-- -->. |
| [createCookieSessionStorageFactory](./kibana-plugin-server.httpservicesetup.createcookiesessionstoragefactory.md) | <code>&lt;T&gt;(cookieOptions: SessionStorageCookieOptions&lt;T&gt;) =&gt; Promise&lt;SessionStorageFactory&lt;T&gt;&gt;</code> | Creates cookie based session storage factory [SessionStorageFactory](./kibana-plugin-server.sessionstoragefactory.md) |
| [createRouter](./kibana-plugin-server.httpservicesetup.createrouter.md) | <code>() =&gt; IRouter</code> | Provides ability to declare a handler function for a particular path and HTTP request method. |
| [createRouter](./kibana-plugin-server.httpservicesetup.createrouter.md) | <code>&lt;Context extends RequestHandlerContext = RequestHandlerContext&gt;() =&gt; IRouter&lt;Context&gt;</code> | Provides ability to declare a handler function for a particular path and HTTP request method. |
| [csp](./kibana-plugin-server.httpservicesetup.csp.md) | <code>ICspConfig</code> | The CSP config used for Kibana. |
| [getServerInfo](./kibana-plugin-server.httpservicesetup.getserverinfo.md) | <code>() =&gt; HttpServerInfo</code> | Provides common [information](./kibana-plugin-server.httpserverinfo.md) about the running http server. |
| [isTlsEnabled](./kibana-plugin-server.httpservicesetup.istlsenabled.md) | <code>boolean</code> | Flag showing whether a server was configured to use TLS connection. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Register a route handler for `DELETE` request.
<b>Signature:</b>

```typescript
delete: RouteRegistrar<'delete'>;
delete: RouteRegistrar<'delete', Context>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Register a route handler for `GET` request.
<b>Signature:</b>

```typescript
get: RouteRegistrar<'get'>;
get: RouteRegistrar<'get', Context>;
```
12 changes: 6 additions & 6 deletions docs/development/core/server/kibana-plugin-server.irouter.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,18 @@ Registers route handlers for specified resource path and method. See [RouteConfi
<b>Signature:</b>

```typescript
export interface IRouter
export interface IRouter<Context extends RequestHandlerContext = RequestHandlerContext>
```

## Properties

| Property | Type | Description |
| --- | --- | --- |
| [delete](./kibana-plugin-server.irouter.delete.md) | <code>RouteRegistrar&lt;'delete'&gt;</code> | Register a route handler for <code>DELETE</code> request. |
| [get](./kibana-plugin-server.irouter.get.md) | <code>RouteRegistrar&lt;'get'&gt;</code> | Register a route handler for <code>GET</code> request. |
| [delete](./kibana-plugin-server.irouter.delete.md) | <code>RouteRegistrar&lt;'delete', Context&gt;</code> | Register a route handler for <code>DELETE</code> request. |
| [get](./kibana-plugin-server.irouter.get.md) | <code>RouteRegistrar&lt;'get', Context&gt;</code> | Register a route handler for <code>GET</code> request. |
| [handleLegacyErrors](./kibana-plugin-server.irouter.handlelegacyerrors.md) | <code>&lt;P, Q, B&gt;(handler: RequestHandler&lt;P, Q, B&gt;) =&gt; RequestHandler&lt;P, Q, B&gt;</code> | Wrap a router handler to catch and converts legacy boom errors to proper custom errors. |
| [patch](./kibana-plugin-server.irouter.patch.md) | <code>RouteRegistrar&lt;'patch'&gt;</code> | Register a route handler for <code>PATCH</code> request. |
| [post](./kibana-plugin-server.irouter.post.md) | <code>RouteRegistrar&lt;'post'&gt;</code> | Register a route handler for <code>POST</code> request. |
| [put](./kibana-plugin-server.irouter.put.md) | <code>RouteRegistrar&lt;'put'&gt;</code> | Register a route handler for <code>PUT</code> request. |
| [patch](./kibana-plugin-server.irouter.patch.md) | <code>RouteRegistrar&lt;'patch', Context&gt;</code> | Register a route handler for <code>PATCH</code> request. |
| [post](./kibana-plugin-server.irouter.post.md) | <code>RouteRegistrar&lt;'post', Context&gt;</code> | Register a route handler for <code>POST</code> request. |
| [put](./kibana-plugin-server.irouter.put.md) | <code>RouteRegistrar&lt;'put', Context&gt;</code> | Register a route handler for <code>PUT</code> request. |
| [routerPath](./kibana-plugin-server.irouter.routerpath.md) | <code>string</code> | Resulted path |

Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Register a route handler for `PATCH` request.
<b>Signature:</b>

```typescript
patch: RouteRegistrar<'patch'>;
patch: RouteRegistrar<'patch', Context>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Register a route handler for `POST` request.
<b>Signature:</b>

```typescript
post: RouteRegistrar<'post'>;
post: RouteRegistrar<'post', Context>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Register a route handler for `PUT` request.
<b>Signature:</b>

```typescript
put: RouteRegistrar<'put'>;
put: RouteRegistrar<'put', Context>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ A function executed when route path matched requested resource path. Request han
<b>Signature:</b>

```typescript
export declare type RequestHandler<P = unknown, Q = unknown, B = unknown, Method extends RouteMethod = any> = (context: RequestHandlerContext, request: KibanaRequest<P, Q, B, Method>, response: KibanaResponseFactory) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;
export declare type RequestHandler<P = unknown, Q = unknown, B = unknown, Method extends RouteMethod = any, Context extends RequestHandlerContext = RequestHandlerContext> = (context: Context, request: KibanaRequest<P, Q, B, Method>, response: KibanaResponseFactory) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;
```

## Example
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Route handler common definition
<b>Signature:</b>

```typescript
export declare type RouteRegistrar<Method extends RouteMethod> = <P, Q, B>(route: RouteConfig<P, Q, B, Method>, handler: RequestHandler<P, Q, B, Method>) => void;
export declare type RouteRegistrar<Method extends RouteMethod, Context extends RequestHandlerContext = RequestHandlerContext> = <P, Q, B>(route: RouteConfig<P, Q, B, Method>, handler: RequestHandler<P, Q, B, Method, Context>) => void;
```
7 changes: 5 additions & 2 deletions src/core/server/http/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,12 @@ export class HttpService implements CoreService<InternalHttpServiceSetup, HttpSe
const contract: InternalHttpServiceSetup = {
...serverContract,

createRouter: (path: string, pluginId: PluginOpaqueId = this.coreContext.coreId) => {
createRouter: <Context extends RequestHandlerContext = RequestHandlerContext>(
path: string,
pluginId: PluginOpaqueId = this.coreContext.coreId
) => {
const enhanceHandler = this.requestHandlerContext!.createHandler.bind(null, pluginId);
const router = new Router(path, this.log, enhanceHandler);
const router = new Router<Context>(path, this.log, enhanceHandler);
registerRouter(router);
return router;
},
Expand Down
10 changes: 5 additions & 5 deletions src/core/server/http/router/error_wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ import { RequestHandler } from './router';
import { RequestHandlerContext } from '../../../server';
import { RouteMethod } from './route';

export const wrapErrors = <P, Q, B>(
handler: RequestHandler<P, Q, B, RouteMethod>
): RequestHandler<P, Q, B, RouteMethod> => {
export const wrapErrors = <P, Q, B, M extends RouteMethod, C extends RequestHandlerContext>(
handler: RequestHandler<P, Q, B, M, C>
): RequestHandler<P, Q, B, M, C> => {
return async (
context: RequestHandlerContext,
request: KibanaRequest<P, Q, B, RouteMethod>,
context: C,
request: KibanaRequest<P, Q, B, M>,
response: KibanaResponseFactory
) => {
try {
Expand Down
68 changes: 42 additions & 26 deletions src/core/server/http/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,12 @@ interface RouterRoute {
*
* @public
*/
export type RouteRegistrar<Method extends RouteMethod> = <P, Q, B>(
export type RouteRegistrar<
Method extends RouteMethod,
Context extends RequestHandlerContext = RequestHandlerContext
> = <P, Q, B>(
route: RouteConfig<P, Q, B, Method>,
handler: RequestHandler<P, Q, B, Method>
handler: RequestHandler<P, Q, B, Method, Context>
) => void;

/**
Expand All @@ -53,7 +56,7 @@ export type RouteRegistrar<Method extends RouteMethod> = <P, Q, B>(
*
* @public
*/
export interface IRouter {
export interface IRouter<Context extends RequestHandlerContext = RequestHandlerContext> {
/**
* Resulted path
*/
Expand All @@ -64,35 +67,35 @@ export interface IRouter {
* @param route {@link RouteConfig} - a route configuration.
* @param handler {@link RequestHandler} - a function to call to respond to an incoming request
*/
get: RouteRegistrar<'get'>;
get: RouteRegistrar<'get', Context>;

/**
* Register a route handler for `POST` request.
* @param route {@link RouteConfig} - a route configuration.
* @param handler {@link RequestHandler} - a function to call to respond to an incoming request
*/
post: RouteRegistrar<'post'>;
post: RouteRegistrar<'post', Context>;

/**
* Register a route handler for `PUT` request.
* @param route {@link RouteConfig} - a route configuration.
* @param handler {@link RequestHandler} - a function to call to respond to an incoming request
*/
put: RouteRegistrar<'put'>;
put: RouteRegistrar<'put', Context>;

/**
* Register a route handler for `PATCH` request.
* @param route {@link RouteConfig} - a route configuration.
* @param handler {@link RequestHandler} - a function to call to respond to an incoming request
*/
patch: RouteRegistrar<'patch'>;
patch: RouteRegistrar<'patch', Context>;

/**
* Register a route handler for `DELETE` request.
* @param route {@link RouteConfig} - a route configuration.
* @param handler {@link RequestHandler} - a function to call to respond to an incoming request
*/
delete: RouteRegistrar<'delete'>;
delete: RouteRegistrar<'delete', Context>;

/**
* Wrap a router handler to catch and converts legacy boom errors to proper custom errors.
Expand All @@ -108,9 +111,15 @@ export interface IRouter {
getRoutes: () => RouterRoute[];
}

export type ContextEnhancer<P, Q, B, Method extends RouteMethod> = (
handler: RequestHandler<P, Q, B, Method>
) => RequestHandlerEnhanced<P, Q, B, Method>;
export type ContextEnhancer<
P,
Q,
B,
Method extends RouteMethod,
Context extends RequestHandlerContext
> = (
handler: RequestHandler<P, Q, B, Method, Context>
) => RequestHandlerEnhanced<P, Q, B, Method, Context>;

function getRouteFullPath(routerPath: string, routePath: string) {
// If router's path ends with slash and route's path starts with slash,
Expand Down Expand Up @@ -193,22 +202,22 @@ function validOptions(
/**
* @internal
*/
export class Router implements IRouter {
export class Router<Context extends RequestHandlerContext> implements IRouter<Context> {
public routes: Array<Readonly<RouterRoute>> = [];
public get: IRouter['get'];
public post: IRouter['post'];
public delete: IRouter['delete'];
public put: IRouter['put'];
public patch: IRouter['patch'];
public get: IRouter<Context>['get'];
public post: IRouter<Context>['post'];
public delete: IRouter<Context>['delete'];
public put: IRouter<Context>['put'];
public patch: IRouter<Context>['patch'];

constructor(
public readonly routerPath: string,
private readonly log: Logger,
private readonly enhanceWithContext: ContextEnhancer<any, any, any, any>
private readonly enhanceWithContext: ContextEnhancer<any, any, any, any, any>
) {
const buildMethod = <Method extends RouteMethod>(method: Method) => <P, Q, B>(
route: RouteConfig<P, Q, B, Method>,
handler: RequestHandler<P, Q, B, Method>
handler: RequestHandler<P, Q, B, Method, Context>
) => {
const routeSchemas = routeSchemasFromRouteConfig(route, method);

Expand Down Expand Up @@ -237,7 +246,9 @@ export class Router implements IRouter {
return [...this.routes];
}

public handleLegacyErrors<P, Q, B>(handler: RequestHandler<P, Q, B>): RequestHandler<P, Q, B> {
public handleLegacyErrors<P, Q, B, M extends RouteMethod, C extends RequestHandlerContext>(
handler: RequestHandler<P, Q, B, M, C>
): RequestHandler<P, Q, B, M, C> {
return wrapErrors(handler);
}

Expand All @@ -249,7 +260,7 @@ export class Router implements IRouter {
}: {
request: Request;
responseToolkit: ResponseToolkit;
handler: RequestHandlerEnhanced<P, Q, B, typeof request.method>;
handler: RequestHandlerEnhanced<P, Q, B, typeof request.method, Context>;
routeSchemas?: RouteValidator<P, Q, B>;
}) {
let kibanaRequest: KibanaRequest<P, Q, B, typeof request.method>;
Expand All @@ -274,9 +285,13 @@ type WithoutHeadArgument<T> = T extends (first: any, ...rest: infer Params) => i
? (...rest: Params) => Return
: never;

type RequestHandlerEnhanced<P, Q, B, Method extends RouteMethod> = WithoutHeadArgument<
RequestHandler<P, Q, B, Method>
>;
type RequestHandlerEnhanced<
P,
Q,
B,
Method extends RouteMethod,
Context extends RequestHandlerContext
> = WithoutHeadArgument<RequestHandler<P, Q, B, Method, Context>>;

/**
* A function executed when route path matched requested resource path.
Expand Down Expand Up @@ -316,9 +331,10 @@ export type RequestHandler<
P = unknown,
Q = unknown,
B = unknown,
Method extends RouteMethod = any
Method extends RouteMethod = any,
Context extends RequestHandlerContext = RequestHandlerContext
> = (
context: RequestHandlerContext,
context: Context,
request: KibanaRequest<P, Q, B, Method>,
response: KibanaResponseFactory
) => IKibanaResponse<any> | Promise<IKibanaResponse<any>>;
9 changes: 7 additions & 2 deletions src/core/server/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ export interface HttpServiceSetup {
* ```
* @public
*/
createRouter: () => IRouter;
createRouter: <Context extends RequestHandlerContext = RequestHandlerContext>() => IRouter<
Context
>;

/**
* Register a context provider for a route handler.
Expand Down Expand Up @@ -264,7 +266,10 @@ export interface InternalHttpServiceSetup
extends Omit<HttpServiceSetup, 'createRouter' | 'registerRouteHandlerContext'> {
auth: HttpServerSetup['auth'];
server: HttpServerSetup['server'];
createRouter: (path: string, plugin?: PluginOpaqueId) => IRouter;
createRouter: <Context extends RequestHandlerContext = RequestHandlerContext>(
path: string,
plugin?: PluginOpaqueId
) => IRouter<Context>;
getAuthHeaders: GetAuthHeaders;
registerRouteHandlerContext: <T extends keyof RequestHandlerContext>(
pluginOpaqueId: PluginOpaqueId,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are more or less deprecating the

declare module 'src/core/server' {
  interface RouteHandlerContext {
    myField: MyType;
  }
}

approach in favor of that, I think the

  registerRouteHandlerContext: <T extends keyof RequestHandlerContext>(
    contextName: T,
    provider: RequestHandlerContextProvider<T>
  ) => RequestHandlerContextContainer;

should change to something like

  registerRouteHandlerContext: <T extends string>(
    contextName: T,
    provider: RequestHandlerContextProvider<T>
  ) => RequestHandlerContextContainer;

Else, plugin would still need to use the declare statement to be able to add the key to the RequestHandlerContext before registering. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only other option I can is making the context type here generic as well, so it'd be:

  registerRouteHandlerContext: <
    TContext extends RequestHandlerContext,
    T extends keyof TContext
  >(
    contextName: T,
    // this would probably need to be changed to accept a type arg as well
    // ...and possibly IContextContainer would need to be changed?
    provider: RequestHandlerContextProvider<T>
  ) => RequestHandlerContextContainer;

Only concern with this is that the generics in IContextContainer are already confusing...adding more generics might make this worse?

Copy link
Contributor Author

@pgayvallet pgayvallet Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have enough generics in this part of the code yes and would like to avoid introducing new confusing ones yes. Also with your proposition, I think the call to registerRouteHandlerContext will always to specify the TContext type as it cannot be guessed from the call:

http.registerRouteHandlerContext<MyCustomRequestHandlerContext>('myKey', provider)

Although, my example is also false:

  registerRouteHandlerContext: <T extends string>(
    contextName: T,
    provider: RequestHandlerContextProvider<T>
  ) => RequestHandlerContextContainer;

Still doesn't work without the declare statement, as the provider is still expected to be a RequestHandlerContextProvider<T>, which is:

export type RequestHandlerContextProvider<
  TContextName extends keyof RequestHandlerContext
> = IContextProvider<RequestHandler<any, any, any>, TContextName>;

which is(...)

export type IContextProvider<
  THandler extends HandlerFunction<any>,
  TContextName extends keyof HandlerContextType<THandler>
> = (
  context: Partial<HandlerContextType<THandler>>,
  ...rest: HandlerParameters<THandler>
) =>
  | Promise<HandlerContextType<THandler>[TContextName]>
  | HandlerContextType<THandler>[TContextName];

Meaning it still expects to resolve it's type from a property of RequestHandlerContext.

Changing the registerRouteHandlerContext signature is going to be way harder than anticipated. Really not sure of the direction to take, the context types in src/core/utils/context.ts are really hard to follow... My guess is that we should cut the relation between the context name and the context provider when registering, as there is no longer a direct link between the name of the context and the provider's result type.

However that's some big changes as it means changing the context base types, and these are used elsewhere. So I'm starting to think your proposition is the only realistic one...

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that we should cut the relation between the context name and the context provider when registering, as there is no longer a direct link between the name of the context and the provider's result type.

I personally prefer that we maintain type safety here so that we can be sure that the provider that is registered is actually returning what it is supposed to.

However, I agree the generics in src/core/utils/context.ts are already confusing. One option could be to only add the generics to registerRouteHandlerContext and cast it to any when passing it over to IContextContainer#registerContext. Reason I think this may be acceptable at this time is that we may be able to replace the super generic IContextContainer with a concrete implementation just for route handlers once we remove ApplicationService's context completely (already deprecated).

Expand Down
5 changes: 3 additions & 2 deletions src/core/server/legacy/legacy_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import {
LegacyVars,
} from './types';
import { LegacyInternals } from './legacy_internals';
import { CoreSetup, CoreStart } from '..';
import { CoreSetup, CoreStart, RequestHandlerContext } from '..';

interface LegacyKbnServer {
applyLoggingConfiguration: (settings: Readonly<LegacyVars>) => void;
Expand Down Expand Up @@ -281,7 +281,8 @@ export class LegacyService implements CoreService {
null,
this.legacyId
),
createRouter: () => setupDeps.core.http.createRouter('', this.legacyId),
createRouter: <T extends RequestHandlerContext>() =>
setupDeps.core.http.createRouter<T>('', this.legacyId),
registerOnPreAuth: setupDeps.core.http.registerOnPreAuth,
registerAuth: setupDeps.core.http.registerAuth,
registerOnPostAuth: setupDeps.core.http.registerOnPostAuth,
Expand Down
5 changes: 3 additions & 2 deletions src/core/server/plugins/plugin_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
config as elasticsearchConfig,
} from '../elasticsearch/elasticsearch_config';
import { pick, deepFreeze } from '../../utils';
import { CoreSetup, CoreStart } from '..';
import { CoreSetup, CoreStart, RequestHandlerContext } from '..';

/**
* This returns a facade for `CoreContext` that will be exposed to the plugin initializer.
Expand Down Expand Up @@ -155,7 +155,8 @@ export function createPluginSetupContext<TPlugin, TPluginDependencies>(
null,
plugin.opaqueId
),
createRouter: () => deps.http.createRouter('', plugin.opaqueId),
createRouter: <T extends RequestHandlerContext>() =>
deps.http.createRouter<T>('', plugin.opaqueId),
registerOnPreAuth: deps.http.registerOnPreAuth,
registerAuth: deps.http.registerAuth,
registerOnPostAuth: deps.http.registerOnPostAuth,
Expand Down
Loading