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

@@ -26,6 +26,7 @@
 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 @@ def msg(self):
 
 
 class Caravel(BaseCaravelView):
-
     """The base views for Caravel!"""
 
     @has_access
+    @expose("/explore/<datasource_type>/<datasource_id>/<slice_id>/")
     @expose("/explore/<datasource_type>/<datasource_id>/")
     @expose("/datasource/<datasource_type>/<datasource_id>/")  # 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 @@ def explore(self, datasource_type, datasource_id):
         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")
-        slc = None
 
-        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)
 
-        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)
 
-        action = request.args.get('action')
+        # 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()
+            )
+            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')
+
+        # handle save or overwrite
+        action = slice_params.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)
+
+        # look for viz type
+        viz_type = slice_params.get("viz_type", slc.viz_type if slc else None)
 
-        viz_type = request.args.get("viz_type")
+        # 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,23 +981,26 @@ def explore(self, datasource_type, datasource_id):
                 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,
                 status=200,
                 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 @@ def explore(self, datasource_type, datasource_id):
                     utils.error_msg_from_exception(e),
                     status=500,
                     mimetype="application/json")
+
             return resp
 
     def save_or_overwrite_slice(
@@ -1003,6 +1041,7 @@ def save_or_overwrite_slice(
             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 @@ def favstar(self, class_name, obj_id, action):
             json.dumps({'count': count}),
             mimetype="application/json")
 
-    @has_access
-    @expose("/slice/<slice_id>/")
-    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/<dashboard_id>/")
     def dashboard(self, dashboard_id):

caravel/viz.py

@@ -65,7 +65,7 @@ def __init__(self, datasource, form_data, slice_=None):
         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 @@ def get_url(self, for_cache_key=False, **kwargs):
         # 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()))

tests/core_tests.py

@@ -86,51 +86,15 @@ def test_slices(self):
         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 = {}