[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 <matt@mattki.me>
This commit is contained in:
Elena Stoeva 2024-10-22 15:53:37 +01:00 committed by GitHub
parent 68b328d36b
commit d8fa996c50
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 162 additions and 63 deletions

View file

@ -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;
}
// 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) },
}
),
};
}
}
if (!value) {
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.dataRetentionFieldRequiredError',
'xpack.idxMgmt.dataStreamsDetailsPanel.editDataRetentionModal.dataRetentionFieldIntegerError',
{
defaultMessage: 'A data retention value is required.',
defaultMessage: 'Only integers are allowed.',
}
),
};
}
if (value <= 0) {
return {
},
},
{
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);
}
}
},
},
{
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);
}
},
},
{
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<Props> = ({
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();

View file

@ -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');
});
});

View file

@ -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;
};