rusackas commented on code in PR #40955:
URL: https://github.com/apache/superset/pull/40955#discussion_r3408120301
##########
superset/commands/chart/update.py:
##########
@@ -101,6 +103,51 @@ def _validate_new_dashboard_access(
if not security_manager.is_owner(dash):
raise DashboardsForbiddenError()
+ def _validate_query_context_datasource(
+ self, exceptions: list[ValidationError]
+ ) -> None:
+ """
+ Ensure a query-context-only update keeps the chart's own datasource.
+
+ The submitted query context is only verified when it carries a
parseable
+ ``datasource`` object; a payload that references a different
datasource than
+ the chart's persisted one is rejected. Payloads without a datasource
fall
+ back to the chart's datasource at execution time and need no check.
+ """
+ if not self._model:
+ return
+
+ raw_query_context = self._properties.get("query_context")
+ if not raw_query_context:
+ return
+
+ try:
+ query_context = json.loads(raw_query_context)
+ except (TypeError, ValueError):
+ # An unparseable payload cannot be verified or replayed; leave it
for
+ # downstream handling rather than guessing at its intent.
+ return
+
+ datasource = (
+ query_context.get("datasource") if isinstance(query_context, dict)
else None
+ )
+ if not isinstance(datasource, dict):
+ return
+
+ try:
+ ids_match = int(datasource["id"]) == self._model.datasource_id
+ except (KeyError, TypeError, ValueError):
+ ids_match = False
+
+ datasource_type = datasource.get("type")
+ types_match = (
+ datasource_type is None
+ or str(datasource_type) == self._model.datasource_type
+ )
Review Comment:
Good catch - `_convert_to_model` reads `datasource["type"]` directly, so an
id-only payload would blow up on replay. Now requiring `type` to be present and
match.
##########
tests/unit_tests/commands/chart/update_test.py:
##########
@@ -151,3 +152,76 @@ def test_update_chart_owner_can_perform_regular_update(
find_by_id.assert_called_once_with(1)
raise_for_ownership.assert_called_once()
+
+
+def _query_context_payload(datasource: object) -> dict[str, object]:
+ return {
+ "query_context": json.dumps({"datasource": datasource, "queries": []}),
+ "query_context_generation": True,
+ }
Review Comment:
Added a docstring to the helper.
##########
superset/commands/chart/exceptions.py:
##########
@@ -103,6 +103,19 @@ def __init__(self) -> None:
)
+class ChartQueryContextDatasourceMismatchValidationError(ValidationError):
+ """
+ Raised when a query-context-only update carries a datasource that does not
+ match the chart's own datasource.
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ _("The query context datasource does not match the chart
datasource"),
+ field_name="query_context",
+ )
Review Comment:
The class itself already has a docstring covering this, and the `__init__`
just forwards to `super()` with no behavior of its own, so a second docstring
there would be noise. Leaving as-is.
--
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]
