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

Make the actions plugin support generics #71439

Merged
merged 18 commits into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 7 additions & 3 deletions x-pack/plugins/actions/server/action_type_registry.test.ts
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) => {
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
26 changes: 21 additions & 5 deletions x-pack/plugins/actions/server/action_type_registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ import Boom from 'boom';
import { i18n } from '@kbn/i18n';
import { RunContext, TaskManagerSetupContract } from '../../task_manager/server';
import { ExecutorError, TaskRunnerFactory, ILicenseState } from './lib';
import { ActionType, PreConfiguredAction } from './types';
import { ActionType as CommonActionType } from '../common';
import { ActionsConfigurationUtilities } from './actions_config';
import {
ActionType,
PreConfiguredAction,
ActionTypeConfig,
ActionTypeSecrets,
ActionTypeParams,
} from './types';

export interface ActionTypeRegistryOpts {
taskManager: TaskManagerSetupContract;
Expand Down Expand Up @@ -77,7 +83,12 @@ export class ActionTypeRegistry {
/**
* Registers an action type to the action type registry
*/
public register(actionType: ActionType) {
public register<
Config extends ActionTypeConfig = ActionTypeConfig,
Secrets extends ActionTypeSecrets = ActionTypeSecrets,
Params extends ActionTypeParams = ActionTypeParams,
ExecutorResultData = void
>(actionType: ActionType<Config, Secrets, Params, ExecutorResultData>) {
if (this.has(actionType.id)) {
throw new Error(
i18n.translate(
Expand All @@ -91,7 +102,7 @@ export class ActionTypeRegistry {
)
);
}
this.actionTypes.set(actionType.id, { ...actionType });
this.actionTypes.set(actionType.id, { ...actionType } as ActionType);
Copy link
Member

Choose a reason for hiding this comment

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

maybe I haven't gotten to the point where this cast is needed, but I woulda thought we could just do

this.actionTypes.set(actionType.id, actionType);

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 think it's to "cast" it to a generic, otherwise typescript complains that it can't convert Config, Secret and Params type to generics.

this.taskManager.registerTaskDefinitions({
[`actions:${actionType.id}`]: {
title: actionType.name,
Expand All @@ -112,7 +123,12 @@ export class ActionTypeRegistry {
/**
* Returns an action type, throws if not registered
*/
public get(id: string): ActionType {
public get<
Copy link
Member

Choose a reason for hiding this comment

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

Haven't gotten to the point where this would be needed, but wondering what the real use case is to have get() parameterized like this. No harm, just seems like it would only be used in unit tests for actions :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmmorris would know more, he helped me come up with the design. From what I saw, it's the only way we could get it to work as I don't know if you can make an interface extend a generic while also having sub-generic types. (CustomActionType extends ActionType, then setting Config, Secrets, Params generics).

Config extends ActionTypeConfig = ActionTypeConfig,
Secrets extends ActionTypeSecrets = ActionTypeSecrets,
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 @@ -123,7 +139,7 @@ export class ActionTypeRegistry {
})
);
}
return this.actionTypes.get(id)!;
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
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
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
18 changes: 13 additions & 5 deletions x-pack/plugins/actions/server/builtin_action_types/case/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import { schema } from '@kbn/config-schema';
import { ActionTypeExecutorOptions, ActionTypeExecutorResult, ActionType } from '../../types';

import { ExecutorParamsSchema } from './schema';
import {
ExternalIncidentServiceConfiguration,
ExternalIncidentServiceSecretConfiguration,
} from './types';

import {
CreateExternalServiceArgs,
Expand All @@ -23,6 +27,7 @@ import {
TransformFieldsArgs,
Comment,
ExecutorSubActionPushParams,
PushToServiceResponse,
} from './types';

import { transformers } from './transformers';
Expand Down Expand Up @@ -63,14 +68,17 @@ export const createConnectorExecutor = ({
api,
createExternalService,
}: CreateExternalServiceBasicArgs) => async (
execOptions: ActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> => {
execOptions: ActionTypeExecutorOptions<
ExternalIncidentServiceConfiguration,
ExternalIncidentServiceSecretConfiguration,
ExecutorParams
>
): Promise<ActionTypeExecutorResult<PushToServiceResponse | {}>> => {
const { actionId, config, params, secrets } = execOptions;
const { subAction, subActionParams } = params as ExecutorParams;
const { subAction, subActionParams } = params;
let data = {};

const res: Pick<ActionTypeExecutorResult, 'status'> &
Pick<ActionTypeExecutorResult, 'actionId'> = {
const res: ActionTypeExecutorResult<void> = {
status: 'ok',
actionId,
};
Expand Down
17 changes: 11 additions & 6 deletions x-pack/plugins/actions/server/builtin_action_types/email.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ jest.mock('./lib/send_email', () => ({

import { Logger } from '../../../../../src/core/server';

import { ActionType, ActionTypeExecutorOptions } from '../types';
import { actionsConfigMock } from '../actions_config.mock';
import { validateConfig, validateSecrets, validateParams } from '../lib';
import { createActionTypeRegistry } from './index.test';
Expand All @@ -21,6 +20,8 @@ import {
ActionTypeConfigType,
ActionTypeSecretsType,
getActionType,
EmailActionType,
EmailActionTypeExecutorOptions,
} from './email';

const sendEmailMock = sendEmail as jest.Mock;
Expand All @@ -29,13 +30,17 @@ const ACTION_TYPE_ID = '.email';

const services = actionsMock.createServices();

let actionType: ActionType;
let actionType: EmailActionType;
let mockedLogger: jest.Mocked<Logger>;

beforeEach(() => {
jest.resetAllMocks();
const { actionTypeRegistry } = createActionTypeRegistry();
actionType = actionTypeRegistry.get(ACTION_TYPE_ID);
actionType = actionTypeRegistry.get<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType
>(ACTION_TYPE_ID);
});

describe('actionTypeRegistry.get() works', () => {
Expand Down Expand Up @@ -242,7 +247,7 @@ describe('execute()', () => {
};

const actionId = 'some-id';
const executorOptions: ActionTypeExecutorOptions = {
const executorOptions: EmailActionTypeExecutorOptions = {
actionId,
config,
params,
Expand Down Expand Up @@ -306,7 +311,7 @@ describe('execute()', () => {
};

const actionId = 'some-id';
const executorOptions: ActionTypeExecutorOptions = {
const executorOptions: EmailActionTypeExecutorOptions = {
actionId,
config,
params,
Expand Down Expand Up @@ -363,7 +368,7 @@ describe('execute()', () => {
};

const actionId = 'some-id';
const executorOptions: ActionTypeExecutorOptions = {
const executorOptions: EmailActionTypeExecutorOptions = {
actionId,
config,
params,
Expand Down
29 changes: 20 additions & 9 deletions x-pack/plugins/actions/server/builtin_action_types/email.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ import { Logger } from '../../../../../src/core/server';
import { ActionType, ActionTypeExecutorOptions, ActionTypeExecutorResult } from '../types';
import { ActionsConfigurationUtilities } from '../actions_config';

export type EmailActionType = ActionType<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType,
unknown
>;
export type EmailActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
ActionTypeSecretsType,
ActionParamsType
>;

// config definition
export type ActionTypeConfigType = TypeOf<typeof ConfigSchema>;

Expand All @@ -30,10 +42,9 @@ const ConfigSchema = schema.object(ConfigSchemaProps);

function validateConfig(
configurationUtilities: ActionsConfigurationUtilities,
configObject: unknown
configObject: ActionTypeConfigType
): string | void {
// avoids circular reference ...
const config = configObject as ActionTypeConfigType;
const config = configObject;

// Make sure service is set, or if not, both host/port must be set.
// If service is set, host/port are ignored, when the email is sent.
Expand Down Expand Up @@ -113,7 +124,7 @@ interface GetActionTypeParams {
}

// action type definition
export function getActionType(params: GetActionTypeParams): ActionType {
export function getActionType(params: GetActionTypeParams): EmailActionType {
const { logger, configurationUtilities } = params;
return {
id: '.email',
Expand All @@ -136,12 +147,12 @@ export function getActionType(params: GetActionTypeParams): ActionType {

async function executor(
{ logger }: { logger: Logger },
execOptions: ActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
execOptions: EmailActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const config = execOptions.config as ActionTypeConfigType;
const secrets = execOptions.secrets as ActionTypeSecretsType;
const params = execOptions.params as ActionParamsType;
const config = execOptions.config;
const secrets = execOptions.secrets;
const params = execOptions.params;

const transport: Transport = {};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,25 @@ jest.mock('./lib/send_email', () => ({
sendEmail: jest.fn(),
}));

import { ActionType, ActionTypeExecutorOptions } from '../types';
import { validateConfig, validateParams } from '../lib';
import { createActionTypeRegistry } from './index.test';
import { ActionParamsType, ActionTypeConfigType } from './es_index';
import { actionsMock } from '../mocks';
import {
ActionParamsType,
ActionTypeConfigType,
ESIndexActionType,
ESIndexActionTypeExecutorOptions,
} from './es_index';

const ACTION_TYPE_ID = '.index';

const services = actionsMock.createServices();

let actionType: ActionType;
let actionType: ESIndexActionType;

beforeAll(() => {
const { actionTypeRegistry } = createActionTypeRegistry();
actionType = actionTypeRegistry.get(ACTION_TYPE_ID);
actionType = actionTypeRegistry.get<ActionTypeConfigType, {}, ActionParamsType>(ACTION_TYPE_ID);
});

beforeEach(() => {
Expand Down Expand Up @@ -144,12 +148,12 @@ describe('params validation', () => {
describe('execute()', () => {
test('ensure parameters are as expected', async () => {
const secrets = {};
let config: Partial<ActionTypeConfigType>;
let config: ActionTypeConfigType;
let params: ActionParamsType;
let executorOptions: ActionTypeExecutorOptions;
let executorOptions: ESIndexActionTypeExecutorOptions;

// minimal params
config = { index: 'index-value', refresh: false };
config = { index: 'index-value', refresh: false, executionTimeField: null };
params = {
documents: [{ jim: 'bob' }],
};
Expand Down Expand Up @@ -215,7 +219,7 @@ describe('execute()', () => {
`);

// minimal params
config = { index: 'index-value', executionTimeField: undefined, refresh: false };
config = { index: 'index-value', executionTimeField: null, refresh: false };
params = {
documents: [{ jim: 'bob' }],
};
Expand Down Expand Up @@ -245,7 +249,7 @@ describe('execute()', () => {
`);

// multiple documents
config = { index: 'index-value', executionTimeField: undefined, refresh: false };
config = { index: 'index-value', executionTimeField: null, refresh: false };
params = {
documents: [{ a: 1 }, { b: 2 }],
};
Expand Down
17 changes: 12 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 @@ -11,6 +11,13 @@ 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, unknown>;
export type ESIndexActionTypeExecutorOptions = ActionTypeExecutorOptions<
ActionTypeConfigType,
{},
ActionParamsType
>;

// config definition

export type ActionTypeConfigType = TypeOf<typeof ConfigSchema>;
Expand All @@ -33,7 +40,7 @@ const ParamsSchema = schema.object({
});

// action type definition
export function getActionType({ logger }: { logger: Logger }): ActionType {
export function getActionType({ logger }: { logger: Logger }): ESIndexActionType {
return {
id: '.index',
minimumLicenseRequired: 'basic',
Expand All @@ -52,11 +59,11 @@ export function getActionType({ logger }: { logger: Logger }): ActionType {

async function executor(
{ logger }: { logger: Logger },
execOptions: ActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult> {
execOptions: ESIndexActionTypeExecutorOptions
): Promise<ActionTypeExecutorResult<unknown>> {
const actionId = execOptions.actionId;
const config = execOptions.config as ActionTypeConfigType;
const params = execOptions.params as ActionParamsType;
const config = execOptions.config;
const params = execOptions.params;
const services = execOptions.services;

const index = config.index;
Expand Down
Loading