From c35bb4b12c181a14d1247e8a6bc6adedbd3e3b23 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 18 Jun 2020 18:34:29 -0500 Subject: [PATCH 01/14] Add pure API functions and react hooks for value list APIs This also adds a generic hook, useAsyncTask, that wraps an async function to provide basic utilities: * loading state * error state * abort/cancel function --- .../common/hooks/use_async_task.test.ts | 83 ++++++++++ .../public/common/hooks/use_async_task.ts | 44 +++++ x-pack/plugins/lists/public/index.tsx | 5 + x-pack/plugins/lists/public/lists/api.test.ts | 155 ++++++++++++++++++ x-pack/plugins/lists/public/lists/api.ts | 87 ++++++++++ .../lists/hooks/use_delete_list.test.ts | 33 ++++ .../public/lists/hooks/use_delete_list.ts | 18 ++ .../lists/hooks/use_export_list.test.ts | 32 ++++ .../public/lists/hooks/use_export_list.ts | 18 ++ .../public/lists/hooks/use_find_lists.test.ts | 33 ++++ .../public/lists/hooks/use_find_lists.ts | 18 ++ .../lists/hooks/use_import_list.test.ts | 90 ++++++++++ .../public/lists/hooks/use_import_list.ts | 18 ++ .../server/routes/export_list_item_route.ts | 2 +- 14 files changed, 635 insertions(+), 1 deletion(-) create mode 100644 x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts create mode 100644 x-pack/plugins/lists/public/common/hooks/use_async_task.ts create mode 100644 x-pack/plugins/lists/public/lists/api.test.ts create mode 100644 x-pack/plugins/lists/public/lists/api.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_export_list.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts create mode 100644 x-pack/plugins/lists/public/lists/hooks/use_import_list.ts diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts new file mode 100644 index 0000000000000..900a9d25a7563 --- /dev/null +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts @@ -0,0 +1,83 @@ +/* + * 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 { act, renderHook } from '@testing-library/react-hooks'; + +import { useAsyncTask } from './use_async_task'; + +describe('useAsyncTask', () => { + let task: jest.Mock; + + beforeEach(() => { + task = jest.fn().mockResolvedValue('resolved value'); + }); + + it('does not invoke task if start was not called', () => { + renderHook(() => useAsyncTask(task)); + expect(task).not.toHaveBeenCalled(); + }); + + it('invokes the task when start is called', async () => { + const { result } = renderHook(() => useAsyncTask(task)); + + await act(() => result.current.start({})); + + expect(task).toHaveBeenCalled(); + }); + + it('invokes the task with a signal and start args', async () => { + const { result } = renderHook(() => useAsyncTask(task)); + + await act(() => + result.current.start({ + arg1: 'value1', + arg2: 'value2', + }) + ); + + expect(task).toHaveBeenCalledWith(expect.any(AbortController), { + arg1: 'value1', + arg2: 'value2', + }); + }); + + it('populates result with the resolved value of the task', async () => { + const { result } = renderHook(() => useAsyncTask(task)); + + await act(() => result.current.start({})); + + expect(result.current.result).toEqual('resolved value'); + }); + + it('start rejects and error is populated if task rejects', async () => { + expect.assertions(3); + task.mockRejectedValue(new Error('whoops')); + const { result } = renderHook(() => useAsyncTask(task)); + + await act(() => result.current.start({}).catch((e) => expect(e).toEqual(new Error('whoops')))); + + expect(result.current.result).toBeUndefined(); + expect(result.current.error).toEqual(new Error('whoops')); + }); + + it('populates the loading state while the task is pending', async () => { + let resolve: () => void; + task.mockImplementation(() => new Promise((_resolve) => (resolve = _resolve))); + + const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); + + act(() => { + result.current.start({}); + }); + + expect(result.current.loading).toBe(true); + + act(() => resolve()); + await waitForNextUpdate(); + + expect(result.current.loading).toBe(false); + }); +}); diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts new file mode 100644 index 0000000000000..0c47c152cb385 --- /dev/null +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts @@ -0,0 +1,44 @@ +/* + * 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 { useCallback, useRef } from 'react'; +import { useAsyncFn } from 'react-use'; + +export interface AsyncTask { + start: (params: Params) => Promise; + abort: () => void; + loading: boolean; + error: Error | undefined; + result: Result | undefined; +} + +export type UseAsyncTask = ( + func: (...args: [AbortController, Params]) => Promise +) => AsyncTask; + +export const useAsyncTask: UseAsyncTask = (fn) => { + const ctrl = useRef(new AbortController()); + const abort = useCallback((): void => { + ctrl.current.abort(); + }, []); + + // @ts-ignore typings are incorrect, see: https://github.com/streamich/react-use/pull/589 + const [state, initiator] = useAsyncFn(fn, [fn]); + + const start = useCallback( + (args) => { + ctrl.current = new AbortController(); + + return initiator(ctrl.current, args).then((result) => + // convert resolved error to rejection: https://github.com/streamich/react-use/issues/981 + result instanceof Error ? Promise.reject(result) : result + ); + }, + [initiator] + ); + + return { abort, error: state.error, loading: state.loading, result: state.value, start }; +}; diff --git a/x-pack/plugins/lists/public/index.tsx b/x-pack/plugins/lists/public/index.tsx index 71187273c731c..1ea24123ccb9a 100644 --- a/x-pack/plugins/lists/public/index.tsx +++ b/x-pack/plugins/lists/public/index.tsx @@ -3,11 +3,16 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ + // Exports to be shared with plugins export { useApi } from './exceptions/hooks/use_api'; export { usePersistExceptionItem } from './exceptions/hooks/persist_exception_item'; export { usePersistExceptionList } from './exceptions/hooks/persist_exception_list'; export { useExceptionList } from './exceptions/hooks/use_exception_list'; +export { useFindLists } from './lists/hooks/use_find_lists'; +export { useImportList } from './lists/hooks/use_import_list'; +export { useDeleteList } from './lists/hooks/use_delete_list'; +export { useExportList } from './lists/hooks/use_export_list'; export { ExceptionList, ExceptionIdentifiers, diff --git a/x-pack/plugins/lists/public/lists/api.test.ts b/x-pack/plugins/lists/public/lists/api.test.ts new file mode 100644 index 0000000000000..28700351db512 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/api.test.ts @@ -0,0 +1,155 @@ +/* + * 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 { HttpFetchOptions } from '../../../../../src/core/public'; +import { httpServiceMock } from '../../../../../src/core/public/mocks'; + +import { deleteList, exportList, findLists, importList } from './api'; + +describe('Value Lists API', () => { + let httpMock: ReturnType; + + beforeEach(() => { + httpMock = httpServiceMock.createStartContract(); + }); + + describe('deleteList', () => { + it('DELETEs specifying the id as a query parameter', async () => { + const abortCtrl = new AbortController(); + await deleteList({ + http: httpMock, + id: 'my_list', + signal: abortCtrl.signal, + }); + + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists', + expect.objectContaining({ + method: 'DELETE', + query: { id: 'my_list' }, + }) + ); + }); + }); + + describe('findLists', () => { + it('GETs from the lists endpoint', async () => { + const abortCtrl = new AbortController(); + await findLists({ + http: httpMock, + pageIndex: 0, + pageSize: 10, + signal: abortCtrl.signal, + }); + + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/_find', + expect.objectContaining({ + method: 'GET', + }) + ); + }); + + it('sends pagination as query parameters', async () => { + const abortCtrl = new AbortController(); + await findLists({ + http: httpMock, + pageIndex: 1, + pageSize: 10, + signal: abortCtrl.signal, + }); + + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/_find', + expect.objectContaining({ + query: { page: 1, per_page: 10 }, + }) + ); + }); + }); + + describe('importList', () => { + it('POSTs the file', async () => { + const abortCtrl = new AbortController(); + const fileMock = ('my file' as unknown) as File; + + await importList({ + file: fileMock, + http: httpMock, + listId: 'my_list', + signal: abortCtrl.signal, + type: 'keyword', + }); + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/items/_import', + expect.objectContaining({ + method: 'POST', + }) + ); + + // httpmock's fetch signature is inferred incorrectly + const [[, { body }]] = (httpMock.fetch.mock.calls as unknown) as Array< + [unknown, HttpFetchOptions] + >; + const actualFile = (body as FormData).get('file'); + expect(actualFile).toEqual('my file'); + }); + + it('sends type and id as query parameters', async () => { + const abortCtrl = new AbortController(); + const fileMock = ('my file' as unknown) as File; + + await importList({ + file: fileMock, + http: httpMock, + listId: 'my_list', + signal: abortCtrl.signal, + type: 'keyword', + }); + + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/items/_import', + expect.objectContaining({ + query: { list_id: 'my_list', type: 'keyword' }, + }) + ); + }); + }); + + describe('exportList', () => { + it('POSTs to the export endpoint', async () => { + const abortCtrl = new AbortController(); + + await exportList({ + http: httpMock, + id: 'my_list', + signal: abortCtrl.signal, + }); + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/items/_export', + expect.objectContaining({ + method: 'POST', + }) + ); + }); + + it('sends type and id as query parameters', async () => { + const abortCtrl = new AbortController(); + + await exportList({ + http: httpMock, + id: 'my_list', + signal: abortCtrl.signal, + }); + expect(httpMock.fetch).toHaveBeenCalledWith( + '/api/lists/items/_export', + expect.objectContaining({ + query: { list_id: 'my_list' }, + }) + ); + }); + }); +}); diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts new file mode 100644 index 0000000000000..bac76215edff4 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -0,0 +1,87 @@ +/* + * 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 { HttpStart } from '../../../../../src/core/public'; +import { FoundListSchema, ListSchema, Type } from '../../common/schemas'; +import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; + +export interface FindListsParams { + http: HttpStart; + pageSize: number | undefined; + pageIndex: number | undefined; + signal: AbortSignal; +} + +export const findLists = async ({ + http, + pageIndex, + pageSize, + signal, +}: FindListsParams): Promise => { + return http.fetch(`${LIST_URL}/_find`, { + method: 'GET', + query: { + page: pageIndex, + per_page: pageSize, + }, + signal, + }); +}; + +export interface ImportListParams { + http: HttpStart; + file: File; + listId: string | undefined; + signal: AbortSignal; + type: Type | undefined; +} + +export const importList = async ({ + file, + http, + listId, + type, + signal, +}: ImportListParams): Promise => { + const formData = new FormData(); + formData.append('file', file); + + return http.fetch(`${LIST_ITEM_URL}/_import`, { + body: formData, + headers: { 'Content-Type': undefined }, + method: 'POST', + query: { list_id: listId, type }, + signal, + }); +}; + +export interface DeleteListParams { + http: HttpStart; + id: string; + signal: AbortSignal; +} + +export const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => { + return http.fetch(LIST_URL, { + method: 'DELETE', + query: { id }, + signal, + }); +}; + +export interface ExportListParams { + http: HttpStart; + id: string; + signal: AbortSignal; +} + +export const exportList = async ({ http, id, signal }: ExportListParams): Promise => { + return http.fetch(`${LIST_ITEM_URL}/_export`, { + method: 'POST', + query: { list_id: id }, + signal, + }); +}; diff --git a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts new file mode 100644 index 0000000000000..9849becfa35f3 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts @@ -0,0 +1,33 @@ +/* + * 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 { act, renderHook } from '@testing-library/react-hooks'; + +import * as Api from '../api'; +import { httpServiceMock } from '../../../../../../src/core/public/mocks'; +import { getListResponseMock } from '../../../common/schemas/response/list_schema.mock'; + +import { useDeleteList } from './use_delete_list'; + +jest.mock('../api'); + +describe('useDeleteList', () => { + let httpMock: ReturnType; + + beforeEach(() => { + httpMock = httpServiceMock.createStartContract(); + (Api.deleteList as jest.Mock).mockResolvedValue(getListResponseMock()); + }); + + it('invokes Api.deleteList', async () => { + const { result } = renderHook(() => useDeleteList()); + await act(() => result.current.start({ http: httpMock, id: 'list' })); + + expect(Api.deleteList).toHaveBeenCalledWith( + expect.objectContaining({ http: httpMock, id: 'list' }) + ); + }); +}); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts new file mode 100644 index 0000000000000..5c41f940ca6b7 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts @@ -0,0 +1,18 @@ +/* + * 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 { useAsyncTask } from '../../common/hooks/use_async_task'; +import { DeleteListParams, deleteList } from '../api'; + +export type DeleteListTaskArgs = Omit; + +const deleteListsTask = ( + { signal }: AbortController, + args: DeleteListTaskArgs +): ReturnType => deleteList({ signal, ...args }); + +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export const useDeleteList = () => useAsyncTask(deleteListsTask); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts new file mode 100644 index 0000000000000..e75ec819a75da --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts @@ -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 { act, renderHook } from '@testing-library/react-hooks'; + +import * as Api from '../api'; +import { httpServiceMock } from '../../../../../../src/core/public/mocks'; + +import { useExportList } from './use_export_list'; + +jest.mock('../api'); + +describe('useExportList', () => { + let httpMock: ReturnType; + + beforeEach(() => { + httpMock = httpServiceMock.createStartContract(); + (Api.exportList as jest.Mock).mockResolvedValue(new Blob()); + }); + + it('invokes Api.exportList', async () => { + const { result } = renderHook(() => useExportList()); + await act(() => result.current.start({ http: httpMock, id: 'list' })); + + expect(Api.exportList).toHaveBeenCalledWith( + expect.objectContaining({ http: httpMock, id: 'list' }) + ); + }); +}); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts new file mode 100644 index 0000000000000..41ccd21e07981 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts @@ -0,0 +1,18 @@ +/* + * 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 { useAsyncTask } from '../../common/hooks/use_async_task'; +import { ExportListParams, exportList } from '../api'; + +export type ExportListTaskArgs = Omit; + +const exportListTask = ( + { signal }: AbortController, + args: ExportListTaskArgs +): ReturnType => exportList({ signal, ...args }); + +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export const useExportList = () => useAsyncTask(exportListTask); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts new file mode 100644 index 0000000000000..585264862b676 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts @@ -0,0 +1,33 @@ +/* + * 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 { act, renderHook } from '@testing-library/react-hooks'; + +import * as Api from '../api'; +import { httpServiceMock } from '../../../../../../src/core/public/mocks'; +import { getFoundListSchemaMock } from '../../../common/schemas/response/found_list_schema.mock'; + +import { useFindLists } from './use_find_lists'; + +jest.mock('../api'); + +describe('useFindLists', () => { + let httpMock: ReturnType; + + beforeEach(() => { + httpMock = httpServiceMock.createStartContract(); + (Api.findLists as jest.Mock).mockResolvedValue(getFoundListSchemaMock()); + }); + + it('invokes Api.findLists', async () => { + const { result } = renderHook(() => useFindLists()); + await act(() => result.current.start({ http: httpMock, pageIndex: 1, pageSize: 10 })); + + expect(Api.findLists).toHaveBeenCalledWith( + expect.objectContaining({ http: httpMock, pageIndex: 1, pageSize: 10 }) + ); + }); +}); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts new file mode 100644 index 0000000000000..f58358ef6e4c4 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts @@ -0,0 +1,18 @@ +/* + * 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 { useAsyncTask } from '../../common/hooks/use_async_task'; +import { FindListsParams, findLists } from '../api'; + +export type FindListsTaskArgs = Omit; + +const findListsTask = ( + { signal }: AbortController, + args: FindListsTaskArgs +): ReturnType => findLists({ signal, ...args }); + +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export const useFindLists = () => useAsyncTask(findListsTask); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts new file mode 100644 index 0000000000000..952e324233e05 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts @@ -0,0 +1,90 @@ +/* + * 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 { act, renderHook } from '@testing-library/react-hooks'; + +import { httpServiceMock } from '../../../../../../src/core/public/mocks'; +import { getListResponseMock } from '../../../common/schemas/response/list_schema.mock'; +import * as Api from '../api'; + +import { useImportList } from './use_import_list'; + +jest.mock('../api'); + +describe('useImportList', () => { + let httpMock: ReturnType; + + beforeEach(() => { + httpMock = httpServiceMock.createStartContract(); + (Api.importList as jest.Mock).mockResolvedValue(getListResponseMock()); + }); + + it('does not invoke importList if start was not called', () => { + renderHook(() => useImportList()); + expect(Api.importList).not.toHaveBeenCalled(); + }); + + it('invokes Api.importList', async () => { + const fileMock = ('my file' as unknown) as File; + + const { result } = renderHook(() => useImportList()); + + await act(() => + result.current.start({ + file: fileMock, + http: httpMock, + listId: 'my_list_id', + type: 'keyword', + }) + ); + + expect(Api.importList).toHaveBeenCalledWith( + expect.objectContaining({ + file: fileMock, + listId: 'my_list_id', + type: 'keyword', + }) + ); + }); + + it('populates result with the response of Api.importList', async () => { + const fileMock = ('my file' as unknown) as File; + + const { result } = renderHook(() => useImportList()); + + await act(() => + result.current.start({ + file: fileMock, + http: httpMock, + listId: 'my_list_id', + type: 'keyword', + }) + ); + + expect(result.current.result).toEqual(getListResponseMock()); + }); + + it('start rejects and error is populated if importList rejects', async () => { + expect.assertions(3); + const fileMock = ('my file' as unknown) as File; + (Api.importList as jest.Mock).mockRejectedValue(new Error('whoops')); + const { result } = renderHook(() => useImportList()); + + await act(() => + result.current + .start({ + file: fileMock, + http: httpMock, + listId: 'my_list_id', + type: 'keyword', + }) + .catch((e) => expect(e).toEqual(new Error('whoops'))) + ); + + expect(result.current.result).toBeUndefined(); + expect(result.current.error).toEqual(new Error('whoops')); + }); +}); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts new file mode 100644 index 0000000000000..de77fff0db250 --- /dev/null +++ b/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts @@ -0,0 +1,18 @@ +/* + * 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 { useAsyncTask } from '../../common/hooks/use_async_task'; +import { ImportListParams, importList } from '../api'; + +export type ImportListTaskArgs = Omit; + +const importListTask = ( + { signal }: AbortController, + args: ImportListTaskArgs +): ReturnType => importList({ signal, ...args }); + +// eslint-disable-next-line @typescript-eslint/explicit-function-return-type +export const useImportList = () => useAsyncTask(importListTask); diff --git a/x-pack/plugins/lists/server/routes/export_list_item_route.ts b/x-pack/plugins/lists/server/routes/export_list_item_route.ts index 32b99bfc512bf..8b50f4666085a 100644 --- a/x-pack/plugins/lists/server/routes/export_list_item_route.ts +++ b/x-pack/plugins/lists/server/routes/export_list_item_route.ts @@ -47,7 +47,7 @@ export const exportListItemRoute = (router: IRouter): void => { body: stream, headers: { 'Content-Disposition': `attachment; filename="${fileName}"`, - 'Content-Type': 'text/plain', + 'Content-Type': 'application/ndjson', }, }); } From c83dabcf91e3e7c6ec3c1a5cbabc8ed2f8f9290c Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 18 Jun 2020 21:36:11 -0500 Subject: [PATCH 02/14] Fix type errors in hook tests These were not caught locally as I was accidentally running typescript without the full project. --- .../common/hooks/use_async_task.test.ts | 30 ++++++++++++------- .../lists/hooks/use_import_list.test.ts | 28 +++++++++-------- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts index 900a9d25a7563..8acb60b4c11f9 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts @@ -21,22 +21,26 @@ describe('useAsyncTask', () => { }); it('invokes the task when start is called', async () => { - const { result } = renderHook(() => useAsyncTask(task)); + const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); - await act(() => result.current.start({})); + act(() => { + result.current.start({}); + }); + await waitForNextUpdate(); expect(task).toHaveBeenCalled(); }); it('invokes the task with a signal and start args', async () => { - const { result } = renderHook(() => useAsyncTask(task)); + const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); - await act(() => + act(() => { result.current.start({ arg1: 'value1', arg2: 'value2', - }) - ); + }); + }); + await waitForNextUpdate(); expect(task).toHaveBeenCalledWith(expect.any(AbortController), { arg1: 'value1', @@ -45,9 +49,12 @@ describe('useAsyncTask', () => { }); it('populates result with the resolved value of the task', async () => { - const { result } = renderHook(() => useAsyncTask(task)); + const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); - await act(() => result.current.start({})); + act(() => { + result.current.start({}); + }); + await waitForNextUpdate(); expect(result.current.result).toEqual('resolved value'); }); @@ -55,9 +62,12 @@ describe('useAsyncTask', () => { it('start rejects and error is populated if task rejects', async () => { expect.assertions(3); task.mockRejectedValue(new Error('whoops')); - const { result } = renderHook(() => useAsyncTask(task)); + const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); - await act(() => result.current.start({}).catch((e) => expect(e).toEqual(new Error('whoops')))); + act(() => { + result.current.start({}).catch((e) => expect(e).toEqual(new Error('whoops'))); + }); + await waitForNextUpdate(); expect(result.current.result).toBeUndefined(); expect(result.current.error).toEqual(new Error('whoops')); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts index 952e324233e05..cdb9e32a899f0 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts @@ -30,16 +30,17 @@ describe('useImportList', () => { it('invokes Api.importList', async () => { const fileMock = ('my file' as unknown) as File; - const { result } = renderHook(() => useImportList()); + const { result, waitForNextUpdate } = renderHook(() => useImportList()); - await act(() => + act(() => { result.current.start({ file: fileMock, http: httpMock, listId: 'my_list_id', type: 'keyword', - }) - ); + }); + }); + await waitForNextUpdate(); expect(Api.importList).toHaveBeenCalledWith( expect.objectContaining({ @@ -53,16 +54,17 @@ describe('useImportList', () => { it('populates result with the response of Api.importList', async () => { const fileMock = ('my file' as unknown) as File; - const { result } = renderHook(() => useImportList()); + const { result, waitForNextUpdate } = renderHook(() => useImportList()); - await act(() => + act(() => { result.current.start({ file: fileMock, http: httpMock, listId: 'my_list_id', type: 'keyword', - }) - ); + }); + }); + await waitForNextUpdate(); expect(result.current.result).toEqual(getListResponseMock()); }); @@ -71,9 +73,9 @@ describe('useImportList', () => { expect.assertions(3); const fileMock = ('my file' as unknown) as File; (Api.importList as jest.Mock).mockRejectedValue(new Error('whoops')); - const { result } = renderHook(() => useImportList()); + const { result, waitForNextUpdate } = renderHook(() => useImportList()); - await act(() => + act(() => { result.current .start({ file: fileMock, @@ -81,8 +83,10 @@ describe('useImportList', () => { listId: 'my_list_id', type: 'keyword', }) - .catch((e) => expect(e).toEqual(new Error('whoops'))) - ); + .catch((e) => expect(e).toEqual(new Error('whoops'))); + }); + + await waitForNextUpdate(); expect(result.current.result).toBeUndefined(); expect(result.current.error).toEqual(new Error('whoops')); From 247f8e45bec37872770d2239c4fed1702e0f036a Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Wed, 24 Jun 2020 14:50:11 -0500 Subject: [PATCH 03/14] Document current limitations of useAsyncTask --- .../public/common/hooks/use_async_task.ts | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts index 0c47c152cb385..26b7d6fcaa5a6 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts @@ -7,6 +7,12 @@ import { useCallback, useRef } from 'react'; import { useAsyncFn } from 'react-use'; +// Params can be generalized to a ...rest parameter extending unknown[] once https://github.com/microsoft/TypeScript/pull/39094 is available. +// for now, the task must still receive unknown as a second argument, and an argument must be passed to start() +export type UseAsyncTask = ( + task: (...args: [AbortController, Params]) => Promise +) => AsyncTask; + export interface AsyncTask { start: (params: Params) => Promise; abort: () => void; @@ -15,18 +21,20 @@ export interface AsyncTask { result: Result | undefined; } -export type UseAsyncTask = ( - func: (...args: [AbortController, Params]) => Promise -) => AsyncTask; - -export const useAsyncTask: UseAsyncTask = (fn) => { +/** + * + * @param task Async function receiving an AbortController and optional arguments + * + * @returns An {@link AsyncTask} containing the underlying task's state along with start/abort helpers + */ +export const useAsyncTask: UseAsyncTask = (task) => { const ctrl = useRef(new AbortController()); const abort = useCallback((): void => { ctrl.current.abort(); }, []); // @ts-ignore typings are incorrect, see: https://github.com/streamich/react-use/pull/589 - const [state, initiator] = useAsyncFn(fn, [fn]); + const [state, initiator] = useAsyncFn(task, [task]); const start = useCallback( (args) => { From b65722910927791a1a16535e467e1a4b09a21932 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Wed, 24 Jun 2020 15:26:38 -0500 Subject: [PATCH 04/14] Defines a new validation function that returns an Either instead of a tuple This allows callers to further leverage fp-ts functions as needed. --- .../security_solution/common/validate.test.ts | 21 ++++++++++++++++++- .../security_solution/common/validate.ts | 13 +++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/security_solution/common/validate.test.ts b/x-pack/plugins/security_solution/common/validate.test.ts index 032f6d9590168..df450300b6ae4 100644 --- a/x-pack/plugins/security_solution/common/validate.test.ts +++ b/x-pack/plugins/security_solution/common/validate.test.ts @@ -9,9 +9,10 @@ * you may not use this file except in compliance with the Elastic License. */ +import { left, right } from 'fp-ts/lib/Either'; import * as t from 'io-ts'; -import { validate } from './validate'; +import { validate, validateEither } from './validate'; describe('validate', () => { test('it should do a validation correctly', () => { @@ -32,3 +33,21 @@ describe('validate', () => { expect(errors).toEqual('Invalid value "some other value" supplied to "a"'); }); }); + +describe('validateEither', () => { + it('returns the decoded payload as right if valid', () => { + const schema = t.exact(t.type({ a: t.number })); + const payload = { a: 1 }; + const result = validateEither(payload, schema); + + expect(result).toEqual(right(payload)); + }); + + it('returns an error string if invalid', () => { + const schema = t.exact(t.type({ a: t.number })); + const payload = { a: 'some other value' }; + const result = validateEither(payload, schema); + + expect(result).toEqual(left('Invalid value "some other value" supplied to "a"')); + }); +}); diff --git a/x-pack/plugins/security_solution/common/validate.ts b/x-pack/plugins/security_solution/common/validate.ts index db9e286e2ebc2..cd6ed6205026e 100644 --- a/x-pack/plugins/security_solution/common/validate.ts +++ b/x-pack/plugins/security_solution/common/validate.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { fold } from 'fp-ts/lib/Either'; +import { fold, Either, mapLeft } from 'fp-ts/lib/Either'; import { pipe } from 'fp-ts/lib/pipeable'; import * as t from 'io-ts'; import { exactCheck } from './exact_check'; @@ -23,3 +23,14 @@ export const validate = ( const right = (output: T): [T | null, string | null] => [output, null]; return pipe(checked, fold(left, right)); }; + +export const validateEither = ( + obj: unknown, + schema: T +): Either> => + pipe( + obj, + schema.decode, + (a: Either) => exactCheck(obj, a), + mapLeft((errors) => formatErrors(errors).join(',')) + ); From d627a640373a7c53d70828ad57682e6166d79777 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Wed, 24 Jun 2020 15:32:13 -0500 Subject: [PATCH 05/14] Remove duplicated copyright comment --- x-pack/plugins/security_solution/common/validate.test.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/x-pack/plugins/security_solution/common/validate.test.ts b/x-pack/plugins/security_solution/common/validate.test.ts index df450300b6ae4..efae3cbc1aa7c 100644 --- a/x-pack/plugins/security_solution/common/validate.test.ts +++ b/x-pack/plugins/security_solution/common/validate.test.ts @@ -3,11 +3,6 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -/* - * 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 { left, right } from 'fp-ts/lib/Either'; import * as t from 'io-ts'; From 8d2eb3a87255be0a049dd48d576a6f4ef2c8c2f3 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Wed, 24 Jun 2020 20:19:26 -0500 Subject: [PATCH 06/14] WIP: Perform request/response validations in the FP style * leverages new validateEither fn which returns an Either * constructs a pipeline that: * validates the payload * performs the API call * validates the response and short-circuits if any of those produce a Left value. It then converts the Either into a promise that either rejects with the Left or resolves with the Right. --- .../plugins/lists/common/siem_common_deps.ts | 2 +- x-pack/plugins/lists/public/lists/api.test.ts | 43 ++++++++++++++++++- x-pack/plugins/lists/public/lists/api.ts | 33 ++++++++++++-- .../security_solution/common/validate.test.ts | 4 +- .../security_solution/common/validate.ts | 4 +- 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/lists/common/siem_common_deps.ts b/x-pack/plugins/lists/common/siem_common_deps.ts index b1bb7d8aace36..dccc548985e77 100644 --- a/x-pack/plugins/lists/common/siem_common_deps.ts +++ b/x-pack/plugins/lists/common/siem_common_deps.ts @@ -9,5 +9,5 @@ export { DefaultUuid } from '../../security_solution/common/detection_engine/sch export { DefaultStringArray } from '../../security_solution/common/detection_engine/schemas/types/default_string_array'; export { exactCheck } from '../../security_solution/common/exact_check'; export { getPaths, foldLeftRight } from '../../security_solution/common/test_utils'; -export { validate } from '../../security_solution/common/validate'; +export { validate, validateEither } from '../../security_solution/common/validate'; export { formatErrors } from '../../security_solution/common/format_errors'; diff --git a/x-pack/plugins/lists/public/lists/api.test.ts b/x-pack/plugins/lists/public/lists/api.test.ts index 28700351db512..09ce284426090 100644 --- a/x-pack/plugins/lists/public/lists/api.test.ts +++ b/x-pack/plugins/lists/public/lists/api.test.ts @@ -6,6 +6,9 @@ import { HttpFetchOptions } from '../../../../../src/core/public'; import { httpServiceMock } from '../../../../../src/core/public/mocks'; +import { getDeleteListSchemaMock } from '../../common/schemas/request/delete_list_schema.mock'; +import { getListResponseMock } from '../../common/schemas/response/list_schema.mock'; +import { DeleteListSchema } from '../../common/schemas'; import { deleteList, exportList, findLists, importList } from './api'; @@ -17,11 +20,16 @@ describe('Value Lists API', () => { }); describe('deleteList', () => { + beforeEach(() => { + httpMock.fetch.mockResolvedValue(getListResponseMock()); + }); + it('DELETEs specifying the id as a query parameter', async () => { const abortCtrl = new AbortController(); + const payload: DeleteListSchema = getDeleteListSchemaMock(); await deleteList({ http: httpMock, - id: 'my_list', + ...payload, signal: abortCtrl.signal, }); @@ -29,10 +37,41 @@ describe('Value Lists API', () => { '/api/lists', expect.objectContaining({ method: 'DELETE', - query: { id: 'my_list' }, + query: getDeleteListSchemaMock(), }) ); }); + + it('rejects with an error if request payload is invalid (and does not make API call)', async () => { + const abortCtrl = new AbortController(); + const payload: Omit & { + id: number; + } = { ...getDeleteListSchemaMock(), id: 23 }; + + await expect( + deleteList({ + http: httpMock, + ...((payload as unknown) as DeleteListSchema), + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "23" supplied to "id"'); + expect(httpMock.fetch).not.toHaveBeenCalled(); + }); + + it('rejects with an error if response payload is invalid', async () => { + const abortCtrl = new AbortController(); + const payload: DeleteListSchema = getDeleteListSchemaMock(); + const badResponse = { ...getListResponseMock(), id: undefined }; + httpMock.fetch.mockResolvedValue(badResponse); + + await expect( + deleteList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "undefined" supplied to "id"'); + }); }); describe('findLists', () => { diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts index bac76215edff4..8af148211a719 100644 --- a/x-pack/plugins/lists/public/lists/api.ts +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -4,9 +4,20 @@ * you may not use this file except in compliance with the Elastic License. */ +import { chain, fromEither, tryCatch } from 'fp-ts/lib/TaskEither'; +import { fold } from 'fp-ts/lib/Either'; +import { pipe } from 'fp-ts/lib/pipeable'; + import { HttpStart } from '../../../../../src/core/public'; -import { FoundListSchema, ListSchema, Type } from '../../common/schemas'; +import { + FoundListSchema, + ListSchema, + Type, + deleteListSchema, + listSchema, +} from '../../common/schemas'; import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; +import { validateEither } from '../../common/siem_common_deps'; export interface FindListsParams { http: HttpStart; @@ -64,12 +75,28 @@ export interface DeleteListParams { signal: AbortSignal; } -export const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => { - return http.fetch(LIST_URL, { +const _deleteList = async ({ http, id, signal }: DeleteListParams): Promise => + http.fetch(LIST_URL, { method: 'DELETE', query: { id }, signal, }); + +export const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => { + const deleteWithValidations = pipe( + { id }, + (payload) => fromEither(validateEither(deleteListSchema, payload)), + chain((payload) => tryCatch(() => _deleteList({ http, signal, ...payload }), String)), + chain((response) => fromEither(validateEither(listSchema, response))) + ); + + return pipe( + await deleteWithValidations(), + fold( + (a) => Promise.reject(a), + (e) => Promise.resolve(e) + ) + ); }; export interface ExportListParams { diff --git a/x-pack/plugins/security_solution/common/validate.test.ts b/x-pack/plugins/security_solution/common/validate.test.ts index efae3cbc1aa7c..54a5baa7503f2 100644 --- a/x-pack/plugins/security_solution/common/validate.test.ts +++ b/x-pack/plugins/security_solution/common/validate.test.ts @@ -33,7 +33,7 @@ describe('validateEither', () => { it('returns the decoded payload as right if valid', () => { const schema = t.exact(t.type({ a: t.number })); const payload = { a: 1 }; - const result = validateEither(payload, schema); + const result = validateEither(schema, payload); expect(result).toEqual(right(payload)); }); @@ -41,7 +41,7 @@ describe('validateEither', () => { it('returns an error string if invalid', () => { const schema = t.exact(t.type({ a: t.number })); const payload = { a: 'some other value' }; - const result = validateEither(payload, schema); + const result = validateEither(schema, payload); expect(result).toEqual(left('Invalid value "some other value" supplied to "a"')); }); diff --git a/x-pack/plugins/security_solution/common/validate.ts b/x-pack/plugins/security_solution/common/validate.ts index cd6ed6205026e..c88008bb57610 100644 --- a/x-pack/plugins/security_solution/common/validate.ts +++ b/x-pack/plugins/security_solution/common/validate.ts @@ -25,8 +25,8 @@ export const validate = ( }; export const validateEither = ( - obj: unknown, - schema: T + schema: T, + obj: unknown ): Either> => pipe( obj, From d99ba5417bf482e8e40dcab930deb6e28930eeb9 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 25 Jun 2020 11:41:45 -0500 Subject: [PATCH 07/14] Adds helper function to convert a TaskEither back to a Promise This cleans up our validation pipeline considerably. --- .../lists/public/common/fp_utils.test.ts | 23 +++++++++++++++++++ .../plugins/lists/public/common/fp_utils.ts | 18 +++++++++++++++ x-pack/plugins/lists/public/lists/api.ts | 19 +++++---------- 3 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 x-pack/plugins/lists/public/common/fp_utils.test.ts create mode 100644 x-pack/plugins/lists/public/common/fp_utils.ts diff --git a/x-pack/plugins/lists/public/common/fp_utils.test.ts b/x-pack/plugins/lists/public/common/fp_utils.test.ts new file mode 100644 index 0000000000000..79042f4f9a72f --- /dev/null +++ b/x-pack/plugins/lists/public/common/fp_utils.test.ts @@ -0,0 +1,23 @@ +/* + * 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 { tryCatch } from 'fp-ts/lib/TaskEither'; + +import { toPromise } from './fp_utils'; + +describe('toPromise', () => { + it('rejects with left if TaskEither is left', async () => { + const task = tryCatch(() => Promise.reject(new Error('whoops')), String); + + await expect(toPromise(task)).rejects.toEqual('Error: whoops'); + }); + + it('resolves with right if TaskEither is right', async () => { + const task = tryCatch(() => Promise.resolve('success'), String); + + await expect(toPromise(task)).resolves.toEqual('success'); + }); +}); diff --git a/x-pack/plugins/lists/public/common/fp_utils.ts b/x-pack/plugins/lists/public/common/fp_utils.ts new file mode 100644 index 0000000000000..04e1033879476 --- /dev/null +++ b/x-pack/plugins/lists/public/common/fp_utils.ts @@ -0,0 +1,18 @@ +/* + * 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 { pipe } from 'fp-ts/lib/pipeable'; +import { TaskEither } from 'fp-ts/lib/TaskEither'; +import { fold } from 'fp-ts/lib/Either'; + +export const toPromise = async (taskEither: TaskEither): Promise => + pipe( + await taskEither(), + fold( + (e) => Promise.reject(e), + (a) => Promise.resolve(a) + ) + ); diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts index 8af148211a719..35bfeb962c1b6 100644 --- a/x-pack/plugins/lists/public/lists/api.ts +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -5,7 +5,7 @@ */ import { chain, fromEither, tryCatch } from 'fp-ts/lib/TaskEither'; -import { fold } from 'fp-ts/lib/Either'; +import { flow } from 'fp-ts/lib/function'; import { pipe } from 'fp-ts/lib/pipeable'; import { HttpStart } from '../../../../../src/core/public'; @@ -18,6 +18,7 @@ import { } from '../../common/schemas'; import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; import { validateEither } from '../../common/siem_common_deps'; +import { toPromise } from '../common/fp_utils'; export interface FindListsParams { http: HttpStart; @@ -82,23 +83,15 @@ const _deleteList = async ({ http, id, signal }: DeleteListParams): Promise => { - const deleteWithValidations = pipe( +export const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => + pipe( { id }, (payload) => fromEither(validateEither(deleteListSchema, payload)), chain((payload) => tryCatch(() => _deleteList({ http, signal, ...payload }), String)), - chain((response) => fromEither(validateEither(listSchema, response))) + chain((response) => fromEither(validateEither(listSchema, response))), + flow(toPromise) ); - return pipe( - await deleteWithValidations(), - fold( - (a) => Promise.reject(a), - (e) => Promise.resolve(e) - ) - ); -}; - export interface ExportListParams { http: HttpStart; id: string; From 692045964e1a4cdd6d23b03b18ee2c0100771716 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 25 Jun 2020 15:16:48 -0500 Subject: [PATCH 08/14] Adds request/response validations to findLists * refactors private API functions to accept the encoded request schema (i.e. snake cased) * refactors validateEither to use `schema.validate` instead of `schema.decode` since we don't actually want the decoded value, we just want to verify that it'll be able to be decoded on the backend. --- .../schemas/request/find_list_schema.ts | 6 +- x-pack/plugins/lists/public/lists/api.test.ts | 62 +++++++++++++++---- x-pack/plugins/lists/public/lists/api.ts | 58 +++++++++++++---- x-pack/plugins/lists/public/lists/types.ts | 13 ++++ .../security_solution/common/validate.test.ts | 2 +- .../security_solution/common/validate.ts | 9 ++- 6 files changed, 117 insertions(+), 33 deletions(-) create mode 100644 x-pack/plugins/lists/public/lists/types.ts diff --git a/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts b/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts index c29ab4f5360dd..c1bd22f322dd9 100644 --- a/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts @@ -9,7 +9,6 @@ import * as t from 'io-ts'; import { cursor, filter, sort_field, sort_order } from '../common/schemas'; -import { RequiredKeepUndefined } from '../../types'; import { StringToPositiveNumber } from '../types/string_to_positive_number'; export const findListSchema = t.exact( @@ -23,6 +22,7 @@ export const findListSchema = t.exact( }) ); -export type FindListSchemaPartial = t.TypeOf; +type FindListSchema = typeof findListSchema; -export type FindListSchema = RequiredKeepUndefined>; +export type FindListSchemaDecoded = t.TypeOf; +export type FindListSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/public/lists/api.test.ts b/x-pack/plugins/lists/public/lists/api.test.ts index 09ce284426090..fa3cea9794fb3 100644 --- a/x-pack/plugins/lists/public/lists/api.test.ts +++ b/x-pack/plugins/lists/public/lists/api.test.ts @@ -6,11 +6,18 @@ import { HttpFetchOptions } from '../../../../../src/core/public'; import { httpServiceMock } from '../../../../../src/core/public/mocks'; -import { getDeleteListSchemaMock } from '../../common/schemas/request/delete_list_schema.mock'; import { getListResponseMock } from '../../common/schemas/response/list_schema.mock'; -import { DeleteListSchema } from '../../common/schemas'; - -import { deleteList, exportList, findLists, importList } from './api'; +import { getFoundListSchemaMock } from '../../common/schemas/response/found_list_schema.mock'; + +import { + DeleteListParams, + FindListsParams, + deleteList, + exportList, + findLists, + importList, +} from './api'; +import { ApiPayload } from './types'; describe('Value Lists API', () => { let httpMock: ReturnType; @@ -26,7 +33,7 @@ describe('Value Lists API', () => { it('DELETEs specifying the id as a query parameter', async () => { const abortCtrl = new AbortController(); - const payload: DeleteListSchema = getDeleteListSchemaMock(); + const payload: ApiPayload = { id: 'list-id' }; await deleteList({ http: httpMock, ...payload, @@ -37,21 +44,21 @@ describe('Value Lists API', () => { '/api/lists', expect.objectContaining({ method: 'DELETE', - query: getDeleteListSchemaMock(), + query: { id: 'list-id' }, }) ); }); it('rejects with an error if request payload is invalid (and does not make API call)', async () => { const abortCtrl = new AbortController(); - const payload: Omit & { + const payload: Omit, 'id'> & { id: number; - } = { ...getDeleteListSchemaMock(), id: 23 }; + } = { id: 23 }; await expect( deleteList({ http: httpMock, - ...((payload as unknown) as DeleteListSchema), + ...((payload as unknown) as ApiPayload), signal: abortCtrl.signal, }) ).rejects.toEqual('Invalid value "23" supplied to "id"'); @@ -60,7 +67,7 @@ describe('Value Lists API', () => { it('rejects with an error if response payload is invalid', async () => { const abortCtrl = new AbortController(); - const payload: DeleteListSchema = getDeleteListSchemaMock(); + const payload: ApiPayload = { id: 'list-id' }; const badResponse = { ...getListResponseMock(), id: undefined }; httpMock.fetch.mockResolvedValue(badResponse); @@ -75,11 +82,15 @@ describe('Value Lists API', () => { }); describe('findLists', () => { + beforeEach(() => { + httpMock.fetch.mockResolvedValue(getFoundListSchemaMock()); + }); + it('GETs from the lists endpoint', async () => { const abortCtrl = new AbortController(); await findLists({ http: httpMock, - pageIndex: 0, + pageIndex: 1, pageSize: 10, signal: abortCtrl.signal, }); @@ -108,6 +119,35 @@ describe('Value Lists API', () => { }) ); }); + + it('rejects with an error if request payload is invalid (and does not make API call)', async () => { + const abortCtrl = new AbortController(); + const payload: ApiPayload = { pageIndex: 10, pageSize: 0 }; + + await expect( + findLists({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "0" supplied to "per_page"'); + expect(httpMock.fetch).not.toHaveBeenCalled(); + }); + + it('rejects with an error if response payload is invalid', async () => { + const abortCtrl = new AbortController(); + const payload: ApiPayload = { pageIndex: 1, pageSize: 10 }; + const badResponse = { ...getFoundListSchemaMock(), cursor: undefined }; + httpMock.fetch.mockResolvedValue(badResponse); + + await expect( + findLists({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "undefined" supplied to "cursor"'); + }); }); describe('importList', () => { diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts index 35bfeb962c1b6..0966fe419aa4f 100644 --- a/x-pack/plugins/lists/public/lists/api.ts +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -10,16 +10,39 @@ import { pipe } from 'fp-ts/lib/pipeable'; import { HttpStart } from '../../../../../src/core/public'; import { + FindListSchemaEncoded, FoundListSchema, ListSchema, Type, deleteListSchema, + findListSchema, + foundListSchema, listSchema, } from '../../common/schemas'; import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; import { validateEither } from '../../common/siem_common_deps'; import { toPromise } from '../common/fp_utils'; +import { ApiParams } from './types'; + +const findLists = async ({ + http, + cursor, + page, + per_page, + signal, +}: ApiParams & FindListSchemaEncoded): Promise => { + return http.fetch(`${LIST_URL}/_find`, { + method: 'GET', + query: { + cursor, + page, + per_page, + }, + signal, + }); +}; + export interface FindListsParams { http: HttpStart; pageSize: number | undefined; @@ -27,21 +50,24 @@ export interface FindListsParams { signal: AbortSignal; } -export const findLists = async ({ +const findListsWithValidation = async ({ http, pageIndex, pageSize, signal, -}: FindListsParams): Promise => { - return http.fetch(`${LIST_URL}/_find`, { - method: 'GET', - query: { - page: pageIndex, - per_page: pageSize, +}: FindListsParams): Promise => + pipe( + { + page: String(pageIndex), + per_page: String(pageSize), }, - signal, - }); -}; + (payload) => fromEither(validateEither(findListSchema, payload)), + chain((payload) => tryCatch(() => findLists({ http, signal, ...payload }), String)), + chain((response) => fromEither(validateEither(foundListSchema, response))), + flow(toPromise) + ); + +export { findListsWithValidation as findLists }; export interface ImportListParams { http: HttpStart; @@ -76,22 +102,28 @@ export interface DeleteListParams { signal: AbortSignal; } -const _deleteList = async ({ http, id, signal }: DeleteListParams): Promise => +const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => http.fetch(LIST_URL, { method: 'DELETE', query: { id }, signal, }); -export const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => +const deleteListWithValidation = async ({ + http, + id, + signal, +}: DeleteListParams): Promise => pipe( { id }, (payload) => fromEither(validateEither(deleteListSchema, payload)), - chain((payload) => tryCatch(() => _deleteList({ http, signal, ...payload }), String)), + chain((payload) => tryCatch(() => deleteList({ http, signal, ...payload }), String)), chain((response) => fromEither(validateEither(listSchema, response))), flow(toPromise) ); +export { deleteListWithValidation as deleteList }; + export interface ExportListParams { http: HttpStart; id: string; diff --git a/x-pack/plugins/lists/public/lists/types.ts b/x-pack/plugins/lists/public/lists/types.ts new file mode 100644 index 0000000000000..ad15df04c241c --- /dev/null +++ b/x-pack/plugins/lists/public/lists/types.ts @@ -0,0 +1,13 @@ +/* + * 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 { HttpStart } from '../../../../../src/core/public'; + +export interface ApiParams { + http: HttpStart; + signal: AbortSignal; +} +export type ApiPayload = Omit; diff --git a/x-pack/plugins/security_solution/common/validate.test.ts b/x-pack/plugins/security_solution/common/validate.test.ts index 54a5baa7503f2..b2217099fca19 100644 --- a/x-pack/plugins/security_solution/common/validate.test.ts +++ b/x-pack/plugins/security_solution/common/validate.test.ts @@ -30,7 +30,7 @@ describe('validate', () => { }); describe('validateEither', () => { - it('returns the decoded payload as right if valid', () => { + it('returns the ORIGINAL payload as right if valid', () => { const schema = t.exact(t.type({ a: t.number })); const payload = { a: 1 }; const result = validateEither(schema, payload); diff --git a/x-pack/plugins/security_solution/common/validate.ts b/x-pack/plugins/security_solution/common/validate.ts index c88008bb57610..f36df38c2a90d 100644 --- a/x-pack/plugins/security_solution/common/validate.ts +++ b/x-pack/plugins/security_solution/common/validate.ts @@ -24,13 +24,12 @@ export const validate = ( return pipe(checked, fold(left, right)); }; -export const validateEither = ( +export const validateEither = ( schema: T, - obj: unknown -): Either> => + obj: A +): Either => pipe( obj, - schema.decode, - (a: Either) => exactCheck(obj, a), + (a) => schema.validate(a, t.getDefaultContext(schema.asDecoder())), mapLeft((errors) => formatErrors(errors).join(',')) ); From 6d6b5cc8eb83e9ffa22f1114ed095e3419b1a576 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 25 Jun 2020 15:30:40 -0500 Subject: [PATCH 09/14] Refactor our API types --- .../schemas/request/delete_list_schema.ts | 3 +- x-pack/plugins/lists/public/lists/api.test.ts | 11 +---- x-pack/plugins/lists/public/lists/api.ts | 44 ++++++------------- x-pack/plugins/lists/public/lists/types.ts | 20 +++++++++ 4 files changed, 37 insertions(+), 41 deletions(-) diff --git a/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts b/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts index fd6aa5b85f81a..8fd0afc415a8c 100644 --- a/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts @@ -16,4 +16,5 @@ export const deleteListSchema = t.exact( }) ); -export type DeleteListSchema = t.TypeOf; +export type DeleteListSchemaDecoded = t.TypeOf; +export type DeleteListSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/public/lists/api.test.ts b/x-pack/plugins/lists/public/lists/api.test.ts index fa3cea9794fb3..60d19196f1d06 100644 --- a/x-pack/plugins/lists/public/lists/api.test.ts +++ b/x-pack/plugins/lists/public/lists/api.test.ts @@ -9,15 +9,8 @@ import { httpServiceMock } from '../../../../../src/core/public/mocks'; import { getListResponseMock } from '../../common/schemas/response/list_schema.mock'; import { getFoundListSchemaMock } from '../../common/schemas/response/found_list_schema.mock'; -import { - DeleteListParams, - FindListsParams, - deleteList, - exportList, - findLists, - importList, -} from './api'; -import { ApiPayload } from './types'; +import { deleteList, exportList, findLists, importList } from './api'; +import { ApiPayload, DeleteListParams, FindListsParams } from './types'; describe('Value Lists API', () => { let httpMock: ReturnType; diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts index 0966fe419aa4f..a3a808ab883d5 100644 --- a/x-pack/plugins/lists/public/lists/api.ts +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -8,12 +8,11 @@ import { chain, fromEither, tryCatch } from 'fp-ts/lib/TaskEither'; import { flow } from 'fp-ts/lib/function'; import { pipe } from 'fp-ts/lib/pipeable'; -import { HttpStart } from '../../../../../src/core/public'; import { + DeleteListSchemaEncoded, FindListSchemaEncoded, FoundListSchema, ListSchema, - Type, deleteListSchema, findListSchema, foundListSchema, @@ -23,7 +22,13 @@ import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; import { validateEither } from '../../common/siem_common_deps'; import { toPromise } from '../common/fp_utils'; -import { ApiParams } from './types'; +import { + ApiParams, + DeleteListParams, + ExportListParams, + FindListsParams, + ImportListParams, +} from './types'; const findLists = async ({ http, @@ -43,13 +48,6 @@ const findLists = async ({ }); }; -export interface FindListsParams { - http: HttpStart; - pageSize: number | undefined; - pageIndex: number | undefined; - signal: AbortSignal; -} - const findListsWithValidation = async ({ http, pageIndex, @@ -69,14 +67,6 @@ const findListsWithValidation = async ({ export { findListsWithValidation as findLists }; -export interface ImportListParams { - http: HttpStart; - file: File; - listId: string | undefined; - signal: AbortSignal; - type: Type | undefined; -} - export const importList = async ({ file, http, @@ -96,13 +86,11 @@ export const importList = async ({ }); }; -export interface DeleteListParams { - http: HttpStart; - id: string; - signal: AbortSignal; -} - -const deleteList = async ({ http, id, signal }: DeleteListParams): Promise => +const deleteList = async ({ + http, + id, + signal, +}: ApiParams & DeleteListSchemaEncoded): Promise => http.fetch(LIST_URL, { method: 'DELETE', query: { id }, @@ -124,12 +112,6 @@ const deleteListWithValidation = async ({ export { deleteListWithValidation as deleteList }; -export interface ExportListParams { - http: HttpStart; - id: string; - signal: AbortSignal; -} - export const exportList = async ({ http, id, signal }: ExportListParams): Promise => { return http.fetch(`${LIST_ITEM_URL}/_export`, { method: 'POST', diff --git a/x-pack/plugins/lists/public/lists/types.ts b/x-pack/plugins/lists/public/lists/types.ts index ad15df04c241c..a3f12cd97cb5e 100644 --- a/x-pack/plugins/lists/public/lists/types.ts +++ b/x-pack/plugins/lists/public/lists/types.ts @@ -5,9 +5,29 @@ */ import { HttpStart } from '../../../../../src/core/public'; +import { Type } from '../../common/schemas'; export interface ApiParams { http: HttpStart; signal: AbortSignal; } export type ApiPayload = Omit; + +export interface FindListsParams extends ApiParams { + pageSize: number | undefined; + pageIndex: number | undefined; +} + +export interface ImportListParams extends ApiParams { + file: File; + listId: string | undefined; + type: Type | undefined; +} + +export interface DeleteListParams extends ApiParams { + id: string; +} + +export interface ExportListParams extends ApiParams { + id: string; +} From 0f223aa1e723bc6f6c35f89e35abf1acc7dd11c8 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 25 Jun 2020 16:42:24 -0500 Subject: [PATCH 10/14] Add request/response validation to import/export functions --- .../request/export_list_item_query_schema.ts | 3 +- .../import_list_item_query_schema.mock.ts | 4 +- .../request/import_list_item_query_schema.ts | 8 +- .../request/import_list_item_schema.ts | 3 +- x-pack/plugins/lists/public/lists/api.test.ts | 120 ++++++++++++++++-- x-pack/plugins/lists/public/lists/api.ts | 72 +++++++++-- x-pack/plugins/lists/public/lists/types.ts | 2 +- 7 files changed, 185 insertions(+), 27 deletions(-) diff --git a/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts b/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts index 14b201bf8089d..70496c504b77c 100644 --- a/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts @@ -17,4 +17,5 @@ export const exportListItemQuerySchema = t.exact( }) ); -export type ExportListItemQuerySchema = t.TypeOf; +export type ExportListItemQuerySchemaDecoded = t.TypeOf; +export type ExportListItemQuerySchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts index 6713083e6a49b..5731916a483ff 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts @@ -6,9 +6,9 @@ import { LIST_ID, TYPE } from '../../constants.mock'; -import { ImportListItemQuerySchema } from './import_list_item_query_schema'; +import { ImportListItemQuerySchemaDecoded } from './import_list_item_query_schema'; -export const getImportListItemQuerySchemaMock = (): ImportListItemQuerySchema => ({ +export const getImportListItemQuerySchemaMock = (): ImportListItemQuerySchemaDecoded => ({ list_id: LIST_ID, type: TYPE, }); diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts index 73d9a53a41e4f..d57d7f8c02b5a 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts @@ -9,11 +9,11 @@ import * as t from 'io-ts'; import { list_id, type } from '../common/schemas'; -import { Identity, RequiredKeepUndefined } from '../../types'; +import { Identity } from '../../types'; export const importListItemQuerySchema = t.exact(t.partial({ list_id, type })); export type ImportListItemQuerySchemaPartial = Identity>; -export type ImportListItemQuerySchema = RequiredKeepUndefined< - t.TypeOf ->; + +export type ImportListItemQuerySchemaDecoded = t.TypeOf; +export type ImportListItemQuerySchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts index ee6a2aa0b339a..b24c60aeaff7c 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts @@ -16,4 +16,5 @@ export const importListItemSchema = t.exact( }) ); -export type ImportListItemSchema = t.TypeOf; +export type ImportListItemSchemaDecoded = t.TypeOf; +export type ImportListItemSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/public/lists/api.test.ts b/x-pack/plugins/lists/public/lists/api.test.ts index 60d19196f1d06..38556e2eabc18 100644 --- a/x-pack/plugins/lists/public/lists/api.test.ts +++ b/x-pack/plugins/lists/public/lists/api.test.ts @@ -10,7 +10,13 @@ import { getListResponseMock } from '../../common/schemas/response/list_schema.m import { getFoundListSchemaMock } from '../../common/schemas/response/found_list_schema.mock'; import { deleteList, exportList, findLists, importList } from './api'; -import { ApiPayload, DeleteListParams, FindListsParams } from './types'; +import { + ApiPayload, + DeleteListParams, + ExportListParams, + FindListsParams, + ImportListParams, +} from './types'; describe('Value Lists API', () => { let httpMock: ReturnType; @@ -144,12 +150,16 @@ describe('Value Lists API', () => { }); describe('importList', () => { + beforeEach(() => { + httpMock.fetch.mockResolvedValue(getListResponseMock()); + }); + it('POSTs the file', async () => { const abortCtrl = new AbortController(); - const fileMock = ('my file' as unknown) as File; + const file = new File([], 'name'); await importList({ - file: fileMock, + file, http: httpMock, listId: 'my_list', signal: abortCtrl.signal, @@ -167,15 +177,15 @@ describe('Value Lists API', () => { [unknown, HttpFetchOptions] >; const actualFile = (body as FormData).get('file'); - expect(actualFile).toEqual('my file'); + expect(actualFile).toEqual(file); }); it('sends type and id as query parameters', async () => { const abortCtrl = new AbortController(); - const fileMock = ('my file' as unknown) as File; + const file = new File([], 'name'); await importList({ - file: fileMock, + file, http: httpMock, listId: 'my_list', signal: abortCtrl.signal, @@ -189,15 +199,76 @@ describe('Value Lists API', () => { }) ); }); + + it('rejects with an error if request body is invalid (and does not make API call)', async () => { + const abortCtrl = new AbortController(); + const payload: ApiPayload = { + file: (undefined as unknown) as File, + listId: 'list-id', + type: 'ip', + }; + + await expect( + importList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "undefined" supplied to "file"'); + expect(httpMock.fetch).not.toHaveBeenCalled(); + }); + + it('rejects with an error if request params are invalid (and does not make API call)', async () => { + const abortCtrl = new AbortController(); + const file = new File([], 'name'); + const payload: ApiPayload = { + file, + listId: 'list-id', + type: 'other' as 'ip', + }; + + await expect( + importList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "other" supplied to "type"'); + expect(httpMock.fetch).not.toHaveBeenCalled(); + }); + + it('rejects with an error if response payload is invalid', async () => { + const abortCtrl = new AbortController(); + const file = new File([], 'name'); + const payload: ApiPayload = { + file, + listId: 'list-id', + type: 'ip', + }; + const badResponse = { ...getListResponseMock(), id: undefined }; + httpMock.fetch.mockResolvedValue(badResponse); + + await expect( + importList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "undefined" supplied to "id"'); + }); }); describe('exportList', () => { + beforeEach(() => { + httpMock.fetch.mockResolvedValue(getListResponseMock()); + }); + it('POSTs to the export endpoint', async () => { const abortCtrl = new AbortController(); await exportList({ http: httpMock, - id: 'my_list', + listId: 'my_list', signal: abortCtrl.signal, }); expect(httpMock.fetch).toHaveBeenCalledWith( @@ -213,7 +284,7 @@ describe('Value Lists API', () => { await exportList({ http: httpMock, - id: 'my_list', + listId: 'my_list', signal: abortCtrl.signal, }); expect(httpMock.fetch).toHaveBeenCalledWith( @@ -223,5 +294,38 @@ describe('Value Lists API', () => { }) ); }); + + it('rejects with an error if request params are invalid (and does not make API call)', async () => { + const abortCtrl = new AbortController(); + const payload: ApiPayload = { + listId: (23 as unknown) as string, + }; + + await expect( + exportList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "23" supplied to "list_id"'); + expect(httpMock.fetch).not.toHaveBeenCalled(); + }); + + it('rejects with an error if response payload is invalid', async () => { + const abortCtrl = new AbortController(); + const payload: ApiPayload = { + listId: 'list-id', + }; + const badResponse = { ...getListResponseMock(), id: undefined }; + httpMock.fetch.mockResolvedValue(badResponse); + + await expect( + exportList({ + http: httpMock, + ...payload, + signal: abortCtrl.signal, + }) + ).rejects.toEqual('Invalid value "undefined" supplied to "id"'); + }); }); }); diff --git a/x-pack/plugins/lists/public/lists/api.ts b/x-pack/plugins/lists/public/lists/api.ts index a3a808ab883d5..d615239f4eb01 100644 --- a/x-pack/plugins/lists/public/lists/api.ts +++ b/x-pack/plugins/lists/public/lists/api.ts @@ -4,18 +4,24 @@ * you may not use this file except in compliance with the Elastic License. */ -import { chain, fromEither, tryCatch } from 'fp-ts/lib/TaskEither'; +import { chain, fromEither, map, tryCatch } from 'fp-ts/lib/TaskEither'; import { flow } from 'fp-ts/lib/function'; import { pipe } from 'fp-ts/lib/pipeable'; import { DeleteListSchemaEncoded, + ExportListItemQuerySchemaEncoded, FindListSchemaEncoded, FoundListSchema, + ImportListItemQuerySchemaEncoded, + ImportListItemSchemaEncoded, ListSchema, deleteListSchema, + exportListItemQuerySchema, findListSchema, foundListSchema, + importListItemQuerySchema, + importListItemSchema, listSchema, } from '../../common/schemas'; import { LIST_ITEM_URL, LIST_URL } from '../../common/constants'; @@ -67,25 +73,53 @@ const findListsWithValidation = async ({ export { findListsWithValidation as findLists }; -export const importList = async ({ +const importList = async ({ file, http, - listId, + list_id, type, signal, -}: ImportListParams): Promise => { +}: ApiParams & ImportListItemSchemaEncoded & ImportListItemQuerySchemaEncoded): Promise< + ListSchema +> => { const formData = new FormData(); - formData.append('file', file); + formData.append('file', file as Blob); return http.fetch(`${LIST_ITEM_URL}/_import`, { body: formData, headers: { 'Content-Type': undefined }, method: 'POST', - query: { list_id: listId, type }, + query: { list_id, type }, signal, }); }; +const importListWithValidation = async ({ + file, + http, + listId, + type, + signal, +}: ImportListParams): Promise => + pipe( + { + list_id: listId, + type, + }, + (query) => fromEither(validateEither(importListItemQuerySchema, query)), + chain((query) => + pipe( + fromEither(validateEither(importListItemSchema, { file })), + map((body) => ({ ...body, ...query })) + ) + ), + chain((payload) => tryCatch(() => importList({ http, signal, ...payload }), String)), + chain((response) => fromEither(validateEither(listSchema, response))), + flow(toPromise) + ); + +export { importListWithValidation as importList }; + const deleteList = async ({ http, id, @@ -112,10 +146,28 @@ const deleteListWithValidation = async ({ export { deleteListWithValidation as deleteList }; -export const exportList = async ({ http, id, signal }: ExportListParams): Promise => { - return http.fetch(`${LIST_ITEM_URL}/_export`, { +const exportList = async ({ + http, + list_id, + signal, +}: ApiParams & ExportListItemQuerySchemaEncoded): Promise => + http.fetch(`${LIST_ITEM_URL}/_export`, { method: 'POST', - query: { list_id: id }, + query: { list_id }, signal, }); -}; + +const exportListWithValidation = async ({ + http, + listId, + signal, +}: ExportListParams): Promise => + pipe( + { list_id: listId }, + (payload) => fromEither(validateEither(exportListItemQuerySchema, payload)), + chain((payload) => tryCatch(() => exportList({ http, signal, ...payload }), String)), + chain((response) => fromEither(validateEither(listSchema, response))), + flow(toPromise) + ); + +export { exportListWithValidation as exportList }; diff --git a/x-pack/plugins/lists/public/lists/types.ts b/x-pack/plugins/lists/public/lists/types.ts index a3f12cd97cb5e..6421ad174d4d9 100644 --- a/x-pack/plugins/lists/public/lists/types.ts +++ b/x-pack/plugins/lists/public/lists/types.ts @@ -29,5 +29,5 @@ export interface DeleteListParams extends ApiParams { } export interface ExportListParams extends ApiParams { - id: string; + listId: string; } From e7f5b86e9389372cc7a23da2058a347439ec14e4 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 25 Jun 2020 16:56:23 -0500 Subject: [PATCH 11/14] Fix type errors * Continue to export decoded types without a qualifier * pull types used by hooks from their new location * Fix errors with usage of act() --- .../lists/common/schemas/request/delete_list_schema.ts | 2 +- .../schemas/request/export_list_item_query_schema.ts | 2 +- .../lists/common/schemas/request/find_list_schema.ts | 6 ++---- .../request/import_list_item_query_schema.mock.ts | 4 ++-- .../schemas/request/import_list_item_query_schema.ts | 2 +- .../common/schemas/request/import_list_item_schema.ts | 2 +- .../lists/public/lists/hooks/use_delete_list.test.ts | 7 +++++-- .../plugins/lists/public/lists/hooks/use_delete_list.ts | 3 ++- .../lists/public/lists/hooks/use_export_list.test.ts | 9 ++++++--- .../plugins/lists/public/lists/hooks/use_export_list.ts | 3 ++- .../lists/public/lists/hooks/use_find_lists.test.ts | 7 +++++-- .../plugins/lists/public/lists/hooks/use_find_lists.ts | 3 ++- .../plugins/lists/public/lists/hooks/use_import_list.ts | 3 ++- 13 files changed, 32 insertions(+), 21 deletions(-) diff --git a/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts b/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts index 8fd0afc415a8c..6f6fc7a9ea33c 100644 --- a/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/delete_list_schema.ts @@ -16,5 +16,5 @@ export const deleteListSchema = t.exact( }) ); -export type DeleteListSchemaDecoded = t.TypeOf; +export type DeleteListSchema = t.TypeOf; export type DeleteListSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts b/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts index 70496c504b77c..58092ffc563b1 100644 --- a/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/export_list_item_query_schema.ts @@ -17,5 +17,5 @@ export const exportListItemQuerySchema = t.exact( }) ); -export type ExportListItemQuerySchemaDecoded = t.TypeOf; +export type ExportListItemQuerySchema = t.TypeOf; export type ExportListItemQuerySchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts b/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts index c1bd22f322dd9..212232f6bc9c1 100644 --- a/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/find_list_schema.ts @@ -22,7 +22,5 @@ export const findListSchema = t.exact( }) ); -type FindListSchema = typeof findListSchema; - -export type FindListSchemaDecoded = t.TypeOf; -export type FindListSchemaEncoded = t.OutputOf; +export type FindListSchema = t.TypeOf; +export type FindListSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts index 5731916a483ff..6713083e6a49b 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.mock.ts @@ -6,9 +6,9 @@ import { LIST_ID, TYPE } from '../../constants.mock'; -import { ImportListItemQuerySchemaDecoded } from './import_list_item_query_schema'; +import { ImportListItemQuerySchema } from './import_list_item_query_schema'; -export const getImportListItemQuerySchemaMock = (): ImportListItemQuerySchemaDecoded => ({ +export const getImportListItemQuerySchemaMock = (): ImportListItemQuerySchema => ({ list_id: LIST_ID, type: TYPE, }); diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts index d57d7f8c02b5a..b37de61d0c2c3 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_query_schema.ts @@ -15,5 +15,5 @@ export const importListItemQuerySchema = t.exact(t.partial({ list_id, type })); export type ImportListItemQuerySchemaPartial = Identity>; -export type ImportListItemQuerySchemaDecoded = t.TypeOf; +export type ImportListItemQuerySchema = t.TypeOf; export type ImportListItemQuerySchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts b/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts index b24c60aeaff7c..7370eecf690c7 100644 --- a/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts +++ b/x-pack/plugins/lists/common/schemas/request/import_list_item_schema.ts @@ -16,5 +16,5 @@ export const importListItemSchema = t.exact( }) ); -export type ImportListItemSchemaDecoded = t.TypeOf; +export type ImportListItemSchema = t.TypeOf; export type ImportListItemSchemaEncoded = t.OutputOf; diff --git a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts index 9849becfa35f3..6262c553dfd52 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.test.ts @@ -23,8 +23,11 @@ describe('useDeleteList', () => { }); it('invokes Api.deleteList', async () => { - const { result } = renderHook(() => useDeleteList()); - await act(() => result.current.start({ http: httpMock, id: 'list' })); + const { result, waitForNextUpdate } = renderHook(() => useDeleteList()); + act(() => { + result.current.start({ http: httpMock, id: 'list' }); + }); + await waitForNextUpdate(); expect(Api.deleteList).toHaveBeenCalledWith( expect.objectContaining({ http: httpMock, id: 'list' }) diff --git a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts index 5c41f940ca6b7..0f1f6facdd7c4 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_delete_list.ts @@ -5,7 +5,8 @@ */ import { useAsyncTask } from '../../common/hooks/use_async_task'; -import { DeleteListParams, deleteList } from '../api'; +import { DeleteListParams } from '../types'; +import { deleteList } from '../api'; export type DeleteListTaskArgs = Omit; diff --git a/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts index e75ec819a75da..2eca0fd11b21a 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_export_list.test.ts @@ -22,11 +22,14 @@ describe('useExportList', () => { }); it('invokes Api.exportList', async () => { - const { result } = renderHook(() => useExportList()); - await act(() => result.current.start({ http: httpMock, id: 'list' })); + const { result, waitForNextUpdate } = renderHook(() => useExportList()); + act(() => { + result.current.start({ http: httpMock, listId: 'list' }); + }); + await waitForNextUpdate(); expect(Api.exportList).toHaveBeenCalledWith( - expect.objectContaining({ http: httpMock, id: 'list' }) + expect.objectContaining({ http: httpMock, listId: 'list' }) ); }); }); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts index 41ccd21e07981..41efde939ead4 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_export_list.ts @@ -5,7 +5,8 @@ */ import { useAsyncTask } from '../../common/hooks/use_async_task'; -import { ExportListParams, exportList } from '../api'; +import { ExportListParams } from '../types'; +import { exportList } from '../api'; export type ExportListTaskArgs = Omit; diff --git a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts index 585264862b676..0d63acbe0bd2c 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.test.ts @@ -23,8 +23,11 @@ describe('useFindLists', () => { }); it('invokes Api.findLists', async () => { - const { result } = renderHook(() => useFindLists()); - await act(() => result.current.start({ http: httpMock, pageIndex: 1, pageSize: 10 })); + const { result, waitForNextUpdate } = renderHook(() => useFindLists()); + act(() => { + result.current.start({ http: httpMock, pageIndex: 1, pageSize: 10 }); + }); + await waitForNextUpdate(); expect(Api.findLists).toHaveBeenCalledWith( expect.objectContaining({ http: httpMock, pageIndex: 1, pageSize: 10 }) diff --git a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts index f58358ef6e4c4..d50a16855a547 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_find_lists.ts @@ -5,7 +5,8 @@ */ import { useAsyncTask } from '../../common/hooks/use_async_task'; -import { FindListsParams, findLists } from '../api'; +import { FindListsParams } from '../types'; +import { findLists } from '../api'; export type FindListsTaskArgs = Omit; diff --git a/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts b/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts index de77fff0db250..2854acd6e522e 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_import_list.ts @@ -5,7 +5,8 @@ */ import { useAsyncTask } from '../../common/hooks/use_async_task'; -import { ImportListParams, importList } from '../api'; +import { ImportListParams } from '../types'; +import { importList } from '../api'; export type ImportListTaskArgs = Omit; From f433c51aa1bcd8d149c043be7762fb592a20cf30 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Fri, 26 Jun 2020 12:12:08 -0500 Subject: [PATCH 12/14] Attempting to reduce plugin bundle size By pulling from the module directly instead of an index, we can hopefully narrow down our dependencies until tree-shaking does this for us. --- x-pack/plugins/lists/public/common/hooks/use_async_task.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts index 26b7d6fcaa5a6..96a582a954c2f 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts @@ -5,7 +5,7 @@ */ import { useCallback, useRef } from 'react'; -import { useAsyncFn } from 'react-use'; +import useAsyncFn from 'react-use/lib/useAsyncFn'; // Params can be generalized to a ...rest parameter extending unknown[] once https://github.com/microsoft/TypeScript/pull/39094 is available. // for now, the task must still receive unknown as a second argument, and an argument must be passed to start() From 02101c7c5225f62eeb8100d5cd9fad20819cca27 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Fri, 26 Jun 2020 18:09:08 -0500 Subject: [PATCH 13/14] useAsyncFn's initiator does not return a promise Rather than returning a promise and requiring the caller to handle a rejection, we instead return nothing and require the user to watch the hook's state. * success can be handled with a useEffect on state.result * errors can be handled with a useEffect on state.error --- .../public/common/hooks/use_async_task.test.ts | 5 +++-- .../lists/public/common/hooks/use_async_task.ts | 8 ++------ .../public/lists/hooks/use_import_list.test.ts | 17 +++++++---------- 3 files changed, 12 insertions(+), 18 deletions(-) diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts index 8acb60b4c11f9..286efe9809e72 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts @@ -57,15 +57,16 @@ describe('useAsyncTask', () => { await waitForNextUpdate(); expect(result.current.result).toEqual('resolved value'); + expect(result.current.error).toBeUndefined(); }); - it('start rejects and error is populated if task rejects', async () => { + it('populates error if task rejects', async () => { expect.assertions(3); task.mockRejectedValue(new Error('whoops')); const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task)); act(() => { - result.current.start({}).catch((e) => expect(e).toEqual(new Error('whoops'))); + result.current.start({}); }); await waitForNextUpdate(); diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts index 96a582a954c2f..f767e9333c234 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.ts @@ -14,7 +14,7 @@ export type UseAsyncTask = ( ) => AsyncTask; export interface AsyncTask { - start: (params: Params) => Promise; + start: (params: Params) => void; abort: () => void; loading: boolean; error: Error | undefined; @@ -39,11 +39,7 @@ export const useAsyncTask: UseAsyncTask = (task) => { const start = useCallback( (args) => { ctrl.current = new AbortController(); - - return initiator(ctrl.current, args).then((result) => - // convert resolved error to rejection: https://github.com/streamich/react-use/issues/981 - result instanceof Error ? Promise.reject(result) : result - ); + initiator(ctrl.current, args); }, [initiator] ); diff --git a/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts index cdb9e32a899f0..00a8b7f3206b0 100644 --- a/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts +++ b/x-pack/plugins/lists/public/lists/hooks/use_import_list.test.ts @@ -69,21 +69,18 @@ describe('useImportList', () => { expect(result.current.result).toEqual(getListResponseMock()); }); - it('start rejects and error is populated if importList rejects', async () => { - expect.assertions(3); + it('error is populated if importList rejects', async () => { const fileMock = ('my file' as unknown) as File; (Api.importList as jest.Mock).mockRejectedValue(new Error('whoops')); const { result, waitForNextUpdate } = renderHook(() => useImportList()); act(() => { - result.current - .start({ - file: fileMock, - http: httpMock, - listId: 'my_list_id', - type: 'keyword', - }) - .catch((e) => expect(e).toEqual(new Error('whoops'))); + result.current.start({ + file: fileMock, + http: httpMock, + listId: 'my_list_id', + type: 'keyword', + }); }); await waitForNextUpdate(); From 4f436373059baed3633dcbd0a76c39d259a2040f Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Sat, 27 Jun 2020 20:23:58 -0500 Subject: [PATCH 14/14] Fix failing test Assertion count wasn't updated following interface changes; we've now got two inline expectations so this isn't needed. --- x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts index 286efe9809e72..af3aa60cfa506 100644 --- a/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts +++ b/x-pack/plugins/lists/public/common/hooks/use_async_task.test.ts @@ -61,7 +61,6 @@ describe('useAsyncTask', () => { }); it('populates error if task rejects', async () => { - expect.assertions(3); task.mockRejectedValue(new Error('whoops')); const { result, waitForNextUpdate } = renderHook(() => useAsyncTask(task));