Skip to content

Commit

Permalink
[data.search.aggs] Remove fieldFormats from AggConfig & AggConfigs (e…
Browse files Browse the repository at this point in the history 
…lastic#69762)
  • Loading branch information
@lukeelmers @Bamieh
lukeelmers authored and Bamieh committed Jul 1, 2020
1 parent a10ae51 commit 1ae6ef3
Show file tree
Hide file tree
Showing 66 changed files with 203 additions and 578 deletions.
8 changes: 6 additions & 2 deletions src/plugins/data/common/field_formats/mocks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
* under the License.
*/

import { IFieldFormatsRegistry } from '.';
import { identity } from 'lodash';
import { FieldFormat, IFieldFormatsRegistry } from '.';

export const fieldFormatsMock: IFieldFormatsRegistry = {
getByFieldType: jest.fn(),
Expand All @@ -35,6 +36,9 @@ export const fieldFormatsMock: IFieldFormatsRegistry = {
init: jest.fn(),
register: jest.fn(),
parseDefaultTypeMap: jest.fn(),
deserialize: jest.fn(),
deserialize: jest.fn().mockImplementation(() => {
const DefaultFieldFormat = FieldFormat.from(identity);
return new DefaultFieldFormat();
}),
getTypeWithoutMetaParams: jest.fn(),
};
5 changes: 1 addition & 4 deletions src/plugins/data/public/plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,7 @@ export class DataPublicPlugin implements Plugin<DataPublicPluginSetup, DataPubli
const query = this.queryService.start(savedObjects);
setQueryService(query);

const search = this.searchService.start(core, {
indexPatterns,
fieldFormats,
});
const search = this.searchService.start(core, { indexPatterns });
setSearchService(search);

uiActions.addTriggerAction(
Expand Down
145 changes: 22 additions & 123 deletions src/plugins/data/public/search/aggs/agg_config.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,31 +25,23 @@ import { AggType } from './agg_type';
import { AggTypesRegistryStart } from './agg_types_registry';
import { mockDataServices, mockAggTypesRegistry } from './test_helpers';
import { MetricAggType } from './metrics/metric_agg_type';
import {
Field as IndexPatternField,
IndexPattern,
IIndexPatternFieldList,
} from '../../index_patterns';
import { IndexPattern, IIndexPatternFieldList } from '../../index_patterns';
import { stubIndexPatternWithFields } from '../../../public/stubs';
import { FieldFormatsStart } from '../../field_formats';
import { fieldFormatsServiceMock } from '../../field_formats/mocks';

describe('AggConfig', () => {
let indexPattern: IndexPattern;
let typesRegistry: AggTypesRegistryStart;
let fieldFormats: FieldFormatsStart;

beforeEach(() => {
jest.restoreAllMocks();
mockDataServices();
fieldFormats = fieldFormatsServiceMock.createStartContract();
indexPattern = stubIndexPatternWithFields as IndexPattern;
typesRegistry = mockAggTypesRegistry();
});

describe('#toDsl', () => {
it('calls #write()', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
enabled: true,
type: 'date_histogram',
Expand All @@ -64,7 +56,7 @@ describe('AggConfig', () => {
});

it('uses the type name as the agg name', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
enabled: true,
type: 'date_histogram',
Expand All @@ -79,7 +71,7 @@ describe('AggConfig', () => {
});

it('uses the params from #write() output as the agg params', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
enabled: true,
type: 'date_histogram',
Expand Down Expand Up @@ -109,7 +101,7 @@ describe('AggConfig', () => {
params: {},
},
];
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry });

const histoConfig = ac.byName('date_histogram')[0];
const avgConfig = ac.byName('avg')[0];
Expand Down Expand Up @@ -219,8 +211,8 @@ describe('AggConfig', () => {

testsIdentical.forEach((configState, index) => {
it(`identical aggregations (${index})`, () => {
const ac1 = new AggConfigs(indexPattern, configState, { typesRegistry, fieldFormats });
const ac2 = new AggConfigs(indexPattern, configState, { typesRegistry, fieldFormats });
const ac1 = new AggConfigs(indexPattern, configState, { typesRegistry });
const ac2 = new AggConfigs(indexPattern, configState, { typesRegistry });
expect(ac1.jsonDataEquals(ac2.aggs)).toBe(true);
});
});
Expand Down Expand Up @@ -260,8 +252,8 @@ describe('AggConfig', () => {

testsIdenticalDifferentOrder.forEach((test, index) => {
it(`identical aggregations (${index}) - init json is in different order`, () => {
const ac1 = new AggConfigs(indexPattern, test.config1, { typesRegistry, fieldFormats });
const ac2 = new AggConfigs(indexPattern, test.config2, { typesRegistry, fieldFormats });
const ac1 = new AggConfigs(indexPattern, test.config1, { typesRegistry });
const ac2 = new AggConfigs(indexPattern, test.config2, { typesRegistry });
expect(ac1.jsonDataEquals(ac2.aggs)).toBe(true);
});
});
Expand Down Expand Up @@ -325,16 +317,16 @@ describe('AggConfig', () => {

testsDifferent.forEach((test, index) => {
it(`different aggregations (${index})`, () => {
const ac1 = new AggConfigs(indexPattern, test.config1, { typesRegistry, fieldFormats });
const ac2 = new AggConfigs(indexPattern, test.config2, { typesRegistry, fieldFormats });
const ac1 = new AggConfigs(indexPattern, test.config1, { typesRegistry });
const ac2 = new AggConfigs(indexPattern, test.config2, { typesRegistry });
expect(ac1.jsonDataEquals(ac2.aggs)).toBe(false);
});
});
});

describe('#serialize', () => {
it('includes the aggs id, params, type and schema', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
enabled: true,
type: 'date_histogram',
Expand Down Expand Up @@ -365,8 +357,8 @@ describe('AggConfig', () => {
params: {},
},
];
const ac1 = new AggConfigs(indexPattern, configStates, { typesRegistry, fieldFormats });
const ac2 = new AggConfigs(indexPattern, configStates, { typesRegistry, fieldFormats });
const ac1 = new AggConfigs(indexPattern, configStates, { typesRegistry });
const ac2 = new AggConfigs(indexPattern, configStates, { typesRegistry });

// this relies on the assumption that js-engines consistently loop over properties in insertion order.
// most likely the case, but strictly speaking not guaranteed by the JS and JSON specifications.
Expand Down Expand Up @@ -394,7 +386,7 @@ describe('AggConfig', () => {
params: { field: 'machine.os.keyword' },
},
];
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry });

expect(ac.aggs.map((agg) => agg.toSerializedFieldFormat())).toMatchInlineSnapshot(`
Array [
Expand Down Expand Up @@ -456,7 +448,7 @@ describe('AggConfig', () => {
},
},
];
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, configStates, { typesRegistry });

expect(ac.aggs.map((agg) => agg.toSerializedFieldFormat())).toMatchInlineSnapshot(`
Array [
Expand All @@ -478,20 +470,8 @@ describe('AggConfig', () => {
});

describe('#toExpressionAst', () => {
beforeEach(() => {
fieldFormats.getDefaultInstance = (() => ({
getConverterFor: (t?: string) => t || identity,
})) as any;
indexPattern.fields.getByName = (name) =>
({
format: {
getConverterFor: (t?: string) => t || identity,
},
} as IndexPatternField);
});

it('works with primitive param types', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
enabled: true,
type: 'terms',
Expand Down Expand Up @@ -540,7 +520,7 @@ describe('AggConfig', () => {
});

it('creates a subexpression for params of type "agg"', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'terms',
params: {
Expand Down Expand Up @@ -616,7 +596,7 @@ describe('AggConfig', () => {
},
});

const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'range',
params: {
Expand Down Expand Up @@ -647,7 +627,7 @@ describe('AggConfig', () => {
});

it('stringifies any other params which are an object', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'terms',
params: {
Expand All @@ -662,7 +642,7 @@ describe('AggConfig', () => {
});

it(`returns undefined if an expressionName doesn't exist on the agg type`, () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'unknown type',
params: {},
Expand All @@ -676,7 +656,7 @@ describe('AggConfig', () => {
let aggConfig: AggConfig;

beforeEach(() => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
aggConfig = ac.createAggConfig({ type: 'count' } as CreateAggConfigParams);
});

Expand All @@ -702,85 +682,4 @@ describe('AggConfig', () => {
expect(label).toBe('');
});
});

describe('#fieldFormatter - custom getFormat handler', () => {
it('returns formatter from getFormat handler', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry, fieldFormats });
const configStates = {
enabled: true,
type: 'count',
schema: 'metric',
params: { field: '@timestamp' },
};
const aggConfig = ac.createAggConfig(configStates);

const fieldFormatter = aggConfig.fieldFormatter();
expect(fieldFormatter).toBeDefined();
expect(fieldFormatter('text')).toBe('text');
});
});

// TODO: Converting these field formatter tests from browser tests to unit
// tests makes them much less helpful due to the extensive use of mocking.
// We should revisit these and rewrite them into something more useful.
describe('#fieldFormatter - no custom getFormat handler', () => {
let aggConfig: AggConfig;

beforeEach(() => {
fieldFormats.getDefaultInstance = (() => ({
getConverterFor: (t?: string) => t || identity,
})) as any;
indexPattern.fields.getByName = (name) =>
({
format: {
getConverterFor: (t?: string) => t || identity,
},
} as IndexPatternField);

const configStates = {
enabled: true,
type: 'histogram',
schema: 'bucket',
params: {
field: 'bytes',
},
};
const ac = new AggConfigs(indexPattern, [configStates], { typesRegistry, fieldFormats });
aggConfig = ac.createAggConfig(configStates);
});

it("returns the field's formatter", () => {
aggConfig.params.field = {
format: {
getConverterFor: (t?: string) => t || identity,
},
};
expect(aggConfig.fieldFormatter().toString()).toBe(
aggConfig.getField().format.getConverterFor().toString()
);
});

it('returns the string format if the field does not have a format', () => {
const agg = aggConfig;
agg.params.field = { type: 'number', format: null };
const fieldFormatter = agg.fieldFormatter();
expect(fieldFormatter).toBeDefined();
expect(fieldFormatter('text')).toBe('text');
});

it('returns the string format if there is no field', () => {
const agg = aggConfig;
delete agg.params.field;
const fieldFormatter = agg.fieldFormatter();
expect(fieldFormatter).toBeDefined();
expect(fieldFormatter('text')).toBe('text');
});

it('returns the html converter if "html" is passed in', () => {
const field = indexPattern.fields.getByName('bytes');
expect(aggConfig.fieldFormatter('html').toString()).toBe(
field!.format.getConverterFor('html').toString()
);
});
});
});
33 changes: 1 addition & 32 deletions src/plugins/data/public/search/aggs/agg_config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ import { writeParams } from './agg_params';
import { IAggConfigs } from './agg_configs';
import { FetchOptions } from '../fetch';
import { ISearchSource } from '../search_source';
import { FieldFormatsContentType, KBN_FIELD_TYPES } from '../../../common';
import { FieldFormatsStart } from '../../field_formats';

type State = string | number | boolean | null | undefined | SerializableState;

Expand All @@ -52,10 +50,6 @@ export type AggConfigSerialized = Ensure<
SerializableState
>;

export interface AggConfigDependencies {
fieldFormats: FieldFormatsStart;
}

export type AggConfigOptions = Assign<AggConfigSerialized, { type: IAggType }>;

/**
Expand Down Expand Up @@ -116,13 +110,8 @@ export class AggConfig {
private __type: IAggType;
private __typeDecorations: any;
private subAggs: AggConfig[] = [];
private readonly fieldFormats: FieldFormatsStart;

constructor(
aggConfigs: IAggConfigs,
opts: AggConfigOptions,
{ fieldFormats }: AggConfigDependencies
) {
constructor(aggConfigs: IAggConfigs, opts: AggConfigOptions) {
this.aggConfigs = aggConfigs;
this.id = String(opts.id || AggConfig.nextId(aggConfigs.aggs as any));
this.enabled = typeof opts.enabled === 'boolean' ? opts.enabled : true;
Expand All @@ -143,8 +132,6 @@ export class AggConfig {

// @ts-ignore
this.__type = this.__type;

this.fieldFormats = fieldFormats;
}

/**
Expand Down Expand Up @@ -433,24 +420,6 @@ export class AggConfig {
return this.aggConfigs.timeRange;
}

fieldFormatter(contentType?: FieldFormatsContentType, defaultFormat?: any) {
const format = this.type && this.type.getFormat(this);

if (format) {
return format.getConverterFor(contentType);
}

return this.fieldOwnFormatter(contentType, defaultFormat);
}

fieldOwnFormatter(contentType?: FieldFormatsContentType, defaultFormat?: any) {
const field = this.getField();
let format = field && field.format;
if (!format) format = defaultFormat;
if (!format) format = this.fieldFormats.getDefaultInstance(KBN_FIELD_TYPES.STRING);
return format.getConverterFor(contentType);
}

fieldName() {
const field = this.getField();
return field ? field.name : '';
Expand Down
Loading

0 comments on commit 1ae6ef3

Please sign in to comment.