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

[Actions] adds a Test Connector tab in the Connectors list #77365

Merged
merged 13 commits into from
Sep 22, 2020
10 changes: 10 additions & 0 deletions x-pack/plugins/actions/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,13 @@ export interface ActionResult {
config: Record<string, any>;
isPreconfigured: boolean;
}

// the result returned from an action type executor function
export interface ActionTypeExecutorResult<Data> {
actionId: string;
status: 'ok' | 'error';
message?: string;
serviceMessage?: string;
data?: Data;
retry?: null | boolean | Date;
}
Original file line number Diff line number Diff line change
Expand Up @@ -284,4 +284,47 @@ describe('execute()', () => {
]
`);
});

test('resolves with an error when an error occurs in the indexing operation', async () => {
const secrets = {};
// minimal params
const config = { index: 'index-value', refresh: false, executionTimeField: null };
const params = {
documents: [{ '': 'bob' }],
};

const actionId = 'some-id';

services.callCluster.mockResolvedValue({
took: 0,
errors: true,
items: [
{
index: {
_index: 'indexme',
_id: '7buTjHQB0SuNSiS9Hayt',
status: 400,
error: {
type: 'mapper_parsing_exception',
reason: 'failed to parse',
caused_by: {
type: 'illegal_argument_exception',
reason: 'field name cannot be an empty string',
},
},
},
},
],
});

expect(await actionType.executor({ actionId, config, secrets, params, services }))
.toMatchInlineSnapshot(`
Object {
"actionId": "some-id",
"message": "error indexing documents",
"serviceMessage": "failed to parse (field name cannot be an empty string)",
"status": "error",
}
`);
});
});
18 changes: 13 additions & 5 deletions x-pack/plugins/actions/server/builtin_action_types/es_index.ts
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 { curry } from 'lodash';
import { curry, find } from 'lodash';
import { i18n } from '@kbn/i18n';
import { schema, TypeOf } from '@kbn/config-schema';

Expand Down Expand Up @@ -85,9 +85,19 @@ async function executor(
refresh: config.refresh,
};

let result;
try {
result = await services.callCluster('bulk', bulkParams);
const result = await services.callCluster('bulk', bulkParams);

const err = find(result.items, 'index.error.reason');
if (err) {
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're throwing here instead of passing back a { status: 'error', ... } object? I'm sure it will be handled fine, but I guess I tend to want to have action executors try to catch "common" errors and return the error status object - some day we may want to reserve catching errors in the caller as some kind of "misbehaving action executor" or such ...

Copy link
Member

Choose a reason for hiding this comment

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

BTW, curious how you found this - seems like a general design bug with this action (not checking the bulk results for individual errors), don't remember seeing an issue for it tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, curious how you found this - seems like a general design bug with this action (not checking the bulk results for individual errors), don't remember seeing an issue for it tho.

Haha because it didn't fail! We swallowed the error and told the user it passed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason we're throwing here instead of passing back a { status: 'error', ... } object? I'm sure it will be handled fine, but I guess I tend to want to have action executors try to catch "common" errors and return the error status object - some day we may want to reserve catching errors in the caller as some kind of "misbehaving action executor" or such ...

Yeah, I considered that and decided to only have one error handling block, but happy to change that 👍

`${err.index.error!.reason}${
err.index.error?.caused_by ? ` (${err.index.error?.caused_by?.reason})` : ''
}`
);
}

return { status: 'ok', data: result, actionId };
} catch (err) {
const message = i18n.translate('xpack.actions.builtin.esIndex.errorIndexingErrorMessage', {
defaultMessage: 'error indexing documents',
Expand All @@ -100,6 +110,4 @@ async function executor(
serviceMessage: err.message,
};
}

return { status: 'ok', data: result, actionId };
}
3 changes: 1 addition & 2 deletions x-pack/plugins/actions/server/routes/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import {
} from 'kibana/server';
import { ILicenseState, verifyApiAccess, isErrorThatHandlesItsOwnResponse } from '../lib';

import { ActionTypeExecutorResult } from '../types';
import { BASE_ACTION_API_PATH } from '../../common';
import { BASE_ACTION_API_PATH, ActionTypeExecutorResult } from '../../common';

const paramSchema = schema.object({
id: schema.string(),
Expand Down
12 changes: 2 additions & 10 deletions x-pack/plugins/actions/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
SavedObjectsClientContract,
SavedObjectAttributes,
} from '../../../../src/core/server';
import { ActionTypeExecutorResult } from '../common';
export { ActionTypeExecutorResult } from '../common';

export type WithoutQueryAndParams<T> = Pick<T, Exclude<keyof T, 'query' | 'params'>>;
export type GetServicesFunction = (request: KibanaRequest) => Services;
Expand Down Expand Up @@ -80,16 +82,6 @@ export interface FindActionResult extends ActionResult {
referencedByCount: number;
}

// the result returned from an action type executor function
export interface ActionTypeExecutorResult<Data> {
actionId: string;
status: 'ok' | 'error';
message?: string;
serviceMessage?: string;
data?: Data;
retry?: null | boolean | Date;
}

// signature of the action type executor function
export type ExecutorType<Config, Secrets, Params, ResultData> = (
options: ActionTypeExecutorOptions<Config, Secrets, Params>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export const AddMessageVariables: React.FunctionComponent<Props> = ({
<EuiButtonIcon
id={`${paramsProperty}AddVariableButton`}
data-test-subj={`${paramsProperty}AddVariableButton`}
isDisabled={(messageVariables?.length ?? 0) === 0}
title={addVariableButtonTitle}
onClick={() => setIsVariablesPopoverOpen(true)}
iconType="indexOpen"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,48 +32,47 @@ export const IndexParamsFields = ({
};

return (
<>
<JsonEditorWithMessageVariables
messageVariables={messageVariables}
paramsProperty={'documents'}
inputTargetValue={
documents && documents.length > 0 ? ((documents[0] as unknown) as string) : undefined
<JsonEditorWithMessageVariables
messageVariables={messageVariables}
paramsProperty={'documents'}
data-test-subj="documentToIndex"
inputTargetValue={
documents && documents.length > 0 ? ((documents[0] as unknown) as string) : undefined
}
label={i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.indexAction.documentsFieldLabel',
{
defaultMessage: 'Document to index',
}
label={i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.indexAction.documentsFieldLabel',
{
defaultMessage: 'Document to index',
}
)}
aria-label={i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.indexAction.jsonDocAriaLabel',
{
defaultMessage: 'Code editor',
}
)}
errors={errors.documents as string[]}
onDocumentsChange={onDocumentsChange}
helpText={
<EuiLink
href={`${docLinks.ELASTIC_WEBSITE_URL}guide/en/kibana/${docLinks.DOC_LINK_VERSION}/index-action-type.html#index-action-configuration`}
target="_blank"
>
<FormattedMessage
id="xpack.triggersActionsUI.components.builtinActionTypes.indexAction.indexDocumentHelpLabel"
defaultMessage="Index document example."
/>
</EuiLink>
)}
aria-label={i18n.translate(
'xpack.triggersActionsUI.components.builtinActionTypes.indexAction.jsonDocAriaLabel',
{
defaultMessage: 'Code editor',
}
onBlur={() => {
if (
!(documents && documents.length > 0 ? ((documents[0] as unknown) as string) : undefined)
) {
// set document as empty to turn on the validation for non empty valid JSON object
onDocumentsChange('{}');
}
}}
/>
</>
)}
errors={errors.documents as string[]}
onDocumentsChange={onDocumentsChange}
helpText={
<EuiLink
href={`${docLinks.ELASTIC_WEBSITE_URL}guide/en/kibana/${docLinks.DOC_LINK_VERSION}/index-action-type.html#index-action-configuration`}
target="_blank"
>
<FormattedMessage
id="xpack.triggersActionsUI.components.builtinActionTypes.indexAction.indexDocumentHelpLabel"
defaultMessage="Index document example."
/>
</EuiLink>
}
onBlur={() => {
if (
!(documents && documents.length > 0 ? ((documents[0] as unknown) as string) : undefined)
) {
// set document as empty to turn on the validation for non empty valid JSON object
onDocumentsChange('{}');
}
}}
/>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
loadActionTypes,
loadAllActions,
updateActionConnector,
executeAction,
} from './action_connector_api';

const http = httpServiceMock.createStartContract();
Expand Down Expand Up @@ -128,3 +129,32 @@ describe('deleteActions', () => {
`);
});
});

describe('executeAction', () => {
test('should call execute API', async () => {
const id = '123';
const params = {
stringParams: 'someString',
numericParams: 123,
};

http.post.mockResolvedValueOnce({
actionId: id,
status: 'ok',
});

const result = await executeAction({ id, http, params });
expect(result).toEqual({
actionId: id,
status: 'ok',
});
expect(http.post.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"/api/actions/action/123/_execute",
Object {
"body": "{\\"params\\":{\\"stringParams\\":\\"someString\\",\\"numericParams\\":123}}",
},
]
`);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { HttpSetup } from 'kibana/public';
import { BASE_ACTION_API_PATH } from '../constants';
import { ActionConnector, ActionConnectorWithoutId, ActionType } from '../../types';
import { ActionTypeExecutorResult } from '../../../../../plugins/actions/common';

export async function loadActionTypes({ http }: { http: HttpSetup }): Promise<ActionType[]> {
return await http.get(`${BASE_ACTION_API_PATH}/list_action_types`);
Expand Down Expand Up @@ -65,3 +66,17 @@ export async function deleteActions({
);
return { successes, errors };
}

export async function executeAction({
id,
params,
http,
}: {
id: string;
http: HttpSetup;
params: Record<string, unknown>;
}): Promise<ActionTypeExecutorResult<unknown>> {
return await http.post(`${BASE_ACTION_API_PATH}/action/${id}/_execute`, {
body: JSON.stringify({ params }),
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.connectorEditFlyoutTabs {
margin-bottom: '-25px';
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,6 @@ describe('connector_edit_flyout', () => {

const preconfiguredBadge = wrapper.find('[data-test-subj="preconfiguredBadge"]');
expect(preconfiguredBadge.exists()).toBeTruthy();
expect(wrapper.find('[data-test-subj="saveEditedActionButton"]').exists()).toBeFalsy();
expect(wrapper.find('[data-test-subj="saveAndCloseEditedActionButton"]').exists()).toBeFalsy();
});
});
Loading