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.

Sign up for GitHub

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

[data.search.aggs] Remove fieldFormats from AggConfig & AggConfigs #69762

Merged
merged 9 commits into from
Jun 29, 2020
Merged

[data.search.aggs] Remove fieldFormats from AggConfig & AggConfigs #69762

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
902a7b6
Add format to serialized agg config.
lukeelmers Jun 13, 2020
ad39196
Add AggConfig.toSerializedFieldFormat.
lukeelmers Jun 13, 2020
9be4d94
Remove buildTabularInspectorData dependency on aggConfig.getFieldForm…
lukeelmers Jun 18, 2020
4dc069f
Remove table vis dependency on agg.type.getFormat.
lukeelmers Jun 24, 2020
d64e119
Remove fieldFormats from AggConfig and AggConfigs.
lukeelmers Jun 18, 2020
b035324
Remove unused i18n translations.
lukeelmers Jun 23, 2020
eb9c6e2
Update tile map test to expect formatted inspector headers.
lukeelmers Jun 24, 2020
cb0d0af
Merge branch 'master' into fix/remove-agg-formats
elasticmachine Jun 25, 2020
d204e88
Merge branch 'master' into fix/remove-agg-formats
elasticmachine Jun 29, 2020
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
8 changes: 6 additions & 2 deletions src/plugins/data/common/field_formats/mocks.ts
View file
Open in desktop
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();
}),
Comment on lines +39 to +42
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a mock implementation to improve these somewhat. I just stole this from the getFormat function inside of fieldFormats.deserialize.

Important to note that since this is configured with identity, any calls against this mock are just going to return whatever they are given. So you'll get a convert function you can use, but of course it won't actually convert what you give to it.

getTypeWithoutMetaParams: jest.fn(),
};
5 changes: 1 addition & 4 deletions src/plugins/data/public/plugin.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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