From 07e06e532587cac1f12bf19fcbbe63420184c30a Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Mon, 10 Jan 2022 11:19:09 -0300 Subject: [PATCH] Changes the parameters to use dataclass --- superset/charts/form_data/api.py | 6 +-- superset/charts/form_data/commands/create.py | 18 ++++---- superset/charts/form_data/commands/delete.py | 6 +-- superset/charts/form_data/commands/get.py | 13 +++--- superset/charts/form_data/commands/update.py | 10 ++--- superset/charts/form_data/utils.py | 6 +-- .../filter_state/commands/create.py | 10 ++--- .../filter_state/commands/delete.py | 6 +-- .../dashboards/filter_state/commands/get.py | 19 ++++---- .../filter_state/commands/update.py | 10 ++--- superset/key_value/api.py | 44 ++++++++----------- superset/key_value/commands/create.py | 6 +-- superset/key_value/commands/delete.py | 4 +- superset/key_value/commands/get.py | 4 +- superset/key_value/commands/parameters.py | 11 ++--- .../charts/form_data/api_tests.py | 12 +++-- 16 files changed, 92 insertions(+), 93 deletions(-) diff --git a/superset/charts/form_data/api.py b/superset/charts/form_data/api.py index 61063b0fe6b8..ed811fd77283 100644 --- a/superset/charts/form_data/api.py +++ b/superset/charts/form_data/api.py @@ -75,8 +75,7 @@ def post(self, pk: int) -> Response: content: application/json: schema: - type: object - $ref: '#/components/schemas/KeyValuePostSchema' + $ref: '#/components/schemas/KeyValuePostSchema' responses: 201: description: The value was stored successfully. @@ -126,8 +125,7 @@ def put(self, pk: int, key: str) -> Response: content: application/json: schema: - type: object - $ref: '#/components/schemas/KeyValuePutSchema' + $ref: '#/components/schemas/KeyValuePutSchema' responses: 200: description: The value was stored successfully. diff --git a/superset/charts/form_data/commands/create.py b/superset/charts/form_data/commands/create.py index 68183359a75d..b0ba9728b2aa 100644 --- a/superset/charts/form_data/commands/create.py +++ b/superset/charts/form_data/commands/create.py @@ -24,12 +24,14 @@ class CreateFormDataCommand(CreateKeyValueCommand): def create(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] - value = cmd_params["value"] - entry: Entry = {"owner": actor.get_user_id(), "value": value} + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key + value = cmd_params.value check_access(cmd_params) - return cache_manager.chart_form_data_cache.set( - cache_key(resource_id, key), entry - ) + if value: + entry: Entry = {"owner": actor.get_user_id(), "value": value} + return cache_manager.chart_form_data_cache.set( + cache_key(resource_id, key), entry + ) + return False diff --git a/superset/charts/form_data/commands/delete.py b/superset/charts/form_data/commands/delete.py index cf4e7efc0244..2f4d18ab99f0 100644 --- a/superset/charts/form_data/commands/delete.py +++ b/superset/charts/form_data/commands/delete.py @@ -25,9 +25,9 @@ class DeleteFormDataCommand(DeleteKeyValueCommand): def delete(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key check_access(cmd_params) entry: Entry = cache_manager.chart_form_data_cache.get( cache_key(resource_id, key) diff --git a/superset/charts/form_data/commands/get.py b/superset/charts/form_data/commands/get.py index c97c4d8e59eb..4cee65b41c3c 100644 --- a/superset/charts/form_data/commands/get.py +++ b/superset/charts/form_data/commands/get.py @@ -27,17 +27,20 @@ class GetFormDataCommand(GetKeyValueCommand): - def get(self, cmd_params: CommandParameters) -> Optional[str]: - resource_id = cmd_params["resource_id"] - key = cmd_params["key"] + def __init__(self, cmd_params: CommandParameters) -> None: + super().__init__(cmd_params) config = app.config["CHART_FORM_DATA_CACHE_CONFIG"] - refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + + def get(self, cmd_params: CommandParameters) -> Optional[str]: + resource_id = cmd_params.resource_id + key = cmd_params.key check_access(cmd_params) entry: Entry = cache_manager.chart_form_data_cache.get( cache_key(resource_id, key) ) if entry: - if refresh_timeout: + 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 index 7f53a35c1523..5f59e728d4bd 100644 --- a/superset/charts/form_data/commands/update.py +++ b/superset/charts/form_data/commands/update.py @@ -25,15 +25,15 @@ class UpdateFormDataCommand(UpdateKeyValueCommand): def update(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] - value = cmd_params["value"] + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key + value = cmd_params.value check_access(cmd_params) entry: Entry = cache_manager.chart_form_data_cache.get( cache_key(resource_id, key) ) - if entry: + if entry and value: user_id = actor.get_user_id() if entry["owner"] != user_id: raise KeyValueAccessDeniedError() diff --git a/superset/charts/form_data/utils.py b/superset/charts/form_data/utils.py index b27261814fc9..fb7ff0ba901b 100644 --- a/superset/charts/form_data/utils.py +++ b/superset/charts/form_data/utils.py @@ -33,9 +33,9 @@ def check_access(parameters: CommandParameters) -> Optional[bool]: - resource_id = parameters["resource_id"] - actor = parameters["actor"] - query_params = parameters["query_params"] + resource_id = parameters.resource_id + actor = parameters.actor + query_params = parameters.query_params if resource_id == 0: if parameters: dataset_id = query_params.get("dataset_id") diff --git a/superset/dashboards/filter_state/commands/create.py b/superset/dashboards/filter_state/commands/create.py index d476a49f7bd8..64ff7c07d25a 100644 --- a/superset/dashboards/filter_state/commands/create.py +++ b/superset/dashboards/filter_state/commands/create.py @@ -24,12 +24,12 @@ class CreateFilterStateCommand(CreateKeyValueCommand): def create(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] - value = cmd_params["value"] + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key + value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) - if dashboard: + if dashboard and value: entry: Entry = {"owner": actor.get_user_id(), "value": value} return cache_manager.filter_state_cache.set( cache_key(resource_id, key), entry diff --git a/superset/dashboards/filter_state/commands/delete.py b/superset/dashboards/filter_state/commands/delete.py index 9998ba2afc85..99368990c956 100644 --- a/superset/dashboards/filter_state/commands/delete.py +++ b/superset/dashboards/filter_state/commands/delete.py @@ -25,9 +25,9 @@ class DeleteFilterStateCommand(DeleteKeyValueCommand): def delete(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) if dashboard: entry: Entry = cache_manager.filter_state_cache.get( diff --git a/superset/dashboards/filter_state/commands/get.py b/superset/dashboards/filter_state/commands/get.py index d56ab5f9abaa..379003d4ee64 100644 --- a/superset/dashboards/filter_state/commands/get.py +++ b/superset/dashboards/filter_state/commands/get.py @@ -16,12 +16,6 @@ # under the License. from typing import Optional - -from superset.dashboards.dao import DashboardDAO -from superset.extensions import cache_manager - -from superset.dashboards.dao import DashboardDAO -from superset.extensions import cache_manager from flask import current_app as app from superset.dashboards.dao import DashboardDAO @@ -32,13 +26,16 @@ class GetFilterStateCommand(GetKeyValueCommand): - def get(self, cmd_params: CommandParameters) -> Optional[str]: - resource_id = cmd_params["resource_id"] - key = cmd_params["key"] + def __init__(self, cmd_params: CommandParameters) -> None: + super().__init__(cmd_params) config = app.config["FILTER_STATE_CACHE_CONFIG"] - refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + self._refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL") + + def get(self, cmd_params: CommandParameters) -> Optional[str]: + resource_id = cmd_params.resource_id + key = cmd_params.key DashboardDAO.get_by_id_or_slug(str(resource_id)) entry = cache_manager.filter_state_cache.get(cache_key(resource_id, key)) or {} - if entry and refresh_timeout: + if entry and self._refresh_timeout: cache_manager.filter_state_cache.set(key, entry) return entry.get("value") diff --git a/superset/dashboards/filter_state/commands/update.py b/superset/dashboards/filter_state/commands/update.py index 0be675fa993f..3e4d93956c44 100644 --- a/superset/dashboards/filter_state/commands/update.py +++ b/superset/dashboards/filter_state/commands/update.py @@ -25,12 +25,12 @@ class UpdateFilterStateCommand(UpdateKeyValueCommand): def update(self, cmd_params: CommandParameters) -> bool: - resource_id = cmd_params["resource_id"] - actor = cmd_params["actor"] - key = cmd_params["key"] - value = cmd_params["value"] + resource_id = cmd_params.resource_id + actor = cmd_params.actor + key = cmd_params.key + value = cmd_params.value dashboard = DashboardDAO.get_by_id_or_slug(str(resource_id)) - if dashboard: + if dashboard and value: entry: Entry = cache_manager.filter_state_cache.get( cache_key(resource_id, key) ) diff --git a/superset/key_value/api.py b/superset/key_value/api.py index a110a223a241..85aa6e374642 100644 --- a/superset/key_value/api.py +++ b/superset/key_value/api.py @@ -74,12 +74,12 @@ def post(self, pk: int) -> Response: raise InvalidPayloadFormatError("Request is not JSON") try: item = self.add_model_schema.load(request.json) - args: CommandParameters = { - "actor": g.user, - "resource_id": pk, - "value": item["value"], - "query_params": request.args, - } + args = CommandParameters( + actor=g.user, + resource_id=pk, + value=item["value"], + query_params=request.args, + ) key = self.get_create_command()(args).run() return self.response(201, key=key) except ValidationError as ex: @@ -99,13 +99,13 @@ def put(self, pk: int, key: str) -> Response: raise InvalidPayloadFormatError("Request is not JSON") try: item = self.edit_model_schema.load(request.json) - args: CommandParameters = { - "actor": g.user, - "resource_id": pk, - "key": key, - "value": item["value"], - "query_params": request.args, - } + args = CommandParameters( + actor=g.user, + resource_id=pk, + key=key, + value=item["value"], + query_params=request.args, + ) result = self.get_update_command()(args).run() if not result: return self.response_404() @@ -124,12 +124,9 @@ def put(self, pk: int, key: str) -> Response: def get(self, pk: int, key: str) -> Response: try: - args: CommandParameters = { - "actor": g.user, - "resource_id": pk, - "key": key, - "query_params": request.args, - } + args = CommandParameters( + actor=g.user, resource_id=pk, key=key, query_params=request.args + ) value = self.get_get_command()(args).run() if not value: return self.response_404() @@ -146,12 +143,9 @@ def get(self, pk: int, key: str) -> Response: def delete(self, pk: int, key: str) -> Response: try: - args: CommandParameters = { - "actor": g.user, - "resource_id": pk, - "key": key, - "query_params": request.args, - } + args = CommandParameters( + actor=g.user, resource_id=pk, key=key, query_params=request.args + ) result = self.get_delete_command()(args).run() if not result: return self.response_404() diff --git a/superset/key_value/commands/create.py b/superset/key_value/commands/create.py index 94672e080cf2..8a7eec9298c0 100644 --- a/superset/key_value/commands/create.py +++ b/superset/key_value/commands/create.py @@ -29,13 +29,13 @@ class CreateKeyValueCommand(BaseCommand, ABC): def __init__(self, cmd_params: CommandParameters): - self._parameters = cmd_params + self._cmd_params = cmd_params def run(self) -> str: try: key = token_urlsafe(48) - self._parameters["key"] = key - self.create(self._parameters) + self._cmd_params.key = key + self.create(self._cmd_params) return key except SQLAlchemyError as ex: logger.exception("Error running create command") diff --git a/superset/key_value/commands/delete.py b/superset/key_value/commands/delete.py index 694ea3a1a790..6d2983c06378 100644 --- a/superset/key_value/commands/delete.py +++ b/superset/key_value/commands/delete.py @@ -28,11 +28,11 @@ class DeleteKeyValueCommand(BaseCommand, ABC): def __init__(self, cmd_params: CommandParameters): - self._parameters = cmd_params + self._cmd_params = cmd_params def run(self) -> bool: try: - return self.delete(self._parameters) + return self.delete(self._cmd_params) except SQLAlchemyError as ex: logger.exception("Error running delete command") raise KeyValueDeleteFailedError() from ex diff --git a/superset/key_value/commands/get.py b/superset/key_value/commands/get.py index 48eefb7fc03a..a697c808c01d 100644 --- a/superset/key_value/commands/get.py +++ b/superset/key_value/commands/get.py @@ -29,11 +29,11 @@ class GetKeyValueCommand(BaseCommand, ABC): def __init__(self, cmd_params: CommandParameters): - self._parameters = cmd_params + self._cmd_params = cmd_params def run(self) -> Optional[str]: try: - return self.get(self._parameters) + return self.get(self._cmd_params) except SQLAlchemyError as ex: logger.exception("Error running get command") raise KeyValueGetFailedError() from ex diff --git a/superset/key_value/commands/parameters.py b/superset/key_value/commands/parameters.py index 6697034859b6..00a933c67c71 100644 --- a/superset/key_value/commands/parameters.py +++ b/superset/key_value/commands/parameters.py @@ -14,15 +14,16 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from typing import Dict +from dataclasses import dataclass +from typing import Dict, Optional from flask_appbuilder.security.sqla.models import User -from typing_extensions import TypedDict -class CommandParameters(TypedDict, total=False): +@dataclass +class CommandParameters: actor: User resource_id: int - key: str - value: str query_params: Dict[str, str] + key: Optional[str] = None + value: Optional[str] = None diff --git a/tests/integration_tests/charts/form_data/api_tests.py b/tests/integration_tests/charts/form_data/api_tests.py index 478f2b606db1..f3de43ad84a3 100644 --- a/tests/integration_tests/charts/form_data/api_tests.py +++ b/tests/integration_tests/charts/form_data/api_tests.py @@ -167,7 +167,11 @@ def test_delete_access_denied(client, chart_id: int): assert resp.status_code == 404 -def test_delete_not_owner(client, chart_id: int): - login(client, "gamma") - resp = client.delete(f"api/v1/chart/{chart_id}/form_data/{key}/") - assert resp.status_code == 404 +def test_delete_not_owner(client, chart_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) + login(client, "admin") + resp = client.delete(f"api/v1/chart/{chart_id}/form_data/{another_key}/") + assert resp.status_code == 403