codeant-ai-for-open-source[bot] commented on code in PR #40853:
URL: https://github.com/apache/superset/pull/40853#discussion_r3396911883
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart.py:
##########
@@ -1344,3 +1345,292 @@ def
test_response_form_data_wraps_sql_metric_strings(self) -> None:
assert "<UNTRUSTED-CONTENT>" in m["sqlExpression"]
assert "<UNTRUSTED-CONTENT>" in m["label"]
assert "<UNTRUSTED-CONTENT>" not in m["optionName"]
+
+
+class TestBuildUpdatePayloadDatasetId:
+ """Tests for dataset_id support in _build_update_payload."""
+
+ def test_dataset_only_update_returns_datasource_fields(self):
+ """dataset_id alone produces a payload with datasource_id +
datasource_type."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42)
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {"datasource_id": 42, "datasource_type": "table"}
+
+ def test_dataset_and_name_update(self):
+ """dataset_id + chart_name: payload includes datasource fields
+ and slice_name."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42,
chart_name="Renamed")
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "Renamed",
+ }
+
+ def test_dataset_and_config_update_includes_datasource(self):
+ """dataset_id + config: payload includes datasource_id and
datasource_type."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=99)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource_id"] == 99
+ assert result["datasource_type"] == "table"
+ assert "params" in result
+ assert "viz_type" in result
+
+ def test_config_without_dataset_does_not_include_datasource(self):
+ """When dataset_id is None, payload must NOT include datasource_id."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert "datasource_id" not in result
+ assert "datasource_type" not in result
+
+
+class TestBuildPreviewFormDataDatasetId:
+ """Tests for dataset_id support in _build_preview_form_data."""
+
+ def test_dataset_only_update_sets_datasource_field(self):
+ """dataset_id alone updates the datasource field in merged
form_data."""
+ request = UpdateChartRequest(identifier=1, dataset_id=55)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "55__table"
+
+ def test_config_and_dataset_uses_new_dataset(self):
+ """config + dataset_id: datasource field reflects the new dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=77)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "77__table"
+
+ def test_no_dataset_keeps_existing_datasource(self):
+ """When dataset_id is None, datasource reflects the existing chart
dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "10__table"
+
+
+class TestUpdateChartDatasetIdIntegration:
+ """Integration test verifying dataset_id is plumbed into
UpdateChartCommand."""
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch(
+ "superset.commands.chart.update.UpdateChartCommand",
+ new_callable=Mock,
+ )
+ @patch(
+
"superset.mcp_service.chart.tool.update_chart._validate_update_against_dataset",
+ return_value=None,
+ )
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_id_passed_to_update_command(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_validate,
+ mock_update_cmd_cls,
+ mock_check_access,
+ mcp_server,
+ ):
+ """dataset_id in request is forwarded to UpdateChartCommand payload."""
+ mock_chart = Mock()
+ mock_chart.id = 55
+ mock_chart.datasource_id = 10
+ mock_chart.slice_name = "Old Chart"
+ mock_chart.viz_type = "table"
+ mock_chart.uuid = "uuid-55"
+ mock_find_by_id.return_value = mock_chart
+
+ mock_check_access.return_value = DatasetValidationResult(
+ is_valid=True,
+ dataset_id=10,
+ dataset_name="old_dataset",
+ warnings=[],
+ )
+
+ updated_chart = Mock()
+ updated_chart.id = 55
+ updated_chart.slice_name = "Old Chart"
+ updated_chart.viz_type = "table"
+ updated_chart.uuid = "uuid-55"
+ mock_update_cmd_cls.return_value.run.return_value = updated_chart
+
+ request = {
+ "identifier": 55,
+ "dataset_id": 1041,
+ "generate_preview": False,
+ }
+
+ async with Client(mcp) as client:
+ result = await client.call_tool("update_chart", {"request":
request})
+
+ assert result.structured_content["success"] is True
+
+ call_args = mock_update_cmd_cls.call_args
+ payload = call_args[0][1]
+ assert payload.get("datasource_id") == 1041
+ assert payload.get("datasource_type") == "table"
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id", new_callable=Mock)
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_only_rebind_invalid_dataset_returns_error(
+ self,
+ mock_db_session,
+ mock_chart_find,
+ mock_dataset_find,
+ mock_check_access,
+ mcp_server,
+ ):
+ """dataset_id pointing to a non-existent dataset returns
+ DatasetNotAccessible."""
+ mock_chart = Mock()
+ mock_chart.id = 55
+ mock_chart.datasource_id = 10
+ mock_chart.slice_name = "Old Chart"
+ mock_chart.viz_type = "table"
+ mock_chart.uuid = "uuid-55"
+ mock_chart_find.return_value = mock_chart
+
+ mock_check_access.return_value = DatasetValidationResult(
+ is_valid=True,
+ dataset_id=10,
+ dataset_name="old_dataset",
+ warnings=[],
+ )
+
+ # Target dataset does not exist
+ mock_dataset_find.return_value = None
+
+ request = {
+ "identifier": 55,
+ "dataset_id": 9999,
+ "generate_preview": False,
+ }
+
+ async with Client(mcp) as client:
+ result = await client.call_tool("update_chart", {"request":
request})
+
+ assert result.structured_content["success"] is False
+ error_type = result.structured_content["error"]["error_type"]
+ assert error_type == "DatasetNotAccessible"
+ assert "9999" in result.structured_content["error"]["details"]
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id", new_callable=Mock)
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_only_rebind_invalid_dataset_preview_returns_error(
+ self,
+ mock_db_session,
+ mock_chart_find,
+ mock_dataset_find,
+ mock_check_access,
+ mcp_server,
Review Comment:
**Suggestion:** Add explicit annotations for all arguments in this newly
introduced async test and declare `-> None` as the return type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added async method also lacks parameter type hints and a `-> None`
return annotation. The custom rule applies directly here, so the suggestion is
verified.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3088aeefbc2b4b73b349f0d240e8d500&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3088aeefbc2b4b73b349f0d240e8d500&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1595:1601
**Comment:**
*Custom Rule: Add explicit annotations for all arguments in this newly
introduced async test and declare `-> None` as the return type.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=73458149b8426549946be69a4c28fd9559773c97f2df44782e1aedfe2644badb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=73458149b8426549946be69a4c28fd9559773c97f2df44782e1aedfe2644badb&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart.py:
##########
@@ -1344,3 +1345,292 @@ def
test_response_form_data_wraps_sql_metric_strings(self) -> None:
assert "<UNTRUSTED-CONTENT>" in m["sqlExpression"]
assert "<UNTRUSTED-CONTENT>" in m["label"]
assert "<UNTRUSTED-CONTENT>" not in m["optionName"]
+
+
+class TestBuildUpdatePayloadDatasetId:
+ """Tests for dataset_id support in _build_update_payload."""
+
+ def test_dataset_only_update_returns_datasource_fields(self):
+ """dataset_id alone produces a payload with datasource_id +
datasource_type."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42)
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {"datasource_id": 42, "datasource_type": "table"}
+
+ def test_dataset_and_name_update(self):
+ """dataset_id + chart_name: payload includes datasource fields
+ and slice_name."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42,
chart_name="Renamed")
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "Renamed",
+ }
+
+ def test_dataset_and_config_update_includes_datasource(self):
+ """dataset_id + config: payload includes datasource_id and
datasource_type."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=99)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource_id"] == 99
+ assert result["datasource_type"] == "table"
+ assert "params" in result
+ assert "viz_type" in result
+
+ def test_config_without_dataset_does_not_include_datasource(self):
+ """When dataset_id is None, payload must NOT include datasource_id."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert "datasource_id" not in result
+ assert "datasource_type" not in result
+
+
+class TestBuildPreviewFormDataDatasetId:
+ """Tests for dataset_id support in _build_preview_form_data."""
+
+ def test_dataset_only_update_sets_datasource_field(self):
+ """dataset_id alone updates the datasource field in merged
form_data."""
+ request = UpdateChartRequest(identifier=1, dataset_id=55)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "55__table"
+
+ def test_config_and_dataset_uses_new_dataset(self):
+ """config + dataset_id: datasource field reflects the new dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=77)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "77__table"
+
+ def test_no_dataset_keeps_existing_datasource(self):
+ """When dataset_id is None, datasource reflects the existing chart
dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "10__table"
+
+
+class TestUpdateChartDatasetIdIntegration:
+ """Integration test verifying dataset_id is plumbed into
UpdateChartCommand."""
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch(
+ "superset.commands.chart.update.UpdateChartCommand",
+ new_callable=Mock,
+ )
+ @patch(
+
"superset.mcp_service.chart.tool.update_chart._validate_update_against_dataset",
+ return_value=None,
+ )
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_id_passed_to_update_command(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_validate,
+ mock_update_cmd_cls,
+ mock_check_access,
+ mcp_server,
Review Comment:
**Suggestion:** Annotate all parameters in this newly added async test
method (including fixture/mocked arguments) and add an explicit `-> None`
return type. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added async test method has untyped fixture/mocked parameters and no
`-> None` return annotation. The rule explicitly flags new Python methods that
omit type hints.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=45518c538d3e479284281473fda7a12a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=45518c538d3e479284281473fda7a12a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1489:1496
**Comment:**
*Custom Rule: Annotate all parameters in this newly added async test
method (including fixture/mocked arguments) and add an explicit `-> None`
return type.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=bff05e7d9bfb84e2c7c0f1d52264d9669d550a9655e34c1f12053715aaa2dc94&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=bff05e7d9bfb84e2c7c0f1d52264d9669d550a9655e34c1f12053715aaa2dc94&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart.py:
##########
@@ -1344,3 +1345,292 @@ def
test_response_form_data_wraps_sql_metric_strings(self) -> None:
assert "<UNTRUSTED-CONTENT>" in m["sqlExpression"]
assert "<UNTRUSTED-CONTENT>" in m["label"]
assert "<UNTRUSTED-CONTENT>" not in m["optionName"]
+
+
+class TestBuildUpdatePayloadDatasetId:
+ """Tests for dataset_id support in _build_update_payload."""
+
+ def test_dataset_only_update_returns_datasource_fields(self):
+ """dataset_id alone produces a payload with datasource_id +
datasource_type."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42)
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {"datasource_id": 42, "datasource_type": "table"}
+
+ def test_dataset_and_name_update(self):
Review Comment:
**Suggestion:** Add type annotations to this new method signature and
declare a `-> None` return type so the added test code is fully typed.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly introduced test method lacks a return annotation and any
parameter typing beyond `self`. That matches the custom rule requiring newly
added Python methods to be fully typed.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82125b3eb6674b7e818765138cafe225&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=82125b3eb6674b7e818765138cafe225&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1364:1364
**Comment:**
*Custom Rule: Add type annotations to this new method signature and
declare a `-> None` return type so the added test code is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=480f1af31cabdc3b861a61afdc3fd32d7078920ae16bea5c733c00013898cead&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=480f1af31cabdc3b861a61afdc3fd32d7078920ae16bea5c733c00013898cead&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart.py:
##########
@@ -1344,3 +1345,292 @@ def
test_response_form_data_wraps_sql_metric_strings(self) -> None:
assert "<UNTRUSTED-CONTENT>" in m["sqlExpression"]
assert "<UNTRUSTED-CONTENT>" in m["label"]
assert "<UNTRUSTED-CONTENT>" not in m["optionName"]
+
+
+class TestBuildUpdatePayloadDatasetId:
+ """Tests for dataset_id support in _build_update_payload."""
+
+ def test_dataset_only_update_returns_datasource_fields(self):
+ """dataset_id alone produces a payload with datasource_id +
datasource_type."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42)
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {"datasource_id": 42, "datasource_type": "table"}
+
+ def test_dataset_and_name_update(self):
+ """dataset_id + chart_name: payload includes datasource fields
+ and slice_name."""
+ request = UpdateChartRequest(identifier=1, dataset_id=42,
chart_name="Renamed")
+ chart = Mock()
+ chart.datasource_id = 10
+
+ result = _build_update_payload(request, chart)
+
+ assert isinstance(result, dict)
+ assert result == {
+ "datasource_id": 42,
+ "datasource_type": "table",
+ "slice_name": "Renamed",
+ }
+
+ def test_dataset_and_config_update_includes_datasource(self):
+ """dataset_id + config: payload includes datasource_id and
datasource_type."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=99)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource_id"] == 99
+ assert result["datasource_type"] == "table"
+ assert "params" in result
+ assert "viz_type" in result
+
+ def test_config_without_dataset_does_not_include_datasource(self):
+ """When dataset_id is None, payload must NOT include datasource_id."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Old Name"
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert "datasource_id" not in result
+ assert "datasource_type" not in result
+
+
+class TestBuildPreviewFormDataDatasetId:
+ """Tests for dataset_id support in _build_preview_form_data."""
+
+ def test_dataset_only_update_sets_datasource_field(self):
+ """dataset_id alone updates the datasource field in merged
form_data."""
+ request = UpdateChartRequest(identifier=1, dataset_id=55)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "55__table"
+
+ def test_config_and_dataset_uses_new_dataset(self):
+ """config + dataset_id: datasource field reflects the new dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config,
dataset_id=77)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "77__table"
+
+ def test_no_dataset_keeps_existing_datasource(self):
+ """When dataset_id is None, datasource reflects the existing chart
dataset."""
+ config = TableChartConfig(
+ chart_type="table",
+ columns=[ColumnRef(name="col1")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock()
+ chart.datasource_id = 10
+ chart.slice_name = "Chart"
+ chart.id = 1
+ chart.params = None
+
+ result = _build_preview_form_data(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ assert result["datasource"] == "10__table"
+
+
+class TestUpdateChartDatasetIdIntegration:
+ """Integration test verifying dataset_id is plumbed into
UpdateChartCommand."""
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch(
+ "superset.commands.chart.update.UpdateChartCommand",
+ new_callable=Mock,
+ )
+ @patch(
+
"superset.mcp_service.chart.tool.update_chart._validate_update_against_dataset",
+ return_value=None,
+ )
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_id_passed_to_update_command(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_validate,
+ mock_update_cmd_cls,
+ mock_check_access,
+ mcp_server,
+ ):
+ """dataset_id in request is forwarded to UpdateChartCommand payload."""
+ mock_chart = Mock()
+ mock_chart.id = 55
+ mock_chart.datasource_id = 10
+ mock_chart.slice_name = "Old Chart"
+ mock_chart.viz_type = "table"
+ mock_chart.uuid = "uuid-55"
+ mock_find_by_id.return_value = mock_chart
+
+ mock_check_access.return_value = DatasetValidationResult(
+ is_valid=True,
+ dataset_id=10,
+ dataset_name="old_dataset",
+ warnings=[],
+ )
+
+ updated_chart = Mock()
+ updated_chart.id = 55
+ updated_chart.slice_name = "Old Chart"
+ updated_chart.viz_type = "table"
+ updated_chart.uuid = "uuid-55"
+ mock_update_cmd_cls.return_value.run.return_value = updated_chart
+
+ request = {
+ "identifier": 55,
+ "dataset_id": 1041,
+ "generate_preview": False,
+ }
+
+ async with Client(mcp) as client:
+ result = await client.call_tool("update_chart", {"request":
request})
+
+ assert result.structured_content["success"] is True
+
+ call_args = mock_update_cmd_cls.call_args
+ payload = call_args[0][1]
+ assert payload.get("datasource_id") == 1041
+ assert payload.get("datasource_type") == "table"
+
+ @patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ new_callable=Mock,
+ )
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id", new_callable=Mock)
+ @patch("superset.daos.chart.ChartDAO.find_by_id", new_callable=Mock)
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_dataset_only_rebind_invalid_dataset_returns_error(
+ self,
+ mock_db_session,
+ mock_chart_find,
+ mock_dataset_find,
+ mock_check_access,
+ mcp_server,
Review Comment:
**Suggestion:** Add full type hints for this new async method's parameters
and include a `-> None` return annotation to comply with typing requirements
for added code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new async test method omits type annotations for its parameters and
does not declare a return type. That is a genuine violation of the fully-typed
new code rule.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=60b6538fceff44b49c3aa86d78a2c493&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=60b6538fceff44b49c3aa86d78a2c493&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1545:1551
**Comment:**
*Custom Rule: Add full type hints for this new async method's
parameters and include a `-> None` return annotation to comply with typing
requirements for added code.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=0225bb85bba1aa0c01d256afffcb86db281141891e344cd5e42394236f7c75b3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=0225bb85bba1aa0c01d256afffcb86db281141891e344cd5e42394236f7c75b3&reaction=dislike'>👎</a>
--
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]
