codeant-ai-for-open-source[bot] commented on code in PR #42877: URL: https://github.com/apache/superset/pull/42877#discussion_r3733419800
########## superset/mcp_service/utils/security_error_utils.py: ########## @@ -0,0 +1,45 @@ +# 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. + +""" +Shared helper for surfacing structured SupersetError info from exceptions +caught while running MCP chart/data tools. +""" + +from typing import Any + + +def extract_error_type_and_extra( + data_error: Exception, +) -> tuple[str | None, dict[str, Any] | None]: + """Pull the SupersetError's error_type value and extra payload off an exception. + + ``SupersetErrorException`` (e.g. ``SupersetSecurityException``, raised by + the security manager when row/column/table-level governance denies + access to a query's underlying resource) carries a ``.error`` attribute + (a ``SupersetError``) with the real ``error_type`` (e.g. + ``TABLE_SECURITY_ACCESS_ERROR``, ``DATASOURCE_SECURITY_ACCESS_ERROR``) and + an ``extra`` dict (e.g. access-request details). Plain exceptions + (``ValueError``, a bare ``CommandException`` raised without a + ``SupersetError``) have neither, so both return values are ``None`` for + them -- callers should fall back to a generic error_type in that case. + """ + error_obj = getattr(data_error, "error", None) + extra = getattr(error_obj, "extra", None) + raw_error_type = getattr(error_obj, "error_type", None) + error_type = getattr(raw_error_type, "value", raw_error_type) Review Comment: **Suggestion:** `extract_error_type_and_extra` only inspects `data_error.error.error_type`, but `SupersetException` stores its structured type on the exception's own `error_type` property. Handlers catching a plain `SupersetException` such as `SupersetSyntaxErrorException` therefore lose its real error type and incorrectly fall back to the tool-specific generic type. Check the exception-level property when no nested `SupersetError` is present. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ `generate_chart` reports syntax failures generically. - ⚠️ MCP clients lose structured Superset error classification. - ⚠️ Other helper callers can similarly discard exception-level types. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=74f8ecafdb0c426284ae3f6f2601f27a&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=74f8ecafdb0c426284ae3f6f2601f27a&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:** superset/mcp_service/utils/security_error_utils.py **Line:** 41:44 **Comment:** *Api Mismatch: `extract_error_type_and_extra` only inspects `data_error.error.error_type`, but `SupersetException` stores its structured type on the exception's own `error_type` property. Handlers catching a plain `SupersetException` such as `SupersetSyntaxErrorException` therefore lose its real error type and incorrectly fall back to the tool-specific generic type. Check the exception-level property when no nested `SupersetError` is present. 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%2F42877&comment_hash=000462694284236692e544de9af33df62e40de961aee3af19f29a9d15dc22b81&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42877&comment_hash=000462694284236692e544de9af33df62e40de961aee3af19f29a9d15dc22b81&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py: ########## @@ -413,6 +413,105 @@ def test_compile_chart_value_error(self, mock_factory_cls, mock_cmd_cls): assert "invalid metric" in (result.error or "") +class TestGenerateChartOuterErrorHandling: + """Tests for generate_chart's outer except handler (around _compile_chart + et al.), which previously (a) didn't catch SupersetException at all -- so + a SupersetSecurityException from the compile-check query (e.g. a denied + table) would propagate out of generate_chart() entirely as an unhandled + exception -- and (b) always reported the generic "chart_generation_error" + type, dropping the real error_type. + + Unlike get_chart_data.py/get_chart_preview.py/preview_utils.py (which + return a plain ChartError untouched by any sanitizer), generate_chart's + error goes through ChartErrorBuilder, whose _sanitize_user_input + truncates template vars at 200 chars. A real Gandalf/DataPortal `extra` + payload (nested entities/approvals) is routinely far longer than that, + so `extra` is deliberately NOT appended to `reason` here -- only the + real error_type is surfaced; the message stays str(e), untouched. + + _compile_chart itself intentionally lets SupersetSecurityException + propagate uncaught (see test_compile_chart_security_exception_from_validate + in test_get_chart_data.py) -- it's generate_chart's own outer except + tuple that must catch it. These tests mirror that except block's + error-construction glue (extract_error_type_and_extra + + ChartErrorBuilder.build_error) directly. + """ + + @staticmethod + def _build_generate_chart_error(data_error: Exception) -> Any: + """Mirror generate_chart.py's except-block error construction.""" + from superset.mcp_service.utils.error_builder import ChartErrorBuilder + from superset.mcp_service.utils.security_error_utils import ( + extract_error_type_and_extra, + ) + + error_type, _extra = extract_error_type_and_extra(data_error) + reason = str(data_error) + return ChartErrorBuilder.build_error( + error_type=error_type or "chart_generation_error", + template_key="generation_failed", + template_vars={ + "reason": reason, + "dataset_id": "10", + "chart_type": "table", + }, + error_code="CHART_GENERATION_FAILED", + ) Review Comment: **Suggestion:** These tests do not execute `generate_chart` or its production exception handler: one only searches the module source for the text `SupersetException`, while the other two call a locally duplicated construction helper. Consequently, an incorrect catch tuple, request access, rollback path, or response wiring would still pass this suite. Invoke the actual tool with a mocked compile-time `SupersetSecurityException` and assert its returned response. [code quality] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Regression tests can pass despite broken production wiring. - ⚠️ `generate_chart` security-error responses lack end-to-end coverage. - ⚠️ Rollback and response-validation failures remain undetected. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae725edb84d24be9a6e1a1aba73332fd&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=ae725edb84d24be9a6e1a1aba73332fd&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_generate_chart.py **Line:** 441:459 **Comment:** *Code Quality: These tests do not execute `generate_chart` or its production exception handler: one only searches the module source for the text `SupersetException`, while the other two call a locally duplicated construction helper. Consequently, an incorrect catch tuple, request access, rollback path, or response wiring would still pass this suite. Invoke the actual tool with a mocked compile-time `SupersetSecurityException` and assert its returned response. 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%2F42877&comment_hash=1ac748da6a07eb930a0f311f8e0c608aa0f00c475aea420e9b100df1a72d59cc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42877&comment_hash=1ac748da6a07eb930a0f311f8e0c608aa0f00c475aea420e9b100df1a72d59cc&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]
