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

adds strict types to Alerting Client #53821

Merged
merged 28 commits into from
Jan 6, 2020
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
dcd574e
added createdAt and updatedAt fields in alerting
gmmorris Dec 24, 2019
1721ec2
use the SavedObjets updated_at instead of a new field in alerting
gmmorris Dec 24, 2019
430fe9b
Merge branch 'master' into alerting/created_at-and-updated_at
gmmorris Dec 27, 2019
f6b0dfb
added concrete types on AlertClients public APIs
gmmorris Dec 27, 2019
36ea491
removed generics in AlertsClient
gmmorris Dec 30, 2019
be3b3a2
Merge branch 'master' into alerting/client-types
gmmorris Dec 30, 2019
b6c9385
Merge branch 'master' into alerting/created_at-and-updated_at
elasticmachine Dec 30, 2019
e8dbc15
added missing assetion
gmmorris Jan 2, 2020
27f54c0
Merge branch 'alerting/created_at-and-updated_at' of github.com:gmmor…
gmmorris Jan 2, 2020
fe1902e
Merge branch 'master' into alerting/created_at-and-updated_at
gmmorris Jan 2, 2020
fa3e74f
Merge branch 'master' into alerting/client-types
gmmorris Jan 2, 2020
1fb5735
used createdAt in place of empty updatedAt
gmmorris Jan 3, 2020
2f50541
fixed misuse of null on updatedAt
gmmorris Jan 3, 2020
1457a5a
Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…
gmmorris Jan 3, 2020
19063db
cleaned up tests
gmmorris Jan 3, 2020
5a4057d
fixed tests
gmmorris Jan 3, 2020
954a71a
fixed tests
gmmorris Jan 3, 2020
c4817e5
Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…
gmmorris Jan 3, 2020
db0d578
Merge branch 'master' into alerting/created_at-and-updated_at
elasticmachine Jan 3, 2020
924393f
updatedAt should equal createdAt on creation
gmmorris Jan 3, 2020
dbd61a7
Merge branch 'alerting/created_at-and-updated_at' of github.com:gmmor…
gmmorris Jan 3, 2020
cf24b03
Merge branch 'master' into alerting/created_at-and-updated_at
gmmorris Jan 3, 2020
b04189e
Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…
gmmorris Jan 3, 2020
6dff233
removed assertions for updatedAt in non-update operations
gmmorris Jan 3, 2020
543c057
Merge branch 'alerting/created_at-and-updated_at' into alerting/clien…
gmmorris Jan 3, 2020
d0452b2
Merge branch 'master' into alerting/client-types
gmmorris Jan 3, 2020
ae0314d
Merge branch 'master' into alerting/client-types
gmmorris Jan 3, 2020
8f5c13d
Merge branch 'master' into alerting/client-types
gmmorris Jan 6, 2020
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
88 changes: 50 additions & 38 deletions x-pack/legacy/plugins/alerting/server/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from 'src/core/server';
import {
Alert,
PartialAlert,
RawAlert,
AlertTypeRegistry,
AlertAction,
Expand Down Expand Up @@ -63,28 +64,26 @@ export interface FindOptions {
};
}

interface FindResult {
export interface FindResult {
page: number;
perPage: number;
total: number;
data: object[];
data: Alert[];
}

interface CreateOptions {
data: Pick<
data: Omit<
Alert,
Exclude<
keyof Alert,
| 'createdBy'
| 'updatedBy'
| 'createdAt'
| 'updatedAt'
| 'apiKey'
| 'apiKeyOwner'
| 'muteAll'
| 'mutedInstanceIds'
| 'actions'
>
| 'id'
| 'createdBy'
| 'updatedBy'
| 'createdAt'
| 'updatedAt'
| 'apiKey'
| 'apiKeyOwner'
| 'muteAll'
| 'mutedInstanceIds'
| 'actions'
> & { actions: NormalizedAlertAction[] };
options?: {
migrationVersion?: Record<string, string>;
Expand Down Expand Up @@ -129,7 +128,7 @@ export class AlertsClient {
this.createAPIKey = createAPIKey;
}

public async create({ data, options }: CreateOptions) {
public async create({ data, options }: CreateOptions): Promise<Alert> {
// Throws an error if alert type isn't registered
const alertType = this.alertTypeRegistry.get(data.alertTypeId);
const validatedAlertTypeParams = validateAlertTypeParams(alertType, data.params);
Expand Down Expand Up @@ -182,26 +181,29 @@ export class AlertsClient {
);
}

public async get({ id }: { id: string }) {
public async get({ id }: { id: string }): Promise<Alert> {
const result = await this.savedObjectsClient.get('alert', id);
return this.getAlertFromRaw(result.id, result.attributes, result.updated_at, result.references);
}

public async find({ options = {} }: FindOptions = {}): Promise<FindResult> {
const results = await this.savedObjectsClient.find({
const {
page,
per_page: perPage,
total,
saved_objects: data,
} = await this.savedObjectsClient.find<RawAlert>({
...options,
type: 'alert',
});

const data = results.saved_objects.map(result =>
this.getAlertFromRaw(result.id, result.attributes, result.updated_at, result.references)
);

return {
page: results.page,
perPage: results.per_page,
total: results.total,
data,
page,
perPage,
total,
data: data.map(({ id, attributes, updated_at, references }) =>
this.getAlertFromRaw(id, attributes, updated_at, references)
),
};
}

Expand All @@ -214,7 +216,7 @@ export class AlertsClient {
return removeResult;
}

public async update({ id, data }: UpdateOptions) {
public async update({ id, data }: UpdateOptions): Promise<PartialAlert> {
const alert = await this.savedObjectsClient.get<RawAlert>('alert', id);
const updateResult = await this.updateAlert({ id, data }, alert);

Expand All @@ -235,7 +237,7 @@ export class AlertsClient {
private async updateAlert(
{ id, data }: UpdateOptions,
{ attributes, version }: SavedObject<RawAlert>
) {
): Promise<PartialAlert> {
const alertType = this.alertTypeRegistry.get(attributes.alertTypeId);

// Validate
Expand All @@ -262,7 +264,7 @@ export class AlertsClient {
references,
}
);
return this.getAlertFromRaw(
return this.getPartialAlertFromRaw(
id,
updatedObject.attributes,
updatedObject.updated_at,
Expand Down Expand Up @@ -437,24 +439,34 @@ export class AlertsClient {
}

private getAlertFromRaw(
id: string,
rawAlert: RawAlert,
updatedAt: SavedObject['updated_at'],
references: SavedObjectReference[] | undefined
): Alert {
// In order to support the partial update API of Saved Objects we have to support
// partial updates of an Alert, but when we receive an actual RawAlert, it is safe
// to cast the result to an Alert
return this.getPartialAlertFromRaw(id, rawAlert, updatedAt, references) as Alert;
}

private getPartialAlertFromRaw(
id: string,
rawAlert: Partial<RawAlert>,
updatedAt: SavedObject['updated_at'],
references: SavedObjectReference[] | undefined
) {
if (!rawAlert.actions) {
return {
id,
...rawAlert,
};
}
const actions = this.injectReferencesIntoActions(rawAlert.actions, references || []);
): PartialAlert {
return {
id,
...rawAlert,
// we currently only support the Interval Schedule type
// Once we support additional types, this type signature will likely change
schedule: rawAlert.schedule as IntervalSchedule,
updatedAt: updatedAt ? new Date(updatedAt) : new Date(rawAlert.createdAt!),
createdAt: new Date(rawAlert.createdAt!),
actions,
actions: rawAlert.actions
? this.injectReferencesIntoActions(rawAlert.actions, references || [])
: [],
};
}

Expand Down
18 changes: 17 additions & 1 deletion x-pack/legacy/plugins/alerting/server/routes/create.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const mockedAlert = {
params: {
bar: true,
},
throttle: '30s',
actions: [
{
group: 'default',
Expand All @@ -44,6 +45,13 @@ test('creates an alert with proper parameters', async () => {
const updatedAt = new Date();
alertsClient.create.mockResolvedValueOnce({
...mockedAlert,
enabled: true,
muteAll: false,
createdBy: '',
updatedBy: '',
apiKey: '',
apiKeyOwner: '',
mutedInstanceIds: [],
createdAt,
updatedAt,
id: '123',
Expand Down Expand Up @@ -71,8 +79,14 @@ test('creates an alert with proper parameters', async () => {
},
],
"alertTypeId": "1",
"apiKey": "",
"apiKeyOwner": "",
"consumer": "bar",
"createdBy": "",
"enabled": true,
"id": "123",
"muteAll": false,
"mutedInstanceIds": Array [],
"name": "abc",
"params": Object {
"bar": true,
Expand All @@ -83,6 +97,8 @@ test('creates an alert with proper parameters', async () => {
"tags": Array [
"foo",
],
"throttle": "30s",
"updatedBy": "",
}
`);
expect(alertsClient.create).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -112,7 +128,7 @@ test('creates an alert with proper parameters', async () => {
"tags": Array [
"foo",
],
"throttle": null,
"throttle": "30s",
},
},
]
Expand Down
11 changes: 11 additions & 0 deletions x-pack/legacy/plugins/alerting/server/routes/get.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ const mockedAlert = {
},
},
],
consumer: 'bar',
name: 'abc',
tags: ['foo'],
enabled: true,
muteAll: false,
createdBy: '',
updatedBy: '',
apiKey: '',
apiKeyOwner: '',
throttle: '30s',
mutedInstanceIds: [],
};

beforeEach(() => jest.resetAllMocks());
Expand Down
3 changes: 3 additions & 0 deletions x-pack/legacy/plugins/alerting/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export interface IntervalSchedule extends SavedObjectAttributes {
}

export interface Alert {
id: string;
enabled: boolean;
name: string;
tags: string[];
Expand All @@ -85,6 +86,8 @@ export interface Alert {
mutedInstanceIds: string[];
}

export type PartialAlert = Pick<Alert, 'id'> & Partial<Omit<Alert, 'id'>>;
Copy link
Member

Choose a reason for hiding this comment

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

could this be shorted to:

export type PartialAlert = Pick<Alert, 'id'> & Partial<Alert>;

Seems like TS usually does a good job allowing some "slop" here - basically, id is defined twice - required on the left type, optional on the right, so . ... it's required in the final type, and the types of id are the same so ... "it works". Not quite sure tho :-)

I feel like there must be a good, simple pattern for building and naming (heh) these sort of structures, but I've not quite figured it out yet ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly no, because that results in id becoming string | undefined which is exactly what we don't want here...


export interface RawAlert extends SavedObjectAttributes {
enabled: boolean;
name: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,7 @@ export const getResult = (): RuleAlertType => ({
],
riskScore: 50,
maxSignals: 100,
size: 1,
severity: 'high',
tags: [],
to: 'now',
type: 'query',
threats: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { SIGNALS_ID } from '../../../../common/constants';
import { FindRuleParams } from './types';
import { FindRuleParams, RuleAlertType } from './types';

export const getFilter = (filter: string | null | undefined) => {
if (filter == null) {
Expand Down Expand Up @@ -33,5 +33,10 @@ export const findRules = async ({
sortOrder,
sortField,
},
});
}) as Promise<{
page: number;
perPage: number;
total: number;
data: RuleAlertType[];
}>;
};
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,9 @@ export interface BulkUpdateRulesRequest extends RequestFacade {
payload: UpdateRuleAlertParamsRest[];
}

export type RuleAlertType = Alert & {
id: string;
export interface RuleAlertType extends Alert {
params: RuleTypeParams;
};
}

export interface RulesRequest extends RequestFacade {
payload: RuleAlertParamsRest;
Expand Down