mirror of https://github.com/apache/superset.git
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
This commit is contained in:
parent
4f125eedb5
commit
bae21194c1
|
@ -26,6 +26,7 @@ from flask_babel import lazy_gettext as _
|
||||||
from flask_appbuilder.models.sqla.filters import BaseFilter
|
from flask_appbuilder.models.sqla.filters import BaseFilter
|
||||||
|
|
||||||
from sqlalchemy import create_engine
|
from sqlalchemy import create_engine
|
||||||
|
from werkzeug.datastructures import ImmutableMultiDict, MultiDict
|
||||||
from werkzeug.routing import BaseConverter
|
from werkzeug.routing import BaseConverter
|
||||||
from wtforms.validators import ValidationError
|
from wtforms.validators import ValidationError
|
||||||
|
|
||||||
|
@ -866,14 +867,14 @@ appbuilder.add_view_no_menu(R)
|
||||||
|
|
||||||
|
|
||||||
class Caravel(BaseCaravelView):
|
class Caravel(BaseCaravelView):
|
||||||
|
|
||||||
"""The base views for Caravel!"""
|
"""The base views for Caravel!"""
|
||||||
|
|
||||||
@has_access
|
@has_access
|
||||||
|
@expose("/explore/<datasource_type>/<datasource_id>/<slice_id>/")
|
||||||
@expose("/explore/<datasource_type>/<datasource_id>/")
|
@expose("/explore/<datasource_type>/<datasource_id>/")
|
||||||
@expose("/datasource/<datasource_type>/<datasource_id>/") # Legacy url
|
@expose("/datasource/<datasource_type>/<datasource_id>/") # Legacy url
|
||||||
@log_this
|
@log_this
|
||||||
def explore(self, datasource_type, datasource_id):
|
def explore(self, datasource_type, datasource_id, slice_id=None):
|
||||||
|
|
||||||
error_redirect = '/slicemodelview/list/'
|
error_redirect = '/slicemodelview/list/'
|
||||||
datasource_class = models.SqlaTable \
|
datasource_class = models.SqlaTable \
|
||||||
|
@ -886,51 +887,84 @@ class Caravel(BaseCaravelView):
|
||||||
datasources = sorted(datasources, key=lambda ds: ds.full_name)
|
datasources = sorted(datasources, key=lambda ds: ds.full_name)
|
||||||
datasource = [ds for ds in datasources if int(datasource_id) == ds.id]
|
datasource = [ds for ds in datasources if int(datasource_id) == ds.id]
|
||||||
datasource = datasource[0] if datasource else None
|
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
|
slc = None
|
||||||
|
|
||||||
|
slice_params = request.args
|
||||||
|
|
||||||
if slice_id:
|
if slice_id:
|
||||||
slc = (
|
slc = (
|
||||||
db.session.query(models.Slice)
|
db.session.query(models.Slice)
|
||||||
.filter_by(id=slice_id)
|
.filter_by(id=slice_id)
|
||||||
.first()
|
.first()
|
||||||
)
|
)
|
||||||
if not datasource:
|
try:
|
||||||
flash(__("The datasource seems to have been deleted"), "alert")
|
param_dict = json.loads(slc.params)
|
||||||
return redirect(error_redirect)
|
|
||||||
|
|
||||||
|
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_add_perm = self.can_access('can_add', 'SliceModelView')
|
||||||
slice_edit_perm = check_ownership(slc, raise_if_false=False)
|
slice_edit_perm = check_ownership(slc, raise_if_false=False)
|
||||||
slice_download_perm = self.can_access('can_download', 'SliceModelView')
|
slice_download_perm = self.can_access('can_download', 'SliceModelView')
|
||||||
|
|
||||||
all_datasource_access = self.can_access(
|
# handle save or overwrite
|
||||||
'all_datasource_access', 'all_datasource_access')
|
action = slice_params.get('action')
|
||||||
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')
|
|
||||||
if action in ('saveas', 'overwrite'):
|
if action in ('saveas', 'overwrite'):
|
||||||
return self.save_or_overwrite_slice(
|
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:
|
if not viz_type and datasource.default_endpoint:
|
||||||
return redirect(datasource.default_endpoint)
|
return redirect(datasource.default_endpoint)
|
||||||
|
|
||||||
|
# default to table if no default endpoint and no viz_type
|
||||||
if not viz_type:
|
if not viz_type:
|
||||||
viz_type = "table"
|
viz_type = "table"
|
||||||
|
|
||||||
|
# validate viz params
|
||||||
try:
|
try:
|
||||||
obj = viz.viz_types[viz_type](
|
obj = viz.viz_types[viz_type](
|
||||||
datasource,
|
datasource,
|
||||||
form_data=request.args,
|
form_data=slice_params,
|
||||||
slice_=slc)
|
slice_=slc)
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
flash(utils.error_msg_from_exception(e), "danger")
|
flash(utils.error_msg_from_exception(e), "danger")
|
||||||
return redirect(error_redirect)
|
return redirect(error_redirect)
|
||||||
if request.args.get("json") == "true":
|
|
||||||
|
# handle different endpoints
|
||||||
|
if slice_params.get("json") == "true":
|
||||||
status = 200
|
status = 200
|
||||||
if config.get("DEBUG"):
|
if config.get("DEBUG"):
|
||||||
# Allows for nice debugger stack traces in debug mode
|
# Allows for nice debugger stack traces in debug mode
|
||||||
|
@ -947,7 +981,8 @@ class Caravel(BaseCaravelView):
|
||||||
status=status,
|
status=status,
|
||||||
mimetype="application/json")
|
mimetype="application/json")
|
||||||
return resp
|
return resp
|
||||||
elif request.args.get("csv") == "true":
|
|
||||||
|
elif slice_params.get("csv") == "true":
|
||||||
payload = obj.get_csv()
|
payload = obj.get_csv()
|
||||||
return Response(
|
return Response(
|
||||||
payload,
|
payload,
|
||||||
|
@ -955,15 +990,17 @@ class Caravel(BaseCaravelView):
|
||||||
headers=generate_download_headers("csv"),
|
headers=generate_download_headers("csv"),
|
||||||
mimetype="application/csv")
|
mimetype="application/csv")
|
||||||
else:
|
else:
|
||||||
if request.args.get("standalone") == "true":
|
if slice_params.get("standalone") == "true":
|
||||||
template = "caravel/standalone.html"
|
template = "caravel/standalone.html"
|
||||||
else:
|
else:
|
||||||
template = "caravel/explore.html"
|
template = "caravel/explore.html"
|
||||||
|
|
||||||
resp = self.render_template(
|
resp = self.render_template(
|
||||||
template, viz=obj, slice=slc, datasources=datasources,
|
template, viz=obj, slice=slc, datasources=datasources,
|
||||||
can_add=slice_add_perm, can_edit=slice_edit_perm,
|
can_add=slice_add_perm, can_edit=slice_edit_perm,
|
||||||
can_download=slice_download_perm,
|
can_download=slice_download_perm,
|
||||||
userid=g.user.get_id() if g.user else '')
|
userid=g.user.get_id() if g.user else '')
|
||||||
|
|
||||||
try:
|
try:
|
||||||
pass
|
pass
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
@ -973,6 +1010,7 @@ class Caravel(BaseCaravelView):
|
||||||
utils.error_msg_from_exception(e),
|
utils.error_msg_from_exception(e),
|
||||||
status=500,
|
status=500,
|
||||||
mimetype="application/json")
|
mimetype="application/json")
|
||||||
|
|
||||||
return resp
|
return resp
|
||||||
|
|
||||||
def save_or_overwrite_slice(
|
def save_or_overwrite_slice(
|
||||||
|
@ -1003,6 +1041,7 @@ class Caravel(BaseCaravelView):
|
||||||
table_id = args.get('datasource_id')
|
table_id = args.get('datasource_id')
|
||||||
|
|
||||||
if action in ('saveas'):
|
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 = models.Slice(owners=[g.user] if g.user else [])
|
||||||
|
|
||||||
slc.params = json.dumps(d, indent=4, sort_keys=True)
|
slc.params = json.dumps(d, indent=4, sort_keys=True)
|
||||||
|
@ -1216,21 +1255,6 @@ class Caravel(BaseCaravelView):
|
||||||
json.dumps({'count': count}),
|
json.dumps({'count': count}),
|
||||||
mimetype="application/json")
|
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
|
@has_access
|
||||||
@expose("/dashboard/<dashboard_id>/")
|
@expose("/dashboard/<dashboard_id>/")
|
||||||
def dashboard(self, dashboard_id):
|
def dashboard(self, dashboard_id):
|
||||||
|
|
|
@ -65,7 +65,7 @@ class BaseViz(object):
|
||||||
form_class = ff.get_form()
|
form_class = ff.get_form()
|
||||||
defaults = form_class().data.copy()
|
defaults = form_class().data.copy()
|
||||||
previous_viz_type = form_data.get('previous_viz_type')
|
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)
|
form = form_class(form_data)
|
||||||
else:
|
else:
|
||||||
form = form_class(**form_data)
|
form = form_class(**form_data)
|
||||||
|
@ -120,10 +120,16 @@ class BaseViz(object):
|
||||||
# Remove unchecked checkboxes because HTML is weird like that
|
# Remove unchecked checkboxes because HTML is weird like that
|
||||||
od = MultiDict()
|
od = MultiDict()
|
||||||
for key in sorted(d.keys()):
|
for key in sorted(d.keys()):
|
||||||
|
# if MultiDict is initialized with MD({key:[emptyarray]}),
|
||||||
|
# key is included in d.keys() but accessing it throws
|
||||||
|
try:
|
||||||
if d[key] is False:
|
if d[key] is False:
|
||||||
del d[key]
|
del d[key]
|
||||||
else:
|
continue
|
||||||
if isinstance(d, MultiDict):
|
except IndexError:
|
||||||
|
pass
|
||||||
|
|
||||||
|
if isinstance(d, (MultiDict, ImmutableMultiDict)):
|
||||||
v = d.getlist(key)
|
v = d.getlist(key)
|
||||||
else:
|
else:
|
||||||
v = d.get(key)
|
v = d.get(key)
|
||||||
|
@ -131,6 +137,7 @@ class BaseViz(object):
|
||||||
v = [v]
|
v = [v]
|
||||||
for item in v:
|
for item in v:
|
||||||
od.add(key, item)
|
od.add(key, item)
|
||||||
|
|
||||||
href = Href(
|
href = Href(
|
||||||
'/caravel/explore/{self.datasource.type}/'
|
'/caravel/explore/{self.datasource.type}/'
|
||||||
'{self.datasource.id}/'.format(**locals()))
|
'{self.datasource.id}/'.format(**locals()))
|
||||||
|
|
|
@ -86,51 +86,15 @@ class CoreTests(CaravelTestCase):
|
||||||
for slc in db.session.query(Slc).all():
|
for slc in db.session.query(Slc).all():
|
||||||
urls += [
|
urls += [
|
||||||
(slc.slice_name, 'slice_url', slc.slice_url),
|
(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, 'json_endpoint', slc.viz.json_endpoint),
|
||||||
(slc.slice_name, 'csv_endpoint', slc.viz.csv_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:
|
for name, method, url in urls:
|
||||||
print("[{name}]/[{method}]: {url}".format(**locals()))
|
print("[{name}]/[{method}]: {url}".format(**locals()))
|
||||||
self.client.get(url)
|
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):
|
def test_dashboard(self):
|
||||||
self.login(username='admin')
|
self.login(username='admin')
|
||||||
urls = {}
|
urls = {}
|
||||||
|
|
Loading…
Reference in New Issue