Copilot commented on code in PR #39891:
URL: https://github.com/apache/superset/pull/39891#discussion_r3190533289
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -183,6 +194,7 @@ def update_chart_preview(
"explore_url": explore_url,
"form_data_key": new_form_data_key,
"previous_form_data_key": request.form_data_key, # For reference
+ "warnings": warnings,
"api_endpoints": {}, # No API endpoints for unsaved charts
Review Comment:
Now that `warnings` is part of the top-level success payload, the response
shape is less consistent across return paths (e.g., early error returns like
the missing-`form_data_key` branch won’t include `warnings`). If clients are
expected to rely on this field, consider including `warnings: []` in
non-success returns as well to keep the schema stable.
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:
##########
@@ -472,3 +480,165 @@ async def test_update_chart_preview_table_sorting(self):
)
assert request.config.sort_by == ["sales", "profit"]
assert len(request.config.columns) == 3
+
+ @patch("superset.commands.explore.form_data.get.GetFormDataCommand")
+ def test_get_previous_form_data_parses_json_cache_hit(
+ self,
+ mock_get_form_data_command,
+ ) -> None:
+ """Previous form_data lookup parses JSON strings from the cache."""
+ cached_adhoc_filters = [
+ {
+ "clause": "WHERE",
+ "comparator": "North",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "region",
+ }
+ ]
+ mock_get_form_data_command.return_value.run.return_value = (
+ '{"adhoc_filters": ['
+ '{"clause": "WHERE", "comparator": "North", '
+ '"expressionType": "SIMPLE", "operator": "==", "subject":
"region"}'
+ '], "viz_type": "table"}'
+ )
+
+ result =
update_chart_preview_module._get_previous_form_data("valid_key_12345")
+
+ assert result == {
+ "adhoc_filters": cached_adhoc_filters,
+ "viz_type": "table",
+ }
+ command_params = mock_get_form_data_command.call_args.args[0]
+ assert command_params.key == "valid_key_12345"
+
+ @patch("superset.commands.explore.form_data.get.GetFormDataCommand")
+ def test_get_previous_form_data_returns_none_for_cache_failure(
+ self,
+ mock_get_form_data_command,
+ ) -> None:
+ """Previous form_data lookup treats command failures as cache
misses."""
+ mock_get_form_data_command.return_value.run.side_effect = (
+ update_chart_preview_module.CommandException("cache read failed")
+ )
Review Comment:
`update_chart_preview_module.CommandException` is not defined/exported by
`superset.mcp_service.chart.tool.update_chart_preview` (the production code
imports `CommandException` inside `_get_previous_form_data`). This will raise
`AttributeError` in the test before the code under test runs. Fix by importing
`CommandException` from `superset.commands.exceptions` in the test and using
that in `side_effect`, or alternatively make `CommandException` available at
module scope in `update_chart_preview.py` and reference it consistently.
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -51,9 +51,15 @@
logger = logging.getLogger(__name__)
+INVALID_FORM_DATA_KEY_WARNING = (
+ "Previous cached chart state could not be loaded from form_data_key. "
+ "The preview was generated from the supplied configuration only; the "
+ "previous form_data_key may be invalid or expired."
Review Comment:
The warning text is slightly ambiguous because it says 'loaded from
form_data_key' (which could be read as the newly generated key) while later
referring to the 'previous form_data_key'. Consider rewording the first
sentence to explicitly say 'from the previous form_data_key' (and optionally
include the key value) to reduce confusion for API consumers.
--
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]
