[6.7] Don't save the current timezone in visualizations (#34795) (#34927

) * 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 <[email protected]> * Fix migrationVersion in integration tests * Fix tests * Fix migrationVersion in tests * Fix more tests
elastic · Apr 15, 2019 · 5e86ec3 · 5e86ec3
1 parent 5d1f4a5
commit 5e86ec3
Show file tree

Hide file tree

Showing 10 changed files with 235 additions and 0 deletions.
diff --git a/src/legacy/core_plugins/kibana/index.js b/src/legacy/core_plugins/kibana/index.js
@@ -21,6 +21,7 @@ import Promise from 'bluebird';
 import { mkdirp as mkdirpNode } from 'mkdirp';
 import { resolve } from 'path';
 
+import { migrations } from './migrations';
 import manageUuid from './server/lib/manage_uuid';
 import { searchApi } from './server/routes/api/search';
 import { scrollSearchApi } from './server/routes/api/scroll_search';
@@ -160,6 +161,8 @@ export default function (kibana) {
 
       mappings,
       uiSettingDefaults: getUiSettingDefaults(),
+
+      migrations,
     },
 
     preInit: async function (server) {

diff --git a/src/legacy/core_plugins/kibana/migrations.js b/src/legacy/core_plugins/kibana/migrations.js
@@ -0,0 +1,63 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { get } from 'lodash';
+
+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 = {
+  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,
+  },
+};
diff --git a/src/legacy/core_plugins/kibana/migrations.test.js b/src/legacy/core_plugins/kibana/migrations.test.js
@@ -0,0 +1,141 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { migrations } from './migrations';
+
+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');
+    });
+  });
+
+});
diff --git a/src/ui/public/agg_types/buckets/date_histogram.js b/src/ui/public/agg_types/buckets/date_histogram.js
@@ -161,6 +161,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',

diff --git a/test/api_integration/apis/saved_objects/bulk_get.js b/test/api_integration/apis/saved_objects/bulk_get.js
@@ -54,6 +54,9 @@ export default function ({ getService }) {
               saved_objects: [
                 {
                   id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab',
+                  migrationVersion: {
+                    visualization: '6.7.2',
+                  },
                   type: 'visualization',
                   updated_at: '2017-09-21T18:51:23.794Z',
                   version: resp.body.saved_objects[0].version,

diff --git a/test/api_integration/apis/saved_objects/create.js b/test/api_integration/apis/saved_objects/create.js
@@ -46,6 +46,9 @@ export default function ({ getService }) {
 
             expect(resp.body).to.eql({
               id: resp.body.id,
+              migrationVersion: {
+                visualization: '6.7.2',
+              },
               type: 'visualization',
               updated_at: resp.body.updated_at,
               version: 'WzgsMV0=',
@@ -84,6 +87,9 @@ export default function ({ getService }) {
 
             expect(resp.body).to.eql({
               id: resp.body.id,
+              migrationVersion: {
+                visualization: '6.7.2',
+              },
               type: 'visualization',
               updated_at: resp.body.updated_at,
               version: 'WzAsMV0=',

diff --git a/test/api_integration/apis/saved_objects/get.js b/test/api_integration/apis/saved_objects/get.js
@@ -36,6 +36,9 @@ export default function ({ getService }) {
           .then(resp => {
             expect(resp.body).to.eql({
               id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab',
+              migrationVersion: {
+                visualization: '6.7.2',
+              },
               type: 'visualization',
               updated_at: '2017-09-21T18:51:23.794Z',
               version: resp.body.version,

diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts
@@ -97,6 +97,9 @@ export function bulkGetTestSuiteFactory(esArchiver: any, supertest: SuperTest<an
         {
           id: `${getIdPrefix(spaceId)}dd7caf20-9efd-11e7-acb3-3dab96693fab`,
           type: 'visualization',
+          migrationVersion: {
+            visualization: '6.7.2',
+          },
           updated_at: '2017-09-21T18:51:23.794Z',
           version: resp.body.saved_objects[0].version,
           attributes: {

diff --git a/x-pack/test/saved_object_api_integration/common/suites/create.ts b/x-pack/test/saved_object_api_integration/common/suites/create.ts
@@ -67,6 +67,9 @@ export function createTestSuiteFactory(es: any, esArchiver: any, supertest: Supe
 
     expect(resp.body).to.eql({
       id: resp.body.id,
+      migrationVersion: {
+        visualization: '6.7.2',
+      },
       type: spaceAwareType,
       updated_at: resp.body.updated_at,
       version: resp.body.version,

diff --git a/x-pack/test/saved_object_api_integration/common/suites/get.ts b/x-pack/test/saved_object_api_integration/common/suites/get.ts
@@ -103,6 +103,9 @@ export function getTestSuiteFactory(esArchiver: any, supertest: SuperTest<any>)
     expect(resp.body).to.eql({
       id: `${getIdPrefix(spaceId)}dd7caf20-9efd-11e7-acb3-3dab96693fab`,
       type: 'visualization',
+      migrationVersion: {
+        visualization: '6.7.2',
+      },
       updated_at: '2017-09-21T18:51:23.794Z',
       version: resp.body.version,
       attributes: {