Skip to content

Commit

Permalink
Add generics to ActionType for ActionTypeExecutorResult
Browse files Browse the repository at this point in the history
  • Loading branch information
@mikecote
mikecote committed Jul 23, 2020
1 parent 35f95f0 commit ab64713
Show file tree
Hide file tree
Showing 22 changed files with 93 additions and 62 deletions.
10 changes: 7 additions & 3 deletions x-pack/plugins/actions/server/action_type_registry.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ beforeEach(() => {
};
});

const executor: ExecutorType<{}, {}, {}> = async (options) => {
const executor: ExecutorType<{}, {}, {}, void> = async (options) => {
return { status: 'ok', actionId: options.actionId };
};

Expand Down Expand Up @@ -203,7 +203,9 @@ describe('isActionTypeEnabled', () => {
id: 'foo',
name: 'Foo',
minimumLicenseRequired: 'basic',
executor: async () => {},
executor: async (options) => {

This comment has been minimized.

Sign in to view

Copy link
@pmuellr

pmuellr Jul 23, 2020

Member

just. a note that we should have a test that returns undefined, somewhere, as I think we try to cope with this situation in the calling code - in case the action did the wrong thing. Obviously having this typed helps, maybe enough that we don't need that extra protection anymore?

This comment has been minimized.

Sign in to view

Copy link
@mikecote

mikecote Jul 23, 2020

Author Contributor

I think having this typed helps enough that I don't think there is need for the extra protection anymore. The developer would have to work around the types to be able to return undefined or use a JS file which they'll have to understand the expected shapes.

This comment has been minimized.

Sign in to view

Copy link
@mikecote

mikecote Jul 23, 2020

Author Contributor

On the same note, I did find a unit test in the routes that does this: https://github.com/elastic/kibana/blob/master/x-pack/plugins/actions/server/routes/execute.test.ts#L74. It shows what you have to do to work around the types.
return { status: 'ok', actionId: options.actionId };
},
};

beforeEach(() => {
Expand Down Expand Up @@ -258,7 +260,9 @@ describe('ensureActionTypeEnabled', () => {
id: 'foo',
name: 'Foo',
minimumLicenseRequired: 'basic',
executor: async () => {},
executor: async (options) => {
return { status: 'ok', actionId: options.actionId };
},
};

beforeEach(() => {
Expand Down
12 changes: 7 additions & 5 deletions x-pack/plugins/actions/server/action_type_registry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ export class ActionTypeRegistry {
public register<
Config extends ActionTypeConfig = ActionTypeConfig,
Secrets extends ActionTypeSecrets = ActionTypeSecrets,
Params extends ActionTypeParams = ActionTypeParams
>(actionType: ActionType<Config, Secrets, Params>) {
Params extends ActionTypeParams = ActionTypeParams,
ExecutorResultData = void
>(actionType: ActionType<Config, Secrets, Params, ExecutorResultData>) {
if (this.has(actionType.id)) {
throw new Error(
i18n.translate(
Expand Down Expand Up @@ -125,8 +126,9 @@ export class ActionTypeRegistry {
public get<
Config extends ActionTypeConfig = ActionTypeConfig,
Secrets extends ActionTypeSecrets = ActionTypeSecrets,
Params extends ActionTypeParams = ActionTypeParams
>(id: string): ActionType<Config, Secrets, Params> {
Params extends ActionTypeParams = ActionTypeParams,
ExecutorResultData = void
>(id: string): ActionType<Config, Secrets, Params, ExecutorResultData> {
if (!this.has(id)) {
throw Boom.badRequest(
i18n.translate('xpack.actions.actionTypeRegistry.get.missingActionTypeErrorMessage', {
Expand All @@ -137,7 +139,7 @@ export class ActionTypeRegistry {
})
);
}
return this.actionTypes.get(id)! as ActionType<Config, Secrets, Params>;
return this.actionTypes.get(id)! as ActionType<Config, Secrets, Params, ExecutorResultData>;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/actions/server/actions_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ let actionsClient: ActionsClient;
let mockedLicenseState: jest.Mocked<ILicenseState>;
let actionTypeRegistry: ActionTypeRegistry;
let actionTypeRegistryParams: ActionTypeRegistryOpts;
const executor: ExecutorType<{}, {}, {}> = async (options) => {
const executor: ExecutorType<{}, {}, {}, void> = async (options) => {
return { status: 'ok', actionId: options.actionId };
};

Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/actions/server/actions_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ export class ActionsClient {
public async execute({
actionId,
params,
}: Omit<ExecuteOptions, 'request'>): Promise<ActionTypeExecutorResult> {
}: Omit<ExecuteOptions, 'request'>): Promise<ActionTypeExecutorResult<unknown>> {
await this.authorization.ensureAuthorized('execute');
return this.actionExecutor.execute({ actionId, params, request: this.request });
}
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/actions/server/builtin_action_types/case/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
TransformFieldsArgs,
Comment,
ExecutorSubActionPushParams,
PushToServiceResponse,
} from './types';

import { transformers } from './transformers';
Expand Down Expand Up @@ -72,13 +73,12 @@ export const createConnectorExecutor = ({
ExternalIncidentServiceSecretConfiguration,
ExecutorParams
>
): Promise<ActionTypeExecutorResult> => {
): Promise<ActionTypeExecutorResult<PushToServiceResponse | {}>> => {
const { actionId, config, params, secrets } = execOptions;
const { subAction, subActionParams } = params;
let data = {};

const res: Pick<ActionTypeExecutorResult, 'status'> &
Pick<ActionTypeExecutorResult, 'actionId'> = {
const res: ActionTypeExecutorResult<void> = {
status: 'ok',
actionId,
};
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/email.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import { ActionsConfigurationUtilities } from '../actions_config';
export type EmailActionType = ActionType<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType
ActionParamsType,
unknown
>;
export type EmailActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
Expand Down Expand Up @@ -147,7 +148,7 @@ export function getActionType(params: GetActionTypeParams): EmailActionType {
async function executor(
{ logger }: { logger: Logger },
execOptions: EmailActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const config = execOptions.config;
const secrets = execOptions.secrets;
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/es_index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { schema, TypeOf } from '@kbn/config-schema';
import { Logger } from '../../../../../src/core/server';
import { ActionType, ActionTypeExecutorOptions, ActionTypeExecutorResult } from '../types';

export type ESIndexActionType = ActionType<ActionTypeConfigType, {}, ActionParamsType>;
export type ESIndexActionType = ActionType<ActionTypeConfigType, {}, ActionParamsType, unknown>;
export type ESIndexActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
{},
Expand Down Expand Up @@ -60,7 +60,7 @@ export function getActionType({ logger }: { logger: Logger }): ESIndexActionType
async function executor(
{ logger }: { logger: Logger },
execOptions: ESIndexActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const config = execOptions.config;
const params = execOptions.params;
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/pagerduty.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ const PAGER_DUTY_API_URL = 'https://events.pagerduty.com/v2/enqueue';
export type PagerDutyActionType = ActionType<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType
ActionParamsType,
unknown
>;
export type PagerDutyActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
Expand Down Expand Up @@ -154,7 +155,7 @@ function getPagerDutyApiUrl(config: ActionTypeConfigType): string {
async function executor(
{ logger }: { logger: Logger },
execOptions: PagerDutyActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const config = execOptions.config;
const secrets = execOptions.secrets;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/actions/server/builtin_action_types/server_log.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export function getActionType({ logger }: { logger: Logger }): ServerLogActionTy
async function executor(
{ logger }: { logger: Logger },
execOptions: ServerLogActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<void>> {

This comment has been minimized.

Sign in to view

Copy link
@pmuellr

pmuellr Jul 23, 2020

Member

well now I just feel bad/sad for having you add this typing, when it's mostly void and unknown 😭
const actionId = execOptions.actionId;
const params = execOptions.params;

Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/servicenow/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ export function getActionType(
): ActionType<
ServiceNowPublicConfigurationType,
ServiceNowSecretConfigurationType,
ExecutorParams
ExecutorParams,
PushToServiceResponse | {}
> {
const { logger, configurationUtilities } = params;
return {
Expand Down Expand Up @@ -70,7 +71,7 @@ async function executor(
ServiceNowSecretConfigurationType,
ExecutorParams
>
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<PushToServiceResponse | {}>> {
const { actionId, config, params, secrets } = execOptions;
const { subAction, subActionParams } = params;
let data: PushToServiceResponse | null = null;
Expand Down
6 changes: 4 additions & 2 deletions x-pack/plugins/actions/server/builtin_action_types/slack.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ let actionType: SlackActionType;

beforeAll(() => {
actionType = getActionType({
async executor() {},
async executor(options) {
return { status: 'ok', actionId: options.actionId };
},
configurationUtilities: actionsConfigMock.create(),
});
});
Expand Down Expand Up @@ -129,7 +131,7 @@ describe('execute()', () => {
text: `slack mockExecutor success: ${message}`,
actionId: '',
status: 'ok',
} as ActionTypeExecutorResult;
} as ActionTypeExecutorResult<void>;
}

actionType = getActionType({
Expand Down
19 changes: 11 additions & 8 deletions x-pack/plugins/actions/server/builtin_action_types/slack.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
} from '../types';
import { ActionsConfigurationUtilities } from '../actions_config';

export type SlackActionType = ActionType<{}, ActionTypeSecretsType, ActionParamsType>;
export type SlackActionType = ActionType<{}, ActionTypeSecretsType, ActionParamsType, unknown>;
export type SlackActionTypeExecutorOptions = ActionTypeExecutorOptions<
{},
ActionTypeSecretsType,
Expand Down Expand Up @@ -53,7 +53,7 @@ export function getActionType({
executor = slackExecutor,
}: {
configurationUtilities: ActionsConfigurationUtilities;
executor?: ExecutorType<{}, ActionTypeSecretsType, ActionParamsType>;
executor?: ExecutorType<{}, ActionTypeSecretsType, ActionParamsType, unknown>;
}): SlackActionType {
return {
id: '.slack',
Expand Down Expand Up @@ -100,7 +100,7 @@ function valdiateActionTypeConfig(

async function slackExecutor(
execOptions: SlackActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const secrets = execOptions.secrets;
const params = execOptions.params;
Expand Down Expand Up @@ -163,18 +163,21 @@ async function slackExecutor(
return successResult(actionId, result);
}

function successResult(actionId: string, data: unknown): ActionTypeExecutorResult {
function successResult(actionId: string, data: unknown): ActionTypeExecutorResult<unknown> {
return { status: 'ok', data, actionId };
}

function errorResult(actionId: string, message: string): ActionTypeExecutorResult {
function errorResult(actionId: string, message: string): ActionTypeExecutorResult<void> {
return {
status: 'error',
message,
actionId,
};
}
function serviceErrorResult(actionId: string, serviceMessage: string): ActionTypeExecutorResult {
function serviceErrorResult(
actionId: string,
serviceMessage: string
): ActionTypeExecutorResult<void> {
const errMessage = i18n.translate('xpack.actions.builtin.slack.errorPostingErrorMessage', {
defaultMessage: 'error posting slack message',
});
Expand All @@ -186,7 +189,7 @@ function serviceErrorResult(actionId: string, serviceMessage: string): ActionTyp
};
}

function retryResult(actionId: string, message: string): ActionTypeExecutorResult {
function retryResult(actionId: string, message: string): ActionTypeExecutorResult<void> {
const errMessage = i18n.translate(
'xpack.actions.builtin.slack.errorPostingRetryLaterErrorMessage',
{
Expand All @@ -205,7 +208,7 @@ function retryResultSeconds(
actionId: string,
message: string,
retryAfter: number
): ActionTypeExecutorResult {
): ActionTypeExecutorResult<void> {
const retryEpoch = Date.now() + retryAfter * 1000;
const retry = new Date(retryEpoch);
const retryString = retry.toISOString();
Expand Down
18 changes: 11 additions & 7 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ export enum WebhookMethods {
export type WebhookActionType = ActionType<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType
ActionParamsType,
unknown
>;
export type WebhookActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
Expand Down Expand Up @@ -124,7 +125,7 @@ function validateActionTypeConfig(
export async function executor(
{ logger }: { logger: Logger },
execOptions: WebhookActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const { method, url, headers = {} } = execOptions.config;
const { body: data } = execOptions.params;
Expand Down Expand Up @@ -183,11 +184,14 @@ export async function executor(
}

// Action Executor Result w/ internationalisation
function successResult(actionId: string, data: unknown): ActionTypeExecutorResult {
function successResult(actionId: string, data: unknown): ActionTypeExecutorResult<unknown> {
return { status: 'ok', data, actionId };
}

function errorResultInvalid(actionId: string, serviceMessage: string): ActionTypeExecutorResult {
function errorResultInvalid(
actionId: string,
serviceMessage: string
): ActionTypeExecutorResult<void> {
const errMessage = i18n.translate('xpack.actions.builtin.webhook.invalidResponseErrorMessage', {
defaultMessage: 'error calling webhook, invalid response',
});
Expand All @@ -199,7 +203,7 @@ function errorResultInvalid(actionId: string, serviceMessage: string): ActionTyp
};
}

function errorResultUnexpectedError(actionId: string): ActionTypeExecutorResult {
function errorResultUnexpectedError(actionId: string): ActionTypeExecutorResult<void> {
const errMessage = i18n.translate('xpack.actions.builtin.webhook.unreachableErrorMessage', {
defaultMessage: 'error calling webhook, unexpected error',
});
Expand All @@ -210,7 +214,7 @@ function errorResultUnexpectedError(actionId: string): ActionTypeExecutorResult
};
}

function retryResult(actionId: string, serviceMessage: string): ActionTypeExecutorResult {
function retryResult(actionId: string, serviceMessage: string): ActionTypeExecutorResult<void> {
const errMessage = i18n.translate(
'xpack.actions.builtin.webhook.invalidResponseRetryLaterErrorMessage',
{
Expand All @@ -231,7 +235,7 @@ function retryResultSeconds(
serviceMessage: string,

retryAfter: number
): ActionTypeExecutorResult {
): ActionTypeExecutorResult<void> {
const retryEpoch = Date.now() + retryAfter * 1000;
const retry = new Date(retryEpoch);
const retryString = retry.toISOString();
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/actions/server/lib/action_executor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class ActionExecutor {
actionId,
params,
request,
}: ExecuteOptions): Promise<ActionTypeExecutorResult> {
}: ExecuteOptions): Promise<ActionTypeExecutorResult<unknown>> {
if (!this.isInitialized) {
throw new Error('ActionExecutor not initialized');
}
Expand Down Expand Up @@ -125,7 +125,7 @@ export class ActionExecutor {
};

eventLogger.startTiming(event);
let rawResult: ActionTypeExecutorResult | null | undefined | void;
let rawResult: ActionTypeExecutorResult<unknown>;
try {
rawResult = await actionType.executor({
actionId,
Expand Down Expand Up @@ -173,7 +173,7 @@ export class ActionExecutor {
}
}

function actionErrorToMessage(result: ActionTypeExecutorResult): string {
function actionErrorToMessage(result: ActionTypeExecutorResult<unknown>): string {
let message = result.message || 'unknown error running action';

if (result.serviceMessage) {
Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugins/actions/server/lib/license_state.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ describe('isLicenseValidForActionType', () => {
id: 'foo',
name: 'Foo',
minimumLicenseRequired: 'gold',
executor: async () => {},
executor: async (options) => {
return { status: 'ok', actionId: options.actionId };
},
};

beforeEach(() => {
Expand Down Expand Up @@ -120,7 +122,9 @@ describe('ensureLicenseForActionType()', () => {
id: 'foo',
name: 'Foo',
minimumLicenseRequired: 'gold',
executor: async () => {},
executor: async (options) => {
return { status: 'ok', actionId: options.actionId };
},
};

beforeEach(() => {
Expand Down
Loading

0 comments on commit ab64713

Please sign in to comment.