Add compatibility wrapper for Boom errors thrown from route handler (#…

…51157)

* add wrapErrors method to router

* add RouteRegistrar type

* update generated doc

* add migration example

* rename wrapErrors to handleLegacyErrors

docs/development/core/server/kibana-plugin-server.irouter.delete.md

@@ -9,5 +9,5 @@ Register a route handler for `DELETE` request.
 <b>Signature:</b>
 
 ```typescript
-delete: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>) => void;
+delete: RouteRegistrar;
 ```

docs/development/core/server/kibana-plugin-server.irouter.get.md

@@ -9,5 +9,5 @@ Register a route handler for `GET` request.
 <b>Signature:</b>
 
 ```typescript
-get: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>) => void;
+get: RouteRegistrar;
 ```

docs/development/core/server/kibana-plugin-server.irouter.handlelegacyerrors.md

@@ -0,0 +1,13 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [IRouter](./kibana-plugin-server.irouter.md) &gt; [handleLegacyErrors](./kibana-plugin-server.irouter.handlelegacyerrors.md)
+
+## IRouter.handleLegacyErrors property
+
+Wrap a router handler to catch and converts legacy boom errors to proper custom errors.
+
+<b>Signature:</b>
+
+```typescript
+handleLegacyErrors: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(handler: RequestHandler<P, Q, B>) => RequestHandler<P, Q, B>;
+```

docs/development/core/server/kibana-plugin-server.irouter.md

@@ -16,9 +16,10 @@ export interface IRouter
 
 |  Property | Type | Description |
 |  --- | --- | --- |
-|  [delete](./kibana-plugin-server.irouter.delete.md) | <code>&lt;P extends ObjectType, Q extends ObjectType, B extends ObjectType&gt;(route: RouteConfig&lt;P, Q, B&gt;, handler: RequestHandler&lt;P, Q, B&gt;) =&gt; void</code> | Register a route handler for <code>DELETE</code> request. |
-|  [get](./kibana-plugin-server.irouter.get.md) | <code>&lt;P extends ObjectType, Q extends ObjectType, B extends ObjectType&gt;(route: RouteConfig&lt;P, Q, B&gt;, handler: RequestHandler&lt;P, Q, B&gt;) =&gt; void</code> | Register a route handler for <code>GET</code> request. |
-|  [post](./kibana-plugin-server.irouter.post.md) | <code>&lt;P extends ObjectType, Q extends ObjectType, B extends ObjectType&gt;(route: RouteConfig&lt;P, Q, B&gt;, handler: RequestHandler&lt;P, Q, B&gt;) =&gt; void</code> | Register a route handler for <code>POST</code> request. |
-|  [put](./kibana-plugin-server.irouter.put.md) | <code>&lt;P extends ObjectType, Q extends ObjectType, B extends ObjectType&gt;(route: RouteConfig&lt;P, Q, B&gt;, handler: RequestHandler&lt;P, Q, B&gt;) =&gt; void</code> | Register a route handler for <code>PUT</code> request. |
+|  [delete](./kibana-plugin-server.irouter.delete.md) | <code>RouteRegistrar</code> | Register a route handler for <code>DELETE</code> request. |
+|  [get](./kibana-plugin-server.irouter.get.md) | <code>RouteRegistrar</code> | Register a route handler for <code>GET</code> request. |
+|  [handleLegacyErrors](./kibana-plugin-server.irouter.handlelegacyerrors.md) | <code>&lt;P extends ObjectType, Q extends ObjectType, B extends ObjectType&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. |
+|  [post](./kibana-plugin-server.irouter.post.md) | <code>RouteRegistrar</code> | Register a route handler for <code>POST</code> request. |
+|  [put](./kibana-plugin-server.irouter.put.md) | <code>RouteRegistrar</code> | Register a route handler for <code>PUT</code> request. |
 |  [routerPath](./kibana-plugin-server.irouter.routerpath.md) | <code>string</code> | Resulted path |
 

docs/development/core/server/kibana-plugin-server.irouter.post.md

@@ -9,5 +9,5 @@ Register a route handler for `POST` request.
 <b>Signature:</b>
 
 ```typescript
-post: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>) => void;
+post: RouteRegistrar;
 ```

docs/development/core/server/kibana-plugin-server.irouter.put.md

@@ -9,5 +9,5 @@ Register a route handler for `PUT` request.
 <b>Signature:</b>
 
 ```typescript
-put: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>) => void;
+put: RouteRegistrar;
 ```

docs/development/core/server/kibana-plugin-server.md

@@ -170,6 +170,7 @@ The plugin integrates with the core system via lifecycle events: `setup`<!-- -->
 |  [ResponseErrorAttributes](./kibana-plugin-server.responseerrorattributes.md) | Additional data to provide error details. |
 |  [ResponseHeaders](./kibana-plugin-server.responseheaders.md) | Http response headers to set. |
 |  [RouteMethod](./kibana-plugin-server.routemethod.md) | The set of common HTTP methods supported by Kibana routing. |
+|  [RouteRegistrar](./kibana-plugin-server.routeregistrar.md) | Handler to declare a route. |
 |  [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) | Type definition for a Saved Object attribute value |
 |  [SavedObjectAttributeSingle](./kibana-plugin-server.savedobjectattributesingle.md) | Don't use this type, it's simply a helper type for [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) |
 |  [SavedObjectsClientContract](./kibana-plugin-server.savedobjectsclientcontract.md) | Saved Objects is Kibana's data persisentence mechanism allowing plugins to use Elasticsearch for storing plugin state.<!-- -->\#\# SavedObjectsClient errors<!-- -->Since the SavedObjectsClient has its hands in everything we are a little paranoid about the way we present errors back to to application code. Ideally, all errors will be either:<!-- -->1. Caused by bad implementation (ie. undefined is not a function) and as such unpredictable 2. An error that has been classified and decorated appropriately by the decorators in [SavedObjectsErrorHelpers](./kibana-plugin-server.savedobjectserrorhelpers.md)<!-- -->Type 1 errors are inevitable, but since all expected/handle-able errors should be Type 2 the <code>isXYZError()</code> helpers exposed at <code>SavedObjectsErrorHelpers</code> should be used to understand and manage error responses from the <code>SavedObjectsClient</code>.<!-- -->Type 2 errors are decorated versions of the source error, so if the elasticsearch client threw an error it will be decorated based on its type. That means that rather than looking for <code>error.body.error.type</code> or doing substring checks on <code>error.body.error.reason</code>, just use the helpers to understand the meaning of the error:<!-- -->\`\`\`<!-- -->js if (SavedObjectsErrorHelpers.isNotFoundError(error)) { // handle 404 }<!-- -->if (SavedObjectsErrorHelpers.isNotAuthorizedError(error)) { // 401 handling should be automatic, but in case you wanted to know }<!-- -->// always rethrow the error unless you handle it throw error; \`\`\`<!-- -->\#\#\# 404s from missing index<!-- -->From the perspective of application code and APIs the SavedObjectsClient is a black box that persists objects. One of the internal details that users have no control over is that we use an elasticsearch index for persistance and that index might be missing.<!-- -->At the time of writing we are in the process of transitioning away from the operating assumption that the SavedObjects index is always available. Part of this transition is handling errors resulting from an index missing. These used to trigger a 500 error in most cases, and in others cause 404s with different error messages.<!-- -->From my (Spencer) perspective, a 404 from the SavedObjectsApi is a 404; The object the request/call was targeting could not be found. This is why \#14141 takes special care to ensure that 404 errors are generic and don't distinguish between index missing or document missing.<!-- -->\#\#\# 503s from missing index<!-- -->Unlike all other methods, create requests are supposed to succeed even when the Kibana index does not exist because it will be automatically created by elasticsearch. When that is not the case it is because Elasticsearch's <code>action.auto_create_index</code> setting prevents it from being created automatically so we throw a special 503 with the intention of informing the user that their Elasticsearch settings need to be updated.<!-- -->See [SavedObjectsClient](./kibana-plugin-server.savedobjectsclient.md) See [SavedObjectsErrorHelpers](./kibana-plugin-server.savedobjectserrorhelpers.md) |

docs/development/core/server/kibana-plugin-server.routeregistrar.md

@@ -0,0 +1,13 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [RouteRegistrar](./kibana-plugin-server.routeregistrar.md)
+
+## RouteRegistrar type
+
+Handler to declare a route.
+
+<b>Signature:</b>
+
+```typescript
+export declare type RouteRegistrar = <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(route: RouteConfig<P, Q, B>, handler: RequestHandler<P, Q, B>) => void;
+```

src/core/MIGRATION_EXAMPLES.md

@@ -298,6 +298,35 @@ class Plugin {
   }
 }
 ```
+If your plugin still relies on throwing Boom errors from routes, you can use the `router.handleLegacyErrors` 
+as a temporary solution until error migration is complete:
+```ts
+// legacy/plugins/demoplugin/server/plugin.ts
+import { schema } from '@kbn/config-schema';
+import { CoreSetup } from 'src/core/server';
+
+export interface DemoPluginsSetup {};
+
+class Plugin {
+  public setup(core: CoreSetup, pluginSetup: DemoPluginSetup) {
+    const router = core.http.createRouter();
+    router.post(
+      {
+        path: '/api/demoplugin/search',
+        validate: {
+          body: schema.object({
+            field1: schema.string(),
+          }),
+        }
+      },
+      router.wrapErrors((context, req, res) => {
+        throw Boom.notFound('not there'); // will be converted into proper New Platform error
+      })
+    )
+  }
+}
+```
+
 
 #### 4. New Platform plugin
 As the final step we delete the shim and move all our code into a New Platform

src/core/server/http/http_service.mock.ts

@@ -45,6 +45,7 @@ const createRouterMock = (): jest.Mocked<IRouter> => ({
   put: jest.fn(),
   delete: jest.fn(),
   getRoutes: jest.fn(),
+  handleLegacyErrors: jest.fn().mockImplementation(handler => handler),
 });
 
 const createSetupContractMock = () => {

src/core/server/http/index.ts

@@ -45,6 +45,7 @@ export {
   IRouter,
   RouteMethod,
   RouteConfigOptions,
+  RouteRegistrar,
 } from './router';
 export { BasePathProxyServer } from './base_path_proxy_server';
 export { OnPreAuthHandler, OnPreAuthToolkit } from './lifecycle/on_pre_auth';

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

@@ -164,6 +164,53 @@ describe('Handler', () => {
   });
 });
 
+describe('handleLegacyErrors', () => {
+  it('properly convert Boom errors', async () => {
+    const { server: innerServer, createRouter } = await server.setup(setupDeps);
+    const router = createRouter('/');
+
+    router.get(
+      { path: '/', validate: false },
+      router.handleLegacyErrors((context, req, res) => {
+        throw Boom.notFound();
+      })
+    );
+    await server.start();
+
+    const result = await supertest(innerServer.listener)
+      .get('/')
+      .expect(404);
+
+    expect(result.body.message).toBe('Not Found');
+  });
+
+  it('returns default error when non-Boom errors are thrown', async () => {
+    const { server: innerServer, createRouter } = await server.setup(setupDeps);
+    const router = createRouter('/');
+
+    router.get(
+      {
+        path: '/',
+        validate: false,
+      },
+      router.handleLegacyErrors((context, req, res) => {
+        throw new Error('Unexpected');
+      })
+    );
+    await server.start();
+
+    const result = await supertest(innerServer.listener)
+      .get('/')
+      .expect(500);
+
+    expect(result.body).toEqual({
+      error: 'Internal Server Error',
+      message: 'An internal server error occurred.',
+      statusCode: 500,
+    });
+  });
+});
+
 describe('Response factory', () => {
   describe('Success', () => {
     it('supports answering with json object', async () => {

src/core/server/http/router/error_wrapper.test.ts

@@ -0,0 +1,80 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import Boom from 'boom';
+import { KibanaResponse, KibanaResponseFactory, kibanaResponseFactory } from './response';
+import { wrapErrors } from './error_wrapper';
+import { KibanaRequest, RequestHandler, RequestHandlerContext } from 'kibana/server';
+
+const createHandler = (handler: () => any): RequestHandler<any, any, any> => () => {
+  return handler();
+};
+
+describe('wrapErrors', () => {
+  let context: RequestHandlerContext;
+  let request: KibanaRequest<any, any, any>;
+  let response: KibanaResponseFactory;
+
+  beforeEach(() => {
+    context = {} as any;
+    request = {} as any;
+    response = kibanaResponseFactory;
+  });
+
+  it('should pass-though call parameters to the handler', async () => {
+    const handler = jest.fn();
+    const wrapped = wrapErrors(handler);
+    await wrapped(context, request, response);
+    expect(handler).toHaveBeenCalledWith(context, request, response);
+  });
+
+  it('should pass-though result from the handler', async () => {
+    const handler = createHandler(() => {
+      return 'handler-response';
+    });
+    const wrapped = wrapErrors(handler);
+    const result = await wrapped(context, request, response);
+    expect(result).toBe('handler-response');
+  });
+
+  it('should intercept and convert thrown Boom errors', async () => {
+    const handler = createHandler(() => {
+      throw Boom.notFound('not there');
+    });
+    const wrapped = wrapErrors(handler);
+    const result = await wrapped(context, request, response);
+    expect(result).toBeInstanceOf(KibanaResponse);
+    expect(result.status).toBe(404);
+    expect(result.payload).toEqual({
+      error: 'Not Found',
+      message: 'not there',
+      statusCode: 404,
+    });
+  });
+
+  it('should re-throw non-Boom errors', async () => {
+    const handler = createHandler(() => {
+      throw new Error('something went bad');
+    });
+    const wrapped = wrapErrors(handler);
+    await expect(wrapped(context, request, response)).rejects.toMatchInlineSnapshot(
+      `[Error: something went bad]`
+    );
+  });
+});

src/core/server/http/router/error_wrapper.ts

@@ -0,0 +1,48 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import Boom from 'boom';
+import { ObjectType, TypeOf } from '@kbn/config-schema';
+import { KibanaRequest } from './request';
+import { KibanaResponseFactory } from './response';
+import { RequestHandler } from './router';
+import { RequestHandlerContext } from '../../../server';
+
+export const wrapErrors = <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(
+  handler: RequestHandler<P, Q, B>
+): RequestHandler<P, Q, B> => {
+  return async (
+    context: RequestHandlerContext,
+    request: KibanaRequest<TypeOf<P>, TypeOf<Q>, TypeOf<B>>,
+    response: KibanaResponseFactory
+  ) => {
+    try {
+      return await handler(context, request, response);
+    } catch (e) {
+      if (Boom.isBoom(e)) {
+        return response.customError({
+          body: e.output.payload,
+          statusCode: e.output.statusCode,
+          headers: e.output.headers,
+        });
+      }
+      throw e;
+    }
+  };
+};

src/core/server/http/router/index.ts

@@ -18,7 +18,7 @@
  */
 
 export { Headers, filterHeaders, ResponseHeaders, KnownHeaders } from './headers';
-export { Router, RequestHandler, IRouter } from './router';
+export { Router, RequestHandler, IRouter, RouteRegistrar } from './router';
 export {
   KibanaRequest,
   KibanaRequestRoute,