aminghadersohi commented on code in PR #44549:
URL: https://github.com/apache/superset/pull/44549#discussion_r4270451394
##########
superset/utils/schema.py:
##########
@@ -56,6 +57,63 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
raise ValidationError("JSON not valid") from ex
+def is_query_context_metadata_complete(value: Any) -> bool:
+ """
+ Whether a parsed ``query_context`` payload carries the ``datasource`` and
+ ``queries`` keys ``QueryContextFactory.create()`` requires (apache/superset
+ #35774): both are required keyword-only arguments there, and a chart saved
+ without them fails every subsequent read with a raw ``TypeError`` instead
+ of a clear error at save time. ``datasource`` must be a mapping carrying
+ the ``id``/``type`` keys ``DatasourceDict`` requires --
``_convert_to_model``
+ indexes both directly and raises ``KeyError`` if either is absent, and
+ passes ``type`` straight to ``DatasourceType(...)``, which raises
+ ``ValueError`` for a value outside that enum -- but ``queries`` may
+ legitimately be an empty list (a chart with no query objects yet still
+ round-trips through ``create()`` without error), so it is only required
+ to be a list of mappings, not to be non-empty.
+
+ :param value: the object obtained by JSON-decoding a query_context string
+ """
+ if not isinstance(value, dict) or not isinstance(value.get("queries"),
list):
+ return False
+ if not all(isinstance(query, dict) for query in value["queries"]):
+ return False
+ datasource = value.get("datasource")
+ if not isinstance(datasource, dict) or not datasource.get("id"):
+ return False
+ datasource_type = datasource.get("type")
+ if not isinstance(datasource_type, str):
+ return False
+ try:
+ DatasourceType(datasource_type)
+ except ValueError:
+ return False
+ return True
+
+
+def validate_query_context_metadata(value: Union[bytes, bytearray, str]) ->
None:
+ """
+ Validator for a chart's ``query_context`` field: beyond being well-formed
+ JSON (see ``validate_json``), it must be a JSON object carrying a
+ non-empty ``datasource`` key and a ``queries`` key that is a list
+ (apache/superset#35774).
+
+ :raises ValidationError: if value is not valid JSON, or is valid JSON that
+ is not an object with a non-empty ``datasource`` key and a ``queries``
+ key holding a list
+ :param value: an object that should be parseable to a query_context JSON
object
+ """
+ try:
+ parsed = json.loads(value)
+ except json.JSONDecodeError as ex:
+ raise ValidationError("JSON not valid") from ex
+ if not is_query_context_metadata_complete(parsed):
+ raise ValidationError(
+ "query_context must be a JSON object with a non-empty "
+ "'datasource' key and a 'queries' key holding a list"
+ )
Review Comment:
Nit: `{"queries": ["x"]}`, or a datasource without `id` or with an unknown
`type`, is rejected with "...a 'queries' key holding a list", which that
payload already satisfies, so the caller cannot tell what to fix.
```suggestion
raise ValidationError(
"query_context must be a JSON object with a 'datasource' object "
"carrying a valid 'id' and 'type', and a 'queries' list of
objects"
)
```
##########
superset/utils/schema.py:
##########
@@ -56,6 +57,63 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
raise ValidationError("JSON not valid") from ex
+def is_query_context_metadata_complete(value: Any) -> bool:
+ """
+ Whether a parsed ``query_context`` payload carries the ``datasource`` and
+ ``queries`` keys ``QueryContextFactory.create()`` requires (apache/superset
+ #35774): both are required keyword-only arguments there, and a chart saved
+ without them fails every subsequent read with a raw ``TypeError`` instead
+ of a clear error at save time. ``datasource`` must be a mapping carrying
+ the ``id``/``type`` keys ``DatasourceDict`` requires --
``_convert_to_model``
+ indexes both directly and raises ``KeyError`` if either is absent, and
+ passes ``type`` straight to ``DatasourceType(...)``, which raises
+ ``ValueError`` for a value outside that enum -- but ``queries`` may
+ legitimately be an empty list (a chart with no query objects yet still
+ round-trips through ``create()`` without error), so it is only required
+ to be a list of mappings, not to be non-empty.
+
+ :param value: the object obtained by JSON-decoding a query_context string
+ """
+ if not isinstance(value, dict) or not isinstance(value.get("queries"),
list):
+ return False
+ if not all(isinstance(query, dict) for query in value["queries"]):
+ return False
+ datasource = value.get("datasource")
+ if not isinstance(datasource, dict) or not datasource.get("id"):
+ return False
+ datasource_type = datasource.get("type")
+ if not isinstance(datasource_type, str):
+ return False
+ try:
+ DatasourceType(datasource_type)
+ except ValueError:
+ return False
+ return True
+
+
+def validate_query_context_metadata(value: Union[bytes, bytearray, str]) ->
None:
+ """
+ Validator for a chart's ``query_context`` field: beyond being well-formed
+ JSON (see ``validate_json``), it must be a JSON object carrying a
+ non-empty ``datasource`` key and a ``queries`` key that is a list
+ (apache/superset#35774).
+
+ :raises ValidationError: if value is not valid JSON, or is valid JSON that
+ is not an object with a non-empty ``datasource`` key and a ``queries``
+ key holding a list
+ :param value: an object that should be parseable to a query_context JSON
object
+ """
+ try:
+ parsed = json.loads(value)
+ except json.JSONDecodeError as ex:
+ raise ValidationError("JSON not valid") from ex
Review Comment:
`validate_json` let `""` through (falsy skip), so `query_context: ""` was
accepted on POST/PUT and read back as absent. This re-parses it and now 400s
with "JSON not valid", e.g. when re-posting a chart whose stored context is
`""`.
```suggestion
if not value:
return
try:
parsed = json.loads(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
```
--
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]
