From cd3f85e74fd8a1a2e1bc3bb5a2e3320d744c96dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Wed, 29 Jan 2020 16:35:53 +0530 Subject: [PATCH 1/3] Add tests for load_mappings_provider --- .../helpers/mappings_editor.helpers.ts | 2 +- .../mappings_editor.test.tsx | 4 +- .../load_mappings_provider.test.tsx | 77 +++++++++++++++++++ .../load_mappings/load_mappings_provider.tsx | 2 +- 4 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.test.tsx diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.ts index b8b67a0f36bd2..dcee956130a29 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/helpers/mappings_editor.helpers.ts @@ -13,4 +13,4 @@ export const setup = (props: any) => wrapComponent: false, }, defaultProps: props, - }); + })(); diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/mappings_editor.test.tsx b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/mappings_editor.test.tsx index 9e390e785c7d5..723c105d403b8 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/mappings_editor.test.tsx +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/__jest__/client_integration/mappings_editor.test.tsx @@ -28,7 +28,7 @@ describe('', () => { }, }, }; - const testBed = await setup({ onUpdate: mockOnUpdate, defaultValue })(); + const testBed = await setup({ onUpdate: mockOnUpdate, defaultValue }); const { exists } = testBed; expect(exists('mappingsEditor')).toBe(true); @@ -44,7 +44,7 @@ describe('', () => { }, }, }; - const testBed = await setup({ onUpdate: mockOnUpdate, defaultValue })(); + const testBed = await setup({ onUpdate: mockOnUpdate, defaultValue }); const { exists } = testBed; expect(exists('mappingsEditor')).toBe(true); diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.test.tsx b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.test.tsx new file mode 100644 index 0000000000000..a9433d3a7530f --- /dev/null +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.test.tsx @@ -0,0 +1,77 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +import React from 'react'; +import { act } from 'react-dom/test-utils'; + +jest.mock('@elastic/eui', () => ({ + ...jest.requireActual('@elastic/eui'), + // Mocking EuiCodeEditor, which uses React Ace under the hood + EuiCodeEditor: (props: any) => ( + { + props.onChange(syntheticEvent.jsonString); + }} + /> + ), +})); + +import { registerTestBed, nextTick, TestBed } from '../../../../../../../../../test_utils'; +import { LoadMappingsProvider } from './load_mappings_provider'; + +const ComponentToTest = ({ onJson }: { onJson: () => void }) => ( + + {openModal => ( + + )} + +); + +const setup = (props: any) => + registerTestBed(ComponentToTest, { + memoryRouter: { wrapComponent: false }, + defaultProps: props, + })(); + +const openModalWithJsonContent = ({ find, component }: TestBed) => async (json: any) => { + find('load-json-button').simulate('click'); + component.update(); + + // Set the mappings to load + // @ts-ignore + await act(async () => { + find('mockCodeEditor').simulate('change', { + jsonString: JSON.stringify(json), + }); + await nextTick(300); // There is a debounce in the JsonEditor that we need to wait for + }); +}; + +describe('', () => { + test('it should forward valid mapping definition', async () => { + const mappingsToLoad = { + properties: { + title: { + type: 'text', + }, + }, + }; + + const onJson = jest.fn(); + const testBed = await setup({ onJson }); + + // Open the modal and add the JSON + await openModalWithJsonContent(testBed)(mappingsToLoad); + + // Confirm + testBed.find('confirmModalConfirmButton').simulate('click'); + + const [jsonReturned] = onJson.mock.calls[0]; + expect(jsonReturned).toEqual({ ...mappingsToLoad, dynamic_templates: [] }); + }); +}); diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.tsx b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.tsx index a55bd96dce3d0..6bc360a1ec70e 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.tsx +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/load_mappings/load_mappings_provider.tsx @@ -25,7 +25,7 @@ type OpenJsonModalFunc = () => void; interface Props { onJson(json: { [key: string]: any }): void; - children: (deleteProperty: OpenJsonModalFunc) => React.ReactNode; + children: (openModal: OpenJsonModalFunc) => React.ReactNode; } interface State { From 38b1030e722c810187aba7f8b97c06840dcc41df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Wed, 29 Jan 2020 16:37:34 +0530 Subject: [PATCH 2/3] Improve mappings validator: don't allow unknown properties --- .../lib/mappings_validator.test.ts | 26 ++++++- .../mappings_editor/lib/mappings_validator.ts | 74 ++++++++++++++----- 2 files changed, 76 insertions(+), 24 deletions(-) diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.test.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.test.ts index f1e6efb06c649..d67c267dda6ae 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.test.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.test.ts @@ -18,6 +18,24 @@ describe('Mappings configuration validator', () => { }); }); + it('should detect valid mappings configuration', () => { + const mappings = { + _source: { + includes: [], + excludes: [], + enabled: true, + }, + _meta: {}, + _routing: { + required: false, + }, + dynamic: true, + }; + + const { errors } = validateMappings(mappings); + expect(errors).toBe(undefined); + }); + it('should strip out unknown configuration', () => { const mappings = { dynamic: true, @@ -30,6 +48,7 @@ describe('Mappings configuration validator', () => { excludes: ['abc'], }, properties: { title: { type: 'text' } }, + dynamic_templates: [], unknown: 123, }; @@ -37,7 +56,7 @@ describe('Mappings configuration validator', () => { const { unknown, ...expected } = mappings; expect(value).toEqual(expected); - expect(errors).toBe(undefined); + expect(errors).toEqual([{ code: 'ERR_CONFIG', configName: 'unknown' }]); }); it('should strip out invalid configuration and returns the errors for each of them', () => { @@ -47,9 +66,8 @@ describe('Mappings configuration validator', () => { dynamic_date_formats: false, // wrong format _source: { enabled: true, - includes: 'abc', + unknownProp: 'abc', // invalid excludes: ['abc'], - wrong: 123, // parameter not allowed }, properties: 'abc', }; @@ -59,10 +77,10 @@ describe('Mappings configuration validator', () => { expect(value).toEqual({ dynamic: true, properties: {}, + dynamic_templates: [], }); expect(errors).not.toBe(undefined); - expect(errors!.length).toBe(3); expect(errors!).toEqual([ { code: 'ERR_CONFIG', configName: '_source' }, { code: 'ERR_CONFIG', configName: 'dynamic_date_formats' }, diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.ts index 6ccbfeb50dcf4..78d638e398593 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/mappings_validator.ts @@ -196,23 +196,30 @@ export const validateProperties = (properties = {}): PropertiesValidatorResponse * Single source of truth to validate the *configuration* of the mappings. * Whenever a user loads a JSON object it will be validate against this Joi schema. */ -export const mappingsConfigurationSchema = t.partial({ - dynamic: t.union([t.literal(true), t.literal(false), t.literal('strict')]), - date_detection: t.boolean, - numeric_detection: t.boolean, - dynamic_date_formats: t.array(t.string), - _source: t.partial({ - enabled: t.boolean, - includes: t.array(t.string), - excludes: t.array(t.string), - }), - _meta: t.UnknownRecord, - _routing: t.partial({ - required: t.boolean, - }), -}); - -export const mappingsConfigurationSchemaKeys = Object.keys(mappingsConfigurationSchema.props); +export const mappingsConfigurationSchema = t.exact( + t.partial({ + dynamic: t.union([t.literal(true), t.literal(false), t.literal('strict')]), + date_detection: t.boolean, + numeric_detection: t.boolean, + dynamic_date_formats: t.array(t.string), + _source: t.exact( + t.partial({ + enabled: t.boolean, + includes: t.array(t.string), + excludes: t.array(t.string), + }) + ), + _meta: t.UnknownRecord, + _routing: t.interface({ + required: t.boolean, + }), + }) +); + +const mappingsConfigurationSchemaKeys = Object.keys(mappingsConfigurationSchema.type.props); +const sourceConfigurationSchemaKeys = Object.keys( + mappingsConfigurationSchema.type.props._source.type.props +); export const validateMappingsConfiguration = ( mappingsConfiguration: any @@ -222,8 +229,20 @@ export const validateMappingsConfiguration = ( let copyOfMappingsConfig = { ...mappingsConfiguration }; const result = mappingsConfigurationSchema.decode(mappingsConfiguration); + const isSchemaInvalid = isLeft(result); - if (isLeft(result)) { + const unknownConfigurationParameters = Object.keys(mappingsConfiguration).filter( + key => mappingsConfigurationSchemaKeys.includes(key) === false + ); + + const unknownSourceConfigurationParameters = + mappingsConfiguration._source !== undefined + ? Object.keys(mappingsConfiguration._source).filter( + key => sourceConfigurationSchemaKeys.includes(key) === false + ) + : []; + + if (isSchemaInvalid) { /** * To keep the logic simple we will strip out the parameters that contain errors */ @@ -235,6 +254,15 @@ export const validateMappingsConfiguration = ( }); } + if (unknownConfigurationParameters.length > 0) { + unknownConfigurationParameters.forEach(configName => configurationRemoved.add(configName)); + } + + if (unknownSourceConfigurationParameters.length > 0) { + configurationRemoved.add('_source'); + delete copyOfMappingsConfig._source; + } + copyOfMappingsConfig = pick(copyOfMappingsConfig, mappingsConfigurationSchemaKeys); const errors: MappingsValidationError[] = toArray(ordString)(configurationRemoved) @@ -252,7 +280,7 @@ export const validateMappings = (mappings: any = {}): MappingsValidatorResponse return { value: {} }; } - const { properties, dynamic_templates, ...mappingsConfiguration } = mappings; + const { properties, dynamic_templates: dynamicTemplates, ...mappingsConfiguration } = mappings; const { value: parsedConfiguration, errors: configurationErrors } = validateMappingsConfiguration( mappingsConfiguration @@ -265,8 +293,14 @@ export const validateMappings = (mappings: any = {}): MappingsValidatorResponse value: { ...parsedConfiguration, properties: parsedProperties, - dynamic_templates, + dynamic_templates: dynamicTemplates ?? [], }, errors: errors.length ? errors : undefined, }; }; + +export const VALID_MAPPINGS_PARAMETERS = [ + ...mappingsConfigurationSchemaKeys, + 'dynamic_templates', + 'properties', +]; From 4af8138899880e7cba181b941d1883f27ce5768f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien?= Date: Wed, 29 Jan 2020 16:45:21 +0530 Subject: [PATCH 3/3] Refactor extract_mappings_definition --- .../mappings_editor/lib/extract_mappings_definition.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/extract_mappings_definition.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/extract_mappings_definition.ts index eae3c5b15759c..817b0f4a4d3d0 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/extract_mappings_definition.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/lib/extract_mappings_definition.ts @@ -6,15 +6,10 @@ import { isPlainObject } from 'lodash'; import { GenericObject } from '../types'; -import { - validateMappingsConfiguration, - mappingsConfigurationSchemaKeys, -} from './mappings_validator'; - -const ALLOWED_PARAMETERS = [...mappingsConfigurationSchemaKeys, 'dynamic_templates', 'properties']; +import { validateMappingsConfiguration, VALID_MAPPINGS_PARAMETERS } from './mappings_validator'; const isMappingDefinition = (obj: GenericObject): boolean => { - const areAllKeysValid = Object.keys(obj).every(key => ALLOWED_PARAMETERS.includes(key)); + const areAllKeysValid = Object.keys(obj).every(key => VALID_MAPPINGS_PARAMETERS.includes(key)); if (!areAllKeysValid) { return false;