From 807496350b1df73c04790eca3d41035e25905af0 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 22 Nov 2021 13:40:30 -0300 Subject: [PATCH 01/14] feat: Adds a key-value endpoint to store the state of dashboard filters --- superset/config.py | 3 + superset/dashboards/filters_state/__init__.py | 16 ++ superset/dashboards/filters_state/api.py | 234 ++++++++++++++++++ superset/dashboards/filters_state/dao.py | 93 +++++++ superset/initialization/__init__.py | 2 + superset/key_value/api.py | 86 +++++++ superset/key_value/commands/__init__.py | 16 ++ superset/key_value/commands/create.py | 51 ++++ superset/key_value/commands/delete.py | 45 ++++ superset/key_value/commands/exceptions.py | 40 +++ superset/key_value/commands/get.py | 50 ++++ superset/key_value/commands/update.py | 55 ++++ superset/key_value/schemas.py | 29 +++ superset/key_value/utils.py | 26 ++ superset/models/key_value.py | 31 +++ superset/utils/cache_manager.py | 12 + .../dashboards/filters_state/__init__.py | 16 ++ .../dashboards/filters_state/api_tests.py | 89 +++++++ 18 files changed, 894 insertions(+) create mode 100644 superset/dashboards/filters_state/__init__.py create mode 100644 superset/dashboards/filters_state/api.py create mode 100644 superset/dashboards/filters_state/dao.py create mode 100644 superset/key_value/api.py create mode 100644 superset/key_value/commands/__init__.py create mode 100644 superset/key_value/commands/create.py create mode 100644 superset/key_value/commands/delete.py create mode 100644 superset/key_value/commands/exceptions.py create mode 100644 superset/key_value/commands/get.py create mode 100644 superset/key_value/commands/update.py create mode 100644 superset/key_value/schemas.py create mode 100644 superset/key_value/utils.py create mode 100644 superset/models/key_value.py create mode 100644 tests/integration_tests/dashboards/filters_state/__init__.py create mode 100644 tests/integration_tests/dashboards/filters_state/api_tests.py diff --git a/superset/config.py b/superset/config.py index 2d7e7dbb4696..2b35126f7957 100644 --- a/superset/config.py +++ b/superset/config.py @@ -561,6 +561,9 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # Cache for datasource metadata and query results DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} +# Cache for filters state +FILTERS_STATE_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} + # store cache keys by datasource UID (via CacheKey) for custom processing/invalidation STORE_CACHE_KEYS_IN_METADATA_DB = False diff --git a/superset/dashboards/filters_state/__init__.py b/superset/dashboards/filters_state/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/superset/dashboards/filters_state/__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/dashboards/filters_state/api.py b/superset/dashboards/filters_state/api.py new file mode 100644 index 000000000000..c4ea25fcf0f7 --- /dev/null +++ b/superset/dashboards/filters_state/api.py @@ -0,0 +1,234 @@ +# 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 logging +from typing import Type + +from flask import Response +from flask_appbuilder.api import expose, protect, safe + +from superset.dashboards.filters_state.dao import KeyValueDAO +from superset.extensions import event_logger +from superset.key_value.api import KeyValueRestApi +from superset.views.base_api import statsd_metrics + +logger = logging.getLogger(__name__) + + +class FiltersStateRestApi(KeyValueRestApi): + class_permission_name = "FiltersStateRestApi" + resource_name = "dashboard" + openapi_spec_tag = "Filters State" + + def get_dao(self) -> Type[KeyValueDAO]: + return KeyValueDAO + + @expose("//filters_state", methods=["POST"]) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", + log_to_statsd=False, + ) + def post(self, dashboard_id: int) -> Response: + """Stores a new value. + --- + post: + description: >- + Stores a new value. + parameters: + - in: path + schema: + type: integer + name: dashboard_id + requestBody: + required: true + content: + application/json: + schema: + type: object + $ref: '#/components/schemas/KeyValuePostSchema' + responses: + 201: + description: The value was stored successfully. It returns the key to retrieve the value. + content: + application/json: + schema: + type: object + properties: + key: + type: string + description: The key to retrieve the value. + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return super().post(dashboard_id) + + @expose("//filters_state//", methods=["PUT"]) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", + log_to_statsd=False, + ) + def put(self, dashboard_id: int, key: str) -> Response: + """Updates an existing value. + --- + put: + description: >- + Updates an existing value. + parameters: + - in: path + schema: + type: integer + name: dashboard_id + - in: path + schema: + type: string + name: key + requestBody: + required: true + content: + application/json: + schema: + type: object + $ref: '#/components/schemas/KeyValuePutSchema' + responses: + 200: + description: The value was stored successfully. + content: + application/json: + schema: + type: object + properties: + message: + type: string + description: The result of the operation + 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' + """ + return super().put(dashboard_id, key) + + @expose("//filters_state//", methods=["GET"]) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", + log_to_statsd=False, + ) + def get(self, dashboard_id: int, key: str) -> Response: + """Retrives a value. + --- + get: + description: >- + Retrives a value. + parameters: + - in: path + schema: + type: integer + name: dashboard_id + - in: path + schema: + type: string + name: key + responses: + 200: + description: Returns the stored value. + content: + application/json: + schema: + type: object + properties: + value: + type: string + description: The stored value + 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' + """ + return super().get(dashboard_id, key) + + @expose("//filters_state//", methods=["DELETE"]) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", + log_to_statsd=False, + ) + def delete(self, dashboard_id: int, key: str) -> Response: + """Deletes a value. + --- + delete: + description: >- + Deletes a value. + parameters: + - in: path + schema: + type: integer + name: dashboard_id + - in: path + schema: + type: string + name: key + description: The value key. + responses: + 200: + description: Deleted the stored value. + content: + application/json: + schema: + type: object + properties: + message: + type: string + description: The result of the operation + 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' + """ + return super().delete(dashboard_id, key) diff --git a/superset/dashboards/filters_state/dao.py b/superset/dashboards/filters_state/dao.py new file mode 100644 index 000000000000..7019655a2a5b --- /dev/null +++ b/superset/dashboards/filters_state/dao.py @@ -0,0 +1,93 @@ +# 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 logging +from typing import Optional + +from sqlalchemy.exc import SQLAlchemyError + +from superset.dao.exceptions import ( + DAOCreateFailedError, + DAODeleteFailedError, + DAOUpdateFailedError, +) +from superset.dashboards.dao import DashboardDAO +from superset.extensions import cache_manager +from superset.key_value.utils import cache_key + +logger = logging.getLogger(__name__) + + +class KeyValueDAO: + @staticmethod + def find_by_id(dashboard_id: int, id: str) -> Optional[str]: + """ + Finds a value that has been stored using a particular key + """ + try: + dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) + if dashboard: + return cache_manager.filters_state_cache.get( + cache_key(dashboard_id, id) + ) + except SQLAlchemyError as ex: # pragma: no cover + logger.error("Could not get value for the key: %s", str(ex), exc_info=True) + return None + + @staticmethod + def create(dashboard_id: int, key: str, value: str) -> Optional[bool]: + """ + Creates a value for a particular key + """ + try: + dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) + if dashboard: + return cache_manager.filters_state_cache.set( + cache_key(dashboard_id, key), value + ) + except SQLAlchemyError as ex: # pragma: no cover + raise DAOCreateFailedError(exception=ex) from ex + return False + + @staticmethod + def delete(dashboard_id: int, key: str) -> Optional[bool]: + """ + Deletes a value for a particular key + """ + try: + dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) + if dashboard: + return cache_manager.filters_state_cache.delete( + cache_key(dashboard_id, key) + ) + except SQLAlchemyError as ex: # pragma: no cover + raise DAODeleteFailedError(exception=ex) from ex + return False + + @staticmethod + def update(dashboard_id: int, key: str, value: str) -> Optional[bool]: + """ + Updates a value for a particular key + """ + try: + dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) + if dashboard: + return cache_manager.filters_state_cache.set( + cache_key(dashboard_id, key), value + ) + except SQLAlchemyError as ex: # pragma: no cover + raise DAOUpdateFailedError(exception=ex) from ex + return False diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 56e8b1bec087..8b6cf98827bf 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -134,6 +134,7 @@ def init_views(self) -> None: from superset.css_templates.api import CssTemplateRestApi from superset.dashboards.api import DashboardRestApi from superset.dashboards.filter_sets.api import FilterSetRestApi + from superset.dashboards.filters_state.api import FiltersStateRestApi from superset.databases.api import DatabaseRestApi from superset.datasets.api import DatasetRestApi from superset.datasets.columns.api import DatasetColumnsRestApi @@ -212,6 +213,7 @@ def init_views(self) -> None: appbuilder.add_api(ReportScheduleRestApi) appbuilder.add_api(ReportExecutionLogRestApi) appbuilder.add_api(FilterSetRestApi) + appbuilder.add_api(FiltersStateRestApi) # # Setup regular views # diff --git a/superset/key_value/api.py b/superset/key_value/api.py new file mode 100644 index 000000000000..8d0325e02737 --- /dev/null +++ b/superset/key_value/api.py @@ -0,0 +1,86 @@ +# 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 logging +from abc import abstractmethod +from typing import Any + +from flask import g, request, Response +from flask_appbuilder.models.sqla.interface import SQLAInterface + +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.exceptions import InvalidPayloadFormatError +from superset.key_value.commands.create import CreateKeyValueCommand +from superset.key_value.commands.delete import DeleteKeyValueCommand +from superset.key_value.commands.get import GetKeyValueCommand +from superset.key_value.commands.update import UpdateKeyValueCommand +from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema +from superset.models.key_value import KeyValueEntry +from superset.views.base_api import BaseSupersetModelRestApi + +logger = logging.getLogger(__name__) + + +class KeyValueRestApi(BaseSupersetModelRestApi): + datamodel = SQLAInterface(KeyValueEntry) + add_model_schema = KeyValuePostSchema() + edit_model_schema = KeyValuePutSchema() + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + include_route_methods = { + RouteMethod.POST, + RouteMethod.PUT, + RouteMethod.GET, + RouteMethod.DELETE, + } + allow_browser_login = True + openapi_spec_component_schemas = ( + KeyValuePostSchema, + KeyValuePutSchema, + ) + + def post(self, resource_id: int) -> Response: + if not request.is_json: + raise InvalidPayloadFormatError("Request is not JSON") + item = self.add_model_schema.load(request.json) + key = CreateKeyValueCommand(g.user, self.get_dao(), resource_id, item).run() + return self.response(201, key=key) + + def put(self, resource_id: int, key: str) -> Response: + if not request.is_json: + raise InvalidPayloadFormatError("Request is not JSON") + item = self.edit_model_schema.load(request.json) + result = UpdateKeyValueCommand( + g.user, self.get_dao(), resource_id, key, item + ).run() + if not result: + return self.response_404() + return self.response(200, message="Value updated successfully.",) + + def get(self, resource_id: int, key: str) -> Response: + value = GetKeyValueCommand(g.user, self.get_dao(), resource_id, key).run() + if not value: + return self.response_404() + return self.response(200, value=value) + + def delete(self, resource_id: int, key: str) -> Response: + result = DeleteKeyValueCommand(g.user, self.get_dao(), resource_id, key).run() + if not result: + return self.response_404() + return self.response(200, message="Deleted successfully") + + @abstractmethod + def get_dao(self) -> Any: + ... diff --git a/superset/key_value/commands/__init__.py b/superset/key_value/commands/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/superset/key_value/commands/__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/key_value/commands/create.py b/superset/key_value/commands/create.py new file mode 100644 index 000000000000..11a7c3389542 --- /dev/null +++ b/superset/key_value/commands/create.py @@ -0,0 +1,51 @@ +# 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 logging +from secrets import token_urlsafe +from typing import Any, Dict + +from flask_appbuilder.models.sqla import Model +from flask_appbuilder.security.sqla.models import User + +from superset.commands.base import BaseCommand +from superset.dao.exceptions import DAOException +from superset.key_value.commands.exceptions import KeyValueCreateFailedError + +logger = logging.getLogger(__name__) + + +class CreateKeyValueCommand(BaseCommand): + def __init__( + self, user: User, get_dao: Any, resource_id: int, data: Dict[str, Any], + ): + self._actor = user + self._get_dao = get_dao + self._resource_id = resource_id + self._properties = data.copy() + + def run(self) -> Model: + try: + key = token_urlsafe(48) + value = self._properties.get("value") + self._get_dao.create(self._resource_id, key, value) + return key + except DAOException as ex: + logger.exception(ex.exception) + raise KeyValueCreateFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py new file mode 100644 index 000000000000..f979febeac6a --- /dev/null +++ b/superset/key_value/commands/delete.py @@ -0,0 +1,45 @@ +# 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 logging +from typing import Any + +from flask_appbuilder.models.sqla import Model +from flask_appbuilder.security.sqla.models import User + +from superset.commands.base import BaseCommand +from superset.dao.exceptions import DAOException +from superset.key_value.commands.exceptions import KeyValueDeleteFailedError + +logger = logging.getLogger(__name__) + + +class DeleteKeyValueCommand(BaseCommand): + def __init__(self, user: User, get_dao: Any, resource_id: int, key: str): + self._actor = user + self._get_dao = get_dao + self._resource_id = resource_id + self._key = key + + def run(self) -> Model: + try: + return self._get_dao.delete(self._resource_id, self._key) + except DAOException as ex: + logger.exception(ex.exception) + raise KeyValueDeleteFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/key_value/commands/exceptions.py b/superset/key_value/commands/exceptions.py new file mode 100644 index 000000000000..f8c9dbd4a4ff --- /dev/null +++ b/superset/key_value/commands/exceptions.py @@ -0,0 +1,40 @@ +# 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_babel import lazy_gettext as _ + +from superset.commands.exceptions import ( + CommandException, + CreateFailedError, + DeleteFailedError, + UpdateFailedError, +) + + +class KeyValueCreateFailedError(CreateFailedError): + message = _("An error occurred while creating the value.") + + +class KeyValueGetFailedError(CommandException): + message = _("An error occurred while accessing the value.") + + +class KeyValueDeleteFailedError(DeleteFailedError): + message = _("An error occurred while deleting the value.") + + +class KeyValueUpdateFailedError(UpdateFailedError): + message = _("An error occurred while updating the value.") diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py new file mode 100644 index 000000000000..5ecc0431e870 --- /dev/null +++ b/superset/key_value/commands/get.py @@ -0,0 +1,50 @@ +# 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 logging +from typing import Any + +from flask import current_app as app +from flask_appbuilder.models.sqla import Model +from flask_appbuilder.security.sqla.models import User + +from superset.commands.base import BaseCommand +from superset.dao.exceptions import DAOException +from superset.key_value.commands.exceptions import KeyValueGetFailedError + +logger = logging.getLogger(__name__) + + +class GetKeyValueCommand(BaseCommand): + def __init__(self, user: User, get_dao: Any, resource_id: int, key: str): + self._actor = user + self._get_dao = get_dao + self._resource_id = resource_id + self._key = key + + def run(self) -> Model: + try: + value = self._get_dao.find_by_id(self._resource_id, self._key) + config = app.config["FILTERS_STATE_CACHE_CONFIG"] + if config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") == True: + self._get_dao.update(self._resource_id, self._key, value) + return value + except DAOException as ex: + logger.exception(ex.exception) + raise KeyValueGetFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py new file mode 100644 index 000000000000..943df52227eb --- /dev/null +++ b/superset/key_value/commands/update.py @@ -0,0 +1,55 @@ +# 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 logging +from typing import Any, Dict + +from flask_appbuilder.models.sqla import Model +from flask_appbuilder.security.sqla.models import User + +from superset.commands.base import BaseCommand +from superset.dao.exceptions import DAOException +from superset.key_value.commands.exceptions import KeyValueUpdateFailedError + +logger = logging.getLogger(__name__) + + +class UpdateKeyValueCommand(BaseCommand): + def __init__( + self, + user: User, + get_dao: Any, + resource_id: int, + key: str, + data: Dict[str, Any], + ): + self._actor = user + self._get_dao = get_dao + self._resource_id = resource_id + self._key = key + self._properties = data.copy() + + def run(self) -> Model: + try: + return self._get_dao.update( + self._resource_id, self._key, self._properties.get("value") + ) + except DAOException as ex: + logger.exception(ex.exception) + raise KeyValueUpdateFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/key_value/schemas.py b/superset/key_value/schemas.py new file mode 100644 index 000000000000..d3b5eef6e1dc --- /dev/null +++ b/superset/key_value/schemas.py @@ -0,0 +1,29 @@ +# 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 marshmallow import fields, Schema + + +class KeyValuePostSchema(Schema): + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported value." + ) + + +class KeyValuePutSchema(Schema): + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported value." + ) diff --git a/superset/key_value/utils.py b/superset/key_value/utils.py new file mode 100644 index 000000000000..c700fa8f223a --- /dev/null +++ b/superset/key_value/utils.py @@ -0,0 +1,26 @@ +# 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 Any + + +def cache_key(*args: Any) -> str: + result = "" + separator = "" + for arg in args: + result += separator + str(arg) + separator = ";" + return result diff --git a/superset/models/key_value.py b/superset/models/key_value.py new file mode 100644 index 000000000000..b7ab19d24cee --- /dev/null +++ b/superset/models/key_value.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 datetime import datetime + +from flask_appbuilder import Model +from sqlalchemy import Boolean, Column, DateTime, Integer, String, Text + +from superset.models.helpers import AuditMixinNullable + + +class KeyValueEntry(Model, AuditMixinNullable): + + """Key value entry""" + + __tablename__ = "key_value" + key = Column(String(64), primary_key=True) + value = Column(Text, nullable=False) diff --git a/superset/utils/cache_manager.py b/superset/utils/cache_manager.py index 352f62f0273c..dece40b06bae 100644 --- a/superset/utils/cache_manager.py +++ b/superset/utils/cache_manager.py @@ -25,6 +25,7 @@ def __init__(self) -> None: self._cache = Cache() self._data_cache = Cache() self._thumbnail_cache = Cache() + self._filters_state_cache = Cache() def init_app(self, app: Flask) -> None: self._cache.init_app( @@ -48,6 +49,13 @@ def init_app(self, app: Flask) -> None: **app.config["THUMBNAIL_CACHE_CONFIG"], }, ) + self._filters_state_cache.init_app( + app, + { + "CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], + **app.config["FILTERS_STATE_CACHE_CONFIG"], + }, + ) @property def data_cache(self) -> Cache: @@ -60,3 +68,7 @@ def cache(self) -> Cache: @property def thumbnail_cache(self) -> Cache: return self._thumbnail_cache + + @property + def filters_state_cache(self) -> Cache: + return self._filters_state_cache diff --git a/tests/integration_tests/dashboards/filters_state/__init__.py b/tests/integration_tests/dashboards/filters_state/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/integration_tests/dashboards/filters_state/__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/tests/integration_tests/dashboards/filters_state/api_tests.py b/tests/integration_tests/dashboards/filters_state/api_tests.py new file mode 100644 index 000000000000..d4c8a363c170 --- /dev/null +++ b/tests/integration_tests/dashboards/filters_state/api_tests.py @@ -0,0 +1,89 @@ +# 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 pytest +import json +from superset import security_manager +from superset.models.dashboard import Dashboard +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.test_app import app +from sqlalchemy.orm import Session + +dashboardId = 1 +value = "test" + + +class FilterStateTests(SupersetTestCase): + def post(self): + payload = { + "value": value, + } + return self.client.post( + f"api/v1/dashboard/{dashboardId}/filters_state", json=payload + ) + + def clearTable(self, session, model): + rows = session.query(model).all() + for row in rows: + session.delete(row) + session.commit() + + def createDashboard(self, session): + admin = self.get_user("admin") + user = session.query(security_manager.user_model).get(admin.id) + dashboard = Dashboard( + id=dashboardId, + dashboard_title="test", + slug=None, + owners=[user], + roles=[], + position_json="", + css="", + json_metadata="", + slices=[], + published=False, + created_by=None, + ) + session.add(dashboard) + session.commit() + + @pytest.fixture(autouse=True, scope="session") + def beforeAll(self): + with app.app_context() as ctx: + session: Session = ctx.app.appbuilder.get_session + self.clearTable(session, Dashboard) + self.createDashboard(session) + + def setUp(self): + self.login(username="admin") + + def test_post(self): + resp = self.post() + assert resp.status_code == 201 + + def test_get_not_found(self): + resp = self.client.get("unknown-key") + assert resp.status_code == 404 + + def test_get(self): + resp = self.post() + data = json.loads(resp.data.decode("utf-8")) + key = data.get("key") + resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filters_state/{key}/") + assert resp.status_code == 200 + data = json.loads(resp.data.decode("utf-8")) + assert value == data.get("value") From 588a016eca3a80a19e861472fe03470b4cd17d9a Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 24 Nov 2021 11:54:04 -0300 Subject: [PATCH 02/14] Fixes pylint issues --- superset/dashboards/filters_state/api.py | 34 +++++++++---------- superset/dashboards/filters_state/dao.py | 4 +-- superset/key_value/api.py | 22 +++++------- superset/models/key_value.py | 4 +-- .../dashboards/filters_state/api_tests.py | 5 ++- 5 files changed, 32 insertions(+), 37 deletions(-) diff --git a/superset/dashboards/filters_state/api.py b/superset/dashboards/filters_state/api.py index c4ea25fcf0f7..a4262e813535 100644 --- a/superset/dashboards/filters_state/api.py +++ b/superset/dashboards/filters_state/api.py @@ -36,7 +36,7 @@ class FiltersStateRestApi(KeyValueRestApi): def get_dao(self) -> Type[KeyValueDAO]: return KeyValueDAO - @expose("//filters_state", methods=["POST"]) + @expose("//filters_state", methods=["POST"]) @protect() @safe @statsd_metrics @@ -44,7 +44,7 @@ def get_dao(self) -> Type[KeyValueDAO]: action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", log_to_statsd=False, ) - def post(self, dashboard_id: int) -> Response: + def post(self, pk: int) -> Response: """Stores a new value. --- post: @@ -54,7 +54,7 @@ def post(self, dashboard_id: int) -> Response: - in: path schema: type: integer - name: dashboard_id + name: pk requestBody: required: true content: @@ -64,7 +64,7 @@ def post(self, dashboard_id: int) -> Response: $ref: '#/components/schemas/KeyValuePostSchema' responses: 201: - description: The value was stored successfully. It returns the key to retrieve the value. + description: The value was stored successfully. content: application/json: schema: @@ -82,9 +82,9 @@ def post(self, dashboard_id: int) -> Response: 500: $ref: '#/components/responses/500' """ - return super().post(dashboard_id) + return super().post(pk) - @expose("//filters_state//", methods=["PUT"]) + @expose("//filters_state//", methods=["PUT"]) @protect() @safe @statsd_metrics @@ -92,7 +92,7 @@ def post(self, dashboard_id: int) -> Response: action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", log_to_statsd=False, ) - def put(self, dashboard_id: int, key: str) -> Response: + def put(self, pk: int, key: str) -> Response: """Updates an existing value. --- put: @@ -102,7 +102,7 @@ def put(self, dashboard_id: int, key: str) -> Response: - in: path schema: type: integer - name: dashboard_id + name: pk - in: path schema: type: string @@ -136,9 +136,9 @@ def put(self, dashboard_id: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().put(dashboard_id, key) + return super().put(pk, key) - @expose("//filters_state//", methods=["GET"]) + @expose("//filters_state//", methods=["GET"]) @protect() @safe @statsd_metrics @@ -146,7 +146,7 @@ def put(self, dashboard_id: int, key: str) -> Response: action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", log_to_statsd=False, ) - def get(self, dashboard_id: int, key: str) -> Response: + def get(self, pk: int, key: str) -> Response: """Retrives a value. --- get: @@ -156,7 +156,7 @@ def get(self, dashboard_id: int, key: str) -> Response: - in: path schema: type: integer - name: dashboard_id + name: pk - in: path schema: type: string @@ -183,9 +183,9 @@ def get(self, dashboard_id: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().get(dashboard_id, key) + return super().get(pk, key) - @expose("//filters_state//", methods=["DELETE"]) + @expose("//filters_state//", methods=["DELETE"]) @protect() @safe @statsd_metrics @@ -193,7 +193,7 @@ def get(self, dashboard_id: int, key: str) -> Response: action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", log_to_statsd=False, ) - def delete(self, dashboard_id: int, key: str) -> Response: + def delete(self, pk: int, key: str) -> Response: """Deletes a value. --- delete: @@ -203,7 +203,7 @@ def delete(self, dashboard_id: int, key: str) -> Response: - in: path schema: type: integer - name: dashboard_id + name: pk - in: path schema: type: string @@ -231,4 +231,4 @@ def delete(self, dashboard_id: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().delete(dashboard_id, key) + return super().delete(pk, key) diff --git a/superset/dashboards/filters_state/dao.py b/superset/dashboards/filters_state/dao.py index 7019655a2a5b..bcb24d55f15e 100644 --- a/superset/dashboards/filters_state/dao.py +++ b/superset/dashboards/filters_state/dao.py @@ -33,7 +33,7 @@ class KeyValueDAO: @staticmethod - def find_by_id(dashboard_id: int, id: str) -> Optional[str]: + def find_by_id(dashboard_id: int, key: str) -> Optional[str]: """ Finds a value that has been stored using a particular key """ @@ -41,7 +41,7 @@ def find_by_id(dashboard_id: int, id: str) -> Optional[str]: dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) if dashboard: return cache_manager.filters_state_cache.get( - cache_key(dashboard_id, id) + cache_key(dashboard_id, key) ) except SQLAlchemyError as ex: # pragma: no cover logger.error("Could not get value for the key: %s", str(ex), exc_info=True) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 8d0325e02737..3cd355ab4318 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -46,37 +46,31 @@ class KeyValueRestApi(BaseSupersetModelRestApi): RouteMethod.DELETE, } allow_browser_login = True - openapi_spec_component_schemas = ( - KeyValuePostSchema, - KeyValuePutSchema, - ) - def post(self, resource_id: int) -> Response: + def post(self, pk: int) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.add_model_schema.load(request.json) - key = CreateKeyValueCommand(g.user, self.get_dao(), resource_id, item).run() + key = CreateKeyValueCommand(g.user, self.get_dao(), pk, item).run() return self.response(201, key=key) - def put(self, resource_id: int, key: str) -> Response: + def put(self, pk: int, key: str) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.edit_model_schema.load(request.json) - result = UpdateKeyValueCommand( - g.user, self.get_dao(), resource_id, key, item - ).run() + result = UpdateKeyValueCommand(g.user, self.get_dao(), pk, key, item).run() if not result: return self.response_404() return self.response(200, message="Value updated successfully.",) - def get(self, resource_id: int, key: str) -> Response: - value = GetKeyValueCommand(g.user, self.get_dao(), resource_id, key).run() + def get(self, pk: int, key: str) -> Response: + value = GetKeyValueCommand(g.user, self.get_dao(), pk, key).run() if not value: return self.response_404() return self.response(200, value=value) - def delete(self, resource_id: int, key: str) -> Response: - result = DeleteKeyValueCommand(g.user, self.get_dao(), resource_id, key).run() + def delete(self, pk: int, key: str) -> Response: + result = DeleteKeyValueCommand(g.user, self.get_dao(), pk, key).run() if not result: return self.response_404() return self.response(200, message="Deleted successfully") diff --git a/superset/models/key_value.py b/superset/models/key_value.py index b7ab19d24cee..ba1f6b31b3d6 100644 --- a/superset/models/key_value.py +++ b/superset/models/key_value.py @@ -14,10 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from datetime import datetime - from flask_appbuilder import Model -from sqlalchemy import Boolean, Column, DateTime, Integer, String, Text +from sqlalchemy import Column, String, Text from superset.models.helpers import AuditMixinNullable diff --git a/tests/integration_tests/dashboards/filters_state/api_tests.py b/tests/integration_tests/dashboards/filters_state/api_tests.py index d4c8a363c170..561f842d8af9 100644 --- a/tests/integration_tests/dashboards/filters_state/api_tests.py +++ b/tests/integration_tests/dashboards/filters_state/api_tests.py @@ -17,8 +17,9 @@ # isort:skip_file import pytest import json -from superset import security_manager +from superset import security_manager, app from superset.models.dashboard import Dashboard +from superset.extensions import cache_manager from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.test_app import app from sqlalchemy.orm import Session @@ -64,6 +65,8 @@ def createDashboard(self, session): @pytest.fixture(autouse=True, scope="session") def beforeAll(self): with app.app_context() as ctx: + app.config["FILTERS_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + cache_manager.init_app(app) session: Session = ctx.app.appbuilder.get_session self.clearTable(session, Dashboard) self.createDashboard(session) From 2f8f429679a6ee65d01e3bc96b6ca285a809f79f Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 24 Nov 2021 13:08:12 -0300 Subject: [PATCH 03/14] Adds openapi schemas --- superset/key_value/api.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 3cd355ab4318..048b6d88d9cf 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -46,6 +46,10 @@ class KeyValueRestApi(BaseSupersetModelRestApi): RouteMethod.DELETE, } allow_browser_login = True + openapi_spec_component_schemas = ( + KeyValuePostSchema, + KeyValuePutSchema, + ) def post(self, pk: int) -> Response: if not request.is_json: From d1f8bcb4331664998ecc1c497b8e52dd43b29fd3 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 29 Nov 2021 11:45:16 -0300 Subject: [PATCH 04/14] Adds more tests, move logic to commands and use singular form for the endpoint name --- superset/config.py | 2 +- .../__init__.py | 0 .../{filters_state => filter_state}/api.py | 32 +++++-- .../filter_state/commands}/__init__.py | 0 .../filter_state/commands/create.py | 32 +++++++ .../filter_state/commands/delete.py | 30 ++++++ .../dashboards/filter_state/commands/get.py | 33 +++++++ .../filter_state/commands/update.py | 32 +++++++ superset/dashboards/filters_state/dao.py | 93 ------------------- superset/initialization/__init__.py | 4 +- superset/key_value/api.py | 26 ++++-- superset/key_value/commands/create.py | 22 +++-- superset/key_value/commands/delete.py | 18 ++-- superset/key_value/commands/get.py | 24 ++--- superset/key_value/commands/update.py | 28 +++--- superset/key_value/schemas.py | 8 +- superset/key_value/utils.py | 9 +- superset/utils/cache_manager.py | 10 +- .../dashboards/filter_state/__init__.py | 16 ++++ .../api_tests.py | 25 ++++- 20 files changed, 271 insertions(+), 173 deletions(-) rename superset/dashboards/{filters_state => filter_state}/__init__.py (100%) rename superset/dashboards/{filters_state => filter_state}/api.py (85%) rename {tests/integration_tests/dashboards/filters_state => superset/dashboards/filter_state/commands}/__init__.py (100%) create mode 100644 superset/dashboards/filter_state/commands/create.py create mode 100644 superset/dashboards/filter_state/commands/delete.py create mode 100644 superset/dashboards/filter_state/commands/get.py create mode 100644 superset/dashboards/filter_state/commands/update.py delete mode 100644 superset/dashboards/filters_state/dao.py create mode 100644 tests/integration_tests/dashboards/filter_state/__init__.py rename tests/integration_tests/dashboards/{filters_state => filter_state}/api_tests.py (78%) diff --git a/superset/config.py b/superset/config.py index 2b35126f7957..154c1ff72bf9 100644 --- a/superset/config.py +++ b/superset/config.py @@ -562,7 +562,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} # Cache for filters state -FILTERS_STATE_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} +FILTER_STATE_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} # store cache keys by datasource UID (via CacheKey) for custom processing/invalidation STORE_CACHE_KEYS_IN_METADATA_DB = False diff --git a/superset/dashboards/filters_state/__init__.py b/superset/dashboards/filter_state/__init__.py similarity index 100% rename from superset/dashboards/filters_state/__init__.py rename to superset/dashboards/filter_state/__init__.py diff --git a/superset/dashboards/filters_state/api.py b/superset/dashboards/filter_state/api.py similarity index 85% rename from superset/dashboards/filters_state/api.py rename to superset/dashboards/filter_state/api.py index a4262e813535..6b0e78f93ee9 100644 --- a/superset/dashboards/filters_state/api.py +++ b/superset/dashboards/filter_state/api.py @@ -20,7 +20,10 @@ from flask import Response from flask_appbuilder.api import expose, protect, safe -from superset.dashboards.filters_state.dao import KeyValueDAO +from superset.dashboards.filter_state.commands.create import CreateFilterStateCommand +from superset.dashboards.filter_state.commands.delete import DeleteFilterStateCommand +from superset.dashboards.filter_state.commands.get import GetFilterStateCommand +from superset.dashboards.filter_state.commands.update import UpdateFilterStateCommand from superset.extensions import event_logger from superset.key_value.api import KeyValueRestApi from superset.views.base_api import statsd_metrics @@ -28,15 +31,24 @@ logger = logging.getLogger(__name__) -class FiltersStateRestApi(KeyValueRestApi): - class_permission_name = "FiltersStateRestApi" +class DashboardFilterStateRestApi(KeyValueRestApi): + class_permission_name = "FilterStateRestApi" resource_name = "dashboard" - openapi_spec_tag = "Filters State" + openapi_spec_tag = "Filter State" - def get_dao(self) -> Type[KeyValueDAO]: - return KeyValueDAO + def get_create_command(self) -> Type[CreateFilterStateCommand]: + return CreateFilterStateCommand - @expose("//filters_state", methods=["POST"]) + def get_update_command(self) -> Type[UpdateFilterStateCommand]: + return UpdateFilterStateCommand + + def get_get_command(self) -> Type[GetFilterStateCommand]: + return GetFilterStateCommand + + def get_delete_command(self) -> Type[DeleteFilterStateCommand]: + return DeleteFilterStateCommand + + @expose("//filter_state", methods=["POST"]) @protect() @safe @statsd_metrics @@ -84,7 +96,7 @@ def post(self, pk: int) -> Response: """ return super().post(pk) - @expose("//filters_state//", methods=["PUT"]) + @expose("//filter_state//", methods=["PUT"]) @protect() @safe @statsd_metrics @@ -138,7 +150,7 @@ def put(self, pk: int, key: str) -> Response: """ return super().put(pk, key) - @expose("//filters_state//", methods=["GET"]) + @expose("//filter_state//", methods=["GET"]) @protect() @safe @statsd_metrics @@ -185,7 +197,7 @@ def get(self, pk: int, key: str) -> Response: """ return super().get(pk, key) - @expose("//filters_state//", methods=["DELETE"]) + @expose("//filter_state//", methods=["DELETE"]) @protect() @safe @statsd_metrics diff --git a/tests/integration_tests/dashboards/filters_state/__init__.py b/superset/dashboards/filter_state/commands/__init__.py similarity index 100% rename from tests/integration_tests/dashboards/filters_state/__init__.py rename to superset/dashboards/filter_state/commands/__init__.py diff --git a/superset/dashboards/filter_state/commands/create.py b/superset/dashboards/filter_state/commands/create.py new file mode 100644 index 000000000000..4b8a7890049a --- /dev/null +++ b/superset/dashboards/filter_state/commands/create.py @@ -0,0 +1,32 @@ +# 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 Optional + +from superset.dashboards.dao import DashboardDAO +from superset.extensions import cache_manager +from superset.key_value.commands.create import CreateKeyValueCommand +from superset.key_value.utils import cache_key + + +class CreateFilterStateCommand(CreateKeyValueCommand): + def create(self, resource_id: int, key: str, value: str) -> Optional[bool]: + dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) + if dashboard: + return cache_manager.filter_state_cache.set( + cache_key(resource_id, key), value + ) + return False diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py new file mode 100644 index 000000000000..89ee287945e4 --- /dev/null +++ b/superset/dashboards/filter_state/commands/delete.py @@ -0,0 +1,30 @@ +# 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 Optional + +from superset.dashboards.dao import DashboardDAO +from superset.extensions import cache_manager +from superset.key_value.commands.delete import DeleteKeyValueCommand +from superset.key_value.utils import cache_key + + +class DeleteFilterStateCommand(DeleteKeyValueCommand): + def delete(self, resource_id: int, key: str) -> Optional[bool]: + dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) + if dashboard: + return cache_manager.filter_state_cache.delete(cache_key(resource_id, key)) + return False diff --git a/superset/dashboards/filter_state/commands/get.py b/superset/dashboards/filter_state/commands/get.py new file mode 100644 index 000000000000..ea9dc9d9be70 --- /dev/null +++ b/superset/dashboards/filter_state/commands/get.py @@ -0,0 +1,33 @@ +# 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 Optional + +from superset.dashboards.dao import DashboardDAO +from superset.extensions import cache_manager +from superset.key_value.commands.get import GetKeyValueCommand +from superset.key_value.utils import cache_key + + +class GetFilterStateCommand(GetKeyValueCommand): + def get(self, resource_id: int, key: str, refreshTimeout: bool) -> Optional[str]: + dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) + if dashboard: + value = cache_manager.filter_state_cache.get(cache_key(resource_id, key)) + if refreshTimeout: + cache_manager.filter_state_cache.set(key, value) + return value + return None diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py new file mode 100644 index 000000000000..a432df612dc9 --- /dev/null +++ b/superset/dashboards/filter_state/commands/update.py @@ -0,0 +1,32 @@ +# 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 Optional + +from superset.dashboards.dao import DashboardDAO +from superset.extensions import cache_manager +from superset.key_value.commands.update import UpdateKeyValueCommand +from superset.key_value.utils import cache_key + + +class UpdateFilterStateCommand(UpdateKeyValueCommand): + def update(self, resource_id: int, key: str, value: str) -> Optional[bool]: + dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) + if dashboard: + return cache_manager.filter_state_cache.set( + cache_key(resource_id, key), value + ) + return False diff --git a/superset/dashboards/filters_state/dao.py b/superset/dashboards/filters_state/dao.py deleted file mode 100644 index bcb24d55f15e..000000000000 --- a/superset/dashboards/filters_state/dao.py +++ /dev/null @@ -1,93 +0,0 @@ -# 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 logging -from typing import Optional - -from sqlalchemy.exc import SQLAlchemyError - -from superset.dao.exceptions import ( - DAOCreateFailedError, - DAODeleteFailedError, - DAOUpdateFailedError, -) -from superset.dashboards.dao import DashboardDAO -from superset.extensions import cache_manager -from superset.key_value.utils import cache_key - -logger = logging.getLogger(__name__) - - -class KeyValueDAO: - @staticmethod - def find_by_id(dashboard_id: int, key: str) -> Optional[str]: - """ - Finds a value that has been stored using a particular key - """ - try: - dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) - if dashboard: - return cache_manager.filters_state_cache.get( - cache_key(dashboard_id, key) - ) - except SQLAlchemyError as ex: # pragma: no cover - logger.error("Could not get value for the key: %s", str(ex), exc_info=True) - return None - - @staticmethod - def create(dashboard_id: int, key: str, value: str) -> Optional[bool]: - """ - Creates a value for a particular key - """ - try: - dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) - if dashboard: - return cache_manager.filters_state_cache.set( - cache_key(dashboard_id, key), value - ) - except SQLAlchemyError as ex: # pragma: no cover - raise DAOCreateFailedError(exception=ex) from ex - return False - - @staticmethod - def delete(dashboard_id: int, key: str) -> Optional[bool]: - """ - Deletes a value for a particular key - """ - try: - dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) - if dashboard: - return cache_manager.filters_state_cache.delete( - cache_key(dashboard_id, key) - ) - except SQLAlchemyError as ex: # pragma: no cover - raise DAODeleteFailedError(exception=ex) from ex - return False - - @staticmethod - def update(dashboard_id: int, key: str, value: str) -> Optional[bool]: - """ - Updates a value for a particular key - """ - try: - dashboard = DashboardDAO.get_by_id_or_slug(str(dashboard_id)) - if dashboard: - return cache_manager.filters_state_cache.set( - cache_key(dashboard_id, key), value - ) - except SQLAlchemyError as ex: # pragma: no cover - raise DAOUpdateFailedError(exception=ex) from ex - return False diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 8b6cf98827bf..e33c038b11a7 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -134,7 +134,7 @@ def init_views(self) -> None: from superset.css_templates.api import CssTemplateRestApi from superset.dashboards.api import DashboardRestApi from superset.dashboards.filter_sets.api import FilterSetRestApi - from superset.dashboards.filters_state.api import FiltersStateRestApi + from superset.dashboards.filter_state.api import DashboardFilterStateRestApi from superset.databases.api import DatabaseRestApi from superset.datasets.api import DatasetRestApi from superset.datasets.columns.api import DatasetColumnsRestApi @@ -213,7 +213,7 @@ def init_views(self) -> None: appbuilder.add_api(ReportScheduleRestApi) appbuilder.add_api(ReportExecutionLogRestApi) appbuilder.add_api(FilterSetRestApi) - appbuilder.add_api(FiltersStateRestApi) + appbuilder.add_api(DashboardFilterStateRestApi) # # Setup regular views # diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 048b6d88d9cf..3bbdfba541f6 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. import logging -from abc import abstractmethod +from abc import ABC, abstractmethod from typing import Any from flask import g, request, Response @@ -34,7 +34,7 @@ logger = logging.getLogger(__name__) -class KeyValueRestApi(BaseSupersetModelRestApi): +class KeyValueRestApi(BaseSupersetModelRestApi, ABC): datamodel = SQLAInterface(KeyValueEntry) add_model_schema = KeyValuePostSchema() edit_model_schema = KeyValuePutSchema() @@ -55,30 +55,42 @@ def post(self, pk: int) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.add_model_schema.load(request.json) - key = CreateKeyValueCommand(g.user, self.get_dao(), pk, item).run() + key = self.get_create_command()(g.user, pk, item).run() return self.response(201, key=key) def put(self, pk: int, key: str) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.edit_model_schema.load(request.json) - result = UpdateKeyValueCommand(g.user, self.get_dao(), pk, key, item).run() + result = self.get_update_command()(g.user, pk, key, item).run() if not result: return self.response_404() return self.response(200, message="Value updated successfully.",) def get(self, pk: int, key: str) -> Response: - value = GetKeyValueCommand(g.user, self.get_dao(), pk, key).run() + value = self.get_get_command()(g.user, pk, key).run() if not value: return self.response_404() return self.response(200, value=value) def delete(self, pk: int, key: str) -> Response: - result = DeleteKeyValueCommand(g.user, self.get_dao(), pk, key).run() + result = self.get_delete_command()(g.user, pk, key).run() if not result: return self.response_404() return self.response(200, message="Deleted successfully") @abstractmethod - def get_dao(self) -> Any: + def get_create_command(self) -> Any: + ... + + @abstractmethod + def get_update_command(self) -> Any: + ... + + @abstractmethod + def get_get_command(self) -> Any: + ... + + @abstractmethod + def get_delete_command(self) -> Any: ... diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 11a7c3389542..c73dd55ae1ba 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -15,25 +15,25 @@ # specific language governing permissions and limitations # under the License. import logging +from abc import ABC, abstractmethod from secrets import token_urlsafe -from typing import Any, Dict +from typing import Any, Dict, Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.dao.exceptions import DAOException from superset.key_value.commands.exceptions import KeyValueCreateFailedError logger = logging.getLogger(__name__) -class CreateKeyValueCommand(BaseCommand): +class CreateKeyValueCommand(BaseCommand, ABC): def __init__( - self, user: User, get_dao: Any, resource_id: int, data: Dict[str, Any], + self, user: User, resource_id: int, data: Dict[str, Any], ): self._actor = user - self._get_dao = get_dao self._resource_id = resource_id self._properties = data.copy() @@ -41,11 +41,17 @@ def run(self) -> Model: try: key = token_urlsafe(48) value = self._properties.get("value") - self._get_dao.create(self._resource_id, key, value) - return key - except DAOException as ex: + if value: + self.create(self._resource_id, key, value) + return key + except SQLAlchemyError as ex: logger.exception(ex.exception) raise KeyValueCreateFailedError() from ex + return False def validate(self) -> None: pass + + @abstractmethod + def create(self, resource_id: int, key: str, value: str) -> Optional[bool]: + ... diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py index f979febeac6a..7784c29f8f77 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -15,31 +15,35 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Any +from abc import ABC, abstractmethod +from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.dao.exceptions import DAOException from superset.key_value.commands.exceptions import KeyValueDeleteFailedError logger = logging.getLogger(__name__) -class DeleteKeyValueCommand(BaseCommand): - def __init__(self, user: User, get_dao: Any, resource_id: int, key: str): +class DeleteKeyValueCommand(BaseCommand, ABC): + def __init__(self, user: User, resource_id: int, key: str): self._actor = user - self._get_dao = get_dao self._resource_id = resource_id self._key = key def run(self) -> Model: try: - return self._get_dao.delete(self._resource_id, self._key) - except DAOException as ex: + return self.delete(self._resource_id, self._key) + except SQLAlchemyError as ex: logger.exception(ex.exception) raise KeyValueDeleteFailedError() from ex def validate(self) -> None: pass + + @abstractmethod + def delete(self, resource_id: int, key: str) -> Optional[bool]: + ... diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py index 5ecc0431e870..f2d1629a63fa 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -15,36 +15,38 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Any +from abc import ABC, abstractmethod +from typing import Optional from flask import current_app as app from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.dao.exceptions import DAOException from superset.key_value.commands.exceptions import KeyValueGetFailedError logger = logging.getLogger(__name__) -class GetKeyValueCommand(BaseCommand): - def __init__(self, user: User, get_dao: Any, resource_id: int, key: str): +class GetKeyValueCommand(BaseCommand, ABC): + def __init__(self, user: User, resource_id: int, key: str): self._actor = user - self._get_dao = get_dao self._resource_id = resource_id self._key = key def run(self) -> Model: try: - value = self._get_dao.find_by_id(self._resource_id, self._key) - config = app.config["FILTERS_STATE_CACHE_CONFIG"] - if config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") == True: - self._get_dao.update(self._resource_id, self._key, value) - return value - except DAOException as ex: + config = app.config["FILTER_STATE_CACHE_CONFIG"] + refreshTimeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + return self.get(self._resource_id, self._key, refreshTimeout) + except SQLAlchemyError as ex: logger.exception(ex.exception) raise KeyValueGetFailedError() from ex def validate(self) -> None: pass + + @abstractmethod + def get(self, resource_id: int, key: str, refreshTimeout: bool) -> Optional[str]: + ... diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 943df52227eb..7bd4c05a84c6 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -15,41 +15,41 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Any, Dict +from abc import ABC, abstractmethod +from typing import Any, Dict, Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User +from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.dao.exceptions import DAOException from superset.key_value.commands.exceptions import KeyValueUpdateFailedError logger = logging.getLogger(__name__) -class UpdateKeyValueCommand(BaseCommand): +class UpdateKeyValueCommand(BaseCommand, ABC): def __init__( - self, - user: User, - get_dao: Any, - resource_id: int, - key: str, - data: Dict[str, Any], + self, user: User, resource_id: int, key: str, data: Dict[str, Any], ): self._actor = user - self._get_dao = get_dao self._resource_id = resource_id self._key = key self._properties = data.copy() def run(self) -> Model: try: - return self._get_dao.update( - self._resource_id, self._key, self._properties.get("value") - ) - except DAOException as ex: + value = self._properties.get("value") + if value: + return self.update(self._resource_id, self._key, value) + except SQLAlchemyError as ex: logger.exception(ex.exception) raise KeyValueUpdateFailedError() from ex + return False def validate(self) -> None: pass + + @abstractmethod + def update(self, resource_id: int, key: str, value: str) -> Optional[bool]: + ... diff --git a/superset/key_value/schemas.py b/superset/key_value/schemas.py index d3b5eef6e1dc..88e1d9c2109c 100644 --- a/superset/key_value/schemas.py +++ b/superset/key_value/schemas.py @@ -18,12 +18,8 @@ class KeyValuePostSchema(Schema): - value = fields.String( - required=True, allow_none=False, description="Any type of JSON supported value." - ) + value = fields.String(required=True, allow_none=False, description="A JSON value.") class KeyValuePutSchema(Schema): - value = fields.String( - required=True, allow_none=False, description="Any type of JSON supported value." - ) + value = fields.String(required=True, allow_none=False, description="A JSON value.") diff --git a/superset/key_value/utils.py b/superset/key_value/utils.py index c700fa8f223a..9ba2a8d03607 100644 --- a/superset/key_value/utils.py +++ b/superset/key_value/utils.py @@ -16,11 +16,8 @@ # under the License. from typing import Any +SEPARATOR = ";" + def cache_key(*args: Any) -> str: - result = "" - separator = "" - for arg in args: - result += separator + str(arg) - separator = ";" - return result + return SEPARATOR.join(str(arg) for arg in args) diff --git a/superset/utils/cache_manager.py b/superset/utils/cache_manager.py index dece40b06bae..5e6e96ebd253 100644 --- a/superset/utils/cache_manager.py +++ b/superset/utils/cache_manager.py @@ -25,7 +25,7 @@ def __init__(self) -> None: self._cache = Cache() self._data_cache = Cache() self._thumbnail_cache = Cache() - self._filters_state_cache = Cache() + self._filter_state_cache = Cache() def init_app(self, app: Flask) -> None: self._cache.init_app( @@ -49,11 +49,11 @@ def init_app(self, app: Flask) -> None: **app.config["THUMBNAIL_CACHE_CONFIG"], }, ) - self._filters_state_cache.init_app( + self._filter_state_cache.init_app( app, { "CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], - **app.config["FILTERS_STATE_CACHE_CONFIG"], + **app.config["FILTER_STATE_CACHE_CONFIG"], }, ) @@ -70,5 +70,5 @@ def thumbnail_cache(self) -> Cache: return self._thumbnail_cache @property - def filters_state_cache(self) -> Cache: - return self._filters_state_cache + def filter_state_cache(self) -> Cache: + return self._filter_state_cache diff --git a/tests/integration_tests/dashboards/filter_state/__init__.py b/tests/integration_tests/dashboards/filter_state/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/integration_tests/dashboards/filter_state/__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/tests/integration_tests/dashboards/filters_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py similarity index 78% rename from tests/integration_tests/dashboards/filters_state/api_tests.py rename to tests/integration_tests/dashboards/filter_state/api_tests.py index 561f842d8af9..0756956c59f7 100644 --- a/tests/integration_tests/dashboards/filters_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -34,7 +34,7 @@ def post(self): "value": value, } return self.client.post( - f"api/v1/dashboard/{dashboardId}/filters_state", json=payload + f"api/v1/dashboard/{dashboardId}/filter_state", json=payload ) def clearTable(self, session, model): @@ -65,7 +65,7 @@ def createDashboard(self, session): @pytest.fixture(autouse=True, scope="session") def beforeAll(self): with app.app_context() as ctx: - app.config["FILTERS_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} cache_manager.init_app(app) session: Session = ctx.app.appbuilder.get_session self.clearTable(session, Dashboard) @@ -78,6 +78,18 @@ def test_post(self): resp = self.post() assert resp.status_code == 201 + def test_put(self): + resp = self.post() + data = json.loads(resp.data.decode("utf-8")) + key = data.get("key") + payload = { + "value": "new value", + } + resp = self.client.put( + f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 200 + def test_get_not_found(self): resp = self.client.get("unknown-key") assert resp.status_code == 404 @@ -86,7 +98,14 @@ def test_get(self): resp = self.post() data = json.loads(resp.data.decode("utf-8")) key = data.get("key") - resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filters_state/{key}/") + resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") + + def test_delete(self): + resp = self.post() + data = json.loads(resp.data.decode("utf-8")) + key = data.get("key") + resp = self.client.delete(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") + assert resp.status_code == 200 From adc0409ca48b3713e7e427e4742d8b82ac49acde Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 29 Nov 2021 11:55:29 -0300 Subject: [PATCH 05/14] Fixes model description --- superset/key_value/schemas.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/superset/key_value/schemas.py b/superset/key_value/schemas.py index 88e1d9c2109c..3583a0cb769b 100644 --- a/superset/key_value/schemas.py +++ b/superset/key_value/schemas.py @@ -18,8 +18,12 @@ class KeyValuePostSchema(Schema): - value = fields.String(required=True, allow_none=False, description="A JSON value.") + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported text." + ) class KeyValuePutSchema(Schema): - value = fields.String(required=True, allow_none=False, description="A JSON value.") + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported text." + ) From b9004e0768b96b01cec1211dc424b45e4b1354cb Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 29 Nov 2021 15:47:46 -0300 Subject: [PATCH 06/14] Removes database model --- superset/dashboards/filter_state/api.py | 5 ----- superset/key_value/api.py | 10 ++------- superset/key_value/commands/create.py | 2 +- superset/key_value/commands/delete.py | 2 +- superset/key_value/commands/get.py | 2 +- superset/key_value/commands/update.py | 2 +- superset/models/key_value.py | 29 ------------------------- 7 files changed, 6 insertions(+), 46 deletions(-) delete mode 100644 superset/models/key_value.py diff --git a/superset/dashboards/filter_state/api.py b/superset/dashboards/filter_state/api.py index 6b0e78f93ee9..afc7616d9cf0 100644 --- a/superset/dashboards/filter_state/api.py +++ b/superset/dashboards/filter_state/api.py @@ -26,7 +26,6 @@ from superset.dashboards.filter_state.commands.update import UpdateFilterStateCommand from superset.extensions import event_logger from superset.key_value.api import KeyValueRestApi -from superset.views.base_api import statsd_metrics logger = logging.getLogger(__name__) @@ -51,7 +50,6 @@ def get_delete_command(self) -> Type[DeleteFilterStateCommand]: @expose("//filter_state", methods=["POST"]) @protect() @safe - @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", log_to_statsd=False, @@ -99,7 +97,6 @@ def post(self, pk: int) -> Response: @expose("//filter_state//", methods=["PUT"]) @protect() @safe - @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", log_to_statsd=False, @@ -153,7 +150,6 @@ def put(self, pk: int, key: str) -> Response: @expose("//filter_state//", methods=["GET"]) @protect() @safe - @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", log_to_statsd=False, @@ -200,7 +196,6 @@ def get(self, pk: int, key: str) -> Response: @expose("//filter_state//", methods=["DELETE"]) @protect() @safe - @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", log_to_statsd=False, diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 3bbdfba541f6..c90a77024f56 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -19,23 +19,17 @@ from typing import Any from flask import g, request, Response +from flask_appbuilder.api import BaseApi from flask_appbuilder.models.sqla.interface import SQLAInterface from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.exceptions import InvalidPayloadFormatError -from superset.key_value.commands.create import CreateKeyValueCommand -from superset.key_value.commands.delete import DeleteKeyValueCommand -from superset.key_value.commands.get import GetKeyValueCommand -from superset.key_value.commands.update import UpdateKeyValueCommand from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema -from superset.models.key_value import KeyValueEntry -from superset.views.base_api import BaseSupersetModelRestApi logger = logging.getLogger(__name__) -class KeyValueRestApi(BaseSupersetModelRestApi, ABC): - datamodel = SQLAInterface(KeyValueEntry) +class KeyValueRestApi(BaseApi, ABC): add_model_schema = KeyValuePostSchema() edit_model_schema = KeyValuePutSchema() method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index c73dd55ae1ba..6ea923b68c2d 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -45,7 +45,7 @@ def run(self) -> Model: self.create(self._resource_id, key, value) return key except SQLAlchemyError as ex: - logger.exception(ex.exception) + logger.exception("Error running create command") raise KeyValueCreateFailedError() from ex return False diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py index 7784c29f8f77..9990c4450c48 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -38,7 +38,7 @@ def run(self) -> Model: try: return self.delete(self._resource_id, self._key) except SQLAlchemyError as ex: - logger.exception(ex.exception) + logger.exception("Error running delete command") raise KeyValueDeleteFailedError() from ex def validate(self) -> None: diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py index f2d1629a63fa..32991fa96f54 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -41,7 +41,7 @@ def run(self) -> Model: refreshTimeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") return self.get(self._resource_id, self._key, refreshTimeout) except SQLAlchemyError as ex: - logger.exception(ex.exception) + logger.exception("Error running get command") raise KeyValueGetFailedError() from ex def validate(self) -> None: diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 7bd4c05a84c6..4ebf880c561f 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -43,7 +43,7 @@ def run(self) -> Model: if value: return self.update(self._resource_id, self._key, value) except SQLAlchemyError as ex: - logger.exception(ex.exception) + logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex return False diff --git a/superset/models/key_value.py b/superset/models/key_value.py deleted file mode 100644 index ba1f6b31b3d6..000000000000 --- a/superset/models/key_value.py +++ /dev/null @@ -1,29 +0,0 @@ -# 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_appbuilder import Model -from sqlalchemy import Column, String, Text - -from superset.models.helpers import AuditMixinNullable - - -class KeyValueEntry(Model, AuditMixinNullable): - - """Key value entry""" - - __tablename__ = "key_value" - key = Column(String(64), primary_key=True) - value = Column(Text, nullable=False) From db29e6fd67adb235f33ca3fb811bf4f88e5561f5 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 29 Nov 2021 16:21:26 -0300 Subject: [PATCH 07/14] Adds open api specs --- superset/key_value/api.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index c90a77024f56..146889ce4a38 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -18,9 +18,9 @@ from abc import ABC, abstractmethod from typing import Any +from apispec import APISpec from flask import g, request, Response from flask_appbuilder.api import BaseApi -from flask_appbuilder.models.sqla.interface import SQLAInterface from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.exceptions import InvalidPayloadFormatError @@ -40,10 +40,15 @@ class KeyValueRestApi(BaseApi, ABC): RouteMethod.DELETE, } allow_browser_login = True - openapi_spec_component_schemas = ( - KeyValuePostSchema, - KeyValuePutSchema, - ) + + def add_apispec_components(self, api_spec: APISpec) -> None: + api_spec.components.schema( + KeyValuePostSchema.__name__, schema=KeyValuePostSchema, + ) + api_spec.components.schema( + KeyValuePutSchema.__name__, schema=KeyValuePutSchema, + ) + super().add_apispec_components(api_spec) def post(self, pk: int) -> Response: if not request.is_json: From 0f44be592d91e2b98ac30c26998bc37eeaa23e98 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 29 Nov 2021 17:14:49 -0300 Subject: [PATCH 08/14] Simplifies the commands --- superset/key_value/api.py | 4 ++-- superset/key_value/commands/create.py | 11 ++++------- superset/key_value/commands/update.py | 9 +++------ 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 146889ce4a38..d16cc0a84cec 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -54,14 +54,14 @@ def post(self, pk: int) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.add_model_schema.load(request.json) - key = self.get_create_command()(g.user, pk, item).run() + key = self.get_create_command()(g.user, pk, item["value"]).run() return self.response(201, key=key) def put(self, pk: int, key: str) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") item = self.edit_model_schema.load(request.json) - result = self.get_update_command()(g.user, pk, key, item).run() + result = self.get_update_command()(g.user, pk, key, item["value"]).run() if not result: return self.response_404() return self.response(200, message="Value updated successfully.",) diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 6ea923b68c2d..c878abd4b8fa 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -31,23 +31,20 @@ class CreateKeyValueCommand(BaseCommand, ABC): def __init__( - self, user: User, resource_id: int, data: Dict[str, Any], + self, user: User, resource_id: int, value: str, ): self._actor = user self._resource_id = resource_id - self._properties = data.copy() + self._value = value def run(self) -> Model: try: key = token_urlsafe(48) - value = self._properties.get("value") - if value: - self.create(self._resource_id, key, value) - return key + self.create(self._resource_id, key, self._value) + return key except SQLAlchemyError as ex: logger.exception("Error running create command") raise KeyValueCreateFailedError() from ex - return False def validate(self) -> None: pass diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index 4ebf880c561f..ac110877bef6 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -30,22 +30,19 @@ class UpdateKeyValueCommand(BaseCommand, ABC): def __init__( - self, user: User, resource_id: int, key: str, data: Dict[str, Any], + self, user: User, resource_id: int, key: str, value: str, ): self._actor = user self._resource_id = resource_id self._key = key - self._properties = data.copy() + self._value = value def run(self) -> Model: try: - value = self._properties.get("value") - if value: - return self.update(self._resource_id, self._key, value) + return self.update(self._resource_id, self._key, self._value) except SQLAlchemyError as ex: logger.exception("Error running update command") raise KeyValueUpdateFailedError() from ex - return False def validate(self) -> None: pass From 7c7617240a7c2826e76423b280e3b326d077eca1 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 30 Nov 2021 09:37:52 -0300 Subject: [PATCH 09/14] Adds more tests --- superset/key_value/api.py | 50 ++++++-- .../dashboards/filter_state/api_tests.py | 111 ++++++++++++------ 2 files changed, 113 insertions(+), 48 deletions(-) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index d16cc0a84cec..177af3429d79 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -23,6 +23,10 @@ from flask_appbuilder.api import BaseApi from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.dashboards.commands.exceptions import ( + DashboardAccessDeniedError, + DashboardNotFoundError, +) from superset.exceptions import InvalidPayloadFormatError from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema @@ -53,30 +57,50 @@ def add_apispec_components(self, api_spec: APISpec) -> None: def post(self, pk: int) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") - item = self.add_model_schema.load(request.json) - key = self.get_create_command()(g.user, pk, item["value"]).run() - return self.response(201, key=key) + try: + item = self.add_model_schema.load(request.json) + key = self.get_create_command()(g.user, pk, item["value"]).run() + return self.response(201, key=key) + except DashboardAccessDeniedError: + return self.response_403() + except DashboardNotFoundError: + return self.response_404() def put(self, pk: int, key: str) -> Response: if not request.is_json: raise InvalidPayloadFormatError("Request is not JSON") - item = self.edit_model_schema.load(request.json) - result = self.get_update_command()(g.user, pk, key, item["value"]).run() - if not result: + try: + item = self.edit_model_schema.load(request.json) + result = self.get_update_command()(g.user, pk, key, item["value"]).run() + if not result: + return self.response_404() + return self.response(200, message="Value updated successfully.") + except DashboardAccessDeniedError: + return self.response_403() + except DashboardNotFoundError: return self.response_404() - return self.response(200, message="Value updated successfully.",) def get(self, pk: int, key: str) -> Response: - value = self.get_get_command()(g.user, pk, key).run() - if not value: + try: + value = self.get_get_command()(g.user, pk, key).run() + if not value: + return self.response_404() + return self.response(200, value=value) + except DashboardAccessDeniedError: + return self.response_403() + except DashboardNotFoundError: return self.response_404() - return self.response(200, value=value) def delete(self, pk: int, key: str) -> Response: - result = self.get_delete_command()(g.user, pk, key).run() - if not result: + try: + result = self.get_delete_command()(g.user, pk, key).run() + if not result: + return self.response_404() + return self.response(200, message="Deleted successfully") + except DashboardAccessDeniedError: + return self.response_403() + except DashboardNotFoundError: return self.response_404() - return self.response(200, message="Deleted successfully") @abstractmethod def get_create_command(self) -> Any: diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 0756956c59f7..c394d77c80aa 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -23,27 +23,38 @@ from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.test_app import app from sqlalchemy.orm import Session +from unittest.mock import patch +from superset.dashboards.commands.exceptions import DashboardAccessDeniedError +from superset.key_value.utils import cache_key -dashboardId = 1 +dashboardId = 985374 +key = "test-key" +deleteKey = "delete-key" value = "test" class FilterStateTests(SupersetTestCase): - def post(self): - payload = { - "value": value, - } - return self.client.post( - f"api/v1/dashboard/{dashboardId}/filter_state", json=payload - ) + @pytest.fixture(scope="session", autouse=True) + def before_all(self): + with app.app_context() as ctx: + # init cache + app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + cache_manager.init_app(app) + cache_manager.filter_state_cache.set(cache_key(dashboardId, key), value) + cache_manager.filter_state_cache.set( + cache_key(dashboardId, deleteKey), value + ) - def clearTable(self, session, model): - rows = session.query(model).all() - for row in rows: - session.delete(row) - session.commit() + # create dashboard + session: Session = ctx.app.appbuilder.get_session + dashboard = self.create_dashboard(session) + yield dashboard + + # delete dashboard + session.delete(dashboard) + session.commit() - def createDashboard(self, session): + def create_dashboard(self, session): admin = self.get_user("admin") user = session.query(security_manager.user_model).get(admin.id) dashboard = Dashboard( @@ -61,27 +72,32 @@ def createDashboard(self, session): ) session.add(dashboard) session.commit() - - @pytest.fixture(autouse=True, scope="session") - def beforeAll(self): - with app.app_context() as ctx: - app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} - cache_manager.init_app(app) - session: Session = ctx.app.appbuilder.get_session - self.clearTable(session, Dashboard) - self.createDashboard(session) + return dashboard def setUp(self): self.login(username="admin") def test_post(self): - resp = self.post() + payload = { + "value": value, + } + resp = self.client.post( + f"api/v1/dashboard/{dashboardId}/filter_state", json=payload + ) assert resp.status_code == 201 + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_post_access_denied(self, mock_raise_for_dashboard_access): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + payload = { + "value": value, + } + resp = self.client.post( + f"api/v1/dashboard/{dashboardId}/filter_state", json=payload + ) + assert resp.status_code == 403 + def test_put(self): - resp = self.post() - data = json.loads(resp.data.decode("utf-8")) - key = data.get("key") payload = { "value": "new value", } @@ -90,22 +106,47 @@ def test_put(self): ) assert resp.status_code == 200 - def test_get_not_found(self): + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_put_access_denied(self, mock_raise_for_dashboard_access): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + payload = { + "value": "new value", + } + resp = self.client.put( + f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 403 + + def test_get_key_not_found(self): resp = self.client.get("unknown-key") assert resp.status_code == 404 + def test_get_dashboard_not_found(self): + resp = self.client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") + assert resp.status_code == 404 + def test_get(self): - resp = self.post() - data = json.loads(resp.data.decode("utf-8")) - key = data.get("key") resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_get_access_denied(self, mock_raise_for_dashboard_access): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") + assert resp.status_code == 403 + def test_delete(self): - resp = self.post() - data = json.loads(resp.data.decode("utf-8")) - key = data.get("key") - resp = self.client.delete(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") + resp = self.client.delete( + f"api/v1/dashboard/{dashboardId}/filter_state/{deleteKey}/" + ) assert resp.status_code == 200 + + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") + def test_delete_access_denied(self, mock_raise_for_dashboard_access): + mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() + resp = self.client.delete( + f"api/v1/dashboard/{dashboardId}/filter_state/{deleteKey}/" + ) + assert resp.status_code == 403 From 4af801a358f734e7728ffeb4b5dabc477763ce19 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 30 Nov 2021 14:31:36 -0300 Subject: [PATCH 10/14] Validates the value content and submits the correct http status code --- superset/key_value/api.py | 5 +++++ .../dashboards/filter_state/api_tests.py | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index 177af3429d79..ae6f19e8a677 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -17,6 +17,7 @@ import logging from abc import ABC, abstractmethod from typing import Any +from marshmallow import ValidationError from apispec import APISpec from flask import g, request, Response @@ -61,6 +62,8 @@ def post(self, pk: int) -> Response: item = self.add_model_schema.load(request.json) key = self.get_create_command()(g.user, pk, item["value"]).run() return self.response(201, key=key) + except ValidationError as error: + return self.response_400(message=error.messages) except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: @@ -75,6 +78,8 @@ def put(self, pk: int, key: str) -> Response: if not result: return self.response_404() return self.response(200, message="Value updated successfully.") + except ValidationError as error: + return self.response_400(message=error.messages) except DashboardAccessDeniedError: return self.response_403() except DashboardNotFoundError: diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index c394d77c80aa..5ebd2c080313 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -86,6 +86,15 @@ def test_post(self): ) assert resp.status_code == 201 + def test_post_bad_request(self): + payload = { + "value": 1234, + } + resp = self.client.put( + f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 400 + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") def test_post_access_denied(self, mock_raise_for_dashboard_access): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() @@ -106,6 +115,15 @@ def test_put(self): ) assert resp.status_code == 200 + def test_put_bad_request(self): + payload = { + "value": 1234, + } + resp = self.client.put( + f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + ) + assert resp.status_code == 400 + @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") def test_put_access_denied(self, mock_raise_for_dashboard_access): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() From 458d468064d990f69b7d03bbc8833847552c7839 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 30 Nov 2021 14:38:08 -0300 Subject: [PATCH 11/14] Fixes import order --- superset/config.py | 8 +++++++- superset/key_value/api.py | 2 +- tests/integration_tests/databases/commands_tests.py | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/superset/config.py b/superset/config.py index 154c1ff72bf9..aabda42d36ba 100644 --- a/superset/config.py +++ b/superset/config.py @@ -562,7 +562,13 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} # Cache for filters state -FILTER_STATE_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"} +FILTER_STATE_CACHE_CONFIG: CacheConfig = { + "CACHE_TYPE": "filesystem", + "CACHE_DIR": os.path.join(DATA_DIR, "cache"), + "CACHE_DEFAULT_TIMEOUT": int(timedelta(days=90).total_seconds()), + "CACHE_THRESHOLD": 0, + "REFRESH_TIMEOUT_ON_RETRIEVAL": True, +} # store cache keys by datasource UID (via CacheKey) for custom processing/invalidation STORE_CACHE_KEYS_IN_METADATA_DB = False diff --git a/superset/key_value/api.py b/superset/key_value/api.py index ae6f19e8a677..d67852a29bbe 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -17,11 +17,11 @@ import logging from abc import ABC, abstractmethod from typing import Any -from marshmallow import ValidationError from apispec import APISpec from flask import g, request, Response from flask_appbuilder.api import BaseApi +from marshmallow import ValidationError from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.dashboards.commands.exceptions import ( diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 0ec96abf9b44..71ee11b96215 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -313,6 +313,7 @@ def test_export_database_command_key_order(self, mock_g): class TestImportDatabasesCommand(SupersetTestCase): + @pytest.mark.skip(reason="This test is being affected by unrelated tests") def test_import_v1_database(self): """Test that a database can be imported""" contents = { From 01249af7c2cb958f0f96930430a52eb3b069bade Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 30 Nov 2021 15:24:14 -0300 Subject: [PATCH 12/14] Skips flakky test --- tests/integration_tests/databases/commands_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 71ee11b96215..97b29477ebeb 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -313,7 +313,6 @@ def test_export_database_command_key_order(self, mock_g): class TestImportDatabasesCommand(SupersetTestCase): - @pytest.mark.skip(reason="This test is being affected by unrelated tests") def test_import_v1_database(self): """Test that a database can be imported""" contents = { @@ -415,6 +414,7 @@ def test_import_v1_database_multiple(self): db.session.delete(database) db.session.commit() + @pytest.mark.skip(reason="This test is being affected by unrelated tests") def test_import_v1_database_with_dataset(self): """Test that a database can be imported with datasets""" contents = { From ff22b330332f0cf97a8bca809fb5e81fc05c5919 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 30 Nov 2021 17:58:36 -0300 Subject: [PATCH 13/14] Fixes tests --- superset/key_value/commands/create.py | 2 +- superset/key_value/commands/update.py | 2 +- .../dashboards/filter_state/api_tests.py | 149 ++++++++---------- .../databases/commands_tests.py | 1 - 4 files changed, 70 insertions(+), 84 deletions(-) diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index c878abd4b8fa..361a3ad22280 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -17,7 +17,7 @@ import logging from abc import ABC, abstractmethod from secrets import token_urlsafe -from typing import Any, Dict, Optional +from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User diff --git a/superset/key_value/commands/update.py b/superset/key_value/commands/update.py index ac110877bef6..dac91daf3103 100644 --- a/superset/key_value/commands/update.py +++ b/superset/key_value/commands/update.py @@ -16,7 +16,7 @@ # under the License. import logging from abc import ABC, abstractmethod -from typing import Any, Dict, Optional +from typing import Optional from flask_appbuilder.models.sqla import Model from flask_appbuilder.security.sqla.models import User diff --git a/tests/integration_tests/dashboards/filter_state/api_tests.py b/tests/integration_tests/dashboards/filter_state/api_tests.py index 5ebd2c080313..1c2beef66162 100644 --- a/tests/integration_tests/dashboards/filter_state/api_tests.py +++ b/tests/integration_tests/dashboards/filter_state/api_tests.py @@ -14,157 +14,144 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -# isort:skip_file -import pytest import json -from superset import security_manager, app -from superset.models.dashboard import Dashboard -from superset.extensions import cache_manager -from tests.integration_tests.base_tests import SupersetTestCase -from tests.integration_tests.test_app import app -from sqlalchemy.orm import Session +from typing import Any from unittest.mock import patch + +import pytest +from flask.testing import FlaskClient +from sqlalchemy.orm import Session + +from superset import app from superset.dashboards.commands.exceptions import DashboardAccessDeniedError +from superset.extensions import cache_manager from superset.key_value.utils import cache_key +from superset.models.dashboard import Dashboard +from tests.integration_tests.fixtures.world_bank_dashboard import ( + load_world_bank_dashboard_with_slices, +) +from tests.integration_tests.test_app import app dashboardId = 985374 key = "test-key" -deleteKey = "delete-key" value = "test" -class FilterStateTests(SupersetTestCase): - @pytest.fixture(scope="session", autouse=True) - def before_all(self): +class FilterStateTests: + @pytest.fixture + def client(self): + with app.test_client() as client: + with app.app_context(): + yield client + + @pytest.fixture + def dashboard_id(self, load_world_bank_dashboard_with_slices) -> int: with app.app_context() as ctx: - # init cache - app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} - cache_manager.init_app(app) - cache_manager.filter_state_cache.set(cache_key(dashboardId, key), value) - cache_manager.filter_state_cache.set( - cache_key(dashboardId, deleteKey), value - ) - - # create dashboard session: Session = ctx.app.appbuilder.get_session - dashboard = self.create_dashboard(session) - yield dashboard - - # delete dashboard - session.delete(dashboard) - session.commit() - - def create_dashboard(self, session): - admin = self.get_user("admin") - user = session.query(security_manager.user_model).get(admin.id) - dashboard = Dashboard( - id=dashboardId, - dashboard_title="test", - slug=None, - owners=[user], - roles=[], - position_json="", - css="", - json_metadata="", - slices=[], - published=False, - created_by=None, - ) - session.add(dashboard) - session.commit() - return dashboard + dashboard = session.query(Dashboard).filter_by(slug="world_health").one() + return dashboard.id + + @pytest.fixture + def cache(self, dashboard_id): + app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + cache_manager.init_app(app) + cache_manager.filter_state_cache.set(cache_key(dashboard_id, key), value) def setUp(self): self.login(username="admin") - def test_post(self): + def test_post(self, client, dashboard_id: int): payload = { "value": value, } - resp = self.client.post( - f"api/v1/dashboard/{dashboardId}/filter_state", json=payload + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload ) assert resp.status_code == 201 - def test_post_bad_request(self): + def test_post_bad_request(self, client, dashboard_id: int): payload = { "value": 1234, } - resp = self.client.put( - f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload ) assert resp.status_code == 400 @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") - def test_post_access_denied(self, mock_raise_for_dashboard_access): + def test_post_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() payload = { "value": value, } - resp = self.client.post( - f"api/v1/dashboard/{dashboardId}/filter_state", json=payload + resp = client.post( + f"api/v1/dashboard/{dashboard_id}/filter_state", json=payload ) assert resp.status_code == 403 - def test_put(self): + def test_put(self, client, dashboard_id: int): payload = { "value": "new value", } - resp = self.client.put( - f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload ) assert resp.status_code == 200 - def test_put_bad_request(self): + def test_put_bad_request(self, client, dashboard_id: int): payload = { "value": 1234, } - resp = self.client.put( - f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload ) assert resp.status_code == 400 @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") - def test_put_access_denied(self, mock_raise_for_dashboard_access): + def test_put_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() payload = { "value": "new value", } - resp = self.client.put( - f"api/v1/dashboard/{dashboardId}/filter_state/{key}/", json=payload + resp = client.put( + f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/", json=payload ) assert resp.status_code == 403 - def test_get_key_not_found(self): - resp = self.client.get("unknown-key") + def test_get_key_not_found(self, client): + resp = client.get("unknown-key") assert resp.status_code == 404 - def test_get_dashboard_not_found(self): - resp = self.client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") + def test_get_dashboard_not_found(self, client): + resp = client.get(f"api/v1/dashboard/{-1}/filter_state/{key}/") assert resp.status_code == 404 - def test_get(self): - resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") + def test_get(self, client, dashboard_id: int): + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") - def test_get_access_denied(self, mock_raise_for_dashboard_access): + def test_get_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id + ): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = self.client.get(f"api/v1/dashboard/{dashboardId}/filter_state/{key}/") + resp = client.get(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") assert resp.status_code == 403 - def test_delete(self): - resp = self.client.delete( - f"api/v1/dashboard/{dashboardId}/filter_state/{deleteKey}/" - ) + def test_delete(self, client, dashboard_id: int): + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") assert resp.status_code == 200 @patch("superset.security.SupersetSecurityManager.raise_for_dashboard_access") - def test_delete_access_denied(self, mock_raise_for_dashboard_access): + def test_delete_access_denied( + self, client, mock_raise_for_dashboard_access, dashboard_id: int + ): mock_raise_for_dashboard_access.side_effect = DashboardAccessDeniedError() - resp = self.client.delete( - f"api/v1/dashboard/{dashboardId}/filter_state/{deleteKey}/" - ) + resp = client.delete(f"api/v1/dashboard/{dashboard_id}/filter_state/{key}/") assert resp.status_code == 403 diff --git a/tests/integration_tests/databases/commands_tests.py b/tests/integration_tests/databases/commands_tests.py index 97b29477ebeb..0ec96abf9b44 100644 --- a/tests/integration_tests/databases/commands_tests.py +++ b/tests/integration_tests/databases/commands_tests.py @@ -414,7 +414,6 @@ def test_import_v1_database_multiple(self): db.session.delete(database) db.session.commit() - @pytest.mark.skip(reason="This test is being affected by unrelated tests") def test_import_v1_database_with_dataset(self): """Test that a database can be imported with datasets""" contents = { From b0e5a4282ac213bb0f69d8ed470f75b7163ce880 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 1 Dec 2021 07:46:08 -0300 Subject: [PATCH 14/14] Updates UPDATING.md --- UPDATING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/UPDATING.md b/UPDATING.md index cf52611a0ada..56dd8aa89ef2 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -38,11 +38,13 @@ assists people when migrating to a new version. - [17539](https://github.com/apache/superset/pull/17539): all Superset CLI commands (init, load_examples and etc) require setting the FLASK_APP environment variable (which is set by default when .flaskenv is loaded) + ### Deprecations ### Other - [16809](https://github.com/apache/incubator-superset/pull/16809): When building the superset frontend assets manually, you should now use Node 16 (previously Node 14 was required/recommended). Node 14 will most likely still work for at least some time, but is no longer actively tested for on CI. +- [17536](https://github.com/apache/superset/pull/17536): introduced a key-value endpoint to store dashboard filter state. This endpoint is backed by Flask-Caching and the default configuration assumes that the values will be stored in the file system. If you are already using another cache backend like Redis or Memchached, you'll probably want to change this setting in `superset_config.py`. The key is `FILTER_STATE_CACHE_CONFIG` and the available settings can be found in Flask-Caching [docs](https://flask-caching.readthedocs.io/en/latest/). ## 1.3.0