adds strict types to Alerting Client (elastic#53821) (elastic#54007)

The AlertsClient API currently returns mixed inferred types instead of a clear strict type, making it harder to work with the client's type signatures. The root causes for this difficulty is that we have to support the SavedObjects API which allows partial updates of types, and the implementation of code that converts the SavedObject from a RawAlert to an Alert in a non type-strict manner. To address this we've added concrete types on the AlertsClient APIs, using Partial on update due to the SavedObjects API, and a strict Alert on the other APIs.
afgomez · Jan 6, 2020 · c2c5b66 · c2c5b66
1 parent 647d7cd
commit c2c5b66
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 46 deletions.
diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts
@@ -15,6 +15,7 @@ import {
 } from 'src/core/server';
 import {
   Alert,
+  PartialAlert,
   RawAlert,
   AlertTypeRegistry,
   AlertAction,
@@ -69,28 +70,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>;
@@ -146,7 +145,7 @@ export class AlertsClient {
     this.encryptedSavedObjectsPlugin = encryptedSavedObjectsPlugin;
   }
 
-  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);
@@ -199,26 +198,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)
+      ),
     };
   }
 
@@ -234,7 +236,7 @@ export class AlertsClient {
     return removeResult;
   }
 
-  public async update({ id, data }: UpdateOptions) {
+  public async update({ id, data }: UpdateOptions): Promise<PartialAlert> {
     const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser<
       RawAlert
     >('alert', id, { namespace: this.namespace });
@@ -257,7 +259,7 @@ export class AlertsClient {
   private async updateAlert(
     { id, data }: UpdateOptions,
     { attributes, version }: SavedObject<RawAlert>
-  ) {
+  ): Promise<PartialAlert> {
     const alertType = this.alertTypeRegistry.get(attributes.alertTypeId);
 
     // Validate
@@ -287,7 +289,7 @@ export class AlertsClient {
 
     await this.invalidateApiKey({ apiKey: attributes.apiKey });
 
-    return this.getAlertFromRaw(
+    return this.getPartialAlertFromRaw(
       id,
       updatedObject.attributes,
       updatedObject.updated_at,
@@ -494,24 +496,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 || [])
+        : [],
     };
   }
 

diff --git a/x-pack/legacy/plugins/alerting/server/routes/create.test.ts b/x-pack/legacy/plugins/alerting/server/routes/create.test.ts
@@ -20,6 +20,7 @@ const mockedAlert = {
   params: {
     bar: true,
   },
+  throttle: '30s',
   actions: [
     {
       group: 'default',
@@ -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',
@@ -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,
@@ -83,6 +97,8 @@ test('creates an alert with proper parameters', async () => {
       "tags": Array [
         "foo",
       ],
+      "throttle": "30s",
+      "updatedBy": "",
     }
   `);
   expect(alertsClient.create).toHaveBeenCalledTimes(1);
@@ -112,7 +128,7 @@ test('creates an alert with proper parameters', async () => {
           "tags": Array [
             "foo",
           ],
-          "throttle": null,
+          "throttle": "30s",
         },
       },
     ]

diff --git a/x-pack/legacy/plugins/alerting/server/routes/get.test.ts b/x-pack/legacy/plugins/alerting/server/routes/get.test.ts
@@ -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());

diff --git a/x-pack/legacy/plugins/alerting/server/types.ts b/x-pack/legacy/plugins/alerting/server/types.ts
@@ -65,6 +65,7 @@ export interface IntervalSchedule extends SavedObjectAttributes {
 }
 
 export interface Alert {
+  id: string;
   enabled: boolean;
   name: string;
   tags: string[];
@@ -85,6 +86,8 @@ export interface Alert {
   mutedInstanceIds: string[];
 }
 
+export type PartialAlert = Pick<Alert, 'id'> & Partial<Omit<Alert, 'id'>>;
+
 export interface RawAlert extends SavedObjectAttributes {
   enabled: boolean;
   name: string;

diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts
@@ -283,9 +283,7 @@ export const getResult = (): RuleAlertType => ({
     ],
     riskScore: 50,
     maxSignals: 100,
-    size: 1,
     severity: 'high',
-    tags: [],
     to: 'now',
     type: 'query',
     threats: [

diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts
@@ -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) {
@@ -33,5 +33,10 @@ export const findRules = async ({
       sortOrder,
       sortField,
     },
-  });
+  }) as Promise<{
+    page: number;
+    perPage: number;
+    total: number;
+    data: RuleAlertType[];
+  }>;
 };
diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts
@@ -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;