[Vis Editor] Fix fields filtering in top hit agg (#56367) (#56871)

* Fix top hit agg * Add unit tests * Fix fields filtering in table vis * Resolve merge conflicts Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
elastic · Feb 6, 2020 · c9ede92 · c9ede92
1 parent ce0c341
commit c9ede92
Show file tree

Hide file tree

Showing 9 changed files with 98 additions and 39 deletions.
diff --git a/src/legacy/core_plugins/data/public/search/aggs/agg_config.ts b/src/legacy/core_plugins/data/public/search/aggs/agg_config.ts
@@ -58,6 +58,11 @@ const unknownSchema: Schema = {
   defaults: {},
   editor: false,
   group: AggGroupNames.Metrics,
+  aggSettings: {
+    top_hits: {
+      allowStrings: true,
+    },
+  },
 };
 
 const getTypeFromRegistry = (type: string): IAggType => {
@@ -438,7 +443,7 @@ export class AggConfig {
 
     if (fieldParam) {
       // @ts-ignore
-      availableFields = fieldParam.getAvailableFields(this.getIndexPattern().fields);
+      availableFields = fieldParam.getAvailableFields(this);
     }
 
     // clear out the previous params except for a few special ones

diff --git a/src/legacy/core_plugins/data/public/search/aggs/metrics/metric_agg_type.ts b/src/legacy/core_plugins/data/public/search/aggs/metrics/metric_agg_type.ts
@@ -24,14 +24,15 @@ import { AggParamType } from '../param_types/agg';
 import { AggConfig } from '../agg_config';
 import { METRIC_TYPES } from './metric_agg_types';
 import { KBN_FIELD_TYPES } from '../../../../../../../plugins/data/public';
+import { FilterFieldTypes } from '../param_types/field';
 
 export interface IMetricAggConfig extends AggConfig {
   type: InstanceType<typeof MetricAggType>;
 }
 
 export interface MetricAggParam<TMetricAggConfig extends AggConfig>
   extends AggParamType<TMetricAggConfig> {
-  filterFieldTypes?: KBN_FIELD_TYPES | KBN_FIELD_TYPES[] | '*';
+  filterFieldTypes?: FilterFieldTypes;
   onlyAggregatable?: boolean;
 }
 

diff --git a/src/legacy/core_plugins/data/public/search/aggs/metrics/top_hit.ts b/src/legacy/core_plugins/data/public/search/aggs/metrics/top_hit.ts
@@ -20,7 +20,6 @@
 import _ from 'lodash';
 import { i18n } from '@kbn/i18n';
 import { IMetricAggConfig, MetricAggType } from './metric_agg_type';
-import { aggTypeFieldFilters } from '../param_types/filter';
 import { METRIC_TYPES } from './metric_agg_types';
 import { KBN_FIELD_TYPES } from '../../../../../../../plugins/data/public';
 
@@ -33,17 +32,6 @@ const isNumericFieldSelected = (agg: IMetricAggConfig) => {
   return field && field.type && field.type === KBN_FIELD_TYPES.NUMBER;
 };
 
-aggTypeFieldFilters.addFilter((field, aggConfig) => {
-  if (
-    aggConfig.type.name !== METRIC_TYPES.TOP_HITS ||
-    _.get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
-  ) {
-    return true;
-  }
-
-  return field.type === KBN_FIELD_TYPES.NUMBER;
-});
-
 export const topHitMetricAgg = new MetricAggType({
   name: METRIC_TYPES.TOP_HITS,
   title: i18n.translate('data.search.aggs.metrics.topHitTitle', {
@@ -75,7 +63,10 @@ export const topHitMetricAgg = new MetricAggType({
       name: 'field',
       type: 'field',
       onlyAggregatable: false,
-      filterFieldTypes: '*',
+      filterFieldTypes: (aggConfig: IMetricAggConfig) =>
+        _.get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
+          ? '*'
+          : KBN_FIELD_TYPES.NUMBER,
       write(agg, output) {
         const field = agg.getParam('field');
         output.params = {};

diff --git a/src/legacy/core_plugins/data/public/search/aggs/param_types/field.test.ts b/src/legacy/core_plugins/data/public/search/aggs/param_types/field.test.ts
@@ -17,9 +17,13 @@
  * under the License.
  */
 
+import { get } from 'lodash';
 import { BaseParamType } from './base';
 import { FieldParamType } from './field';
 import { ES_FIELD_TYPES, KBN_FIELD_TYPES } from '../../../../../../../plugins/data/public';
+import { IAggConfig } from '../agg_config';
+import { IMetricAggConfig } from '../metrics/metric_agg_type';
+import { Schema } from '../schemas';
 
 jest.mock('ui/new_platform');
 
@@ -45,7 +49,11 @@ describe('Field', () => {
         searchable: true,
       },
     ],
-  } as any;
+  };
+
+  const agg = ({
+    getIndexPattern: jest.fn(() => indexPattern),
+  } as unknown) as IAggConfig;
 
   describe('constructor', () => {
     it('it is an instance of BaseParamType', () => {
@@ -65,7 +73,7 @@ describe('Field', () => {
         type: 'field',
       });
 
-      const fields = aggParam.getAvailableFields(indexPattern.fields);
+      const fields = aggParam.getAvailableFields(agg);
 
       expect(fields.length).toBe(1);
 
@@ -82,7 +90,58 @@ describe('Field', () => {
 
       aggParam.onlyAggregatable = false;
 
-      const fields = aggParam.getAvailableFields(indexPattern.fields);
+      const fields = aggParam.getAvailableFields(agg);
+
+      expect(fields.length).toBe(2);
+    });
+
+    it('should return all fields if filterFieldTypes was not specified', () => {
+      const aggParam = new FieldParamType({
+        name: 'field',
+        type: 'field',
+      });
+
+      indexPattern.fields[1].aggregatable = true;
+
+      const fields = aggParam.getAvailableFields(agg);
+
+      expect(fields.length).toBe(2);
+    });
+
+    it('should return only numeric fields if filterFieldTypes was specified as a function', () => {
+      const aggParam = new FieldParamType({
+        name: 'field',
+        type: 'field',
+        filterFieldTypes: (aggConfig: IMetricAggConfig) =>
+          get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
+            ? '*'
+            : KBN_FIELD_TYPES.NUMBER,
+      });
+      const fields = aggParam.getAvailableFields(agg);
+
+      expect(fields.length).toBe(1);
+      expect(fields[0].type).toBe(KBN_FIELD_TYPES.NUMBER);
+    });
+
+    it('should return all fields if filterFieldTypes was specified as a function and aggSettings allow string type fields', () => {
+      const aggParam = new FieldParamType({
+        name: 'field',
+        type: 'field',
+        filterFieldTypes: (aggConfig: IMetricAggConfig) =>
+          get(aggConfig.schema, 'aggSettings.top_hits.allowStrings', false)
+            ? '*'
+            : KBN_FIELD_TYPES.NUMBER,
+      });
+
+      agg.schema = {
+        aggSettings: {
+          top_hits: {
+            allowStrings: true,
+          },
+        },
+      } as Schema;
+
+      const fields = aggParam.getAvailableFields(agg);
 
       expect(fields.length).toBe(2);
     });

diff --git a/src/legacy/core_plugins/data/public/search/aggs/param_types/field.ts b/src/legacy/core_plugins/data/public/search/aggs/param_types/field.ts
@@ -17,24 +17,27 @@
  * under the License.
  */
 
-// @ts-ignore
 import { i18n } from '@kbn/i18n';
+import { isFunction } from 'lodash';
 import { npStart } from 'ui/new_platform';
-import { AggConfig } from '../agg_config';
+import { IAggConfig } from '../agg_config';
 import { SavedObjectNotFound } from '../../../../../../../plugins/kibana_utils/public';
 import { BaseParamType } from './base';
 import { propFilter } from '../filter';
-import { Field, IFieldList, isNestedField } from '../../../../../../../plugins/data/public';
+import { IMetricAggConfig } from '../metrics/metric_agg_type';
+import { Field, isNestedField, KBN_FIELD_TYPES } from '../../../../../../../plugins/data/public';
 
 const filterByType = propFilter('type');
 
+type FieldTypes = KBN_FIELD_TYPES | KBN_FIELD_TYPES[] | '*';
+export type FilterFieldTypes = ((aggConfig: IMetricAggConfig) => FieldTypes) | FieldTypes;
 // TODO need to make a more explicit interface for this
 export type IFieldParamType = FieldParamType;
 
 export class FieldParamType extends BaseParamType {
   required = true;
   scriptable = true;
-  filterFieldTypes: string;
+  filterFieldTypes: FilterFieldTypes;
   onlyAggregatable: boolean;
 
   constructor(config: Record<string, any>) {
@@ -44,7 +47,7 @@ export class FieldParamType extends BaseParamType {
     this.onlyAggregatable = config.onlyAggregatable !== false;
 
     if (!config.write) {
-      this.write = (aggConfig: AggConfig, output: Record<string, any>) => {
+      this.write = (aggConfig: IAggConfig, output: Record<string, any>) => {
         const field = aggConfig.getField();
 
         if (!field) {
@@ -73,7 +76,7 @@ export class FieldParamType extends BaseParamType {
       return field.name;
     };
 
-    this.deserialize = (fieldName: string, aggConfig?: AggConfig) => {
+    this.deserialize = (fieldName: string, aggConfig?: IAggConfig) => {
       if (!aggConfig) {
         throw new Error('aggConfig was not provided to FieldParamType deserialize function');
       }
@@ -84,9 +87,7 @@ export class FieldParamType extends BaseParamType {
       }
 
       // @ts-ignore
-      const validField = this.getAvailableFields(aggConfig.getIndexPattern().fields).find(
-        (f: any) => f.name === fieldName
-      );
+      const validField = this.getAvailableFields(aggConfig).find((f: any) => f.name === fieldName);
       if (!validField) {
         npStart.core.notifications.toasts.addDanger(
           i18n.translate(
@@ -109,7 +110,8 @@ export class FieldParamType extends BaseParamType {
   /**
    * filter the fields to the available ones
    */
-  getAvailableFields = (fields: IFieldList) => {
+  getAvailableFields = (aggConfig: IAggConfig) => {
+    const fields = aggConfig.getIndexPattern().fields;
     const filteredFields = fields.filter((field: Field) => {
       const { onlyAggregatable, scriptable, filterFieldTypes } = this;
 
@@ -120,8 +122,10 @@ export class FieldParamType extends BaseParamType {
         return false;
       }
 
-      if (!filterFieldTypes) {
-        return true;
+      if (isFunction(filterFieldTypes)) {
+        const filter = filterFieldTypes(aggConfig as IMetricAggConfig);
+
+        return filterByType([field], filter).length !== 0;
       }
 
       return filterByType([field], filterFieldTypes).length !== 0;

diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts
@@ -17,9 +17,9 @@
  * under the License.
  */
 
-import { IndexPattern, Field } from 'src/plugins/data/public';
+import { IndexPattern } from 'src/plugins/data/public';
 import { VisState } from 'src/legacy/core_plugins/visualizations/public';
-import { IAggConfig, IAggType, AggGroupNames, BUCKET_TYPES, IndexedArray } from '../legacy_imports';
+import { IAggConfig, IAggType, AggGroupNames, BUCKET_TYPES } from '../legacy_imports';
 import {
   getAggParamsToRender,
   getAggTypeOptions,
@@ -105,8 +105,10 @@ describe('DefaultEditorAggParams helpers', () => {
               name: 'field',
               type: 'field',
               filterFieldTypes,
-              getAvailableFields: jest.fn((fields: IndexedArray<Field>) =>
-                fields.filter(({ type }) => filterFieldTypes.includes(type))
+              getAvailableFields: jest.fn((aggConfig: IAggConfig) =>
+                aggConfig
+                  .getIndexPattern()
+                  .fields.filter(({ type }) => filterFieldTypes.includes(type))
               ),
             },
             {

diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts
@@ -73,9 +73,7 @@ function getAggParamsToRender({ agg, editorConfig, metricAggs, state }: ParamIns
     }
     // if field param exists, compute allowed fields
     if (param.type === 'field') {
-      const availableFields: Field[] = (param as IFieldParamType).getAvailableFields(
-        agg.getIndexPattern().fields
-      );
+      const availableFields: Field[] = (param as IFieldParamType).getAvailableFields(agg);
       fields = aggTypeFieldFilters.filter(availableFields, agg);
       indexedFields = groupAndSortBy(fields, 'type', 'name');
 

diff --git a/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts b/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts
@@ -49,7 +49,6 @@ export { AggParamOption } from 'ui/agg_types';
 export { CidrMask } from 'ui/agg_types';
 
 export { PersistedState } from 'ui/persisted_state';
-export { IndexedArray } from 'ui/indexed_array';
 export { getDocLink } from 'ui/documentation_links';
 export { documentationLinks } from 'ui/documentation_links/documentation_links';
 export { move } from 'ui/utils/collection';

diff --git a/src/plugins/data/public/index_patterns/index.ts b/src/plugins/data/public/index_patterns/index.ts
@@ -44,7 +44,7 @@ export const indexPatterns = {
   isDefault,
 };
 
-export { Field, FieldList, IFieldList } from './fields';
+export { Field, FieldList } from './fields';
 
 // TODO: figure out how to replace IndexPatterns in get_inner_angular.
 export {