[Controls] Fix error thrown on numeric options list (elastic#188789)

## Summary This PR makes the suggestions returned from the options list route more type safe by ensuring that strings are **always** returned - previously, it sometimes returned numbers, which was inconsistent with our defined types. Unfortunately, this messed with the field formatter for date fields specifically - so, to get around this, I've had to convert date fields back to a number specifically for the formatter **only**. This resolves the error that was getting thrown for numeric options list controls, which was happening because we were returning **numbers** from the suggestions route rather than strings and some recent changes to the `EuiSelectable` component require strings: **Before:** https://github.com/user-attachments/assets/0e723e2f-e8f0-4466-b857-8164088cd1e7 **After** https://github.com/user-attachments/assets/d9b138b9-de27-4e14-8c85-0ce4bfde16ce ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]>
PhilippeOberti · Jul 29, 2024 · 28b6179 · 28b6179
1 parent 5859c69
commit 28b6179
Show file tree

Hide file tree

Showing 21 changed files with 305 additions and 115 deletions.
diff --git a/src/plugins/controls/common/options_list/options_list_selections.ts b/src/plugins/controls/common/options_list/options_list_selections.ts
@@ -0,0 +1,16 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { FieldSpec } from '@kbn/data-views-plugin/common';
+
+export type OptionsListSelection = string | number;
+
+export const getSelectionAsFieldType = (field: FieldSpec, key: string): OptionsListSelection => {
+  const storeAsNumber = field.type === 'number' || field.type === 'date';
+  return storeAsNumber ? +key : key;
+};
diff --git a/src/plugins/controls/common/options_list/types.ts b/src/plugins/controls/common/options_list/types.ts
@@ -10,6 +10,7 @@ import { DataView, FieldSpec, RuntimeFieldSpec } from '@kbn/data-views-plugin/co
 import type { BoolQuery, Filter, Query, TimeRange } from '@kbn/es-query';
 
 import type { DataControlInput } from '../types';
+import { OptionsListSelection } from './options_list_selections';
 import { OptionsListSearchTechnique } from './suggestions_searching';
 import type { OptionsListSortingType } from './suggestions_sorting';
 
@@ -18,7 +19,7 @@ export const OPTIONS_LIST_CONTROL = 'optionsListControl'; // TODO: Replace with
 export interface OptionsListEmbeddableInput extends DataControlInput {
   searchTechnique?: OptionsListSearchTechnique;
   sort?: OptionsListSortingType;
-  selectedOptions?: string[];
+  selectedOptions?: OptionsListSelection[];
   existsSelected?: boolean;
   runPastTimeout?: boolean;
   singleSelect?: boolean;
@@ -30,15 +31,15 @@ export interface OptionsListEmbeddableInput extends DataControlInput {
   exclude?: boolean;
 }
 
-export type OptionsListSuggestions = Array<{ value: string; docCount?: number }>;
+export type OptionsListSuggestions = Array<{ value: OptionsListSelection; docCount?: number }>;
 
 /**
  * The Options list response is returned from the serverside Options List route.
  */
 export interface OptionsListSuccessResponse {
   suggestions: OptionsListSuggestions;
   totalCardinality?: number; // total cardinality will be undefined when `useExpensiveQueries` is `false`
-  invalidSelections?: string[];
+  invalidSelections?: OptionsListSelection[];
 }
 
 /**
@@ -61,12 +62,9 @@ export type OptionsListResponse = OptionsListSuccessResponse | OptionsListFailur
  */
 export type OptionsListRequest = Omit<
   OptionsListRequestBody,
-  'filters' | 'fieldName' | 'fieldSpec' | 'textFieldName'
+  'filters' | 'fieldName' | 'fieldSpec'
 > & {
-  searchTechnique?: OptionsListSearchTechnique;
-  allowExpensiveQueries: boolean;
   timeRange?: TimeRange;
-  runPastTimeout?: boolean;
   dataView: DataView;
   filters?: Filter[];
   field: FieldSpec;
@@ -76,16 +74,16 @@ export type OptionsListRequest = Omit<
 /**
  * The Options list request body is sent to the serverside Options List route and is used to create the ES query.
  */
-export interface OptionsListRequestBody {
+export interface OptionsListRequestBody
+  extends Pick<
+    OptionsListEmbeddableInput,
+    'fieldName' | 'searchTechnique' | 'sort' | 'selectedOptions'
+  > {
   runtimeFieldMap?: Record<string, RuntimeFieldSpec>;
-  searchTechnique?: OptionsListSearchTechnique;
   allowExpensiveQueries: boolean;
-  sort?: OptionsListSortingType;
   filters?: Array<{ bool: BoolQuery }>;
-  selectedOptions?: Array<string | number>;
   runPastTimeout?: boolean;
   searchString?: string;
   fieldSpec?: FieldSpec;
-  fieldName: string;
   size: number;
 }
diff --git a/src/plugins/controls/public/hooks/use_field_formatter.ts b/src/plugins/controls/public/hooks/use_field_formatter.ts
@@ -5,8 +5,10 @@
  * in compliance with, at your election, the Elastic License 2.0 or the Server
  * Side Public License, v 1.
  */
-import { FieldSpec } from '@kbn/data-views-plugin/common';
 import { useEffect, useState } from 'react';
+
+import { FieldSpec } from '@kbn/data-views-plugin/common';
+
 import { pluginServices } from '../services';
 
 export const useFieldFormatter = ({
@@ -19,7 +21,7 @@ export const useFieldFormatter = ({
   const {
     dataViews: { get: getDataViewById },
   } = pluginServices.getServices();
-  const [fieldFormatter, setFieldFormatter] = useState(() => (toFormat: string) => toFormat);
+  const [fieldFormatter, setFieldFormatter] = useState(() => (toFormat: any) => String(toFormat));
 
   /**
    * derive field formatter from fieldSpec and dataViewId
@@ -32,7 +34,7 @@ export const useFieldFormatter = ({
       setFieldFormatter(
         () =>
           dataView?.getFormatterForField(fieldSpec).getConverterFor('text') ??
-          ((toFormat: string) => toFormat)
+          ((toFormat: any) => String(toFormat))
       );
     })();
   }, [fieldSpec, dataViewId, getDataViewById]);

diff --git a/src/plugins/controls/public/options_list/components/options_list_control.tsx b/src/plugins/controls/public/options_list/components/options_list_control.tsx
@@ -6,10 +6,10 @@
  * Side Public License, v 1.
  */
 
-import { Subject } from 'rxjs';
 import classNames from 'classnames';
 import { debounce, isEmpty } from 'lodash';
 import React, { useCallback, useEffect, useMemo, useState } from 'react';
+import { Subject } from 'rxjs';
 
 import {
   EuiFilterButton,
@@ -22,15 +22,16 @@ import {
   htmlIdGenerator,
 } from '@elastic/eui';
 
+import { OptionsListSelection } from '../../../common/options_list/options_list_selections';
+import { MIN_POPOVER_WIDTH } from '../../constants';
+import { ControlError } from '../../control_group/component/control_error_component';
+import { useFieldFormatter } from '../../hooks/use_field_formatter';
+import { useOptionsList } from '../embeddable/options_list_embeddable';
 import { MAX_OPTIONS_LIST_REQUEST_SIZE } from '../types';
-import { OptionsListStrings } from './options_list_strings';
 import { OptionsListPopover } from './options_list_popover';
-import { useOptionsList } from '../embeddable/options_list_embeddable';
+import { OptionsListStrings } from './options_list_strings';
 
 import './options_list.scss';
-import { ControlError } from '../../control_group/component/control_error_component';
-import { MIN_POPOVER_WIDTH } from '../../constants';
-import { useFieldFormatter } from '../../hooks/use_field_formatter';
 
 export const OptionsListControl = ({
   typeaheadSubject,
@@ -128,13 +129,14 @@ export const OptionsListControl = ({
               ) : (
                 <>
                   {selectedOptions?.length
-                    ? selectedOptions.map((value: string, i, { length }) => {
+                    ? selectedOptions.map((value: OptionsListSelection, i, { length }) => {
                         const text = `${fieldFormatter(value)}${
                           i + 1 === length ? '' : delimiter
                         } `;
                         const isInvalid = invalidSelections?.includes(value);
                         return (
                           <span
+                            key={text} // each item must have a unique key to prevent warning
                             className={`optionsList__filter ${
                               isInvalid ? 'optionsList__filterInvalid' : ''
                             }`}

diff --git a/src/plugins/controls/public/options_list/components/options_list_popover.test.tsx b/src/plugins/controls/public/options_list/components/options_list_popover.test.tsx
@@ -7,16 +7,17 @@
  */
 
 import React from 'react';
-import { render, RenderResult, within } from '@testing-library/react';
-import userEvent from '@testing-library/user-event';
 
 import { FieldSpec } from '@kbn/data-views-plugin/common';
+import { stubDataView } from '@kbn/data-views-plugin/common/data_view.stub';
+import { render, RenderResult, within } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 
-import { pluginServices } from '../../services';
-import { mockOptionsListEmbeddable } from '../../../common/mocks';
 import { ControlOutput, OptionsListEmbeddableInput } from '../..';
-import { OptionsListComponentState, OptionsListReduxState } from '../types';
+import { mockOptionsListEmbeddable } from '../../../common/mocks';
+import { pluginServices } from '../../services';
 import { OptionsListEmbeddableContext } from '../embeddable/options_list_embeddable';
+import { OptionsListComponentState, OptionsListReduxState } from '../types';
 import { OptionsListPopover, OptionsListPopoverProps } from './options_list_popover';
 
 describe('Options list popover', () => {
@@ -168,6 +169,7 @@ describe('Options list popover', () => {
     test('clicking another option unselects "Exists"', async () => {
       const popover = await mountComponent({
         explicitInput: { existsSelected: true },
+        componentState: { field: { type: 'string' } as FieldSpec },
       });
       const woofOption = popover.getByTestId('optionsList-control-selection-woof');
       userEvent.click(woofOption);
@@ -185,6 +187,7 @@ describe('Options list popover', () => {
       const selections = ['woof', 'bark'];
       const popover = await mountComponent({
         explicitInput: { existsSelected: false, selectedOptions: selections },
+        componentState: { field: { type: 'number' } as FieldSpec },
       });
       const existsOption = popover.getByTestId('optionsList-control-selection-exists');
       let availableOptionsDiv = popover.getByTestId('optionsList-control-available-options');
@@ -363,4 +366,56 @@ describe('Options list popover', () => {
       });
     });
   });
+
+  describe('field formatter', () => {
+    const mockedFormatter = jest.fn().mockImplementation((value: unknown) => `formatted:${value}`);
+
+    beforeAll(() => {
+      stubDataView.getFormatterForField = jest.fn().mockReturnValue({
+        getConverterFor: () => mockedFormatter,
+      });
+      pluginServices.getServices().dataViews.get = jest.fn().mockResolvedValue(stubDataView);
+    });
+
+    afterEach(() => {
+      mockedFormatter.mockClear();
+    });
+
+    test('uses field formatter on suggestions', async () => {
+      const popover = await mountComponent({
+        componentState: {
+          field: stubDataView.fields.getByName('bytes')?.toSpec(),
+          availableOptions: [
+            { value: 1000, docCount: 1 },
+            { value: 123456789, docCount: 4 },
+          ],
+        },
+      });
+
+      expect(mockedFormatter).toHaveBeenNthCalledWith(1, 1000);
+      expect(mockedFormatter).toHaveBeenNthCalledWith(2, 123456789);
+      const options = await popover.findAllByRole('option');
+      expect(options[0].textContent).toEqual('Exists');
+      expect(
+        options[1].getElementsByClassName('euiSelectableListItem__text')[0].textContent
+      ).toEqual('formatted:1000');
+      expect(
+        options[2].getElementsByClassName('euiSelectableListItem__text')[0].textContent
+      ).toEqual('formatted:123456789');
+    });
+
+    test('converts string to number for date field', async () => {
+      await mountComponent({
+        componentState: {
+          field: stubDataView.fields.getByName('@timestamp')?.toSpec(),
+          availableOptions: [
+            { value: 1721283696000, docCount: 1 },
+            { value: 1721295533000, docCount: 2 },
+          ],
+        },
+      });
+      expect(mockedFormatter).toHaveBeenNthCalledWith(1, 1721283696000);
+      expect(mockedFormatter).toHaveBeenNthCalledWith(2, 1721295533000);
+    });
+  });
 });
diff --git a/...ugins/controls/public/options_list/components/options_list_popover_invalid_selections.tsx b/...ugins/controls/public/options_list/components/options_list_popover_invalid_selections.tsx
@@ -19,6 +19,7 @@ import {
   EuiTitle,
 } from '@elastic/eui';
 
+import { getSelectionAsFieldType } from '../../../common/options_list/options_list_selections';
 import { useFieldFormatter } from '../../hooks/use_field_formatter';
 import { useOptionsList } from '../embeddable/options_list_embeddable';
 import { OptionsListStrings } from './options_list_strings';
@@ -39,7 +40,7 @@ export const OptionsListPopoverInvalidSelections = () => {
     /* This useEffect makes selectableOptions responsive to unchecking options */
     const options: EuiSelectableOption[] = (invalidSelections ?? []).map((key) => {
       return {
-        key,
+        key: String(key),
         label: fieldFormatter(key),
         checked: 'on',
         className: 'optionsList__selectionInvalid',
@@ -91,8 +92,15 @@ export const OptionsListPopoverInvalidSelections = () => {
         options={selectableOptions}
         listProps={{ onFocusBadge: false, isVirtualized: false }}
         onChange={(newSuggestions, _, changedOption) => {
+          if (!fieldSpec || !changedOption.key) {
+            // this should never happen, but early return for type safety
+            // eslint-disable-next-line no-console
+            console.warn(OptionsListStrings.popover.getInvalidSelectionMessage());
+            return;
+          }
           setSelectableOptions(newSuggestions);
-          optionsList.dispatch.deselectOption(changedOption.key ?? changedOption.label);
+          const key = getSelectionAsFieldType(fieldSpec, changedOption.key);
+          optionsList.dispatch.deselectOption(key);
         }}
       >
         {(list) => list}

diff --git a/src/plugins/controls/public/options_list/components/options_list_popover_suggestions.tsx b/src/plugins/controls/public/options_list/components/options_list_popover_suggestions.tsx
@@ -8,16 +8,20 @@
 
 import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';
 
-import { euiThemeVars } from '@kbn/ui-theme';
 import { EuiHighlight, EuiSelectable } from '@elastic/eui';
 import { EuiSelectableOption } from '@elastic/eui/src/components/selectable/selectable_option';
+import { euiThemeVars } from '@kbn/ui-theme';
 
-import { MAX_OPTIONS_LIST_REQUEST_SIZE } from '../types';
-import { OptionsListStrings } from './options_list_strings';
+import {
+  getSelectionAsFieldType,
+  OptionsListSelection,
+} from '../../../common/options_list/options_list_selections';
+import { useFieldFormatter } from '../../hooks/use_field_formatter';
 import { useOptionsList } from '../embeddable/options_list_embeddable';
+import { MAX_OPTIONS_LIST_REQUEST_SIZE } from '../types';
 import { OptionsListPopoverEmptyMessage } from './options_list_popover_empty_message';
 import { OptionsListPopoverSuggestionBadge } from './options_list_popover_suggestion_badge';
-import { useFieldFormatter } from '../../hooks/use_field_formatter';
+import { OptionsListStrings } from './options_list_strings';
 
 interface OptionsListPopoverSuggestionsProps {
   showOnlySelected: boolean;
@@ -64,9 +68,12 @@ export const OptionsListPopoverSuggestions = ({
   );
 
   // track selectedOptions and invalidSelections in sets for more efficient lookup
-  const selectedOptionsSet = useMemo(() => new Set<string>(selectedOptions), [selectedOptions]);
+  const selectedOptionsSet = useMemo(
+    () => new Set<OptionsListSelection>(selectedOptions),
+    [selectedOptions]
+  );
   const invalidSelectionsSet = useMemo(
-    () => new Set<string>(invalidSelections),
+    () => new Set<OptionsListSelection>(invalidSelections),
     [invalidSelections]
   );
   const suggestions = useMemo(() => {
@@ -95,8 +102,8 @@ export const OptionsListPopoverSuggestions = ({
       }
 
       return {
-        key: suggestion.value,
-        label: fieldFormatter(suggestion.value) ?? suggestion.value,
+        key: String(suggestion.value),
+        label: fieldFormatter(suggestion.value) ?? String(suggestion.value),
         checked: selectedOptionsSet?.has(suggestion.value) ? 'on' : undefined,
         'data-test-subj': `optionsList-control-selection-${suggestion.value}`,
         className:
@@ -191,12 +198,21 @@ export const OptionsListPopoverSuggestions = ({
           )}
           emptyMessage={<OptionsListPopoverEmptyMessage showOnlySelected={showOnlySelected} />}
           onChange={(newSuggestions, _, changedOption) => {
-            const key = changedOption.key ?? changedOption.label;
+            if (!fieldSpec || !changedOption.key) {
+              // this should never happen, but early return for type safety
+              // eslint-disable-next-line no-console
+              console.warn(OptionsListStrings.popover.getInvalidSelectionMessage());
+              return;
+            }
             setSelectableOptions(newSuggestions);
-            // the order of these checks matters, so be careful if rearranging them
-            if (key === 'exists-option') {
+            if (changedOption.key === 'exists-option') {
               optionsList.dispatch.selectExists(!Boolean(existsSelected));
-            } else if (showOnlySelected || selectedOptionsSet.has(key)) {
+              return;
+            }
+
+            const key = getSelectionAsFieldType(fieldSpec, changedOption.key);
+            // the order of these checks matters, so be careful if rearranging them
+            if (showOnlySelected || selectedOptionsSet.has(key)) {
               optionsList.dispatch.deselectOption(key);
             } else if (singleSelect) {
               optionsList.dispatch.replaceSelection(key);

diff --git a/src/plugins/controls/public/options_list/components/options_list_strings.ts b/src/plugins/controls/public/options_list/components/options_list_strings.ts
@@ -147,6 +147,10 @@ export const OptionsListStrings = {
       i18n.translate('controls.optionsList.popover.selectionsEmpty', {
         defaultMessage: 'You have no selections',
       }),
+    getInvalidSelectionMessage: () =>
+      i18n.translate('controls.optionsList.popover.selectionError', {
+        defaultMessage: 'There was an error when making your selection',
+      }),
     getInvalidSearchMessage: (fieldType: string) => {
       switch (fieldType) {
         case 'ip': {