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

Use HTTP request schemas to create types, use those types in the client #59340

Merged
merged 15 commits into from
Mar 9, 2020
7 changes: 6 additions & 1 deletion src/core/public/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,12 @@ export interface HttpRequestInit {

/** @public */
export interface HttpFetchQuery {
[key: string]: string | number | boolean | undefined;
[key: string]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying code supports this already. This change loosens up the type

| string
| number
| boolean
| undefined
| Array<string | number | boolean | undefined>;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ export interface HttpFetchOptionsWithPath extends HttpFetchOptions {
// @public (undocumented)
export interface HttpFetchQuery {
// (undocumented)
[key: string]: string | number | boolean | undefined;
[key: string]: string | number | boolean | undefined | Array<string | number | boolean | undefined>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generated docs

}

// @public
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/endpoint/common/schema/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Schemas

These schemas are used to validate, coerce, and provide types for the comms between the client, server, and ES.

# Future work
In the future, we may be able to locate these under 'server'.
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { decode } from 'rison-node';

import { schema, Type } from '@kbn/config-schema';
import { i18n } from '@kbn/i18n';
import { schema } from '@kbn/config-schema';
import { esKuery } from '../../../../../../../src/plugins/data/server';
import { EndpointAppConstants } from '../../../../common/types';
import { decode } from 'rison-node';
import { fromKueryExpression } from '../../../../../src/plugins/data/common';
import { EndpointAppConstants } from '../types';

export const alertListReqSchema = schema.object(
/**
* Used to validate GET requests against the index of the alerting APIs.
*/
export const alertingIndexGetQuerySchema = schema.object(
{
page_size: schema.maybe(
schema.number({
Expand All @@ -26,31 +30,21 @@ export const alertListReqSchema = schema.object(
schema.arrayOf(schema.string(), {
minSize: 2,
maxSize: 2,
})
}) as Type<[string, string]> // Cast this to a string tuple. `@kbn/config-schema` doesn't do this automatically
),
before: schema.maybe(
schema.arrayOf(schema.string(), {
minSize: 2,
maxSize: 2,
})
}) as Type<[string, string]> // Cast this to a string tuple. `@kbn/config-schema` doesn't do this automatically
),
sort: schema.maybe(schema.string()),
order: schema.maybe(
schema.string({
validate(value) {
if (value !== 'asc' && value !== 'desc') {
return i18n.translate('xpack.endpoint.alerts.errors.bad_sort_direction', {
defaultMessage: 'must be `asc` or `desc`',
});
}
},
})
),
order: schema.maybe(schema.oneOf([schema.literal('asc'), schema.literal('desc')])),
query: schema.maybe(
schema.string({
validate(value) {
try {
esKuery.fromKueryExpression(value);
fromKueryExpression(value);
} catch (err) {
return i18n.translate('xpack.endpoint.alerts.errors.bad_kql', {
defaultMessage: 'must be valid KQL',
Expand Down
78 changes: 73 additions & 5 deletions x-pack/plugins/endpoint/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
*/

import { SearchResponse } from 'elasticsearch';
import { TypeOf } from '@kbn/config-schema';
import * as kbnConfigSchemaTypes from '@kbn/config-schema/target/types/types';
import { alertingIndexGetQuerySchema } from './schema/alert_index';

/**
* A deep readonly type that will make all children of a given object readonly recursively
Expand All @@ -24,10 +27,7 @@ export type ImmutableMap<K, V> = ReadonlyMap<Immutable<K>, Immutable<V>>;
export type ImmutableSet<T> = ReadonlySet<Immutable<T>>;
export type ImmutableObject<T> = { readonly [K in keyof T]: Immutable<T[K]> };

export enum Direction {
asc = 'asc',
desc = 'desc',
}
export type Direction = 'asc' | 'desc';
Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller Is type better than enum? TypeScript n00b here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other plugins have used enum for this... for example, siem. So just curious what the advantages/disadvantages are.

Copy link
Contributor Author

@oatkiller oatkiller Mar 5, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so i tried to go back and make everything the enum, but it turns out we have a whole lot of places in the app writing 'asc' and 'desc' as literals. I don't honestly know what the best practice is, but for the sake of keeping the (runtime) enum out of the validation that will be used on FE and BE, I'm going to keep this as a type for now. I'm aware that this may not be right, I just don't have quite enough experience w/ this yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, that's fine. I imagine that could be difficult to work into the schema validation. Let's come back to it later.


export class EndpointAppConstants {
static BASE_API_URL = '/api/endpoint';
Expand All @@ -45,7 +45,6 @@ export class EndpointAppConstants {
**/
static ALERT_LIST_DEFAULT_PAGE_SIZE = 10;
static ALERT_LIST_DEFAULT_SORT = '@timestamp';
static ALERT_LIST_DEFAULT_ORDER = Direction.desc;
}

export interface AlertResultList {
Expand Down Expand Up @@ -336,3 +335,72 @@ export type ResolverEvent = EndpointEvent | LegacyEndpointEvent;
* The PageId type is used for the payload when firing userNavigatedToPage actions
*/
export type PageId = 'alertsPage' | 'managementPage' | 'policyListPage';

/**
* Takes a @kbn/config-schema 'schema' type and returns a type that represents valid inputs.
* Similar to `TypeOf`, but allows strings as input for `schema.number()` (which is inline
* with the behavior of the validator.) Also, for `schema.object`, when a value is a `schema.maybe`
* the key will be marked optional (via `?`) so that you can omit keys for optional values.
*
* Use this when creating a value that will be passed to the schema.
* e.g.
* ```ts
* const input: KbnConfigSchemaInputTypeOf<typeof schema> = value
* schema.validate(input) // should be valid
* ```
*/
type KbnConfigSchemaInputTypeOf<
T extends kbnConfigSchemaTypes.Type<unknown>
> = T extends kbnConfigSchemaTypes.ObjectType
? KbnConfigSchemaInputObjectTypeOf<
T
> /** `schema.number()` accepts strings, so this type should accept them as well. */
: kbnConfigSchemaTypes.Type<number> extends T
? TypeOf<T> | string
: TypeOf<T>;

/**
* Works like ObjectResultType, except that 'maybe' schema will create an optional key.
* This allows us to avoid passing 'maybeKey: undefined' when constructing such an object.
*
* Instead of using this directly, use `InputTypeOf`.
*/
type KbnConfigSchemaInputObjectTypeOf<
T extends kbnConfigSchemaTypes.ObjectType
> = T extends kbnConfigSchemaTypes.ObjectType<infer P>
? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This maps the properties of a schema object. It does it twice, once for values that accept 'undefined', and once for keys that do not. If a value accepts undefined, they key is marked optional (?)
TODO add above in comment in code

/** Use ? to make the field optional if the prop accepts undefined.
* This allows us to avoid writing `field: undefined` for optional fields.
*/
[K in Exclude<
keyof P,
keyof KbnConfigSchemaNonOptionalProps<P>
>]?: KbnConfigSchemaInputTypeOf<P[K]>;
} &
{ [K in keyof KbnConfigSchemaNonOptionalProps<P>]: KbnConfigSchemaInputTypeOf<P[K]> }
: never;

/**
* Takes the props of a schema.object type, and returns a version that excludes
* optional values. Used by `InputObjectTypeOf`.
*
* Instead of using this directly, use `InputTypeOf`.
*/
type KbnConfigSchemaNonOptionalProps<Props extends kbnConfigSchemaTypes.Props> = Pick<
Props,
{
[Key in keyof Props]: undefined extends TypeOf<Props[Key]> ? never : Key;
}[keyof Props]
>;

/**
* Query params to pass to the alert API when fetching new data.
*/
export type AlertingIndexGetQueryInput = KbnConfigSchemaInputTypeOf<
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

typeof alertingIndexGetQuerySchema
>;

/**
* Result of the validated query params when handling alert index requests.
*/
export type AlertingIndexGetQueryResult = TypeOf<typeof alertingIndexGetQuerySchema>;
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ import { AlertResultList, AlertData } from '../../../../../common/types';
import { AppAction } from '../action';
import { MiddlewareFactory, AlertListState } from '../../types';
import { isOnAlertPage, apiQueryParams, hasSelectedAlert, uiQueryParams } from './selectors';
import { cloneHttpFetchQuery } from '../../../../common/clone_http_fetch_query';

export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = coreStart => {
return api => next => async (action: AppAction) => {
next(action);
const state = api.getState();
if (action.type === 'userChangedUrl' && isOnAlertPage(state)) {
const response: AlertResultList = await coreStart.http.get(`/api/endpoint/alerts`, {
query: apiQueryParams(state),
query: cloneHttpFetchQuery(apiQueryParams(state)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The http.get function won't accept our Immutable type. I'm not sure if this is the best approach long term.
Basically, we require that the return value of apiQueryParams not be modified, and .get isn't promising that it won't modify them. Therefore we clone it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems okay to me.

});
api.dispatch({ type: 'serverReturnedAlertsData', payload: response });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,11 @@ import {
createSelector,
createStructuredSelector as createStructuredSelectorWithBadType,
} from 'reselect';
import {
AlertListState,
AlertingIndexUIQueryParams,
AlertsAPIQueryParams,
CreateStructuredSelector,
} from '../../types';
import { Immutable } from '../../../../../common/types';
import { AlertListState, AlertingIndexUIQueryParams, CreateStructuredSelector } from '../../types';
import { Immutable, AlertingIndexGetQueryInput } from '../../../../../common/types';

const createStructuredSelector: CreateStructuredSelector = createStructuredSelectorWithBadType;

/**
* Returns the Alert Data array from state
*/
Expand Down Expand Up @@ -82,14 +78,18 @@ export const uiQueryParams: (
*/
export const apiQueryParams: (
state: AlertListState
) => Immutable<AlertsAPIQueryParams> = createSelector(
) => Immutable<AlertingIndexGetQueryInput> = createSelector(
uiQueryParams,
({ page_size, page_index }) => ({
page_size,
page_index,
})
);

/**
* True if the user has selected an alert to see details about.
* Populated via the browsers query params.
*/
export const hasSelectedAlert: (state: AlertListState) => boolean = createSelector(
uiQueryParams,
({ selected_alert: selectedAlert }) => selectedAlert !== undefined
Expand Down
16 changes: 1 addition & 15 deletions x-pack/plugins/endpoint/public/applications/endpoint/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import { Dispatch, MiddlewareAPI } from 'redux';
import { CoreStart } from 'kibana/public';
import {
EndpointMetadata,
AlertData,
Expand All @@ -14,6 +13,7 @@ import {
ImmutableArray,
} from '../../../common/types';
import { AppAction } from './store/action';
import { CoreStart } from '../../../../../../src/core/public';

export { AppAction };
export type MiddlewareFactory<S = GlobalState> = (
Expand Down Expand Up @@ -140,17 +140,3 @@ export interface AlertingIndexUIQueryParams {
*/
selected_alert?: string;
}

/**
* Query params to pass to the alert API when fetching new data.
*/
export interface AlertsAPIQueryParams {
/**
* Number of results to return.
*/
page_size?: string;
/**
* 0-based index of 'page' to return.
*/
page_index?: string;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { cloneHttpFetchQuery } from './clone_http_fetch_query';
import { Immutable } from '../../common/types';
import { HttpFetchQuery } from '../../../../../src/core/public';

describe('cloneHttpFetchQuery', () => {
it('can clone complex queries', () => {
const query: Immutable<HttpFetchQuery> = {
a: 'a',
'1': 1,
undefined,
array: [1, 2, undefined],
};
expect(cloneHttpFetchQuery(query)).toMatchInlineSnapshot(`
Object {
"1": 1,
"a": "a",
"array": Array [
1,
2,
undefined,
],
"undefined": undefined,
}
`);
});
});
22 changes: 22 additions & 0 deletions x-pack/plugins/endpoint/public/common/clone_http_fetch_query.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { Immutable } from '../../common/types';

import { HttpFetchQuery } from '../../../../../src/core/public';

export function cloneHttpFetchQuery(query: Immutable<HttpFetchQuery>): HttpFetchQuery {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

simply clone a query object.

const clone: HttpFetchQuery = {};
for (const [key, value] of Object.entries(query)) {
if (Array.isArray(value)) {
clone[key] = [...value];
} else {
// Array.isArray is not removing ImmutableArray from the union.
clone[key] = value as string | number | boolean;
}
}
return clone;
}
Loading