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

fixes filtering on other bucket outside of vislib #19860

Merged
merged 9 commits into from
Jun 20, 2018
Merged
6 changes: 4 additions & 2 deletions src/core_plugins/tagcloud/public/tag_cloud_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ export class TagCloudVisualization {
if (!this._bucketAgg) {
return;
}
const filter = this._bucketAgg.createFilter(event);
this._vis.API.queryFilter.addFilters(filter);
this._vis.API.events.filter(
this._data, 0, this._data.rows.findIndex(row => row[0] === event)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest instead of trying to find the row, we change the tagcloud visualization to attach that data and pass it when clicking a tag:

diff --git a/src/core_plugins/tagcloud/public/tag_cloud.js b/src/core_plugins/tagcloud/public/tag_cloud.js
index db3b1a8506..3d158ec4d5 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud.js
@@ -224,7 +224,7 @@ class TagCloud extends EventEmitter {
       const self = this;
       enteringTags.on({
         click: function (event) {
-          self.emit('select', event.rawText);
+          self.emit('select', event);
         },
         mouseover: function () {
           d3.select(this).style('cursor', 'pointer');
diff --git a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
index e9cc02ee9e..0fb5cc11ee 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
@@ -47,7 +47,7 @@ export class TagCloudVisualization {
         return;
       }
       this._vis.API.events.addFilter(
-        this._data, 0, this._data.rows.findIndex(row => row[0] === event)
+        this._data, 0, event.meta.rowIndex
       );
     });
     this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');
@@ -121,12 +121,15 @@ export class TagCloudVisualization {
       this._bucketAgg = null;
     }
 
-    const tags = data.rows.map(row => {
+    const tags = data.rows.map((row, rowIndex) => {
       const [tag, count] = row;
       return {
         displayText: this._bucketAgg ? this._bucketAgg.fieldFormatter()(tag) : tag,
         rawText: tag,
-        value: count
+        value: count,
+        meta: {
+          rowIndex,
+        },
       };
     });

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should even attach the data to each tag's meta and use that one instead of writing it to this._data, which I think could potentially in very bad timing not be the data that was used to render the tag anymore, since tag cloud does render async.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e.

diff --git a/src/core_plugins/tagcloud/public/tag_cloud.js b/src/core_plugins/tagcloud/public/tag_cloud.js
index db3b1a8506..3d158ec4d5 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud.js
@@ -224,7 +224,7 @@ class TagCloud extends EventEmitter {
       const self = this;
       enteringTags.on({
         click: function (event) {
-          self.emit('select', event.rawText);
+          self.emit('select', event);
         },
         mouseover: function () {
           d3.select(this).style('cursor', 'pointer');
diff --git a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
index e9cc02ee9e..e3e4bee1dd 100644
--- a/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
+++ b/src/core_plugins/tagcloud/public/tag_cloud_visualization.js
@@ -47,7 +47,7 @@ export class TagCloudVisualization {
         return;
       }
       this._vis.API.events.addFilter(
-        this._data, 0, this._data.rows.findIndex(row => row[0] === event)
+        event.meta.data, 0, event.meta.rowIndex
       );
     });
     this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');
@@ -121,12 +121,16 @@ export class TagCloudVisualization {
       this._bucketAgg = null;
     }
 
-    const tags = data.rows.map(row => {
+    const tags = data.rows.map((row, rowIndex) => {
       const [tag, count] = row;
       return {
         displayText: this._bucketAgg ? this._bucketAgg.fieldFormatter()(tag) : tag,
         rawText: tag,
-        value: count
+        value: count,
+        meta: {
+          data,
+          rowIndex,
+        },
       };
     });

);
});
this._renderComplete$ = Observable.fromEvent(this._tagCloud, 'renderComplete');

Expand Down Expand Up @@ -112,6 +113,7 @@ export class TagCloudVisualization {
}

const data = response.tables[0];
this._data = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed now (sorry was missing in my diff).

const segmentAggs = this._vis.aggs.bySchemaName.segment;
if (segmentAggs && segmentAggs.length > 0) {
this._bucketAgg = segmentAggs[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,7 @@ describe('Terms Agg Other bucket helper', () => {
vis.aggs[0],
otherAggConfig);

expect(mergedResponse.aggregations['1'].buckets[3].key).to.equal('Other');
expect(mergedResponse.aggregations['1'].buckets[3].filters.length).to.equal(2);
expect(mergedResponse.aggregations['1'].buckets[3].key).to.equal('__other__');
});

it('correctly merges other bucket with nested terms agg', () => {
Expand All @@ -255,8 +254,7 @@ describe('Terms Agg Other bucket helper', () => {
const mergedResponse = mergeOtherBucketAggResponse(vis.aggs, nestedTermResponse,
nestedOtherResponse, vis.aggs[1], otherAggConfig);

expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].key).to.equal('Other');
expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].filters.length).to.equal(2);
expect(mergedResponse.aggregations['1'].buckets[1]['2'].buckets[3].key).to.equal('__other__');
});

});
Expand All @@ -265,17 +263,8 @@ describe('Terms Agg Other bucket helper', () => {
it('correctly updates missing bucket key', () => {
init(visConfigNestedTerm);
const updatedResponse = updateMissingBucket(singleTermResponse, vis.aggs, vis.aggs[0]);
expect(updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === 'Missing')).to.not.be('undefined');
expect(updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === '__missing__')).to.not.be('undefined');
});

it('correctly sets the bucket filter', () => {
const updatedResponse = updateMissingBucket(singleTermResponse, vis.aggs, vis.aggs[0]);
const missingBucket = updatedResponse.aggregations['1'].buckets.find(bucket => bucket.key === 'Missing');
expect(missingBucket.filters).to.not.be('undefined');
expect(missingBucket.filters[0]).to.eql({
meta: { index: 'logstash-*', negate: true },
exists: { field: 'geo.src' }
});
});
});
});
31 changes: 31 additions & 0 deletions src/ui/public/agg_types/__tests__/buckets/create_filter/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,36 @@ describe('AggConfig Filters', function () {
expect(filterTrue.query.match).to.have.property('ssl');
expect(filterTrue.query.match.ssl).to.have.property('query', true);
});

it('should generate correct __missing__ filter', () => {
const vis = new Vis(indexPattern, {
type: 'histogram',
aggs: [ { type: 'terms', schema: 'segment', params: { field: '_type' } } ]
});
const aggConfig = vis.aggs.byTypeName.terms[0];
const filter = createFilterTerms(aggConfig, '__missing__');
expect(filter).to.have.property('exists');
expect(filter.exists).to.have.property('field', '_type');
expect(filter).to.have.property('meta');
expect(filter.meta).to.have.property('index', indexPattern.id);
expect(filter.meta).to.have.property('negate', true);
});

it('should generate correct __other__ filter', () => {
const vis = new Vis(indexPattern, {
type: 'histogram',
aggs: [ { type: 'terms', schema: 'segment', params: { field: '_type' } } ]
});
const aggConfig = vis.aggs.byTypeName.terms[0];
const filter = createFilterTerms(aggConfig, '__other__', { terms: ['apache'] })[0];
expect(filter).to.have.property('query');
expect(filter.query).to.have.property('bool');
expect(filter.query.bool).to.have.property('should');
expect(filter.query.bool.should[0]).to.have.property('match_phrase');
expect(filter.query.bool.should[0].match_phrase).to.have.property('_type', 'apache');
expect(filter).to.have.property('meta');
expect(filter.meta).to.have.property('index', indexPattern.id);
expect(filter.meta).to.have.property('negate', true);
});
});
});
30 changes: 30 additions & 0 deletions src/ui/public/agg_types/__tests__/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,36 @@ describe('Terms Agg', function () {
expect($rootScope.agg.params.orderBy).to.be('_key');
});

describe('custom field formatter', () => {
beforeEach(() => {
init({
responseValueAggs: [
{
id: 'agg1',
type: {
name: 'count'
}
}
],
aggParams: {
otherBucketLabel: 'Other',
missingBucketLabel: 'Missing'
}
});
$rootScope.$digest();
});

it ('converts __other__ key', () => {
const formatter = $rootScope.agg.type.getFormat($rootScope.agg).getConverterFor('text');
expect(formatter('__other__')).to.be('Other');
});

it ('converts __missing__ key', () => {
const formatter = $rootScope.agg.type.getFormat($rootScope.agg).getConverterFor('text');
expect(formatter('__missing__')).to.be('Missing');
});
});

it('adds "custom metric" option');
it('lists all metric agg responses');
it('lists individual values of a multi-value metric');
Expand Down
17 changes: 9 additions & 8 deletions src/ui/public/agg_types/buckets/_terms_other_bucket_helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@

import _ from 'lodash';
import { AggConfig } from '../../vis/agg_config';
import { buildPhrasesFilter } from '../../filter_manager/lib/phrases';
import { buildExistsFilter } from '../../filter_manager/lib/exists';
import { buildPhrasesFilter } from '../../filter_manager/lib/phrases';
import { buildQueryFromFilters } from '../../courier/data_source/build_query/from_filters';

/**
Expand Down Expand Up @@ -102,6 +102,7 @@ const getOtherAggTerms = (requestAgg, key, otherAgg) => {
);
};


const buildOtherBucketAgg = (aggConfigs, aggWithOtherBucket, response) => {
const bucketAggs = aggConfigs.filter(agg => agg.type.type === 'buckets');
const index = bucketAggs.findIndex(agg => agg.id === aggWithOtherBucket.id);
Expand Down Expand Up @@ -175,12 +176,11 @@ const mergeOtherBucketAggResponse = (aggsConfig, response, otherResponse, otherA
const phraseFilter = buildPhrasesFilter(otherAgg.params.field, requestFilterTerms, otherAgg.params.field.indexPattern);
phraseFilter.meta.negate = true;
bucket.filters = [ phraseFilter ];
bucket.key = otherAgg.params.otherBucketLabel;
bucket.key = '__other__';
Copy link
Member

Choose a reason for hiding this comment

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

What about moving __other__ and __missing__ strings to an enum object somewhere?
It can be easily refactored in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tim is gonna try with using symbols in a separate PR


if (aggResultBuckets.some(bucket => bucket.key === '__missing__')) {
bucket.filters.push(buildExistsFilter(otherAgg.params.field, otherAgg.params.field.indexPattern));
}

aggResultBuckets.push(bucket);
});
return updatedResponse;
Expand All @@ -190,12 +190,13 @@ const updateMissingBucket = (response, aggConfigs, agg) => {
const updatedResponse = _.cloneDeep(response);
const aggResultBuckets = getAggConfigResultMissingBuckets(updatedResponse.aggregations, agg.id);
aggResultBuckets.forEach(bucket => {
bucket.key = agg.params.missingBucketLabel;
const existsFilter = buildExistsFilter(agg.params.field, agg.params.field.indexPattern);
existsFilter.meta.negate = true;
bucket.filters = [ existsFilter ];
bucket.key = '__missing__';
Copy link
Member

Choose a reason for hiding this comment

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

Same as comment above

});
return updatedResponse;
};

export { buildOtherBucketAgg, mergeOtherBucketAggResponse, updateMissingBucket };
export {
buildOtherBucketAgg,
mergeOtherBucketAggResponse,
updateMissingBucket,
};
27 changes: 25 additions & 2 deletions src/ui/public/agg_types/buckets/create_filter/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,30 @@
*/

import { buildPhraseFilter } from '../../../filter_manager/lib/phrase';
import { buildPhrasesFilter } from '../../../filter_manager/lib/phrases';
import { buildExistsFilter } from '../../../filter_manager/lib/exists';

export function createFilterTerms(aggConfig, key) {
return buildPhraseFilter(aggConfig.params.field, key, aggConfig.vis.indexPattern);
export function createFilterTerms(aggConfig, key, params) {
const field = aggConfig.params.field;
const indexPattern = field.indexPattern;

if (key === '__other__') {
const terms = params.terms;

const phraseFilter = buildPhrasesFilter(field, terms, indexPattern);
phraseFilter.meta.negate = true;

const filters = [phraseFilter];

if (terms.some(term => term === '__missing__')) {
filters.push(buildExistsFilter(field, indexPattern));
}

return filters;
} else if (key === '__missing__') {
const existsFilter = buildExistsFilter(field, indexPattern);
existsFilter.meta.negate = true;
return existsFilter;
}
return buildPhraseFilter(field, key, indexPattern);
}
11 changes: 11 additions & 0 deletions src/ui/public/agg_types/buckets/terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,17 @@ export const termsBucketAgg = new BucketAggType({
const params = agg.params;
return agg.getFieldDisplayName() + ': ' + params.order.display;
},
getFormat: function (bucket) {
return {
getConverterFor: (type) => {
return (val) => {
if (val === '__other__') return bucket.params.otherBucketLabel;
if (val === '__missing__') return bucket.params.missingBucketLabel;
Copy link
Member

Choose a reason for hiding this comment

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

Needs to follow styleguides on braces for conditionals

return bucket.params.field.format.getConverterFor(type)(val);
Copy link
Contributor

@timroes timroes Jun 19, 2018

Choose a reason for hiding this comment

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

Slight performance improvement, move bucket.params.field.format.getConverterFor(type) into gerConverterFor above the return, i.e.

  getFormat: function (bucket) {
    return {
      getConverterFor: (type) => {
        const converter = bucket.params.field.format.getConverterFor(type);
        return (val) => {
          if (val === '__other__') return bucket.params.otherBucketLabel;
          if (val === '__missing__') return bucket.params.missingBucketLabel;
          return converter(val);
        };
      }
    };
  },

};
}
};
},
createFilter: createFilterTerms,
postFlightRequest: async (resp, aggConfigs, aggConfig, nestedSearchSource) => {
if (aggConfig.params.otherBucket) {
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/__tests__/_agg_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ describe('AggConfig', function () {
type: 'table',
aggs: [
{
type: 'terms',
type: 'histogram',
schema: 'bucket',
params: { field: 'bytes' }
}
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/vis/agg_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ AggConfig.prototype.isFilterable = function () {
return _.isFunction(this.type.createFilter);
};

AggConfig.prototype.createFilter = function (key) {
AggConfig.prototype.createFilter = function (key, params = {}) {
if (!this.isFilterable()) {
throw new TypeError('The "' + this.type.title + '" aggregation does not support filtering.');
}
Expand All @@ -197,7 +197,7 @@ AggConfig.prototype.createFilter = function (key) {
throw new TypeError(message);
}

return this.type.createFilter(this, key);
return this.type.createFilter(this, key, params);
};

/**
Expand Down
23 changes: 22 additions & 1 deletion src/ui/public/vis/vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ import { queryManagerFactory } from '../query_manager';
import { SearchSourceProvider } from '../courier/data_source/search_source';
import { SavedObjectsClientProvider } from '../saved_objects';

const getTerms = (table, columnIndex, rowIndex) => {
return [...new Set(
table.rows.filter(row => {
if (row[columnIndex] === '__other__') return false;
return row.every((cell, i) => cell === table.rows[rowIndex][i] || i >= columnIndex);
}).map(row => row[columnIndex])
)];
};

export function VisProvider(Private, Promise, indexPatterns, timefilter, getAppState) {
const visTypes = Private(VisTypesRegistryProvider);
const brushEvent = Private(UtilsBrushEventProvider);
Expand Down Expand Up @@ -75,9 +84,21 @@ export function VisProvider(Private, Promise, indexPatterns, timefilter, getAppS
queryFilter: queryFilter,
queryManager: queryManagerFactory(getAppState),
events: {
filter: (event) => {
filter_legacy: (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please add a comment to this, why it's legacy, and that it should not be used anymore, because we will remove it at some point.

const appState = getAppState();
filterBarClickHandler(appState)(event);
},
filter: (data, columnIndex, rowIndex) => {
const agg = data.columns[columnIndex].aggConfig;
let filter = [];
const value = data.rows[rowIndex][columnIndex];
if (agg.type.name === 'terms' && agg.params.otherBucket) {
const terms = getTerms(data, columnIndex, rowIndex);
filter = agg.createFilter(value, { terms });
} else {
filter = agg.createFilter(value);
}
queryFilter.addFilters(filter);
}, brush: (event) => {
const appState = getAppState();
brushEvent(appState)(event);
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/vis_types/vislib_vis_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export function VislibVisTypeProvider(Private, $rootScope, $timeout, $compile) {

this.vis.vislibVis = new vislib.Vis(this.chartEl, this.vis.params);
this.vis.vislibVis.on('brush', this.vis.API.events.brush);
this.vis.vislibVis.on('click', this.vis.API.events.filter);
this.vis.vislibVis.on('click', this.vis.API.events.filter_legacy);
this.vis.vislibVis.on('renderComplete', resolve);
this.vis.vislibVis.render(esResponse, this.vis.getUiState());

Expand Down