Changes the parameters to use dataclass

apache · Jan 10, 2022 · 07e06e5 · 07e06e5
1 parent 203fb6d
commit 07e06e5
Show file tree

Hide file tree

Showing 16 changed files with 92 additions and 93 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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