superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From maximebeauche...@apache.org
Subject [incubator-superset] branch master updated: bugfix: Improve support for special characters in schema and table names (#7297)
Date Wed, 08 May 2019 05:37:51 GMT
This is an automated email from the ASF dual-hosted git repository.

maximebeauchemin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 959c35d  bugfix: Improve support for special characters in schema and table names
(#7297)
959c35d is described below

commit 959c35d506759bdbea48db55ef61677f4c1ebd7d
Author: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
AuthorDate: Wed May 8 08:37:44 2019 +0300

    bugfix: Improve support for special characters in schema and table names (#7297)
    
    * Bugfix to SQL Lab to support tables and schemas with characters that require quoting
    
    * Remove debugging prints
    
    * Add uri encoding to secondary tables call
    
    * Quote schema names for presto
    
    * Quote selected_schema on Snowflake, MySQL and Hive
    
    * Remove redundant parens
    
    * Add python unit tests
    
    * Add js unit test
    
    * Fix flake8 linting error
---
 .../javascripts/components/TableSelector_spec.jsx    | 20 ++++++++++++++++++++
 superset/assets/src/SqlLab/actions/sqlLab.js         |  6 ++++--
 superset/assets/src/components/TableSelector.jsx     | 10 +++++-----
 superset/db_engine_specs.py                          |  7 +++++--
 superset/utils/core.py                               | 15 +++++++++++++--
 superset/views/core.py                               | 18 ++++++++++--------
 tests/utils_tests.py                                 | 16 ++++++++++++++++
 7 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/superset/assets/spec/javascripts/components/TableSelector_spec.jsx b/superset/assets/spec/javascripts/components/TableSelector_spec.jsx
index 46d5763..70e2cca 100644
--- a/superset/assets/spec/javascripts/components/TableSelector_spec.jsx
+++ b/superset/assets/spec/javascripts/components/TableSelector_spec.jsx
@@ -85,6 +85,7 @@ describe('TableSelector', () => {
         .getTableNamesBySubStr('')
         .then((data) => {
           expect(data).toEqual({ options: [] });
+          return Promise.resolve();
         }));
 
     it('should handle table name', () => {
@@ -104,6 +105,23 @@ describe('TableSelector', () => {
         .then((data) => {
           expect(fetchMock.calls(GET_TABLE_NAMES_GLOB)).toHaveLength(1);
           expect(data).toEqual(mockTableOptions);
+          return Promise.resolve();
+        });
+    });
+
+    it('should escape schema and table names', () => {
+      const GET_TABLE_GLOB = 'glob:*/superset/tables/1/*/*';
+      const mockTableOptions = { options: [table] };
+      wrapper.setProps({ schema: 'slashed/schema' });
+      fetchMock.get(GET_TABLE_GLOB, mockTableOptions, { overwriteRoutes: true });
+
+      return wrapper
+        .instance()
+        .getTableNamesBySubStr('slashed/table')
+        .then(() => {
+          expect(fetchMock.lastUrl(GET_TABLE_GLOB))
+            .toContain('/slashed%252Fschema/slashed%252Ftable');
+          return Promise.resolve();
         });
     });
   });
@@ -125,6 +143,7 @@ describe('TableSelector', () => {
         .fetchTables(true, 'birth_names')
         .then(() => {
           expect(wrapper.state().tableOptions).toHaveLength(3);
+          return Promise.resolve();
         });
     });
 
@@ -138,6 +157,7 @@ describe('TableSelector', () => {
           expect(wrapper.state().tableOptions).toEqual([]);
           expect(wrapper.state().tableOptions).toHaveLength(0);
           expect(mockedProps.handleError.callCount).toBe(1);
+          return Promise.resolve();
         });
     });
   });
diff --git a/superset/assets/src/SqlLab/actions/sqlLab.js b/superset/assets/src/SqlLab/actions/sqlLab.js
index b3d61a8..f6ac455 100644
--- a/superset/assets/src/SqlLab/actions/sqlLab.js
+++ b/superset/assets/src/SqlLab/actions/sqlLab.js
@@ -285,7 +285,8 @@ export function addTable(query, tableName, schemaName) {
       }),
     );
 
-    SupersetClient.get({ endpoint: `/superset/table/${query.dbId}/${tableName}/${schemaName}/`
})
+    SupersetClient.get({ endpoint: encodeURI(`/superset/table/${query.dbId}/` +
+            `${encodeURIComponent(tableName)}/${encodeURIComponent(schemaName)}/`) })
       .then(({ json }) => {
         const dataPreviewQuery = {
           id: shortid.generate(),
@@ -322,7 +323,8 @@ export function addTable(query, tableName, schemaName) {
       );
 
     SupersetClient.get({
-      endpoint: `/superset/extra_table_metadata/${query.dbId}/${tableName}/${schemaName}/`,
+      endpoint: encodeURI(`/superset/extra_table_metadata/${query.dbId}/` +
+          `${encodeURIComponent(tableName)}/${encodeURIComponent(schemaName)}/`),
     })
       .then(({ json }) =>
         dispatch(mergeTable({ ...table, ...json, isExtraMetadataLoading: false })),
diff --git a/superset/assets/src/components/TableSelector.jsx b/superset/assets/src/components/TableSelector.jsx
index c3e405e..ba2cebb 100644
--- a/superset/assets/src/components/TableSelector.jsx
+++ b/superset/assets/src/components/TableSelector.jsx
@@ -90,7 +90,7 @@ export default class TableSelector extends React.PureComponent {
   onChange() {
     this.props.onChange({
       dbId: this.state.dbId,
-      shema: this.state.schema,
+      schema: this.state.schema,
       tableName: this.state.tableName,
     });
   }
@@ -101,9 +101,8 @@ export default class TableSelector extends React.PureComponent {
       return Promise.resolve({ options });
     }
     return SupersetClient.get({
-      endpoint: (
-        `/superset/tables/${this.props.dbId}/` +
-        `${this.props.schema}/${input}`),
+      endpoint: encodeURI(`/superset/tables/${this.props.dbId}/` +
+        `${encodeURIComponent(this.props.schema)}/${encodeURIComponent(input)}`),
     }).then(({ json }) => ({ options: this.addOptionIfMissing(json.options, tableName)
}));
   }
   dbMutator(data) {
@@ -123,7 +122,8 @@ export default class TableSelector extends React.PureComponent {
     const { dbId, schema } = this.props;
     if (dbId && schema) {
       this.setState(() => ({ tableLoading: true, tableOptions: [] }));
-      const endpoint = `/superset/tables/${dbId}/${schema}/${substr}/${forceRefresh}/`;
+      const endpoint = encodeURI(`/superset/tables/${dbId}/` +
+          `${encodeURIComponent(schema)}/${encodeURIComponent(substr)}/${forceRefresh}/`);
        return SupersetClient.get({ endpoint })
         .then(({ json }) => {
           const filterOptions = createFilterOptions({ options: json.options });
diff --git a/superset/db_engine_specs.py b/superset/db_engine_specs.py
index 5fed480..35a591f 100644
--- a/superset/db_engine_specs.py
+++ b/superset/db_engine_specs.py
@@ -37,6 +37,7 @@ import re
 import textwrap
 import time
 from typing import List, Tuple
+from urllib import parse
 
 from flask import g
 from flask_babel import lazy_gettext as _
@@ -577,6 +578,7 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec):
         if '/' in uri.database:
             database = uri.database.split('/')[0]
         if selected_schema:
+            selected_schema = parse.quote(selected_schema, safe='')
             uri.database = database + '/' + selected_schema
         return uri
 
@@ -757,7 +759,7 @@ class MySQLEngineSpec(BaseEngineSpec):
     @classmethod
     def adjust_database_uri(cls, uri, selected_schema=None):
         if selected_schema:
-            uri.database = selected_schema
+            uri.database = parse.quote(selected_schema, safe='')
         return uri
 
     @classmethod
@@ -1081,6 +1083,7 @@ class PrestoEngineSpec(BaseEngineSpec):
     def adjust_database_uri(cls, uri, selected_schema=None):
         database = uri.database
         if selected_schema and database:
+            selected_schema = parse.quote(selected_schema, safe='')
             if '/' in database:
                 database = database.split('/')[0] + '/' + selected_schema
             else:
@@ -1484,7 +1487,7 @@ class HiveEngineSpec(PrestoEngineSpec):
     @classmethod
     def adjust_database_uri(cls, uri, selected_schema=None):
         if selected_schema:
-            uri.database = selected_schema
+            uri.database = parse.quote(selected_schema, safe='')
         return uri
 
     @classmethod
diff --git a/superset/utils/core.py b/superset/utils/core.py
index 3e80c76..3b41457 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -33,6 +33,7 @@ import smtplib
 import sys
 from time import struct_time
 from typing import List, Optional, Tuple
+from urllib.parse import unquote_plus
 import uuid
 import zlib
 
@@ -141,8 +142,18 @@ def memoized(func=None, watch=None):
         return wrapper
 
 
-def js_string_to_python(item: str) -> Optional[str]:
-    return None if item in ('null', 'undefined') else item
+def parse_js_uri_path_item(item: Optional[str], unquote: bool = True,
+                           eval_undefined: bool = False) -> Optional[str]:
+    """Parse a uri path item made with js.
+
+    :param item: a uri path component
+    :param unquote: Perform unquoting of string using urllib.parse.unquote_plus()
+    :param eval_undefined: When set to True and item is either 'null'  or 'undefined',
+    assume item is undefined and return None.
+    :return: Either None, the original item or unquoted item
+    """
+    item = None if eval_undefined and item in ('null', 'undefined') else item
+    return unquote_plus(item) if unquote and item else item
 
 
 def string_to_num(s: str):
diff --git a/superset/views/core.py b/superset/views/core.py
index eb25cd0..1ccfa10 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -1566,8 +1566,8 @@ class Superset(BaseSupersetView):
         """Endpoint to fetch the list of tables for given database"""
         db_id = int(db_id)
         force_refresh = force_refresh.lower() == 'true'
-        schema = utils.js_string_to_python(schema)
-        substr = utils.js_string_to_python(substr)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        substr = utils.parse_js_uri_path_item(substr, eval_undefined=True)
         database = db.session.query(models.Database).filter_by(id=db_id).one()
 
         if schema:
@@ -2350,11 +2350,11 @@ class Superset(BaseSupersetView):
         }))
 
     @has_access
-    @expose('/table/<database_id>/<path:table_name>/<schema>/')
+    @expose('/table/<database_id>/<table_name>/<schema>/')
     @log_this
     def table(self, database_id, table_name, schema):
-        schema = utils.js_string_to_python(schema)
-        table_name = parse.unquote_plus(table_name)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         mydb = db.session.query(models.Database).filter_by(id=database_id).one()
         payload_columns = []
         indexes = []
@@ -2410,11 +2410,11 @@ class Superset(BaseSupersetView):
         return json_success(json.dumps(tbl))
 
     @has_access
-    @expose('/extra_table_metadata/<database_id>/<path:table_name>/<schema>/')
+    @expose('/extra_table_metadata/<database_id>/<table_name>/<schema>/')
     @log_this
     def extra_table_metadata(self, database_id, table_name, schema):
-        schema = utils.js_string_to_python(schema)
-        table_name = parse.unquote_plus(table_name)
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         mydb = db.session.query(models.Database).filter_by(id=database_id).one()
         payload = mydb.db_engine_spec.extra_table_metadata(
             mydb, table_name, schema)
@@ -2427,6 +2427,8 @@ class Superset(BaseSupersetView):
     def select_star(self, database_id, table_name, schema=None):
         mydb = db.session.query(
             models.Database).filter_by(id=database_id).first()
+        schema = utils.parse_js_uri_path_item(schema, eval_undefined=True)
+        table_name = utils.parse_js_uri_path_item(table_name)
         return json_success(
             mydb.select_star(
                 table_name,
diff --git a/tests/utils_tests.py b/tests/utils_tests.py
index 7e29089..40dedaf 100644
--- a/tests/utils_tests.py
+++ b/tests/utils_tests.py
@@ -35,6 +35,7 @@ from superset.utils.core import (
     merge_extra_filters,
     merge_request_params,
     parse_human_timedelta,
+    parse_js_uri_path_item,
     validate_json,
     zlib_compress,
     zlib_decompress_to_string,
@@ -756,3 +757,18 @@ class UtilsTestCase(unittest.TestCase):
         }
         convert_legacy_filters_into_adhoc(form_data)
         self.assertEquals(form_data, expected)
+
+    def test_parse_js_uri_path_items_eval_undefined(self):
+        self.assertIsNone(parse_js_uri_path_item('undefined', eval_undefined=True))
+        self.assertIsNone(parse_js_uri_path_item('null', eval_undefined=True))
+        self.assertEqual('undefined', parse_js_uri_path_item('undefined'))
+        self.assertEqual('null', parse_js_uri_path_item('null'))
+
+    def test_parse_js_uri_path_items_unquote(self):
+        self.assertEqual('slashed/name', parse_js_uri_path_item('slashed%2fname'))
+        self.assertEqual('slashed%2fname', parse_js_uri_path_item('slashed%2fname',
+                                                                  unquote=False))
+
+    def test_parse_js_uri_path_items_item_optional(self):
+        self.assertIsNone(parse_js_uri_path_item(None))
+        self.assertIsNotNone(parse_js_uri_path_item('item'))


Mime
View raw message