Remove nested table splits from table vis (#26057)

src/legacy/core_plugins/kibana/public/visualize/editor/editor.js

@@ -32,6 +32,7 @@ import { VisTypesRegistryProvider } from 'ui/registry/vis_types';
 import { DocTitleProvider } from 'ui/doc_title';
 import { FilterBarQueryFilterProvider } from 'ui/filter_bar/query_filter';
 import { stateMonitorFactory } from 'ui/state_management/state_monitor_factory';
+import { migrateAppState } from './lib';
 import uiRoutes from 'ui/routes';
 import { uiModules } from 'ui/modules';
 import editorTemplate from './editor.html';
@@ -265,6 +266,12 @@ function VisEditor(
     // This is used to sync visualization state with the url when `appState.save()` is called.
     const appState = new AppState(stateDefaults);
 
+    // Initializing appState does two things - first it translates the defaults into AppState,
+    // second it updates appState based on the url (the url trumps the defaults). This means if
+    // we update the state format at all and want to handle BWC, we must not only migrate the
+    // data stored with saved vis, but also any old state in the url.
+    migrateAppState(appState);
+
     // The savedVis is pulled from elasticsearch, but the appState is pulled from the url, with the
     // defaults applied. If the url was from a previous session which included modifications to the
     // appState then they won't be equal.

src/legacy/core_plugins/kibana/public/visualize/editor/lib/index.js

@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+
+export { migrateAppState } from './migrate_app_state';

src/legacy/core_plugins/kibana/public/visualize/editor/lib/migrate_app_state.js

@@ -0,0 +1,61 @@
+/*
+ * 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, omit } from 'lodash';
+
+/**
+ * Creates a new instance of AppState based on the table vis state.
+ *
+ * Dashboards have a similar implementation; see
+ * core_plugins/kibana/public/dashboard/lib/migrate_app_state
+ *
+ * @param appState {AppState} AppState class to instantiate
+ */
+export function migrateAppState(appState) {
+  // For BWC in pre 7.0 versions where table visualizations could have multiple aggs
+  // with `schema === 'split'`. This ensures that bookmarked URLs with deprecated params
+  // are rewritten to the correct state. See core_plugins/table_vis/migrations.
+  if (appState.vis.type !== 'table') {
+    return;
+  }
+
+  const visAggs = get(appState, 'vis.aggs', []);
+  let splitCount = 0;
+  const migratedAggs = visAggs.map(agg => {
+    if (agg.schema !== 'split') {
+      return agg;
+    }
+
+    splitCount++;
+    if (splitCount === 1) {
+      return agg; // leave the first split agg unchanged
+    }
+    agg.schema = 'bucket';
+    // the `row` param is exclusively used by split aggs, so we remove it
+    agg.params = omit(agg.params, ['row']);
+    return agg;
+  });
+
+  if (splitCount <= 1) {
+    return; // do nothing; we only want to touch tables with multiple split aggs
+  }
+
+  appState.vis.aggs = migratedAggs;
+  appState.save();
+}

src/legacy/core_plugins/table_vis/index.js

@@ -18,6 +18,7 @@
  */
 
 import { resolve } from 'path';
+import { migrations } from './migrations';
 
 export default function (kibana) {
 
@@ -27,7 +28,8 @@ export default function (kibana) {
         'plugins/table_vis/table_vis'
       ],
       styleSheetPaths: resolve(__dirname, 'public/index.scss'),
-    }
+      migrations,
+    },
   });
 
 }

src/legacy/core_plugins/table_vis/migrations.js

@@ -0,0 +1,59 @@
+/*
+ * 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 { cloneDeep, get, omit } from 'lodash';
+
+export const migrations = {
+  visualization: {
+    '7.0.0': (doc) => {
+      try {
+        const visState = JSON.parse(doc.attributes.visState);
+        if (get(visState, 'type') !== 'table') {
+          return doc; // do nothing; we only want to touch tables
+        }
+
+        let splitCount = 0;
+        visState.aggs = visState.aggs.map(agg => {
+          if (agg.schema !== 'split') {
+            return agg;
+          }
+
+          splitCount++;
+          if (splitCount === 1) {
+            return agg; // leave the first split agg unchanged
+          }
+          agg.schema = 'bucket';
+          // the `row` param is exclusively used by split aggs, so we remove it
+          agg.params = omit(agg.params, ['row']);
+          return agg;
+        });
+
+        if (splitCount <= 1) {
+          return doc; // do nothing; we only want to touch tables with multiple split aggs
+        }
+
+        const newDoc = cloneDeep(doc);
+        newDoc.attributes.visState = JSON.stringify(visState);
+        return newDoc;
+      } catch (e) {
+        throw new Error(`Failure attempting to migrate saved object '${doc.attributes.title}' - ${e}`);
+      }
+    }
+  }
+};

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

@@ -0,0 +1,184 @@
+/*
+ * 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('table vis migrations', () => {
+
+  describe('7.0.0', () => {
+    const migrate = doc => migrations.visualization['7.0.0'](doc);
+    const generateDoc = ({ type, aggs }) => ({
+      attributes: {
+        title: 'My Vis',
+        description: 'This is my super cool vis.',
+        visState: JSON.stringify({ type, aggs }),
+        uiStateJSON: '{}',
+        version: 1,
+        kibanaSavedObjectMeta: {
+          searchSourceJSON: '{}'
+        }
+      }
+    });
+
+    it('should return a new object if vis is table and has multiple split aggs', () => {
+      const aggs = [
+        {
+          id: '1',
+          schema: 'metric',
+          params: {}
+        },
+        {
+          id: '2',
+          schema: 'split',
+          params: { foo: 'bar', row: true }
+        },
+        {
+          id: '3',
+          schema: 'split',
+          params: { hey: 'ya', row: false }
+        },
+      ];
+      const tableDoc = generateDoc({ type: 'table', aggs });
+      const expected = tableDoc;
+      const actual = migrate(tableDoc);
+      expect(actual).not.toBe(expected);
+    });
+
+    it('should not touch any vis that is not table', () => {
+      const aggs = [];
+      const pieDoc = generateDoc({ type: 'pie', aggs });
+      const expected = pieDoc;
+      const actual = migrate(pieDoc);
+      expect(actual).toBe(expected);
+    });
+
+    it('should not change values in any vis that is not table', () => {
+      const aggs = [
+        {
+          id: '1',
+          schema: 'metric',
+          params: {}
+        },
+        {
+          id: '2',
+          schema: 'split',
+          params: { foo: 'bar', row: true }
+        },
+        {
+          id: '3',
+          schema: 'segment',
+          params: { hey: 'ya' }
+        }
+      ];
+      const pieDoc = generateDoc({ type: 'pie', aggs });
+      const expected = pieDoc;
+      const actual = migrate(pieDoc);
+      expect(actual).toEqual(expected);
+    });
+
+    it('should not touch table vis if there are not multiple split aggs', () => {
+      const aggs = [
+        {
+          id: '1',
+          schema: 'metric',
+          params: {}
+        },
+        {
+          id: '2',
+          schema: 'split',
+          params: { foo: 'bar', row: true }
+        }
+      ];
+      const tableDoc = generateDoc({ type: 'table', aggs });
+      const expected = tableDoc;
+      const actual = migrate(tableDoc);
+      expect(actual).toBe(expected);
+    });
+
+    it('should change all split aggs to `bucket` except the first', () => {
+      const aggs = [
+        {
+          id: '1',
+          schema: 'metric',
+          params: {}
+        },
+        {
+          id: '2',
+          schema: 'split',
+          params: { foo: 'bar', row: true }
+        },
+        {
+          id: '3',
+          schema: 'split',
+          params: { hey: 'ya', row: false }
+        },
+        {
+          id: '4',
+          schema: 'bucket',
+          params: { heyyy: 'yaaa' }
+        }
+      ];
+      const expected = ['metric', 'split', 'bucket', 'bucket'];
+      const migrated = migrate(generateDoc({ type: 'table', aggs }));
+      const actual = JSON.parse(migrated.attributes.visState);
+      expect(actual.aggs.map(agg => agg.schema)).toEqual(expected);
+    });
+
+    it('should remove `rows` param from any aggs that are not `split`', () => {
+      const aggs = [
+        {
+          id: '1',
+          schema: 'metric',
+          params: {}
+        },
+        {
+          id: '2',
+          schema: 'split',
+          params: { foo: 'bar', row: true }
+        },
+        {
+          id: '3',
+          schema: 'split',
+          params: { hey: 'ya', row: false }
+        }
+      ];
+      const expected = [{}, { foo: 'bar', row: true }, { hey: 'ya' }];
+      const migrated = migrate(generateDoc({ type: 'table', aggs }));
+      const actual = JSON.parse(migrated.attributes.visState);
+      expect(actual.aggs.map(agg => agg.params)).toEqual(expected);
+    });
+
+    it('should throw with a reference to the doc name if something goes wrong', () => {
+      const doc = {
+        attributes: {
+          title: 'My Vis',
+          description: 'This is my super cool vis.',
+          visState: '!/// Intentionally malformed JSON ///!',
+          uiStateJSON: '{}',
+          version: 1,
+          kibanaSavedObjectMeta: {
+            searchSourceJSON: '{}'
+          }
+        }
+      };
+      expect(() => migrate(doc)).toThrowError(/My Vis/);
+    });
+  });
+
+});

src/legacy/core_plugins/table_vis/public/table_vis.js

@@ -102,6 +102,8 @@ function TableVisTypeProvider(Private) {
           title: i18n.translate('tableVis.tableVisEditorConfig.schemas.splitTitle', {
             defaultMessage: 'Split Table',
           }),
+          min: 0,
+          max: 1,
           aggFilter: ['!filter']
         }
       ])

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: '7.0.0'
+                  },
                   type: 'visualization',
                   updated_at: '2017-09-21T18:51:23.794Z',
                   version: resp.body.saved_objects[0].version,

test/api_integration/apis/saved_objects/create.js

@@ -47,6 +47,9 @@ export default function ({ getService }) {
             expect(resp.body).to.eql({
               id: resp.body.id,
               type: 'visualization',
+              migrationVersion: {
+                visualization: '7.0.0'
+              },
               updated_at: resp.body.updated_at,
               version: 1,
               attributes: {
@@ -85,6 +88,9 @@ export default function ({ getService }) {
             expect(resp.body).to.eql({
               id: resp.body.id,
               type: 'visualization',
+              migrationVersion: {
+                visualization: '7.0.0'
+              },
               updated_at: resp.body.updated_at,
               version: 1,
               attributes: {

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: '7.0.0'
+              },
               type: 'visualization',
               updated_at: '2017-09-21T18:51:23.794Z',
               version: resp.body.version,

test/functional/apps/visualize/_data_table.js

@@ -388,21 +388,6 @@ export default function ({ getService, getPageObjects }) {
           ]
         ]);
       });
-
-      it.skip('should allow nesting multiple splits', async () => {
-        // This test can be removed as soon as we remove the nested split table
-        // feature (https://github.com/elastic/kibana/issues/24560). (7.0)
-        await PageObjects.visualize.clickData();
-        await PageObjects.visualize.clickAddBucket();
-        await PageObjects.visualize.clickBucket('Split Table');
-        await PageObjects.visualize.selectAggregation('Terms');
-        await PageObjects.visualize.clickSplitDirection('column');
-        await PageObjects.visualize.selectField('machine.os.raw');
-        await PageObjects.visualize.setSize(2);
-        await PageObjects.visualize.clickGo();
-        const splitCount = await PageObjects.visualize.countNestedTables();
-        expect(splitCount).to.be.eql([ 12, 10, 8 ]);
-      });
     });
 
   });

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

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

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

@@ -58,6 +58,9 @@ export function createTestSuiteFactory(es: any, esArchiver: any, supertest: Supe
 
     expect(resp.body).to.eql({
       id: resp.body.id,
+      migrationVersion: {
+        visualization: '7.0.0',
+      },
       type: spaceAwareType,
       updated_at: resp.body.updated_at,
       version: 1,

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

@@ -94,6 +94,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: '7.0.0',
+      },
       updated_at: '2017-09-21T18:51:23.794Z',
       version: resp.body.version,
       attributes: {