codeant-ai-for-open-source[bot] commented on code in PR #41924: URL: https://github.com/apache/superset/pull/41924#discussion_r3573711587
########## tests/unit_tests/mcp_service/database/tool/test_database_tools.py: ########## @@ -120,7 +121,7 @@ def mock_auth(): @pytest.fixture(autouse=True) -def allow_data_model_metadata(): +def _allow_data_model_metadata(): Review Comment: **Suggestion:** Add an explicit return type annotation to this fixture function so it complies with the required type-hinting convention. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The fixture is new/modified Python code and lacks a return type annotation. Under the type-hint rule, this should be annotated (for example, `-> None`) because fixture functions are relevant variables/functions that can be typed. </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=ac09bd6bcc1c4a7ca5563ee6ae6e04f8&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=ac09bd6bcc1c4a7ca5563ee6ae6e04f8&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/database/tool/test_database_tools.py **Line:** 124:124 **Comment:** *Custom Rule: Add an explicit return type annotation to this fixture function so it complies with the required type-hinting convention. 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=08aa98d37a3a31ef2af47d5f8ed26c8dd9f295d9b391658e815027f087f8c344&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=08aa98d37a3a31ef2af47d5f8ed26c8dd9f295d9b391658e815027f087f8c344&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py: ########## @@ -297,3 +298,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: dict[str, Any] = { + "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 Review Comment: **Suggestion:** Add a precise type hint for this kwargs extraction variable so the new test code remains fully typed. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This new local variable is derived from a dict-like kwargs object and is left unannotated. It is a relevant variable that can be typed, so the suggestion is consistent with the 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=c3c93aeb12164e3bbd6de15f1a98f8bc&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=c3c93aeb12164e3bbd6de15f1a98f8bc&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:** 439:439 **Comment:** *Custom Rule: Add a precise type hint for this kwargs extraction variable so the new test code remains 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%2F41924&comment_hash=dd3139d8a6b867c2438b58e0c0b5cf4538be5052d468fd4a3b85c8d419ff1c03&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=dd3139d8a6b867c2438b58e0c0b5cf4538be5052d468fd4a3b85c8d419ff1c03&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py: ########## @@ -297,3 +298,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: dict[str, Any] = { + "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"] Review Comment: **Suggestion:** Provide an explicit type annotation for this forwarded filters variable to comply with the rule requiring type hints on relevant new variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is another newly added local variable in Python test code, and it has no type annotation despite being annotatable as a collection. That is a real instance of the type-hint rule 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=d724b7373fe74e6da4a7c1ea37080e9d&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=d724b7373fe74e6da4a7c1ea37080e9d&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:** 440:440 **Comment:** *Custom Rule: Provide an explicit type annotation for this forwarded filters variable to comply with the rule requiring type hints on 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=b9dc34901ec02594d511287ab2d12aa2a037c06d0d362d9a5aef208cba078bb5&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=b9dc34901ec02594d511287ab2d12aa2a037c06d0d362d9a5aef208cba078bb5&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]
