From 8cca8b9c8c56f21d50301f231e321e9382dbf702 Mon Sep 17 00:00:00 2001 From: Daniil Suleiman <31325372+sulemanof@users.noreply.github.com> Date: Wed, 5 Feb 2020 19:20:19 +0300 Subject: [PATCH] [Vis Editor] Fix fields filtering in top hit agg (#56367) * Fix top hit agg * Add unit tests * Fix fields filtering in table vis * Resolve merge conflicts Co-authored-by: Elastic Machine --- .../data/public/search/aggs/agg_config.ts | 7 +- .../search/aggs/metrics/metric_agg_type.ts | 3 +- .../public/search/aggs/metrics/top_hit.ts | 17 ++--- .../search/aggs/param_types/field.test.ts | 65 ++++++++++++++++++- .../public/search/aggs/param_types/field.ts | 28 ++++---- .../components/agg_params_helper.test.ts | 10 +-- .../public/components/agg_params_helper.ts | 4 +- .../public/legacy_imports.ts | 1 - .../data/public/index_patterns/index.ts | 2 +- 9 files changed, 98 insertions(+), 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 index 769347a26c34c..ba7faf8c34b59 100644 --- 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 index e7d286c187ef8..3bae7b92618dc 100644 --- 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,6 +24,7 @@ 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; @@ -31,7 +32,7 @@ export interface IMetricAggConfig extends AggConfig { export interface MetricAggParam extends AggParamType { - 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 index 81bd14ded75b0..3112d882bb87e 100644 --- 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 index d0fa711d89c70..fa88754ac60b9 100644 --- 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 index c41c159ad0f78..9a204bb151e2d 100644 --- 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) { @@ -44,7 +47,7 @@ export class FieldParamType extends BaseParamType { this.onlyAggregatable = config.onlyAggregatable !== false; if (!config.write) { - this.write = (aggConfig: AggConfig, output: Record) => { + this.write = (aggConfig: IAggConfig, output: Record) => { 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 index b9fb81fccd32c..f3bee80baa1ba 100644 --- 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) => - 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 index 25aa21fc83b31..124c41a50c0df 100644 --- 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 index a700995ec596b..b7fd6b1e9ebb6 100644 --- 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 index 3d902ebbb7c23..ecddd893d1a54 100644 --- 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 {