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

Adds explicit SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107104

Closed
Show file tree
Hide file tree
Changes from 6 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
@@ -0,0 +1,23 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsErrorHelpers](./kibana-plugin-core-server.savedobjectserrorhelpers.md) &gt; [createGenericNotFoundEsUnavailableError](./kibana-plugin-core-server.savedobjectserrorhelpers.creategenericnotfoundesunavailableerror.md)

## SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError() method

<b>Signature:</b>

```typescript
static createGenericNotFoundEsUnavailableError(type?: string | null, id?: string | null): DecoratedError;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| type | <code>string &#124; null</code> | |
| id | <code>string &#124; null</code> | |

<b>Returns:</b>

`DecoratedError`

Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsErrorHelpers](./kibana-plugin-core-server.savedobjectserrorhelpers.md) &gt; [isNotFoundEsUnavailableError](./kibana-plugin-core-server.savedobjectserrorhelpers.isnotfoundesunavailableerror.md)

## SavedObjectsErrorHelpers.isNotFoundEsUnavailableError() method

<b>Signature:</b>

```typescript
static isNotFoundEsUnavailableError(error: Error | DecoratedError): boolean;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| error | <code>Error &#124; DecoratedError</code> | |

<b>Returns:</b>

`boolean`

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export declare class SavedObjectsErrorHelpers
| [createBadRequestError(reason)](./kibana-plugin-core-server.savedobjectserrorhelpers.createbadrequesterror.md) | <code>static</code> | |
| [createConflictError(type, id, reason)](./kibana-plugin-core-server.savedobjectserrorhelpers.createconflicterror.md) | <code>static</code> | |
| [createGenericNotFoundError(type, id)](./kibana-plugin-core-server.savedobjectserrorhelpers.creategenericnotfounderror.md) | <code>static</code> | |
| [createGenericNotFoundEsUnavailableError(type, id)](./kibana-plugin-core-server.savedobjectserrorhelpers.creategenericnotfoundesunavailableerror.md) | <code>static</code> | |
| [createIndexAliasNotFoundError(alias)](./kibana-plugin-core-server.savedobjectserrorhelpers.createindexaliasnotfounderror.md) | <code>static</code> | |
| [createInvalidVersionError(versionInput)](./kibana-plugin-core-server.savedobjectserrorhelpers.createinvalidversionerror.md) | <code>static</code> | |
| [createTooManyRequestsError(type, id)](./kibana-plugin-core-server.savedobjectserrorhelpers.createtoomanyrequestserror.md) | <code>static</code> | |
Expand All @@ -41,6 +42,7 @@ export declare class SavedObjectsErrorHelpers
| [isInvalidVersionError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.isinvalidversionerror.md) | <code>static</code> | |
| [isNotAuthorizedError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.isnotauthorizederror.md) | <code>static</code> | |
| [isNotFoundError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.isnotfounderror.md) | <code>static</code> | |
| [isNotFoundEsUnavailableError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.isnotfoundesunavailableerror.md) | <code>static</code> | |
| [isRequestEntityTooLargeError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.isrequestentitytoolargeerror.md) | <code>static</code> | |
| [isSavedObjectsClientError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.issavedobjectsclienterror.md) | <code>static</code> | |
| [isTooManyRequestsError(error)](./kibana-plugin-core-server.savedobjectserrorhelpers.istoomanyrequestserror.md) | <code>static</code> | |
Expand Down
13 changes: 7 additions & 6 deletions src/core/server/elasticsearch/client/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,16 @@ export type MockedTransportRequestPromise<T> = TransportRequestPromise<T> & {
abort: jest.MockedFunction<() => undefined>;
};

const createSuccessTransportRequestPromise = <T>(
const createSuccessTransportRequestPromise = <T, U>(
body: T,
{ statusCode = 200 }: { statusCode?: number } = {}
): MockedTransportRequestPromise<ApiResponse<T>> => {
const response = createApiResponse({ body, statusCode });
{ statusCode = 200 }: { statusCode?: number } = {},
headers?: U
): MockedTransportRequestPromise<ApiResponse<T, U>> => {
const response = createApiResponse({ body, statusCode, headers });
const promise = Promise.resolve(response);
(promise as MockedTransportRequestPromise<ApiResponse<T>>).abort = jest.fn();
(promise as MockedTransportRequestPromise<ApiResponse<T, U>>).abort = jest.fn();

return promise as MockedTransportRequestPromise<ApiResponse<T>>;
return promise as MockedTransportRequestPromise<ApiResponse<T, U>>;
};

const createErrorTransportRequestPromise = (err: any): MockedTransportRequestPromise<never> => {
Expand Down
51 changes: 51 additions & 0 deletions src/core/server/saved_objects/service/lib/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -439,4 +439,55 @@ describe('savedObjectsClient/errorTypes', () => {
});
});
});

describe('NotFoundEsUnavailableError', () => {
it('makes an error identifiable as an EsUnavailable error', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
expect(SavedObjectsErrorHelpers.isEsUnavailableError(error)).toBe(true);
});

it('makes an error identifiable as an NotFoundEsUnavailableError error', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
expect(SavedObjectsErrorHelpers.isNotFoundEsUnavailableError(error)).toBe(true);
});

it('returns a boom error', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
expect(error).toHaveProperty('isBoom', true);
});

it('decorates the error message with the saved object that was not found', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError('foo', 'bar');
expect(error.output.payload).toHaveProperty(
'message',
'x-elastic-product not present or not recognized: Saved object [foo/bar] not found'
);
});

describe('error.output', () => {
it('prefixes Not Found message with passed reason', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
expect(error.output.payload).toHaveProperty(
'message',
'x-elastic-product not present or not recognized: Not Found'
);
});

it('specifies the saved object that was not found', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(
'foo',
'bar'
);
expect(error.output.payload).toHaveProperty(
'message',
'x-elastic-product not present or not recognized: Saved object [foo/bar] not found'
);
});

it('sets statusCode to 503', () => {
const error = SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
expect(error.output).toHaveProperty('statusCode', 503);
});
});
});
});
19 changes: 19 additions & 0 deletions src/core/server/saved_objects/service/lib/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,4 +202,23 @@ export class SavedObjectsErrorHelpers {
public static isGeneralError(error: Error | DecoratedError) {
return isSavedObjectsClientError(error) && error[code] === CODE_GENERAL_ERROR;
}

public static createGenericNotFoundEsUnavailableError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to differentiate this error from isEsUnavailableError? I'm inclined to say that we want applications to handle this condition in the same way they would handle any other issue reaching Elasticsearch. I'm not sure if there's anything different a consumer would or can do in this case vs. any other isEsUnavailableError. Another benefit of using the existing error is that any existing code that is handling it will also handle this new case.

Curious what @mshustov thinks as well.

Copy link
Contributor

@mshustov mshustov Jul 29, 2021

Choose a reason for hiding this comment

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

Agree, it's ES server is unavailable rather than a Kibana entity is not found.
A user reacts to them in different ways: in the case of a Kibana entity is not found, the user needs to adjust the request params and try again.
While in the case of ES server is unavailable, the error is not actionable: the user needs to contact an admin to address the availability problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I did try throwing isEsUnavailableError but that returns a boolean, not an actual Error or DecoratedError. If you mean that we should rather throw a decorated EsUnavailableError that will return true on checking that the error is isEsUnavailable, then we can do that.
If I'm mis-reading the types here, please let me know!.

Copy link
Contributor

Choose a reason for hiding this comment

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

f you mean that we should rather throw a decorated EsUnavailableError that will return true on checking that the error is isEsUnavailable, then we can do that.

Yeah, I was thinking about something like this. @joshdover Did you mean the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially replied with this but then deleted it again. I'm adding it back for more context on the error that I'm using:

createGenericNotFoundEsUnavailableError will return a 503 error that adds the message from a NotFoundError to the EsUnavailableError. The decorated error that's thrown will assert true to a check for isEsUnavailableError, since the latter returns a boolean for the statusCode being 503.

type: string | null = null,
id: string | null = null
) {
const notFoundError = this.createGenericNotFoundError(type, id);
return this.decorateEsUnavailableError(
new Error(`${notFoundError.message}`),
`x-elastic-product not present or not recognized`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is the best text string to use as a descriptor for the "missing" header. I took inspiration from the client's team but made it a lot more specific. We need to get Product's input on wording if we choose to follow this implementation.

);
}

public static isNotFoundEsUnavailableError(error: Error | DecoratedError) {
return (
isSavedObjectsClientError(error) &&
error[code] === CODE_ES_UNAVAILABLE &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically set a status code of 503 to indicate an ES availability error. This overrides the 404 we would otherwise have thrown.

error.message.startsWith('x-elastic-product not present or not recognized:')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need : at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Good catch!

);
}
}
Loading