[GitHub] [superset] villebro commented on a change in pull request #17882: feat: Adds a key-value endpoint to store charts form data

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #17882:
URL: https://github.com/apache/superset/pull/17882#discussion_r777818380



##########
File path: superset/charts/form_data/commands/create.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 typing import Optional
+
+from superset.charts.form_data.utils import check_access
+from superset.extensions import cache_manager
+from superset.key_value.commands.args import Args
+from superset.key_value.commands.create import CreateKeyValueCommand
+from superset.key_value.commands.entry import Entry
+from superset.key_value.utils import cache_key
+
+
+class CreateFormDataCommand(CreateKeyValueCommand):
+    def create(self, args: Args,) -> Optional[bool]:

Review comment:
       nit (not sure why black doesn't handle these trailing commas on single lines correctly):
   ```suggestion
       def create(self, args: Args) -> Optional[bool]:
   ```

##########
File path: tests/integration_tests/charts/form_data/api_tests.py
##########
@@ -0,0 +1,173 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from sqlalchemy.orm import Session
+
+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 (
+    load_world_bank_dashboard_with_slices,
+    load_world_bank_data,
+)
+from tests.integration_tests.test_app import app
+
+key = "test-key"
+value = "test"
+
+
+@pytest.fixture
+def client():
+    with app.test_client() as client:
+        with app.app_context():
+            yield client
+
+
+@pytest.fixture
+def chart_id(load_world_bank_dashboard_with_slices) -> int:
+    with app.app_context() as ctx:
+        session: Session = ctx.app.appbuilder.get_session
+        chart = session.query(Slice).filter_by(slice_name="World's Population").one()
+        return chart.id
+
+
+@pytest.fixture
+def admin_id() -> int:
+    with app.app_context() as ctx:
+        session: Session = ctx.app.appbuilder.get_session
+        admin = session.query(User).filter_by(username="admin").one()
+        return admin.id
+
+
+@pytest.fixture(autouse=True)
+def cache(chart_id, admin_id):
+    app.config["FILTER_STATE_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)
+
+
+def test_post(client, chart_id: int):
+    login(client, "admin")
+    payload = {
+        "value": value,
+    }
+    resp = client.post(f"api/v1/chart/{chart_id}/form_data", json=payload)
+    assert resp.status_code == 201
+
+
+def test_post_bad_request(client, chart_id: int):
+    login(client, "admin")
+    payload = {
+        "value": 1234,
+    }
+    resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}/", json=payload)
+    assert resp.status_code == 400
+
+
+def test_post_access_denied(client, chart_id: int):
+    login(client, "gamma")
+    payload = {
+        "value": value,
+    }
+    resp = client.post(f"api/v1/chart/{chart_id}/form_data", json=payload)
+    assert resp.status_code == 404
+
+
+def test_put(client, chart_id: int):
+    login(client, "admin")
+    payload = {
+        "value": "new value",
+    }
+    resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}/", json=payload)
+    assert resp.status_code == 200
+
+
+def test_put_bad_request(client, chart_id: int):
+    login(client, "admin")
+    payload = {
+        "value": 1234,
+    }
+    resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}/", json=payload)
+    assert resp.status_code == 400
+
+
+def test_put_access_denied(client, chart_id: int):
+    login(client, "gamma")
+    payload = {
+        "value": "new value",
+    }
+    resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}/", json=payload)
+    assert resp.status_code == 404
+
+
+def test_put_not_owner(client, chart_id: int):
+    login(client, "gamma")
+    payload = {
+        "value": "new value",
+    }
+    resp = client.put(f"api/v1/chart/{chart_id}/form_data/{key}/", json=payload)
+    assert resp.status_code == 404
+
+
+def test_get_key_not_found(client, chart_id: int):
+    login(client, "admin")
+    resp = client.get(f"api/v1/chart/{chart_id}/form_data/unknown-key/")
+    assert resp.status_code == 404
+
+
+def test_get_chart_not_found(client):
+    login(client, "admin")
+    resp = client.get(f"api/v1/chart/{-1}/form_data/{key}/")

Review comment:
       nit:
   ```suggestion
       resp = client.get(f"api/v1/chart/-1/form_data/{key}/")
   ```

##########
File path: superset/key_value/commands/create.py
##########
@@ -50,7 +47,5 @@ def validate(self) -> None:
         pass
 
     @abstractmethod
-    def create(
-        self, actor: User, resource_id: int, key: str, value: str
-    ) -> Optional[bool]:
+    def create(self, args: Args,) -> Optional[bool]:

Review comment:
       Bycatch - shouldn't the return type here be `bool`, not `Optional[bool]`?

##########
File path: superset/key_value/commands/args.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 typing import Dict
+
+from flask_appbuilder.security.sqla.models import User
+from typing_extensions import TypedDict
+
+
+class Args(TypedDict, total=False):

Review comment:
       Could we make the name more explicit, like `KeyValueArgs`? Also, usually `args` refers to a list of values, while `kwargs` refers to a dictionary, so maybe `Args` isn't the best name for this, as it's maybe more like an "entry", which makes it slightly ambiguous with the `Entry` class. So maybe we could make these more unambiguous and clarify their relative roles?

##########
File path: tests/integration_tests/charts/form_data/api_tests.py
##########
@@ -0,0 +1,173 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from sqlalchemy.orm import Session
+
+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 (
+    load_world_bank_dashboard_with_slices,
+    load_world_bank_data,
+)
+from tests.integration_tests.test_app import app
+
+key = "test-key"
+value = "test"
+
+
+@pytest.fixture
+def client():
+    with app.test_client() as client:
+        with app.app_context():
+            yield client
+
+
+@pytest.fixture
+def chart_id(load_world_bank_dashboard_with_slices) -> int:
+    with app.app_context() as ctx:
+        session: Session = ctx.app.appbuilder.get_session
+        chart = session.query(Slice).filter_by(slice_name="World's Population").one()
+        return chart.id
+
+
+@pytest.fixture
+def admin_id() -> int:
+    with app.app_context() as ctx:
+        session: Session = ctx.app.appbuilder.get_session
+        admin = session.query(User).filter_by(username="admin").one()
+        return admin.id
+
+
+@pytest.fixture(autouse=True)
+def cache(chart_id, admin_id):
+    app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"}

Review comment:
       Shouldn't this be `
   ```suggestion
       app.config["CHART_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "SimpleCache"}
   ```

##########
File path: superset/key_value/commands/get.py
##########
@@ -20,26 +20,24 @@
 
 from flask import current_app as app
 from flask_appbuilder.models.sqla import Model
-from flask_appbuilder.security.sqla.models import User
 from sqlalchemy.exc import SQLAlchemyError
 
 from superset.commands.base import BaseCommand
+from superset.key_value.commands.args import Args
 from superset.key_value.commands.exceptions import KeyValueGetFailedError
 
 logger = logging.getLogger(__name__)
 
 
 class GetKeyValueCommand(BaseCommand, ABC):
-    def __init__(self, actor: User, resource_id: int, key: str):
-        self._actor = actor
-        self._resource_id = resource_id
-        self._key = key
+    def __init__(self, args: Args):
+        self._args = args
 
     def run(self) -> Model:
         try:
             config = app.config["FILTER_STATE_CACHE_CONFIG"]
-            refresh_timeout = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL")
-            return self.get(self._resource_id, self._key, refresh_timeout)
+            self._args["refresh_timeout"] = config.get("REFRESH_TIMEOUT_ON_RETRIEVAL")
+            return self.get(self._args)

Review comment:
       Bycatch - we probably can't be referencing `app.config["FILTER_STATE_CACHE_CONFIG"]` on line 38 here. Maybe we need to pass the cache config key to `self._args` as it will be different for each kv implementation?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org