From 130a4723ef48795b805645159f1986992fea2dc6 Mon Sep 17 00:00:00 2001 From: Spencer Alger Date: Thu, 13 Nov 2014 14:49:41 -0700 Subject: [PATCH 1/5] [aggConfig] make the id part of state --- src/kibana/components/vis/_agg_config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/kibana/components/vis/_agg_config.js b/src/kibana/components/vis/_agg_config.js index 80e79549f345bc..3f9749ea29eb70 100644 --- a/src/kibana/components/vis/_agg_config.js +++ b/src/kibana/components/vis/_agg_config.js @@ -118,6 +118,7 @@ define(function (require) { }, {}); return { + id: self.id, type: self.type && self.type.name, schema: self.schema && self.schema.name, params: outParams From ab92bc924bdd2c0ebe82ae9ea4b6c5ccc0082120 Mon Sep 17 00:00:00 2001 From: Spencer Alger Date: Thu, 13 Nov 2014 14:50:50 -0700 Subject: [PATCH 2/5] [aggConfig] read the id from state, pick ids for new states --- src/kibana/components/vis/_agg_config.js | 35 ++++++++++++++++++++++- src/kibana/components/vis/_agg_configs.js | 8 ++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/kibana/components/vis/_agg_config.js b/src/kibana/components/vis/_agg_config.js index 3f9749ea29eb70..9970a6644ca466 100644 --- a/src/kibana/components/vis/_agg_config.js +++ b/src/kibana/components/vis/_agg_config.js @@ -6,7 +6,7 @@ define(function (require) { function AggConfig(vis, opts) { var self = this; - self.id = _.uniqueId('agg_'); + self.id = opts.id || AggConfig.nextId(vis.aggs); self.vis = vis; self._opts = opts = (opts || {}); @@ -26,6 +26,39 @@ define(function (require) { self.fillDefaults(opts.params); } + /** + * Ensure that all of the objects in the list have ids, the objects + * and list are modified by reference. + * + * @param {array[object]} list - a list of objects, objects can be anything really + * @return {array} - the list that was passed in + */ + AggConfig.ensureIds = function (list) { + var have = []; + var haveNot = []; + list.forEach(function (obj) { + (obj.id ? have : haveNot).push(obj); + }); + + var nextId = AggConfig.nextId(have); + haveNot.forEach(function (obj) { + obj.id = nextId++; + }); + + return list; + }; + + /** + * Calculate the next id based on the ids in this list + * + * @return {array} list - a list of objects with id properties + */ + AggConfig.nextId = function (list) { + return 1 + list.reduce(function (max, obj) { + return Math.max(max, obj.id || 0); + }, 0); + }; + /** * Write the current values to this.params, filling in the defaults as we go * diff --git a/src/kibana/components/vis/_agg_configs.js b/src/kibana/components/vis/_agg_configs.js index 331f95efad6e1f..690be891670cff 100644 --- a/src/kibana/components/vis/_agg_configs.js +++ b/src/kibana/components/vis/_agg_configs.js @@ -9,11 +9,12 @@ define(function (require) { var self = this; this.vis = vis; + configStates = AggConfig.ensureIds(configStates || []); AggConfigs.Super.call(this, { index: ['id'], group: ['schema.group', 'type.name', 'schema.name'], - initialSet: (configStates || []).map(function (aggConfigState) { + initialSet: configStates.map(function (aggConfigState) { if (aggConfigState instanceof AggConfig) return aggConfigState; return new AggConfig(vis, aggConfigState); }) @@ -33,8 +34,9 @@ define(function (require) { .each(function (schema) { if (!self.bySchemaName[schema.name]) { var defaults = schema.defaults.slice(0, schema.max); - _.each(defaults, function (def) { - self.push(new AggConfig(vis, def)); + _.each(defaults, function (defaultState) { + var state = _.defaults({ id: AggConfig.nextId(self) }, defaultState); + self.push(new AggConfig(vis, state)); }); } }); From 76236421966970c4695cfb0dedd66840a81e348b Mon Sep 17 00:00:00 2001 From: Spencer Alger Date: Thu, 13 Nov 2014 14:51:48 -0700 Subject: [PATCH 3/5] [discover/segmented_fetch] itterate the agg keys, rather than trying to find them --- .../plugins/discover/_segmented_fetch.js | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/kibana/plugins/discover/_segmented_fetch.js b/src/kibana/plugins/discover/_segmented_fetch.js index 011868999945f0..a1c9dd59b98a15 100644 --- a/src/kibana/plugins/discover/_segmented_fetch.js +++ b/src/kibana/plugins/discover/_segmented_fetch.js @@ -315,30 +315,30 @@ define(function (require) { if (!resp.aggregations) return; - var aggKey = _.find(Object.keys(resp.aggregations), function (key) { - return key.substr(0, 4) === 'agg_'; - }); - - if (!aggKey) throw new Error('aggKey not found in response: ' + Object.keys(resp.aggregations)); + Object.keys(resp.aggregations).forEach(function (aggKey) { - // start merging aggregations - if (!requestStats.aggregations) { - requestStats.aggregations = {}; - requestStats.aggregations[aggKey] = { - buckets: [] - }; - requestStats._bucketIndex = {}; - } + if (!requestStats.aggregations) { + // start merging aggregations + requestStats.aggregations = {}; + requestStats._bucketIndex = {}; + } - resp.aggregations[aggKey].buckets.forEach(function (bucket) { - var mbucket = requestStats._bucketIndex[bucket.key]; - if (mbucket) { - mbucket.doc_count += bucket.doc_count; - return; + if (!requestStats.aggregations[aggKey]) { + requestStats.aggregations[aggKey] = { + buckets: [] + }; } - mbucket = requestStats._bucketIndex[bucket.key] = bucket; - requestStats.aggregations[aggKey].buckets.push(mbucket); + resp.aggregations[aggKey].buckets.forEach(function (bucket) { + var mbucket = requestStats._bucketIndex[bucket.key]; + if (mbucket) { + mbucket.doc_count += bucket.doc_count; + return; + } + + mbucket = requestStats._bucketIndex[bucket.key] = bucket; + requestStats.aggregations[aggKey].buckets.push(mbucket); + }); }); } From 35dab6c4b123e65911594f2e8ec95f3b39745651 Mon Sep 17 00:00:00 2001 From: Spencer Alger Date: Thu, 13 Nov 2014 16:33:58 -0700 Subject: [PATCH 4/5] [aggConfig] test the placement and export of ids --- test/unit/specs/components/vis/_agg_config.js | 106 +++++++++++++++++- .../unit/specs/components/vis/_agg_configs.js | 25 ++++- 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/test/unit/specs/components/vis/_agg_config.js b/test/unit/specs/components/vis/_agg_config.js index 4243bf626656ee..99475ade6ead2b 100644 --- a/test/unit/specs/components/vis/_agg_config.js +++ b/test/unit/specs/components/vis/_agg_config.js @@ -3,12 +3,14 @@ define(function (require) { var sinon = require('test_utils/auto_release_sinon'); var Vis; + var AggType; var AggConfig; var indexPattern; beforeEach(module('kibana')); beforeEach(inject(function (Private) { Vis = Private(require('components/vis/vis')); + AggType = Private(require('components/agg_types/_agg_type')); AggConfig = Private(require('components/vis/_agg_config')); indexPattern = Private(require('fixtures/stubbed_logstash_index_pattern')); })); @@ -50,7 +52,6 @@ define(function (require) { expect(dsl).to.have.property('date_histogram'); }); - it('uses the params from #write() output as the agg params', function () { var vis = new Vis(indexPattern, { type: 'histogram', @@ -102,5 +103,108 @@ define(function (require) { expect(dsl.aggs[avgConfig.id].avg).to.be(football); }); }); + + describe('::ensureIds', function () { + it('accepts an array of objects and assigns ids to them', function () { + var objs = [ + {}, + {}, + {}, + {} + ]; + AggConfig.ensureIds(objs); + expect(objs[0]).to.have.property('id', 1); + expect(objs[1]).to.have.property('id', 2); + expect(objs[2]).to.have.property('id', 3); + expect(objs[3]).to.have.property('id', 4); + }); + + it('assigns ids relative to the other items in the list', function () { + var objs = [ + { id: 100 }, + {}, + ]; + AggConfig.ensureIds(objs); + expect(objs[0]).to.have.property('id', 100); + expect(objs[1]).to.have.property('id', 101); + }); + + it('assigns ids relative to the other items in the list', function () { + var objs = [ + { id: 100 }, + { id: 200 }, + { id: 500 }, + { id: 350 }, + {}, + ]; + AggConfig.ensureIds(objs); + expect(objs[0]).to.have.property('id', 100); + expect(objs[1]).to.have.property('id', 200); + expect(objs[2]).to.have.property('id', 500); + expect(objs[3]).to.have.property('id', 350); + expect(objs[4]).to.have.property('id', 501); + }); + + it('uses ::nextId to get the starting value', function () { + sinon.stub(AggConfig, 'nextId').returns(534); + var objs = AggConfig.ensureIds([{}]); + expect(objs[0]).to.have.property('id', 534); + }); + + it('only calls ::nextId once', function () { + var start = 420; + sinon.stub(AggConfig, 'nextId').returns(start); + var objs = AggConfig.ensureIds([{}, {}, {}, {}, {}, {}, {}]); + + expect(AggConfig.nextId).to.have.property('callCount', 1); + objs.forEach(function (obj, i) { + expect(obj).to.have.property('id', start + i); + }); + }); + }); + + describe('::nextId', function () { + it('accepts a list of objects and picks the next id', function () { + var next = AggConfig.nextId([ {id: 100}, {id: 500} ]); + expect(next).to.be(501); + }); + + it('handles an empty list', function () { + var next = AggConfig.nextId([]); + expect(next).to.be(1); + }); + + it('fails when the list is not defined', function () { + expect(function () { + AggConfig.nextId(); + }).to.throwError(); + }); + }); + + describe('#toJSON', function () { + it('includes the aggs id, params, type and schema', function () { + var vis = new Vis(indexPattern, { + type: 'histogram', + aggs: [ + { + type: 'date_histogram', + schema: 'segment' + } + ] + }); + + var aggConfig = vis.aggs.byTypeName.date_histogram[0]; + expect(aggConfig.id).to.be(1); + expect(aggConfig.params).to.be.an('object'); + expect(aggConfig.type).to.be.an(AggType).and.have.property('name', 'date_histogram'); + expect(aggConfig.schema).to.be.an('object').and.have.property('name', 'segment'); + + var state = aggConfig.toJSON(); + expect(state).to.have.property('id', 1); + expect(state.params).to.be.an('object'); + expect(state).to.have.property('type', 'date_histogram'); + expect(state).to.have.property('schema', 'segment'); + }); + }); }]; }); \ No newline at end of file diff --git a/test/unit/specs/components/vis/_agg_configs.js b/test/unit/specs/components/vis/_agg_configs.js index 483e53720d9231..f125ac31644534 100644 --- a/test/unit/specs/components/vis/_agg_configs.js +++ b/test/unit/specs/components/vis/_agg_configs.js @@ -54,7 +54,7 @@ define(function (require) { type: 'date_histogram', schema: 'segment' }, - new AggConfig({ + new AggConfig(vis, { type: 'terms', schema: 'split' }) @@ -64,6 +64,29 @@ define(function (require) { expect(SpiedAggConfig).to.have.property('callCount', 3); }); + it('attemps to ensure that all states have an id', function () { + var vis = new Vis(indexPattern, { + type: 'histogram', + aggs: [] + }); + + var states = [ + { + type: 'date_histogram', + schema: 'segment' + }, + { + type: 'terms', + schema: 'split' + } + ]; + + var spy = sinon.spy(SpiedAggConfig, 'ensureIds'); + var ac = new AggConfigs(vis, states); + expect(spy.callCount).to.be(1); + expect(spy.firstCall.args[0]).to.be(states); + }); + describe('defaults', function () { var vis; beforeEach(function () { From 5d790c4d3a9be5c9fca5de877b09a167e1d6549e Mon Sep 17 00:00:00 2001 From: Spencer Alger Date: Thu, 13 Nov 2014 16:57:57 -0700 Subject: [PATCH 5/5] [editor/nestingIndicator] use the id of the item if available --- src/kibana/plugins/visualize/editor/nesting_indicator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kibana/plugins/visualize/editor/nesting_indicator.js b/src/kibana/plugins/visualize/editor/nesting_indicator.js index e6621837cd7061..c7c379362f3ec0 100644 --- a/src/kibana/plugins/visualize/editor/nesting_indicator.js +++ b/src/kibana/plugins/visualize/editor/nesting_indicator.js @@ -11,7 +11,7 @@ define(function (require) { var colorPool = Private(require('components/vislib/components/color/color_palette'))(100); var assigned = {}; return function (item) { - var key = item.$$hashKey; + var key = item.id || item.$$hashKey; if (!key) throw new Error('expected an item that is part of an ngRepeat'); if (!assigned[key]) {