Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@cnasikas
cnasikas committed Oct 23, 2021
1 parent 270218e commit fa8a208
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 48 deletions.
24 changes: 12 additions & 12 deletions x-pack/plugins/actions/server/builtin_action_types/jira/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
} from './types';

import * as i18n from './translations';
import { request, getErrorMessage, throwIfRequestIsNotValid } from '../lib/axios_utils';
import { request, getErrorMessage, throwIfResponseIsNotValid } from '../lib/axios_utils';
import { ActionsConfigurationUtilities } from '../../actions_config';

const VERSION = '2';
Expand Down Expand Up @@ -181,7 +181,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id', 'key'],
});
Expand Down Expand Up @@ -235,7 +235,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id'],
});
Expand Down Expand Up @@ -281,7 +281,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -319,7 +319,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id', 'created'],
});
Expand Down Expand Up @@ -351,7 +351,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['capabilities'],
});
Expand Down Expand Up @@ -382,7 +382,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand All @@ -397,7 +397,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -429,7 +429,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand All @@ -444,7 +444,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -507,7 +507,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -535,7 +535,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down
28 changes: 14 additions & 14 deletions x-pack/plugins/actions/server/builtin_action_types/lib/axios_utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
request,
patch,
getErrorMessage,
throwIfRequestIsNotValid,
throwIfResponseIsNotValid,
createAxiosResponse,
} from './axios_utils';
import { loggingSystemMock } from '../../../../../../src/core/server/mocks';
Expand Down Expand Up @@ -300,29 +300,29 @@ describe('getErrorMessage', () => {
});
});

describe('throwIfRequestIsNotValid', () => {
describe('throwIfResponseIsNotValid', () => {
const res = createAxiosResponse({
headers: { ['content-type']: 'application/json' },
data: { incident: { id: '1' } },
});

test('it does NOT throw if the request is valid', () => {
expect(() => throwIfRequestIsNotValid({ res })).not.toThrow();
expect(() => throwIfResponseIsNotValid({ res })).not.toThrow();
});

test('it does throw if the content-type is not json', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, headers: { ['content-type']: 'text/html' } },
})
).toThrow(
'Unsupported content type: text/html in GET https://example.com. Supported content types: application/json'
);
});

test('it does throw if the content-type is defined', () => {
test('it does throw if the content-type is undefined', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, headers: {} },
})
).toThrow(
Expand All @@ -332,46 +332,46 @@ describe('throwIfRequestIsNotValid', () => {

test('it does throw if the data is not an object or array', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, data: 'string' },
})
).toThrow('Response is not a valid JSON');
});

test('it does NOT throw if the data is an array', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, data: ['test'] },
})
).not.toThrow();
});

test.each(['', [], {}])('it does NOT throw if the data is %p', (data) => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, data },
})
).not.toThrow();
});

test('it does throw if the required attribute is not in the response', () => {
expect(() =>
throwIfRequestIsNotValid({ res, requiredAttributesToBeInTheResponse: ['not-exist'] })
throwIfResponseIsNotValid({ res, requiredAttributesToBeInTheResponse: ['not-exist'] })
).toThrow('Response is missing at least one of the expected fields: not-exist');
});

test('it does NOT throw if the required attribute is not in the response and the data are an array', () => {
test('it does throw if the required attribute are defined and the data is an array', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, data: ['test'] },
requiredAttributesToBeInTheResponse: ['not-exist'],
})
).not.toThrow();
).toThrow('Response is missing at least one of the expected fields: not-exist');
});

test('it does NOT throw if the value of the required attribute is null', () => {
expect(() =>
throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res: { ...res, data: { id: null } },
requiredAttributesToBeInTheResponse: ['id'],
})
Expand Down
30 changes: 19 additions & 11 deletions x-pack/plugins/actions/server/builtin_action_types/lib/axios_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const getErrorMessage = (connector: string, msg: string) => {
return `[Action][${connector}]: ${msg}`;
};

export const throwIfRequestIsNotValid = ({
export const throwIfResponseIsNotValid = ({
res,
requiredAttributesToBeInTheResponse = [],
}: {
Expand Down Expand Up @@ -110,19 +110,27 @@ export const throwIfRequestIsNotValid = ({
throw new Error('Response is not a valid JSON');
}

/**
* Check for required attributes only if provided and
* data is an object and not an array
*/
if (requiredAttributesToBeInTheResponse.length > 0 && !Array.isArray(data)) {
if (requiredAttributesToBeInTheResponse.length > 0) {
const requiredAttributesError = new Error(
`Response is missing at least one of the expected fields: ${requiredAttributesToBeInTheResponse.join(
','
)}`
);

/**
* If the response is an array and requiredAttributesToBeInTheResponse
* are not empty then we thrown an error assuming that the consumer
* expects an object response and not an array.
*/

if (Array.isArray(data)) {
throw requiredAttributesError;
}

requiredAttributesToBeInTheResponse.forEach((attr) => {
// Check only for undefined as null is a valid value
if (data[attr] === undefined) {
throw new Error(
`Response is missing at least one of the expected fields: ${requiredAttributesToBeInTheResponse.join(
','
)}`
);
throw requiredAttributesError;
}
});
}
Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugins/actions/server/builtin_action_types/resilient/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
} from './types';

import * as i18n from './translations';
import { getErrorMessage, request, throwIfRequestIsNotValid } from '../lib/axios_utils';
import { getErrorMessage, request, throwIfResponseIsNotValid } from '../lib/axios_utils';
import { ActionsConfigurationUtilities } from '../../actions_config';

const VIEW_INCIDENT_URL = `#incidents`;
Expand Down Expand Up @@ -134,7 +134,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -186,7 +186,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id', 'create_date'],
});
Expand Down Expand Up @@ -221,7 +221,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -258,7 +258,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down Expand Up @@ -287,7 +287,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand All @@ -313,7 +313,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand All @@ -338,7 +338,7 @@ export const createExternalService = (
configurationUtilities,
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
});

Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/actions/server/builtin_action_types/swimlane/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Logger } from '@kbn/logging';
import axios from 'axios';

import { ActionsConfigurationUtilities } from '../../actions_config';
import { getErrorMessage, request, throwIfRequestIsNotValid } from '../lib/axios_utils';
import { getErrorMessage, request, throwIfResponseIsNotValid } from '../lib/axios_utils';
import { getBodyForEventAction } from './helpers';
import {
CreateCommentParams,
Expand Down Expand Up @@ -90,7 +90,7 @@ export const createExternalService = (
url: getPostRecordUrl(appId),
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id', 'name', 'createdDate'],
});
Expand Down Expand Up @@ -130,7 +130,7 @@ export const createExternalService = (
url: getPostRecordIdUrl(appId, params.incidentId),
});

throwIfRequestIsNotValid({
throwIfResponseIsNotValid({
res,
requiredAttributesToBeInTheResponse: ['id', 'name', 'modifiedDate'],
});
Expand Down

0 comments on commit fa8a208

Please sign in to comment.