codeant-ai-for-open-source[bot] commented on code in PR #41924:
URL: https://github.com/apache/superset/pull/41924#discussion_r3561948091
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_metrics.py:
##########
@@ -278,3 +278,91 @@ async def
test_get_compatible_metrics_not_found(mcp_server: FastMCP) -> None:
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_metrics_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return all dataset metrics rather than
+ a validation failure.
+ """
+ mock_ds = _make_dataset(42)
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable to satisfy the type-hint requirement. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is newly added Python code introducing a local variable without a type
hint, and it is a value that can be annotated (for example as MagicMock). This
matches the type-hint requirement.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b06893377f804fd29275af90df1f6075&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b06893377f804fd29275af90df1f6075&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/semantic_layer/tool/test_get_compatible_metrics.py
**Line:** 293:293
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable to satisfy the type-hint requirement.
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%2F41924&comment_hash=9f6c7d608c5c11be7236cd6c38f7bfe7d90744aca7848967a0f8df0e22623966&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=9f6c7d608c5c11be7236cd6c38f7bfe7d90744aca7848967a0f8df0e22623966&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_metrics.py:
##########
@@ -278,3 +278,91 @@ async def
test_get_compatible_metrics_not_found(mcp_server: FastMCP) -> None:
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_metrics_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return all dataset metrics rather than
+ a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_metrics",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
Review Comment:
**Suggestion:** Add an explicit type annotation to this parsed-response
variable in the newly added test. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This newly introduced local variable is unannotated and can be typed (e.g.
as dict[str, Any] or a suitable response type). That is a real omission under
the type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9aa9a740daf146be98424dafe82bcf1a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9aa9a740daf146be98424dafe82bcf1a&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/semantic_layer/tool/test_get_compatible_metrics.py
**Line:** 307:307
**Comment:**
*Custom Rule: Add an explicit type annotation to this parsed-response
variable in the newly added test.
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%2F41924&comment_hash=649a2da7a490a94bfe3413896f3197a1ce02271050d1c0897aa8bd285904c3f4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=649a2da7a490a94bfe3413896f3197a1ce02271050d1c0897aa8bd285904c3f4&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_metrics.py:
##########
@@ -278,3 +278,91 @@ async def
test_get_compatible_metrics_not_found(mcp_server: FastMCP) -> None:
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_metrics_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return all dataset metrics rather than
+ a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_metrics",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ names = {m["name"] for m in data["compatible_metrics"]}
Review Comment:
**Suggestion:** Add a concrete type annotation for this derived collection
variable in the new test code. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is newly added Python code introducing a set-valued local variable
without any type annotation. It can be annotated, so it falls under the
type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d8a2536c6897447aa02fd674727e4f65&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d8a2536c6897447aa02fd674727e4f65&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/semantic_layer/tool/test_get_compatible_metrics.py
**Line:** 310:310
**Comment:**
*Custom Rule: Add a concrete type annotation for this derived
collection variable in the new test 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%2F41924&comment_hash=b6d78208d68670262d0197d924d731af6e0f6c6cf7d0d2209ba8400250d87563&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=b6d78208d68670262d0197d924d731af6e0f6c6cf7d0d2209ba8400250d87563&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_metrics.py:
##########
@@ -278,3 +278,91 @@ async def
test_get_compatible_metrics_not_found(mcp_server: FastMCP) -> None:
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_metrics_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return all dataset metrics rather than
+ a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_metrics",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ names = {m["name"] for m in data["compatible_metrics"]}
+ assert names == {"count", "revenue"}
+
+
[email protected]
+async def test_get_compatible_metrics_external_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """External views handle an explicitly empty selection without error."""
+ mock_view = _make_view(5)
Review Comment:
**Suggestion:** Annotate this newly introduced mock variable with its
expected type. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This added local variable is unannotated in new Python code and is a good
candidate for a type hint (for example MagicMock). The omission matches the
custom rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab2e95fbdbc3445395ba4290d8e4682c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ab2e95fbdbc3445395ba4290d8e4682c&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/semantic_layer/tool/test_get_compatible_metrics.py
**Line:** 319:319
**Comment:**
*Custom Rule: Annotate this newly introduced mock variable with its
expected 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%2F41924&comment_hash=d5857243ba41a42c79a460f347d6e3389a992e3b7c060114bca71db6d4b48ba6&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d5857243ba41a42c79a460f347d6e3389a992e3b7c060114bca71db6d4b48ba6&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py:
##########
@@ -297,3 +297,227 @@ async def
test_get_table_external_time_range_without_dttm_validation_error(
assert data["success"] is False
assert data["error_type"] == "ValidationError"
assert "no datetime dimension" in data["message"]
+
+
[email protected]
+async def test_get_table_dataset_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when dataset_id doesn't resolve to a
dataset."""
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=None):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"dataset_id": 999999, "metrics": ["revenue"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_view_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when view_id doesn't resolve to a view."""
+ with patch(
+ "superset.daos.semantic_layer.SemanticViewDAO.find_by_id",
+ return_value=None,
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"view_id": 999999, "metrics": ["bookings"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_invalid_filter_column_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when a filter references an unknown column."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "filters": [{"col": "bogus_col", "op": "==", "val":
"x"}],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown filter column: 'bogus_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_invalid_order_by_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when order_by references an unknown column/metric."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "order_by": ["bogus_order_col"],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown order_by: 'bogus_order_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_unknown_filter_operator_passes_through(
+ mcp_server: FastMCP,
+) -> None:
+ """An operator string outside the documented set is not schema-validated.
+
+ ``GetTableFilter.op`` is a plain ``str`` field (not a Literal/Enum), so
+ the tool does not reject unrecognized operator values itself -- it
+ forwards them verbatim to the query layer, which is responsible for
+ interpreting/rejecting them.
+ """
+ mock_ds = _make_dataset(42)
+ query_result = {
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable so the test data structure is typed. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The rule requires type hints for new or modified Python variables that can
be annotated. This newly introduced local variable is assigned a dictionary
literal without any annotation, so the suggestion correctly identifies a real
violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=be36f152b1cc49028e8cc718849511b9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=be36f152b1cc49028e8cc718849511b9&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/semantic_layer/tool/test_get_table.py
**Line:** 401:401
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable so the test data structure is 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%2F41924&comment_hash=66d85bb263995ba7ed3dd9abb661f5ea5348f815b64b5987d65edfd88f77c3bf&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=66d85bb263995ba7ed3dd9abb661f5ea5348f815b64b5987d65edfd88f77c3bf&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_compatible_metrics.py:
##########
@@ -278,3 +278,91 @@ async def
test_get_compatible_metrics_not_found(mcp_server: FastMCP) -> None:
assert data["success"] is False
assert data["error_type"] == "NotFound"
+
+
[email protected]
+async def test_get_compatible_metrics_builtin_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """Explicitly empty selected_metrics/selected_dimensions is not an error.
+
+ An empty selection is the natural starting state of a query builder
+ (nothing picked yet), so it must return all dataset metrics rather than
+ a validation failure.
+ """
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_metrics",
+ {
+ "request": {
+ "dataset_id": 42,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ names = {m["name"] for m in data["compatible_metrics"]}
+ assert names == {"count", "revenue"}
+
+
[email protected]
+async def test_get_compatible_metrics_external_empty_selection(
+ mcp_server: FastMCP,
+) -> None:
+ """External views handle an explicitly empty selection without error."""
+ mock_view = _make_view(5)
+ mock_view.get_compatible_metrics = MagicMock(return_value=[])
+
+ with patch(
+ "superset.daos.semantic_layer.SemanticViewDAO.find_by_id",
+ return_value=mock_view,
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_compatible_metrics",
+ {
+ "request": {
+ "view_id": 5,
+ "selected_metrics": [],
+ "selected_dimensions": [],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is True
+ assert data["compatible_metrics"] == []
+ mock_view.get_compatible_metrics.assert_called_once_with([], [])
+
+
[email protected]
+async def
test_get_compatible_metrics_unicode_unknown_selection_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """Unicode/special-character names in an unknown selection surface
cleanly."""
+ mock_ds = _make_dataset(42)
Review Comment:
**Suggestion:** Add a type annotation to this newly added local dataset
variable in the unicode-selection test. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly added local variable without a type hint in Python
test code, and it is suitable for annotation. The custom type-hint rule applies.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=562b5b313b464dabb420db0515525a73&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=562b5b313b464dabb420db0515525a73&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/semantic_layer/tool/test_get_compatible_metrics.py
**Line:** 349:349
**Comment:**
*Custom Rule: Add a type annotation to this newly added local dataset
variable in the unicode-selection test.
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%2F41924&comment_hash=dbc5bbd92ca3f692724a396d0882c15ad236a88fc717b92ca93ff7c4f4dc5060&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=dbc5bbd92ca3f692724a396d0882c15ad236a88fc717b92ca93ff7c4f4dc5060&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py:
##########
@@ -297,3 +297,227 @@ async def
test_get_table_external_time_range_without_dttm_validation_error(
assert data["success"] is False
assert data["error_type"] == "ValidationError"
assert "no datetime dimension" in data["message"]
+
+
[email protected]
+async def test_get_table_dataset_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when dataset_id doesn't resolve to a
dataset."""
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=None):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"dataset_id": 999999, "metrics": ["revenue"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_view_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when view_id doesn't resolve to a view."""
+ with patch(
+ "superset.daos.semantic_layer.SemanticViewDAO.find_by_id",
+ return_value=None,
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"view_id": 999999, "metrics": ["bookings"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_invalid_filter_column_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when a filter references an unknown column."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "filters": [{"col": "bogus_col", "op": "==", "val":
"x"}],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown filter column: 'bogus_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_invalid_order_by_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when order_by references an unknown column/metric."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "order_by": ["bogus_order_col"],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown order_by: 'bogus_order_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_unknown_filter_operator_passes_through(
+ mcp_server: FastMCP,
+) -> None:
+ """An operator string outside the documented set is not schema-validated.
+
+ ``GetTableFilter.op`` is a plain ``str`` field (not a Literal/Enum), so
+ the tool does not reject unrecognized operator values itself -- it
+ forwards them verbatim to the query layer, which is responsible for
+ interpreting/rejecting them.
+ """
+ mock_ds = _make_dataset(42)
+ query_result = {
+ "queries": [
+ {
+ "data": [{"region": "west", "revenue": 100}],
+ "colnames": ["region", "revenue"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as mock_command_cls,
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory"
+ ) as mock_factory_cls,
+ ):
+ mock_command_cls.return_value.run.return_value = query_result
+ mock_factory_cls.return_value.create.return_value = MagicMock()
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "filters": [
+ {"col": "region", "op": "TOTALLY_BOGUS_OP", "val":
"x"}
+ ],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ create_kwargs = mock_factory_cls.return_value.create.call_args.kwargs
+ forwarded_filters = create_kwargs["queries"][0]["filters"]
+
+ assert data["success"] is True
+ assert {"col": "region", "op": "TOTALLY_BOGUS_OP", "val": "x"} in
forwarded_filters
+
+
[email protected]
+async def test_get_table_unicode_filter_value_passes_through(
+ mcp_server: FastMCP,
+) -> None:
+ """Unicode filter values are forwarded to the query layer unmodified."""
+ mock_ds = _make_dataset(42)
+ query_result = {
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable so the test data structure is typed. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly added local dictionary assignment with no type
annotation. Under the stated Python type-hint rule, that omission is a valid
finding.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=85ba104f03b6412086922ea940a59390&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=85ba104f03b6412086922ea940a59390&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/semantic_layer/tool/test_get_table.py
**Line:** 451:451
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable so the test data structure is 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%2F41924&comment_hash=7d6787721ab6043e00c82c190b02c7169105807a4c6a0c5cc486d90a56b89b51&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=7d6787721ab6043e00c82c190b02c7169105807a4c6a0c5cc486d90a56b89b51&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py:
##########
@@ -297,3 +297,227 @@ async def
test_get_table_external_time_range_without_dttm_validation_error(
assert data["success"] is False
assert data["error_type"] == "ValidationError"
assert "no datetime dimension" in data["message"]
+
+
[email protected]
+async def test_get_table_dataset_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when dataset_id doesn't resolve to a
dataset."""
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=None):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"dataset_id": 999999, "metrics": ["revenue"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_view_not_found(mcp_server: FastMCP) -> None:
+ """get_table returns NotFound when view_id doesn't resolve to a view."""
+ with patch(
+ "superset.daos.semantic_layer.SemanticViewDAO.find_by_id",
+ return_value=None,
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {"request": {"view_id": 999999, "metrics": ["bookings"]}},
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "NotFound"
+ assert "999999" in data["message"]
+
+
[email protected]
+async def test_get_table_invalid_filter_column_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when a filter references an unknown column."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "filters": [{"col": "bogus_col", "op": "==", "val":
"x"}],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown filter column: 'bogus_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_invalid_order_by_validation_error(
+ mcp_server: FastMCP,
+) -> None:
+ """get_table errors when order_by references an unknown column/metric."""
+ mock_ds = _make_dataset(42)
+
+ with patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "order_by": ["bogus_order_col"],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ assert data["success"] is False
+ assert data["error_type"] == "ValidationError"
+ assert "Unknown order_by: 'bogus_order_col'" in data["error"]
+
+
[email protected]
+async def test_get_table_unknown_filter_operator_passes_through(
+ mcp_server: FastMCP,
+) -> None:
+ """An operator string outside the documented set is not schema-validated.
+
+ ``GetTableFilter.op`` is a plain ``str`` field (not a Literal/Enum), so
+ the tool does not reject unrecognized operator values itself -- it
+ forwards them verbatim to the query layer, which is responsible for
+ interpreting/rejecting them.
+ """
+ mock_ds = _make_dataset(42)
+ query_result = {
+ "queries": [
+ {
+ "data": [{"region": "west", "revenue": 100}],
+ "colnames": ["region", "revenue"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=mock_ds),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as mock_command_cls,
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory"
+ ) as mock_factory_cls,
+ ):
+ mock_command_cls.return_value.run.return_value = query_result
+ mock_factory_cls.return_value.create.return_value = MagicMock()
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_table",
+ {
+ "request": {
+ "dataset_id": 42,
+ "metrics": ["revenue"],
+ "filters": [
+ {"col": "region", "op": "TOTALLY_BOGUS_OP", "val":
"x"}
+ ],
+ }
+ },
+ )
+ data = json.loads(result.content[0].text)
+
+ create_kwargs = mock_factory_cls.return_value.create.call_args.kwargs
+ forwarded_filters = create_kwargs["queries"][0]["filters"]
+
+ assert data["success"] is True
+ assert {"col": "region", "op": "TOTALLY_BOGUS_OP", "val": "x"} in
forwarded_filters
+
+
[email protected]
+async def test_get_table_unicode_filter_value_passes_through(
+ mcp_server: FastMCP,
+) -> None:
+ """Unicode filter values are forwarded to the query layer unmodified."""
+ mock_ds = _make_dataset(42)
+ query_result = {
+ "queries": [
+ {
+ "data": [{"region": "west", "revenue": 100}],
+ "colnames": ["region", "revenue"],
+ "rowcount": 1,
+ }
+ ]
+ }
+ unicode_val = "ๆฅๆฌ่ช cafรฉ โฌ"
Review Comment:
**Suggestion:** Add a concrete type hint to this new scalar variable to keep
local test inputs fully annotated. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The variable is a newly introduced Python local with an obvious string type,
yet it is left unannotated. That matches the custom rule requiring type hints
where they can be added.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f51b9fe49e9f447a982c45f6b8b0797b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f51b9fe49e9f447a982c45f6b8b0797b&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/semantic_layer/tool/test_get_table.py
**Line:** 460:460
**Comment:**
*Custom Rule: Add a concrete type hint to this new scalar variable to
keep local test inputs fully annotated.
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%2F41924&comment_hash=9c1c2adb81c5aff6dc8de397391830f2896403235e2e29102a67f756d7bbbdc8&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=9c1c2adb81c5aff6dc8de397391830f2896403235e2e29102a67f756d7bbbdc8&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -17,7 +17,38 @@
"""Tests for health_check MCP tool."""
+import importlib
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
from superset.mcp_service.system.schemas import HealthCheckResponse
+from superset.utils import json
+
+# Import the submodule directly so ``patch.object`` targets the module (not the
+# ``health_check`` function that ``tool/__init__.py`` re-exports onto the
+# package).
+health_check_module = importlib.import_module(
+ "superset.mcp_service.system.tool.health_check"
+)
+
+
[email protected]
+def mcp_server():
+ return mcp
Review Comment:
**Suggestion:** Add an explicit return type annotation to this fixture
function to satisfy the required type-hint rule. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new fixture `mcp_server` has no return type annotation, which matches
the custom rule requiring type hints on new or modified Python functions.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d13867b5bfdf4ef1a6a3655f246119db&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d13867b5bfdf4ef1a6a3655f246119db&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/system/tool/test_health_check.py
**Line:** 38:40
**Comment:**
*Custom Rule: Add an explicit return type annotation to this fixture
function to satisfy the required type-hint rule.
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%2F41924&comment_hash=0d8c190b292575521a853215ece7fd312399d332c18664fa4dab06e063176df4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=0d8c190b292575521a853215ece7fd312399d332c18664fa4dab06e063176df4&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -17,7 +17,38 @@
"""Tests for health_check MCP tool."""
+import importlib
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
from superset.mcp_service.system.schemas import HealthCheckResponse
+from superset.utils import json
+
+# Import the submodule directly so ``patch.object`` targets the module (not the
+# ``health_check`` function that ``tool/__init__.py`` re-exports onto the
+# package).
+health_check_module = importlib.import_module(
+ "superset.mcp_service.system.tool.health_check"
+)
+
+
[email protected]
+def mcp_server():
+ return mcp
+
+
[email protected](autouse=True)
+def mock_auth():
+ """Mock authentication for all tests."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as
mock_get_user:
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
Review Comment:
**Suggestion:** Add a return type annotation to this autouse fixture (and
typed local bindings where appropriate) so the new function is fully type
hinted. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The autouse fixture `mock_auth` is a new function without any type
annotation on its signature or yielded value, so it violates the type-hint
requirement.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6c6cd411436c486ea19c54ef08f01919&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6c6cd411436c486ea19c54ef08f01919&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/system/tool/test_health_check.py
**Line:** 43:51
**Comment:**
*Custom Rule: Add a return type annotation to this autouse fixture (and
typed local bindings where appropriate) so the new function is fully type
hinted.
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%2F41924&comment_hash=7a8d50589caaef753194968b765e566a1bb3dac9b612776a2a449a0f2254fbe9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=7a8d50589caaef753194968b765e566a1bb3dac9b612776a2a449a0f2254fbe9&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -53,3 +84,94 @@ def test_health_check_response_with_uptime():
)
assert response.uptime_seconds == 123.45
+
+
+# ---------------------------------------------------------------------------
+# Tool-level tests: health_check via MCP Client
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_health_check_success_via_client(mcp_server):
Review Comment:
**Suggestion:** Annotate the fixture parameter and coroutine return type for
this async test function to comply with the type-hint requirement. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This async test function adds a fixture parameter but no type annotations
for the parameter or coroutine return type, which is a direct violation of the
type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=828f9c43fc8447dc9afab0bd36fc4431&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=828f9c43fc8447dc9afab0bd36fc4431&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/system/tool/test_health_check.py
**Line:** 94:95
**Comment:**
*Custom Rule: Annotate the fixture parameter and coroutine return type
for this async test function to comply with the type-hint requirement.
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%2F41924&comment_hash=8e82c30db84883f0753e28efb3e80b7e146e05cb548c0bbee00901b27dc7be9b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=8e82c30db84883f0753e28efb3e80b7e146e05cb548c0bbee00901b27dc7be9b&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -53,3 +84,94 @@ def test_health_check_response_with_uptime():
)
assert response.uptime_seconds == 123.45
+
+
+# ---------------------------------------------------------------------------
+# Tool-level tests: health_check via MCP Client
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_health_check_success_via_client(mcp_server):
+ """Happy path: tool returns a healthy status with real system info."""
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+
+ data = json.loads(result.content[0].text)
+ assert data["status"] == "healthy"
+ assert data["service"] == "Superset MCP Service"
+ assert data["version"] == "4.1.0"
+ assert data["timestamp"] is not None
+ assert data["uptime_seconds"] is not None
+ assert data["uptime_seconds"] >= 0
+
+
[email protected]
+async def test_health_check_uses_configured_app_name(mcp_server, app):
Review Comment:
**Suggestion:** Add explicit type annotations for both parameters and the
async return type in this test function signature. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The test signature omits type hints for both fixture parameters and the
async return type, so it matches the custom type-hint violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=432a02d9a0b841f8be512321cf83d84b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=432a02d9a0b841f8be512321cf83d84b&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/system/tool/test_health_check.py
**Line:** 114:115
**Comment:**
*Custom Rule: Add explicit type annotations for both parameters and the
async return type in this test function signature.
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%2F41924&comment_hash=940782ff9a11af8dc7c131b9466b9010a493130296dc06e6ad317c2941aed23a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=940782ff9a11af8dc7c131b9466b9010a493130296dc06e6ad317c2941aed23a&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -53,3 +84,94 @@ def test_health_check_response_with_uptime():
)
assert response.uptime_seconds == 123.45
+
+
+# ---------------------------------------------------------------------------
+# Tool-level tests: health_check via MCP Client
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_health_check_success_via_client(mcp_server):
+ """Happy path: tool returns a healthy status with real system info."""
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+
+ data = json.loads(result.content[0].text)
+ assert data["status"] == "healthy"
+ assert data["service"] == "Superset MCP Service"
+ assert data["version"] == "4.1.0"
+ assert data["timestamp"] is not None
+ assert data["uptime_seconds"] is not None
+ assert data["uptime_seconds"] >= 0
+
+
[email protected]
+async def test_health_check_uses_configured_app_name(mcp_server, app):
+ """service name is derived from the APP_NAME config, not hardcoded."""
+ app.config["APP_NAME"] = "Acme Analytics"
+ try:
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+ finally:
+ app.config.pop("APP_NAME", None)
+
+ data = json.loads(result.content[0].text)
+ assert data["service"] == "Acme Analytics MCP Service"
+
+
[email protected]
+async def test_health_check_returns_error_status_when_version_metadata_raises(
+ mcp_server,
+):
Review Comment:
**Suggestion:** Provide type annotations on the fixture argument and async
return type for this multiline function definition. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This multiline async test function still omits type hints on its fixture
parameter and return type, so the issue is real and present in the new code.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a1a2aeb62cce4ba89218fa3e04eaa236&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a1a2aeb62cce4ba89218fa3e04eaa236&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/system/tool/test_health_check.py
**Line:** 133:136
**Comment:**
*Custom Rule: Provide type annotations on the fixture argument and
async return type for this multiline function definition.
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%2F41924&comment_hash=e114196e9205fd88d92f4beaafee67d5bb49395de2569405000baa2c3e1e64bb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=e114196e9205fd88d92f4beaafee67d5bb49395de2569405000baa2c3e1e64bb&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/test_rbac_tool_enforcement.py:
##########
@@ -0,0 +1,256 @@
+# 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.
+
+"""
+End-to-end RBAC-rejection coverage for mutating MCP tools.
+
+``tests/unit_tests/mcp_service/conftest.py`` disables RBAC for every other
+test module in this package (``disable_mcp_rbac``, autouse), and
+``test_auth_rbac.py`` unit-tests ``check_tool_permission`` in isolation by
+calling it directly with hand-built stub functions. Neither proves that a
+*real, registered* MCP tool actually enforces RBAC when invoked through the
+full FastMCP tool-call path (``Client(mcp).call_tool(...)`` ->
+``mcp_auth_hook`` -> ``check_tool_permission`` -> the tool body).
+
+This module closes that gap: for each mutating tool below, it re-enables
+RBAC, denies the mocked ``security_manager.can_access`` check, calls the
+*actual registered tool* through ``fastmcp.Client``, and asserts the call is
+rejected with a ``fastmcp.exceptions.ToolError`` before the tool body runs.
+It also includes one control test proving a permitted caller is NOT blocked
+by the RBAC gate, so the denial tests above cannot be a false negative caused
+by the test harness itself (e.g. a client/transport error that looks like a
+rejection for unrelated reasons).
+
+Scope: only the RBAC-rejection path. General tool behavior (happy path,
+validation errors, DAO error handling, etc.) is already covered by each
+tool's own test module under ``tests/unit_tests/mcp_service/<module>/tool/``
+and is intentionally not duplicated here.
+"""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from fastmcp.exceptions import ToolError
+
+from superset.mcp_service.app import mcp
+
+# (tool_name, minimal-but-valid request payload, method_permission_name,
+# class_permission_name) for each mutating tool audited for RBAC enforcement.
+# The request payloads are the smallest bodies that pass each tool's Pydantic
+# schema validation (which FastMCP performs while binding arguments, before
+# ``mcp_auth_hook`` runs) so the call reaches the RBAC gate itself.
+_MUTATING_TOOLS: list[tuple[str, dict[str, Any], str, str]] = [
+ (
+ "execute_sql",
+ {"database_id": 1, "sql": "SELECT 1"},
+ "execute_sql_query",
+ "SQLLab",
+ ),
+ (
+ "update_chart",
+ {"identifier": 1},
+ "write",
+ "Chart",
+ ),
+ (
+ "update_dashboard",
+ {"identifier": 1},
+ "write",
+ "Dashboard",
+ ),
+ (
+ "generate_chart",
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "col1"}]},
+ },
+ "write",
+ "Chart",
+ ),
+ (
+ "generate_dashboard",
+ {"chart_ids": [1]},
+ "write",
+ "Dashboard",
+ ),
+ (
+ "manage_native_filters",
+ # ``reorder: []`` satisfies ManageNativeFiltersRequest's "at least one
+ # operation" validator (checked via ``is None``, not falsiness).
+ {"dashboard_id": 1, "reorder": []},
+ "write",
+ "Dashboard",
+ ),
+ (
+ "create_dataset",
+ {"database_id": 1, "table_name": "my_table"},
+ "write",
+ "Dataset",
+ ),
+ (
+ "create_virtual_dataset",
+ {"database_id": 1, "sql": "SELECT 1", "dataset_name":
"my_virtual_dataset"},
+ "write",
+ "Dataset",
+ ),
+ (
+ "create_theme",
+ {"theme_name": "Denied Theme", "json_data": {"token": {}}},
+ "write",
+ "Theme",
+ ),
+ (
+ "save_sql_query",
+ {"database_id": 1, "label": "my query", "sql": "SELECT 1"},
+ "write",
+ "SavedQuery",
+ ),
+]
+
+_TOOL_IDS = [name for name, *_ in _MUTATING_TOOLS]
Review Comment:
**Suggestion:** Add an explicit type annotation to the module-level
`_TOOL_IDS` variable to satisfy the requirement for annotating relevant new
variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new module-level variable `_TOOL_IDS` is inferred from a list
comprehension and can be explicitly annotated as a list of strings. This
matches the rule requiring type hints on relevant new variables that can be
annotated.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e695428576ea4384ba8822f7d16fa72d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e695428576ea4384ba8822f7d16fa72d&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/test_rbac_tool_enforcement.py
**Line:** 127:127
**Comment:**
*Custom Rule: Add an explicit type annotation to the module-level
`_TOOL_IDS` variable to satisfy the requirement for annotating relevant new
variables.
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%2F41924&comment_hash=a5b9edbb42d6c6f253f2290605deb0857c25173766017a85a07d842664de6027&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=a5b9edbb42d6c6f253f2290605deb0857c25173766017a85a07d842664de6027&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]
