From d8fa996c5052d10da2f438b93723437f5631bbde Mon Sep 17 00:00:00 2001 From: Elena Stoeva <59341489+ElenaStoeva@users.noreply.github.com> Date: Tue, 22 Oct 2024 15:53:37 +0100 Subject: [PATCH] [Index Management] Fix unhandled error in ds data retention modal (#196524) Fixes https://github.com/elastic/kibana/issues/196331 ## Summary This PR fixes the bug in the Edit ds data retention modal where we were comparing the max retention period with an undefined `value` (now the comparison happens only if `value` is defined). Also, the PR makes the data retention field get re-validated only when the time unit changes (otherwise, when we switch off the toggle to enable to data retention field, the field would get validated and would immediately show an error "A data retention value is required." which is not great UX). ### How to test: 1. Start serverless ES with `yarn es serverless --projectType=security -E data_streams.lifecycle.retention.max=200d` and kibana with `yarn serverless-security` 2. Navigate to Kibana, create a data stream using Dev Tools: ``` PUT _index_template/ds { "index_patterns": ["ds"], "data_stream": {} } POST ds/_doc { "@timestamp": "2020-01-27" } ``` 3. Navigate to Index Management. Find the data stream and select it --> Click "Manage" --> Click "Edit data retention" 4. Disable the toggle "Keep data up to maximum retention period" 5. Verify that the field is enabled correctly, there is not endless spinner, and no console error. https://github.com/user-attachments/assets/957e0869-ee23-46d9-8f20-134937f6f8cf --------- Co-authored-by: Matthew Kime --- .../edit_data_retention_modal.tsx | 109 ++++++++---------- .../validations.test.ts | 53 +++++++++ .../edit_data_retention_modal/validations.ts | 61 ++++++++++ 3 files changed, 161 insertions(+), 62 deletions(-) create mode 100644 x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.test.ts create mode 100644 x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.ts diff --git a/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx index ca3612bdab168..f747abca19f05 100644 --- a/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx +++ b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx @@ -23,6 +23,7 @@ import { has } from 'lodash'; import { ScopedHistory } from '@kbn/core/public'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n-react'; +import { isBiggerThanGlobalMaxRetention } from './validations'; import { useForm, useFormData, @@ -34,6 +35,7 @@ import { UseField, ToggleField, NumericField, + fieldValidators, } from '../../../../../shared_imports'; import { reactRouterNavigate } from '../../../../../shared_imports'; @@ -53,35 +55,6 @@ interface Props { onClose: (data?: { hasUpdatedDataRetention: boolean }) => void; } -const convertToMinutes = (value: string) => { - const { size, unit } = splitSizeAndUnits(value); - const sizeNum = parseInt(size, 10); - - switch (unit) { - case 'd': - // days to minutes - return sizeNum * 24 * 60; - case 'h': - // hours to minutes - return sizeNum * 60; - case 'm': - // minutes to minutes - return sizeNum; - case 's': - // seconds to minutes - return sizeNum / 60; - default: - throw new Error(`Unknown unit: ${unit}`); - } -}; - -const isRetentionBiggerThan = (valueA: string, valueB: string) => { - const minutesA = convertToMinutes(valueA); - const minutesB = convertToMinutes(valueB); - - return minutesA > minutesB; -}; - const configurationFormSchema: FormSchema = { dataRetention: { type: FIELD_TYPES.TEXT, @@ -94,50 +67,62 @@ const configurationFormSchema: FormSchema = { formatters: [fieldFormatters.toInt], validations: [ { - validator: ({ value, formData, customData }) => { - // If infiniteRetentionPeriod is set, we dont need to validate the data retention field - if (formData.infiniteRetentionPeriod) { - return undefined; + validator: ({ value }) => { + // TODO: Replace with validator added in https://github.com/elastic/kibana/pull/196527/ + if (!Number.isInteger(Number(value ?? ''))) { + return { + message: i18n.translate( + 'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldIntegerError', + { + defaultMessage: 'Only integers are allowed.', + } + ), + }; } - - // If project level data retention is enabled, we need to enforce the global max retention - const { globalMaxRetention, enableProjectLevelRetentionChecks } = customData.value as any; - if (enableProjectLevelRetentionChecks) { - const currentValue = `${value}${formData.timeUnit}`; - if (globalMaxRetention && isRetentionBiggerThan(currentValue, globalMaxRetention)) { - return { - message: i18n.translate( - 'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldMaxError', - { - defaultMessage: - 'Maximum data retention period on this project is {maxRetention} days.', - // Remove the unit from the globalMaxRetention value - values: { maxRetention: globalMaxRetention.slice(0, -1) }, - } - ), - }; + }, + }, + { + validator: ({ value, formData, customData }) => { + // We only need to validate the data retention field if infiniteRetentionPeriod is set to false + if (!formData.infiniteRetentionPeriod) { + // If project level data retention is enabled, we need to enforce the global max retention + const { globalMaxRetention, enableProjectLevelRetentionChecks } = + customData.value as any; + if (enableProjectLevelRetentionChecks) { + return isBiggerThanGlobalMaxRetention(value, formData.timeUnit, globalMaxRetention); } } - - if (!value) { - return { - message: i18n.translate( + }, + }, + { + validator: (args) => { + // We only need to validate the data retention field if infiniteRetentionPeriod is set to false + if (!args.formData.infiniteRetentionPeriod) { + return fieldValidators.emptyField( + i18n.translate( 'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldRequiredError', { defaultMessage: 'A data retention value is required.', } - ), - }; + ) + )(args); } - if (value <= 0) { - return { + }, + }, + { + validator: (args) => { + // We only need to validate the data retention field if infiniteRetentionPeriod is set to false + if (!args.formData.infiniteRetentionPeriod) { + return fieldValidators.numberGreaterThanField({ + than: 0, + allowEquality: false, message: i18n.translate( 'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldNonNegativeError', { defaultMessage: `A positive value is required.`, } ), - }; + })(args); } }, }, @@ -258,11 +243,11 @@ export const EditDataRetentionModal: React.FunctionComponent = ({ const formHasErrors = form.getErrors().length > 0; const disableSubmit = formHasErrors || !isDirty || form.isValid === false; - // Whenever the formData changes, we need to re-validate the dataRetention field - // as it depends on the timeUnit field. + // Whenever the timeUnit field changes, we need to re-validate + // the dataRetention field useEffect(() => { form.validateFields(['dataRetention']); - }, [formData, form]); + }, [formData.timeUnit, form]); const onSubmitForm = async () => { const { isValid, data } = await form.submit(); diff --git a/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.test.ts b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.test.ts new file mode 100644 index 0000000000000..0768d0990cdc0 --- /dev/null +++ b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.test.ts @@ -0,0 +1,53 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { isBiggerThanGlobalMaxRetention } from './validations'; + +describe('isBiggerThanGlobalMaxRetention', () => { + it('should return undefined if any argument is missing', () => { + expect(isBiggerThanGlobalMaxRetention('', 'd', '30d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(10, '', '30d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(10, 'd', '')).toBeUndefined(); + }); + + it('should return an error message if retention is bigger than global max retention (in days)', () => { + const result = isBiggerThanGlobalMaxRetention(40, 'd', '30d'); + expect(result).toEqual({ + message: 'Maximum data retention period on this project is 30 days.', + }); + }); + + it('should return undefined if retention is smaller than or equal to global max retention (in days)', () => { + expect(isBiggerThanGlobalMaxRetention(30, 'd', '30d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(25, 'd', '30d')).toBeUndefined(); + }); + + it('should correctly compare retention in different time units against days', () => { + expect(isBiggerThanGlobalMaxRetention(24, 'h', '1d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(23, 'h', '1d')).toBeUndefined(); + // 30 days = 720 hours + expect(isBiggerThanGlobalMaxRetention(800, 'h', '30d')).toEqual({ + message: 'Maximum data retention period on this project is 30 days.', + }); + + // 1 day = 1440 minutes + expect(isBiggerThanGlobalMaxRetention(1440, 'm', '1d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(3000, 'm', '2d')).toEqual({ + message: 'Maximum data retention period on this project is 2 days.', + }); + + // 1 day = 86400 seconds + expect(isBiggerThanGlobalMaxRetention(1000, 's', '1d')).toBeUndefined(); + expect(isBiggerThanGlobalMaxRetention(87000, 's', '1d')).toEqual({ + message: 'Maximum data retention period on this project is 1 days.', + }); + }); + + it('should throw an error for unknown time units', () => { + expect(() => isBiggerThanGlobalMaxRetention(10, 'x', '30d')).toThrow('Unknown unit: x'); + }); +}); diff --git a/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.ts b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.ts new file mode 100644 index 0000000000000..831ac2f4c26b9 --- /dev/null +++ b/x-pack/plugins/index_management/public/application/sections/home/data_stream_list/edit_data_retention_modal/validations.ts @@ -0,0 +1,61 @@ +/* + * 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; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { i18n } from '@kbn/i18n'; +import { splitSizeAndUnits } from '../../../../../../common'; + +const convertToMinutes = (value: string) => { + const { size, unit } = splitSizeAndUnits(value); + const sizeNum = parseInt(size, 10); + + switch (unit) { + case 'd': + // days to minutes + return sizeNum * 24 * 60; + case 'h': + // hours to minutes + return sizeNum * 60; + case 'm': + // minutes to minutes + return sizeNum; + case 's': + // seconds to minutes (round up if any remainder) + return Math.ceil(sizeNum / 60); + default: + throw new Error(`Unknown unit: ${unit}`); + } +}; + +const isRetentionBiggerThan = (valueA: string, valueB: string) => { + const minutesA = convertToMinutes(valueA); + const minutesB = convertToMinutes(valueB); + + return minutesA > minutesB; +}; + +export const isBiggerThanGlobalMaxRetention = ( + retentionValue: string | number, + retentionTimeUnit: string, + globalMaxRetention: string +) => { + if (!retentionValue || !retentionTimeUnit || !globalMaxRetention) { + return undefined; + } + + return isRetentionBiggerThan(`${retentionValue}${retentionTimeUnit}`, globalMaxRetention) + ? { + message: i18n.translate( + 'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldMaxError', + { + defaultMessage: 'Maximum data retention period on this project is {maxRetention} days.', + // Remove the unit from the globalMaxRetention value + values: { maxRetention: globalMaxRetention.slice(0, -1) }, + } + ), + } + : undefined; +};