Skip to content
Merged
Changes from all 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
185 changes: 185 additions & 0 deletions rfcs/text/0005_route_handler.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
- Start Date: 2019-06-29
- RFC PR: (leave this empty)
- Kibana Issue: https://github.com/elastic/kibana/issues/33779

# Summary

Http Service in New platform should provide the ability to execute some logic in response to an incoming request and send the result of this operation back.

# Basic example
Declaring a route handler for `/url` endpoint:
```typescript
router.get(
{ path: '/url', ...[otherRouteParameters] },
(context: Context, request: KibanaRequest, t: KibanaResponseToolkit) => {
// logic to handle request ...
return t.ok(result);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are missing a } here.

);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may be out of the scope of this RFC.

In previous discussions, we've talked about having more explicit methods for registering routes. For example, we've talked about having a registerPublicApi and a registerInternalApi methods to distinguish between internal and public APIs.

Do we intend these functions to live on the HttpSetup contract and accept a Router or for them to live on the Router object itself?

Copy link
Copy Markdown
Contributor Author

@mshustov mshustov Jul 9, 2019

Choose a reason for hiding this comment

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

In previous discussions, we've talked about having more explicit methods for registering routes. For example, we've talked about having a registerPublicApi and a registerInternalApi methods to distinguish between internal and public APIs.

Where can I find this discussion? I heard about something similar only once #33775 (comment)

I want to structure for myself what considered as public API and internal API? what is system api in this picture?

And yeah, we can create a separate issue to discuss

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We discussed this offline at GAH Orlando as part of the discussion to provide OpenAPI specs for public APIs.

In my view:

  • Public APIs are stable across minor releases, provide an OpenAPI spec, are documented publicly, and are exposed to other plugins via a JS client.
  • Internal APIs are specific to a plugin and do not provide any stability guarantees.

It's not immediately obvious to me exactly what a "system API" is right now, but it seems like the same concept as an Internal API. I just think it should be more explicit (via the HTTP route instead of a header).

I'll move this to a separate discussion here: #40623


```

# Motivation
The new platform is built with library-agnostic philosophy and we cannot transfer the current solution for Network layer from Hapi. To avoid vendor lock-in in the future, we have to define route handler logic and request/response objects formats that can be implemented in any low-level library such as Express, Hapi, etc. It means that we are going to operate our own abstractions for such Http domain entities as Router, Route, Route Handler, Request, Response.

# Detailed design
The new platform doesn't support the Legacy platform `Route Handler` format nor exposes implementation details, such as [Hapi.ResponseToolkit](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/hapi/v17/index.d.ts#L984).
Rather `Route Handler` in New platform has the next signature:
```typescript
type RequestHandler = (
context: Context,
request: KibanaRequest,
t: KibanaResponseToolkit
) => KibanaResponse | Promise<KibanaResponse>;
```
and accepts next Kibana specific parameters as arguments:
- context: [Context](https://github.com/elastic/kibana/blob/master/rfcs/text/0003_handler_interface.md#handler-context). A handler context contains core service and plugin functionality already scoped to the incoming request.
- request: [KibanaRequest](https://github.com/elastic/kibana/blob/master/src/core/server/http/router/request.ts). An immutable representation of the incoming request details, such as body, parameters, query, url and route information. Note: you **must** to specify route schema during route declaration to have access to `body, parameters, query` in the request object. You cannot extend KibanaRequest with arbitrary data nor remove any properties from it.
```typescript
interface KibanaRequest {
url: url.Url;
headers: Record<string, string | string [] | undefined>;
params?: Record<string, any>;
body?: Record<string, any>;
query?: Record<string, any>;
route: {
path: string;
method: 'get' | 'post' | ...
options: {
authRequired: boolean;
tags: string [];
}
}
}
```
- t: [KibanaResponseToolkit](https://github.com/elastic/kibana/blob/master/src/core/server/http/router/response.ts#L27)
Provides a set of pre-configured methods to respond to an incoming request. It is expected that handler **always** returns a result of one of `KibanaResponseToolkit` methods as an output:
```typescript
interface KibanaResponseToolkit {
[method:string]: (...params: any) => KibanaResponse
}
router.get(...,
(context: Context, request: KibanaRequest, t: KibanaResponseToolkit): KibanaResponse => {
return t.ok();
// or
return t.redirected('/url');
// or
return t.badRequest(error);
);
```
*KibanaResponseToolkit* methods allow an end user to adjust the next response parameters:
- Body. Supported values:`undefined | string | JSONValue | Buffer | Stream`.
- Status code.
- Headers. Supports adjusting [known values](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/v10/http.d.ts#L8) and attaching [custom values as well](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/v10/http.d.ts#L67)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note: known values - not really related to the RFC, but wondering if relying on IncomingHttpHeaders gives us much benefits. It can definitely makes our lives a bit harder though (unless we commit to patch it on the Kibana level or contribute to DT typings), e.g. with the current Node typings WWW-Authenticate supports only string while in fact it should support string | string[]. Some browsers (e.g. Firefox) better deal with separate headers, and have quite a few bugs when single WWW-Authenticate header includes multiple comma separated entries even though it's supported by the spec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I noticed it. we can use only names of known headers to improve DX with autocomplete in IDE. on the whole this is optional step and may not implement it.


Other response parameters, such as `etag`, `MIME-type`, `bytes` that used in the Legacy platform could be adjusted via Headers.

The router handler doesn't expect that logic inside can throw or return something different from `KibanaResponse`. In this case, Http service will respond with `Server error` to prevent exposure of internal logic details.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If a handler throws a KibanaResponse, the server should still use this thrown response to populate the actual HTTP response. Any other types of objects thrown should result in a 500.

It's not clear whether or not this is the case in this design.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A handler function should:

  • do not throw an error.
  • always return a KibanaResponse.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting, so with this approach you are encouraged to construct the response in the handler code itself, rather than allow business logic to construct HTTP responses.

This encourages less coupling of our logic to the HTTP framework, which is a big plus.

// Without allowing exceptions (this RFC)
const async myHandler(context, req, t) {
  try {
    const obj = await retrieveObj(context.savedObjectClient, req.params.id);
    const res = processObj(obj);
    return t.ok(res);
  } catch (e) {
    // handler constructs the error response
    return t.notFound();
  }
}
// Allowing exceptions (not this RFC - more like Hapi)
const async myHandler(context, req, t) {
  // business logic construct and throw a 404 on it's own
  const obj = await retrieveObj(context.savedObjectClient, req.params.id, t);
  const res = processObj(obj);
  return t.ok(res);
}

Copy link
Copy Markdown
Contributor Author

@mshustov mshustov Jul 10, 2019

Choose a reason for hiding this comment

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

  • Throwing and re-throwing errors inside the code are not fun and hard to follow.
  • If we mix all exceptions it's hard to type properly and distinguish between business errors & exception raised due to a bug.
  • Core doesn't dictate how to handle errors. If a plugin wants to use Boom - ok, but it's not a part of the core contract anymore. If

I'd write your example as:

const async myHandler(context, req, t) {
    const obj = await retrieveObj(context.savedObjectClient, req.params.id);
    if (!obj) return t.notFound();
    const res = processObj(obj);
    return t.ok(res);
}

On the other hand, I understand that people are used to the Hapi feature to response with thrown Error and I'm open to introducing this functionality if needed as long as we allow throwing only KibanaResponseError object instead of Boom error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like that exceptions are used for exceptional or unexpected behaviour so I prefer the API of the RFC above the way Hapi does it 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Throwing and re-throwing errors inside the code are not fun and hard to follow.

++ I like how this removes a small element of "magic" and forces you to explicitly respond with errors in the handler itself. To me the approach in this RFC feels simpler, even if it isn't the Hapi way of doing things.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++, same here, I'd expect "HTTP related logic" (e.g. errors status codes or HTTP headers) to live inside HTTP handlers only where feasible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds like we're all on the same page here 🎉 . I just wanted to be clear on how this would work in the NP.


#### KibanaResponseToolkit methods
Basic primitives:
```typescript
type HttpResponsePayload = undefined | string | JSONValue | Buffer | Stream;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume a more detailed design of the response payloads for various types of responses is outside the scope of this RFC?

It would be nice to provide some consistency in the payload, for example requiring a message in the payload for a 400 error. The response toolkit DSL would enable us to do this; I'm mostly wondering where/when this discussion would take place. (Or more broadly, how much of this we'd actually plan to enforce in core).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice to provide some consistency in the payload, for example requiring a message in the payload for a 400 error. The response toolkit DSL would enable us to do this;

Yeah, our own response toolkit is great, we can tighten signatures as much as we want. Feels like every such case (or group of related cases) would deserve a dedicated PR (not RFC, but at least PR). Another thing to keep in mind is that browsers are full of various quirks and legacy workarounds, so even if we tailor response helpers for the most common use cases we should leave an escape hatch to deal with all that Web weirdness in case it's needed (and I see we have it as custom response toolkit helper, that's great).

Copy link
Copy Markdown
Contributor Author

@mshustov mshustov Jul 24, 2019

Choose a reason for hiding this comment

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

I'd start with more generic definitions outlined in this PR and then tighten them case by case in separate PRs

interface HttpResponseOptions {
headers?: {
// list of known headers
...
// for custom headers:
[header: string]: string | string[];
}
}

```

##### Success
Server indicated that request was accepted:
```typescript
type SuccessResponse<T> = <T extends HttpResponsePayload>(
payload: T,
options?: HttpResponseOptions
) => KibanaResponse<T>;

const kibanaResponseToolkit = {
ok: <T extends HttpResponsePayload>(payload: T, options?: HttpResponseOptions) =>
new KibanaResponse(200, payload, options),
accepted: <T extends HttpResponsePayload>(payload: T, options?: HttpResponseOptions) =>
new KibanaResponse(202, payload, options),
noContent: (options?: HttpResponseOptions) => new KibanaResponse(204, undefined, options)
```

##### Redirection
The server wants a user to perform additional actions.
```typescript
const kibanaResponseToolkit = {
redirected: (url: string, options?: HttpResponseOptions) => new KibanaResponse(302, url, options),
notModified: (options?: HttpResponseOptions) => new KibanaResponse(304, undefined, options),
```

##### Error
Server signals that request cannot be handled and explains details of the error situation
```typescript
// Supports attaching additional data to send to the client
interface ResponseError extends Error {
meta?: {
data?: JSONValue;
errorCode?: string; // error code to simplify search, translations in i18n, etc.
docLink?: string; // link to the docs
}
}

export const createResponseError = (error: Error | string, meta?: ResponseErrorType['meta']) =>
new ResponseError(error, meta)

const kibanaResponseToolkit = {
// Client errors
badRequest: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(400, err, options),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know Error is an interface, but it's also a class which makes creating instances easy. Maybe there should be a convenience method for constructing instances of ResponseError.

Copy link
Copy Markdown
Contributor Author

@mshustov mshustov Jul 9, 2019

Choose a reason for hiding this comment

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

you mean provide a class or a factory as well?

interface ResponseErrorType extends Error {
  meta?: {
    data?: JSONValue;
    errorCode?: string; // error code to simplify search, translations in i18n, etc.
    docLink?: string; // link to the docs
  };
}

class ResponseError extends Error implements ResponseErrorType {
  constructor(error: Error | string, public readonly meta: ResponseErrorType['meta']) {
    super(typeof error === 'string' ? error : error.message);
    Object.setPrototypeOf(this, ResponseError.prototype);
  }
}

// or a factory
const createError = (error: Error | string, meta?: ResponseErrorType['meta']) => 
  new ResponseError(error, meta)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Factory makes sense to me, seems more ergonomic / similar to the response factory methods.

unauthorized: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(401, err, options),

forbidden: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(403, err, options),
notFound: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(404, err, options),
conflict: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(409, err, options),

// Server errors
internal: <T extends ResponseError>(err: T, options?: HttpResponseOptions) =>
new KibanaResponse(500, err, options),
```

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do these above abstraction methods provide enough value to be worth their existence/DSL/maintenance? It seems like something similar to:

t.respond(options: {
  body?: HttpResponsePayload | ResponseError;
  statusCode: number;
  options?: HttpResponseOptions;
});

would be sufficient? Just thinking that most devs are familiar with HTTP, status codes, etc. and the DSL/abstraction may be more trouble than it's worth unless I'm missing context on why it's necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only advantage of a DSL that I can think of is much more control over the shape of responses e.g. a 404 can't return its own custom html markup and that a 204 can't return a body. But the value in doing so might be low.

But I agree with what you said an the drawbacks in the rfc:

KibanaResponseToolkit may not cover all use cases and requires an extension for specific use-cases.
KibanaResponseToolkit operates low-level Http primitives, such as Headers e.g., and it is not always handy to work with them directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only advantage of a DSL that I can think of is much more control over the shape of responses e.g. a 404 can't return its own custom html markup and that a 204 can't return a body

right, it's easier for the platform to perform validation and we have some space to extend core functionality in future (for example, add a type definition that WWW-Authenticate header required when a user is rejected as unauthorized).

Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers Jul 22, 2019

Choose a reason for hiding this comment

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

I love the idea of a simple DSL for validation & consistency; for me the only thing that's missing is a clear understanding of how the DSL translates to an actual http response payload. (But that might just be me)

IMO that shouldn't prevent us from going this direction -- but to @jasonrhodes' point, many devs are already going to be thinking this way, so making it clear exactly what these methods are doing for them will be important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for me the only thing that's missing is a clear understanding of how the DSL translates to an actual http response payload. (But that might just be me)

For success responses, we pass payload to hapi server as is. As I understand, nothing has changed for plugin authors here.
Although, it defines different logic for the error responses. Hapi doesn't allow to respond with Error object, but our DSL does. For the plugin authors it looks like they throw Boom error in hapi request handler.

##### Custom
If a custom response is required
```typescript
interface CustomOptions extends HttpResponseOptions {
statusCode: number;
}
export const kibanaResponseToolkit = {
custom: <T extends HttpResponsePayload>(payload: T, {statusCode, ...options}: CustomOptions) =>
new KibanaResponse(statusCode, payload, options),
```
# Drawbacks
- `Handler` is not compatible with Legacy platform implementation when anything can be returned or thrown from handler function and server send it as a valid result. Transition to the new format may require additional work in plugins.
- `Handler` doesn't cover **all** functionality of the Legacy server at the current moment. For example, we cannot render a view in New platform yet and in this case, we have to proxy the request to the Legacy platform endpoint to perform rendering. All such cases should be considered in an individual order.
- `KibanaResponseToolkit` may not cover all use cases and requires an extension for specific use-cases.
- `KibanaResponseToolkit` operates low-level Http primitives, such as Headers e.g., and it is not always handy to work with them directly.
- `KibanaResponse` cannot be extended with arbitrary data.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: is there any valid use case in KIbana that we treat it as a drawback?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not a drawback per se.
We have custom helper as escape-hatch, but if one doesn't cover a very weird use case it will take some time to extend the core.


# Alternatives

- `Route Handler` may adopt well-known Hapi-compatible format.
- `KibanaResponseToolkit` can expose only one method that allows specifying any type of response body, headers, status without creating additional abstractions and restrictions.
- `KibanaResponseToolkit` may provide helpers for more granular use-cases, say `
binary(data: Buffer, type: MimeType, size: number) => KibanaResponse`

# Adoption strategy

Breaking changes are expected during migration to the New platform. To simplify adoption we could provide an extended set of type definitions for primitives with high variability of possible values (such as content-type header, all headers in general).

# How we teach this

`Route Handler`, `Request`, `Response` terms are familiar to all Kibana developers. Even if their interface is different from existing ones, it shouldn't be a problem to adopt the code to the new format. Adding a section to the Migration guide should be sufficient.

# Unresolved questions

Is proposed functionality cover all the use cases of the `Route Handler` and responding to a request?