From 332b803ca50c530a5f2ad55c4840d57c0fd8fa73 Mon Sep 17 00:00:00 2001 From: Elena Stoeva Date: Thu, 26 Sep 2024 14:18:28 +0100 Subject: [PATCH 1/5] [Mappings editor] Only add advanced options to request if different from default value --- .../forms/hook_form_lib/hooks/use_form.ts | 29 ++++++++++-- .../configuration_form/configuration_form.tsx | 47 +++++++------------ 2 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index ecf3a242f0f16..f2d14074550a8 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -25,6 +25,7 @@ import { createArrayItem, getInternalArrayFieldPath } from '../components/use_ar const DEFAULT_OPTIONS = { valueChangeDebounceTime: 500, stripEmptyFields: true, + stripUnmodifiedFields: false, }; export interface UseFormReturn { @@ -66,13 +67,18 @@ export function useForm( return initDefaultValue(defaultValue); }, [defaultValue, initDefaultValue]); - const { valueChangeDebounceTime, stripEmptyFields: doStripEmptyFields } = options ?? {}; + const { + valueChangeDebounceTime, + stripEmptyFields: doStripEmptyFields, + stripUnmodifiedFields, + } = options ?? {}; const formOptions = useMemo( () => ({ stripEmptyFields: doStripEmptyFields ?? DEFAULT_OPTIONS.stripEmptyFields, valueChangeDebounceTime: valueChangeDebounceTime ?? DEFAULT_OPTIONS.valueChangeDebounceTime, + stripUnmodifiedFields: stripUnmodifiedFields ?? DEFAULT_OPTIONS.stripUnmodifiedFields }), - [valueChangeDebounceTime, doStripEmptyFields] + [valueChangeDebounceTime, doStripEmptyFields, stripUnmodifiedFields] ); const [isSubmitted, setIsSubmitted] = useState(false); @@ -178,7 +184,10 @@ export function useForm( const fieldsToArray = useCallback<() => FieldHook[]>(() => Object.values(fieldsRefs.current), []); const getFieldsForOutput = useCallback( - (fields: FieldsMap, opts: { stripEmptyFields: boolean }): FieldsMap => { + ( + fields: FieldsMap, + opts: { stripEmptyFields: boolean; stripUnmodifiedFields: boolean } + ): FieldsMap => { return Object.entries(fields).reduce((acc, [key, field]) => { if (!field.__isIncludedInOutput) { return acc; @@ -191,6 +200,12 @@ export function useForm( } } + if (opts.stripUnmodifiedFields) { + if (!field.isModified) { + return acc; + } + } + acc[key] = field; return acc; }, {} as FieldsMap); @@ -396,12 +411,18 @@ export function useForm( const getFormData: FormHook['getFormData'] = useCallback(() => { const fieldsToOutput = getFieldsForOutput(fieldsRefs.current, { stripEmptyFields: formOptions.stripEmptyFields, + stripUnmodifiedFields: formOptions.stripUnmodifiedFields, }); const fieldsValue = mapFormFields(fieldsToOutput, (field) => field.__serializeValue()); return serializer ? serializer(unflattenObject(fieldsValue)) : unflattenObject(fieldsValue); - }, [getFieldsForOutput, formOptions.stripEmptyFields, serializer]); + }, [ + getFieldsForOutput, + formOptions.stripEmptyFields, + formOptions.stripUnmodifiedFields, + serializer, + ]); const getErrors: FormHook['getErrors'] = useCallback(() => { if (isValid === true) { diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx index b326779d8a76c..f9e786906d88f 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx @@ -29,30 +29,19 @@ interface Props { } const formSerializer = (formData: GenericObject, sourceFieldMode?: string) => { - const { - dynamicMapping: { - enabled: dynamicMappingsEnabled, - throwErrorsForUnmappedFields, - /* eslint-disable @typescript-eslint/naming-convention */ - numeric_detection, - date_detection, - dynamic_date_formats, - /* eslint-enable @typescript-eslint/naming-convention */ - }, - sourceField, - metaField, - _routing, - _size, - subobjects, - } = formData; + const { dynamicMapping, sourceField, metaField, _routing, _size, subobjects } = formData; - const dynamic = dynamicMappingsEnabled ? true : throwErrorsForUnmappedFields ? 'strict' : false; + const dynamic = dynamicMapping?.enabled + ? true + : dynamicMapping?.throwErrorsForUnmappedFields + ? 'strict' + : dynamicMapping?.enabled; const serialized = { dynamic, - numeric_detection, - date_detection, - dynamic_date_formats, + numeric_detection: dynamicMapping?.numeric_detection, + date_detection: dynamicMapping?.date_detection, + dynamic_date_formats: dynamicMapping?.dynamic_date_formats, _source: sourceFieldMode ? { mode: sourceFieldMode } : sourceField, _meta: metaField, _routing, @@ -85,18 +74,18 @@ const formDeserializer = (formData: GenericObject) => { return { dynamicMapping: { - enabled: dynamic === true || dynamic === undefined, + enabled: dynamic === 'strict' ? false : dynamic, throwErrorsForUnmappedFields: dynamic === 'strict', numeric_detection, date_detection, dynamic_date_formats, }, sourceField: { - enabled: enabled === true || enabled === undefined, + enabled, includes, excludes, }, - metaField: _meta ?? {}, + metaField: _meta, _routing, _size, subobjects, @@ -119,11 +108,11 @@ export const ConfigurationForm = React.memo(({ value, esNodesPlugins }: Props) = schema: configurationFormSchema, serializer: serializerCallback, deserializer: formDeserializer, - defaultValue: value, id: 'configurationForm', + options: { stripUnmodifiedFields: true }, }); const dispatch = useDispatch(); - const { subscribe, submit, reset, getFormData } = form; + const { subscribe, submit, getFormData, updateFieldValues } = form; const isMapperSizeSectionVisible = value?._size !== undefined || esNodesPlugins.includes(MapperSizePluginId); @@ -145,12 +134,8 @@ export const ConfigurationForm = React.memo(({ value, esNodesPlugins }: Props) = }, [dispatch, subscribe, submit]); useEffect(() => { - if (isMounted.current) { - // If the value has changed (it probably means that we have loaded a new JSON) - // we need to reset the form to update the fields values. - reset({ resetValues: true, defaultValue: value }); - } - }, [value, reset]); + updateFieldValues(value); + }, [value, updateFieldValues]); useEffect(() => { isMounted.current = true; From 8e58d7090da744f89687291ecf86e04e7089fa77 Mon Sep 17 00:00:00 2001 From: Elena Stoeva Date: Tue, 1 Oct 2024 21:37:38 +0100 Subject: [PATCH 2/5] Preserve set values in request --- .../forms/hook_form_lib/hooks/use_form.ts | 23 +++++------- .../static/forms/hook_form_lib/types.ts | 4 +++ .../configuration_form/configuration_form.tsx | 15 +++++--- .../index_management/index_template_wizard.ts | 35 +++++++++++++++++++ .../page_objects/index_management_page.ts | 15 ++++++++ 5 files changed, 73 insertions(+), 19 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index f2d14074550a8..da68c584ad61f 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -25,7 +25,7 @@ import { createArrayItem, getInternalArrayFieldPath } from '../components/use_ar const DEFAULT_OPTIONS = { valueChangeDebounceTime: 500, stripEmptyFields: true, - stripUnmodifiedFields: false, + stripUnsetFields: false, }; export interface UseFormReturn { @@ -70,15 +70,15 @@ export function useForm( const { valueChangeDebounceTime, stripEmptyFields: doStripEmptyFields, - stripUnmodifiedFields, + stripUnsetFields, } = options ?? {}; const formOptions = useMemo( () => ({ stripEmptyFields: doStripEmptyFields ?? DEFAULT_OPTIONS.stripEmptyFields, valueChangeDebounceTime: valueChangeDebounceTime ?? DEFAULT_OPTIONS.valueChangeDebounceTime, - stripUnmodifiedFields: stripUnmodifiedFields ?? DEFAULT_OPTIONS.stripUnmodifiedFields + stripUnsetFields: stripUnsetFields ?? DEFAULT_OPTIONS.stripUnsetFields, }), - [valueChangeDebounceTime, doStripEmptyFields, stripUnmodifiedFields] + [valueChangeDebounceTime, doStripEmptyFields, stripUnsetFields] ); const [isSubmitted, setIsSubmitted] = useState(false); @@ -186,7 +186,7 @@ export function useForm( const getFieldsForOutput = useCallback( ( fields: FieldsMap, - opts: { stripEmptyFields: boolean; stripUnmodifiedFields: boolean } + opts: { stripEmptyFields: boolean; stripUnsetFields: boolean } ): FieldsMap => { return Object.entries(fields).reduce((acc, [key, field]) => { if (!field.__isIncludedInOutput) { @@ -200,8 +200,8 @@ export function useForm( } } - if (opts.stripUnmodifiedFields) { - if (!field.isModified) { + if (opts.stripUnsetFields) { + if (!field.isDirty && getFieldDefaultValue(field.path) === undefined) { return acc; } } @@ -411,18 +411,13 @@ export function useForm( const getFormData: FormHook['getFormData'] = useCallback(() => { const fieldsToOutput = getFieldsForOutput(fieldsRefs.current, { stripEmptyFields: formOptions.stripEmptyFields, - stripUnmodifiedFields: formOptions.stripUnmodifiedFields, + stripUnsetFields: formOptions.stripUnsetFields, }); const fieldsValue = mapFormFields(fieldsToOutput, (field) => field.__serializeValue()); return serializer ? serializer(unflattenObject(fieldsValue)) : unflattenObject(fieldsValue); - }, [ - getFieldsForOutput, - formOptions.stripEmptyFields, - formOptions.stripUnmodifiedFields, - serializer, - ]); + }, [getFieldsForOutput, formOptions.stripEmptyFields, formOptions.stripUnsetFields, serializer]); const getErrors: FormHook['getErrors'] = useCallback(() => { if (isValid === true) { diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/types.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/types.ts index f700faa66e6fd..282b9de8d3ee4 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/types.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/types.ts @@ -141,6 +141,10 @@ export interface FormOptions { * Remove empty string field ("") from form data */ stripEmptyFields?: boolean; + /** + * Remove fields from form data that don't have initial value and are not modified by the user. + */ + stripUnsetFields?: boolean; } export interface FieldHook { diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx index f9e786906d88f..e3e32c55aada0 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/configuration_form/configuration_form.tsx @@ -75,7 +75,7 @@ const formDeserializer = (formData: GenericObject) => { return { dynamicMapping: { enabled: dynamic === 'strict' ? false : dynamic, - throwErrorsForUnmappedFields: dynamic === 'strict', + throwErrorsForUnmappedFields: dynamic === 'strict' ? true : undefined, numeric_detection, date_detection, dynamic_date_formats, @@ -108,11 +108,12 @@ export const ConfigurationForm = React.memo(({ value, esNodesPlugins }: Props) = schema: configurationFormSchema, serializer: serializerCallback, deserializer: formDeserializer, + defaultValue: value, id: 'configurationForm', - options: { stripUnmodifiedFields: true }, + options: { stripUnsetFields: true }, }); const dispatch = useDispatch(); - const { subscribe, submit, getFormData, updateFieldValues } = form; + const { subscribe, submit, reset, getFormData } = form; const isMapperSizeSectionVisible = value?._size !== undefined || esNodesPlugins.includes(MapperSizePluginId); @@ -134,8 +135,12 @@ export const ConfigurationForm = React.memo(({ value, esNodesPlugins }: Props) = }, [dispatch, subscribe, submit]); useEffect(() => { - updateFieldValues(value); - }, [value, updateFieldValues]); + if (isMounted.current) { + // If the value has changed (it probably means that we have loaded a new JSON) + // we need to reset the form to update the fields values. + reset({ resetValues: true, defaultValue: value }); + } + }, [value, reset]); useEffect(() => { isMounted.current = true; diff --git a/x-pack/test/functional/apps/index_management/index_template_wizard.ts b/x-pack/test/functional/apps/index_management/index_template_wizard.ts index e9eb68f749e78..5b49286f6182b 100644 --- a/x-pack/test/functional/apps/index_management/index_template_wizard.ts +++ b/x-pack/test/functional/apps/index_management/index_template_wizard.ts @@ -15,6 +15,7 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { const comboBox = getService('comboBox'); const find = getService('find'); const browser = getService('browser'); + const log = getService('log'); describe('Index template wizard', function () { before(async () => { @@ -162,6 +163,40 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => { expect(await testSubjects.exists('fieldSubType')).to.be(true); expect(await testSubjects.exists('nextButton')).to.be(true); }); + + it("advanced options tab doesn't add default values to request by default", async () => { + await pageObjects.indexManagement.changeMappingsEditorTab('advancedOptions'); + await testSubjects.click('previewIndexTemplate'); + const templatePreview = await testSubjects.getVisibleText('simulateTemplatePreview'); + + await log.debug(`Template preview text: ${templatePreview}`); + + // All advanced options should not be part of the request + expect(templatePreview).to.not.contain('"dynamic"'); + expect(templatePreview).to.not.contain('"subobjects"'); + expect(templatePreview).to.not.contain('"dynamic_date_formats"'); + expect(templatePreview).to.not.contain('"date_detection"'); + expect(templatePreview).to.not.contain('"numeric_detection"'); + }); + + it('advanced options tab adds the set values to the request', async () => { + await pageObjects.indexManagement.changeMappingsEditorTab('advancedOptions'); + + // Toggle the subobjects field to false + await testSubjects.click('subobjectsToggle'); + + await testSubjects.click('previewIndexTemplate'); + const templatePreview = await testSubjects.getVisibleText('simulateTemplatePreview'); + + await log.debug(`Template preview text: ${templatePreview}`); + + // Only the subobjects option should be part of the request + expect(templatePreview).to.contain('"subobjects": false'); + expect(templatePreview).to.not.contain('"dynamic"'); + expect(templatePreview).to.not.contain('"dynamic_date_formats"'); + expect(templatePreview).to.not.contain('"date_detection"'); + expect(templatePreview).to.not.contain('"numeric_detection"'); + }); }); }); }; diff --git a/x-pack/test/functional/page_objects/index_management_page.ts b/x-pack/test/functional/page_objects/index_management_page.ts index 8730d2807cd21..990ddd694f3ed 100644 --- a/x-pack/test/functional/page_objects/index_management_page.ts +++ b/x-pack/test/functional/page_objects/index_management_page.ts @@ -6,6 +6,7 @@ */ import expect from '@kbn/expect'; import { FtrProviderContext } from '../ftr_provider_context'; +import { act } from "react-dom/test-utils"; export function IndexManagementPageProvider({ getService }: FtrProviderContext) { const retry = getService('retry'); @@ -118,6 +119,20 @@ export function IndexManagementPageProvider({ getService }: FtrProviderContext) await testSubjects.click(tab); }, + async changeMappingsEditorTab( + tab: 'mappedFields' | 'runtimeFields' | 'dynamicTemplates' | 'advancedOptions' + ) { + const index = [ + 'mappedFields', + 'runtimeFields', + 'dynamicTemplates', + 'advancedOptions', + ].indexOf(tab); + + const tabs = await testSubjects.findAll('formTab'); + await tabs[index].click(); + }, + async clickNextButton() { await testSubjects.click('nextButton'); }, From 6e0c81f2f86eed064a7ddc1c8b318004557c2c6e Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 1 Oct 2024 21:47:44 +0000 Subject: [PATCH 3/5] [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' --- x-pack/test/functional/page_objects/index_management_page.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/test/functional/page_objects/index_management_page.ts b/x-pack/test/functional/page_objects/index_management_page.ts index 990ddd694f3ed..8077581bbbb48 100644 --- a/x-pack/test/functional/page_objects/index_management_page.ts +++ b/x-pack/test/functional/page_objects/index_management_page.ts @@ -6,7 +6,6 @@ */ import expect from '@kbn/expect'; import { FtrProviderContext } from '../ftr_provider_context'; -import { act } from "react-dom/test-utils"; export function IndexManagementPageProvider({ getService }: FtrProviderContext) { const retry = getService('retry'); From 99b702380ea2638475164ce9dec43a1e81f3f9ef Mon Sep 17 00:00:00 2001 From: Elena Stoeva Date: Wed, 2 Oct 2024 10:58:54 +0100 Subject: [PATCH 4/5] Fix lint error and add documentation --- .../static/forms/docs/core/use_form_hook.mdx | 45 +++++++++++++++++++ .../forms/hook_form_lib/hooks/use_form.ts | 2 +- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/plugins/es_ui_shared/static/forms/docs/core/use_form_hook.mdx b/src/plugins/es_ui_shared/static/forms/docs/core/use_form_hook.mdx index 82cd0c88834a3..b8186aac3ac84 100644 --- a/src/plugins/es_ui_shared/static/forms/docs/core/use_form_hook.mdx +++ b/src/plugins/es_ui_shared/static/forms/docs/core/use_form_hook.mdx @@ -261,3 +261,48 @@ With this option you can decide if you want empty string value to be returned by "role": "" } ``` + +#### stripUnsetFields + +**Type:** `boolean` +**Default:** `false` + +Sometimes, we only want to include fields that have a defined initial value or if their value has been set by the user. +In this case, set `stripUnsetFields` to `true`. + +Suppose we have a toggle field `autocompleteEnabled`, which doesn't have a specified default value passed to `useForm`: + +```js +const { form } = useForm({ + defaultValue: { + darkModeEnabled: false, + accessibilityEnabled: true, + autocompleteEnabled: undefined, + }, + options: { stripUnsetFields: true }, +}); +``` + +Initially, the form data includes only `darkModeEnabled` and `accessibilityEnabled` because `autocompleteEnabled` is stripped. + +```js +{ + "darkModeEnabled": false, + "accessibilityEnabled": true, +} +``` + +Then the user toggles the `autocompleteEnabled` field to `false`. Now the field is included in the form data: + +```js +{ + "darkModeEnabled": false, + "accessibilityEnabled": true, + "autocompleteEnabled": false, +} +``` + +Note: This option only considers the `defaultValue` config passed to `useForm()` to determine if the initial value is +undefined. If a default value has been specified as a prop to the `` component or in the form schema, +but not in the `defaultValue` config for `useForm()`, the field would initially be populated with the specified default +value, but it won't be included in the form data until the user explicitly sets its value. diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index da68c584ad61f..d4bdc17bd5cea 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -210,7 +210,7 @@ export function useForm( return acc; }, {} as FieldsMap); }, - [] + [getFieldDefaultValue] ); const updateFormDataAt: FormHook['__updateFormDataAt'] = useCallback( From 6f577c7f2d7a76899a83f892c59a9b6515b3f2e3 Mon Sep 17 00:00:00 2001 From: Elena Stoeva Date: Wed, 2 Oct 2024 17:13:32 +0100 Subject: [PATCH 5/5] Fix type error --- .../static/forms/hook_form_lib/hooks/use_form.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index d4bdc17bd5cea..84b338fb95106 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -183,6 +183,11 @@ export function useForm( const fieldsToArray = useCallback<() => FieldHook[]>(() => Object.values(fieldsRefs.current), []); + const getFieldDefaultValue: FormHook['getFieldDefaultValue'] = useCallback( + (fieldName) => get(defaultValueDeserialized.current ?? {}, fieldName), + [] + ); + const getFieldsForOutput = useCallback( ( fields: FieldsMap, @@ -471,11 +476,6 @@ export function useForm( const getFields: FormHook['getFields'] = useCallback(() => fieldsRefs.current, []); - const getFieldDefaultValue: FormHook['getFieldDefaultValue'] = useCallback( - (fieldName) => get(defaultValueDeserialized.current ?? {}, fieldName), - [] - ); - const updateFieldValues: FormHook['updateFieldValues'] = useCallback( (updatedFormData, { runDeserializer = true } = {}) => { if (