From a955d45eafb37889e7e837cb89e0d3fc49d5a9c2 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 3 Jan 2020 15:35:33 +0000 Subject: [PATCH 01/23] [charts] New REST API --- superset/views/api.py | 1 + superset/views/chart/__init__.py | 16 +++ superset/views/chart/api.py | 223 +++++++++++++++++++++++++++++++ superset/views/chart/filters.py | 31 +++++ superset/views/chart/mixin.py | 87 ++++++++++++ superset/views/chart/views.py | 97 ++++++++++++++ superset/views/core.py | 153 +-------------------- 7 files changed, 459 insertions(+), 149 deletions(-) create mode 100644 superset/views/chart/__init__.py create mode 100644 superset/views/chart/api.py create mode 100644 superset/views/chart/filters.py create mode 100644 superset/views/chart/mixin.py create mode 100644 superset/views/chart/views.py diff --git a/superset/views/api.py b/superset/views/api.py index bfe70f0c1ea7..088266f6f342 100644 --- a/superset/views/api.py +++ b/superset/views/api.py @@ -27,6 +27,7 @@ from superset.utils import core as utils from .base import api, BaseSupersetView, handle_api_exception +from .chart import api as chart_api # pylint: disable=unused-import from .dashboard import api as dashboard_api # pylint: disable=unused-import from .database import api as database_api # pylint: disable=unused-import diff --git a/superset/views/chart/__init__.py b/superset/views/chart/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/superset/views/chart/__init__.py @@ -0,0 +1,16 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py new file mode 100644 index 000000000000..68c5335432bb --- /dev/null +++ b/superset/views/chart/api.py @@ -0,0 +1,223 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from flask import current_app, request +from flask_appbuilder.api import expose, protect, safe +from flask_appbuilder.models.sqla.interface import SQLAInterface +from marshmallow import fields, post_load, ValidationError +from marshmallow.validate import Length +from sqlalchemy.exc import SQLAlchemyError + +from superset import appbuilder +from superset.exceptions import SupersetException +from superset.models.slice import Slice +from superset.utils import core as utils +from superset.views.base import BaseSupersetModelRestApi, BaseSupersetSchema + +from .mixin import SliceMixin + + +def validate_json(value): + try: + utils.validate_json(value) + except SupersetException: + raise ValidationError("JSON not valid") + + +def validate_owners(value): + owner = ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one_or_none() + ) + if not owner: + raise ValidationError(f"User {value} does not exist") + + +class ChartPutSchema(BaseSupersetSchema): + slice_name = fields.String(allow_none=True, validate=Length(0, 250)) + description = fields.String(allow_none=True) + viz_type = fields.String(allow_none=True, validate=Length(0, 250)) + owners = fields.List(fields.Integer(validate=validate_owners)) + params = fields.String(allow_none=True) + cache_timeout = fields.Integer() + + @post_load + def make_object(self, data): # pylint: disable=no-self-use + for field in data: + setattr(self.instance, field, data.get(field)) + return self.instance + + +class ChartRestApi(SliceMixin, BaseSupersetModelRestApi): + datamodel = SQLAInterface(Slice) + + resource_name = "chart" + allow_browser_login = True + + class_permission_name = "SliceModelView" + method_permission_name = { + "get_list": "list", + "get": "show", + "post": "add", + "put": "edit", + "delete": "delete", + "info": "list", + "related": "list", + } + show_columns = [ + "slice_name", + "description", + "owners.id", + "owners.username", + "dashboards.id", + "dashboards.dashboard_title", + "viz_type", + "params", + "cache_timeout", + ] + list_columns = [ + "slice_name", + "description", + "changed_by.username", + "changed_by_name", + "changed_on", + "viz_type", + "params", + "cache_timeout", + ] + + add_model_schema = ChartPutSchema() + edit_model_schema = ChartPutSchema() + + order_rel_fields = { + "slices": ("slice_name", "asc"), + "owners": ("first_name", "asc"), + } + filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"} + + @expose("/", methods=["POST"]) + @protect() + @safe + def post(self): + """Creates a new Chart + --- + post: + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + responses: + 201: + description: Dashboard added + content: + application/json: + schema: + type: object + properties: + id: + type: string + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + return self.response_400(message="Request is not JSON") + item = self.add_model_schema.load(request.json) + # This validates custom Schema with custom validations + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.add(item.data, raise_exception=True) + return self.response( + 201, + result=self.add_model_schema.dump(item.data, many=False).data, + id=item.data.id, + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + @expose("/", methods=["PUT"]) + @protect() + @safe + def put(self, pk): + """Changes a Chart + --- + put: + parameters: + - in: path + schema: + type: integer + name: pk + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + responses: + 200: + description: Item changed + content: + application/json: + schema: + type: object + properties: + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + self.response_400(message="Request is not JSON") + item = self.datamodel.get(pk, self._base_filters) + if not item: + return self.response_404() + + item = self.edit_model_schema.load(request.json, instance=item) + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.edit(item.data, raise_exception=True) + return self.response( + 200, result=self.edit_model_schema.dump(item.data, many=False).data + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + +appbuilder.add_api(ChartRestApi) diff --git a/superset/views/chart/filters.py b/superset/views/chart/filters.py new file mode 100644 index 000000000000..f0bea4720670 --- /dev/null +++ b/superset/views/chart/filters.py @@ -0,0 +1,31 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from sqlalchemy import or_ + +from superset import security_manager +from superset.views.base import BaseFilter + + +class SliceFilter(BaseFilter): # pylint: disable=too-few-public-methods + def apply(self, query, value): + if security_manager.all_datasource_access(): + return query + perms = security_manager.user_view_menu_names("datasource_access") + schema_perms = security_manager.user_view_menu_names("schema_access") + return query.filter( + or_(self.model.perm.in_(perms), self.model.schema_perm.in_(schema_perms)) + ) diff --git a/superset/views/chart/mixin.py b/superset/views/chart/mixin.py new file mode 100644 index 000000000000..c5905ea8f118 --- /dev/null +++ b/superset/views/chart/mixin.py @@ -0,0 +1,87 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from flask import Markup +from flask_babel import lazy_gettext as _ + +from superset.views.chart.filters import SliceFilter +from superset.views.dashboard.filters import DashboardFilter + + +class SliceMixin: # pylint: disable=too-few-public-methods + + list_title = _("Charts") + show_title = _("Show Chart") + add_title = _("Add Chart") + edit_title = _("Edit Chart") + + can_add = False + search_columns = ( + "slice_name", + "description", + "viz_type", + "datasource_name", + "owners", + ) + list_columns = ["slice_link", "viz_type", "datasource_link", "creator", "modified"] + order_columns = ["viz_type", "datasource_link", "modified"] + edit_columns = [ + "slice_name", + "description", + "viz_type", + "owners", + "dashboards", + "params", + "cache_timeout", + ] + base_order = ("changed_on", "desc") + description_columns = { + "description": Markup( + "The content here can be displayed as widget headers in the " + "dashboard view. Supports " + '' + "markdown" + ), + "params": _( + "These parameters are generated dynamically when clicking " + "the save or overwrite button in the explore view. This JSON " + "object is exposed here for reference and for power users who may " + "want to alter specific parameters." + ), + "cache_timeout": _( + "Duration (in seconds) of the caching timeout for this chart. " + "Note this defaults to the datasource/table timeout if undefined." + ), + } + base_filters = [["id", SliceFilter, lambda: []]] + label_columns = { + "cache_timeout": _("Cache Timeout"), + "creator": _("Creator"), + "dashboards": _("Dashboards"), + "datasource_link": _("Datasource"), + "description": _("Description"), + "modified": _("Last Modified"), + "owners": _("Owners"), + "params": _("Parameters"), + "slice_link": _("Chart"), + "slice_name": _("Name"), + "table": _("Table"), + "viz_type": _("Visualization Type"), + } + + add_form_query_rel_fields = {"dashboards": [["name", DashboardFilter, None]]} + + edit_form_query_rel_fields = add_form_query_rel_fields diff --git a/superset/views/chart/views.py b/superset/views/chart/views.py new file mode 100644 index 000000000000..b7bd5c7ae647 --- /dev/null +++ b/superset/views/chart/views.py @@ -0,0 +1,97 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import json + +from flask_appbuilder import expose, has_access +from flask_appbuilder.models.sqla.interface import SQLAInterface +from flask_babel import lazy_gettext as _ + +from superset import db +from superset.connectors.connector_registry import ConnectorRegistry +from superset.models.slice import Slice +from superset.utils import core as utils +from superset.views.base import check_ownership, DeleteMixin, SupersetModelView + +from .mixin import SliceMixin + + +class SliceModelView( + SliceMixin, SupersetModelView, DeleteMixin +): # pylint: disable=too-many-ancestors + route_base = "/chart" + datamodel = SQLAInterface(Slice) + + def pre_add(self, item): + utils.validate_json(item.params) + + def pre_update(self, item): + utils.validate_json(item.params) + check_ownership(item) + + def pre_delete(self, item): + check_ownership(item) + + @expose("/add", methods=["GET", "POST"]) + @has_access + def add(self): + datasources = ConnectorRegistry.get_all_datasources(db.session) + datasources = [ + {"value": str(d.id) + "__" + d.type, "label": repr(d)} for d in datasources + ] + return self.render_template( + "superset/add_slice.html", + bootstrap_data=json.dumps( + {"datasources": sorted(datasources, key=lambda d: d["label"])} + ), + ) + + +class SliceAsync(SliceModelView): # pylint: disable=too-many-ancestors + route_base = "/sliceasync" + list_columns = [ + "id", + "slice_link", + "viz_type", + "slice_name", + "creator", + "modified", + "icons", + "changed_on_humanized", + ] + label_columns = {"icons": " ", "slice_link": _("Chart")} + + +class SliceAddView(SliceModelView): # pylint: disable=too-many-ancestors + route_base = "/sliceaddview" + list_columns = [ + "id", + "slice_name", + "slice_url", + "edit_url", + "viz_type", + "params", + "description", + "description_markeddown", + "datasource_id", + "datasource_type", + "datasource_name_text", + "datasource_link", + "owners", + "modified", + "changed_on", + "changed_on_humanized", + ] diff --git a/superset/views/core.py b/superset/views/core.py index 8504ab14a390..bc687b2df33b 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -105,8 +105,8 @@ json_success, SupersetModelView, ) +from .chart import views as chart_views from .dashboard import views as dash_views -from .dashboard.filters import DashboardFilter from .database import views as in_views from .utils import ( apply_display_max_row_limit, @@ -248,17 +248,6 @@ def _deserialize_results_payload( return json.loads(payload) # type: ignore -class SliceFilter(BaseFilter): - def apply(self, query, func): # noqa - if security_manager.all_datasource_access(): - return query - perms = security_manager.user_view_menu_names("datasource_access") - schema_perms = security_manager.user_view_menu_names("schema_access") - return query.filter( - or_(self.model.perm.in_(perms), self.model.schema_perm.in_(schema_perms)) - ) - - if config["ENABLE_ACCESS_REQUEST"]: class AccessRequestsModelView(SupersetModelView, DeleteMixin): @@ -290,101 +279,8 @@ class AccessRequestsModelView(SupersetModelView, DeleteMixin): icon="fa-table", ) - -class SliceModelView(SupersetModelView, DeleteMixin): - route_base = "/chart" - datamodel = SQLAInterface(Slice) - - list_title = _("Charts") - show_title = _("Show Chart") - add_title = _("Add Chart") - edit_title = _("Edit Chart") - - can_add = False - search_columns = ( - "slice_name", - "description", - "viz_type", - "datasource_name", - "owners", - ) - list_columns = ["slice_link", "viz_type", "datasource_link", "creator", "modified"] - order_columns = ["viz_type", "datasource_link", "modified"] - edit_columns = [ - "slice_name", - "description", - "viz_type", - "owners", - "dashboards", - "params", - "cache_timeout", - ] - base_order = ("changed_on", "desc") - description_columns = { - "description": Markup( - "The content here can be displayed as widget headers in the " - "dashboard view. Supports " - '' - "markdown" - ), - "params": _( - "These parameters are generated dynamically when clicking " - "the save or overwrite button in the explore view. This JSON " - "object is exposed here for reference and for power users who may " - "want to alter specific parameters." - ), - "cache_timeout": _( - "Duration (in seconds) of the caching timeout for this chart. " - "Note this defaults to the datasource/table timeout if undefined." - ), - } - base_filters = [["id", SliceFilter, lambda: []]] - label_columns = { - "cache_timeout": _("Cache Timeout"), - "creator": _("Creator"), - "dashboards": _("Dashboards"), - "datasource_link": _("Datasource"), - "description": _("Description"), - "modified": _("Last Modified"), - "owners": _("Owners"), - "params": _("Parameters"), - "slice_link": _("Chart"), - "slice_name": _("Name"), - "table": _("Table"), - "viz_type": _("Visualization Type"), - } - - add_form_query_rel_fields = {"dashboards": [["name", DashboardFilter, None]]} - - edit_form_query_rel_fields = add_form_query_rel_fields - - def pre_add(self, obj): - utils.validate_json(obj.params) - - def pre_update(self, obj): - utils.validate_json(obj.params) - check_ownership(obj) - - def pre_delete(self, obj): - check_ownership(obj) - - @expose("/add", methods=["GET", "POST"]) - @has_access - def add(self): - datasources = ConnectorRegistry.get_all_datasources(db.session) - datasources = [ - {"value": str(d.id) + "__" + d.type, "label": repr(d)} for d in datasources - ] - return self.render_template( - "superset/add_slice.html", - bootstrap_data=json.dumps( - {"datasources": sorted(datasources, key=lambda d: d["label"])} - ), - ) - - appbuilder.add_view( - SliceModelView, + chart_views.SliceModelView, "Charts", label=__("Charts"), icon="fa-bar-chart", @@ -392,49 +288,8 @@ def add(self): category_icon="", ) - -class SliceAsync(SliceModelView): - route_base = "/sliceasync" - list_columns = [ - "id", - "slice_link", - "viz_type", - "slice_name", - "creator", - "modified", - "icons", - "changed_on_humanized", - ] - label_columns = {"icons": " ", "slice_link": _("Chart")} - - -appbuilder.add_view_no_menu(SliceAsync) - - -class SliceAddView(SliceModelView): - route_base = "/sliceaddview" - list_columns = [ - "id", - "slice_name", - "slice_url", - "edit_url", - "viz_type", - "params", - "description", - "description_markeddown", - "datasource_id", - "datasource_type", - "datasource_name_text", - "datasource_link", - "owners", - "modified", - "changed_on", - "changed_on_humanized", - ] - - -appbuilder.add_view_no_menu(SliceAddView) - +appbuilder.add_view_no_menu(chart_views.SliceAsync) +appbuilder.add_view_no_menu(chart_views.SliceAddView) appbuilder.add_view( dash_views.DashboardModelView, From 00000ebfee71873c4bbf4f6526e0b1fc45f866c6 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 11:10:10 +0000 Subject: [PATCH 02/23] [charts] Small improvements --- superset/views/api.py | 9 ++++----- superset/views/chart/api.py | 19 ++++++++++--------- superset/views/chart/views.py | 3 +-- superset/views/core.py | 3 +-- superset/views/dashboard/api.py | 16 +++++++++------- 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/superset/views/api.py b/superset/views/api.py index 088266f6f342..90018d495ddb 100644 --- a/superset/views/api.py +++ b/superset/views/api.py @@ -25,11 +25,10 @@ from superset.legacy import update_time_range from superset.models.slice import Slice from superset.utils import core as utils - -from .base import api, BaseSupersetView, handle_api_exception -from .chart import api as chart_api # pylint: disable=unused-import -from .dashboard import api as dashboard_api # pylint: disable=unused-import -from .database import api as database_api # pylint: disable=unused-import +from superset.views.base import api, BaseSupersetView, handle_api_exception +from superset.views.chart import api as chart_api # pylint: disable=unused-import +from superset.views.dashboard import api as dashboard_api # pylint: disable=unused-import +from superset.views.database import api as database_api # pylint: disable=unused-import class Api(BaseSupersetView): diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 68c5335432bb..540cc205ec26 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -20,14 +20,14 @@ from marshmallow import fields, post_load, ValidationError from marshmallow.validate import Length from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound from superset import appbuilder from superset.exceptions import SupersetException from superset.models.slice import Slice from superset.utils import core as utils from superset.views.base import BaseSupersetModelRestApi, BaseSupersetSchema - -from .mixin import SliceMixin +from superset.views.chart.mixin import SliceMixin def validate_json(value): @@ -38,14 +38,15 @@ def validate_json(value): def validate_owners(value): - owner = ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id + try: + ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one() ) - .filter_by(id=value) - .one_or_none() - ) - if not owner: + except NoResultFound: raise ValidationError(f"User {value} does not exist") diff --git a/superset/views/chart/views.py b/superset/views/chart/views.py index b7bd5c7ae647..d6a0c7ed8818 100644 --- a/superset/views/chart/views.py +++ b/superset/views/chart/views.py @@ -25,8 +25,7 @@ from superset.models.slice import Slice from superset.utils import core as utils from superset.views.base import check_ownership, DeleteMixin, SupersetModelView - -from .mixin import SliceMixin +from superset.views.chart.mixin import SliceMixin class SliceModelView( diff --git a/superset/views/core.py b/superset/views/core.py index bc687b2df33b..dd8fa2d465c3 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -19,7 +19,6 @@ import re from contextlib import closing from datetime import datetime, timedelta -from enum import Enum from typing import Any, cast, Dict, List, Optional, Union from urllib import parse @@ -105,7 +104,7 @@ json_success, SupersetModelView, ) -from .chart import views as chart_views +from superset.views.chart import views as chart_views from .dashboard import views as dash_views from .database import views as in_views from .utils import ( diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index 506847873880..927304426c45 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -23,6 +23,7 @@ from marshmallow import fields, post_load, pre_load, Schema, ValidationError from marshmallow.validate import Length from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound import superset.models.core as models from superset import appbuilder @@ -76,14 +77,15 @@ def validate_slug_uniqueness(value): def validate_owners(value): - owner = ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id + try: + ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one() ) - .filter_by(id=value) - .one_or_none() - ) - if not owner: + except NoResultFound: raise ValidationError(f"User {value} does not exist") From 1044bb8826d17f5150d73b6318de33014a4bc6cd Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 11:18:34 +0000 Subject: [PATCH 03/23] [charts] Fix, lint --- superset/views/api.py | 4 +++- superset/views/core.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/views/api.py b/superset/views/api.py index 90018d495ddb..2df52e371f35 100644 --- a/superset/views/api.py +++ b/superset/views/api.py @@ -27,7 +27,9 @@ from superset.utils import core as utils from superset.views.base import api, BaseSupersetView, handle_api_exception from superset.views.chart import api as chart_api # pylint: disable=unused-import -from superset.views.dashboard import api as dashboard_api # pylint: disable=unused-import +from superset.views.dashboard import ( # pylint: disable=unused-import + api as dashboard_api, +) from superset.views.database import api as database_api # pylint: disable=unused-import diff --git a/superset/views/core.py b/superset/views/core.py index dd8fa2d465c3..f92af099b0d4 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -86,6 +86,7 @@ from superset.utils import core as utils, dashboard_import_export from superset.utils.dates import now_as_float from superset.utils.decorators import etag_cache, stats_timing +from superset.views.chart import views as chart_views from .base import ( api, @@ -104,7 +105,6 @@ json_success, SupersetModelView, ) -from superset.views.chart import views as chart_views from .dashboard import views as dash_views from .database import views as in_views from .utils import ( From d06421fa7db8be77cbca03a5b787119b16f9396f Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 13:05:05 +0000 Subject: [PATCH 04/23] [charts] Tests and datasource validation --- superset/views/chart/api.py | 55 ++++++++++++++++++- tests/base_tests.py | 9 +++ tests/chart_api_tests.py | 103 +++++++++++++++++++++++++++++++++++ tests/dashboard_api_tests.py | 8 --- 4 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 tests/chart_api_tests.py diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 540cc205ec26..f1b0f1aac24f 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -17,12 +17,13 @@ from flask import current_app, request from flask_appbuilder.api import expose, protect, safe from flask_appbuilder.models.sqla.interface import SQLAInterface -from marshmallow import fields, post_load, ValidationError +from marshmallow import fields, post_load, validates_schema, ValidationError from marshmallow.validate import Length from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound from superset import appbuilder +from superset.connectors.connector_registry import ConnectorRegistry from superset.exceptions import SupersetException from superset.models.slice import Slice from superset.utils import core as utils @@ -50,6 +51,54 @@ def validate_owners(value): raise ValidationError(f"User {value} does not exist") +class BaseChartSchema(BaseSupersetSchema): + @staticmethod + def set_owners(instance, owners): + owner_objs = list() + for owner_id in owners: + user = current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model + ).get(owner_id) + owner_objs.append(user) + instance.owners = owner_objs + + +class ChartPostSchema(BaseChartSchema): + slice_name = fields.String(validate=Length(1, 250)) + description = fields.String(allow_none=True) + viz_type = fields.String(allow_none=True, validate=Length(0, 250)) + owners = fields.List(fields.Integer(validate=validate_owners)) + params = fields.String(allow_none=True) + cache_timeout = fields.Integer() + datasource_id = fields.Integer() + datasource_type = fields.String() + datasource_name = fields.String(allow_none=True) + + @validates_schema + def validate_datasource(self, data, **kwargs): + datasource_type = data["datasource_type"] + datasource_id = data["datasource_id"] + try: + datasource = ConnectorRegistry.get_datasource( + datasource_type, datasource_id, current_app.appbuilder.get_session + ) + except NoResultFound: + raise ValidationError( + f"Datasource [{datasource_type}].{datasource_id} does not exist" + ) + data["datasource_name"] = datasource.name + + @post_load + def make_object(self, data): # pylint: disable=no-self-use + instance = Slice() + for field in data: + if field == "owners": + self.set_owners(instance, data["owners"]) + else: + setattr(instance, field, data.get(field)) + return instance + + class ChartPutSchema(BaseSupersetSchema): slice_name = fields.String(allow_none=True, validate=Length(0, 250)) description = fields.String(allow_none=True) @@ -57,6 +106,8 @@ class ChartPutSchema(BaseSupersetSchema): owners = fields.List(fields.Integer(validate=validate_owners)) params = fields.String(allow_none=True) cache_timeout = fields.Integer() + datasource_id = fields.Integer(allow_none=True) + datasource_type = fields.String(allow_none=True) @post_load def make_object(self, data): # pylint: disable=no-self-use @@ -103,7 +154,7 @@ class ChartRestApi(SliceMixin, BaseSupersetModelRestApi): "cache_timeout", ] - add_model_schema = ChartPutSchema() + add_model_schema = ChartPostSchema() edit_model_schema = ChartPutSchema() order_rel_fields = { diff --git a/tests/base_tests.py b/tests/base_tests.py index 0549c97b209b..10c557fa2172 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -61,6 +61,15 @@ def create_user( username, first_name, last_name, email, role_admin, password ) + @staticmethod + def get_user(username: str) -> ab_models.User: + user = ( + db.session.query(security_manager.user_model) + .filter_by(username=username) + .one_or_none() + ) + return user + @classmethod def create_druid_test_objects(cls): # create druid cluster and druid datasources diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py new file mode 100644 index 000000000000..5068a3fd03c1 --- /dev/null +++ b/tests/chart_api_tests.py @@ -0,0 +1,103 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Unit tests for Superset""" +import json +from typing import List + +import prison +from flask_appbuilder.security.sqla import models as ab_models + +from superset import db, security_manager +from superset.connectors.connector_registry import ConnectorRegistry +from superset.models.slice import Slice + +from .base_tests import SupersetTestCase + + +class ChartApiTests(SupersetTestCase): + def __init__(self, *args, **kwargs): + super(ChartApiTests, self).__init__(*args, **kwargs) + + def insert_chart( + self, + slice_name: str, + owners: List[int], + datasource_id: int, + datasource_type: str = "table", + description: str = "", + viz_type: str = "", + params: str = "", + cache_timeout: int = 30, + ) -> Slice: + obj_owners = list() + for owner in owners: + user = db.session.query(security_manager.user_model).get(owner) + obj_owners.append(user) + datasource = ConnectorRegistry.get_datasource( + datasource_type, datasource_id, db.session + ) + slice = Slice( + slice_name=slice_name, + datasource_id=datasource.id, + datasource_name=datasource.name, + datasource_type=datasource.type, + owners=obj_owners, + description=description, + viz_type=viz_type, + params=params, + cache_timeout=cache_timeout, + ) + db.session.add(slice) + db.session.commit() + return slice + + def test_delete_chart(self): + """ + Chart API: Test delete + """ + admin_id = self.get_user("admin").id + chart_id = self.insert_chart("name", [admin_id], 1).id + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.client.delete(uri) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + self.assertEqual(model, None) + + def test_delete_not_found_chart(self): + """ + Chart API: Test not found delete + """ + self.login(username="admin") + dashboard_id = 1000 + uri = f"api/v1/chart/{dashboard_id}" + rv = self.client.delete(uri) + self.assertEqual(rv.status_code, 404) + + def test_delete_chart_admin_not_owned(self): + """ + Dashboard API: Test admin delete not owned + """ + gamma_id = self.get_user("gamma").id + chart_id = self.insert_chart("title", [gamma_id], 1).id + + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.client.delete(uri) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + self.assertEqual(model, None) diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index 9a60d0a422c7..ad00063f1725 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -58,14 +58,6 @@ def insert_dashboard( db.session.commit() return dashboard - def get_user(self, username: str) -> ab_models.User: - user = ( - db.session.query(security_manager.user_model) - .filter_by(username=username) - .one_or_none() - ) - return user - def test_delete_dashboard(self): """ Dashboard API: Test delete From 08969bf64c2cb497209b9ea02ce3145738c14053 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 14:12:56 +0000 Subject: [PATCH 05/23] [charts] Fix, lint --- superset/views/chart/api.py | 3 ++- tests/base_tests.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index f1b0f1aac24f..418c14b1cad4 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -74,8 +74,9 @@ class ChartPostSchema(BaseChartSchema): datasource_type = fields.String() datasource_name = fields.String(allow_none=True) + @staticmethod @validates_schema - def validate_datasource(self, data, **kwargs): + def validate_datasource(data): datasource_type = data["datasource_type"] datasource_id = data["datasource_id"] try: diff --git a/tests/base_tests.py b/tests/base_tests.py index 10c557fa2172..c314cda18297 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -65,8 +65,8 @@ def create_user( def get_user(username: str) -> ab_models.User: user = ( db.session.query(security_manager.user_model) - .filter_by(username=username) - .one_or_none() + .filter_by(username=username) + .one_or_none() ) return user From c90010dd5544afe610e3dd9bd9fbce7536b6a4c4 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 14:56:56 +0000 Subject: [PATCH 06/23] [charts] DRY post schemas --- superset/views/base.py | 48 +++++++++++++++++++++++++++++++-- superset/views/chart/api.py | 34 +++++------------------ superset/views/dashboard/api.py | 30 ++++----------------- 3 files changed, 58 insertions(+), 54 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 3a5dd64944a8..8faebf1a18e5 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -22,7 +22,16 @@ import simplejson as json import yaml -from flask import abort, flash, g, get_flashed_messages, redirect, Response, session +from flask import ( + abort, + current_app, + flash, + g, + get_flashed_messages, + redirect, + Response, + session, +) from flask_appbuilder import BaseView, Model, ModelRestApi, ModelView from flask_appbuilder.actions import action from flask_appbuilder.api import expose, protect, rison, safe @@ -32,7 +41,7 @@ from flask_appbuilder.widgets import ListWidget from flask_babel import get_locale, gettext as __, lazy_gettext as _ from flask_wtf.form import FlaskForm -from marshmallow import Schema +from marshmallow import post_load, pre_load, Schema from sqlalchemy import or_ from werkzeug.exceptions import HTTPException from wtforms.fields.core import Field, UnboundField @@ -376,6 +385,41 @@ def load( return super().load(data, many=many, partial=partial, **kwargs) +class BaseOwnedSchema(BaseSupersetSchema): + """ + Implements owners validation and pre load + """ + + __class_model__ = None + + @post_load + def make_object(self, data): # pylint: disable=no-self-use + instance = self.__class_model__() + self.set_owners(instance, data["owners"]) + for field in data: + if field == "owners": + self.set_owners(instance, data["owners"]) + else: + setattr(instance, field, data.get(field)) + return instance + + @pre_load + def pre_load(self, data): # pylint: disable=no-self-use + data["owners"] = data.get("owners", []) + + @staticmethod + def set_owners(instance, owners): + owner_objs = list() + if g.user.id not in owners: + owners.append(g.user.id) + for owner_id in owners: + user = current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model + ).get(owner_id) + owner_objs.append(user) + instance.owners = owner_objs + + get_related_schema = { "type": "object", "properties": { diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 418c14b1cad4..93eb4fc1fa1d 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -27,7 +27,7 @@ from superset.exceptions import SupersetException from superset.models.slice import Slice from superset.utils import core as utils -from superset.views.base import BaseSupersetModelRestApi, BaseSupersetSchema +from superset.views.base import BaseOwnedSchema, BaseSupersetModelRestApi from superset.views.chart.mixin import SliceMixin @@ -51,27 +51,17 @@ def validate_owners(value): raise ValidationError(f"User {value} does not exist") -class BaseChartSchema(BaseSupersetSchema): - @staticmethod - def set_owners(instance, owners): - owner_objs = list() - for owner_id in owners: - user = current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model - ).get(owner_id) - owner_objs.append(user) - instance.owners = owner_objs - +class ChartPostSchema(BaseOwnedSchema): + __class_model__ = Slice -class ChartPostSchema(BaseChartSchema): - slice_name = fields.String(validate=Length(1, 250)) + slice_name = fields.String(required=True, validate=Length(1, 250)) description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) owners = fields.List(fields.Integer(validate=validate_owners)) params = fields.String(allow_none=True) cache_timeout = fields.Integer() - datasource_id = fields.Integer() - datasource_type = fields.String() + datasource_id = fields.Integer(required=True) + datasource_type = fields.String(required=True) datasource_name = fields.String(allow_none=True) @staticmethod @@ -89,18 +79,8 @@ def validate_datasource(data): ) data["datasource_name"] = datasource.name - @post_load - def make_object(self, data): # pylint: disable=no-self-use - instance = Slice() - for field in data: - if field == "owners": - self.set_owners(instance, data["owners"]) - else: - setattr(instance, field, data.get(field)) - return instance - -class ChartPutSchema(BaseSupersetSchema): +class ChartPutSchema(BaseOwnedSchema): slice_name = fields.String(allow_none=True, validate=Length(0, 250)) description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index 927304426c45..c4359c4d7567 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -29,7 +29,7 @@ from superset import appbuilder from superset.exceptions import SupersetException from superset.utils import core as utils -from superset.views.base import BaseSupersetModelRestApi, BaseSupersetSchema +from superset.views.base import BaseOwnedSchema, BaseSupersetModelRestApi from .mixin import DashboardMixin @@ -89,21 +89,10 @@ def validate_owners(value): raise ValidationError(f"User {value} does not exist") -class BaseDashboardSchema(BaseSupersetSchema): - @staticmethod - def set_owners(instance, owners): - owner_objs = list() - if g.user.id not in owners: - owners.append(g.user.id) - for owner_id in owners: - user = current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model - ).get(owner_id) - owner_objs.append(user) - instance.owners = owner_objs - +class BaseDashboardSchema(BaseOwnedSchema): @pre_load def pre_load(self, data): # pylint: disable=no-self-use + super().pre_load(data) data["slug"] = data.get("slug") data["owners"] = data.get("owners", []) if data["slug"]: @@ -113,6 +102,8 @@ def pre_load(self, data): # pylint: disable=no-self-use class DashboardPostSchema(BaseDashboardSchema): + __class_model__ = models.Dashboard + dashboard_title = fields.String(allow_none=True, validate=Length(0, 500)) slug = fields.String( allow_none=True, validate=[Length(1, 255), validate_slug_uniqueness] @@ -123,17 +114,6 @@ class DashboardPostSchema(BaseDashboardSchema): json_metadata = fields.String(validate=validate_json_metadata) published = fields.Boolean() - @post_load - def make_object(self, data): # pylint: disable=no-self-use - instance = models.Dashboard() - self.set_owners(instance, data["owners"]) - for field in data: - if field == "owners": - self.set_owners(instance, data["owners"]) - else: - setattr(instance, field, data.get(field)) - return instance - class DashboardPutSchema(BaseDashboardSchema): dashboard_title = fields.String(allow_none=True, validate=Length(0, 500)) From c20a1582aa61ee0dade3e04c1b8c463213bb561e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 15:19:50 +0000 Subject: [PATCH 07/23] [charts] lint and improve type declarations --- superset/views/base.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 8faebf1a18e5..1ac69319d0ef 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -18,7 +18,7 @@ import logging import traceback from datetime import datetime -from typing import Any, Dict, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple import simplejson as json import yaml @@ -387,14 +387,14 @@ def load( class BaseOwnedSchema(BaseSupersetSchema): """ - Implements owners validation and pre load + Implements owners validation and pre load on Marshmallow schemas """ - __class_model__ = None + __class_model__: Model = None @post_load - def make_object(self, data): # pylint: disable=no-self-use - instance = self.__class_model__() + def make_object(self, data: Dict): + instance = self.__class_model__() # pylint: disable=not-callable self.set_owners(instance, data["owners"]) for field in data: if field == "owners": @@ -404,11 +404,11 @@ def make_object(self, data): # pylint: disable=no-self-use return instance @pre_load - def pre_load(self, data): # pylint: disable=no-self-use + def pre_load(self, data: Dict): # pylint: disable=no-self-use data["owners"] = data.get("owners", []) @staticmethod - def set_owners(instance, owners): + def set_owners(instance: Model, owners: List[int]): owner_objs = list() if g.user.id not in owners: owners.append(g.user.id) From 98d841a6918d0ee1459400aad9de06bc4e72a94b Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 16:14:12 +0000 Subject: [PATCH 08/23] [charts] DRY owned REST APIs --- superset/views/base.py | 151 ++++++++++++++++++++++++++++++ superset/views/chart/api.py | 117 +---------------------- superset/views/dashboard/api.py | 159 +------------------------------- 3 files changed, 158 insertions(+), 269 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 98835ef786a1..95665b9a7ab5 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -29,6 +29,7 @@ g, get_flashed_messages, redirect, + request, Response, session, ) @@ -43,6 +44,7 @@ from flask_wtf.form import FlaskForm from marshmallow import post_load, pre_load, Schema from sqlalchemy import or_ +from sqlalchemy.exc import SQLAlchemyError from werkzeug.exceptions import HTTPException from wtforms.fields.core import Field, UnboundField @@ -564,6 +566,155 @@ def related(self, column_name: str, **kwargs): return self.response(200, count=count, result=result) +class BaseOwnedModelRestApi(BaseSupersetModelRestApi): + @expose("/", methods=["PUT"]) + @protect() + @check_ownership_and_item_exists + @safe + def put(self, item): # pylint: disable=arguments-differ + """Changes a dashboard + --- + put: + parameters: + - in: path + schema: + type: integer + name: pk + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + responses: + 200: + description: Item changed + content: + application/json: + schema: + type: object + properties: + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + self.response_400(message="Request is not JSON") + item = self.edit_model_schema.load(request.json, instance=item) + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.edit(item.data, raise_exception=True) + return self.response( + 200, result=self.edit_model_schema.dump(item.data, many=False).data + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + @expose("/", methods=["POST"]) + @protect() + @safe + def post(self): + """Creates a new dashboard + --- + post: + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + responses: + 201: + description: Dashboard added + content: + application/json: + schema: + type: object + properties: + id: + type: string + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + return self.response_400(message="Request is not JSON") + item = self.add_model_schema.load(request.json) + # This validates custom Schema with custom validations + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.add(item.data, raise_exception=True) + return self.response( + 201, + result=self.add_model_schema.dump(item.data, many=False).data, + id=item.data.id, + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + @expose("/", methods=["DELETE"]) + @protect() + @check_ownership_and_item_exists + @safe + def delete(self, item): # pylint: disable=arguments-differ + """Delete Dashboard + --- + delete: + parameters: + - in: path + schema: + type: integer + name: pk + responses: + 200: + description: Dashboard delete + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + self.datamodel.delete(item, raise_exception=True) + return self.response(200, message="OK") + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + class CsvResponse(Response): # pylint: disable=too-many-ancestors """ Override Response to take into account csv encoding from config.py diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 93eb4fc1fa1d..fa257e72c76a 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -14,12 +14,10 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from flask import current_app, request -from flask_appbuilder.api import expose, protect, safe +from flask import current_app from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, validates_schema, ValidationError from marshmallow.validate import Length -from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound from superset import appbuilder @@ -27,7 +25,7 @@ from superset.exceptions import SupersetException from superset.models.slice import Slice from superset.utils import core as utils -from superset.views.base import BaseOwnedSchema, BaseSupersetModelRestApi +from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema from superset.views.chart.mixin import SliceMixin @@ -97,7 +95,7 @@ def make_object(self, data): # pylint: disable=no-self-use return self.instance -class ChartRestApi(SliceMixin, BaseSupersetModelRestApi): +class ChartRestApi(SliceMixin, BaseOwnedModelRestApi): datamodel = SQLAInterface(Slice) resource_name = "chart" @@ -134,6 +132,7 @@ class ChartRestApi(SliceMixin, BaseSupersetModelRestApi): "params", "cache_timeout", ] + exclude_route_methods = ("info",) add_model_schema = ChartPostSchema() edit_model_schema = ChartPutSchema() @@ -144,113 +143,5 @@ class ChartRestApi(SliceMixin, BaseSupersetModelRestApi): } filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"} - @expose("/", methods=["POST"]) - @protect() - @safe - def post(self): - """Creates a new Chart - --- - post: - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - responses: - 201: - description: Dashboard added - content: - application/json: - schema: - type: object - properties: - id: - type: string - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - return self.response_400(message="Request is not JSON") - item = self.add_model_schema.load(request.json) - # This validates custom Schema with custom validations - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.add(item.data, raise_exception=True) - return self.response( - 201, - result=self.add_model_schema.dump(item.data, many=False).data, - id=item.data.id, - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - @expose("/", methods=["PUT"]) - @protect() - @safe - def put(self, pk): - """Changes a Chart - --- - put: - parameters: - - in: path - schema: - type: integer - name: pk - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - responses: - 200: - description: Item changed - content: - application/json: - schema: - type: object - properties: - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - self.response_400(message="Request is not JSON") - item = self.datamodel.get(pk, self._base_filters) - if not item: - return self.response_404() - - item = self.edit_model_schema.load(request.json, instance=item) - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.edit(item.data, raise_exception=True) - return self.response( - 200, result=self.edit_model_schema.dump(item.data, many=False).data - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - appbuilder.add_api(ChartRestApi) diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index a7993ad2c6cc..e43d81c25698 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -17,23 +17,17 @@ import json import re -from flask import current_app, g, request -from flask_appbuilder.api import expose, protect, safe +from flask import current_app, g from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, pre_load, Schema, ValidationError from marshmallow.validate import Length -from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound from superset import appbuilder from superset.exceptions import SupersetException from superset.models.dashboard import Dashboard from superset.utils import core as utils -from superset.views.base import ( - BaseSupersetModelRestApi, - BaseOwnedSchema, - check_ownership_and_item_exists, -) +from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema from .mixin import DashboardMixin @@ -142,7 +136,7 @@ def make_object(self, data): # pylint: disable=no-self-use return self.instance -class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi): +class DashboardRestApi(DashboardMixin, BaseOwnedModelRestApi): datamodel = SQLAInterface(Dashboard) resource_name = "dashboard" @@ -193,152 +187,5 @@ class DashboardRestApi(DashboardMixin, BaseSupersetModelRestApi): } filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"} - @expose("/", methods=["PUT"]) - @protect() - @check_ownership_and_item_exists - @safe - def put(self, item): # pylint: disable=arguments-differ - """Changes a dashboard - --- - put: - parameters: - - in: path - schema: - type: integer - name: pk - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - responses: - 200: - description: Item changed - content: - application/json: - schema: - type: object - properties: - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 403: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - self.response_400(message="Request is not JSON") - item = self.edit_model_schema.load(request.json, instance=item) - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.edit(item.data, raise_exception=True) - return self.response( - 200, result=self.edit_model_schema.dump(item.data, many=False).data - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - @expose("/", methods=["POST"]) - @protect() - @safe - def post(self): - """Creates a new dashboard - --- - post: - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - responses: - 201: - description: Dashboard added - content: - application/json: - schema: - type: object - properties: - id: - type: string - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - return self.response_400(message="Request is not JSON") - item = self.add_model_schema.load(request.json) - # This validates custom Schema with custom validations - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.add(item.data, raise_exception=True) - return self.response( - 201, - result=self.add_model_schema.dump(item.data, many=False).data, - id=item.data.id, - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - @expose("/", methods=["DELETE"]) - @protect() - @check_ownership_and_item_exists - @safe - def delete(self, item): # pylint: disable=arguments-differ - """Delete Dashboard - --- - delete: - parameters: - - in: path - schema: - type: integer - name: pk - responses: - 200: - description: Dashboard delete - content: - application/json: - schema: - type: object - properties: - message: - type: string - 401: - $ref: '#/components/responses/401' - 403: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - try: - self.datamodel.delete(item, raise_exception=True) - return self.response(200, message="OK") - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - appbuilder.add_api(DashboardRestApi) From f33eae746ff6fc1382ac90ed71960f7550a88466 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 16:49:22 +0000 Subject: [PATCH 09/23] [charts] Small fixes --- superset/views/base.py | 10 +++++----- superset/views/chart/api.py | 10 ++++++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 95665b9a7ab5..9462123fe8da 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -572,7 +572,7 @@ class BaseOwnedModelRestApi(BaseSupersetModelRestApi): @check_ownership_and_item_exists @safe def put(self, item): # pylint: disable=arguments-differ - """Changes a dashboard + """Changes a owned Model --- put: parameters: @@ -627,7 +627,7 @@ def put(self, item): # pylint: disable=arguments-differ @protect() @safe def post(self): - """Creates a new dashboard + """Creates a new owned Model --- post: requestBody: @@ -639,7 +639,7 @@ def post(self): $ref: '#/components/schemas/{{self.__class__.__name__}}.post' responses: 201: - description: Dashboard added + description: Model added content: application/json: schema: @@ -679,7 +679,7 @@ def post(self): @check_ownership_and_item_exists @safe def delete(self, item): # pylint: disable=arguments-differ - """Delete Dashboard + """Deletes owned Model --- delete: parameters: @@ -689,7 +689,7 @@ def delete(self, item): # pylint: disable=arguments-differ name: pk responses: 200: - description: Dashboard delete + description: Model delete content: application/json: schema: diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index fa257e72c76a..df834a9a16ec 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -132,7 +132,13 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi): "params", "cache_timeout", ] - exclude_route_methods = ("info",) + # Will just affect _info endpoint + edit_columns = [ + "slice_name", + ] + add_columns = edit_columns + +# exclude_route_methods = ("info",) add_model_schema = ChartPostSchema() edit_model_schema = ChartPutSchema() @@ -141,7 +147,7 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi): "slices": ("slice_name", "asc"), "owners": ("first_name", "asc"), } - filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"} + filter_rel_fields_field = {"owners": "first_name", "dashboards": "dashboard_title"} appbuilder.add_api(ChartRestApi) From 712436b219961bee89d298434387214d312c261b Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Mon, 6 Jan 2020 19:01:46 +0000 Subject: [PATCH 10/23] [charts] More tests --- tests/chart_api_tests.py | 66 ++++++++++++++++++++++++++++++++++-- tests/dashboard_api_tests.py | 2 +- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py index 5068a3fd03c1..648518241302 100644 --- a/tests/chart_api_tests.py +++ b/tests/chart_api_tests.py @@ -83,8 +83,8 @@ def test_delete_not_found_chart(self): Chart API: Test not found delete """ self.login(username="admin") - dashboard_id = 1000 - uri = f"api/v1/chart/{dashboard_id}" + chart_id = 1000 + uri = f"api/v1/chart/{chart_id}" rv = self.client.delete(uri) self.assertEqual(rv.status_code, 404) @@ -101,3 +101,65 @@ def test_delete_chart_admin_not_owned(self): self.assertEqual(rv.status_code, 200) model = db.session.query(Slice).get(chart_id) self.assertEqual(model, None) + + def test_delete_chart_not_owned(self): + """ + Chart API: Test delete try not owned + """ + user_alpha1 = self.create_user( + "alpha1", "password", "Alpha", email="alpha1@superset.org" + ) + user_alpha2 = self.create_user( + "alpha2", "password", "Alpha", email="alpha2@superset.org" + ) + chart = self.insert_chart("title", [user_alpha1.id], 1) + self.login(username="alpha2", password="password") + uri = f"api/v1/chart/{chart.id}" + rv = self.client.delete(uri) + self.assertEqual(rv.status_code, 403) + db.session.delete(chart) + db.session.delete(user_alpha1) + db.session.delete(user_alpha2) + db.session.commit() + + def test_create_chart(self): + """ + Chart API: Test create chart + """ + admin_id = self.get_user("admin").id + chart_data = { + "slice_name": "name1", + "description": "description1", + "owners": [admin_id], + "viz_type": "viz_type1", + "params": "1234", + "cache_timeout": 1000, + "datasource_id": 1, + "datasource_type": "table", + } + self.login(username="admin") + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 201) + data = json.loads(rv.data.decode("utf-8")) + model = db.session.query(Slice).get(data.get("id")) + db.session.delete(model) + db.session.commit() + + def test_create_simple_chart(self): + """ + Dashboard API: Test create simple chart + """ + dashboard_data = { + "dashboard_title": "title1", + "datasource_id": 1, + "datasource_type": "table", + } + self.login(username="admin") + uri = f"api/v1/dashboard/" + rv = self.client.post(uri, json=dashboard_data) + self.assertEqual(rv.status_code, 201) + data = json.loads(rv.data.decode("utf-8")) + model = db.session.query(Slice).get(data.get("id")) + db.session.delete(model) + db.session.commit() diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index 99c2970edbd9..dda8942957ef 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -372,7 +372,7 @@ def test_get_related_owners(self): "result": [ {"text": "admin user", "value": 1}, {"text": "alpha user", "value": 5}, - {"text": "explore_beta user", "value": 6}, + {"text": "explore_beta user", "value": 8}, {"text": "gamma user", "value": 2}, {"text": "gamma2 user", "value": 3}, {"text": "gamma_sqllab user", "value": 4}, From 17d7f96e1267632f5e71610f7e19ba73057c5054 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 7 Jan 2020 01:50:15 +0000 Subject: [PATCH 11/23] [charts] Tests and DRY --- superset/views/base.py | 16 +++++++++++++++- superset/views/chart/api.py | 25 +++++-------------------- superset/views/dashboard/api.py | 19 +++---------------- tests/dashboard_api_tests.py | 31 ++++++++++++++++--------------- 4 files changed, 39 insertions(+), 52 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 9462123fe8da..a0242f33821e 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -42,9 +42,10 @@ from flask_appbuilder.widgets import ListWidget from flask_babel import get_locale, gettext as __, lazy_gettext as _ from flask_wtf.form import FlaskForm -from marshmallow import post_load, pre_load, Schema +from marshmallow import post_load, pre_load, Schema, ValidationError from sqlalchemy import or_ from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound from werkzeug.exceptions import HTTPException from wtforms.fields.core import Field, UnboundField @@ -407,6 +408,19 @@ def load( return super().load(data, many=many, partial=partial, **kwargs) +def validate_owner(value): + try: + ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one() + ) + except NoResultFound: + raise ValidationError(f"User {value} does not exist") + + class BaseOwnedSchema(BaseSupersetSchema): """ Implements owners validation and pre load on Marshmallow schemas diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index df834a9a16ec..794f195dd975 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -25,7 +25,7 @@ from superset.exceptions import SupersetException from superset.models.slice import Slice from superset.utils import core as utils -from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema +from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema, validate_owner from superset.views.chart.mixin import SliceMixin @@ -36,26 +36,13 @@ def validate_json(value): raise ValidationError("JSON not valid") -def validate_owners(value): - try: - ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id - ) - .filter_by(id=value) - .one() - ) - except NoResultFound: - raise ValidationError(f"User {value} does not exist") - - class ChartPostSchema(BaseOwnedSchema): __class_model__ = Slice slice_name = fields.String(required=True, validate=Length(1, 250)) description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) - owners = fields.List(fields.Integer(validate=validate_owners)) + owners = fields.List(fields.Integer(validate=validate_owner)) params = fields.String(allow_none=True) cache_timeout = fields.Integer() datasource_id = fields.Integer(required=True) @@ -82,7 +69,7 @@ class ChartPutSchema(BaseOwnedSchema): slice_name = fields.String(allow_none=True, validate=Length(0, 250)) description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) - owners = fields.List(fields.Integer(validate=validate_owners)) + owners = fields.List(fields.Integer(validate=validate_owner)) params = fields.String(allow_none=True) cache_timeout = fields.Integer() datasource_id = fields.Integer(allow_none=True) @@ -133,12 +120,10 @@ class ChartRestApi(SliceMixin, BaseOwnedModelRestApi): "cache_timeout", ] # Will just affect _info endpoint - edit_columns = [ - "slice_name", - ] + edit_columns = ["slice_name"] add_columns = edit_columns -# exclude_route_methods = ("info",) + # exclude_route_methods = ("info",) add_model_schema = ChartPostSchema() edit_model_schema = ChartPutSchema() diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index e43d81c25698..38050cc533ab 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -27,7 +27,7 @@ from superset.exceptions import SupersetException from superset.models.dashboard import Dashboard from superset.utils import core as utils -from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema +from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema, validate_owner from .mixin import DashboardMixin @@ -74,19 +74,6 @@ def validate_slug_uniqueness(value): raise ValidationError("Must be unique") -def validate_owners(value): - try: - ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id - ) - .filter_by(id=value) - .one() - ) - except NoResultFound: - raise ValidationError(f"User {value} does not exist") - - class BaseDashboardSchema(BaseOwnedSchema): @pre_load def pre_load(self, data): # pylint: disable=no-self-use @@ -106,7 +93,7 @@ class DashboardPostSchema(BaseDashboardSchema): slug = fields.String( allow_none=True, validate=[Length(1, 255), validate_slug_uniqueness] ) - owners = fields.List(fields.Integer(validate=validate_owners)) + owners = fields.List(fields.Integer(validate=validate_owner)) position_json = fields.String(validate=validate_json) css = fields.String() json_metadata = fields.String(validate=validate_json_metadata) @@ -116,7 +103,7 @@ class DashboardPostSchema(BaseDashboardSchema): class DashboardPutSchema(BaseDashboardSchema): dashboard_title = fields.String(allow_none=True, validate=Length(0, 500)) slug = fields.String(allow_none=True, validate=Length(0, 255)) - owners = fields.List(fields.Integer(validate=validate_owners)) + owners = fields.List(fields.Integer(validate=validate_owner)) position_json = fields.String(validate=validate_json) css = fields.String() json_metadata = fields.String(validate=validate_json_metadata) diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index dda8942957ef..afde635845e4 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -367,22 +367,23 @@ def test_get_related_owners(self): rv = self.client.get(uri) self.assertEqual(rv.status_code, 200) response = json.loads(rv.data.decode("utf-8")) - expected_response = { - "count": 6, - "result": [ - {"text": "admin user", "value": 1}, - {"text": "alpha user", "value": 5}, - {"text": "explore_beta user", "value": 8}, - {"text": "gamma user", "value": 2}, - {"text": "gamma2 user", "value": 3}, - {"text": "gamma_sqllab user", "value": 4}, - ], - } - self.assertEqual(response["count"], expected_response["count"]) - # This is needed to be implemented like this because ordering varies between + expected_users = [ + "admin user", + "alpha user", + "explore_beta user", + "gamma user", + "gamma2 user", + "gamma_sqllab user", + ] + self.assertEqual(response["count"], 6) + # This needs to be implemented like this, because ordering varies between # postgres and mysql - for result in expected_response["result"]: - self.assertIn(result, response["result"]) + response_users = [ + result["text"] + for result in response["result"] + ] + for expected_user in expected_users: + self.assertIn(expected_user, response_users) def test_get_filter_related_owners(self): """ From beecb25c2612d16326f29b5e5331b253834b7095 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 7 Jan 2020 10:57:52 +0000 Subject: [PATCH 12/23] [charts] Tests for update --- superset/views/chart/api.py | 11 ++- superset/views/dashboard/api.py | 1 - tests/chart_api_tests.py | 120 ++++++++++++++++++++++++++++++-- tests/dashboard_api_tests.py | 5 +- 4 files changed, 123 insertions(+), 14 deletions(-) diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 794f195dd975..195a8bee5d22 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -14,7 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from flask import current_app +from flask import current_app, g from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, validates_schema, ValidationError from marshmallow.validate import Length @@ -43,7 +43,7 @@ class ChartPostSchema(BaseOwnedSchema): description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) owners = fields.List(fields.Integer(validate=validate_owner)) - params = fields.String(allow_none=True) + params = fields.String(allow_none=True, validate=validate_json) cache_timeout = fields.Integer() datasource_id = fields.Integer(required=True) datasource_type = fields.String(required=True) @@ -77,8 +77,13 @@ class ChartPutSchema(BaseOwnedSchema): @post_load def make_object(self, data): # pylint: disable=no-self-use + if "owners" not in data and g.user not in self.instance.owners: + self.instance.owners.append(g.user) for field in data: - setattr(self.instance, field, data.get(field)) + if field == "owners": + self.set_owners(self.instance, data["owners"]) + else: + setattr(self.instance, field, data.get(field)) return self.instance diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index 38050cc533ab..eea34dd8b9d5 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -21,7 +21,6 @@ from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, pre_load, Schema, ValidationError from marshmallow.validate import Length -from sqlalchemy.orm.exc import NoResultFound from superset import appbuilder from superset.exceptions import SupersetException diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py index 648518241302..0e2ca320c24f 100644 --- a/tests/chart_api_tests.py +++ b/tests/chart_api_tests.py @@ -90,7 +90,7 @@ def test_delete_not_found_chart(self): def test_delete_chart_admin_not_owned(self): """ - Dashboard API: Test admin delete not owned + Chart API: Test admin delete not owned """ gamma_id = self.get_user("gamma").id chart_id = self.insert_chart("title", [gamma_id], 1).id @@ -148,18 +148,126 @@ def test_create_chart(self): def test_create_simple_chart(self): """ - Dashboard API: Test create simple chart + Chart API: Test create simple chart """ - dashboard_data = { - "dashboard_title": "title1", + chart_data = { + "slice_name": "title1", "datasource_id": 1, "datasource_type": "table", } self.login(username="admin") - uri = f"api/v1/dashboard/" - rv = self.client.post(uri, json=dashboard_data) + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) self.assertEqual(rv.status_code, 201) data = json.loads(rv.data.decode("utf-8")) model = db.session.query(Slice).get(data.get("id")) db.session.delete(model) db.session.commit() + + def test_create_chart_validate_owners(self): + """ + Chart API: Test create validate owners + """ + chart_data = { + "slice_name": "title1", + "datasource_id": 1, + "datasource_type": "table", + "owners": [1000], + } + self.login(username="admin") + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + expected_response = {"message": {"owners": {"0": ["User 1000 does not exist"]}}} + self.assertEqual(response, expected_response) + + def test_create_chart_validate_params(self): + """ + Chart API: Test create validate json + """ + chart_data = { + "slice_name": "title1", + "datasource_id": 1, + "datasource_type": "table", + "params": '{"A:"a"}', + } + self.login(username="admin") + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + + def test_update_chart(self): + """ + Chart API: Test update + """ + admin = self.get_user("admin") + gamma = self.get_user("gamma") + + chart_id = self.insert_chart("title", [admin.id], 1).id + chart_data = { + "slice_name": "title1_changed", + "description": "description1", + "owners": [gamma.id], + "viz_type": "viz_type1", + "params": "{'a': 1}", + "cache_timeout": 1000, + "datasource_id": 1, + "datasource_type": "table", + } + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.client.put(uri, json=chart_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + self.assertEqual(model.slice_name, "title1_changed") + self.assertEqual(model.description, "description1") + self.assertIn(admin, model.owners) + self.assertIn(gamma, model.owners) + self.assertEqual(model.viz_type, "viz_type1") + self.assertEqual(model.params, "{'a': 1}") + self.assertEqual(model.cache_timeout, 1000) + self.assertEqual(model.datasource_id, 1) + self.assertEqual(model.datasource_type, "table") + self.assertEqual(model.datasource_name, "birth_names") + db.session.delete(model) + db.session.commit() + + def test_update_chart_new_owner(self): + """ + Chart API: Test update set new owner to current user + """ + gamma = self.get_user("gamma") + admin = self.get_user("admin") + chart_id = self.insert_chart("title", [gamma.id], 1).id + chart_data = {"slice_name": "title1_changed"} + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.client.put(uri, json=chart_data) + self.assertEqual(rv.status_code, 200) + model = db.session.query(Slice).get(chart_id) + self.assertIn(admin, model.owners) + db.session.delete(model) + db.session.commit() + + def test_update_chart_not_owned(self): + """ + Chart API: Test update not owned + """ + user_alpha1 = self.create_user( + "alpha1", "password", "Alpha", email="alpha1@superset.org" + ) + user_alpha2 = self.create_user( + "alpha2", "password", "Alpha", email="alpha2@superset.org" + ) + chart = self.insert_chart("title", [user_alpha1.id], 1) + + self.login(username="alpha2", password="password") + chart_data = {"slice_name": "title1_changed"} + uri = f"api/v1/chart/{chart.id}" + rv = self.client.put(uri, json=chart_data) + self.assertEqual(rv.status_code, 403) + db.session.delete(chart) + db.session.delete(user_alpha1) + db.session.delete(user_alpha2) + db.session.commit() diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index afde635845e4..e442dbb1985d 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -378,10 +378,7 @@ def test_get_related_owners(self): self.assertEqual(response["count"], 6) # This needs to be implemented like this, because ordering varies between # postgres and mysql - response_users = [ - result["text"] - for result in response["result"] - ] + response_users = [result["text"] for result in response["result"]] for expected_user in expected_users: self.assertIn(expected_user, response_users) From d8f0c31b19165d3ebe81186361fec1d0348b1d58 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 7 Jan 2020 16:54:09 +0000 Subject: [PATCH 13/23] [charts] More tests --- superset/views/base.py | 5 +- superset/views/chart/api.py | 36 +++++++++- superset/views/dashboard/api.py | 2 +- tests/base_api_tests.py | 78 ++++++++++++++++++++ tests/chart_api_tests.py | 123 ++++++++++++++++++++++++++++++-- tests/dashboard_api_tests.py | 64 ++--------------- 6 files changed, 237 insertions(+), 71 deletions(-) create mode 100644 tests/base_api_tests.py diff --git a/superset/views/base.py b/superset/views/base.py index a0242f33821e..8e05e24a24d0 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -429,13 +429,14 @@ class BaseOwnedSchema(BaseSupersetSchema): __class_model__: Model = None @post_load - def make_object(self, data: Dict): + def make_object(self, data: Dict, discard: List = None): + discard = discard or [] instance = self.__class_model__() # pylint: disable=not-callable self.set_owners(instance, data["owners"]) for field in data: if field == "owners": self.set_owners(instance, data["owners"]) - else: + elif field not in discard: setattr(instance, field, data.get(field)) return instance diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 195a8bee5d22..f9bfeb6c0feb 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -14,6 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from typing import Dict, List + from flask import current_app, g from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, validates_schema, ValidationError @@ -23,6 +25,7 @@ from superset import appbuilder from superset.connectors.connector_registry import ConnectorRegistry from superset.exceptions import SupersetException +from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.utils import core as utils from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema, validate_owner @@ -36,6 +39,27 @@ def validate_json(value): raise ValidationError("JSON not valid") +def validate_dashboard(value): + try: + (current_app.appbuilder.get_session.query(Dashboard).filter_by(id=value).one()) + except NoResultFound: + raise ValidationError(f"Dashboard {value} does not exist") + + +def populate_dashboards(instance: Slice, dashboards: List[int]): + """ + Mutates a Slice with the dashboards SQLA Models + """ + dashboards_tmp = [] + for dashboard_id in dashboards: + dashboards_tmp.append( + current_app.appbuilder.get_session.query(Dashboard) + .filter_by(id=dashboard_id) + .one() + ) + instance.dashboards = dashboards_tmp + + class ChartPostSchema(BaseOwnedSchema): __class_model__ = Slice @@ -48,6 +72,7 @@ class ChartPostSchema(BaseOwnedSchema): datasource_id = fields.Integer(required=True) datasource_type = fields.String(required=True) datasource_name = fields.String(allow_none=True) + dashboards = fields.List(fields.Integer(validate=validate_dashboard)) @staticmethod @validates_schema @@ -64,6 +89,12 @@ def validate_datasource(data): ) data["datasource_name"] = datasource.name + @post_load + def make_object(self, data: Dict, discard=None): + instance = super().make_object(data, discard=["dashboards"]) + populate_dashboards(instance, data.get("dashboards", [])) + return instance + class ChartPutSchema(BaseOwnedSchema): slice_name = fields.String(allow_none=True, validate=Length(0, 250)) @@ -74,14 +105,17 @@ class ChartPutSchema(BaseOwnedSchema): cache_timeout = fields.Integer() datasource_id = fields.Integer(allow_none=True) datasource_type = fields.String(allow_none=True) + dashboards = fields.List(fields.Integer(validate=validate_dashboard)) @post_load - def make_object(self, data): # pylint: disable=no-self-use + def make_object(self, data, discard=None): # pylint: disable=no-self-use if "owners" not in data and g.user not in self.instance.owners: self.instance.owners.append(g.user) for field in data: if field == "owners": self.set_owners(self.instance, data["owners"]) + elif field == "dashboards": + populate_dashboards(self.instance, data.get("dashboards", [])) else: setattr(self.instance, field, data.get(field)) return self.instance diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index eea34dd8b9d5..d0c990a56168 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -109,7 +109,7 @@ class DashboardPutSchema(BaseDashboardSchema): published = fields.Boolean() @post_load - def make_object(self, data): # pylint: disable=no-self-use + def make_object(self, data, discard=None): # pylint: disable=no-self-use if "owners" not in data and g.user not in self.instance.owners: self.instance.owners.append(g.user) for field in data: diff --git a/tests/base_api_tests.py b/tests/base_api_tests.py new file mode 100644 index 000000000000..c9e53d4d7673 --- /dev/null +++ b/tests/base_api_tests.py @@ -0,0 +1,78 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# isort:skip_file +import json + +import prison + +from superset import db, security_manager + + +class ApiOwnersTestCaseMixin: + """ + Implements shared tests for owners related field + """ + + resource_name: str = "" + + def test_get_related_owners(self): + """ + API: Test get related owners + """ + self.login(username="admin") + uri = f"api/v1/{self.resource_name}/related/owners" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + response = json.loads(rv.data.decode("utf-8")) + users = db.session.query(security_manager.user_model).all() + expected_users = [str(user) for user in users] + self.assertEqual(response["count"], len(users)) + # This needs to be implemented like this, because ordering varies between + # postgres and mysql + response_users = [result["text"] for result in response["result"]] + for expected_user in expected_users: + self.assertIn(expected_user, response_users) + + def test_get_filter_related_owners(self): + """ + API: Test get filter related owners + """ + self.login(username="admin") + argument = {"filter": "a"} + uri = f"api/v1/{self.resource_name}/related/owners?q={prison.dumps(argument)}" + + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + response = json.loads(rv.data.decode("utf-8")) + expected_response = { + "count": 2, + "result": [ + {"text": "admin user", "value": 1}, + {"text": "alpha user", "value": 5}, + ], + } + self.assertEqual(response, expected_response) + + def test_get_related_fail(self): + """ + API: Test get related fail + """ + self.login(username="admin") + uri = f"api/v1/{self.resource_name}/related/owner" + + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py index 0e2ca320c24f..6b76fbc89601 100644 --- a/tests/chart_api_tests.py +++ b/tests/chart_api_tests.py @@ -16,19 +16,22 @@ # under the License. """Unit tests for Superset""" import json -from typing import List +from typing import List, Optional import prison -from flask_appbuilder.security.sqla import models as ab_models from superset import db, security_manager from superset.connectors.connector_registry import ConnectorRegistry +from superset.models.dashboard import Dashboard from superset.models.slice import Slice from .base_tests import SupersetTestCase +from .base_api_tests import ApiOwnersTestCaseMixin -class ChartApiTests(SupersetTestCase): +class ChartApiTests(SupersetTestCase, ApiOwnersTestCaseMixin): + resource_name = "chart" + def __init__(self, *args, **kwargs): super(ChartApiTests, self).__init__(*args, **kwargs) @@ -38,10 +41,10 @@ def insert_chart( owners: List[int], datasource_id: int, datasource_type: str = "table", - description: str = "", - viz_type: str = "", - params: str = "", - cache_timeout: int = 30, + description: str = None, + viz_type: str = None, + params: str = None, + cache_timeout: Optional[int] = None, ) -> Slice: obj_owners = list() for owner in owners: @@ -136,6 +139,7 @@ def test_create_chart(self): "cache_timeout": 1000, "datasource_id": 1, "datasource_type": "table", + "dashboards": [1, 2], } self.login(username="admin") uri = f"api/v1/chart/" @@ -214,12 +218,14 @@ def test_update_chart(self): "cache_timeout": 1000, "datasource_id": 1, "datasource_type": "table", + "dashboards": [1], } self.login(username="admin") uri = f"api/v1/chart/{chart_id}" rv = self.client.put(uri, json=chart_data) self.assertEqual(rv.status_code, 200) model = db.session.query(Slice).get(chart_id) + related_dashboard = db.session.query(Dashboard).get(1) self.assertEqual(model.slice_name, "title1_changed") self.assertEqual(model.description, "description1") self.assertIn(admin, model.owners) @@ -230,6 +236,7 @@ def test_update_chart(self): self.assertEqual(model.datasource_id, 1) self.assertEqual(model.datasource_type, "table") self.assertEqual(model.datasource_name, "birth_names") + self.assertIn(related_dashboard, model.dashboards) db.session.delete(model) db.session.commit() @@ -271,3 +278,105 @@ def test_update_chart_not_owned(self): db.session.delete(user_alpha1) db.session.delete(user_alpha2) db.session.commit() + + def test_get_chart(self): + """ + Chart API: Test get chart + """ + admin = self.get_user("admin") + chart = self.insert_chart("title", [admin.id], 1) + self.login(username="admin") + uri = f"api/v1/chart/{chart.id}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + expected_result = { + "cache_timeout": None, + "dashboards": [], + "description": None, + "owners": [{"id": 1, "username": "admin"}], + "params": None, + "slice_name": "title", + "viz_type": None, + } + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(data["result"], expected_result) + db.session.delete(chart) + db.session.commit() + + def test_get_chart_not_found(self): + """ + Chart API: Test get chart not found + """ + chart_id = 1000 + self.login(username="admin") + uri = f"api/v1/chart/{chart_id}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) + + def test_get_chart_no_data_access(self): + """ + Chart API: Test get chart without data access + """ + self.login(username="gamma") + chart_no_access = ( + db.session.query(Slice) + .filter_by(slice_name="Girl Name Cloud") + .one_or_none() + ) + uri = f"api/v1/chart/{chart_no_access.id}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 404) + + def test_get_charts(self): + """ + Chart API: Test get charts + """ + self.login(username="admin") + uri = f"api/v1/chart/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(data["count"], 33) + + def test_get_charts_filter(self): + """ + Chart API: Test get charts filter + """ + self.login(username="admin") + arguments = {"filters": [{"col": "slice_name", "opr": "sw", "value": "G"}]} + uri = f"api/v1/chart/?q={prison.dumps(arguments)}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(data["count"], 5) + + def test_get_charts_page(self): + """ + Chart API: Test get charts filter + """ + # Assuming we have 33 sample charts + self.login(username="admin") + arguments = {"page_size": 10, "page": 0} + uri = f"api/v1/chart/?q={prison.dumps(arguments)}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(len(data["result"]), 10) + + arguments = {"page_size": 10, "page": 3} + uri = f"api/v1/chart/?q={prison.dumps(arguments)}" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(len(data["result"]), 3) + + def test_get_charts_no_data_access(self): + """ + Chart API: Test get charts no data access + """ + self.login(username="gamma") + uri = f"api/v1/chart/" + rv = self.client.get(uri) + self.assertEqual(rv.status_code, 200) + data = json.loads(rv.data.decode("utf-8")) + self.assertEqual(data["count"], 0) diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index e442dbb1985d..044a292452f3 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -18,17 +18,17 @@ import json from typing import List -import prison -from flask_appbuilder.security.sqla import models as ab_models - from superset import db, security_manager from superset.models import core as models from superset.models.slice import Slice from .base_tests import SupersetTestCase +from .base_api_tests import ApiOwnersTestCaseMixin + +class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin): + resource_name = "dashboard" -class DashboardApiTests(SupersetTestCase): def __init__(self, *args, **kwargs): super(DashboardApiTests, self).__init__(*args, **kwargs) @@ -357,59 +357,3 @@ def test_update_dashboard_not_owned(self): db.session.delete(user_alpha1) db.session.delete(user_alpha2) db.session.commit() - - def test_get_related_owners(self): - """ - Dashboard API: Test dashboard get related owners - """ - self.login(username="admin") - uri = f"api/v1/dashboard/related/owners" - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 200) - response = json.loads(rv.data.decode("utf-8")) - expected_users = [ - "admin user", - "alpha user", - "explore_beta user", - "gamma user", - "gamma2 user", - "gamma_sqllab user", - ] - self.assertEqual(response["count"], 6) - # This needs to be implemented like this, because ordering varies between - # postgres and mysql - response_users = [result["text"] for result in response["result"]] - for expected_user in expected_users: - self.assertIn(expected_user, response_users) - - def test_get_filter_related_owners(self): - """ - Dashboard API: Test dashboard get filter related owners - """ - self.login(username="admin") - argument = {"filter": "a"} - uri = "api/v1/dashboard/related/owners?{}={}".format( - "q", prison.dumps(argument) - ) - - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 200) - response = json.loads(rv.data.decode("utf-8")) - expected_response = { - "count": 2, - "result": [ - {"text": "admin user", "value": 1}, - {"text": "alpha user", "value": 5}, - ], - } - self.assertEqual(response, expected_response) - - def test_get_related_fail(self): - """ - Dashboard API: Test dashboard get related fail - """ - self.login(username="admin") - uri = "api/v1/dashboard/related/owner" - - rv = self.client.get(uri) - self.assertEqual(rv.status_code, 404) From fb6bb5e9e9e961009b6f9eed89998716a49f86d7 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 7 Jan 2020 17:05:26 +0000 Subject: [PATCH 14/23] [charts] Fix, isort --- tests/chart_api_tests.py | 2 +- tests/dashboard_api_tests.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py index 6b76fbc89601..283ce49675c0 100644 --- a/tests/chart_api_tests.py +++ b/tests/chart_api_tests.py @@ -25,8 +25,8 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice -from .base_tests import SupersetTestCase from .base_api_tests import ApiOwnersTestCaseMixin +from .base_tests import SupersetTestCase class ChartApiTests(SupersetTestCase, ApiOwnersTestCaseMixin): diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index 044a292452f3..47f8fe469092 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -22,8 +22,8 @@ from superset.models import core as models from superset.models.slice import Slice -from .base_tests import SupersetTestCase from .base_api_tests import ApiOwnersTestCaseMixin +from .base_tests import SupersetTestCase class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin): From 1342b588e7d988bcced9f8b898f634baf9cb4ce6 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 8 Jan 2020 10:53:04 +0000 Subject: [PATCH 15/23] [charts] DRY and improve quality --- superset/views/base.py | 62 +++++++++++++++++++++++-------------- superset/views/chart/api.py | 49 ++++++++++++++--------------- 2 files changed, 63 insertions(+), 48 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 8e05e24a24d0..608797c91774 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -390,6 +390,19 @@ def apply(self, query, value): ) +def validate_owner(value): + try: + ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one() + ) + except NoResultFound: + raise ValidationError(f"User {value} does not exist") + + class BaseSupersetSchema(Schema): """ Extends Marshmallow schema so that we can pass a Model to load @@ -397,8 +410,10 @@ class BaseSupersetSchema(Schema): to perform partial model merges on HTTP PUT """ + __class_model__: Model = None + def __init__(self, **kwargs): - self.instance = None + self.instance: Optional[Model] = None super().__init__(**kwargs) def load( @@ -407,18 +422,22 @@ def load( self.instance = instance return super().load(data, many=many, partial=partial, **kwargs) + @post_load + def make_object(self, data: Dict, discard: List = None) -> Model: + """ + Creates a Model object from POST or PUT requests. PUT will use self.instance + previously fetched from the endpoint handler -def validate_owner(value): - try: - ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id - ) - .filter_by(id=value) - .one() - ) - except NoResultFound: - raise ValidationError(f"User {value} does not exist") + :param data: Schema data payload + :param discard: List of fields to not set on the model + """ + discard = discard or [] + if not self.instance: + self.instance = self.__class_model__() # pylint: disable=not-callable + for field in data: + if field not in discard: + setattr(self.instance, field, data.get(field)) + return self.instance class BaseOwnedSchema(BaseSupersetSchema): @@ -426,23 +445,20 @@ class BaseOwnedSchema(BaseSupersetSchema): Implements owners validation and pre load on Marshmallow schemas """ - __class_model__: Model = None + owners_field_name = "owners" @post_load - def make_object(self, data: Dict, discard: List = None): + def make_object(self, data: Dict, discard: List = None) -> Model: discard = discard or [] - instance = self.__class_model__() # pylint: disable=not-callable - self.set_owners(instance, data["owners"]) - for field in data: - if field == "owners": - self.set_owners(instance, data["owners"]) - elif field not in discard: - setattr(instance, field, data.get(field)) + discard.append(self.owners_field_name) + instance = super().make_object(data, discard) + if self.owners_field_name in data: + self.set_owners(instance, data[self.owners_field_name]) return instance @pre_load - def pre_load(self, data: Dict): # pylint: disable=no-self-use - data["owners"] = data.get("owners", []) + def pre_load(self, data: Dict): + data[self.owners_field_name] = data.get(self.owners_field_name, []) @staticmethod def set_owners(instance: Model, owners: List[int]): diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index f9bfeb6c0feb..23b1d043840a 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -60,23 +60,10 @@ def populate_dashboards(instance: Slice, dashboards: List[int]): instance.dashboards = dashboards_tmp -class ChartPostSchema(BaseOwnedSchema): - __class_model__ = Slice - - slice_name = fields.String(required=True, validate=Length(1, 250)) - description = fields.String(allow_none=True) - viz_type = fields.String(allow_none=True, validate=Length(0, 250)) - owners = fields.List(fields.Integer(validate=validate_owner)) - params = fields.String(allow_none=True, validate=validate_json) - cache_timeout = fields.Integer() - datasource_id = fields.Integer(required=True) - datasource_type = fields.String(required=True) - datasource_name = fields.String(allow_none=True) - dashboards = fields.List(fields.Integer(validate=validate_dashboard)) - +class BaseChartSchema(BaseOwnedSchema): @staticmethod @validates_schema - def validate_datasource(data): + def validate_datasource(data: Dict): datasource_type = data["datasource_type"] datasource_id = data["datasource_id"] try: @@ -89,14 +76,31 @@ def validate_datasource(data): ) data["datasource_name"] = datasource.name + +class ChartPostSchema(BaseChartSchema): + __class_model__ = Slice + + slice_name = fields.String(required=True, validate=Length(1, 250)) + description = fields.String(allow_none=True) + viz_type = fields.String(allow_none=True, validate=Length(0, 250)) + owners = fields.List(fields.Integer(validate=validate_owner)) + params = fields.String(allow_none=True, validate=validate_json) + cache_timeout = fields.Integer() + datasource_id = fields.Integer(required=True) + datasource_type = fields.String(required=True) + datasource_name = fields.String(allow_none=True) + dashboards = fields.List(fields.Integer(validate=validate_dashboard)) + @post_load - def make_object(self, data: Dict, discard=None): + def make_object(self, data: Dict, discard=None) -> Slice: instance = super().make_object(data, discard=["dashboards"]) populate_dashboards(instance, data.get("dashboards", [])) return instance -class ChartPutSchema(BaseOwnedSchema): +class ChartPutSchema(BaseChartSchema): + instance: Slice + slice_name = fields.String(allow_none=True, validate=Length(0, 250)) description = fields.String(allow_none=True) viz_type = fields.String(allow_none=True, validate=Length(0, 250)) @@ -108,16 +112,11 @@ class ChartPutSchema(BaseOwnedSchema): dashboards = fields.List(fields.Integer(validate=validate_dashboard)) @post_load - def make_object(self, data, discard=None): # pylint: disable=no-self-use + def make_object(self, data: Dict, discard: List = None) -> Slice: if "owners" not in data and g.user not in self.instance.owners: self.instance.owners.append(g.user) - for field in data: - if field == "owners": - self.set_owners(self.instance, data["owners"]) - elif field == "dashboards": - populate_dashboards(self.instance, data.get("dashboards", [])) - else: - setattr(self.instance, field, data.get(field)) + self.instance = super().make_object(data, ["dashboards"]) + populate_dashboards(self.instance, data.get("dashboards", [])) return self.instance From c6260515f69bc316b1c06bd969f568f3ecee8d36 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 8 Jan 2020 15:37:19 +0000 Subject: [PATCH 16/23] [charts] DRY and more tests --- superset/views/base.py | 6 ++- superset/views/chart/api.py | 53 ++++++++++++---------- superset/views/dashboard/api.py | 13 ++---- tests/chart_api_tests.py | 78 ++++++++++++++++++++++++++++++++- 4 files changed, 114 insertions(+), 36 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 608797c91774..28579b248dea 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -423,7 +423,7 @@ def load( return super().load(data, many=many, partial=partial, **kwargs) @post_load - def make_object(self, data: Dict, discard: List = None) -> Model: + def make_object(self, data: Dict, discard: List[str] = None) -> Model: """ Creates a Model object from POST or PUT requests. PUT will use self.instance previously fetched from the endpoint handler @@ -448,10 +448,12 @@ class BaseOwnedSchema(BaseSupersetSchema): owners_field_name = "owners" @post_load - def make_object(self, data: Dict, discard: List = None) -> Model: + def make_object(self, data: Dict, discard: List[str] = None) -> Model: discard = discard or [] discard.append(self.owners_field_name) instance = super().make_object(data, discard) + if "owners" not in data and g.user not in instance.owners: + instance.owners.append(g.user) if self.owners_field_name in data: self.set_owners(instance, data[self.owners_field_name]) return instance diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 23b1d043840a..45134184f6d0 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -16,7 +16,7 @@ # under the License. from typing import Dict, List -from flask import current_app, g +from flask import current_app from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, validates_schema, ValidationError from marshmallow.validate import Length @@ -46,6 +46,22 @@ def validate_dashboard(value): raise ValidationError(f"Dashboard {value} does not exist") +def validate_update_datasource(data: Dict): + if not ("datasource_type" in data and "datasource_id" in data): + return + datasource_type = data["datasource_type"] + datasource_id = data["datasource_id"] + try: + datasource = ConnectorRegistry.get_datasource( + datasource_type, datasource_id, current_app.appbuilder.get_session + ) + except (NoResultFound, KeyError): + raise ValidationError( + f"Datasource [{datasource_type}].{datasource_id} does not exist" + ) + data["datasource_name"] = datasource.name + + def populate_dashboards(instance: Slice, dashboards: List[int]): """ Mutates a Slice with the dashboards SQLA Models @@ -60,24 +76,7 @@ def populate_dashboards(instance: Slice, dashboards: List[int]): instance.dashboards = dashboards_tmp -class BaseChartSchema(BaseOwnedSchema): - @staticmethod - @validates_schema - def validate_datasource(data: Dict): - datasource_type = data["datasource_type"] - datasource_id = data["datasource_id"] - try: - datasource = ConnectorRegistry.get_datasource( - datasource_type, datasource_id, current_app.appbuilder.get_session - ) - except NoResultFound: - raise ValidationError( - f"Datasource [{datasource_type}].{datasource_id} does not exist" - ) - data["datasource_name"] = datasource.name - - -class ChartPostSchema(BaseChartSchema): +class ChartPostSchema(BaseOwnedSchema): __class_model__ = Slice slice_name = fields.String(required=True, validate=Length(1, 250)) @@ -91,14 +90,18 @@ class ChartPostSchema(BaseChartSchema): datasource_name = fields.String(allow_none=True) dashboards = fields.List(fields.Integer(validate=validate_dashboard)) + @validates_schema + def validate_schema(self, data: Dict): # pylint: disable=no-self-use + validate_update_datasource(data) + @post_load - def make_object(self, data: Dict, discard=None) -> Slice: + def make_object(self, data: Dict, discard: List[str] = None) -> Slice: instance = super().make_object(data, discard=["dashboards"]) populate_dashboards(instance, data.get("dashboards", [])) return instance -class ChartPutSchema(BaseChartSchema): +class ChartPutSchema(BaseOwnedSchema): instance: Slice slice_name = fields.String(allow_none=True, validate=Length(0, 250)) @@ -111,10 +114,12 @@ class ChartPutSchema(BaseChartSchema): datasource_type = fields.String(allow_none=True) dashboards = fields.List(fields.Integer(validate=validate_dashboard)) + @validates_schema + def validate_schema(self, data: Dict): # pylint: disable=no-self-use + validate_update_datasource(data) + @post_load - def make_object(self, data: Dict, discard: List = None) -> Slice: - if "owners" not in data and g.user not in self.instance.owners: - self.instance.owners.append(g.user) + def make_object(self, data: Dict, discard: List[str] = None) -> Slice: self.instance = super().make_object(data, ["dashboards"]) populate_dashboards(self.instance, data.get("dashboards", [])) return self.instance diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index d0c990a56168..695e599bc684 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -16,8 +16,9 @@ # under the License. import json import re +from typing import Dict, List -from flask import current_app, g +from flask import current_app from flask_appbuilder.models.sqla.interface import SQLAInterface from marshmallow import fields, post_load, pre_load, Schema, ValidationError from marshmallow.validate import Length @@ -109,14 +110,8 @@ class DashboardPutSchema(BaseDashboardSchema): published = fields.Boolean() @post_load - def make_object(self, data, discard=None): # pylint: disable=no-self-use - if "owners" not in data and g.user not in self.instance.owners: - self.instance.owners.append(g.user) - for field in data: - if field == "owners": - self.set_owners(self.instance, data["owners"]) - else: - setattr(self.instance, field, data.get(field)) + def make_object(self, data: Dict, discard: List[str] = None) -> Dashboard: + self.instance = super().make_object(data, []) for slc in self.instance.slices: slc.owners = list(set(self.instance.owners) | set(slc.owners)) return self.instance diff --git a/tests/chart_api_tests.py b/tests/chart_api_tests.py index 283ce49675c0..8a67b030c87a 100644 --- a/tests/chart_api_tests.py +++ b/tests/chart_api_tests.py @@ -188,7 +188,7 @@ def test_create_chart_validate_owners(self): def test_create_chart_validate_params(self): """ - Chart API: Test create validate json + Chart API: Test create validate params json """ chart_data = { "slice_name": "title1", @@ -201,6 +201,37 @@ def test_create_chart_validate_params(self): rv = self.client.post(uri, json=chart_data) self.assertEqual(rv.status_code, 422) + def test_create_chart_validate_datasource(self): + """ + Chart API: Test create validate datasource + """ + self.login(username="admin") + chart_data = { + "slice_name": "title1", + "datasource_id": 1, + "datasource_type": "unknown", + } + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual( + response, + {"message": {"_schema": ["Datasource [unknown].1 does not exist"]}}, + ) + chart_data = { + "slice_name": "title1", + "datasource_id": 0, + "datasource_type": "table", + } + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual( + response, {"message": {"_schema": ["Datasource [table].0 does not exist"]}} + ) + def test_update_chart(self): """ Chart API: Test update @@ -279,6 +310,51 @@ def test_update_chart_not_owned(self): db.session.delete(user_alpha2) db.session.commit() + def test_update_chart_validate_datasource(self): + """ + Chart API: Test update validate datasource + """ + admin = self.get_user("admin") + chart = self.insert_chart("title", [admin.id], 1) + self.login(username="admin") + chart_data = {"datasource_id": 1, "datasource_type": "unknown"} + uri = f"api/v1/chart/{chart.id}" + rv = self.client.put(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual( + response, + {"message": {"_schema": ["Datasource [unknown].1 does not exist"]}}, + ) + chart_data = {"datasource_id": 0, "datasource_type": "table"} + uri = f"api/v1/chart/{chart.id}" + rv = self.client.put(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + self.assertEqual( + response, {"message": {"_schema": ["Datasource [table].0 does not exist"]}} + ) + db.session.delete(chart) + db.session.commit() + + def test_update_chart_validate_owners(self): + """ + Chart API: Test update validate owners + """ + chart_data = { + "slice_name": "title1", + "datasource_id": 1, + "datasource_type": "table", + "owners": [1000], + } + self.login(username="admin") + uri = f"api/v1/chart/" + rv = self.client.post(uri, json=chart_data) + self.assertEqual(rv.status_code, 422) + response = json.loads(rv.data.decode("utf-8")) + expected_response = {"message": {"owners": {"0": ["User 1000 does not exist"]}}} + self.assertEqual(response, expected_response) + def test_get_chart(self): """ Chart API: Test get chart From 3c262f4d8d8d5c8f2a1396af062ab53a101d208a Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 8 Jan 2020 15:59:11 +0000 Subject: [PATCH 17/23] [charts] Refactor base for api and schemas --- superset/views/base.py | 388 +------------------------------- superset/views/base_api.py | 303 +++++++++++++++++++++++++ superset/views/base_schemas.py | 108 +++++++++ superset/views/chart/api.py | 3 +- superset/views/dashboard/api.py | 3 +- 5 files changed, 417 insertions(+), 388 deletions(-) create mode 100644 superset/views/base_api.py create mode 100644 superset/views/base_schemas.py diff --git a/superset/views/base.py b/superset/views/base.py index 28579b248dea..a6560841045f 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -18,13 +18,12 @@ import logging import traceback from datetime import datetime -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, Optional import simplejson as json import yaml from flask import ( abort, - current_app, flash, g, get_flashed_messages, @@ -33,19 +32,14 @@ Response, session, ) -from flask_appbuilder import BaseView, Model, ModelRestApi, ModelView +from flask_appbuilder import BaseView, ModelView from flask_appbuilder.actions import action -from flask_appbuilder.api import expose, protect, rison, safe from flask_appbuilder.forms import DynamicForm -from flask_appbuilder.models.filters import Filters from flask_appbuilder.models.sqla.filters import BaseFilter from flask_appbuilder.widgets import ListWidget from flask_babel import get_locale, gettext as __, lazy_gettext as _ from flask_wtf.form import FlaskForm -from marshmallow import post_load, pre_load, Schema, ValidationError from sqlalchemy import or_ -from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from werkzeug.exceptions import HTTPException from wtforms.fields.core import Field, UnboundField @@ -167,26 +161,6 @@ def wraps(self, *args, **kwargs): return functools.update_wrapper(wraps, f) -def check_ownership_and_item_exists(f): - """ - A Decorator that checks if an object exists and is owned by the current user - """ - - def wraps(self, pk): # pylint: disable=invalid-name - item = self.datamodel.get( - pk, self._base_filters # pylint: disable=protected-access - ) - if not item: - return self.response_404() - try: - check_ownership(item) - except SupersetSecurityException as e: - return self.response(403, message=str(e)) - return f(self, item) - - return functools.update_wrapper(wraps, f) - - def get_datasource_exist_error_msg(full_name): return __("Datasource %(name)s already exists", name=full_name) @@ -390,364 +364,6 @@ def apply(self, query, value): ) -def validate_owner(value): - try: - ( - current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model.id - ) - .filter_by(id=value) - .one() - ) - except NoResultFound: - raise ValidationError(f"User {value} does not exist") - - -class BaseSupersetSchema(Schema): - """ - Extends Marshmallow schema so that we can pass a Model to load - (following marshamallow-sqlalchemy pattern). This is useful - to perform partial model merges on HTTP PUT - """ - - __class_model__: Model = None - - def __init__(self, **kwargs): - self.instance: Optional[Model] = None - super().__init__(**kwargs) - - def load( - self, data, many=None, partial=None, instance: Model = None, **kwargs - ): # pylint: disable=arguments-differ - self.instance = instance - return super().load(data, many=many, partial=partial, **kwargs) - - @post_load - def make_object(self, data: Dict, discard: List[str] = None) -> Model: - """ - Creates a Model object from POST or PUT requests. PUT will use self.instance - previously fetched from the endpoint handler - - :param data: Schema data payload - :param discard: List of fields to not set on the model - """ - discard = discard or [] - if not self.instance: - self.instance = self.__class_model__() # pylint: disable=not-callable - for field in data: - if field not in discard: - setattr(self.instance, field, data.get(field)) - return self.instance - - -class BaseOwnedSchema(BaseSupersetSchema): - """ - Implements owners validation and pre load on Marshmallow schemas - """ - - owners_field_name = "owners" - - @post_load - def make_object(self, data: Dict, discard: List[str] = None) -> Model: - discard = discard or [] - discard.append(self.owners_field_name) - instance = super().make_object(data, discard) - if "owners" not in data and g.user not in instance.owners: - instance.owners.append(g.user) - if self.owners_field_name in data: - self.set_owners(instance, data[self.owners_field_name]) - return instance - - @pre_load - def pre_load(self, data: Dict): - data[self.owners_field_name] = data.get(self.owners_field_name, []) - - @staticmethod - def set_owners(instance: Model, owners: List[int]): - owner_objs = list() - if g.user.id not in owners: - owners.append(g.user.id) - for owner_id in owners: - user = current_app.appbuilder.get_session.query( - current_app.appbuilder.sm.user_model - ).get(owner_id) - owner_objs.append(user) - instance.owners = owner_objs - - -get_related_schema = { - "type": "object", - "properties": { - "page_size": {"type": "integer"}, - "page": {"type": "integer"}, - "filter": {"type": "string"}, - }, -} - - -class BaseSupersetModelRestApi(ModelRestApi): - """ - Extends FAB's ModelResApi to implement specific superset generic functionality - """ - - order_rel_fields: Dict[str, Tuple[str, str]] = {} - """ - Impose ordering on related fields query:: - - order_rel_fields = { - "": ("", ""), - ... - } - """ # pylint: disable=pointless-string-statement - filter_rel_fields_field: Dict[str, str] = {} - """ - Declare the related field field for filtering:: - - filter_rel_fields_field = { - "": "", "") - } - """ # pylint: disable=pointless-string-statement - - def _get_related_filter(self, datamodel, column_name: str, value: str) -> Filters: - filter_field = self.filter_rel_fields_field.get(column_name) - filters = datamodel.get_filters([filter_field]) - if value: - filters.rest_add_filters( - [{"opr": "sw", "col": filter_field, "value": value}] - ) - return filters - - @expose("/related/", methods=["GET"]) - @protect() - @safe - @rison(get_related_schema) - def related(self, column_name: str, **kwargs): - """Get related fields data - --- - get: - parameters: - - in: path - schema: - type: string - name: column_name - - in: query - name: q - content: - application/json: - schema: - type: object - properties: - page_size: - type: integer - page: - type: integer - filter: - type: string - responses: - 200: - description: Related column data - content: - application/json: - schema: - type: object - properties: - count: - type: integer - result: - type: object - properties: - value: - type: integer - text: - type: string - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - args = kwargs.get("rison", {}) - # handle pagination - page, page_size = self._handle_page_args(args) - try: - datamodel = self.datamodel.get_related_interface(column_name) - except KeyError: - return self.response_404() - page, page_size = self._sanitize_page_args(page, page_size) - # handle ordering - order_field = self.order_rel_fields.get(column_name) - if order_field: - order_column, order_direction = order_field - else: - order_column, order_direction = "", "" - # handle filters - filters = self._get_related_filter(datamodel, column_name, args.get("filter")) - # Make the query - count, values = datamodel.query( - filters, order_column, order_direction, page=page, page_size=page_size - ) - # produce response - result = [ - {"value": datamodel.get_pk_value(value), "text": str(value)} - for value in values - ] - return self.response(200, count=count, result=result) - - -class BaseOwnedModelRestApi(BaseSupersetModelRestApi): - @expose("/", methods=["PUT"]) - @protect() - @check_ownership_and_item_exists - @safe - def put(self, item): # pylint: disable=arguments-differ - """Changes a owned Model - --- - put: - parameters: - - in: path - schema: - type: integer - name: pk - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - responses: - 200: - description: Item changed - content: - application/json: - schema: - type: object - properties: - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.put' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 403: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - self.response_400(message="Request is not JSON") - item = self.edit_model_schema.load(request.json, instance=item) - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.edit(item.data, raise_exception=True) - return self.response( - 200, result=self.edit_model_schema.dump(item.data, many=False).data - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - @expose("/", methods=["POST"]) - @protect() - @safe - def post(self): - """Creates a new owned Model - --- - post: - requestBody: - description: Model schema - required: true - content: - application/json: - schema: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - responses: - 201: - description: Model added - content: - application/json: - schema: - type: object - properties: - id: - type: string - result: - $ref: '#/components/schemas/{{self.__class__.__name__}}.post' - 400: - $ref: '#/components/responses/400' - 401: - $ref: '#/components/responses/401' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - if not request.is_json: - return self.response_400(message="Request is not JSON") - item = self.add_model_schema.load(request.json) - # This validates custom Schema with custom validations - if item.errors: - return self.response_422(message=item.errors) - try: - self.datamodel.add(item.data, raise_exception=True) - return self.response( - 201, - result=self.add_model_schema.dump(item.data, many=False).data, - id=item.data.id, - ) - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - @expose("/", methods=["DELETE"]) - @protect() - @check_ownership_and_item_exists - @safe - def delete(self, item): # pylint: disable=arguments-differ - """Deletes owned Model - --- - delete: - parameters: - - in: path - schema: - type: integer - name: pk - responses: - 200: - description: Model delete - content: - application/json: - schema: - type: object - properties: - message: - type: string - 401: - $ref: '#/components/responses/401' - 403: - $ref: '#/components/responses/401' - 404: - $ref: '#/components/responses/404' - 422: - $ref: '#/components/responses/422' - 500: - $ref: '#/components/responses/500' - """ - try: - self.datamodel.delete(item, raise_exception=True) - return self.response(200, message="OK") - except SQLAlchemyError as e: - return self.response_422(message=str(e)) - - class CsvResponse(Response): # pylint: disable=too-many-ancestors """ Override Response to take into account csv encoding from config.py diff --git a/superset/views/base_api.py b/superset/views/base_api.py new file mode 100644 index 000000000000..2536e90407ac --- /dev/null +++ b/superset/views/base_api.py @@ -0,0 +1,303 @@ +import functools +from typing import Dict, Tuple + +from flask import request +from flask_appbuilder import ModelRestApi +from flask_appbuilder.api import expose, protect, rison, safe +from flask_appbuilder.models.filters import Filters +from sqlalchemy.exc import SQLAlchemyError + +from superset.exceptions import SupersetSecurityException +from superset.views.base import check_ownership + +get_related_schema = { + "type": "object", + "properties": { + "page_size": {"type": "integer"}, + "page": {"type": "integer"}, + "filter": {"type": "string"}, + }, +} + + +def check_ownership_and_item_exists(f): + """ + A Decorator that checks if an object exists and is owned by the current user + """ + + def wraps(self, pk): # pylint: disable=invalid-name + item = self.datamodel.get( + pk, self._base_filters # pylint: disable=protected-access + ) + if not item: + return self.response_404() + try: + check_ownership(item) + except SupersetSecurityException as e: + return self.response(403, message=str(e)) + return f(self, item) + + return functools.update_wrapper(wraps, f) + + +class BaseSupersetModelRestApi(ModelRestApi): + """ + Extends FAB's ModelResApi to implement specific superset generic functionality + """ + + order_rel_fields: Dict[str, Tuple[str, str]] = {} + """ + Impose ordering on related fields query:: + + order_rel_fields = { + "": ("", ""), + ... + } + """ # pylint: disable=pointless-string-statement + filter_rel_fields_field: Dict[str, str] = {} + """ + Declare the related field field for filtering:: + + filter_rel_fields_field = { + "": "", "") + } + """ # pylint: disable=pointless-string-statement + + def _get_related_filter(self, datamodel, column_name: str, value: str) -> Filters: + filter_field = self.filter_rel_fields_field.get(column_name) + filters = datamodel.get_filters([filter_field]) + if value: + filters.rest_add_filters( + [{"opr": "sw", "col": filter_field, "value": value}] + ) + return filters + + @expose("/related/", methods=["GET"]) + @protect() + @safe + @rison(get_related_schema) + def related(self, column_name: str, **kwargs): + """Get related fields data + --- + get: + parameters: + - in: path + schema: + type: string + name: column_name + - in: query + name: q + content: + application/json: + schema: + type: object + properties: + page_size: + type: integer + page: + type: integer + filter: + type: string + responses: + 200: + description: Related column data + content: + application/json: + schema: + type: object + properties: + count: + type: integer + result: + type: object + properties: + value: + type: integer + text: + type: string + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + args = kwargs.get("rison", {}) + # handle pagination + page, page_size = self._handle_page_args(args) + try: + datamodel = self.datamodel.get_related_interface(column_name) + except KeyError: + return self.response_404() + page, page_size = self._sanitize_page_args(page, page_size) + # handle ordering + order_field = self.order_rel_fields.get(column_name) + if order_field: + order_column, order_direction = order_field + else: + order_column, order_direction = "", "" + # handle filters + filters = self._get_related_filter(datamodel, column_name, args.get("filter")) + # Make the query + count, values = datamodel.query( + filters, order_column, order_direction, page=page, page_size=page_size + ) + # produce response + result = [ + {"value": datamodel.get_pk_value(value), "text": str(value)} + for value in values + ] + return self.response(200, count=count, result=result) + + +class BaseOwnedModelRestApi(BaseSupersetModelRestApi): + @expose("/", methods=["PUT"]) + @protect() + @check_ownership_and_item_exists + @safe + def put(self, item): # pylint: disable=arguments-differ + """Changes a owned Model + --- + put: + parameters: + - in: path + schema: + type: integer + name: pk + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + responses: + 200: + description: Item changed + content: + application/json: + schema: + type: object + properties: + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.put' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + self.response_400(message="Request is not JSON") + item = self.edit_model_schema.load(request.json, instance=item) + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.edit(item.data, raise_exception=True) + return self.response( + 200, result=self.edit_model_schema.dump(item.data, many=False).data + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + @expose("/", methods=["POST"]) + @protect() + @safe + def post(self): + """Creates a new owned Model + --- + post: + requestBody: + description: Model schema + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + responses: + 201: + description: Model added + content: + application/json: + schema: + type: object + properties: + id: + type: string + result: + $ref: '#/components/schemas/{{self.__class__.__name__}}.post' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + if not request.is_json: + return self.response_400(message="Request is not JSON") + item = self.add_model_schema.load(request.json) + # This validates custom Schema with custom validations + if item.errors: + return self.response_422(message=item.errors) + try: + self.datamodel.add(item.data, raise_exception=True) + return self.response( + 201, + result=self.add_model_schema.dump(item.data, many=False).data, + id=item.data.id, + ) + except SQLAlchemyError as e: + return self.response_422(message=str(e)) + + @expose("/", methods=["DELETE"]) + @protect() + @check_ownership_and_item_exists + @safe + def delete(self, item): # pylint: disable=arguments-differ + """Deletes owned Model + --- + delete: + parameters: + - in: path + schema: + type: integer + name: pk + responses: + 200: + description: Model delete + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/401' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + self.datamodel.delete(item, raise_exception=True) + return self.response(200, message="OK") + except SQLAlchemyError as e: + return self.response_422(message=str(e)) diff --git a/superset/views/base_schemas.py b/superset/views/base_schemas.py new file mode 100644 index 000000000000..8349ae8f30b9 --- /dev/null +++ b/superset/views/base_schemas.py @@ -0,0 +1,108 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Dict, List, Optional + +from flask import current_app, g +from flask_appbuilder import Model +from marshmallow import post_load, pre_load, Schema, ValidationError +from sqlalchemy.orm.exc import NoResultFound + + +def validate_owner(value): + try: + ( + current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model.id + ) + .filter_by(id=value) + .one() + ) + except NoResultFound: + raise ValidationError(f"User {value} does not exist") + + +class BaseSupersetSchema(Schema): + """ + Extends Marshmallow schema so that we can pass a Model to load + (following marshamallow-sqlalchemy pattern). This is useful + to perform partial model merges on HTTP PUT + """ + + __class_model__: Model = None + + def __init__(self, **kwargs): + self.instance: Optional[Model] = None + super().__init__(**kwargs) + + def load( + self, data, many=None, partial=None, instance: Model = None, **kwargs + ): # pylint: disable=arguments-differ + self.instance = instance + return super().load(data, many=many, partial=partial, **kwargs) + + @post_load + def make_object(self, data: Dict, discard: List[str] = None) -> Model: + """ + Creates a Model object from POST or PUT requests. PUT will use self.instance + previously fetched from the endpoint handler + + :param data: Schema data payload + :param discard: List of fields to not set on the model + """ + discard = discard or [] + if not self.instance: + self.instance = self.__class_model__() # pylint: disable=not-callable + for field in data: + if field not in discard: + setattr(self.instance, field, data.get(field)) + return self.instance + + +class BaseOwnedSchema(BaseSupersetSchema): + """ + Implements owners validation,pre load and post_load + (to populate the owners field) on Marshmallow schemas + """ + + owners_field_name = "owners" + + @post_load + def make_object(self, data: Dict, discard: List[str] = None) -> Model: + discard = discard or [] + discard.append(self.owners_field_name) + instance = super().make_object(data, discard) + if "owners" not in data and g.user not in instance.owners: + instance.owners.append(g.user) + if self.owners_field_name in data: + self.set_owners(instance, data[self.owners_field_name]) + return instance + + @pre_load + def pre_load(self, data: Dict): + data[self.owners_field_name] = data.get(self.owners_field_name, []) + + @staticmethod + def set_owners(instance: Model, owners: List[int]): + owner_objs = list() + if g.user.id not in owners: + owners.append(g.user.id) + for owner_id in owners: + user = current_app.appbuilder.get_session.query( + current_app.appbuilder.sm.user_model + ).get(owner_id) + owner_objs.append(user) + instance.owners = owner_objs diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index 45134184f6d0..ee6075fbaed9 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -28,7 +28,8 @@ from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.utils import core as utils -from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema, validate_owner +from superset.views.base_api import BaseOwnedModelRestApi +from superset.views.base_schemas import BaseOwnedSchema, validate_owner from superset.views.chart.mixin import SliceMixin diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index 695e599bc684..af274562ad52 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -27,7 +27,8 @@ from superset.exceptions import SupersetException from superset.models.dashboard import Dashboard from superset.utils import core as utils -from superset.views.base import BaseOwnedModelRestApi, BaseOwnedSchema, validate_owner +from superset.views.base_api import BaseOwnedModelRestApi +from superset.views.base_schemas import BaseOwnedSchema, validate_owner from .mixin import DashboardMixin From b93595e1c765ff66c0146252849f831ea3afc2af Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 8 Jan 2020 16:19:17 +0000 Subject: [PATCH 18/23] [charts] Fix bug on partial updates for dashboards --- superset/views/base.py | 11 +---------- superset/views/base_schemas.py | 4 +++- superset/views/chart/api.py | 3 ++- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index a6560841045f..604876e26959 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -22,16 +22,7 @@ import simplejson as json import yaml -from flask import ( - abort, - flash, - g, - get_flashed_messages, - redirect, - request, - Response, - session, -) +from flask import abort, flash, g, get_flashed_messages, redirect, Response, session from flask_appbuilder import BaseView, ModelView from flask_appbuilder.actions import action from flask_appbuilder.forms import DynamicForm diff --git a/superset/views/base_schemas.py b/superset/views/base_schemas.py index 8349ae8f30b9..704ede4e3106 100644 --- a/superset/views/base_schemas.py +++ b/superset/views/base_schemas.py @@ -93,7 +93,9 @@ def make_object(self, data: Dict, discard: List[str] = None) -> Model: @pre_load def pre_load(self, data: Dict): - data[self.owners_field_name] = data.get(self.owners_field_name, []) + # if PUT request don't set owners to empty list + if not self.instance: + data[self.owners_field_name] = data.get(self.owners_field_name, []) @staticmethod def set_owners(instance: Model, owners: List[int]): diff --git a/superset/views/chart/api.py b/superset/views/chart/api.py index ee6075fbaed9..511b62ae9fcd 100644 --- a/superset/views/chart/api.py +++ b/superset/views/chart/api.py @@ -122,7 +122,8 @@ def validate_schema(self, data: Dict): # pylint: disable=no-self-use @post_load def make_object(self, data: Dict, discard: List[str] = None) -> Slice: self.instance = super().make_object(data, ["dashboards"]) - populate_dashboards(self.instance, data.get("dashboards", [])) + if "dashboards" in data: + populate_dashboards(self.instance, data["dashboards"]) return self.instance From c8841e79dba98b97e4701c3f47be7fc1e34df75e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 8 Jan 2020 16:42:05 +0000 Subject: [PATCH 19/23] [charts] Fix missing apache license --- superset/views/base_api.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 2536e90407ac..4c2bc617e063 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -1,3 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. import functools from typing import Dict, Tuple From c9f333f600ee967a64cd990ad832849dc0ad2e1d Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 9 Jan 2020 10:04:54 +0000 Subject: [PATCH 20/23] black app.py after merge --- superset/app.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/superset/app.py b/superset/app.py index 4732e5ca1022..e30c63f01041 100644 --- a/superset/app.py +++ b/superset/app.py @@ -149,11 +149,7 @@ def init_views(self) -> None: CssTemplateAsyncModelView, ) from superset.views.chart.api import ChartRestApi - from superset.views.chart.views import ( - SliceModelView, - SliceAsync, - SliceAddView, - ) + from superset.views.chart.views import SliceModelView, SliceAsync, SliceAddView from superset.views.dashboard.api import DashboardRestApi from superset.views.dashboard.views import ( DashboardModelView, From 8bb704f931b889b5f1ef71effed4441018a184bc Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Fri, 17 Jan 2020 12:05:40 +0000 Subject: [PATCH 21/23] [charts] Fix, missing imports and black --- superset/views/dashboard/api.py | 4 +++- tests/dashboard_api_tests.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/superset/views/dashboard/api.py b/superset/views/dashboard/api.py index 0d1a15e35525..8704904db5fb 100644 --- a/superset/views/dashboard/api.py +++ b/superset/views/dashboard/api.py @@ -27,9 +27,9 @@ from superset.exceptions import SupersetException from superset.models.dashboard import Dashboard from superset.utils import core as utils +from superset.views.base import generate_download_headers from superset.views.base_api import BaseOwnedModelRestApi from superset.views.base_schemas import BaseOwnedSchema, validate_owner -from superset.views.base import generate_download_headers from .mixin import DashboardMixin @@ -118,8 +118,10 @@ def make_object(self, data: Dict, discard: List[str] = None) -> Dashboard: slc.owners = list(set(self.instance.owners) | set(slc.owners)) return self.instance + get_export_ids_schema = {"type": "array", "items": {"type": "integer"}} + class DashboardRestApi(DashboardMixin, BaseOwnedModelRestApi): datamodel = SQLAInterface(Dashboard) diff --git a/tests/dashboard_api_tests.py b/tests/dashboard_api_tests.py index 7ce5eb19a7cd..6f037217795c 100644 --- a/tests/dashboard_api_tests.py +++ b/tests/dashboard_api_tests.py @@ -18,6 +18,8 @@ import json from typing import List +import prison + from superset import db, security_manager from superset.models import core as models from superset.models.slice import Slice From 6a83c7d90a73bad5c4eca73f9a30f6aea3eb849a Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 21 Jan 2020 12:12:40 +0000 Subject: [PATCH 22/23] [api] Log on sqlalchemy error --- superset/views/base_api.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 4c2bc617e063..df4eec77b8d5 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import functools +import logging from typing import Dict, Tuple from flask import request @@ -26,6 +27,7 @@ from superset.exceptions import SupersetSecurityException from superset.views.base import check_ownership + get_related_schema = { "type": "object", "properties": { @@ -61,6 +63,8 @@ class BaseSupersetModelRestApi(ModelRestApi): Extends FAB's ModelResApi to implement specific superset generic functionality """ + logger = logging.getLogger(__name__) + order_rel_fields: Dict[str, Tuple[str, str]] = {} """ Impose ordering on related fields query:: @@ -225,6 +229,7 @@ def put(self, item): # pylint: disable=arguments-differ 200, result=self.edit_model_schema.dump(item.data, many=False).data ) except SQLAlchemyError as e: + self.logger.error(f"Error updating model {self.__class__.__name__}: {e}") return self.response_422(message=str(e)) @expose("/", methods=["POST"]) @@ -276,6 +281,7 @@ def post(self): id=item.data.id, ) except SQLAlchemyError as e: + self.logger.error(f"Error creating model {self.__class__.__name__}: {e}") return self.response_422(message=str(e)) @expose("/", methods=["DELETE"]) @@ -316,4 +322,5 @@ def delete(self, item): # pylint: disable=arguments-differ self.datamodel.delete(item, raise_exception=True) return self.response(200, message="OK") except SQLAlchemyError as e: + self.logger.error(f"Error deleting model {self.__class__.__name__}: {e}") return self.response_422(message=str(e)) From 32e28da66373ca517487baaf5fab0828e4244082 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Tue, 21 Jan 2020 13:01:31 +0000 Subject: [PATCH 23/23] [api] isort --- superset/views/base_api.py | 1 - 1 file changed, 1 deletion(-) diff --git a/superset/views/base_api.py b/superset/views/base_api.py index df4eec77b8d5..a2ee1d4084bc 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -27,7 +27,6 @@ from superset.exceptions import SupersetSecurityException from superset.views.base import check_ownership - get_related_schema = { "type": "object", "properties": {