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.

Sign up for GitHub

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 new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Merged
merged 23 commits into from
Aug 10, 2021
Merged

Adds new SavedObjectsRespository error type for 404 that do not originate from Elasticsearch responses #107301

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
81d435d
Adds generic not found ES unavailable error, updates ES client mock
TinaHeiligers Jul 30, 2021
98a785f
Adds response header check
TinaHeiligers Jul 30, 2021
7c52b54
clean up
TinaHeiligers Jul 30, 2021
e173b77
Updates server API docs to include new error type
TinaHeiligers Jul 30, 2021
5614cda
Cleans up commented out code
TinaHeiligers Jul 30, 2021
7d00fdd
Merge branch 'master' into so-errors/identify-not-found-form-esunavai…
kibanamachine Aug 2, 2021
e45920d
removes case sensititivy check on headers
TinaHeiligers Aug 3, 2021
449dda6
Fixes code comment
TinaHeiligers Aug 3, 2021
b20e9c2
Improves elasticsearch client mocks types
TinaHeiligers Aug 3, 2021
0befcbd
Merge branch 'master' into so-errors/identify-not-found-form-esunavai…
kibanamachine Aug 4, 2021
9ab33bb
Addresses PR review comments
TinaHeiligers Aug 4, 2021
987c941
Updates tests
TinaHeiligers Aug 5, 2021
dc5dc67
Adds comment to isSupportedEsServer
TinaHeiligers Aug 5, 2021
0edd561
Merge branch 'master' into so-errors/identify-not-found-form-esunavai…
TinaHeiligers Aug 5, 2021
b9955ae
Merge branch 'master' of github.com:elastic/kibana into so-errors/ide…
TinaHeiligers Aug 5, 2021
35cf9d0
Updates tests
TinaHeiligers Aug 5, 2021
81d7571
Updates API docs
TinaHeiligers Aug 5, 2021
9cbbb57
Merge branch 'master' into so-errors/identify-not-found-form-esunavai…
kibanamachine Aug 8, 2021
4b9bfc0
Removes isNotFoundEsUnavailableError
TinaHeiligers Aug 9, 2021
e720a62
updates code comment
TinaHeiligers Aug 9, 2021
55aabbf
Moves response header check to elasticsearch service
TinaHeiligers Aug 9, 2021
2af49d3
Updates API docs
TinaHeiligers Aug 9, 2021
2f76296
Merge branch 'master' into so-errors/identify-not-found-form-esunavai…
kibanamachine Aug 9, 2021
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
23 changes: 23 additions & 0 deletions ...core-server.savedobjectserrorhelpers.creategenericnotfoundesunavailableerror.md
View file
Open in desktop
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`

22 changes: 22 additions & 0 deletions ...ana-plugin-core-server.savedobjectserrorhelpers.isnotfoundesunavailableerror.md
View file
Open in desktop
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`

2 changes: 2 additions & 0 deletions docs/development/core/server/kibana-plugin-core-server.savedobjectserrorhelpers.md
View file
Open in desktop
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
11 changes: 6 additions & 5 deletions src/core/server/elasticsearch/client/mocks.ts
View file
Open in desktop
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 } = {}
{ statusCode = 200 }: { statusCode?: number } = {},
headers?: U
): MockedTransportRequestPromise<ApiResponse<T>> => {
const response = createApiResponse({ body, statusCode });
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>>;
mshustov marked this conversation as resolved.
Show resolved Hide resolved
};

const createErrorTransportRequestPromise = (err: any): MockedTransportRequestPromise<never> => {
Expand Down
51 changes: 51 additions & 0 deletions src/core/server/saved_objects/service/lib/errors.test.ts
View file
Open in desktop
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
View file
Open in desktop
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(
type: string | null = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

under what circumstances can they be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! This PR only adds the checks to get, update and delete, all of which require a type and id.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've made type and id required for this PR.

under what circumstances can they be null

They would be null if we were to implement createGenericNotFoundEsUnavailableError when the x-product-elasticsearch header is missing or has the wrong value in the response from openPointInTimeForType:

Before:

    const { body, statusCode } = await this.client.openPointInTime(esOptions, {
      ignore: [404],
    });
    if (statusCode === 404) {
      throw SavedObjectsErrorHelpers.createGenericNotFoundError();
    }

After:

    const { body, statusCode, headers } = await this.client.openPointInTime(esOptions, {
      ignore: [404],
    });
    if (statusCode === 404) {
    // throw if we can't verify the response is from Elasticsearch
      if (!isSupportedEsServer(headers)) {
        throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError();
      } else {
        throw SavedObjectsErrorHelpers.createGenericNotFoundError();
      }
    }

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.

Copied self-review comment:

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) {
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 this API? If we want plugins to treat this error as any other ES not found issue, I think it'd be ok to only include the factory function createGenericNotFoundEsUnavailableError so that plugins don't waste time trying to figure out how they should handle this error in a different way.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm actually using the API within the repository to verify error types here. It also ensures we stay consistent with repository error type checks, such as isNotFoundError, with the additional check on the error reason.

isNotFoundEsUnavailableError isn't an error itself, it returns a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but as a plugin developer, I'd want to handle each error exposed on SavedObjectsErrorHelpers.is* in order to make my code bulletproof. I'd expect that each of these is*Error methods to only correspond to one error type and not overlap, but now that is no longer the case since both isEsUnavailableError and isNotFoundEsUnavailableError could return true on the same error. I'd also expect that each error type would correspond to something different I could do to handle this, however the underlying problem for both of these errors is the same and there's really nothing different I can/should do as a developer for the isNotFoundEsUnavailableError case vs the isEsUnavailable one.

Are you planning to add similar new error guards for each method as part of #107343? Personally, I think we should only expose isEsUnavailable for all of these situations to signal to plugins that they should handle this error in the same way for every method.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Aug 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I think we should only expose isEsUnavailable for all of these situations to signal to plugins that they should handle this error in the same way for every method.

That makes sense. I've removed isNotFoundEsUnavailableError.

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.

Copied self-review comment:

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`)
);
}
}
Loading