Skip to content

Commit

Permalink
[Index Management] Fix unhandled error in ds data retention modal (#1…
Browse files Browse the repository at this point in the history
…96524)

Fixes #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 <[email protected]>
  • Loading branch information
ElenaStoeva and mattkime authored Oct 22, 2024
1 parent 68b328d commit d8fa996
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -34,6 +35,7 @@ import {
UseField,
ToggleField,
NumericField,
fieldValidators,
} from '../../../../../shared_imports';

import { reactRouterNavigate } from '../../../../../shared_imports';
Expand All @@ -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,
Expand All @@ -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);
}
},
},
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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');
});
});
Original file line number Diff line number Diff line change
@@ -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;
};

0 comments on commit d8fa996

Please sign in to comment.