Skip to content

Commit

Permalink
Split RegistryError into 3 Registry*Error types
Browse files Browse the repository at this point in the history
```
-import { RegistryError } from '../../../errors';
+import {
+  RegistryConnectionError,
+  RegistryResponseError,
+  RegistryResponseNotFoundError,
+} from '../../../errors';
```
  • Loading branch information
John Schulz committed Aug 24, 2020
1 parent 95400e5 commit 907c761
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 41 deletions.
45 changes: 25 additions & 20 deletions x-pack/plugins/ingest_manager/server/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

/* eslint-disable max-classes-per-file */
import Boom from 'boom';
import Boom, { isBoom } from 'boom';
import {
RequestHandlerContext,
KibanaRequest,
Expand All @@ -22,32 +22,34 @@ export class IngestManagerError extends Error {
}

export const getHTTPResponseCode = (error: IngestManagerError): number => {
if (error instanceof RegistryError) {
return 502; // Bad Gateway
}
if (error instanceof PackageNotFoundError) {
return 404;
}
if (error instanceof PackageOutdatedError) {
return 400;
} else {
return 400; // Bad Request
switch (true) {
case error instanceof RegistryConnectionError:
return 502; // Bad Gateway

case error instanceof RegistryResponseNotFoundError:
case error instanceof PackageNotFoundError:
return 404;

case error instanceof PackageOutdatedError:
return 400;

default:
return 500;
}
};

interface IngestErrorHandlerParams {
export type IngestErrorHandler = (
params: IngestErrorHandlerParams
) => IKibanaResponse | Promise<IKibanaResponse>;

export interface IngestErrorHandlerParams {
error: IngestManagerError | Boom | Error;
response: KibanaResponseFactory;
request?: KibanaRequest;
context?: RequestHandlerContext;
}

export const defaultIngestErrorHandler = async ({
error,
request,
response,
context,
}: IngestErrorHandlerParams): Promise<IKibanaResponse> => {
export const defaultIngestErrorHandler: IngestErrorHandler = async ({ error, response }) => {
const logger = appContextService.getLogger();
if (error instanceof IngestManagerError) {
logger.error(error.message);
Expand All @@ -56,7 +58,7 @@ export const defaultIngestErrorHandler = async ({
body: { message: error.message },
});
}
if ('isBoom' in error) {
if (isBoom(error)) {
logger.error(error.output.payload.message);
return response.customError({
statusCode: error.output.statusCode,
Expand All @@ -70,6 +72,9 @@ export const defaultIngestErrorHandler = async ({
});
};

export class RegistryError extends IngestManagerError {}
export class RegistryResponseError extends IngestManagerError {}
export class RegistryResponseNotFoundError extends IngestManagerError {}
export class RegistryConnectionError extends IngestManagerError {}

export class PackageNotFoundError extends IngestManagerError {}
export class PackageOutdatedError extends IngestManagerError {}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { xpackMocks } from '../../../../../../x-pack/mocks';
import { httpServerMock } from 'src/core/server/mocks';
import { PostIngestSetupResponse } from '../../../common';
import { RegistryError } from '../../errors';
import { RegistryConnectionError } from '../../errors';
import { createAppContextStartContractMock } from '../../mocks';
import { ingestManagerSetupHandler } from './handlers';
import { appContextService } from '../../services/app_context';
Expand Down Expand Up @@ -66,9 +66,9 @@ describe('ingestManagerSetupHandler', () => {
});
});

it('POST /setup fails w/502 on RegistryError', async () => {
it('POST /setup fails w/502 on RegistryConnectionError', async () => {
mockSetupIngestManager.mockImplementation(() =>
Promise.reject(new RegistryError('Registry method mocked to throw'))
Promise.reject(new RegistryConnectionError('Registry method mocked to throw'))
);

await ingestManagerSetupHandler(context, request, response);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { fetchUrl } from './requests';
import { RegistryError } from '../../../errors';
import { RegistryConnectionError } from '../../../errors';
jest.mock('node-fetch');

const { Response, FetchError } = jest.requireActual('node-fetch');
Expand All @@ -20,22 +20,22 @@ describe('setupIngestManager', () => {
});

describe('fetchUrl / getResponse errors', () => {
it('regular Errors do not retry. Becomes RegistryError', async () => {
it('regular Errors do not retry. Becomes RegistryConnectionError', async () => {
fetchMock.mockImplementationOnce(() => {
throw new Error('mocked');
});
const promise = fetchUrl('');
await expect(promise).rejects.toThrow(RegistryError);
await expect(promise).rejects.toThrow(RegistryConnectionError);
expect(fetchMock).toHaveBeenCalledTimes(1);
});

it('TypeErrors do not retry. Becomes RegistryError', async () => {
it('TypeErrors do not retry. Becomes RegistryConnectionError', async () => {
fetchMock.mockImplementationOnce(() => {
// @ts-expect-error
null.f();
});
const promise = fetchUrl('');
await expect(promise).rejects.toThrow(RegistryError);
await expect(promise).rejects.toThrow(RegistryConnectionError);
expect(fetchMock).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -69,7 +69,7 @@ describe('setupIngestManager', () => {
expect(actualResultsOrder).toEqual(['throw', 'throw', 'throw', 'return']);
});

it('or error after 1 failure & 5 retries with RegistryError', async () => {
it('or error after 1 failure & 5 retries with RegistryConnectionError', async () => {
fetchMock
.mockImplementationOnce(() => {
throw new FetchError('message 1', 'system', { code: 'ESOMETHING' });
Expand Down Expand Up @@ -97,7 +97,7 @@ describe('setupIngestManager', () => {
});

const promise = fetchUrl('');
await expect(promise).rejects.toThrow(RegistryError);
await expect(promise).rejects.toThrow(RegistryConnectionError);
// doesn't retry after 1 failure & 5 failed retries
expect(fetchMock).toHaveBeenCalledTimes(6);
const actualResultsOrder = fetchMock.mock.results.map(({ type }: { type: string }) => type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
import fetch, { FetchError, Response } from 'node-fetch';
import pRetry from 'p-retry';
import { streamToString } from './streams';
import { RegistryError } from '../../../errors';
import {
RegistryConnectionError,
RegistryResponseError,
RegistryResponseNotFoundError,
} from '../../../errors';

type FailedAttemptErrors = pRetry.FailedAttemptError | FetchError | Error;

Expand All @@ -19,10 +23,13 @@ async function registryFetch(url: string) {
return response;
} else {
// 4xx & 5xx responses
// exit without retry & throw RegistryError
throw new pRetry.AbortError(
new RegistryError(`Error connecting to package registry at ${url}: ${response.statusText}`)
);
const is404 = response.status === 404;
const ErrorCtor = is404 ? RegistryResponseNotFoundError : RegistryResponseError;
const message = `'${response.status} ${response.statusText}' response from package registry at ${url}`;
const error = new ErrorCtor(message);

// exit without retry (will ultimately throw RegistryConnectionError)
throw new pRetry.AbortError(error);
}
}

Expand All @@ -38,17 +45,16 @@ export async function getResponse(url: string): Promise<Response> {
// and let the others through without retrying
//
// throwing in onFailedAttempt will abandon all retries & fail the request
// we only want to retry system errors, so throw a RegistryError for everything else
// we only want to retry system errors, so re-throw for everything else
if (!isSystemError(error)) {
throw new RegistryError(
`Error connecting to package registry at ${url}: ${error.message}`
);
throw error;
}
},
});
return response;
} catch (e) {
throw new RegistryError(`Error connecting to package registry at ${url}: ${e.message}`);
} catch (error) {
// throw same error type whether isSystemError (after max retry attempts) or something else
throw new RegistryConnectionError(error);
}
}

Expand Down

0 comments on commit 907c761

Please sign in to comment.