From 385ed6d103213ba9e246a7dda998655696103d54 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Thu, 2 Jul 2020 11:02:52 +0200 Subject: [PATCH] [Ingest Pipelines] Error messages (#70167) * improved error messages * traverse recursive error struct * add check for object with keys * update button position and copy * size adjustments * Refactor i18n texts and change wording Also added missing translation and refactored maximum errors in collapsed state to external constant * use io-ts, add CIT and unit tests * refactor error utilities to separate file Co-authored-by: Elastic Machine --- .../__jest__/client_integration/fixtures.ts | 117 ++++++++++++++++++ .../helpers/pipeline_form.helpers.ts | 2 + .../ingest_pipelines_create.test.tsx | 21 ++++ .../pipeline_form/pipeline_form.tsx | 13 +- .../pipeline_form/pipeline_form_error.tsx | 34 ----- .../pipeline_form_error/error_utils.test.ts | 67 ++++++++++ .../pipeline_form_error/error_utils.ts | 85 +++++++++++++ .../pipeline_form_error/i18n_texts.ts | 38 ++++++ .../pipeline_form_error/index.ts | 7 ++ .../pipeline_form_error.tsx | 99 +++++++++++++++ .../server/routes/api/create.ts | 8 +- .../server/routes/api/shared/index.ts | 7 ++ .../routes/api/shared/is_object_with_keys.ts | 9 ++ .../server/routes/api/update.ts | 8 +- 14 files changed, 473 insertions(+), 42 deletions(-) create mode 100644 x-pack/plugins/ingest_pipelines/__jest__/client_integration/fixtures.ts delete mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error.tsx create mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.test.ts create mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.ts create mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/i18n_texts.ts create mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/index.ts create mode 100644 x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/pipeline_form_error.tsx create mode 100644 x-pack/plugins/ingest_pipelines/server/routes/api/shared/index.ts create mode 100644 x-pack/plugins/ingest_pipelines/server/routes/api/shared/is_object_with_keys.ts diff --git a/x-pack/plugins/ingest_pipelines/__jest__/client_integration/fixtures.ts b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/fixtures.ts new file mode 100644 index 0000000000000..8dddb2421f03d --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/fixtures.ts @@ -0,0 +1,117 @@ +/* + * 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. + */ + +export const nestedProcessorsErrorFixture = { + attributes: { + error: { + root_cause: [ + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + suppressed: [ + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + suppressed: [ + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'csv', + }, + ], + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + ], + }, + ], + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + suppressed: [ + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + suppressed: [ + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'csv', + }, + ], + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + { + type: 'parse_exception', + reason: '[field] required property is missing', + property_name: 'field', + processor_type: 'circle', + }, + ], + }, + status: 400, + }, +}; diff --git a/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts index 8a14ed13f2022..85848b3d2f73c 100644 --- a/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts +++ b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/helpers/pipeline_form.helpers.ts @@ -42,6 +42,8 @@ export type PipelineFormTestSubjects = | 'submitButton' | 'pageTitle' | 'savePipelineError' + | 'savePipelineError.showErrorsButton' + | 'savePipelineError.hideErrorsButton' | 'pipelineForm' | 'versionToggle' | 'versionField' diff --git a/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx index 2cfccbdc6d578..813057813f139 100644 --- a/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx +++ b/x-pack/plugins/ingest_pipelines/__jest__/client_integration/ingest_pipelines_create.test.tsx @@ -9,6 +9,8 @@ import { act } from 'react-dom/test-utils'; import { setupEnvironment, pageHelpers, nextTick } from './helpers'; import { PipelinesCreateTestBed } from './helpers/pipelines_create.helpers'; +import { nestedProcessorsErrorFixture } from './fixtures'; + const { setup } = pageHelpers.pipelinesCreate; jest.mock('@elastic/eui', () => { @@ -163,6 +165,25 @@ describe('', () => { expect(exists('savePipelineError')).toBe(true); expect(find('savePipelineError').text()).toContain(error.message); }); + + test('displays nested pipeline errors as a flat list', async () => { + const { actions, find, exists, waitFor } = testBed; + httpRequestsMockHelpers.setCreatePipelineResponse(undefined, { + body: nestedProcessorsErrorFixture, + }); + + await act(async () => { + actions.clickSubmitButton(); + await waitFor('savePipelineError'); + }); + + expect(exists('savePipelineError')).toBe(true); + expect(exists('savePipelineError.showErrorsButton')).toBe(true); + find('savePipelineError.showErrorsButton').simulate('click'); + expect(exists('savePipelineError.hideErrorsButton')).toBe(true); + expect(exists('savePipelineError.showErrorsButton')).toBe(false); + expect(find('savePipelineError').find('li').length).toBe(8); + }); }); describe('test pipeline', () => { diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx index 05c9f0a08b0c7..a68e667f4ab43 100644 --- a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx @@ -11,17 +11,18 @@ import { EuiButton, EuiButtonEmpty, EuiFlexGroup, EuiFlexItem, EuiSpacer } from import { useForm, Form, FormConfig } from '../../../shared_imports'; import { Pipeline } from '../../../../common/types'; -import { PipelineRequestFlyout } from './pipeline_request_flyout'; -import { PipelineTestFlyout } from './pipeline_test_flyout'; -import { PipelineFormFields } from './pipeline_form_fields'; -import { PipelineFormError } from './pipeline_form_error'; -import { pipelineFormSchema } from './schema'; import { OnUpdateHandlerArg, OnUpdateHandler, SerializeResult, } from '../pipeline_processors_editor'; +import { PipelineRequestFlyout } from './pipeline_request_flyout'; +import { PipelineTestFlyout } from './pipeline_test_flyout'; +import { PipelineFormFields } from './pipeline_form_fields'; +import { PipelineFormError } from './pipeline_form_error'; +import { pipelineFormSchema } from './schema'; + export interface PipelineFormProps { onSave: (pipeline: Pipeline) => void; onCancel: () => void; @@ -116,7 +117,7 @@ export const PipelineForm: React.FunctionComponent = ({ error={form.getErrors()} > {/* Request error */} - {saveError && } + {saveError && } {/* All form fields */} = ({ errorMessage }) => { - return ( - <> - - } - color="danger" - iconType="alert" - data-test-subj="savePipelineError" - > -

{errorMessage}

- - - - ); -}; diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.test.ts b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.test.ts new file mode 100644 index 0000000000000..1739365eb197d --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.test.ts @@ -0,0 +1,67 @@ +/* + * 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 { toKnownError } from './error_utils'; +import { nestedProcessorsErrorFixture } from '../../../../../__jest__/client_integration/fixtures'; + +describe('toKnownError', () => { + test('undefined, null, numbers, arrays and bad objects', () => { + expect(toKnownError(undefined)).toEqual({ errors: [{ reason: 'An unknown error occurred.' }] }); + expect(toKnownError(null)).toEqual({ errors: [{ reason: 'An unknown error occurred.' }] }); + expect(toKnownError(123)).toEqual({ errors: [{ reason: 'An unknown error occurred.' }] }); + expect(toKnownError([])).toEqual({ errors: [{ reason: 'An unknown error occurred.' }] }); + expect(toKnownError({})).toEqual({ errors: [{ reason: 'An unknown error occurred.' }] }); + expect(toKnownError({ attributes: {} })).toEqual({ + errors: [{ reason: 'An unknown error occurred.' }], + }); + }); + + test('non-processors errors', () => { + expect(toKnownError(new Error('my error'))).toEqual({ errors: [{ reason: 'my error' }] }); + expect(toKnownError({ message: 'my error' })).toEqual({ errors: [{ reason: 'my error' }] }); + }); + + test('processors errors', () => { + expect(toKnownError(nestedProcessorsErrorFixture)).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "csv", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + Object { + "processorType": "circle", + "reason": "[field] required property is missing", + }, + ], + } + `); + }); +}); diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.ts b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.ts new file mode 100644 index 0000000000000..7f32f962f657c --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/error_utils.ts @@ -0,0 +1,85 @@ +/* + * 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 * as t from 'io-ts'; +import { flow } from 'fp-ts/lib/function'; +import { isRight } from 'fp-ts/lib/Either'; + +import { i18nTexts } from './i18n_texts'; + +export interface PipelineError { + reason: string; + processorType?: string; +} +interface PipelineErrors { + errors: PipelineError[]; +} + +interface ErrorNode { + reason: string; + processor_type?: string; + suppressed?: ErrorNode[]; +} + +// This is a runtime type (RT) for an error node which is a recursive type +const errorNodeRT = t.recursion('ErrorNode', (ErrorNode) => + t.intersection([ + t.interface({ + reason: t.string, + }), + t.partial({ + processor_type: t.string, + suppressed: t.array(ErrorNode), + }), + ]) +); + +// This is a runtime type for the attributes object we expect to receive from the server +// for processor errors +const errorAttributesObjectRT = t.interface({ + attributes: t.interface({ + error: t.interface({ + root_cause: t.array(errorNodeRT), + }), + }), +}); + +const isProcessorsError = flow(errorAttributesObjectRT.decode, isRight); + +type ErrorAttributesObject = t.TypeOf; + +const flattenErrorsTree = (node: ErrorNode): PipelineError[] => { + const result: PipelineError[] = []; + const recurse = (_node: ErrorNode) => { + result.push({ reason: _node.reason, processorType: _node.processor_type }); + if (_node.suppressed && Array.isArray(_node.suppressed)) { + _node.suppressed.forEach(recurse); + } + }; + recurse(node); + return result; +}; + +export const toKnownError = (error: unknown): PipelineErrors => { + if (typeof error === 'object' && error != null && isProcessorsError(error)) { + const errorAttributes = error as ErrorAttributesObject; + const rootCause = errorAttributes.attributes.error.root_cause[0]; + return { errors: flattenErrorsTree(rootCause) }; + } + + if (typeof error === 'string') { + return { errors: [{ reason: error }] }; + } + + if ( + error instanceof Error || + (typeof error === 'object' && error != null && (error as any).message) + ) { + return { errors: [{ reason: (error as any).message }] }; + } + + return { errors: [{ reason: i18nTexts.errors.unknownError }] }; +}; diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/i18n_texts.ts b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/i18n_texts.ts new file mode 100644 index 0000000000000..e354541db8e7b --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/i18n_texts.ts @@ -0,0 +1,38 @@ +/* + * 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 { i18n } from '@kbn/i18n'; + +export const i18nTexts = { + title: i18n.translate('xpack.ingestPipelines.form.savePipelineError', { + defaultMessage: 'Unable to create pipeline', + }), + errors: { + processor: (processorType: string) => + i18n.translate('xpack.ingestPipelines.form.savePipelineError.processorLabel', { + defaultMessage: '{type} processor', + values: { type: processorType }, + }), + showErrors: (hiddenErrorsCount: number) => + i18n.translate('xpack.ingestPipelines.form.savePipelineError.showAllButton', { + defaultMessage: + 'Show {hiddenErrorsCount, plural, one {# more error} other {# more errors}}', + values: { + hiddenErrorsCount, + }, + }), + hideErrors: (hiddenErrorsCount: number) => + i18n.translate('xpack.ingestPipelines.form.savePip10mbelineError.showFewerButton', { + defaultMessage: 'Hide {hiddenErrorsCount, plural, one {# error} other {# errors}}', + values: { + hiddenErrorsCount, + }, + }), + unknownError: i18n.translate('xpack.ingestPipelines.form.unknownError', { + defaultMessage: 'An unknown error occurred.', + }), + }, +}; diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/index.ts b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/index.ts new file mode 100644 index 0000000000000..656691f639498 --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/index.ts @@ -0,0 +1,7 @@ +/* + * 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. + */ + +export { PipelineFormError } from './pipeline_form_error'; diff --git a/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/pipeline_form_error.tsx b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/pipeline_form_error.tsx new file mode 100644 index 0000000000000..23fb9a1648434 --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_error/pipeline_form_error.tsx @@ -0,0 +1,99 @@ +/* + * 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 React, { useState, useEffect } from 'react'; + +import { EuiSpacer, EuiCallOut, EuiFlexGroup, EuiFlexItem, EuiButtonEmpty } from '@elastic/eui'; +import { useKibana } from '../../../../shared_imports'; + +import { i18nTexts } from './i18n_texts'; +import { toKnownError, PipelineError } from './error_utils'; + +interface Props { + error: unknown; +} + +const numberOfErrorsToDisplay = 5; + +export const PipelineFormError: React.FunctionComponent = ({ error }) => { + const { services } = useKibana(); + const [isShowingAllErrors, setIsShowingAllErrors] = useState(false); + const safeErrorResult = toKnownError(error); + const hasMoreErrors = safeErrorResult.errors.length > numberOfErrorsToDisplay; + const hiddenErrorsCount = safeErrorResult.errors.length - numberOfErrorsToDisplay; + const results = isShowingAllErrors + ? safeErrorResult.errors + : safeErrorResult.errors.slice(0, numberOfErrorsToDisplay); + + const renderErrorListItem = ({ processorType, reason }: PipelineError) => { + return ( + <> + {processorType ? <>{i18nTexts.errors.processor(processorType) + ':'}  : undefined} + {reason} + + ); + }; + + useEffect(() => { + services.notifications.toasts.addDanger({ title: i18nTexts.title }); + }, [services, error]); + return ( + <> + + {results.length > 1 ? ( +
    + {results.map((e, idx) => ( +
  • {renderErrorListItem(e)}
    • + ))} +
+ ) : ( + renderErrorListItem(results[0]) + )} + {hasMoreErrors ? ( + + + {isShowingAllErrors ? ( + setIsShowingAllErrors(false)} + color="danger" + iconSide="right" + iconType="arrowUp" + data-test-subj="hideErrorsButton" + > + {i18nTexts.errors.hideErrors(hiddenErrorsCount)} + + ) : ( + setIsShowingAllErrors(true)} + color="danger" + iconSide="right" + iconType="arrowDown" + data-test-subj="showErrorsButton" + > + {i18nTexts.errors.showErrors(hiddenErrorsCount)} + + )} + + + ) : undefined} + + + + ); +}; diff --git a/x-pack/plugins/ingest_pipelines/server/routes/api/create.ts b/x-pack/plugins/ingest_pipelines/server/routes/api/create.ts index c1ab3852ee784..c2328bcc9d0ab 100644 --- a/x-pack/plugins/ingest_pipelines/server/routes/api/create.ts +++ b/x-pack/plugins/ingest_pipelines/server/routes/api/create.ts @@ -10,6 +10,7 @@ import { Pipeline } from '../../../common/types'; import { API_BASE_PATH } from '../../../common/constants'; import { RouteDependencies } from '../../types'; import { pipelineSchema } from './pipeline_schema'; +import { isObjectWithKeys } from './shared'; const bodySchema = schema.object({ name: schema.string(), @@ -70,7 +71,12 @@ export const registerCreateRoute = ({ if (isEsError(error)) { return res.customError({ statusCode: error.statusCode, - body: error, + body: isObjectWithKeys(error.body) + ? { + message: error.message, + attributes: error.body, + } + : error, }); } diff --git a/x-pack/plugins/ingest_pipelines/server/routes/api/shared/index.ts b/x-pack/plugins/ingest_pipelines/server/routes/api/shared/index.ts new file mode 100644 index 0000000000000..1fa794a4fb996 --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/server/routes/api/shared/index.ts @@ -0,0 +1,7 @@ +/* + * 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. + */ + +export { isObjectWithKeys } from './is_object_with_keys'; diff --git a/x-pack/plugins/ingest_pipelines/server/routes/api/shared/is_object_with_keys.ts b/x-pack/plugins/ingest_pipelines/server/routes/api/shared/is_object_with_keys.ts new file mode 100644 index 0000000000000..0617bde26cfb6 --- /dev/null +++ b/x-pack/plugins/ingest_pipelines/server/routes/api/shared/is_object_with_keys.ts @@ -0,0 +1,9 @@ +/* + * 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. + */ + +export const isObjectWithKeys = (value: unknown) => { + return typeof value === 'object' && !!value && Object.keys(value).length > 0; +}; diff --git a/x-pack/plugins/ingest_pipelines/server/routes/api/update.ts b/x-pack/plugins/ingest_pipelines/server/routes/api/update.ts index 214b293a43c6c..cd0e3568f0f60 100644 --- a/x-pack/plugins/ingest_pipelines/server/routes/api/update.ts +++ b/x-pack/plugins/ingest_pipelines/server/routes/api/update.ts @@ -8,6 +8,7 @@ import { schema } from '@kbn/config-schema'; import { API_BASE_PATH } from '../../../common/constants'; import { RouteDependencies } from '../../types'; import { pipelineSchema } from './pipeline_schema'; +import { isObjectWithKeys } from './shared'; const bodySchema = schema.object(pipelineSchema); @@ -52,7 +53,12 @@ export const registerUpdateRoute = ({ if (isEsError(error)) { return res.customError({ statusCode: error.statusCode, - body: error, + body: isObjectWithKeys(error.body) + ? { + message: error.message, + attributes: error.body, + } + : error, }); }