From 83524f97d7fcf3c8c86efd25f6e5f76fda1b48f5 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Tue, 27 Feb 2018 15:08:06 -0800 Subject: [PATCH] [WiP] Cleanup & fix URL scheme for the explore view (#4490) * Improve URLs * Fix js tests --- run_specific_test.sh | 1 + run_tests.sh | 1 + .../addSlice/AddSliceContainer.jsx | 3 +- .../javascripts/explore/exploreUtils.js | 3 - .../addSlice/AddSliceContainer_spec.jsx | 2 +- .../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 47e8307440264..f267e08226bfd 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 448a750fdf30f..de40d6c75c163 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 2396b1b303f40..48b31eb4797b8 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 309fce1e8e19c..1c1271b0458c2 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 ab3d4ff878a9c..2a1f09b79c11c 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 8d4f86837fc22..fe18f3907335b 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 9c267ba498f37..b4dbada947f4e 100644 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -201,26 +201,30 @@ def form_data(self): 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 b71f73173248c..c8101b8a8fc49 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -943,24 +943,39 @@ def clean_fulfilled_requests(session): 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 @@ def get_viz( @has_access @expose('/slice//') 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 @@ def slice_json(self, slice_id): 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 @@ def slice_json(self, slice_id): @has_access_api @expose('/annotation_json/') 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 @@ def annotation_json(self, layer_id): @log_this @has_access_api @expose('/explore_json///', 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 @@ def import_dashboards(self): @has_access @expose('/explorev2///') 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///', 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 @@ def explore(self, datasource_type, datasource_id): # 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 @@ def save_or_overwrite_slice( """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 ab2c6e6782fe7..91ae56f158679 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -97,7 +97,7 @@ def test_cache_key(self): 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 @@ def test_slice_json_endpoint(self): 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 @@ def test_slice_csv_endpoint(self): 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 @@ def test_slices(self): 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 @@ def test_comments_in_sqlatable_query(self): 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()