From bae21194c1586955bb9b883d293537b383996bd7 Mon Sep 17 00:00:00 2001 From: Chris Williams Date: Wed, 31 Aug 2016 16:45:03 -0700 Subject: [PATCH] Chris/remove redirect from slice id endpoint (#1044) * call explore() directly from '/caravel/slice/id/' endpoint instead of redirecting to it. * pass slice_id instead of slice_params in 'caravel/slice/id/' endpoint to avoid logging errors. add fix for MultiDict bug * address max's comments * remove the /slice/id/ endpoint in preference of /datasource_type/datasource_id/slice_id/ * pep8 --- caravel/views.py | 96 ++++++++++++++++++++++++++++----------------- caravel/viz.py | 29 ++++++++------ tests/core_tests.py | 40 +------------------ 3 files changed, 80 insertions(+), 85 deletions(-) diff --git a/caravel/views.py b/caravel/views.py index d3b8af67ad..e2b24a57e6 100755 --- a/caravel/views.py +++ b/caravel/views.py @@ -26,6 +26,7 @@ from flask_babel import lazy_gettext as _ from flask_appbuilder.models.sqla.filters import BaseFilter from sqlalchemy import create_engine +from werkzeug.datastructures import ImmutableMultiDict, MultiDict from werkzeug.routing import BaseConverter from wtforms.validators import ValidationError @@ -866,14 +867,14 @@ appbuilder.add_view_no_menu(R) class Caravel(BaseCaravelView): - """The base views for Caravel!""" @has_access + @expose("/explore////") @expose("/explore///") @expose("/datasource///") # Legacy url @log_this - def explore(self, datasource_type, datasource_id): + def explore(self, datasource_type, datasource_id, slice_id=None): error_redirect = '/slicemodelview/list/' datasource_class = models.SqlaTable \ @@ -886,51 +887,84 @@ class Caravel(BaseCaravelView): datasources = sorted(datasources, key=lambda ds: ds.full_name) datasource = [ds for ds in datasources if int(datasource_id) == ds.id] datasource = datasource[0] if datasource else None - slice_id = request.args.get("slice_id") + + if not datasource: + flash(__("The datasource seems to have been deleted"), "alert") + return redirect(error_redirect) + + all_datasource_access = self.can_access( + 'all_datasource_access', 'all_datasource_access') + + datasource_access = self.can_access( + 'datasource_access', datasource.perm) + + if not (all_datasource_access or datasource_access): + flash(__("You don't seem to have access to this datasource"), + "danger") + return redirect(error_redirect) + + # handle slc / viz obj + slice_id = slice_id or request.args.get("slice_id") + viz_type = None slc = None + slice_params = request.args + if slice_id: slc = ( db.session.query(models.Slice) .filter_by(id=slice_id) .first() ) - if not datasource: - flash(__("The datasource seems to have been deleted"), "alert") - return redirect(error_redirect) + try: + param_dict = json.loads(slc.params) + except Exception as e: + logging.exception(e) + param_dict = {} + + # override slice params with anything passed in url + # some overwritten slices have been saved with original slice_ids + param_dict["slice_id"] = slice_id + param_dict["json"] = "false" + param_dict.update(request.args.to_dict(flat=False)) + slice_params = ImmutableMultiDict(param_dict) + + # slc perms slice_add_perm = self.can_access('can_add', 'SliceModelView') slice_edit_perm = check_ownership(slc, raise_if_false=False) slice_download_perm = self.can_access('can_download', 'SliceModelView') - all_datasource_access = self.can_access( - 'all_datasource_access', 'all_datasource_access') - datasource_access = self.can_access( - 'datasource_access', datasource.perm) - if not (all_datasource_access or datasource_access): - flash(__("You don't seem to have access to this datasource"), - "danger") - return redirect(error_redirect) + # handle save or overwrite + action = slice_params.get('action') - action = request.args.get('action') if action in ('saveas', 'overwrite'): return self.save_or_overwrite_slice( - request.args, slc, slice_add_perm, slice_edit_perm) + slice_params, slc, slice_add_perm, slice_edit_perm) - viz_type = request.args.get("viz_type") + # look for viz type + viz_type = slice_params.get("viz_type", slc.viz_type if slc else None) + + # go to default endpoint if no viz_type if not viz_type and datasource.default_endpoint: return redirect(datasource.default_endpoint) + + # default to table if no default endpoint and no viz_type if not viz_type: viz_type = "table" + + # validate viz params try: obj = viz.viz_types[viz_type]( datasource, - form_data=request.args, + form_data=slice_params, slice_=slc) except Exception as e: flash(utils.error_msg_from_exception(e), "danger") return redirect(error_redirect) - if request.args.get("json") == "true": + + # handle different endpoints + if slice_params.get("json") == "true": status = 200 if config.get("DEBUG"): # Allows for nice debugger stack traces in debug mode @@ -947,7 +981,8 @@ class Caravel(BaseCaravelView): status=status, mimetype="application/json") return resp - elif request.args.get("csv") == "true": + + elif slice_params.get("csv") == "true": payload = obj.get_csv() return Response( payload, @@ -955,15 +990,17 @@ class Caravel(BaseCaravelView): headers=generate_download_headers("csv"), mimetype="application/csv") else: - if request.args.get("standalone") == "true": + if slice_params.get("standalone") == "true": template = "caravel/standalone.html" else: template = "caravel/explore.html" + resp = self.render_template( template, viz=obj, slice=slc, datasources=datasources, can_add=slice_add_perm, can_edit=slice_edit_perm, can_download=slice_download_perm, userid=g.user.get_id() if g.user else '') + try: pass except Exception as e: @@ -973,6 +1010,7 @@ class Caravel(BaseCaravelView): utils.error_msg_from_exception(e), status=500, mimetype="application/json") + return resp def save_or_overwrite_slice( @@ -1003,6 +1041,7 @@ class Caravel(BaseCaravelView): table_id = args.get('datasource_id') if action in ('saveas'): + d.pop('slice_id') # don't save old slice_id slc = models.Slice(owners=[g.user] if g.user else []) slc.params = json.dumps(d, indent=4, sort_keys=True) @@ -1216,21 +1255,6 @@ class Caravel(BaseCaravelView): json.dumps({'count': count}), mimetype="application/json") - @has_access - @expose("/slice//") - def slice(self, slice_id): - """Redirects a request for a slice id to its corresponding URL""" - session = db.session() - qry = session.query(models.Slice).filter_by(id=int(slice_id)) - slc = qry.first() - if slc: - url = '{slc.slice_url}&standalone={standalone}'.format( - slc=slc, standalone=request.args.get('standalone', 'false')) - return redirect(url) - else: - flash("The specified slice could not be found", "danger") - return redirect('/slicemodelview/list/') - @has_access @expose("/dashboard//") def dashboard(self, dashboard_id): diff --git a/caravel/viz.py b/caravel/viz.py index 3ecf5fb7f3..13f99f8854 100755 --- a/caravel/viz.py +++ b/caravel/viz.py @@ -65,7 +65,7 @@ class BaseViz(object): form_class = ff.get_form() defaults = form_class().data.copy() previous_viz_type = form_data.get('previous_viz_type') - if isinstance(form_data, ImmutableMultiDict): + if isinstance(form_data, (MultiDict, ImmutableMultiDict)): form = form_class(form_data) else: form = form_class(**form_data) @@ -120,17 +120,24 @@ class BaseViz(object): # Remove unchecked checkboxes because HTML is weird like that od = MultiDict() for key in sorted(d.keys()): - if d[key] is False: - del d[key] + # if MultiDict is initialized with MD({key:[emptyarray]}), + # key is included in d.keys() but accessing it throws + try: + if d[key] is False: + del d[key] + continue + except IndexError: + pass + + if isinstance(d, (MultiDict, ImmutableMultiDict)): + v = d.getlist(key) else: - if isinstance(d, MultiDict): - v = d.getlist(key) - else: - v = d.get(key) - if not isinstance(v, list): - v = [v] - for item in v: - od.add(key, item) + v = d.get(key) + if not isinstance(v, list): + v = [v] + for item in v: + od.add(key, item) + href = Href( '/caravel/explore/{self.datasource.type}/' '{self.datasource.id}/'.format(**locals())) diff --git a/tests/core_tests.py b/tests/core_tests.py index f312cb9f5b..05a5358655 100644 --- a/tests/core_tests.py +++ b/tests/core_tests.py @@ -86,51 +86,15 @@ class CoreTests(CaravelTestCase): for slc in db.session.query(Slc).all(): urls += [ (slc.slice_name, 'slice_url', slc.slice_url), - (slc.slice_name, 'slice_id_endpoint', '/caravel/slices/{}'. - format(slc.id)), (slc.slice_name, 'json_endpoint', slc.viz.json_endpoint), (slc.slice_name, 'csv_endpoint', slc.viz.csv_endpoint), + (slc.slice_name, 'slice_id_url', + "/caravel/{slc.datasource_type}/{slc.datasource_id}/{slc.id}/".format(slc=slc)), ] for name, method, url in urls: print("[{name}]/[{method}]: {url}".format(**locals())) self.client.get(url) - def test_slice_id_redirects(self, username='admin'): - def make_assertions(resp, standalone): - decoded = resp.data.decode('utf-8') - if standalone: - assert "Query" not in decoded - assert 'data-standalone="true"' in decoded - - else: - assert "Query" in decoded - assert 'data-standalone="true"' not in decoded - - self.login(username=username) - slc = db.session.query(models.Slice).filter_by(slice_name="Name Cloud").first() - get = self.client.get - - # Standard redirect - slc_url = slc.slice_url - id_url = '/caravel/slice/{slc.id}'.format(slc=slc) - - make_assertions(get(slc_url, follow_redirects=True), False) - make_assertions(get(id_url, follow_redirects=True), False) - - # Explicit standalone - slc_url_standalone = '{slc_url}&standalone=true'.format(slc_url=slc_url) - id_url_standalone = '{id_url}?standalone=true'.format(id_url=id_url) - - make_assertions(get(slc_url_standalone, follow_redirects=True), True) - make_assertions(get(id_url_standalone, follow_redirects=True), True) - - # Explicit not-standalone - slc_url_notstandalone = '{slc_url}&standalone=false'.format(slc_url=slc_url) - id_url_notstandalone = '{id_url}?standalone=false'.format(id_url=id_url) - - make_assertions(get(slc_url_notstandalone, follow_redirects=True), False) - make_assertions(get(id_url_notstandalone, follow_redirects=True), False) - def test_dashboard(self): self.login(username='admin') urls = {}