Skip to content

Commit b4aa2b2

Browse files
spalgerspalger
authored
Merge pull request elastic#8856 from elastic/jasper/backport/8734/5.x
[backport] PR elastic#8734 to 5.x - Unify getting fields for aggs, and filter scripted fields for significant terms agg Former-commit-id: fa11c03
2 parents 0c145df + f888b04 commit b4aa2b2

File tree

10 files changed

+130
-87
lines changed

10 files changed

+130
-87
lines changed

‎src/core_plugins/kibana/public/visualize/editor/__tests__/agg.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ describe('Vis-Editor-Agg plugin directive', function () {
4747
$parentScope.agg = {
4848
id: 1,
4949
params: {},
50-
schema: makeConfig()
50+
schema: makeConfig(),
51+
getFieldOptions: () => null
5152
};
5253
$parentScope.groupName = 'metrics';
5354
$parentScope.group = [{

‎src/core_plugins/kibana/public/visualize/editor/agg_params.js

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ uiModules
5959

6060
// create child scope, used in the editors
6161
$aggParamEditorsScope = $scope.$new();
62+
$aggParamEditorsScope.indexedFields = $scope.agg.getFieldOptions();
6263

6364
const agg = $scope.agg;
6465
if (!agg) return;
@@ -81,10 +82,6 @@ uiModules
8182
// build collection of agg params html
8283
type.params.forEach(function (param, i) {
8384
let aggParam;
84-
// if field param exists, compute allowed fields
85-
if (param.name === 'field') {
86-
$aggParamEditorsScope.indexedFields = getIndexedFields(param);
87-
}
8885

8986
if ($aggParamEditorsScope.indexedFields) {
9087
const hasIndexedFields = $aggParamEditorsScope.indexedFields.length > 0;
@@ -136,30 +133,6 @@ uiModules
136133
.append(param.editor)
137134
.get(0);
138135
}
139-
140-
function getIndexedFields(param) {
141-
let fields = _.filter($scope.agg.vis.indexPattern.fields.raw, 'aggregatable');
142-
const fieldTypes = param.filterFieldTypes;
143-
144-
if (fieldTypes) {
145-
fields = $filter('fieldType')(fields, fieldTypes);
146-
fields = $filter('orderBy')(fields, ['type', 'name']);
147-
}
148-
149-
return new IndexedArray({
150-
151-
/**
152-
* @type {Array}
153-
*/
154-
index: ['name'],
155-
156-
/**
157-
* [group description]
158-
* @type {Array}
159-
*/
160-
initialSet: fields
161-
});
162-
}
163136
}
164137
};
165138
});

‎src/fixtures/logstash_fields.js

Lines changed: 60 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,65 @@
11
function stubbedLogstashFields() {
2-
let sourceData = [
3-
{ name: 'bytes', type: 'number', indexed: true, analyzed: true, sortable: true, filterable: true, count: 10 },
4-
{ name: 'ssl', type: 'boolean', indexed: true, analyzed: true, sortable: true, filterable: true, count: 20 },
5-
{ name: '@timestamp', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true, count: 30 },
6-
{ name: 'time', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true, count: 30 },
7-
{ name: '@tags', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
8-
{ name: 'utc_time', type: 'date', indexed: true, analyzed: true, sortable: true, filterable: true },
9-
{ name: 'phpmemory', type: 'number', indexed: true, analyzed: true, sortable: true, filterable: true },
10-
{ name: 'ip', type: 'ip', indexed: true, analyzed: true, sortable: true, filterable: true },
11-
{ name: 'request_body', type: 'attachment', indexed: true, analyzed: true, sortable: false, filterable: true },
12-
{ name: 'point', type: 'geo_point', indexed: true, analyzed: true, sortable: false, filterable: false },
13-
{ name: 'area', type: 'geo_shape', indexed: true, analyzed: true, sortable: true, filterable: false },
14-
{ name: 'hashed', type: 'murmur3', indexed: true, analyzed: true, sortable: false, filterable: false },
15-
{ name: 'geo.coordinates', type: 'geo_point', indexed: true, analyzed: true, sortable: false, filterable: true },
16-
{ name: 'extension', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
17-
{ name: 'machine.os', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
18-
{ name: 'geo.src', type: 'string', indexed: true, analyzed: true, sortable: true, filterable: true },
19-
{ name: '_type', type: 'string', indexed: false, analyzed: true, sortable: true, filterable: true },
20-
{ name: '_id', type: 'string', indexed: false, analyzed: false, sortable: false, filterable: true},
21-
{ name: '_source', type: 'string', indexed: false, analyzed: false, sortable: false, filterable: false},
22-
{ name: 'custom_user_field', type: 'conflict', indexed: false, analyzed: false, sortable: false, filterable: true },
23-
{ name: 'script string', type: 'string', scripted: true, script: '\'i am a string\'', lang: 'expression' },
24-
{ name: 'script number', type: 'number', scripted: true, script: '1234', lang: 'expression' },
25-
{ name: 'script date', type: 'date', scripted: true, script: '1234', lang: 'painless' },
26-
{ name: 'script murmur3', type: 'murmur3', scripted: true, script: '1234', lang: 'expression'},
27-
].map(function (field) {
28-
field.count = field.count || 0;
29-
field.scripted = field.scripted || false;
30-
return field;
31-
});
2+
return [
3+
// |indexed
4+
// | |analyzed
5+
// | | |aggregatable
6+
// | | | |searchable
7+
// name type | | | | |metadata
8+
['bytes', 'number', true, true, true, true, { count: 10 } ],
9+
['ssl', 'boolean', true, true, true, true, { count: 20 } ],
10+
['@timestamp', 'date', true, true, true, true, { count: 30 } ],
11+
['time', 'date', true, true, true, true, { count: 30 } ],
12+
['@tags', 'string', true, true, true, true ],
13+
['utc_time', 'date', true, true, true, true ],
14+
['phpmemory', 'number', true, true, true, true ],
15+
['ip', 'ip', true, true, true, true ],
16+
['request_body', 'attachment', true, true, true, true ],
17+
['point', 'geo_point', true, true, true, true ],
18+
['area', 'geo_shape', true, true, true, true ],
19+
['hashed', 'murmur3', true, true, false, true ],
20+
['geo.coordinates', 'geo_point', true, true, true, true ],
21+
['extension', 'string', true, true, true, true ],
22+
['machine.os', 'string', true, true, true, true ],
23+
['geo.src', 'string', true, true, true, true ],
24+
['_id', 'string', false, false, true, true ],
25+
['_type', 'string', false, false, true, true ],
26+
['_source', 'string', false, false, true, true ],
27+
['custom_user_field', 'conflict', false, false, true, true ],
28+
['script string', 'string', false, false, true, false, { script: '\'i am a string\'' } ],
29+
['script number', 'number', false, false, true, false, { script: '1234' } ],
30+
['script date', 'date', false, false, true, false, { script: '1234', lang: 'painless' } ],
31+
['script murmur3', 'murmur3', false, false, true, false, { script: '1234' } ],
32+
].map(function (row) {
33+
const [
34+
name,
35+
type,
36+
indexed,
37+
analyzed,
38+
aggregatable,
39+
searchable,
40+
metadata = {}
41+
] = row;
42+
43+
const {
44+
count = 0,
45+
script,
46+
lang = script ? 'expression' : undefined,
47+
scripted = !!script,
48+
} = metadata;
3249

33-
return sourceData;
50+
return {
51+
name,
52+
type,
53+
indexed,
54+
analyzed,
55+
aggregatable,
56+
searchable,
57+
count,
58+
script,
59+
lang,
60+
scripted,
61+
};
62+
});
3463
}
3564

3665
export default stubbedLogstashFields;

‎src/ui/public/agg_response/tabify/__tests__/_response_writer.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ describe('ResponseWriter class', function () {
5353
let aggs = [
5454
{ type: 'date_histogram', schema: 'segment', params: { field: '@timestamp' } },
5555
{ type: 'terms', schema: 'segment', params: { field: 'extension' } },
56-
{ type: 'avg', schema: 'metric', params: { field: '@timestamp' } }
56+
{ type: 'avg', schema: 'metric', params: { field: 'bytes' } }
5757
];
5858

5959
getColumns.returns(aggs.map(function (agg) {

‎src/ui/public/agg_types/buckets/significant_terms.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ export default function SignificantTermsAggDefinition(Private) {
1616
params: [
1717
{
1818
name: 'field',
19+
scriptable: false,
1920
filterFieldTypes: 'string'
2021
},
2122
{

‎src/ui/public/agg_types/buckets/terms.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ export default function TermsAggDefinition(Private) {
4343
params: [
4444
{
4545
name: 'field',
46-
scriptable: true,
4746
filterFieldTypes: ['number', 'boolean', 'date', 'ip', 'string']
4847
},
4948
{

‎src/ui/public/agg_types/param_types/field.js

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,21 @@ import { SavedObjectNotFound } from 'ui/errors';
22
import _ from 'lodash';
33
import editorHtml from 'ui/agg_types/controls/field.html';
44
import AggTypesParamTypesBaseProvider from 'ui/agg_types/param_types/base';
5-
export default function FieldAggParamFactory(Private) {
5+
import 'ui/filters/field_type';
6+
import IndexedArray from 'ui/indexed_array';
7+
import Notifier from 'ui/notify/notifier';
68

9+
export default function FieldAggParamFactory(Private, $filter) {
710
let BaseAggParam = Private(AggTypesParamTypesBaseProvider);
11+
const notifier = new Notifier();
812

913
_.class(FieldAggParam).inherits(BaseAggParam);
1014
function FieldAggParam(config) {
1115
FieldAggParam.Super.call(this, config);
1216
}
1317

1418
FieldAggParam.prototype.editor = editorHtml;
15-
FieldAggParam.prototype.scriptable = false;
19+
FieldAggParam.prototype.scriptable = true;
1620
FieldAggParam.prototype.filterFieldTypes = '*';
1721

1822
/**
@@ -25,6 +29,32 @@ export default function FieldAggParamFactory(Private) {
2529
return field.name;
2630
};
2731

32+
/**
33+
* Get the options for this field from the indexPattern
34+
*/
35+
FieldAggParam.prototype.getFieldOptions = function (aggConfig) {
36+
const indexPattern = aggConfig.getIndexPattern();
37+
let fields = indexPattern.fields.raw;
38+
39+
fields = fields.filter(f => f.aggregatable);
40+
41+
if (!this.scriptable) {
42+
fields = fields.filter(field => !field.scripted);
43+
}
44+
45+
if (this.filterFieldTypes) {
46+
fields = $filter('fieldType')(fields, this.filterFieldTypes);
47+
fields = $filter('orderBy')(fields, ['type', 'name']);
48+
}
49+
50+
51+
return new IndexedArray({
52+
index: ['name'],
53+
group: ['type'],
54+
initialSet: fields
55+
});
56+
};
57+
2858
/**
2959
* Called to read values from a database record into the
3060
* aggConfig object
@@ -33,13 +63,18 @@ export default function FieldAggParamFactory(Private) {
3363
* @return {field}
3464
*/
3565
FieldAggParam.prototype.deserialize = function (fieldName, aggConfig) {
36-
let field = aggConfig.vis.indexPattern.fields.byName[fieldName];
66+
const field = aggConfig.getIndexPattern().fields.byName[fieldName];
3767

3868
if (!field) {
3969
throw new SavedObjectNotFound('index-pattern-field', fieldName);
4070
}
4171

42-
return field;
72+
const validField = this.getFieldOptions(aggConfig).byName[fieldName];
73+
if (!validField) {
74+
notifier.error(`Saved "field" parameter is now invalid. Please select a new field.`);
75+
}
76+
77+
return validField;
4378
};
4479

4580
/**
@@ -56,7 +91,7 @@ export default function FieldAggParamFactory(Private) {
5691
let field = aggConfig.getField();
5792

5893
if (!field) {
59-
throw new Error(`"${aggConfig.makeLabel()}" requires a field`);
94+
throw new TypeError('"field" is a required parameter');
6095
}
6196

6297
if (field.scripted) {

‎src/ui/public/index_patterns/__tests__/_index_pattern.js

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@ import IndexPatternsMapperProvider from 'ui/index_patterns/_mapper';
1212
import UtilsMappingSetupProvider from 'ui/utils/mapping_setup';
1313
import IndexPatternsIntervalsProvider from 'ui/index_patterns/_intervals';
1414
import IndexPatternsIndexPatternProvider from 'ui/index_patterns/_index_pattern';
15+
import NoDigestPromises from 'test_utils/no_digest_promises';
1516

1617
describe('index pattern', function () {
18+
NoDigestPromises.activateForSuite();
19+
1720
let IndexPattern;
1821
let mapper;
1922
let mappingSetup;
@@ -55,7 +58,7 @@ describe('index pattern', function () {
5558

5659
// stub calculateIndices
5760
calculateIndices = sinon.spy(function () {
58-
return $injector.get('Promise').resolve([
61+
return Promise.resolve([
5962
{ index: 'foo', max: Infinity, min: -Infinity },
6063
{ index: 'bar', max: Infinity, min: -Infinity }
6164
]);
@@ -150,7 +153,6 @@ describe('index pattern', function () {
150153

151154
describe('refresh fields', function () {
152155
// override the default indexPattern, with a truncated field list
153-
require('test_utils/no_digest_promises').activateForSuite();
154156
const indexPatternId = 'test-pattern';
155157
let indexPattern;
156158
let fieldLength;
@@ -321,7 +323,6 @@ describe('index pattern', function () {
321323
});
322324

323325
describe('#toDetailedIndexList', function () {
324-
require('test_utils/no_digest_promises').activateForSuite();
325326
context('when index pattern is an interval', function () {
326327
let interval;
327328
beforeEach(function () {
@@ -400,7 +401,6 @@ describe('index pattern', function () {
400401

401402
describe('#toIndexList', function () {
402403
context('when index pattern is an interval', function () {
403-
require('test_utils/no_digest_promises').activateForSuite();
404404

405405
let interval;
406406
beforeEach(function () {
@@ -431,7 +431,6 @@ describe('index pattern', function () {
431431
});
432432

433433
context('when index pattern is a time-base wildcard', function () {
434-
require('test_utils/no_digest_promises').activateForSuite();
435434
beforeEach(function () {
436435
sinon.stub(indexPattern, 'getInterval').returns(false);
437436
sinon.stub(indexPattern, 'hasTimeField').returns(true);
@@ -453,7 +452,6 @@ describe('index pattern', function () {
453452
});
454453

455454
context('when index pattern is a time-base wildcard that is configured not to expand', function () {
456-
require('test_utils/no_digest_promises').activateForSuite();
457455
beforeEach(function () {
458456
sinon.stub(indexPattern, 'getInterval').returns(false);
459457
sinon.stub(indexPattern, 'hasTimeField').returns(true);
@@ -472,13 +470,8 @@ describe('index pattern', function () {
472470
sinon.stub(indexPattern, 'getInterval').returns(false);
473471
});
474472

475-
it('is fulfilled by id', function () {
476-
let indexList;
477-
indexPattern.toIndexList().then(function (val) {
478-
indexList = val;
479-
});
480-
$rootScope.$apply();
481-
473+
it('is fulfilled by id', async function () {
474+
let indexList = await indexPattern.toIndexList();
482475
expect(indexList).to.equal(indexPattern.id);
483476
});
484477
});

‎src/ui/public/vis/__tests__/_agg_config.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -481,12 +481,12 @@ describe('AggConfig', function () {
481481
{
482482
type: 'avg',
483483
schema: 'metric',
484-
params: { field: 'ssl' }
484+
params: { field: 'bytes' }
485485
}
486486
]
487487
});
488488

489-
let field = indexPattern.fields.byName.ssl;
489+
let field = indexPattern.fields.byName.bytes;
490490
expect(vis.aggs[0].fieldFormatter('html')).to.be(field.format.getConverterFor('html'));
491491
});
492492
});

‎src/ui/public/vis/agg_config.js

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -148,13 +148,11 @@ export default function AggConfigFactory(Private, fieldTypeFilter) {
148148
* @return {object} the new params object
149149
*/
150150
AggConfig.prototype.resetParams = function () {
151-
let fieldParam = this.type && this.type.params.byName.field;
152151
let field;
152+
const fieldOptions = this.getFieldOptions();
153153

154-
if (fieldParam) {
155-
let prevField = this.params.field;
156-
let fieldOpts = fieldTypeFilter(this.vis.indexPattern.fields, fieldParam.filterFieldTypes);
157-
field = _.contains(fieldOpts, prevField) ? prevField : null;
154+
if (fieldOptions) {
155+
field = fieldOptions.byName[this.fieldName()] || null;
158156
}
159157

160158
return this.fillDefaults({ row: this.params.row, field: field });
@@ -286,6 +284,20 @@ export default function AggConfigFactory(Private, fieldTypeFilter) {
286284
return pre += this.type.makeLabel(this);
287285
};
288286

287+
AggConfig.prototype.getIndexPattern = function () {
288+
return this.vis.indexPattern;
289+
};
290+
291+
AggConfig.prototype.getFieldOptions = function () {
292+
const fieldParamType = this.type && this.type.params.byName.field;
293+
294+
if (!fieldParamType || !fieldParamType.getFieldOptions) {
295+
return null;
296+
}
297+
298+
return fieldParamType.getFieldOptions(this);
299+
};
300+
289301
AggConfig.prototype.fieldFormatter = function (contentType, defaultFormat) {
290302
let format = this.type && this.type.getFormat(this);
291303
if (format) return format.getConverterFor(contentType);

0 commit comments

Comments
 (0)