From bea7d4a6a5edfb919f235b54be8a9be729d9e937 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 24 Jan 2022 13:28:09 -0300 Subject: [PATCH 1/5] refactor: Moves the Explore form_data endpoint --- superset/charts/form_data/commands/create.py | 35 ---- superset/charts/form_data/commands/delete.py | 37 ---- superset/charts/form_data/commands/get.py | 44 ---- superset/charts/form_data/commands/update.py | 40 ---- superset/config.py | 2 +- .../{charts/form_data => explore}/__init__.py | 0 .../form_data}/__init__.py | 0 superset/{charts => explore}/form_data/api.py | 195 +++++++++++------- .../explore/form_data/commands}/__init__.py | 0 superset/explore/form_data/commands/create.py | 59 ++++++ superset/explore/form_data/commands/delete.py | 56 +++++ superset/explore/form_data/commands/entry.py | 24 +++ superset/explore/form_data/commands/get.py | 57 +++++ .../explore/form_data/commands/parameters.py | 29 +++ superset/explore/form_data/commands/update.py | 68 ++++++ superset/explore/form_data/schemas.py | 37 ++++ .../{charts => explore}/form_data/utils.py | 25 +-- superset/initialization/__init__.py | 4 +- superset/utils/cache_manager.py | 10 +- tests/integration_tests/explore/__init__.py | 16 ++ .../explore/form_data/__init__.py | 16 ++ .../form_data/api_tests.py | 128 +++++------- tests/unit_tests/explore/__init__.py | 16 ++ .../unit_tests/explore/form_data/__init__.py | 16 ++ .../form_data/utils_test.py | 73 ++----- 25 files changed, 602 insertions(+), 385 deletions(-) delete mode 100644 superset/charts/form_data/commands/create.py delete mode 100644 superset/charts/form_data/commands/delete.py delete mode 100644 superset/charts/form_data/commands/get.py delete mode 100644 superset/charts/form_data/commands/update.py rename superset/{charts/form_data => explore}/__init__.py (100%) rename superset/{charts/form_data/commands => explore/form_data}/__init__.py (100%) rename superset/{charts => explore}/form_data/api.py (52%) rename {tests/integration_tests/charts/form_data => superset/explore/form_data/commands}/__init__.py (100%) create mode 100644 superset/explore/form_data/commands/create.py create mode 100644 superset/explore/form_data/commands/delete.py create mode 100644 superset/explore/form_data/commands/entry.py create mode 100644 superset/explore/form_data/commands/get.py create mode 100644 superset/explore/form_data/commands/parameters.py create mode 100644 superset/explore/form_data/commands/update.py create mode 100644 superset/explore/form_data/schemas.py rename superset/{charts => explore}/form_data/utils.py (72%) create mode 100644 tests/integration_tests/explore/__init__.py create mode 100644 tests/integration_tests/explore/form_data/__init__.py rename tests/integration_tests/{charts => explore}/form_data/api_tests.py (59%) create mode 100644 tests/unit_tests/explore/__init__.py create mode 100644 tests/unit_tests/explore/form_data/__init__.py rename tests/unit_tests/{charts => explore}/form_data/utils_test.py (68%) diff --git a/superset/charts/form_data/commands/create.py b/superset/charts/form_data/commands/create.py deleted file mode 100644 index d7c2ad413a4f..000000000000 --- a/superset/charts/form_data/commands/create.py +++ /dev/null @@ -1,35 +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 superset.charts.form_data.utils import check_access, get_dataset_id -from superset.extensions import cache_manager -from superset.key_value.commands.create import CreateKeyValueCommand -from superset.key_value.commands.entry import Entry -from superset.key_value.commands.parameters import CommandParameters -from superset.key_value.utils import cache_key - - -class CreateFormDataCommand(CreateKeyValueCommand): - def create(self, cmd_params: CommandParameters) -> bool: - check_access(cmd_params) - resource_id = cmd_params.resource_id - actor = cmd_params.actor - key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) - value = cmd_params.value - if value: - entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.chart_form_data_cache.set(key, entry) - return False diff --git a/superset/charts/form_data/commands/delete.py b/superset/charts/form_data/commands/delete.py deleted file mode 100644 index be6c33a00508..000000000000 --- a/superset/charts/form_data/commands/delete.py +++ /dev/null @@ -1,37 +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 superset.charts.form_data.utils import check_access, get_dataset_id -from superset.extensions import cache_manager -from superset.key_value.commands.delete import DeleteKeyValueCommand -from superset.key_value.commands.entry import Entry -from superset.key_value.commands.exceptions import KeyValueAccessDeniedError -from superset.key_value.commands.parameters import CommandParameters -from superset.key_value.utils import cache_key - - -class DeleteFormDataCommand(DeleteKeyValueCommand): - def delete(self, cmd_params: CommandParameters) -> bool: - check_access(cmd_params) - resource_id = cmd_params.resource_id - actor = cmd_params.actor - key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) - entry: Entry = cache_manager.chart_form_data_cache.get(key) - if entry: - if entry["owner"] != actor.get_user_id(): - raise KeyValueAccessDeniedError() - return cache_manager.chart_form_data_cache.delete(key) - return True diff --git a/superset/charts/form_data/commands/get.py b/superset/charts/form_data/commands/get.py deleted file mode 100644 index f23acfc2d186..000000000000 --- a/superset/charts/form_data/commands/get.py +++ /dev/null @@ -1,44 +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 typing import Optional - -from flask import current_app as app - -from superset.charts.form_data.utils import check_access, get_dataset_id -from superset.extensions import cache_manager -from superset.key_value.commands.entry import Entry -from superset.key_value.commands.get import GetKeyValueCommand -from superset.key_value.commands.parameters import CommandParameters -from superset.key_value.utils import cache_key - - -class GetFormDataCommand(GetKeyValueCommand): - def __init__(self, cmd_params: CommandParameters) -> None: - super().__init__(cmd_params) - config = app.config["CHART_FORM_DATA_CACHE_CONFIG"] - self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") - - def get(self, cmd_params: CommandParameters) -> Optional[str]: - check_access(cmd_params) - resource_id = cmd_params.resource_id - key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) - entry: Entry = cache_manager.chart_form_data_cache.get(key) - if entry: - if self._refresh_timeout: - cache_manager.chart_form_data_cache.set(key, entry) - return entry["value"] - return None diff --git a/superset/charts/form_data/commands/update.py b/superset/charts/form_data/commands/update.py deleted file mode 100644 index cab988da8713..000000000000 --- a/superset/charts/form_data/commands/update.py +++ /dev/null @@ -1,40 +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 superset.charts.form_data.utils import check_access, get_dataset_id -from superset.extensions import cache_manager -from superset.key_value.commands.entry import Entry -from superset.key_value.commands.exceptions import KeyValueAccessDeniedError -from superset.key_value.commands.parameters import CommandParameters -from superset.key_value.commands.update import UpdateKeyValueCommand -from superset.key_value.utils import cache_key - - -class UpdateFormDataCommand(UpdateKeyValueCommand): - def update(self, cmd_params: CommandParameters) -> bool: - check_access(cmd_params) - resource_id = cmd_params.resource_id - actor = cmd_params.actor - key = cache_key(resource_id or get_dataset_id(cmd_params), cmd_params.key) - value = cmd_params.value - entry: Entry = cache_manager.chart_form_data_cache.get(key) - if entry and value: - user_id = actor.get_user_id() - if entry["owner"] != user_id: - raise KeyValueAccessDeniedError() - new_entry: Entry = {"owner": actor.get_user_id(), "value": value} - return cache_manager.chart_form_data_cache.set(key, new_entry) - return False diff --git a/superset/config.py b/superset/config.py index b9a98c4ccfba..d137a77e2265 100644 --- a/superset/config.py +++ b/superset/config.py @@ -585,7 +585,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: } # Cache for chart form data -CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = { +EXPLORE_FORM_DATA_CACHE_CONFIG: CacheConfig = { "CACHE_TYPE": "FileSystemCache", "CACHE_DIR": os.path.join(DATA_DIR, "cache"), "CACHE_DEFAULT_TIMEOUT": int(timedelta(days=7).total_seconds()), diff --git a/superset/charts/form_data/__init__.py b/superset/explore/__init__.py similarity index 100% rename from superset/charts/form_data/__init__.py rename to superset/explore/__init__.py diff --git a/superset/charts/form_data/commands/__init__.py b/superset/explore/form_data/__init__.py similarity index 100% rename from superset/charts/form_data/commands/__init__.py rename to superset/explore/form_data/__init__.py diff --git a/superset/charts/form_data/api.py b/superset/explore/form_data/api.py similarity index 52% rename from superset/charts/form_data/api.py rename to superset/explore/form_data/api.py index bc908b25d6b8..5a325dc4a90b 100644 --- a/superset/charts/form_data/api.py +++ b/superset/explore/form_data/api.py @@ -15,67 +15,68 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Type +from abc import ABC -from flask import Response -from flask_appbuilder.api import expose, protect, safe +from flask import g, request, Response +from flask_appbuilder.api import BaseApi, expose, protect, safe +from marshmallow import ValidationError -from superset.charts.form_data.commands.create import CreateFormDataCommand -from superset.charts.form_data.commands.delete import DeleteFormDataCommand -from superset.charts.form_data.commands.get import GetFormDataCommand -from superset.charts.form_data.commands.update import UpdateFormDataCommand +from superset.charts.commands.exceptions import ( + ChartAccessDeniedError, + ChartNotFoundError, +) +from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.datasets.commands.exceptions import ( + DatasetAccessDeniedError, + DatasetNotFoundError, +) +from superset.exceptions import InvalidPayloadFormatError +from superset.explore.form_data.commands.create import CreateFormDataCommand +from superset.explore.form_data.commands.delete import DeleteFormDataCommand +from superset.explore.form_data.commands.get import GetFormDataCommand +from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.commands.update import UpdateFormDataCommand +from superset.explore.form_data.schemas import FormDataPostSchema, FormDataPutSchema from superset.extensions import event_logger -from superset.key_value.api import KeyValueRestApi +from superset.key_value.commands.exceptions import KeyValueAccessDeniedError logger = logging.getLogger(__name__) -class ChartFormDataRestApi(KeyValueRestApi): - class_permission_name = "ChartFormDataRestApi" - resource_name = "chart" - openapi_spec_tag = "Chart Form Data" +class ExploreFormDataRestApi(BaseApi, ABC): + add_model_schema = FormDataPostSchema() + edit_model_schema = FormDataPutSchema() + method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + include_route_methods = { + RouteMethod.POST, + RouteMethod.PUT, + RouteMethod.GET, + RouteMethod.DELETE, + } + allow_browser_login = True + class_permission_name = "ExploreFormDataRestApi" + resource_name = "explore" + openapi_spec_tag = "Explore Form Data" - def get_create_command(self) -> Type[CreateFormDataCommand]: - return CreateFormDataCommand - - def get_update_command(self) -> Type[UpdateFormDataCommand]: - return UpdateFormDataCommand - - def get_get_command(self) -> Type[GetFormDataCommand]: - return GetFormDataCommand - - def get_delete_command(self) -> Type[DeleteFormDataCommand]: - return DeleteFormDataCommand - - @expose("//form_data", methods=["POST"]) + @expose("/form_data", methods=["POST"]) @protect() @safe @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.post", log_to_statsd=False, ) - def post(self, pk: int) -> Response: + def post(self) -> Response: """Stores a new value. --- post: description: >- Stores a new value. - parameters: - - in: path - schema: - type: integer - name: pk - - in: query - schema: - type: integer - name: dataset_id - required: true requestBody: required: true content: application/json: schema: - $ref: '#/components/schemas/KeyValuePostSchema' + $ref: '#/components/schemas/FormDataPostSchema' responses: 201: description: The value was stored successfully. @@ -96,41 +97,53 @@ def post(self, pk: int) -> Response: 500: $ref: '#/components/responses/500' """ - return super().post(pk) + if not request.is_json: + raise InvalidPayloadFormatError("Request is not JSON") + try: + item = self.add_model_schema.load(request.json) + args = CommandParameters( + actor=g.user, + dataset_id=item["dataset_id"], + chart_id=item.get("dataset_id"), + value=item["value"], + ) + key = CreateFormDataCommand(args).run() + return self.response(201, key=key) + except ValidationError as ex: + return self.response(400, message=ex.messages) + except ( + ChartAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) - @expose("//form_data/", methods=["PUT"]) + @expose("/form_data/", methods=["PUT"]) @protect() @safe @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", log_to_statsd=False, ) - def put(self, pk: int, key: str) -> Response: + def put(self, key: str) -> Response: """Updates an existing value. --- put: description: >- Updates an existing value. parameters: - - in: path - schema: - type: integer - name: pk - in: path schema: type: string name: key - - in: query - schema: - type: integer - name: dataset_id - required: true requestBody: required: true content: application/json: schema: - $ref: '#/components/schemas/KeyValuePutSchema' + $ref: '#/components/schemas/FormDataPutSchema' responses: 200: description: The value was stored successfully. @@ -153,35 +166,50 @@ def put(self, pk: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().put(pk, key) + if not request.is_json: + raise InvalidPayloadFormatError("Request is not JSON") + try: + item = self.edit_model_schema.load(request.json) + args = CommandParameters( + actor=g.user, + dataset_id=item["dataset_id"], + chart_id=item.get("dataset_id"), + key=key, + value=item["value"], + ) + result = UpdateFormDataCommand(args).run() + if not result: + return self.response_404() + return self.response(200, message="Value updated successfully.") + except ValidationError as ex: + return self.response(400, message=ex.messages) + except ( + ChartAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) - @expose("//form_data/", methods=["GET"]) + @expose("/form_data/", methods=["GET"]) @protect() @safe @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get", log_to_statsd=False, ) - def get(self, pk: int, key: str) -> Response: + def get(self, key: str) -> Response: """Retrives a value. --- get: description: >- Retrives a value. parameters: - - in: path - schema: - type: integer - name: pk - in: path schema: type: string name: key - - in: query - schema: - type: integer - name: dataset_id - required: true responses: 200: description: Returns the stored value. @@ -204,36 +232,40 @@ def get(self, pk: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().get(pk, key) + try: + args = CommandParameters(actor=g.user, key=key) + value = GetFormDataCommand(args).run() + if not value: + return self.response_404() + return self.response(200, value=value) + except ( + ChartAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) - @expose("//form_data/", methods=["DELETE"]) + @expose("/form_data/", methods=["DELETE"]) @protect() @safe @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", log_to_statsd=False, ) - def delete(self, pk: int, key: str) -> Response: + def delete(self, key: str) -> Response: """Deletes a value. --- delete: description: >- Deletes a value. parameters: - - in: path - schema: - type: integer - name: pk - in: path schema: type: string name: key description: The value key. - - in: query - schema: - type: integer - name: dataset_id - required: true responses: 200: description: Deleted the stored value. @@ -256,4 +288,17 @@ def delete(self, pk: int, key: str) -> Response: 500: $ref: '#/components/responses/500' """ - return super().delete(pk, key) + try: + args = CommandParameters(actor=g.user, key=key) + result = DeleteFormDataCommand(args).run() + if not result: + return self.response_404() + return self.response(200, message="Deleted successfully") + except ( + ChartAccessDeniedError, + DatasetAccessDeniedError, + KeyValueAccessDeniedError, + ) as ex: + return self.response(403, message=str(ex)) + except (ChartNotFoundError, DatasetNotFoundError) as ex: + return self.response(404, message=str(ex)) diff --git a/tests/integration_tests/charts/form_data/__init__.py b/superset/explore/form_data/commands/__init__.py similarity index 100% rename from tests/integration_tests/charts/form_data/__init__.py rename to superset/explore/form_data/commands/__init__.py diff --git a/superset/explore/form_data/commands/create.py b/superset/explore/form_data/commands/create.py new file mode 100644 index 000000000000..611d967f9fe7 --- /dev/null +++ b/superset/explore/form_data/commands/create.py @@ -0,0 +1,59 @@ +# 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 sqlalchemy.exc import SQLAlchemyError + +from superset.commands.base import BaseCommand +from superset.explore.form_data.commands.entry import Entry +from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.utils import check_access +from superset.extensions import cache_manager +from superset.key_value.commands.exceptions import KeyValueCreateFailedError +from superset.key_value.utils import cache_key + +logger = logging.getLogger(__name__) + + +class CreateFormDataCommand(BaseCommand): + def __init__(self, cmd_params: CommandParameters): + self._cmd_params = cmd_params + + def run(self) -> str: + try: + dataset_id = self._cmd_params.dataset_id + chart_id = self._cmd_params.chart_id + actor = self._cmd_params.actor + value = self._cmd_params.value + check_access(dataset_id, chart_id, actor) + key = token_urlsafe(48) + if value: + entry: Entry = { + "owner": actor.get_user_id(), + "dataset_id": dataset_id, + "chart_id": chart_id, + "value": value, + } + cache_manager.explore_form_data_cache.set(key, entry) + return key + except SQLAlchemyError as ex: + logger.exception("Error running create command") + raise KeyValueCreateFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/explore/form_data/commands/delete.py b/superset/explore/form_data/commands/delete.py new file mode 100644 index 000000000000..f6da3ee11477 --- /dev/null +++ b/superset/explore/form_data/commands/delete.py @@ -0,0 +1,56 @@ +# 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 ABC + +from sqlalchemy.exc import SQLAlchemyError + +from superset.commands.base import BaseCommand +from superset.explore.form_data.commands.entry import Entry +from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.utils import check_access +from superset.extensions import cache_manager +from superset.key_value.commands.exceptions import ( + KeyValueAccessDeniedError, + KeyValueDeleteFailedError, +) +from superset.key_value.utils import cache_key + +logger = logging.getLogger(__name__) + + +class DeleteFormDataCommand(BaseCommand, ABC): + def __init__(self, cmd_params: CommandParameters): + self._cmd_params = cmd_params + + def run(self) -> bool: + try: + actor = self._cmd_params.actor + key = self._cmd_params.key + entry: Entry = cache_manager.explore_form_data_cache.get(key) + if entry: + check_access(entry["dataset_id"], entry["chart_id"], actor) + if entry["owner"] != actor.get_user_id(): + raise KeyValueAccessDeniedError() + return cache_manager.explore_form_data_cache.delete(key) + return False + except SQLAlchemyError as ex: + logger.exception("Error running delete command") + raise KeyValueDeleteFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/explore/form_data/commands/entry.py b/superset/explore/form_data/commands/entry.py new file mode 100644 index 000000000000..0e8cdfc6874b --- /dev/null +++ b/superset/explore/form_data/commands/entry.py @@ -0,0 +1,24 @@ +# 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_extensions import TypedDict + + +class Entry(TypedDict): + owner: int + dataset_id: int + chart_id: int + value: str diff --git a/superset/explore/form_data/commands/get.py b/superset/explore/form_data/commands/get.py new file mode 100644 index 000000000000..0d9d03fd9428 --- /dev/null +++ b/superset/explore/form_data/commands/get.py @@ -0,0 +1,57 @@ +# 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 ABC +from typing import Optional + +from flask import current_app as app +from sqlalchemy.exc import SQLAlchemyError + +from superset.commands.base import BaseCommand +from superset.explore.form_data.commands.entry import Entry +from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.utils import check_access +from superset.extensions import cache_manager +from superset.key_value.commands.exceptions import KeyValueGetFailedError +from superset.key_value.utils import cache_key + +logger = logging.getLogger(__name__) + + +class GetFormDataCommand(BaseCommand, ABC): + def __init__(self, cmd_params: CommandParameters) -> None: + self._cmd_params = cmd_params + config = app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"] + self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + + def run(self) -> Optional[str]: + try: + actor = self._cmd_params.actor + key = self._cmd_params.key + entry: Entry = cache_manager.explore_form_data_cache.get(key) + if entry: + check_access(entry["dataset_id"], entry["chart_id"], actor) + if self._refresh_timeout: + cache_manager.explore_form_data_cache.set(key, entry) + return entry["value"] + return None + except SQLAlchemyError as ex: + logger.exception("Error running get command") + raise KeyValueGetFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/explore/form_data/commands/parameters.py b/superset/explore/form_data/commands/parameters.py new file mode 100644 index 000000000000..476fcc1afc42 --- /dev/null +++ b/superset/explore/form_data/commands/parameters.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 dataclasses import dataclass +from typing import Optional + +from flask_appbuilder.security.sqla.models import User + + +@dataclass +class CommandParameters: + actor: User + dataset_id: int = 0 + chart_id: int = 0 + key: Optional[str] = None + value: Optional[str] = None diff --git a/superset/explore/form_data/commands/update.py b/superset/explore/form_data/commands/update.py new file mode 100644 index 000000000000..05be249311bd --- /dev/null +++ b/superset/explore/form_data/commands/update.py @@ -0,0 +1,68 @@ +# 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 ABC + +from sqlalchemy.exc import SQLAlchemyError + +from superset.commands.base import BaseCommand +from superset.explore.form_data.commands.entry import Entry +from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.utils import check_access +from superset.extensions import cache_manager +from superset.key_value.commands.exceptions import ( + KeyValueAccessDeniedError, + KeyValueUpdateFailedError, +) +from superset.key_value.utils import cache_key + +logger = logging.getLogger(__name__) + + +class UpdateFormDataCommand(BaseCommand, ABC): + def __init__( + self, cmd_params: CommandParameters, + ): + self._cmd_params = cmd_params + + def run(self) -> bool: + try: + dataset_id = self._cmd_params.dataset_id + chart_id = self._cmd_params.chart_id + actor = self._cmd_params.actor + key = self._cmd_params.key + value = self._cmd_params.value + check_access(dataset_id, chart_id, actor) + entry: Entry = cache_manager.explore_form_data_cache.get(key) + if entry and value: + user_id = actor.get_user_id() + if entry["owner"] != user_id: + raise KeyValueAccessDeniedError() + new_entry: Entry = { + "owner": actor.get_user_id(), + "dataset_id": dataset_id, + "chart_id": chart_id, + "value": value, + } + return cache_manager.explore_form_data_cache.set(key, new_entry) + return False + except SQLAlchemyError as ex: + logger.exception("Error running update command") + raise KeyValueUpdateFailedError() from ex + + def validate(self) -> None: + pass diff --git a/superset/explore/form_data/schemas.py b/superset/explore/form_data/schemas.py new file mode 100644 index 000000000000..a1a3e4c4f914 --- /dev/null +++ b/superset/explore/form_data/schemas.py @@ -0,0 +1,37 @@ +# 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 FormDataPostSchema(Schema): + dataset_id = fields.Integer( + required=True, allow_none=False, description="The dataset ID" + ) + chart_id = fields.Integer(required=False, description="The chart ID") + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported text." + ) + + +class FormDataPutSchema(Schema): + dataset_id = fields.Integer( + required=True, allow_none=False, description="The dataset ID" + ) + chart_id = fields.Integer(required=False, description="The chart ID") + value = fields.String( + required=True, allow_none=False, description="Any type of JSON supported text." + ) diff --git a/superset/charts/form_data/utils.py b/superset/explore/form_data/utils.py similarity index 72% rename from superset/charts/form_data/utils.py rename to superset/explore/form_data/utils.py index c1f98a238840..25b549dd9e12 100644 --- a/superset/charts/form_data/utils.py +++ b/superset/explore/form_data/utils.py @@ -16,6 +16,8 @@ # under the License. from typing import Optional +from flask_appbuilder.security.sqla.models import User + from superset import security_manager from superset.charts.commands.exceptions import ( ChartAccessDeniedError, @@ -27,22 +29,13 @@ DatasetNotFoundError, ) from superset.datasets.dao import DatasetDAO -from superset.key_value.commands.parameters import CommandParameters from superset.views.base import is_user_admin from superset.views.utils import is_owner -def get_dataset_id(cmd_params: CommandParameters) -> Optional[str]: - query_params = cmd_params.query_params - if query_params: - return query_params.get("dataset_id") - return None - - -def check_dataset_access(cmd_params: CommandParameters) -> Optional[bool]: - dataset_id = get_dataset_id(cmd_params) +def check_dataset_access(dataset_id: int) -> Optional[bool]: if dataset_id: - dataset = DatasetDAO.find_by_id(int(dataset_id)) + dataset = DatasetDAO.find_by_id(dataset_id) if dataset: can_access_datasource = security_manager.can_access_datasource(dataset) if can_access_datasource: @@ -51,13 +44,11 @@ def check_dataset_access(cmd_params: CommandParameters) -> Optional[bool]: raise DatasetNotFoundError() -def check_access(cmd_params: CommandParameters) -> Optional[bool]: - resource_id = cmd_params.resource_id - actor = cmd_params.actor - check_dataset_access(cmd_params) - if resource_id == 0: +def check_access(dataset_id: int, chart_id: int, actor: User) -> Optional[bool]: + check_dataset_access(dataset_id) + if not chart_id: return True - chart = ChartDAO.find_by_id(resource_id) + chart = ChartDAO.find_by_id(chart_id) if chart: can_access_chart = ( is_user_admin() diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 98cd9af264cf..f2bce87afbc5 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -119,7 +119,6 @@ def init_views(self) -> None: from superset.cachekeys.api import CacheRestApi from superset.charts.api import ChartRestApi from superset.charts.data.api import ChartDataRestApi - from superset.charts.form_data.api import ChartFormDataRestApi from superset.connectors.druid.views import ( Druid, DruidClusterModelView, @@ -141,6 +140,7 @@ def init_views(self) -> None: from superset.datasets.api import DatasetRestApi from superset.datasets.columns.api import DatasetColumnsRestApi from superset.datasets.metrics.api import DatasetMetricRestApi + from superset.explore.form_data.api import ExploreFormDataRestApi from superset.queries.api import QueryRestApi from superset.queries.saved_queries.api import SavedQueryRestApi from superset.reports.api import ReportScheduleRestApi @@ -204,7 +204,6 @@ def init_views(self) -> None: appbuilder.add_api(CacheRestApi) appbuilder.add_api(ChartRestApi) appbuilder.add_api(ChartDataRestApi) - appbuilder.add_api(ChartFormDataRestApi) appbuilder.add_api(CssTemplateRestApi) appbuilder.add_api(DashboardFilterStateRestApi) appbuilder.add_api(DashboardRestApi) @@ -212,6 +211,7 @@ def init_views(self) -> None: appbuilder.add_api(DatasetRestApi) appbuilder.add_api(DatasetColumnsRestApi) appbuilder.add_api(DatasetMetricRestApi) + appbuilder.add_api(ExploreFormDataRestApi) appbuilder.add_api(FilterSetRestApi) appbuilder.add_api(QueryRestApi) appbuilder.add_api(ReportScheduleRestApi) diff --git a/superset/utils/cache_manager.py b/superset/utils/cache_manager.py index 3624d918f9c7..e92d930d2ef8 100644 --- a/superset/utils/cache_manager.py +++ b/superset/utils/cache_manager.py @@ -26,7 +26,7 @@ def __init__(self) -> None: self._data_cache = Cache() self._thumbnail_cache = Cache() self._filter_state_cache = Cache() - self._chart_form_data_cache = Cache() + self._explore_form_data_cache = Cache() def init_app(self, app: Flask) -> None: self._cache.init_app( @@ -57,11 +57,11 @@ def init_app(self, app: Flask) -> None: **app.config["FILTER_STATE_CACHE_CONFIG"], }, ) - self._chart_form_data_cache.init_app( + self._explore_form_data_cache.init_app( app, { "CACHE_DEFAULT_TIMEOUT": app.config["CACHE_DEFAULT_TIMEOUT"], - **app.config["CHART_FORM_DATA_CACHE_CONFIG"], + **app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"], }, ) @@ -82,5 +82,5 @@ def filter_state_cache(self) -> Cache: return self._filter_state_cache @property - def chart_form_data_cache(self) -> Cache: - return self._chart_form_data_cache + def explore_form_data_cache(self) -> Cache: + return self._explore_form_data_cache diff --git a/tests/integration_tests/explore/__init__.py b/tests/integration_tests/explore/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/integration_tests/explore/__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/explore/form_data/__init__.py b/tests/integration_tests/explore/form_data/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/integration_tests/explore/form_data/__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/charts/form_data/api_tests.py b/tests/integration_tests/explore/form_data/api_tests.py similarity index 59% rename from tests/integration_tests/charts/form_data/api_tests.py rename to tests/integration_tests/explore/form_data/api_tests.py index f411c1a6edc9..99330aacfdc2 100644 --- a/tests/integration_tests/charts/form_data/api_tests.py +++ b/tests/integration_tests/explore/form_data/api_tests.py @@ -23,9 +23,8 @@ from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.exceptions import DatasetAccessDeniedError +from superset.explore.form_data.commands.entry import Entry from superset.extensions import cache_manager -from superset.key_value.commands.entry import Entry -from superset.key_value.utils import cache_key from superset.models.slice import Slice from tests.integration_tests.base_tests import login from tests.integration_tests.fixtures.world_bank_dashboard import ( @@ -75,171 +74,144 @@ def dataset_id() -> int: @pytest.fixture(autouse=True) def cache(chart_id, admin_id, dataset_id): - app.config["CHART_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} + app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} cache_manager.init_app(app) - entry: Entry = {"owner": admin_id, "value": value} - cache_manager.chart_form_data_cache.set(cache_key(chart_id, key), entry) - cache_manager.chart_form_data_cache.set(cache_key(dataset_id, key), entry) + entry: Entry = { + "owner": admin_id, + "dataset_id": dataset_id, + "chart_id": chart_id, + "value": value, + } + cache_manager.explore_form_data_cache.set(key, entry) def test_post(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": value, } - resp = client.post( - f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload - ) + resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 201 def test_post_bad_request(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": 1234, } - resp = client.post( - f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload - ) + resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 400 def test_post_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": value, } - resp = client.post( - f"api/v1/chart/{chart_id}/form_data?dataset_id={dataset_id}", json=payload - ) + resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 404 def test_put(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": "new value", } - resp = client.put( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload - ) + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 200 def test_put_bad_request(client, chart_id: int, dataset_id: int): login(client, "admin") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": 1234, } - resp = client.put( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload - ) + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 400 def test_put_access_denied(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": "new value", } - resp = client.put( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload - ) + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 404 def test_put_not_owner(client, chart_id: int, dataset_id: int): login(client, "gamma") payload = { + "dataset_id": dataset_id, + "chart_id": chart_id, "value": "new value", } - resp = client.put( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}", json=payload - ) - assert resp.status_code == 404 - - -def test_get_key_not_found(client, chart_id: int, dataset_id: int): - login(client, "admin") - resp = client.get( - f"api/v1/chart/{chart_id}/form_data/unknown-key?dataset_id={dataset_id}" - ) + resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 404 -def test_get_chart_not_found(client, dataset_id: int): +def test_get_key_not_found(client): login(client, "admin") - resp = client.get(f"api/v1/chart/-1/form_data/{key}?dataset_id={dataset_id}") + resp = client.get(f"api/v1/explore/form_data/unknown-key") assert resp.status_code == 404 -def test_get(client, chart_id: int, dataset_id: int): +def test_get(client): login(client, "admin") - resp = client.get( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" - ) + resp = client.get(f"api/v1/explore/form_data/{key}") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) assert value == data.get("value") -def test_get_access_denied(client, chart_id: int, dataset_id: int): +def test_get_access_denied(client): login(client, "gamma") - resp = client.get( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" - ) - assert resp.status_code == 404 - - -def test_get_no_dataset(client): - login(client, "admin") - resp = client.get(f"api/v1/chart/0/form_data/{key}") - assert resp.status_code == 404 - - -def test_get_dataset(client, dataset_id: int): - login(client, "admin") - resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id={dataset_id}") - assert resp.status_code == 200 - - -def test_get_dataset_not_found(client): - login(client, "admin") - resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id=-1") + resp = client.get(f"api/v1/explore/form_data/{key}") assert resp.status_code == 404 @patch("superset.security.SupersetSecurityManager.can_access_datasource") -def test_get_dataset_access_denied(mock_can_access_datasource, client, dataset_id): +def test_get_dataset_access_denied(mock_can_access_datasource, client): mock_can_access_datasource.side_effect = DatasetAccessDeniedError() login(client, "admin") - resp = client.get(f"api/v1/chart/0/form_data/{key}?dataset_id={dataset_id}") + resp = client.get(f"api/v1/explore/form_data/{key}") assert resp.status_code == 403 -def test_delete(client, chart_id: int, dataset_id: int): +def test_delete(client): login(client, "admin") - resp = client.delete( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" - ) + resp = client.delete(f"api/v1/explore/form_data/{key}") assert resp.status_code == 200 -def test_delete_access_denied(client, chart_id: int, dataset_id: int): +def test_delete_access_denied(client): login(client, "gamma") - resp = client.delete( - f"api/v1/chart/{chart_id}/form_data/{key}?dataset_id={dataset_id}" - ) + resp = client.delete(f"api/v1/explore/form_data/{key}") assert resp.status_code == 404 def test_delete_not_owner(client, chart_id: int, dataset_id: int, admin_id: int): another_key = "another_key" another_owner = admin_id + 1 - entry: Entry = {"owner": another_owner, "value": value} - cache_manager.chart_form_data_cache.set(cache_key(chart_id, another_key), entry) + entry: Entry = { + "owner": another_owner, + "dataset_id": dataset_id, + "chart_id": chart_id, + "value": value, + } + cache_manager.explore_form_data_cache.set(another_key, entry) login(client, "admin") - resp = client.delete( - f"api/v1/chart/{chart_id}/form_data/{another_key}?dataset_id={dataset_id}" - ) + resp = client.delete(f"api/v1/explore/form_data/{another_key}") assert resp.status_code == 403 diff --git a/tests/unit_tests/explore/__init__.py b/tests/unit_tests/explore/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/unit_tests/explore/__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/unit_tests/explore/form_data/__init__.py b/tests/unit_tests/explore/form_data/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/unit_tests/explore/form_data/__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/unit_tests/charts/form_data/utils_test.py b/tests/unit_tests/explore/form_data/utils_test.py similarity index 68% rename from tests/unit_tests/charts/form_data/utils_test.py rename to tests/unit_tests/explore/form_data/utils_test.py index 1180b10d36ed..4025dc951e11 100644 --- a/tests/unit_tests/charts/form_data/utils_test.py +++ b/tests/unit_tests/explore/form_data/utils_test.py @@ -27,12 +27,11 @@ DatasetAccessDeniedError, DatasetNotFoundError, ) -from superset.key_value.commands.parameters import CommandParameters dataset_find_by_id = "superset.datasets.dao.DatasetDAO.find_by_id" chart_find_by_id = "superset.charts.dao.ChartDAO.find_by_id" -is_user_admin = "superset.charts.form_data.utils.is_user_admin" -is_owner = "superset.charts.form_data.utils.is_owner" +is_user_admin = "superset.explore.form_data.utils.is_user_admin" +is_owner = "superset.explore.form_data.utils.is_owner" can_access_datasource = ( "superset.security.SupersetSecurityManager.can_access_datasource" ) @@ -40,104 +39,85 @@ def test_unsaved_chart_no_dataset_id(app_context: AppContext) -> None: - from superset.charts.form_data.utils import check_access + from superset.explore.form_data.utils import check_access with raises(DatasetNotFoundError): - cmd_params = CommandParameters(resource_id=0, actor=User(), query_params={}) - check_access(cmd_params) + check_access(dataset_id=0, chart_id=0, actor=User()) def test_unsaved_chart_unknown_dataset_id( mocker: MockFixture, app_context: AppContext ) -> None: - from superset.charts.form_data.utils import check_access + from superset.explore.form_data.utils import check_access with raises(DatasetNotFoundError): mocker.patch(dataset_find_by_id, return_value=None) - cmd_params = CommandParameters( - resource_id=0, actor=User(), query_params={"dataset_id": "1"} - ) - check_access(cmd_params) + check_access(dataset_id=1, chart_id=0, actor=User()) def test_unsaved_chart_unauthorized_dataset( mocker: MockFixture, app_context: AppContext ) -> None: - from superset.charts.form_data import utils from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data import utils with raises(DatasetAccessDeniedError): mocker.patch(dataset_find_by_id, return_value=SqlaTable()) mocker.patch(can_access_datasource, return_value=False) - cmd_params = CommandParameters( - resource_id=0, actor=User(), query_params={"dataset_id": "1"} - ) - utils.check_access(cmd_params) + utils.check_access(dataset_id=1, chart_id=0, actor=User()) def test_unsaved_chart_authorized_dataset( mocker: MockFixture, app_context: AppContext ) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access mocker.patch(dataset_find_by_id, return_value=SqlaTable()) mocker.patch(can_access_datasource, return_value=True) - cmd_params = CommandParameters( - resource_id=0, actor=User(), query_params={"dataset_id": "1"} - ) - assert check_access(cmd_params) == True + assert check_access(dataset_id=1, chart_id=0, actor=User()) == True def test_saved_chart_unknown_chart_id( mocker: MockFixture, app_context: AppContext ) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access with raises(ChartNotFoundError): mocker.patch(dataset_find_by_id, return_value=SqlaTable()) mocker.patch(can_access_datasource, return_value=True) mocker.patch(chart_find_by_id, return_value=None) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - check_access(cmd_params) + check_access(dataset_id=1, chart_id=1, actor=User()) def test_saved_chart_unauthorized_dataset( mocker: MockFixture, app_context: AppContext ) -> None: - from superset.charts.form_data import utils from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data import utils with raises(DatasetAccessDeniedError): mocker.patch(dataset_find_by_id, return_value=SqlaTable()) mocker.patch(can_access_datasource, return_value=False) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - utils.check_access(cmd_params) + utils.check_access(dataset_id=1, chart_id=1, actor=User()) def test_saved_chart_is_admin(mocker: MockFixture, app_context: AppContext) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access from superset.models.slice import Slice mocker.patch(dataset_find_by_id, return_value=SqlaTable()) mocker.patch(can_access_datasource, return_value=True) mocker.patch(is_user_admin, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - assert check_access(cmd_params) == True + assert check_access(dataset_id=1, chart_id=1, actor=User()) == True def test_saved_chart_is_owner(mocker: MockFixture, app_context: AppContext) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access from superset.models.slice import Slice mocker.patch(dataset_find_by_id, return_value=SqlaTable()) @@ -145,15 +125,12 @@ def test_saved_chart_is_owner(mocker: MockFixture, app_context: AppContext) -> N mocker.patch(is_user_admin, return_value=False) mocker.patch(is_owner, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - assert check_access(cmd_params) == True + assert check_access(dataset_id=1, chart_id=1, actor=User()) == True def test_saved_chart_has_access(mocker: MockFixture, app_context: AppContext) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access from superset.models.slice import Slice mocker.patch(dataset_find_by_id, return_value=SqlaTable()) @@ -162,15 +139,12 @@ def test_saved_chart_has_access(mocker: MockFixture, app_context: AppContext) -> mocker.patch(is_owner, return_value=False) mocker.patch(can_access, return_value=True) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - assert check_access(cmd_params) == True + assert check_access(dataset_id=1, chart_id=1, actor=User()) == True def test_saved_chart_no_access(mocker: MockFixture, app_context: AppContext) -> None: - from superset.charts.form_data.utils import check_access from superset.connectors.sqla.models import SqlaTable + from superset.explore.form_data.utils import check_access from superset.models.slice import Slice with raises(ChartAccessDeniedError): @@ -180,7 +154,4 @@ def test_saved_chart_no_access(mocker: MockFixture, app_context: AppContext) -> mocker.patch(is_owner, return_value=False) mocker.patch(can_access, return_value=False) mocker.patch(chart_find_by_id, return_value=Slice()) - cmd_params = CommandParameters( - resource_id=1, actor=User(), query_params={"dataset_id": "1"} - ) - check_access(cmd_params) + check_access(dataset_id=1, chart_id=1, actor=User()) From 818fe5a2963058b0fbd2fd4a7183b51377b8a559 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 24 Jan 2022 14:10:02 -0300 Subject: [PATCH 2/5] Removes unused imports --- superset/explore/form_data/commands/create.py | 1 - superset/explore/form_data/commands/delete.py | 1 - superset/explore/form_data/commands/get.py | 1 - superset/explore/form_data/commands/update.py | 1 - 4 files changed, 4 deletions(-) diff --git a/superset/explore/form_data/commands/create.py b/superset/explore/form_data/commands/create.py index 611d967f9fe7..d64bc3f31f97 100644 --- a/superset/explore/form_data/commands/create.py +++ b/superset/explore/form_data/commands/create.py @@ -25,7 +25,6 @@ from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import KeyValueCreateFailedError -from superset.key_value.utils import cache_key logger = logging.getLogger(__name__) diff --git a/superset/explore/form_data/commands/delete.py b/superset/explore/form_data/commands/delete.py index f6da3ee11477..c14bf537b9f8 100644 --- a/superset/explore/form_data/commands/delete.py +++ b/superset/explore/form_data/commands/delete.py @@ -28,7 +28,6 @@ KeyValueAccessDeniedError, KeyValueDeleteFailedError, ) -from superset.key_value.utils import cache_key logger = logging.getLogger(__name__) diff --git a/superset/explore/form_data/commands/get.py b/superset/explore/form_data/commands/get.py index 0d9d03fd9428..1f82b871277b 100644 --- a/superset/explore/form_data/commands/get.py +++ b/superset/explore/form_data/commands/get.py @@ -27,7 +27,6 @@ from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import KeyValueGetFailedError -from superset.key_value.utils import cache_key logger = logging.getLogger(__name__) diff --git a/superset/explore/form_data/commands/update.py b/superset/explore/form_data/commands/update.py index 05be249311bd..c9df3b364670 100644 --- a/superset/explore/form_data/commands/update.py +++ b/superset/explore/form_data/commands/update.py @@ -28,7 +28,6 @@ KeyValueAccessDeniedError, KeyValueUpdateFailedError, ) -from superset.key_value.utils import cache_key logger = logging.getLogger(__name__) From 3a02cf4a4c9a24f80b5e797784a0682d719ce506 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 24 Jan 2022 14:42:01 -0300 Subject: [PATCH 3/5] Fixes openapi schema error --- superset/explore/form_data/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/superset/explore/form_data/api.py b/superset/explore/form_data/api.py index 5a325dc4a90b..f7d2b1daa115 100644 --- a/superset/explore/form_data/api.py +++ b/superset/explore/form_data/api.py @@ -57,6 +57,7 @@ class ExploreFormDataRestApi(BaseApi, ABC): class_permission_name = "ExploreFormDataRestApi" resource_name = "explore" openapi_spec_tag = "Explore Form Data" + openapi_spec_component_schemas = (FormDataPostSchema, FormDataPutSchema) @expose("/form_data", methods=["POST"]) @protect() From c6e09c76e1af586a467eedc3eb667fa04aa0b370 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 24 Jan 2022 15:34:35 -0300 Subject: [PATCH 4/5] Fixes typo --- superset/explore/form_data/api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/explore/form_data/api.py b/superset/explore/form_data/api.py index f7d2b1daa115..c328ab515510 100644 --- a/superset/explore/form_data/api.py +++ b/superset/explore/form_data/api.py @@ -105,7 +105,7 @@ def post(self) -> Response: args = CommandParameters( actor=g.user, dataset_id=item["dataset_id"], - chart_id=item.get("dataset_id"), + chart_id=item.get("chart_id"), value=item["value"], ) key = CreateFormDataCommand(args).run() @@ -174,7 +174,7 @@ def put(self, key: str) -> Response: args = CommandParameters( actor=g.user, dataset_id=item["dataset_id"], - chart_id=item.get("dataset_id"), + chart_id=item.get("chart_id"), key=key, value=item["value"], ) From a2daa48cde72cebdc281cc355fddf35932f697b9 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 25 Jan 2022 09:40:49 -0300 Subject: [PATCH 5/5] Renames and UPDATING.md --- UPDATING.md | 1 + superset/explore/form_data/api.py | 42 +++++++++---------- superset/explore/form_data/commands/create.py | 12 +++--- superset/explore/form_data/commands/delete.py | 12 +++--- superset/explore/form_data/commands/get.py | 14 ++++--- .../explore/form_data/commands/parameters.py | 2 +- .../form_data/commands/{entry.py => state.py} | 8 ++-- superset/explore/form_data/commands/update.py | 18 ++++---- superset/explore/form_data/schemas.py | 4 +- superset/explore/form_data/utils.py | 4 +- .../explore/form_data/api_tests.py | 28 ++++++------- 11 files changed, 78 insertions(+), 67 deletions(-) rename superset/explore/form_data/commands/{entry.py => state.py} (88%) diff --git a/UPDATING.md b/UPDATING.md index 26ab65ff5389..4ded7b25422a 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -46,6 +46,7 @@ assists people when migrating to a new version. - [17589](https://github.com/apache/incubator-superset/pull/17589): It is now possible to limit access to users' recent activity data by setting the `ENABLE_BROAD_ACTIVITY_ACCESS` config flag to false, or customizing the `raise_for_user_activity_access` method in the security manager. - [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/). +- [17882](https://github.com/apache/superset/pull/17882): introduced a key-value endpoint to store Explore form data. 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 `EXPLORE_FORM_DATA_CACHE_CONFIG` and the available settings can be found in Flask-Caching [docs](https://flask-caching.readthedocs.io/en/latest/). ## 1.4.0 diff --git a/superset/explore/form_data/api.py b/superset/explore/form_data/api.py index c328ab515510..2ed709676893 100644 --- a/superset/explore/form_data/api.py +++ b/superset/explore/form_data/api.py @@ -67,11 +67,11 @@ class ExploreFormDataRestApi(BaseApi, ABC): log_to_statsd=False, ) def post(self) -> Response: - """Stores a new value. + """Stores a new form_data. --- post: description: >- - Stores a new value. + Stores a new form_data. requestBody: required: true content: @@ -80,7 +80,7 @@ def post(self) -> Response: $ref: '#/components/schemas/FormDataPostSchema' responses: 201: - description: The value was stored successfully. + description: The form_data was stored successfully. content: application/json: schema: @@ -88,7 +88,7 @@ def post(self) -> Response: properties: key: type: string - description: The key to retrieve the value. + description: The key to retrieve the form_data. 400: $ref: '#/components/responses/400' 401: @@ -106,7 +106,7 @@ def post(self) -> Response: actor=g.user, dataset_id=item["dataset_id"], chart_id=item.get("chart_id"), - value=item["value"], + form_data=item["form_data"], ) key = CreateFormDataCommand(args).run() return self.response(201, key=key) @@ -129,11 +129,11 @@ def post(self) -> Response: log_to_statsd=False, ) def put(self, key: str) -> Response: - """Updates an existing value. + """Updates an existing form_data. --- put: description: >- - Updates an existing value. + Updates an existing form_data. parameters: - in: path schema: @@ -147,7 +147,7 @@ def put(self, key: str) -> Response: $ref: '#/components/schemas/FormDataPutSchema' responses: 200: - description: The value was stored successfully. + description: The form_data was stored successfully. content: application/json: schema: @@ -176,7 +176,7 @@ def put(self, key: str) -> Response: dataset_id=item["dataset_id"], chart_id=item.get("chart_id"), key=key, - value=item["value"], + form_data=item["form_data"], ) result = UpdateFormDataCommand(args).run() if not result: @@ -201,11 +201,11 @@ def put(self, key: str) -> Response: log_to_statsd=False, ) def get(self, key: str) -> Response: - """Retrives a value. + """Retrives a form_data. --- get: description: >- - Retrives a value. + Retrives a form_data. parameters: - in: path schema: @@ -213,15 +213,15 @@ def get(self, key: str) -> Response: name: key responses: 200: - description: Returns the stored value. + description: Returns the stored form_data. content: application/json: schema: type: object properties: - value: + form_data: type: string - description: The stored value + description: The stored form_data 400: $ref: '#/components/responses/400' 401: @@ -235,10 +235,10 @@ def get(self, key: str) -> Response: """ try: args = CommandParameters(actor=g.user, key=key) - value = GetFormDataCommand(args).run() - if not value: + form_data = GetFormDataCommand(args).run() + if not form_data: return self.response_404() - return self.response(200, value=value) + return self.response(200, form_data=form_data) except ( ChartAccessDeniedError, DatasetAccessDeniedError, @@ -256,20 +256,20 @@ def get(self, key: str) -> Response: log_to_statsd=False, ) def delete(self, key: str) -> Response: - """Deletes a value. + """Deletes a form_data. --- delete: description: >- - Deletes a value. + Deletes a form_data. parameters: - in: path schema: type: string name: key - description: The value key. + description: The form_data key. responses: 200: - description: Deleted the stored value. + description: Deleted the stored form_data. content: application/json: schema: diff --git a/superset/explore/form_data/commands/create.py b/superset/explore/form_data/commands/create.py index d64bc3f31f97..a2325f0be924 100644 --- a/superset/explore/form_data/commands/create.py +++ b/superset/explore/form_data/commands/create.py @@ -20,8 +20,8 @@ from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.explore.form_data.commands.entry import Entry from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.commands.state import TemporaryExploreState from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import KeyValueCreateFailedError @@ -38,17 +38,17 @@ def run(self) -> str: dataset_id = self._cmd_params.dataset_id chart_id = self._cmd_params.chart_id actor = self._cmd_params.actor - value = self._cmd_params.value + form_data = self._cmd_params.form_data check_access(dataset_id, chart_id, actor) key = token_urlsafe(48) - if value: - entry: Entry = { + if form_data: + state: TemporaryExploreState = { "owner": actor.get_user_id(), "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } - cache_manager.explore_form_data_cache.set(key, entry) + cache_manager.explore_form_data_cache.set(key, state) return key except SQLAlchemyError as ex: logger.exception("Error running create command") diff --git a/superset/explore/form_data/commands/delete.py b/superset/explore/form_data/commands/delete.py index c14bf537b9f8..8dc414ee14cd 100644 --- a/superset/explore/form_data/commands/delete.py +++ b/superset/explore/form_data/commands/delete.py @@ -20,8 +20,8 @@ from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.explore.form_data.commands.entry import Entry from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.commands.state import TemporaryExploreState from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import ( @@ -40,10 +40,12 @@ def run(self) -> bool: try: actor = self._cmd_params.actor key = self._cmd_params.key - entry: Entry = cache_manager.explore_form_data_cache.get(key) - if entry: - check_access(entry["dataset_id"], entry["chart_id"], actor) - if entry["owner"] != actor.get_user_id(): + state: TemporaryExploreState = cache_manager.explore_form_data_cache.get( + key + ) + if state: + check_access(state["dataset_id"], state["chart_id"], actor) + if state["owner"] != actor.get_user_id(): raise KeyValueAccessDeniedError() return cache_manager.explore_form_data_cache.delete(key) return False diff --git a/superset/explore/form_data/commands/get.py b/superset/explore/form_data/commands/get.py index 1f82b871277b..2252387b3408 100644 --- a/superset/explore/form_data/commands/get.py +++ b/superset/explore/form_data/commands/get.py @@ -22,8 +22,8 @@ from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.explore.form_data.commands.entry import Entry from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.commands.state import TemporaryExploreState from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import KeyValueGetFailedError @@ -41,12 +41,14 @@ def run(self) -> Optional[str]: try: actor = self._cmd_params.actor key = self._cmd_params.key - entry: Entry = cache_manager.explore_form_data_cache.get(key) - if entry: - check_access(entry["dataset_id"], entry["chart_id"], actor) + state: TemporaryExploreState = cache_manager.explore_form_data_cache.get( + key + ) + if state: + check_access(state["dataset_id"], state["chart_id"], actor) if self._refresh_timeout: - cache_manager.explore_form_data_cache.set(key, entry) - return entry["value"] + cache_manager.explore_form_data_cache.set(key, state) + return state["form_data"] return None except SQLAlchemyError as ex: logger.exception("Error running get command") diff --git a/superset/explore/form_data/commands/parameters.py b/superset/explore/form_data/commands/parameters.py index 476fcc1afc42..c4d41a184a78 100644 --- a/superset/explore/form_data/commands/parameters.py +++ b/superset/explore/form_data/commands/parameters.py @@ -26,4 +26,4 @@ class CommandParameters: dataset_id: int = 0 chart_id: int = 0 key: Optional[str] = None - value: Optional[str] = None + form_data: Optional[str] = None diff --git a/superset/explore/form_data/commands/entry.py b/superset/explore/form_data/commands/state.py similarity index 88% rename from superset/explore/form_data/commands/entry.py rename to superset/explore/form_data/commands/state.py index 0e8cdfc6874b..2aba14a8cb28 100644 --- a/superset/explore/form_data/commands/entry.py +++ b/superset/explore/form_data/commands/state.py @@ -14,11 +14,13 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from typing import Optional + from typing_extensions import TypedDict -class Entry(TypedDict): +class TemporaryExploreState(TypedDict): owner: int dataset_id: int - chart_id: int - value: str + chart_id: Optional[int] + form_data: str diff --git a/superset/explore/form_data/commands/update.py b/superset/explore/form_data/commands/update.py index c9df3b364670..8b69ae885f71 100644 --- a/superset/explore/form_data/commands/update.py +++ b/superset/explore/form_data/commands/update.py @@ -20,8 +20,8 @@ from sqlalchemy.exc import SQLAlchemyError from superset.commands.base import BaseCommand -from superset.explore.form_data.commands.entry import Entry from superset.explore.form_data.commands.parameters import CommandParameters +from superset.explore.form_data.commands.state import TemporaryExploreState from superset.explore.form_data.utils import check_access from superset.extensions import cache_manager from superset.key_value.commands.exceptions import ( @@ -44,20 +44,22 @@ def run(self) -> bool: chart_id = self._cmd_params.chart_id actor = self._cmd_params.actor key = self._cmd_params.key - value = self._cmd_params.value + form_data = self._cmd_params.form_data check_access(dataset_id, chart_id, actor) - entry: Entry = cache_manager.explore_form_data_cache.get(key) - if entry and value: + state: TemporaryExploreState = cache_manager.explore_form_data_cache.get( + key + ) + if state and form_data: user_id = actor.get_user_id() - if entry["owner"] != user_id: + if state["owner"] != user_id: raise KeyValueAccessDeniedError() - new_entry: Entry = { + new_state: TemporaryExploreState = { "owner": actor.get_user_id(), "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } - return cache_manager.explore_form_data_cache.set(key, new_entry) + return cache_manager.explore_form_data_cache.set(key, new_state) return False except SQLAlchemyError as ex: logger.exception("Error running update command") diff --git a/superset/explore/form_data/schemas.py b/superset/explore/form_data/schemas.py index a1a3e4c4f914..6d5509d777a3 100644 --- a/superset/explore/form_data/schemas.py +++ b/superset/explore/form_data/schemas.py @@ -22,7 +22,7 @@ class FormDataPostSchema(Schema): required=True, allow_none=False, description="The dataset ID" ) chart_id = fields.Integer(required=False, description="The chart ID") - value = fields.String( + form_data = fields.String( required=True, allow_none=False, description="Any type of JSON supported text." ) @@ -32,6 +32,6 @@ class FormDataPutSchema(Schema): required=True, allow_none=False, description="The dataset ID" ) chart_id = fields.Integer(required=False, description="The chart ID") - value = fields.String( + form_data = fields.String( required=True, allow_none=False, description="Any type of JSON supported text." ) diff --git a/superset/explore/form_data/utils.py b/superset/explore/form_data/utils.py index 25b549dd9e12..3eeb1bab9964 100644 --- a/superset/explore/form_data/utils.py +++ b/superset/explore/form_data/utils.py @@ -44,7 +44,9 @@ def check_dataset_access(dataset_id: int) -> Optional[bool]: raise DatasetNotFoundError() -def check_access(dataset_id: int, chart_id: int, actor: User) -> Optional[bool]: +def check_access( + dataset_id: int, chart_id: Optional[int], actor: User +) -> Optional[bool]: check_dataset_access(dataset_id) if not chart_id: return True diff --git a/tests/integration_tests/explore/form_data/api_tests.py b/tests/integration_tests/explore/form_data/api_tests.py index 99330aacfdc2..bf0fc6cab4be 100644 --- a/tests/integration_tests/explore/form_data/api_tests.py +++ b/tests/integration_tests/explore/form_data/api_tests.py @@ -23,7 +23,7 @@ from superset.connectors.sqla.models import SqlaTable from superset.datasets.commands.exceptions import DatasetAccessDeniedError -from superset.explore.form_data.commands.entry import Entry +from superset.explore.form_data.commands.state import TemporaryExploreState from superset.extensions import cache_manager from superset.models.slice import Slice from tests.integration_tests.base_tests import login @@ -34,7 +34,7 @@ from tests.integration_tests.test_app import app key = "test-key" -value = "test" +form_data = "test" @pytest.fixture @@ -76,11 +76,11 @@ def dataset_id() -> int: def cache(chart_id, admin_id, dataset_id): app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"} cache_manager.init_app(app) - entry: Entry = { + entry: TemporaryExploreState = { "owner": admin_id, "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } cache_manager.explore_form_data_cache.set(key, entry) @@ -90,7 +90,7 @@ def test_post(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 201 @@ -101,7 +101,7 @@ def test_post_bad_request(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": 1234, + "form_data": 1234, } resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 400 @@ -112,7 +112,7 @@ def test_post_access_denied(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } resp = client.post("api/v1/explore/form_data", json=payload) assert resp.status_code == 404 @@ -123,7 +123,7 @@ def test_put(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": "new value", + "form_data": "new form_data", } resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 200 @@ -134,7 +134,7 @@ def test_put_bad_request(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": 1234, + "form_data": 1234, } resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 400 @@ -145,7 +145,7 @@ def test_put_access_denied(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": "new value", + "form_data": "new form_data", } resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 404 @@ -156,7 +156,7 @@ def test_put_not_owner(client, chart_id: int, dataset_id: int): payload = { "dataset_id": dataset_id, "chart_id": chart_id, - "value": "new value", + "form_data": "new form_data", } resp = client.put(f"api/v1/explore/form_data/{key}", json=payload) assert resp.status_code == 404 @@ -173,7 +173,7 @@ def test_get(client): resp = client.get(f"api/v1/explore/form_data/{key}") assert resp.status_code == 200 data = json.loads(resp.data.decode("utf-8")) - assert value == data.get("value") + assert form_data == data.get("form_data") def test_get_access_denied(client): @@ -205,11 +205,11 @@ def test_delete_access_denied(client): def test_delete_not_owner(client, chart_id: int, dataset_id: int, admin_id: int): another_key = "another_key" another_owner = admin_id + 1 - entry: Entry = { + entry: TemporaryExploreState = { "owner": another_owner, "dataset_id": dataset_id, "chart_id": chart_id, - "value": value, + "form_data": form_data, } cache_manager.explore_form_data_cache.set(another_key, entry) login(client, "admin")