[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 <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
elastic · Aug 11, 2021 · b4b59d9 · b4b59d9
1 parent 79e45f2
commit b4b59d9
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 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
@@ -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(<IndexPatternDimensionEditorComponent {...props} />);
-      (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
@@ -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 (
     <EuiFormRow
       display="columnCompressed"
+      label={filterByLabel}
       fullWidth
-      label={i18n.translate('xpack.lens.indexPattern.filterBy.label', {
-        defaultMessage: 'Filter by',
-      })}
+      isInvalid={!isInputFilterValid}
     >
       <EuiFlexGroup gutterSize="s" alignItems="center">
         <EuiFlexItem>
           <EuiPopover
             isOpen={filterPopoverOpen}
-            closePopover={() => {
-              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({
               </EuiPanel>
             }
           >
-            <QueryInput
-              indexPatternTitle={indexPattern.title}
-              data-test-subj="indexPattern-filter-by-input"
-              value={selectedColumn.filter || defaultFilter}
-              onChange={(newQuery) => {
-                updateLayer(setFilter(columnId, layer, newQuery));
-              }}
-              isInvalid={false}
-              onSubmit={() => {}}
-            />
+            <EuiFormRow
+              label={filterByLabel}
+              isInvalid={!isQueryInputValid}
+              error={queryInputError}
+              fullWidth={true}
+            >
+              <QueryInput
+                indexPatternTitle={indexPattern.title}
+                data-test-subj="indexPattern-filter-by-input"
+                value={queryInput}
+                onChange={setQueryInput}
+                isInvalid={!isQueryInputValid}
+                onSubmit={() => {}}
+              />
+            </EuiFormRow>
           </EuiPopover>
         </EuiFlexItem>
       </EuiFlexGroup>

diff --git a/...ck/plugins/lens/public/indexpattern_datasource/operations/definitions/filters/filters.tsx b/...ck/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
@@ -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(