From b4b59d9e29ed5c26087673c0add6a4dd65f85abf Mon Sep 17 00:00:00 2001 From: Alexey Antonov Date: Wed, 11 Aug 2021 13:19:45 +0300 Subject: [PATCH] [Lens] fix do not submit invalid query in filtered metric (#107542) (#108144) * [Lens] Do not submit invalid query in filtered metric Closes: #95611 * fix CI * fix PR comments * fix PR comments * fix PR comment * move closePopover to useCallback * add filter validation to utils/isColumnInvalid Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../dimension_panel/dimension_panel.test.tsx | 23 +++-- .../dimension_panel/filtering.tsx | 91 +++++++++++++------ .../definitions/filters/filters.tsx | 33 ++++--- .../public/indexpattern_datasource/utils.ts | 9 +- 4 files changed, 105 insertions(+), 51 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx index 5318255792641..b6d3a230d06f5 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx @@ -7,7 +7,7 @@ import { ReactWrapper, ShallowWrapper } from 'enzyme'; import 'jest-canvas-mock'; -import React, { ChangeEvent, MouseEvent, ReactElement } from 'react'; +import React, { ChangeEvent, MouseEvent } from 'react'; import { act } from 'react-dom/test-utils'; import { EuiComboBox, @@ -16,7 +16,6 @@ import { EuiRange, EuiSelect, EuiButtonIcon, - EuiPopover, } from '@elastic/eui'; import { DataPublicPluginStart } from '../../../../../../src/plugins/data/public'; import { @@ -33,7 +32,7 @@ import { documentField } from '../document_field'; import { OperationMetadata } from '../../types'; import { DateHistogramIndexPatternColumn } from '../operations/definitions/date_histogram'; import { getFieldByNameFactory } from '../pure_helpers'; -import { Filtering } from './filtering'; +import { Filtering, setFilter } from './filtering'; import { TimeShift } from './time_shift'; import { DimensionEditor } from './dimension_editor'; import { AdvancedOptions } from './advanced_options'; @@ -1541,9 +1540,13 @@ describe('IndexPatternDimensionEditorPanel', () => { {...getProps({ filter: { language: 'kuery', query: 'a: b' } })} /> ); + expect( - (wrapper.find(Filtering).find(EuiPopover).prop('children') as ReactElement).props.value - ).toEqual({ language: 'kuery', query: 'a: b' }); + wrapper + .find(Filtering) + .find('button[data-test-subj="indexPattern-filters-existingFilterTrigger"]') + .text() + ).toBe(`a: b`); }); it('should allow to set filter initially', () => { @@ -1609,11 +1612,15 @@ describe('IndexPatternDimensionEditorPanel', () => { const props = getProps({ filter: { language: 'kuery', query: 'a: b' }, }); + wrapper = mount(); - (wrapper.find(Filtering).find(EuiPopover).prop('children') as ReactElement).props.onChange({ - language: 'kuery', - query: 'c: d', + + act(() => { + const { updateLayer, columnId, layer } = wrapper.find(Filtering).props(); + + updateLayer(setFilter(columnId, layer, { language: 'kuery', query: 'c: d' })); }); + expect(setState.mock.calls[0]).toEqual([expect.any(Function), { isDimensionComplete: true }]); expect(setState.mock.calls[0][0](props.state)).toEqual({ ...props.state, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/filtering.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/filtering.tsx index 68705ebf2d157..ddd9718839649 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/filtering.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/filtering.tsx @@ -4,16 +4,28 @@ * 2.0; you may not use this file except in compliance with the Elastic License * 2.0. */ - -import { EuiButtonIcon, EuiLink, EuiPanel, EuiPopover } from '@elastic/eui'; -import { EuiFormRow, EuiFlexItem, EuiFlexGroup } from '@elastic/eui'; +import React, { useState, useEffect, useCallback } from 'react'; import { i18n } from '@kbn/i18n'; -import React, { useState } from 'react'; -import { Query } from 'src/plugins/data/public'; +import { isEqual } from 'lodash'; +import { + EuiButtonIcon, + EuiLink, + EuiPanel, + EuiPopover, + EuiFormRow, + EuiFlexItem, + EuiFlexGroup, + EuiPopoverProps, +} from '@elastic/eui'; +import type { Query } from 'src/plugins/data/public'; import { IndexPatternColumn, operationDefinitionMap } from '../operations'; -import { isQueryValid } from '../operations/definitions/filters'; +import { validateQuery } from '../operations/definitions/filters'; import { QueryInput } from '../query_input'; -import { IndexPattern, IndexPatternLayer } from '../types'; +import type { IndexPattern, IndexPatternLayer } from '../types'; + +const filterByLabel = i18n.translate('xpack.lens.indexPattern.filterBy.label', { + defaultMessage: 'Filter by', +}); // to do: get the language from uiSettings export const defaultFilter: Query = { @@ -49,29 +61,49 @@ export function Filtering({ updateLayer: (newLayer: IndexPatternLayer) => void; isInitiallyOpen: boolean; }) { + const inputFilter = selectedColumn.filter; + const [queryInput, setQueryInput] = useState(inputFilter ?? defaultFilter); const [filterPopoverOpen, setFilterPopoverOpen] = useState(isInitiallyOpen); + + useEffect(() => { + const { isValid } = validateQuery(queryInput, indexPattern); + + if (isValid && !isEqual(inputFilter, queryInput)) { + updateLayer(setFilter(columnId, layer, queryInput)); + } + }, [columnId, layer, queryInput, indexPattern, updateLayer, inputFilter]); + + const onClosePopup: EuiPopoverProps['closePopover'] = useCallback(() => { + setFilterPopoverOpen(false); + if (inputFilter) { + setQueryInput(inputFilter); + } + }, [inputFilter]); + const selectedOperation = operationDefinitionMap[selectedColumn.operationType]; - if (!selectedOperation.filterable || !selectedColumn.filter) { + + if (!selectedOperation.filterable || !inputFilter) { return null; } - const isInvalid = !isQueryValid(selectedColumn.filter, indexPattern); + const { isValid: isInputFilterValid } = validateQuery(inputFilter, indexPattern); + const { isValid: isQueryInputValid, error: queryInputError } = validateQuery( + queryInput, + indexPattern + ); return ( { - setFilterPopoverOpen(false); - }} + closePopover={onClosePopup} anchorClassName="eui-fullWidth" panelClassName="lnsIndexPatternDimensionEditor__filtersEditor" button={ @@ -85,12 +117,12 @@ export function Filtering({ onClick={() => { setFilterPopoverOpen(!filterPopoverOpen); }} - color={isInvalid ? 'danger' : 'text'} + color={isInputFilterValid ? 'text' : 'danger'} title={i18n.translate('xpack.lens.indexPattern.filterBy.clickToEdit', { defaultMessage: 'Click to edit', })} > - {selectedColumn.filter.query || + {inputFilter.query || i18n.translate('xpack.lens.indexPattern.filterBy.emptyFilterQuery', { defaultMessage: '(empty)', })} @@ -113,16 +145,21 @@ export function Filtering({ } > - { - updateLayer(setFilter(columnId, layer, newQuery)); - }} - isInvalid={false} - onSubmit={() => {}} - /> + + {}} + /> + diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx index 7f718498bd95b..84319d8f8e433 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx @@ -6,22 +6,17 @@ */ import './filters.scss'; - import React, { MouseEventHandler, useState } from 'react'; +import { fromKueryExpression, luceneStringToDsl, toElasticsearchQuery } from '@kbn/es-query'; import { omit } from 'lodash'; import { i18n } from '@kbn/i18n'; import { EuiFormRow, EuiLink, htmlIdGenerator } from '@elastic/eui'; import { updateColumnParam } from '../../layer_helpers'; -import { OperationDefinition } from '../index'; -import { BaseIndexPatternColumn } from '../column_types'; +import type { OperationDefinition } from '../index'; +import type { BaseIndexPatternColumn } from '../column_types'; import { FilterPopover } from './filter_popover'; -import { IndexPattern } from '../../../types'; -import { - AggFunctionsMapping, - Query, - esKuery, - esQuery, -} from '../../../../../../../../src/plugins/data/public'; +import type { IndexPattern } from '../../../types'; +import type { AggFunctionsMapping, Query } from '../../../../../../../../src/plugins/data/public'; import { queryFilterToAst } from '../../../../../../../../src/plugins/data/common'; import { buildExpressionFunction } from '../../../../../../../../src/plugins/expressions/public'; import { NewBucketButton, DragDropBuckets, DraggableBucketContainer } from '../shared_components'; @@ -58,19 +53,27 @@ const defaultFilter: Filter = { label: '', }; -export const isQueryValid = (input: Query, indexPattern: IndexPattern) => { +export const validateQuery = (input: Query, indexPattern: IndexPattern) => { + let isValid = true; + let error: string | undefined; + try { if (input.language === 'kuery') { - esKuery.toElasticsearchQuery(esKuery.fromKueryExpression(input.query), indexPattern); + toElasticsearchQuery(fromKueryExpression(input.query), indexPattern); } else { - esQuery.luceneStringToDsl(input.query); + luceneStringToDsl(input.query); } - return true; } catch (e) { - return false; + isValid = false; + error = e.message; } + + return { isValid, error }; }; +export const isQueryValid = (input: Query, indexPattern: IndexPattern) => + validateQuery(input, indexPattern).isValid; + export interface FiltersIndexPatternColumn extends BaseIndexPatternColumn { operationType: typeof OPERATION_NAME; params: { diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts index a9e24c70ab8ac..7d225d730a757 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts @@ -15,6 +15,7 @@ import type { import { operationDefinitionMap, IndexPatternColumn } from './operations'; import { getInvalidFieldMessage } from './operations/definitions/helpers'; +import { isQueryValid } from './operations/definitions/filters'; /** * Normalizes the specified operation type. (e.g. document operations @@ -68,7 +69,13 @@ export function isColumnInvalid( operationDefinitionMap ); - return (operationErrorMessages && operationErrorMessages.length > 0) || referencesHaveErrors; + const filterHasError = column.filter ? !isQueryValid(column.filter, indexPattern) : false; + + return ( + (operationErrorMessages && operationErrorMessages.length > 0) || + referencesHaveErrors || + filterHasError + ); } function getReferencesErrors(