Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Feb 5, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +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 {
AggConfig,
AggType,
AggGroupNames,
BUCKET_TYPES,
IndexedArray,
EditorConfig,
} from '../legacy_imports';
import { AggConfig, AggType, AggGroupNames, BUCKET_TYPES, EditorConfig } from '../legacy_imports';
import {
getAggParamsToRender,
getAggTypeOptions,
Expand Down Expand Up @@ -111,8 +104,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: AggConfig) =>
aggConfig
.getIndexPattern()
.fields.filter(({ type }) => filterFieldTypes.includes(type))
),
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 FieldParamType).getAvailableFields(
agg.getIndexPattern().fields
);
const availableFields: Field[] = (param as FieldParamType).getAvailableFields(agg);
fields = aggTypeFieldFilters.filter(availableFields, agg);
indexedFields = groupAndSortBy(fields, 'type', 'name');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export { AggParamOption } from 'ui/agg_types/agg_params';
export { CidrMask } from 'ui/agg_types/buckets/lib/cidr_mask';

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';
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/ui/public/agg_types/agg_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,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
Expand Down
3 changes: 2 additions & 1 deletion src/legacy/ui/public/agg_types/metrics/metric_agg_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
17 changes: 4 additions & 13 deletions src/legacy/ui/public/agg_types/metrics/top_hit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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('common.ui.aggTypes.metrics.topHitTitle', {
Expand Down Expand Up @@ -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 = {};
Expand Down
65 changes: 62 additions & 3 deletions src/legacy/ui/public/agg_types/param_types/field.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 { AggConfig } from '../agg_config';
import { IMetricAggConfig } from '../metrics/metric_agg_type';
import { Schema } from '../schemas';

jest.mock('ui/new_platform');

Expand All @@ -45,7 +49,11 @@ describe('Field', () => {
searchable: true,
},
],
} as any;
};

const agg = ({
getIndexPattern: jest.fn(() => indexPattern),
} as unknown) as AggConfig;

describe('constructor', () => {
it('it is an instance of BaseParamType', () => {
Expand All @@ -65,7 +73,7 @@ describe('Field', () => {
type: 'field',
});

const fields = aggParam.getAvailableFields(indexPattern.fields);
const fields = aggParam.getAvailableFields(agg);

expect(fields.length).toBe(1);

Expand All @@ -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);
});
Expand Down
24 changes: 14 additions & 10 deletions src/legacy/ui/public/agg_types/param_types/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,25 @@
* under the License.
*/

// @ts-ignore
import { i18n } from '@kbn/i18n';
import { isFunction } from 'lodash';
import { AggConfig } from '../agg_config';
import { SavedObjectNotFound } from '../../../../../plugins/kibana_utils/public';
import { BaseParamType } from './base';
import { toastNotifications } from '../../notify';
import { propFilter } from '../filter';
import { Field, IFieldList } from '../../../../../plugins/data/public';
import { isNestedField } from '../../../../../plugins/data/public';
import { Field, KBN_FIELD_TYPES, isNestedField } from '../../../../../plugins/data/public';
import { IMetricAggConfig } from '../metrics/metric_agg_type';

const filterByType = propFilter('type');

type FieldTypes = KBN_FIELD_TYPES | KBN_FIELD_TYPES[] | '*';
export type FilterFieldTypes = ((aggConfig: IMetricAggConfig) => FieldTypes) | FieldTypes;

export class FieldParamType extends BaseParamType {
required = true;
scriptable = true;
filterFieldTypes: string;
filterFieldTypes: FilterFieldTypes;
onlyAggregatable: boolean;

constructor(config: Record<string, any>) {
Expand Down Expand Up @@ -85,9 +88,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) {
toastNotifications.addDanger(
i18n.translate(
Expand All @@ -110,7 +111,8 @@ export class FieldParamType extends BaseParamType {
/**
* filter the fields to the available ones
*/
getAvailableFields = (fields: IFieldList) => {
getAvailableFields = (aggConfig: AggConfig) => {
const fields = aggConfig.getIndexPattern().fields;
const filteredFields = fields.filter((field: Field) => {
const { onlyAggregatable, scriptable, filterFieldTypes } = this;

Expand All @@ -121,8 +123,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;
Expand Down