villebro commented on a change in pull request #17536:
URL: https://github.com/apache/superset/pull/17536#discussion_r759377257
##########
File path: superset/config.py
##########
@@ -560,6 +560,9 @@ def _try_json_readsha(filepath: str, length: int) ->
Optional[str]:
# Cache for datasource metadata and query results
DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"}
+# Cache for filters state
+FILTER_STATE_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "null"}
Review comment:
Do you think we should default to using a file system cache? Something
like
```python
FILTER_STATE_CACHE_CONFIG = {
"CACHE_TYPE": "FileSystemCache",
"CACHE_DEFAULT_TIMEOUT": 60 * 60 * 24 * 30, # 30 days
"CACHE_DIR": DATA_DIR,
}
```
With the current null cache, saving values will appear to save the result,
but the saved value won't be retrievable. This will become a blocker at least
when the kv-store will be used to store explore form data state, and will
require an additional configuration step before being able to run the
application.
Another alternative would be to define a default file system config for the
key-value stores that will act as a fallback if a specific config hasn't been
provided, much like we do for the metadata database, which defaults to using
sqlite in `DATA_DIR`. We could also issue a warning during startup to notify
the admin that a dedicated config is recommended.
##########
File path: superset/key_value/api.py
##########
@@ -0,0 +1,119 @@
+# 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, abstractmethod
+from typing import Any
+
+from apispec import APISpec
+from flask import g, request, Response
+from flask_appbuilder.api import BaseApi
+
+from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
+from superset.dashboards.commands.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+)
+from superset.exceptions import InvalidPayloadFormatError
+from superset.key_value.schemas import KeyValuePostSchema, KeyValuePutSchema
+
+logger = logging.getLogger(__name__)
+
+
+class KeyValueRestApi(BaseApi, ABC):
+ add_model_schema = KeyValuePostSchema()
+ edit_model_schema = KeyValuePutSchema()
+ method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ include_route_methods = {
+ RouteMethod.POST,
+ RouteMethod.PUT,
+ RouteMethod.GET,
+ RouteMethod.DELETE,
+ }
+ allow_browser_login = True
+
+ def add_apispec_components(self, api_spec: APISpec) -> None:
+ api_spec.components.schema(
+ KeyValuePostSchema.__name__, schema=KeyValuePostSchema,
+ )
+ api_spec.components.schema(
+ KeyValuePutSchema.__name__, schema=KeyValuePutSchema,
+ )
+ super().add_apispec_components(api_spec)
+
+ def post(self, pk: int) -> Response:
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ key = self.get_create_command()(g.user, pk, item["value"]).run()
+ return self.response(201, key=key)
+ except DashboardAccessDeniedError:
+ return self.response_403()
+ except DashboardNotFoundError:
+ return self.response_404()
Review comment:
I noticed that when trying to call the POST endpoint with a non-string
value, I got a 500. I think we may need to add something along these lines here
and on the PUT (I copy-pasted this from `dashboards/api.py`):
```python
from marshmallow.exceptions import ValidationError
...
try:
item = self.add_model_schema.load(request.json)
key = self.get_create_command()(g.user, pk, item["value"]).run()
return self.response(201, key=key)
except ValidationError as error:
return self.response_400(message=error.messages)
....
```
An integration test to confirm that a correct response is returned for both
endpoints for invalid value types would be a nice addition.
##########
File path: superset/dashboards/filter_state/api.py
##########
@@ -0,0 +1,241 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import logging
+from typing import Type
+
+from flask import Response
+from flask_appbuilder.api import expose, protect, safe
+
+from superset.dashboards.filter_state.commands.create import
CreateFilterStateCommand
+from superset.dashboards.filter_state.commands.delete import
DeleteFilterStateCommand
+from superset.dashboards.filter_state.commands.get import GetFilterStateCommand
+from superset.dashboards.filter_state.commands.update import
UpdateFilterStateCommand
+from superset.extensions import event_logger
+from superset.key_value.api import KeyValueRestApi
+
+logger = logging.getLogger(__name__)
+
+
+class DashboardFilterStateRestApi(KeyValueRestApi):
+ class_permission_name = "FilterStateRestApi"
+ resource_name = "dashboard"
+ openapi_spec_tag = "Filter State"
+
+ def get_create_command(self) -> Type[CreateFilterStateCommand]:
+ return CreateFilterStateCommand
+
+ def get_update_command(self) -> Type[UpdateFilterStateCommand]:
+ return UpdateFilterStateCommand
+
+ def get_get_command(self) -> Type[GetFilterStateCommand]:
+ return GetFilterStateCommand
+
+ def get_delete_command(self) -> Type[DeleteFilterStateCommand]:
+ return DeleteFilterStateCommand
+
+ @expose("/<int:pk>/filter_state", methods=["POST"])
Review comment:
Just a thought, but since it's possible to do the request using either
the id or slug on some dashboard endpoints, it might be worth considering
supporting the slug in this ep:
This would be trivial to support, as we're already using the
`DashboardDAO.get_by_id_or_slug` method to retrieve the relevant dashboard in
all commands.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
