[7.0] Don't save the current timezone in visualizations (#34795) (#34926)

* 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 <mail@timroes.de>

* Fix migrationVersion in tests

src/legacy/core_plugins/kibana/migrations.js

@@ -57,8 +57,46 @@ 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 = {
   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 || [];
@@ -138,7 +176,8 @@ export const migrations = {
       } catch (e) {
         throw new Error(`Failure attempting to migrate saved object '${doc.attributes.title}' - ${e}`);
       }
-    }
+    },
+    '7.0.1': removeDateHistogramTimeZones,
   },
   dashboard: {
     '7.0.0': (doc) => {

src/legacy/core_plugins/kibana/migrations.test.js

@@ -20,6 +20,124 @@
 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');
+    });
+  });
+
   describe('7.0.0', () => {
     const migrate = doc => migrations.visualization['7.0.0'](doc);
     const generateDoc = ({ type, aggs }) => ({

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',

test/api_integration/apis/saved_objects/bulk_get.js

@@ -55,7 +55,7 @@ export default function ({ getService }) {
                 {
                   id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab',
                   migrationVersion: {
-                    visualization: '7.0.0'
+                    visualization: '7.0.1'
                   },
                   type: 'visualization',
                   updated_at: '2017-09-21T18:51:23.794Z',
@@ -70,7 +70,7 @@ export default function ({ getService }) {
                     kibanaSavedObjectMeta: resp.body.saved_objects[0].attributes.kibanaSavedObjectMeta
                   },
                   migrationVersion: {
-                    visualization: '7.0.0',
+                    visualization: '7.0.1',
                   },
                   references: [{
                     name: 'kibanaSavedObjectMeta.searchSourceJSON.index',

test/api_integration/apis/saved_objects/create.js

@@ -48,7 +48,7 @@ export default function ({ getService }) {
               id: resp.body.id,
               type: 'visualization',
               migrationVersion: {
-                visualization: '7.0.0'
+                visualization: '7.0.1'
               },
               updated_at: resp.body.updated_at,
               version: 'WzgsMV0=',
@@ -56,7 +56,7 @@ export default function ({ getService }) {
                 title: 'My favorite vis'
               },
               migrationVersion: {
-                visualization: '7.0.0',
+                visualization: '7.0.1',
               },
               references: [],
             });
@@ -93,7 +93,7 @@ export default function ({ getService }) {
               id: resp.body.id,
               type: 'visualization',
               migrationVersion: {
-                visualization: '7.0.0'
+                visualization: '7.0.1'
               },
               updated_at: resp.body.updated_at,
               version: 'WzAsMV0=',
@@ -101,7 +101,7 @@ export default function ({ getService }) {
                 title: 'My favorite vis'
               },
               migrationVersion: {
-                visualization: '7.0.0',
+                visualization: '7.0.1',
               },
               references: [],
             });

test/api_integration/apis/saved_objects/find.js

@@ -47,7 +47,7 @@ export default function ({ getService }) {
                     'title': 'Count of requests'
                   },
                   migrationVersion: {
-                    visualization: '7.0.0',
+                    visualization: '7.0.1',
                   },
                   references: [
                     {

test/api_integration/apis/saved_objects/get.js

@@ -37,13 +37,13 @@ export default function ({ getService }) {
             expect(resp.body).to.eql({
               id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab',
               migrationVersion: {
-                visualization: '7.0.0'
+                visualization: '7.0.1'
               },
               type: 'visualization',
               updated_at: '2017-09-21T18:51:23.794Z',
               version: resp.body.version,
               migrationVersion: {
-                visualization: '7.0.0',
+                visualization: '7.0.1',
               },
               attributes: {
                 title: 'Count of requests',

x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts

@@ -90,7 +90,7 @@ export function bulkGetTestSuiteFactory(esArchiver: any, supertest: SuperTest<an
           id: `${getIdPrefix(spaceId)}dd7caf20-9efd-11e7-acb3-3dab96693fab`,
           type: 'visualization',
           migrationVersion: {
-            visualization: '7.0.0',
+            visualization: '7.0.1',
           },
           updated_at: '2017-09-21T18:51:23.794Z',
           version: resp.body.saved_objects[0].version,

x-pack/test/saved_object_api_integration/common/suites/create.ts

@@ -59,7 +59,7 @@ export function createTestSuiteFactory(es: any, esArchiver: any, supertest: Supe
     expect(resp.body).to.eql({
       id: resp.body.id,
       migrationVersion: {
-        visualization: '7.0.0',
+        visualization: '7.0.1',
       },
       type: spaceAwareType,
       updated_at: resp.body.updated_at,

x-pack/test/saved_object_api_integration/common/suites/find.ts

@@ -102,7 +102,7 @@ export function findTestSuiteFactory(esArchiver: any, supertest: SuperTest<any>)
             title: 'Count of requests',
           },
           migrationVersion: {
-            visualization: '7.0.0',
+            visualization: '7.0.1',
           },
           references: [
             {

x-pack/test/saved_object_api_integration/common/suites/get.ts

@@ -96,7 +96,7 @@ export function getTestSuiteFactory(esArchiver: any, supertest: SuperTest<any>)
       id: `${getIdPrefix(spaceId)}dd7caf20-9efd-11e7-acb3-3dab96693fab`,
       type: 'visualization',
       migrationVersion: {
-        visualization: '7.0.0',
+        visualization: '7.0.1',
       },
       updated_at: '2017-09-21T18:51:23.794Z',
       version: resp.body.version,