From d63c225a5cd6d9970a498efcc3f3ab7e288f20e6 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Mon, 1 Jul 2019 14:32:19 -0400 Subject: [PATCH] Changes x-pack action executor to return result. (#39820) (#40053) * Changes x-pack action executor to return result. Previously, the result was never captured. It's now captured, and returned if it's JSONable. If not JSONable a `{succcess: true}` object is returned with a 200 response (used to be 204). Also added logging for action execution calls, in lieu of having an action history available. Drive-by: - `services.log` was set to the unbound Hapi server.log method, so places where it was set were changed to bind it to the server. --- x-pack/legacy/plugins/actions/server/init.ts | 2 +- .../plugins/actions/server/lib/execute.ts | 44 ++++++++++++++++--- .../actions/server/routes/fire.test.ts | 4 +- .../plugins/actions/server/routes/fire.ts | 4 +- x-pack/legacy/plugins/alerting/server/init.ts | 4 +- .../test/api_integration/apis/actions/fire.ts | 8 ++-- 6 files changed, 50 insertions(+), 16 deletions(-) diff --git a/x-pack/legacy/plugins/actions/server/init.ts b/x-pack/legacy/plugins/actions/server/init.ts index bcc857a8a82d8..765653f0fec8c 100644 --- a/x-pack/legacy/plugins/actions/server/init.ts +++ b/x-pack/legacy/plugins/actions/server/init.ts @@ -43,7 +43,7 @@ export function init(server: Legacy.Server) { getBasePath: () => basePath, }; return { - log: server.log, + log: server.log.bind(server), callCluster: callWithInternalUser, savedObjectsClient: server.savedObjects.getScopedSavedObjectsClient(fakeRequest), ...overwrites, diff --git a/x-pack/legacy/plugins/actions/server/lib/execute.ts b/x-pack/legacy/plugins/actions/server/lib/execute.ts index b614b0a08fe4e..d7b11d045a5fd 100644 --- a/x-pack/legacy/plugins/actions/server/lib/execute.ts +++ b/x-pack/legacy/plugins/actions/server/lib/execute.ts @@ -37,9 +37,43 @@ export async function execute({ }; const validatedConfig = validateActionTypeConfig(actionType, mergedActionTypeConfig); const validatedParams = validateActionTypeParams(actionType, params); - await actionType.executor({ - services, - config: validatedConfig, - params: validatedParams, - }); + + let result; + let error; + try { + result = await actionType.executor({ + services, + config: validatedConfig, + params: validatedParams, + }); + } catch (err) { + error = err; + } + + const { actionTypeId, description } = action.attributes; + const actionLabel = `${actionId} - ${actionTypeId} - ${description}`; + + if (error != null) { + services.log( + ['warning', 'x-pack', 'actions'], + `action executed unsuccessfully: ${actionLabel} - ${error.message}` + ); + throw error; + } + + services.log(['debug', 'x-pack', 'actions'], `action executed successfully: ${actionLabel}`); + + // return result if it's JSONable, otherwise a simple success object + const simpleResult = { status: 'ok' }; + + if (result == null || typeof result !== 'object') { + return simpleResult; + } + + try { + JSON.stringify(result); + return result; + } catch (err) { + return simpleResult; + } } diff --git a/x-pack/legacy/plugins/actions/server/routes/fire.test.ts b/x-pack/legacy/plugins/actions/server/routes/fire.test.ts index 034296df9d507..bd994134c1d91 100644 --- a/x-pack/legacy/plugins/actions/server/routes/fire.test.ts +++ b/x-pack/legacy/plugins/actions/server/routes/fire.test.ts @@ -39,8 +39,8 @@ it('fires an action with proper parameters', async () => { execute.mockResolvedValueOnce({ success: true }); const { payload, statusCode } = await server.inject(request); - expect(statusCode).toBe(204); - expect(payload).toBe(''); + expect(statusCode).toBe(200); + expect(payload).toBe('{"success":true}'); expect(savedObjectsClient.get).toHaveBeenCalledTimes(1); expect(savedObjectsClient.get.mock.calls[0]).toMatchInlineSnapshot(` diff --git a/x-pack/legacy/plugins/actions/server/routes/fire.ts b/x-pack/legacy/plugins/actions/server/routes/fire.ts index 4b6ca53189462..50d4aabe91126 100644 --- a/x-pack/legacy/plugins/actions/server/routes/fire.ts +++ b/x-pack/legacy/plugins/actions/server/routes/fire.ts @@ -55,7 +55,7 @@ export function fireRoute({ server, actionTypeRegistry, getServices }: FireRoute const savedObjectsClient = request.getSavedObjectsClient(); // Ensure user can read the action and has access to it await savedObjectsClient.get('action', id); - await execute({ + const result = await execute({ params, actionTypeRegistry, actionId: id, @@ -63,7 +63,7 @@ export function fireRoute({ server, actionTypeRegistry, getServices }: FireRoute services: getServices(request.getBasePath(), { savedObjectsClient }), encryptedSavedObjectsPlugin: server.plugins.encrypted_saved_objects!, }); - return h.response(); + return result; }, }); } diff --git a/x-pack/legacy/plugins/alerting/server/init.ts b/x-pack/legacy/plugins/alerting/server/init.ts index aa6e939b9b616..e244a23377fb5 100644 --- a/x-pack/legacy/plugins/alerting/server/init.ts +++ b/x-pack/legacy/plugins/alerting/server/init.ts @@ -29,7 +29,7 @@ export function init(server: Legacy.Server) { getBasePath: () => basePath, }; return { - log: server.log, + log: server.log.bind(server), callCluster: callWithInternalUser, savedObjectsClient: server.savedObjects.getScopedSavedObjectsClient(fakeRequest), }; @@ -56,7 +56,7 @@ export function init(server: Legacy.Server) { const request = this; const savedObjectsClient = request.getSavedObjectsClient(); const alertsClient = new AlertsClient({ - log: server.log, + log: server.log.bind(server), savedObjectsClient, alertTypeRegistry, taskManager: taskManager!, diff --git a/x-pack/test/api_integration/apis/actions/fire.ts b/x-pack/test/api_integration/apis/actions/fire.ts index 9271fc30e2655..7c099256b19de 100644 --- a/x-pack/test/api_integration/apis/actions/fire.ts +++ b/x-pack/test/api_integration/apis/actions/fire.ts @@ -64,9 +64,9 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) { message: 'Testing 123', }, }) - .expect(204) + .expect(200) .then((resp: any) => { - expect(resp.body).to.eql({}); + expect(resp.body).to.be.an('object'); }); const indexedRecord = await retry.tryForTime(5000, async () => { const searchResult = await es.search({ @@ -146,9 +146,9 @@ export default function({ getService }: KibanaFunctionalTestDefaultProviders) { message: 'Testing 123', }, }) - .expect(204) + .expect(200) .then((resp: any) => { - expect(resp.body).to.eql({}); + expect(resp.body).to.be.an('object'); }); const indexedRecord = await retry.tryForTime(5000, async () => { const searchResult = await es.search({