From 816d44298f453927b1a73057e0b369310968b6e3 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Tue, 9 Apr 2019 16:39:52 +0200 Subject: [PATCH 1/6] Don't save the current timezone in visualizations --- src/legacy/core_plugins/kibana/migrations.js | 30 +++++++ .../core_plugins/kibana/migrations.test.js | 88 +++++++++++++++++++ .../agg_types/buckets/date_histogram.js | 7 ++ 3 files changed, 125 insertions(+) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index 4842c7eeb4bba..e84074446fcc9 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -57,6 +57,34 @@ function migrateIndexPattern(doc) { doc.attributes.kibanaSavedObjectMeta.searchSourceJSON = JSON.stringify(searchSource); } +function removeDateHistogramTimeZones(doc) { + const newDoc = cloneDeep(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; + } + }); + newDoc.attributes.visState = JSON.stringify(visState); + } + } + return newDoc; +} + export const migrations = { 'index-pattern': { '6.5.0': (doc) => { @@ -66,6 +94,7 @@ export const migrations = { } }, visualization: { + '6.7.0': removeDateHistogramTimeZones, '7.0.0': (doc) => { // Set new "references" attribute doc.references = doc.references || []; @@ -146,6 +175,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 b5e75af68825a..b96b503bf5787 100644 --- a/src/legacy/core_plugins/kibana/migrations.test.js +++ b/src/legacy/core_plugins/kibana/migrations.test.js @@ -59,6 +59,94 @@ Object { }); describe('visualization', () => { + describe('date histogram time zone removal', () => { + const migrate = doc => migrations.visualization['6.7.0'](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': '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[2]).not.toHaveProperty('params.customBucket.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 3f345f7fd9a16..c3c13c823f581 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', From 0ae6ed019cef8ce5a5fdf6262c5ae525070d9cd2 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Tue, 9 Apr 2019 16:53:48 +0200 Subject: [PATCH 2/6] Add additional test --- .../core_plugins/kibana/migrations.test.js | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/legacy/core_plugins/kibana/migrations.test.js b/src/legacy/core_plugins/kibana/migrations.test.js index b96b503bf5787..76f40248d7980 100644 --- a/src/legacy/core_plugins/kibana/migrations.test.js +++ b/src/legacy/core_plugins/kibana/migrations.test.js @@ -93,6 +93,21 @@ describe('visualization', () => { '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', @@ -143,7 +158,13 @@ describe('visualization', () => { it('should remove time_zone from nested aggregations', () => { const migratedDoc = migrate(doc); const aggs = JSON.parse(migratedDoc.attributes.visState).aggs; - expect(aggs[2]).not.toHaveProperty('params.customBucket.params.time_zone'); + 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'); }); }); From 8c91d8ee7fbe13fde6dc3c74f3d2faf9bdb245cc Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Tue, 9 Apr 2019 16:57:29 +0200 Subject: [PATCH 3/6] Add test and switch migration number --- src/legacy/core_plugins/kibana/migrations.js | 2 +- src/legacy/core_plugins/kibana/migrations.test.js | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index e84074446fcc9..2ae9e7047ccc0 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -94,7 +94,7 @@ export const migrations = { } }, visualization: { - '6.7.0': removeDateHistogramTimeZones, + '6.7.2': removeDateHistogramTimeZones, '7.0.0': (doc) => { // Set new "references" attribute doc.references = doc.references || []; diff --git a/src/legacy/core_plugins/kibana/migrations.test.js b/src/legacy/core_plugins/kibana/migrations.test.js index 76f40248d7980..f09696174ae78 100644 --- a/src/legacy/core_plugins/kibana/migrations.test.js +++ b/src/legacy/core_plugins/kibana/migrations.test.js @@ -60,7 +60,7 @@ Object { describe('visualization', () => { describe('date histogram time zone removal', () => { - const migrate = doc => migrations.visualization['6.7.0'](doc); + const migrate = doc => migrations.visualization['6.7.2'](doc); let doc; beforeEach(() => { doc = { @@ -166,6 +166,15 @@ describe('visualization', () => { 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', () => { From 53cda841b54443265c17ceef806d35495448eb41 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Wed, 10 Apr 2019 14:39:32 +0200 Subject: [PATCH 4/6] Don't clone object according to review feedback --- src/legacy/core_plugins/kibana/migrations.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index 2ae9e7047ccc0..79a35e0eaa13a 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -58,7 +58,6 @@ function migrateIndexPattern(doc) { } function removeDateHistogramTimeZones(doc) { - const newDoc = cloneDeep(doc); const visStateJSON = get(doc, 'attributes.visState'); if (visStateJSON) { let visState; @@ -79,10 +78,10 @@ function removeDateHistogramTimeZones(doc) { delete agg.params.customBucket.params.time_zone; } }); - newDoc.attributes.visState = JSON.stringify(visState); + doc.attributes.visState = JSON.stringify(visState); } } - return newDoc; + return doc; } export const migrations = { From 91beb2f1f9a5287ffb6ce5288dde4715ef618aa0 Mon Sep 17 00:00:00 2001 From: Tim Roes Date: Wed, 10 Apr 2019 18:22:47 +0200 Subject: [PATCH 5/6] Better documentation --- src/legacy/core_plugins/kibana/migrations.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index 79a35e0eaa13a..d3a8931667448 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -93,6 +93,16 @@ export const migrations = { } }, visualization: { + /** + * We need to have this mirgation 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 From 6a2dcd2f4c1aa2c189c467a5aa1ddcd530d21c1e Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Thu, 11 Apr 2019 08:42:05 +0200 Subject: [PATCH 6/6] Update src/legacy/core_plugins/kibana/migrations.js Co-Authored-By: timroes --- src/legacy/core_plugins/kibana/migrations.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js index d3a8931667448..4e153e587ee9a 100644 --- a/src/legacy/core_plugins/kibana/migrations.js +++ b/src/legacy/core_plugins/kibana/migrations.js @@ -94,7 +94,7 @@ export const migrations = { }, visualization: { /** - * We need to have this mirgation twice, once with a version prior to 7.0.0 once with a version + * 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