From f4b45f07c398236f59a00cdc8e91e019627e0773 Mon Sep 17 00:00:00 2001
From: Maxime Beauchemin <maximebeauchemin@gmail.com>
Date: Thu, 9 Aug 2018 13:04:09 -0700
Subject: [PATCH] [sql lab] visualization flow to detect unaliased columns
 (#5579)

* [sql lab] visualization flow to detect unaliased columns

* Addressing comments
---
 .../sqllab/ExploreResultsButton_spec.jsx      | 22 +++++++++--
 .../spec/javascripts/sqllab/fixtures.js       | 39 ++++++++++++++++++-
 .../components/ExploreResultsButton.jsx       | 38 +++++++++++++++++-
 3 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/superset/assets/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx b/superset/assets/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx
index b57ddd4950..1d0448e6c7 100644
--- a/superset/assets/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx
+++ b/superset/assets/spec/javascripts/sqllab/ExploreResultsButton_spec.jsx
@@ -9,7 +9,7 @@ import sinon from 'sinon';
 
 import $ from 'jquery';
 import shortid from 'shortid';
-import { queries } from './fixtures';
+import { queries, queryWithBadColumns } from './fixtures';
 import { sqlLabReducer } from '../../../src/SqlLab/reducers';
 import * as actions from '../../../src/SqlLab/actions';
 import ExploreResultsButton from '../../../src/SqlLab/components/ExploreResultsButton';
@@ -60,19 +60,35 @@ describe('ExploreResultsButton', () => {
     requiresTime: true,
     value: 'bar',
   };
-  const getExploreResultsButtonWrapper = () => (
-    shallow(<ExploreResultsButton {...mockedProps} />, {
+  const getExploreResultsButtonWrapper = (props = mockedProps) => (
+    shallow(<ExploreResultsButton {...props} />, {
       context: { store },
     }).dive());
 
   it('renders', () => {
     expect(React.isValidElement(<ExploreResultsButton />)).to.equal(true);
   });
+
   it('renders with props', () => {
     expect(
       React.isValidElement(<ExploreResultsButton {...mockedProps} />),
     ).to.equal(true);
   });
+
+  it('detects bad columns', () => {
+    const wrapper = getExploreResultsButtonWrapper({
+      database,
+      show: true,
+      query: queryWithBadColumns,
+    });
+
+    const badCols = wrapper.instance().getInvalidColumns();
+    expect(badCols).to.deep.equal(['COUNT(*)', '1', '123', 'CASE WHEN 1=1 THEN 1 ELSE 0 END']);
+
+    const msgWrapper = shallow(wrapper.instance().renderInvalidColumnMessage());
+    expect(msgWrapper.find('div')).to.have.length(1);
+  });
+
   it('renders a Button', () => {
     const wrapper = getExploreResultsButtonWrapper();
     expect(wrapper.find(Button)).to.have.length(1);
diff --git a/superset/assets/spec/javascripts/sqllab/fixtures.js b/superset/assets/spec/javascripts/sqllab/fixtures.js
index 510b5d5bda..f8d8911cb7 100644
--- a/superset/assets/spec/javascripts/sqllab/fixtures.js
+++ b/superset/assets/spec/javascripts/sqllab/fixtures.js
@@ -179,7 +179,7 @@ export const defaultQueryEditor = {
 export const queries = [
   {
     dbId: 1,
-    sql: 'SELECT *FROM superset.slices',
+    sql: 'SELECT * FROM superset.slices',
     sqlEditorId: 'SJ8YO72R',
     tab: 'Demo',
     runAsync: false,
@@ -257,6 +257,43 @@ export const queries = [
     results: null,
   },
 ];
+export const queryWithBadColumns = {
+  ...queries[0],
+  results: {
+    data: queries[0].results.data,
+    columns: [{
+      is_date: true,
+      is_dim: false,
+      name: 'COUNT(*)',
+      type: 'STRING',
+    }, {
+      is_date: false,
+      is_dim: true,
+      name: 'this_col_is_ok',
+      type: 'STRING',
+    }, {
+      is_date: false,
+      is_dim: true,
+      name: 'a',
+      type: 'STRING',
+    }, {
+      is_date: false,
+      is_dim: true,
+      name: '1',
+      type: 'STRING',
+    }, {
+      is_date: false,
+      is_dim: true,
+      name: '123',
+      type: 'STRING',
+    }, {
+      is_date: false,
+      is_dim: true,
+      name: 'CASE WHEN 1=1 THEN 1 ELSE 0 END',
+      type: 'STRING',
+    }],
+  },
+};
 export const databases = {
   result: [{
     allow_ctas: true,
diff --git a/superset/assets/src/SqlLab/components/ExploreResultsButton.jsx b/superset/assets/src/SqlLab/components/ExploreResultsButton.jsx
index 493b103c24..f5bbbe8975 100644
--- a/superset/assets/src/SqlLab/components/ExploreResultsButton.jsx
+++ b/superset/assets/src/SqlLab/components/ExploreResultsButton.jsx
@@ -33,12 +33,15 @@ class ExploreResultsButton extends React.PureComponent {
     };
     this.visualize = this.visualize.bind(this);
     this.onClick = this.onClick.bind(this);
+    this.getInvalidColumns = this.getInvalidColumns.bind(this);
+    this.renderInvalidColumnMessage = this.renderInvalidColumnMessage.bind(this);
   }
   onClick() {
     const timeout = this.props.timeout;
+    const msg = this.renderInvalidColumnMessage();
     if (Math.round(this.getQueryDuration()) > timeout) {
       this.dialog.show({
-        title: 'Explore',
+        title: t('Explore'),
         body: this.renderTimeoutWarning(),
         actions: [
           Dialog.CancelAction(),
@@ -51,6 +54,19 @@ class ExploreResultsButton extends React.PureComponent {
           dialog.hide();
         },
       });
+    } else if (msg) {
+      this.dialog.show({
+        title: t('Explore'),
+        body: msg,
+        actions: [
+          Dialog.DefaultAction('Ok', () => {}, 'btn-danger'),
+        ],
+        bsSize: 'large',
+        bsStyle: 'warning',
+        onHide: (dialog) => {
+          dialog.hide();
+        },
+      });
     } else {
       this.visualize();
     }
@@ -65,6 +81,10 @@ class ExploreResultsButton extends React.PureComponent {
   getQueryDuration() {
     return moment.duration(this.props.query.endDttm - this.props.query.startDttm).asSeconds();
   }
+  getInvalidColumns() {
+    const re = /^[A-Za-z_]\w*$/;
+    return this.props.query.results.columns.map(col => col.name).filter(col => !re.test(col));
+  }
   datasourceName() {
     const { query } = this.props;
     const uniqueId = shortid.generate();
@@ -126,6 +146,22 @@ class ExploreResultsButton extends React.PureComponent {
         {t('feature to store a summarized data set that you can then explore.')}
       </Alert>);
   }
+  renderInvalidColumnMessage() {
+    const invalidColumns = this.getInvalidColumns();
+    if (invalidColumns.length === 0) {
+      return null;
+    }
+    return (
+      <div>
+        {t('Column name(s) ')}
+        <code><strong>{invalidColumns.join(', ')} </strong></code>
+        {t('cannot be used as a column name. Please use aliases (as in ')}
+        <code>SELECT count(*)
+          <strong>AS my_alias</strong>
+        </code>){' '}
+        {t('limited to alphanumeric characters and underscores')}
+      </div>);
+  }
   render() {
     return (
       <Button