From 223121bc5d668f560f609a25a9e08778fa504c50 Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Wed, 30 Nov 2022 06:59:47 -0500 Subject: [PATCH] [8.5] [Infrastructure UI] Improve metrics settings error handling (#146272) (#146674) # Backport This will backport the following commits from `main` to `8.5`: - [[Infrastructure UI] Improve metrics settings error handling (#146272)](https://github.com/elastic/kibana/pull/146272) ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) Co-authored-by: Marco Antonio Ghiani --- packages/kbn-io-ts-utils/index.ts | 2 + .../src/index_pattern_rt/index.test.ts | 35 +++++++++++ .../src/index_pattern_rt/index.ts | 36 +++++++++++ .../infra/common/metrics_sources/index.ts | 6 ++ .../indices_configuration_form_state.ts | 14 ++++- .../pages/metrics/settings/input_fields.tsx | 49 ++++++++++++--- .../source_configuration_settings.tsx | 10 +--- .../server/routes/metrics_sources/index.ts | 59 +++++++++---------- 8 files changed, 161 insertions(+), 50 deletions(-) create mode 100644 packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts create mode 100644 packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts diff --git a/packages/kbn-io-ts-utils/index.ts b/packages/kbn-io-ts-utils/index.ts index f15e8c429c8b3..c964dcaa5b749 100644 --- a/packages/kbn-io-ts-utils/index.ts +++ b/packages/kbn-io-ts-utils/index.ts @@ -6,9 +6,11 @@ * Side Public License, v 1. */ +export type { IndexPatternType } from './src/index_pattern_rt'; export type { NonEmptyStringBrand } from './src/non_empty_string_rt'; export { deepExactRt } from './src/deep_exact_rt'; +export { indexPatternRt } from './src/index_pattern_rt'; export { jsonRt } from './src/json_rt'; export { mergeRt } from './src/merge_rt'; export { strictKeysRt } from './src/strict_keys_rt'; diff --git a/packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts new file mode 100644 index 0000000000000..0caf40812fb04 --- /dev/null +++ b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.test.ts @@ -0,0 +1,35 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { indexPatternRt } from '.'; +import { isRight } from 'fp-ts/lib/Either'; + +describe('indexPatternRt', () => { + test('passes on valid index pattern strings', () => { + expect(isRight(indexPatternRt.decode('logs-*'))).toBe(true); + expect(isRight(indexPatternRt.decode('logs-*,filebeat-*'))).toBe(true); + }); + + test('fails if the pattern is an empty string', () => { + expect(isRight(indexPatternRt.decode(''))).toBe(false); + }); + + test('fails if the pattern contains empty spaces', () => { + expect(isRight(indexPatternRt.decode(' '))).toBe(false); + expect(isRight(indexPatternRt.decode(' logs-*'))).toBe(false); + expect(isRight(indexPatternRt.decode('logs-* '))).toBe(false); + expect(isRight(indexPatternRt.decode('logs-*, filebeat-*'))).toBe(false); + }); + + test('fails if the pattern contains empty comma-separated entries', () => { + expect(isRight(indexPatternRt.decode(',logs-*'))).toBe(false); + expect(isRight(indexPatternRt.decode('logs-*,'))).toBe(false); + expect(isRight(indexPatternRt.decode('logs-*,,filebeat-*'))).toBe(false); + expect(isRight(indexPatternRt.decode('logs-*,,,filebeat-*'))).toBe(false); + }); +}); diff --git a/packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts new file mode 100644 index 0000000000000..02696d28ec73a --- /dev/null +++ b/packages/kbn-io-ts-utils/src/index_pattern_rt/index.ts @@ -0,0 +1,36 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ +import * as rt from 'io-ts'; + +export const isEmptyString = (value: string) => value === ''; + +export const containsSpaces = (value: string) => value.includes(' '); + +export const containsEmptyEntries = (value: string) => value.split(',').some(isEmptyString); + +export const validateIndexPattern = (indexPattern: string) => { + return ( + !isEmptyString(indexPattern) && + !containsSpaces(indexPattern) && + !containsEmptyEntries(indexPattern) + ); +}; + +export interface IndexPatternBrand { + readonly IndexPattern: unique symbol; +} + +type IndexPattern = rt.Branded; + +export const indexPatternRt = rt.brand( + rt.string, + (pattern): pattern is IndexPattern => validateIndexPattern(pattern), + 'IndexPattern' +); + +export type IndexPatternType = rt.TypeOf; diff --git a/x-pack/plugins/infra/common/metrics_sources/index.ts b/x-pack/plugins/infra/common/metrics_sources/index.ts index 7fae908707a89..f636fa8e16b50 100644 --- a/x-pack/plugins/infra/common/metrics_sources/index.ts +++ b/x-pack/plugins/infra/common/metrics_sources/index.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { indexPatternRt } from '@kbn/io-ts-utils'; import * as rt from 'io-ts'; import { SourceConfigurationRT, @@ -28,6 +29,11 @@ export type MetricsSourceConfigurationProperties = rt.TypeOf< typeof metricsSourceConfigurationPropertiesRT >; +export const partialMetricsSourceConfigurationReqPayloadRT = rt.partial({ + ...metricsSourceConfigurationPropertiesRT.type.props, + metricAlias: indexPatternRt, +}); + export const partialMetricsSourceConfigurationPropertiesRT = rt.partial({ ...metricsSourceConfigurationPropertiesRT.type.props, }); diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts b/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts index 359ada83b2ffa..55922dd05bfee 100644 --- a/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts +++ b/x-pack/plugins/infra/public/pages/metrics/settings/indices_configuration_form_state.ts @@ -6,9 +6,13 @@ */ import { ReactNode, useCallback, useMemo, useState } from 'react'; + import { + aggregateValidationErrors, createInputFieldProps, createInputRangeFieldProps, + validateInputFieldHasNotEmptyEntries, + validateInputFieldHasNotEmptySpaces, validateInputFieldNotEmpty, } from './input_fields'; @@ -41,7 +45,7 @@ export const useIndicesConfigurationFormState = ({ const nameFieldProps = useMemo( () => createInputFieldProps({ - errors: validateInputFieldNotEmpty(formState.name), + errors: aggregateValidationErrors(validateInputFieldNotEmpty)(formState.name), name: 'name', onChange: (name) => setFormStateChanges((changes) => ({ ...changes, name })), value: formState.name, @@ -51,7 +55,11 @@ export const useIndicesConfigurationFormState = ({ const metricAliasFieldProps = useMemo( () => createInputFieldProps({ - errors: validateInputFieldNotEmpty(formState.metricAlias), + errors: aggregateValidationErrors( + validateInputFieldNotEmpty, + validateInputFieldHasNotEmptyEntries, + validateInputFieldHasNotEmptySpaces + )(formState.metricAlias), name: 'metricAlias', onChange: (metricAlias) => setFormStateChanges((changes) => ({ ...changes, metricAlias })), value: formState.metricAlias, @@ -62,7 +70,7 @@ export const useIndicesConfigurationFormState = ({ const anomalyThresholdFieldProps = useMemo( () => createInputRangeFieldProps({ - errors: validateInputFieldNotEmpty(formState.anomalyThreshold), + errors: aggregateValidationErrors(validateInputFieldNotEmpty)(formState.anomalyThreshold), name: 'anomalyThreshold', onChange: (anomalyThreshold) => setFormStateChanges((changes) => ({ ...changes, anomalyThreshold })), diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx b/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx index 60c976c734707..e2380dedaee15 100644 --- a/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx +++ b/x-pack/plugins/infra/public/pages/metrics/settings/input_fields.tsx @@ -22,6 +22,10 @@ export interface InputFieldProps< export type FieldErrorMessage = string | JSX.Element; +export type ValidationHandlerList = Array< + (value: ValueType) => FieldErrorMessage | false +>; + export const createInputFieldProps = < Value extends string = string, FieldElement extends HTMLInputElement = HTMLInputElement @@ -83,12 +87,39 @@ export const createInputRangeFieldProps = < value, }); -export const validateInputFieldNotEmpty = (value: React.ReactText) => - value === '' - ? [ - , - ] - : []; +export const aggregateValidationErrors = + ( + ...validationHandlers: ValidationHandlerList + ) => + (value: ValueType) => + validationHandlers.map((validator) => validator(value)).filter(Boolean) as FieldErrorMessage[]; + +const isEmptyString = (value: ReactText) => value === ''; + +const containsSpaces = (value: string) => value.includes(' '); + +const containsEmptyEntries = (value: string) => value.split(',').some(isEmptyString); + +export const validateInputFieldNotEmpty = (value: ReactText) => + isEmptyString(value) && ( + + ); + +export const validateInputFieldHasNotEmptyEntries = (value: string) => + containsEmptyEntries(value) && ( + + ); + +export const validateInputFieldHasNotEmptySpaces = (value: string) => + containsSpaces(value) && ( + + ); diff --git a/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx b/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx index 1b21dfbc800f3..1e6d04721f99b 100644 --- a/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx +++ b/x-pack/plugins/infra/public/pages/metrics/settings/source_configuration_settings.tsx @@ -15,7 +15,7 @@ import { } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n-react'; -import React, { useCallback, useMemo } from 'react'; +import React, { useCallback } from 'react'; import { Prompt } from '@kbn/observability-plugin/public'; import { SourceLoadingPage } from '../../../components/source_loading_page'; import { useSourceContext } from '../../../containers/metrics_source'; @@ -75,19 +75,13 @@ export const SourceConfigurationSettings = ({ formStateChanges, ]); - const isWriteable = useMemo( - () => shouldAllowEdit && source && source.origin !== 'internal', - [shouldAllowEdit, source] - ); + const isWriteable = shouldAllowEdit && (!Boolean(source) || source?.origin !== 'internal'); const { hasInfraMLCapabilities } = useInfraMLCapabilitiesContext(); if ((isLoading || isUninitialized) && !source) { return ; } - if (!source?.configuration) { - return null; - } return ( { @@ -35,24 +35,33 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) => const { sourceId } = request.params; const soClient = (await requestContext.core).savedObjects.client; - const [source, metricIndicesExist, indexFields] = await Promise.all([ - libs.sources.getSourceConfiguration(soClient, sourceId), - libs.sourceStatus.hasMetricIndices(requestContext, sourceId), - libs.fields.getFields(requestContext, sourceId, 'METRICS'), - ]); + try { + const [source, metricIndicesExist, indexFields] = await Promise.all([ + libs.sources.getSourceConfiguration(soClient, sourceId), + libs.sourceStatus.hasMetricIndices(requestContext, sourceId), + libs.fields.getFields(requestContext, sourceId, 'METRICS'), + ]); - if (!source) { - return response.notFound(); - } + if (!source) { + return response.notFound(); + } - const status: MetricsSourceStatus = { - metricIndicesExist, - indexFields, - }; + const status: MetricsSourceStatus = { + metricIndicesExist, + indexFields, + }; - return response.ok({ - body: metricsSourceConfigurationResponseRT.encode({ source: { ...source, status } }), - }); + return response.ok({ + body: metricsSourceConfigurationResponseRT.encode({ source: { ...source, status } }), + }); + } catch (error) { + return response.customError({ + statusCode: error.statusCode ?? 500, + body: { + message: error.message ?? 'An unexpected error occurred', + }, + }); + } } ); @@ -64,13 +73,13 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) => params: schema.object({ sourceId: schema.string(), }), - body: createValidationFunction(partialMetricsSourceConfigurationPropertiesRT), + body: createValidationFunction(partialMetricsSourceConfigurationReqPayloadRT), }, }, framework.router.handleLegacyErrors(async (requestContext, request, response) => { const { sources } = libs; const { sourceId } = request.params; - const patchedSourceConfigurationProperties = request.body; + const sourceConfigurationPayload = request.body; try { const soClient = (await requestContext.core).savedObjects.client; @@ -84,18 +93,8 @@ export const initMetricsSourceConfigurationRoutes = (libs: InfraBackendLibs) => const sourceConfigurationExists = sourceConfiguration.origin === 'stored'; const patchedSourceConfiguration = await (sourceConfigurationExists - ? sources.updateSourceConfiguration( - soClient, - sourceId, - // @ts-ignore - patchedSourceConfigurationProperties - ) - : sources.createSourceConfiguration( - soClient, - sourceId, - // @ts-ignore - patchedSourceConfigurationProperties - )); + ? sources.updateSourceConfiguration(soClient, sourceId, sourceConfigurationPayload) + : sources.createSourceConfiguration(soClient, sourceId, sourceConfigurationPayload)); const [metricIndicesExist, indexFields] = await Promise.all([ libs.sourceStatus.hasMetricIndices(requestContext, sourceId),