superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From grace...@apache.org
Subject [incubator-superset] branch master updated: [WiP] Cleanup & fix URL scheme for the explore view (#4490)
Date Tue, 27 Feb 2018 23:08:12 GMT
This is an automated email from the ASF dual-hosted git repository.

graceguo 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 83524f9  [WiP] Cleanup & fix URL scheme for the explore view (#4490)
83524f9 is described below

commit 83524f97d7fcf3c8c86efd25f6e5f76fda1b48f5
Author: Maxime Beauchemin <maximebeauchemin@gmail.com>
AuthorDate: Tue Feb 27 15:08:06 2018 -0800

    [WiP] Cleanup & fix URL scheme for the explore view (#4490)
    
    * Improve URLs
    
    * Fix js tests
---
 run_specific_test.sh                               |  1 +
 run_tests.sh                                       |  1 +
 .../javascripts/addSlice/AddSliceContainer.jsx     |  3 +-
 .../assets/javascripts/explore/exploreUtils.js     |  3 -
 .../addSlice/AddSliceContainer_spec.jsx            |  2 +-
 .../assets/spec/javascripts/explore/utils_spec.jsx | 18 ++---
 superset/models/core.py                            | 24 ++++---
 superset/views/core.py                             | 84 ++++++++++++++--------
 tests/core_tests.py                                | 38 ++++++++--
 9 files changed, 114 insertions(+), 60 deletions(-)

diff --git a/run_specific_test.sh b/run_specific_test.sh
index 47e8307..f267e08 100755
--- a/run_specific_test.sh
+++ b/run_specific_test.sh
@@ -1,6 +1,7 @@
 #!/usr/bin/env bash
 echo $DB
 rm -f .coverage
+export PYTHONPATH=./
 export SUPERSET_CONFIG=tests.superset_test_config
 set -e
 superset/bin/superset version -v
diff --git a/run_tests.sh b/run_tests.sh
index 448a750..de40d6c 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -4,6 +4,7 @@ rm ~/.superset/unittests.db
 rm ~/.superset/celerydb.sqlite
 rm ~/.superset/celery_results.sqlite
 rm -f .coverage
+export PYTHONPATH=./
 export SUPERSET_CONFIG=tests.superset_test_config
 set -e
 superset/bin/superset db upgrade
diff --git a/superset/assets/javascripts/addSlice/AddSliceContainer.jsx b/superset/assets/javascripts/addSlice/AddSliceContainer.jsx
index 2396b1b..48b31eb 100644
--- a/superset/assets/javascripts/addSlice/AddSliceContainer.jsx
+++ b/superset/assets/javascripts/addSlice/AddSliceContainer.jsx
@@ -23,9 +23,8 @@ export default class AddSliceContainer extends React.PureComponent {
   }
 
   exploreUrl() {
-    const baseUrl = `/superset/explore/${this.state.datasourceType}/${this.state.datasourceId}`;
     const formData = encodeURIComponent(JSON.stringify({ viz_type: this.state.visType }));
-    return `${baseUrl}?form_data=${formData}`;
+    return `/superset/explore/?form_data=${formData}`;
   }
 
   gotoSlice() {
diff --git a/superset/assets/javascripts/explore/exploreUtils.js b/superset/assets/javascripts/explore/exploreUtils.js
index 309fce1..1c1271b 100644
--- a/superset/assets/javascripts/explore/exploreUtils.js
+++ b/superset/assets/javascripts/explore/exploreUtils.js
@@ -25,9 +25,6 @@ export function getURIDirectory(formData, endpointType = 'base') {
   if (['json', 'csv', 'query'].indexOf(endpointType) >= 0) {
     directory = '/superset/explore_json/';
   }
-  const [datasource_id, datasource_type] = formData.datasource.split('__');
-  directory += `${datasource_type}/${datasource_id}/`;
-
   return directory;
 }
 
diff --git a/superset/assets/spec/javascripts/addSlice/AddSliceContainer_spec.jsx b/superset/assets/spec/javascripts/addSlice/AddSliceContainer_spec.jsx
index ab3d4ff..2a1f09b 100644
--- a/superset/assets/spec/javascripts/addSlice/AddSliceContainer_spec.jsx
+++ b/superset/assets/spec/javascripts/addSlice/AddSliceContainer_spec.jsx
@@ -53,7 +53,7 @@ describe('AddSliceContainer', () => {
       datasourceId: datasourceValue.split('__')[0],
       datasourceType: datasourceValue.split('__')[1],
     });
-    const formattedUrl = '/superset/explore/table/1?form_data=%7B%22viz_type%22%3A%22table%22%7D';
+    const formattedUrl = '/superset/explore/?form_data=%7B%22viz_type%22%3A%22table%22%7D';
     expect(wrapper.instance().exploreUrl()).to.equal(formattedUrl);
   });
 });
diff --git a/superset/assets/spec/javascripts/explore/utils_spec.jsx b/superset/assets/spec/javascripts/explore/utils_spec.jsx
index 8d4f868..fe18f39 100644
--- a/superset/assets/spec/javascripts/explore/utils_spec.jsx
+++ b/superset/assets/spec/javascripts/explore/utils_spec.jsx
@@ -27,7 +27,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore/table/1/'),
+        URI('/superset/explore/'),
       );
       expect(payload).to.deep.equals(formData);
     });
@@ -40,7 +40,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/'),
+        URI('/superset/explore_json/'),
       );
       expect(payload).to.deep.equals(formData);
     });
@@ -53,7 +53,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/')
+        URI('/superset/explore_json/')
           .search({ force: 'true' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -67,7 +67,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/')
+        URI('/superset/explore_json/')
           .search({ csv: 'true' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -81,7 +81,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore/table/1/')
+        URI('/superset/explore/')
           .search({ standalone: 'true' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -95,7 +95,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/')
+        URI('/superset/explore_json/')
           .search({ foo: 'bar' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -109,7 +109,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/')
+        URI('/superset/explore_json/')
           .search({ foo: 'bar' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -123,7 +123,7 @@ describe('utils', () => {
       });
       compareURI(
         URI(url),
-        URI('/superset/explore_json/table/1/')
+        URI('/superset/explore_json/')
           .search({ foo: 'bar' }),
       );
       expect(payload).to.deep.equals(formData);
@@ -134,7 +134,7 @@ describe('utils', () => {
     it('generates proper base url with form_data', () => {
       compareURI(
         URI(getExploreLongUrl(formData, 'base')),
-        URI('/superset/explore/table/1/').search({ form_data: sFormData }),
+        URI('/superset/explore/').search({ form_data: sFormData }),
       );
     });
   });
diff --git a/superset/models/core.py b/superset/models/core.py
index 9c267ba..b4dbada 100644
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -201,26 +201,30 @@ class Slice(Model, AuditMixinNullable, ImportMixin):
         form_data.update({
             'slice_id': self.id,
             'viz_type': self.viz_type,
-            'datasource': str(self.datasource_id) + '__' + self.datasource_type,
+            'datasource': '{}__{}'.format(
+                self.datasource_id, self.datasource_type),
         })
         if self.cache_timeout:
             form_data['cache_timeout'] = self.cache_timeout
         return form_data
 
+    def get_explore_url(self, base_url='/superset/explore', overrides=None):
+        overrides = overrides or {}
+        form_data = {'slice_id': self.id}
+        form_data.update(overrides)
+        params = parse.quote(json.dumps(form_data))
+        return (
+            '{base_url}/?form_data={params}'.format(**locals()))
+
     @property
     def slice_url(self):
         """Defines the url to access the slice"""
-        form_data = {'slice_id': self.id}
-        return (
-            '/superset/explore/{obj.datasource_type}/'
-            '{obj.datasource_id}/?form_data={params}'.format(
-                obj=self, params=parse.quote(json.dumps(form_data))))
+        return self.get_explore_url()
 
     @property
-    def slice_id_url(self):
-        return (
-            '/superset/{slc.datasource_type}/{slc.datasource_id}/{slc.id}/'
-        ).format(slc=self)
+    def explore_json_url(self):
+        """Defines the url to access the slice"""
+        return self.get_explore_url('/superset/explore_json')
 
     @property
     def edit_url(self):
diff --git a/superset/views/core.py b/superset/views/core.py
index b71f731..c8101b8 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -943,24 +943,39 @@ class Superset(BaseSupersetView):
         session.commit()
         return redirect('/accessrequestsmodelview/list/')
 
-    def get_form_data(self):
-        d = {}
+    def get_form_data(self, slice_id=None):
+        form_data = {}
         post_data = request.form.get('form_data')
         request_args_data = request.args.get('form_data')
         # Supporting POST
         if post_data:
-            d.update(json.loads(post_data))
+            form_data.update(json.loads(post_data))
         # request params can overwrite post body
         if request_args_data:
-            d.update(json.loads(request_args_data))
+            form_data.update(json.loads(request_args_data))
 
         if request.args.get('viz_type'):
             # Converting old URLs
-            d = cast_form_data(d)
+            form_data = cast_form_data(form_data)
 
-        d = {k: v for k, v in d.items() if k not in FORM_DATA_KEY_BLACKLIST}
+        form_data = {
+            k: v
+            for k, v in form_data.items()
+            if k not in FORM_DATA_KEY_BLACKLIST
+        }
 
-        return d
+        # When a slice_id is present, load from DB and override
+        # the form_data from the DB with the other form_data provided
+        slice_id = form_data.get('slice_id') or slice_id
+        slc = None
+        if slice_id:
+            slc = db.session.query(models.Slice).filter_by(id=slice_id).first()
+            slice_form_data = slc.form_data.copy()
+            # allow form_data in request override slice from_data
+            slice_form_data.update(form_data)
+            form_data = slice_form_data
+
+        return form_data, slc
 
     def get_viz(
             self,
@@ -991,11 +1006,9 @@ class Superset(BaseSupersetView):
     @has_access
     @expose('/slice/<slice_id>/')
     def slice(self, slice_id):
-        viz_obj = self.get_viz(slice_id)
-        endpoint = '/superset/explore/{}/{}?form_data={}'.format(
-            viz_obj.datasource.type,
-            viz_obj.datasource.id,
-            parse.quote(json.dumps(viz_obj.form_data)),
+        form_data, slc = self.get_form_data(slice_id)
+        endpoint = '/superset/explore/?form_data={}'.format(
+            parse.quote(json.dumps(form_data)),
         )
         if request.args.get('standalone') == 'true':
             endpoint += '&standalone=true'
@@ -1075,10 +1088,7 @@ class Superset(BaseSupersetView):
             viz_obj = self.get_viz(slice_id)
             datasource_type = viz_obj.datasource.type
             datasource_id = viz_obj.datasource.id
-            form_data = viz_obj.form_data
-            # This allows you to override the saved slice form data with
-            # data from the current request (e.g. change the time window)
-            form_data.update(self.get_form_data())
+            form_data, slc = self.get_form_data()
         except Exception as e:
             return json_error_response(
                 utils.error_msg_from_exception(e),
@@ -1091,7 +1101,7 @@ class Superset(BaseSupersetView):
     @has_access_api
     @expose('/annotation_json/<layer_id>')
     def annotation_json(self, layer_id):
-        form_data = self.get_form_data()
+        form_data = self.get_form_data()[0]
         form_data['layer_id'] = layer_id
         form_data['filters'] = [{'col': 'layer_id',
                                  'op': '==',
@@ -1115,12 +1125,15 @@ class Superset(BaseSupersetView):
     @log_this
     @has_access_api
     @expose('/explore_json/<datasource_type>/<datasource_id>/', methods=['GET',
'POST'])
-    def explore_json(self, datasource_type, datasource_id):
+    @expose('/explore_json/', methods=['GET', 'POST'])
+    def explore_json(self, datasource_type=None, datasource_id=None):
         try:
             csv = request.args.get('csv') == 'true'
             query = request.args.get('query') == 'true'
             force = request.args.get('force') == 'true'
-            form_data = self.get_form_data()
+            form_data = self.get_form_data()[0]
+            datasource_id, datasource_type = self.datasource_info(
+                datasource_id, datasource_type, form_data)
         except Exception as e:
             logging.exception(e)
             return json_error_response(
@@ -1157,19 +1170,36 @@ class Superset(BaseSupersetView):
     @has_access
     @expose('/explorev2/<datasource_type>/<datasource_id>/')
     def explorev2(self, datasource_type, datasource_id):
+        """Deprecated endpoint, here for backward compatibility of urls"""
         return redirect(url_for(
             'Superset.explore',
             datasource_type=datasource_type,
             datasource_id=datasource_id,
             **request.args))
 
+    @staticmethod
+    def datasource_info(datasource_id, datasource_type, form_data):
+        """Compatibility layer for handling of datasource info
+
+        datasource_id & datasource_type used to be passed in the URL
+        directory, now they should come as part of the form_data,
+        This function allows supporting both without duplicating code"""
+        datasource = form_data.get('datasource', '')
+        if '__' in datasource:
+            datasource_id, datasource_type = datasource.split('__')
+        datasource_id = int(datasource_id)
+        return datasource_id, datasource_type
+
     @log_this
     @has_access
     @expose('/explore/<datasource_type>/<datasource_id>/', methods=['GET', 'POST'])
-    def explore(self, datasource_type, datasource_id):
-        datasource_id = int(datasource_id)
+    @expose('/explore/', methods=['GET', 'POST'])
+    def explore(self, datasource_type=None, datasource_id=None):
         user_id = g.user.get_id() if g.user else None
-        form_data = self.get_form_data()
+        form_data, slc = self.get_form_data()
+
+        datasource_id, datasource_type = self.datasource_info(
+            datasource_id, datasource_type, form_data)
 
         saved_url = None
         url_id = request.args.get('r')
@@ -1182,14 +1212,6 @@ class Superset(BaseSupersetView):
                 # allow form_date in request override saved url
                 url_form_data.update(form_data)
                 form_data = url_form_data
-        slice_id = form_data.get('slice_id')
-        slc = None
-        if slice_id:
-            slc = db.session.query(models.Slice).filter_by(id=slice_id).first()
-            slice_form_data = slc.form_data.copy()
-            # allow form_data in request override slice from_data
-            slice_form_data.update(form_data)
-            form_data = slice_form_data
 
         error_redirect = '/slicemodelview/list/'
         datasource = ConnectorRegistry.get_datasource(
@@ -1308,7 +1330,7 @@ class Superset(BaseSupersetView):
         """Save or overwrite a slice"""
         slice_name = args.get('slice_name')
         action = args.get('action')
-        form_data = self.get_form_data()
+        form_data, _ = self.get_form_data()
 
         if action in ('saveas'):
             if 'slice_id' in form_data:
diff --git a/tests/core_tests.py b/tests/core_tests.py
index ab2c6e6..91ae56f 100644
--- a/tests/core_tests.py
+++ b/tests/core_tests.py
@@ -97,7 +97,7 @@ class CoreTests(SupersetTestCase):
         qobj['groupby'] = []
         self.assertNotEqual(cache_key, viz.cache_key(qobj))
 
-    def test_slice_json_endpoint(self):
+    def test_old_slice_json_endpoint(self):
         self.login(username='admin')
         slc = self.get_slice('Girls', db.session)
 
@@ -108,7 +108,13 @@ class CoreTests(SupersetTestCase):
         resp = self.get_resp(json_endpoint, {'form_data': json.dumps(slc.viz.form_data)})
         assert '"Jennifer"' in resp
 
-    def test_slice_csv_endpoint(self):
+    def test_slice_json_endpoint(self):
+        self.login(username='admin')
+        slc = self.get_slice('Girls', db.session)
+        resp = self.get_resp(slc.explore_json_url)
+        assert '"Jennifer"' in resp
+
+    def test_old_slice_csv_endpoint(self):
         self.login(username='admin')
         slc = self.get_slice('Girls', db.session)
 
@@ -119,6 +125,15 @@ class CoreTests(SupersetTestCase):
         resp = self.get_resp(csv_endpoint, {'form_data': json.dumps(slc.viz.form_data)})
         assert 'Jennifer,' in resp
 
+    def test_slice_csv_endpoint(self):
+        self.login(username='admin')
+        slc = self.get_slice('Girls', db.session)
+
+        csv_endpoint = '/superset/explore_json/?csv=true'
+        resp = self.get_resp(
+            csv_endpoint, {'form_data': json.dumps({'slice_id': slc.id})})
+        assert 'Jennifer,' in resp
+
     def test_admin_only_permissions(self):
         def assert_admin_permission_in(role_name, assert_func):
             role = sm.find_role(role_name)
@@ -225,8 +240,8 @@ class CoreTests(SupersetTestCase):
         urls = []
         for slc in db.session.query(Slc).all():
             urls += [
-                (slc.slice_name, 'slice_url', slc.slice_url),
-                (slc.slice_name, 'slice_id_url', slc.slice_id_url),
+                (slc.slice_name, 'explore', slc.slice_url),
+                (slc.slice_name, 'explore_json', slc.explore_json_url),
             ]
         for name, method, url in urls:
             logging.info('[{name}]/[{method}]: {url}'.format(**locals()))
@@ -879,6 +894,21 @@ class CoreTests(SupersetTestCase):
         rendered_query = text_type(table.get_from_clause())
         self.assertEqual(clean_query, rendered_query)
 
+    def test_slice_url_overrides(self):
+        # No override
+        self.login(username='admin')
+        slice_name = 'Girls'
+        slc = self.get_slice(slice_name, db.session)
+        resp = self.get_resp(slc.explore_json_url)
+        assert '"Jennifer"' in resp
+
+        # Overriding groupby
+        url = slc.get_explore_url(
+            base_url='/superset/explore_json',
+            overrides={'groupby': ['state']})
+        resp = self.get_resp(url)
+        assert '"CA"' in resp
+
 
 if __name__ == '__main__':
     unittest.main()

-- 
To stop receiving notification emails like this one, please contact
graceguo@apache.org.

Mime
View raw message