-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Index template] Fix editor should support mappings types #55804
Merged
sebelga
merged 19 commits into
elastic:7.x
from
sebelga:fix/Index-template-editor-should-support-mappings-types
Jan 29, 2020
Merged
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
0bae3df
Add type metadata to extract mappings definition
sebelga bfc5226
Add "include_type_name" parameter to server API req
sebelga 892afa9
Return mappings inside type if one has been detected
sebelga 7d5def9
Add "include_type_name" req param to client requests
sebelga 92875e4
Move query param to "sendRequest" query object
sebelga de48af4
Forward "include_type_name" api call to ES request call
sebelga 028bb28
Fix API integration test
sebelga ba056e1
Refactor client integration tests: call registerTestBed() from setup
sebelga 001442c
Add strictness to mappings validator to not allow unknown parameters
sebelga a97722a
Add test to detect custom type with mappings configuration name
sebelga b9ad4ac
Use "dynamic" in test type detection instead of "_source"
sebelga 1d2a508
Detect mappings type in LoadMappingsProvider
sebelga 3493e7d
Add client integration tests for LoadMappignsProvider
sebelga 0045d0d
Merge remote-tracking branch 'upstream/7.x' into fix/Index-template-e…
sebelga df98c23
Refactor: change helper func to "doMappingsHaveType"
sebelga 9f281c2
Fix TS issues
sebelga 0f5e083
Merge remote-tracking branch 'upstream/7.x' into fix/Index-template-e…
sebelga 8fa68b5
Address CR changes
sebelga 1cbaf4d
Remove comment about io-ts lib
sebelga File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,4 +13,4 @@ export const setup = (props: any) => | |
wrapComponent: false, | ||
}, | ||
defaultProps: props, | ||
}); | ||
})(); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
193 changes: 193 additions & 0 deletions
193
...c/app/components/mappings_editor/components/load_mappings/load_mappings_provider.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,193 @@ | ||
/* | ||
* 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) => ( | ||
<input | ||
data-test-subj="mockCodeEditor" | ||
onChange={(syntheticEvent: any) => { | ||
props.onChange(syntheticEvent.jsonString); | ||
}} | ||
/> | ||
), | ||
})); | ||
|
||
import { registerTestBed, nextTick, TestBed } from '../../../../../../../../../test_utils'; | ||
import { LoadMappingsProvider } from './load_mappings_provider'; | ||
|
||
const ComponentToTest = ({ onJson }: { onJson: () => void }) => ( | ||
<LoadMappingsProvider onJson={onJson}> | ||
{openModal => ( | ||
<button onClick={openModal} data-test-subj="load-json-button"> | ||
Load JSON | ||
</button> | ||
)} | ||
</LoadMappingsProvider> | ||
); | ||
|
||
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('<LoadMappingsProvider />', () => { | ||
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: [] }); | ||
}); | ||
|
||
test('it should detect custom single-type mappings and return it', async () => { | ||
const mappingsToLoadOneType = { | ||
myCustomType: { | ||
_source: { | ||
enabled: true, | ||
}, | ||
properties: { | ||
title: { | ||
type: 'text', | ||
}, | ||
}, | ||
dynamic_templates: [], | ||
}, | ||
}; | ||
|
||
const onJson = jest.fn(); | ||
const testBed = await setup({ onJson }); | ||
|
||
await openModalWithJsonContent(testBed)(mappingsToLoadOneType); | ||
|
||
// Confirm | ||
testBed.find('confirmModalConfirmButton').simulate('click'); | ||
|
||
const [jsonReturned] = onJson.mock.calls[0]; | ||
expect(jsonReturned).toEqual(mappingsToLoadOneType); | ||
}); | ||
|
||
test('it should detect multi-type mappings and return raw without validating', async () => { | ||
const mappingsToLoadMultiType = { | ||
myCustomType1: { | ||
wrongParameter: 'wont be validated neither stripped out', | ||
properties: { | ||
title: { | ||
type: 'wrongType', | ||
}, | ||
}, | ||
dynamic_templates: [], | ||
}, | ||
myCustomType2: { | ||
properties: { | ||
title: { | ||
type: 'text', | ||
}, | ||
}, | ||
dynamic_templates: [], | ||
}, | ||
}; | ||
|
||
const onJson = jest.fn(); | ||
const testBed = await setup({ onJson }); | ||
|
||
await openModalWithJsonContent(testBed)(mappingsToLoadMultiType); | ||
|
||
// Confirm | ||
testBed.find('confirmModalConfirmButton').simulate('click'); | ||
|
||
const [jsonReturned] = onJson.mock.calls[0]; | ||
expect(jsonReturned).toEqual(mappingsToLoadMultiType); | ||
}); | ||
|
||
test('it should detect single-type mappings under a valid mappings definition parameter', async () => { | ||
const mappingsToLoadOneType = { | ||
// Custom type name _is_ a valid mappings definition parameter | ||
_source: { | ||
_source: { | ||
enabled: true, | ||
}, | ||
properties: { | ||
title: { | ||
type: 'text', | ||
}, | ||
}, | ||
dynamic_templates: [], | ||
}, | ||
}; | ||
|
||
const onJson = jest.fn(); | ||
const testBed = await setup({ onJson }); | ||
|
||
await openModalWithJsonContent(testBed)(mappingsToLoadOneType); | ||
|
||
// Confirm | ||
testBed.find('confirmModalConfirmButton').simulate('click'); | ||
|
||
const [jsonReturned] = onJson.mock.calls[0]; | ||
expect(jsonReturned).toEqual(mappingsToLoadOneType); | ||
}); | ||
|
||
test('should treat "properties" as properties definition and **not** as a cutom type', async () => { | ||
const mappingsToLoadOneType = { | ||
// Custom type name _is_ a valid mappings definition parameter | ||
properties: { | ||
_source: { | ||
enabled: true, | ||
}, | ||
properties: { | ||
title: { | ||
type: 'text', | ||
}, | ||
}, | ||
dynamic_templates: [], | ||
}, | ||
}; | ||
|
||
const onJson = jest.fn(); | ||
const testBed = await setup({ onJson }); | ||
|
||
await openModalWithJsonContent(testBed)(mappingsToLoadOneType); | ||
|
||
// Confirm | ||
testBed.find('confirmModalConfirmButton').simulate('click'); | ||
|
||
// Make sure our handler hasn't been called | ||
expect(onJson.mock.calls.length).toBe(0); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
*/ | ||
|
||
import React, { useState, useRef } from 'react'; | ||
import { isPlainObject } from 'lodash'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
import { | ||
|
@@ -17,15 +18,15 @@ import { | |
} from '@elastic/eui'; | ||
|
||
import { JsonEditor, OnJsonEditorUpdateHandler } from '../../shared_imports'; | ||
import { validateMappings, MappingsValidationError } from '../../lib'; | ||
import { validateMappings, MappingsValidationError, VALID_MAPPINGS_PARAMETERS } from '../../lib'; | ||
|
||
const MAX_ERRORS_TO_DISPLAY = 1; | ||
|
||
type OpenJsonModalFunc = () => void; | ||
|
||
interface Props { | ||
onJson(json: { [key: string]: any }): void; | ||
children: (deleteProperty: OpenJsonModalFunc) => React.ReactNode; | ||
children: (openModal: OpenJsonModalFunc) => React.ReactNode; | ||
} | ||
|
||
interface State { | ||
|
@@ -126,10 +127,13 @@ const getErrorMessage = (error: MappingsValidationError) => { | |
} | ||
}; | ||
|
||
const areAllObjectKeysValidParameters = (obj: { [key: string]: any }) => | ||
Object.keys(obj).every(key => VALID_MAPPINGS_PARAMETERS.includes(key)); | ||
|
||
export const LoadMappingsProvider = ({ onJson, children }: Props) => { | ||
const [state, setState] = useState<State>({ isModalOpen: false }); | ||
const [totalErrorsToDisplay, setTotalErrorsToDisplay] = useState<number>(MAX_ERRORS_TO_DISPLAY); | ||
const jsonContent = useRef<Parameters<OnJsonEditorUpdateHandler>['0'] | undefined>(); | ||
const jsonContent = useRef<Parameters<OnJsonEditorUpdateHandler>['0'] | undefined>(undefined); | ||
const view: ModalView = | ||
state.json !== undefined && state.errors !== undefined ? 'validationResult' : 'json'; | ||
const i18nTexts = getTexts(view, state.errors?.length); | ||
|
@@ -146,6 +150,44 @@ export const LoadMappingsProvider = ({ onJson, children }: Props) => { | |
setState({ isModalOpen: false }); | ||
}; | ||
|
||
const getMappingsMetadata = (unparsed: { | ||
[key: string]: any; | ||
}): { customType?: string; isMultiTypeMappings: boolean } => { | ||
let hasCustomType = false; | ||
let isMultiTypeMappings = false; | ||
let customType: string | undefined; | ||
|
||
/** | ||
* We need to check if there are single or multi-types mappings declared, for that we will check for the following: | ||
* | ||
* - Are **all** root level keys valid parameter for the mappings definition. If not, and all keys are plain object, we assume we have multi-type mappings | ||
* - If there are more than two types, return "as is" as the UI does not support more than 1 type and will display a warning callout | ||
* - If there is only 1 type, validate the mappings definition and return it wrapped inside the the custom type | ||
*/ | ||
const areAllKeysValid = areAllObjectKeysValidParameters(unparsed); | ||
const areAllValuesPlainObjects = Object.values(unparsed).every(isPlainObject); | ||
const areAllValuesObjKeysValidParameterName = | ||
areAllValuesPlainObjects && Object.values(unparsed).every(areAllObjectKeysValidParameters); | ||
|
||
if (!areAllKeysValid && areAllValuesPlainObjects) { | ||
hasCustomType = true; | ||
isMultiTypeMappings = Object.keys(unparsed).length > 1; | ||
} | ||
// If all root level keys are *valid* parameters BUT they are all plain object which *also* has ALL valid config parameter | ||
// we can assume that they are custom-type whose name matches a mappings definition parameter. | ||
// This takes care of the case where a custom type would be "dynamic" for example which is a mappings configuration param. | ||
else if (areAllKeysValid && areAllValuesPlainObjects && areAllValuesObjKeysValidParameterName) { | ||
hasCustomType = true; | ||
isMultiTypeMappings = Object.keys(unparsed).length > 1; | ||
} | ||
|
||
if (hasCustomType) { | ||
customType = Object.keys(unparsed)[0]; | ||
} | ||
|
||
return { isMultiTypeMappings, customType }; | ||
}; | ||
|
||
const loadJson = () => { | ||
if (jsonContent.current === undefined) { | ||
// No changes have been made in the JSON, this is probably a "reset()" for the user | ||
|
@@ -159,14 +201,41 @@ export const LoadMappingsProvider = ({ onJson, children }: Props) => { | |
if (isValidJson) { | ||
// Parse and validate the JSON to make sure it won't break the UI | ||
const unparsed = jsonContent.current.data.format(); | ||
const { value: parsed, errors } = validateMappings(unparsed); | ||
|
||
if (Object.keys(unparsed).length === 0) { | ||
// Empty object...exit early | ||
onJson(unparsed); | ||
closeModal(); | ||
return; | ||
} | ||
|
||
let mappingsToValidate = unparsed; | ||
const { isMultiTypeMappings, customType } = getMappingsMetadata(unparsed); | ||
|
||
if (isMultiTypeMappings) { | ||
// Exit early, the UI will show a warning | ||
onJson(unparsed); | ||
closeModal(); | ||
return; | ||
} | ||
|
||
// Custom type can't be "properties", ES will not treat it as such | ||
// as it is reserved for fields definition | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ Great comments! |
||
if (customType !== undefined && customType !== 'properties') { | ||
mappingsToValidate = unparsed[customType]; | ||
} | ||
|
||
const { value: parsed, errors } = validateMappings(mappingsToValidate); | ||
|
||
// Wrap the mappings definition with custom type if one was provided. | ||
const parsedWithType = customType !== undefined ? { [customType]: parsed } : parsed; | ||
|
||
if (errors) { | ||
setState({ isModalOpen: true, json: { unparsed, parsed }, errors }); | ||
setState({ isModalOpen: true, json: { unparsed, parsed: parsedWithType }, errors }); | ||
return; | ||
} | ||
|
||
onJson(parsed); | ||
onJson(parsedWithType); | ||
closeModal(); | ||
} | ||
}; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: In the case of
isMultiTypeMappings === true
, thencustomType
loses meaning because it's not the only custom type. Maybe we should change the condition tohasCustomType && !isMultiTypeMappings
so thatcustomType
is leftundefined
if there are multiple type mappings?