From aab98ae4f4d0d4174cc44fa27bfbcc2e1e456b94 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Thu, 11 Apr 2019 14:24:53 +0200 Subject: [PATCH] Don't save the current timezone in visualizations (#34795) * Don't save the current timezone in visualizations * Add additional test * Add test and switch migration number * Don't clone object according to review feedback * Better documentation * Update src/legacy/core_plugins/kibana/migrations.js Co-Authored-By: timroes --- src/legacy/core_plugins/kibana/migrations.js | 39 ++++++ .../core_plugins/kibana/migrations.test.js | 118 ++++++++++++++++++ .../agg_types/buckets/date_histogram.js | 7 ++ 3 files changed, 164 insertions(+) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index 4842c7eeb4bb..4e153e587ee9 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -57,6 +57,33 @@ function migrateIndexPattern(doc) { doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource); } +function removeDateHistogramTimeZones(doc) { + const visStateJSON = get(doc, 'attributes.visState'); + if (visStateJSON) { + let visState; + try { + visState = JSON.parse(visStateJSON); + } catch (e) { + // Let it go, the data is invalid and we'll leave it as is + } + if (visState && visState.aggs) { + visState.aggs.forEach(agg => { + // We're checking always for the existance of agg.params here. This should always exist, but better + // be safe then sorry during migrations. + if (agg.type === 'date_histogram' && agg.params) { + delete agg.params.time_zone; + } + + if (get(agg, 'params.customBucket.type', null) === 'date_histogram' && agg.params.customBucket.params) { + delete agg.params.customBucket.params.time_zone; + } + }); + doc.attributes.visState = JSON.stringify(visState); + } + } + return doc; +} + export const migrations = { 'index-pattern': { '6.5.0': (doc) => { @@ -66,6 +93,17 @@ export const migrations = { } }, visualization: { + /** + * We need to have this migration twice, once with a version prior to 7.0.0 once with a version + * after it. The reason for that is, that this migration has been introduced once 7.0.0 was already + * released. Thus a user who already had 7.0.0 installed already got the 7.0.0 migrations below running, + * so we need a version higher than that. But this fix was backported to the 6.7 release, meaning if we + * would only have the 7.0.1 migration in here a user on the 6.7 release will migrate their saved objects + * to the 7.0.1 state, and thus when updating their Kibana to 7.0, will never run the 7.0.0 migrations introduced + * in that version. So we apply this twice, once with 6.7.2 and once with 7.0.1 while the backport to 6.7 + * only contained the 6.7.2 migration and not the 7.0.1 migration. + */ + '6.7.2': removeDateHistogramTimeZones, '7.0.0': (doc) => { // Set new "references" attribute doc.references = doc.references || []; @@ -146,6 +184,7 @@ export const migrations = { throw new Error(`Failure attempting to migrate saved object '${doc.attributes.title}' - ${e}`); } }, + '7.0.1': removeDateHistogramTimeZones, '7.1.0': doc => { // [TSVB] Migrate percentile-rank aggregation (value -> values) const migratePercentileRankAggregation = doc => { diff --git a/src/legacy/core_plugins/kibana/migrations.test.js b/src/legacy/core_plugins/kibana/migrations.test.js index b5e75af68825..f09696174ae7 100644 --- a/src/legacy/core_plugins/kibana/migrations.test.js +++ b/src/legacy/core_plugins/kibana/migrations.test.js @@ -59,6 +59,124 @@ Object { }); describe('visualization', () => { + describe('date histogram time zone removal', () => { + const migrate = doc => migrations.visualization['6.7.2'](doc); + let doc; + beforeEach(() => { + doc = { + attributes: { + visState: JSON.stringify({ + aggs: [ + { + 'enabled': true, + 'id': '1', + 'params': { + // Doesn't make much sense but we want to test it's not removing it from anything else + time_zone: 'Europe/Berlin', + }, + 'schema': 'metric', + 'type': 'count' + }, + { + 'enabled': true, + 'id': '2', + 'params': { + 'customInterval': '2h', + 'drop_partials': false, + 'extended_bounds': {}, + 'field': 'timestamp', + 'time_zone': 'Europe/Berlin', + 'interval': 'auto', + 'min_doc_count': 1, + 'useNormalizedEsInterval': true + }, + 'schema': 'segment', + 'type': 'date_histogram' + }, + { + 'enabled': true, + 'id': '4', + 'params': { + 'customInterval': '2h', + 'drop_partials': false, + 'extended_bounds': {}, + 'field': 'timestamp', + 'interval': 'auto', + 'min_doc_count': 1, + 'useNormalizedEsInterval': true + }, + 'schema': 'segment', + 'type': 'date_histogram' + }, + { + 'enabled': true, + 'id': '3', + 'params': { + 'customBucket': { + 'enabled': true, + 'id': '1-bucket', + 'params': { + 'customInterval': '2h', + 'drop_partials': false, + 'extended_bounds': {}, + 'field': 'timestamp', + 'interval': 'auto', + 'min_doc_count': 1, + 'time_zone': 'Europe/Berlin', + 'useNormalizedEsInterval': true + }, + 'type': 'date_histogram' + }, + 'customMetric': { + 'enabled': true, + 'id': '1-metric', + 'params': {}, + 'type': 'count' + } + }, + 'schema': 'metric', + 'type': 'max_bucket' + }, + ] + }), + } + }; + }); + + it('should remove time_zone from date_histogram aggregations', () => { + const migratedDoc = migrate(doc); + const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; + expect(aggs[1]).not.toHaveProperty('params.time_zone'); + }); + + it('should not remove time_zone from non date_histogram aggregations', () => { + const migratedDoc = migrate(doc); + const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; + expect(aggs[0]).toHaveProperty('params.time_zone'); + }); + + it('should remove time_zone from nested aggregations', () => { + const migratedDoc = migrate(doc); + const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; + expect(aggs[3]).not.toHaveProperty('params.customBucket.params.time_zone'); + }); + + it('should not fail on date histograms without a time_zone', () => { + const migratedDoc = migrate(doc); + const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; + expect(aggs[2]).not.toHaveProperty('params.time_zone'); + }); + + it('should be able to apply the migration twice, since we need it for 6.7.2 and 7.0.1', () => { + const migratedDoc = migrate(migrate(doc)); + const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; + expect(aggs[1]).not.toHaveProperty('params.time_zone'); + expect(aggs[0]).toHaveProperty('params.time_zone'); + expect(aggs[3]).not.toHaveProperty('params.customBucket.params.time_zone'); + expect(aggs[2]).not.toHaveProperty('params.time_zone'); + }); + }); + describe('7.0.0', () => { const migrate = doc => migrations.visualization['7.0.0'](doc); const generateDoc = ({ type, aggs }) => ({ diff --git a/src/legacy/ui/public/agg_types/buckets/date_histogram.js b/src/legacy/ui/public/agg_types/buckets/date_histogram.js index 3f345f7fd9a1..c3c13c823f58 100644 --- a/src/legacy/ui/public/agg_types/buckets/date_histogram.js +++ b/src/legacy/ui/public/agg_types/buckets/date_histogram.js @@ -160,6 +160,13 @@ export const dateHistogramBucketAgg = new BucketAggType({ const isDefaultTimezone = config.isDefault('dateFormat:tz'); return isDefaultTimezone ? detectedTimezone || tzOffset : config.get('dateFormat:tz'); }, + serialize() { + // We don't want to store the `time_zone` parameter ever in the saved object for the visualization. + // If we would store this changing the time zone in Kibana would not affect any already saved visualizations + // anymore, which is not the desired behavior. So always returning undefined here, makes sure we're never + // saving that parameter and just keep it "transient". + return undefined; + }, }, { name: 'drop_partials',