codeant-ai-for-open-source[bot] commented on code in PR #40348: URL: https://github.com/apache/superset/pull/40348#discussion_r3305693440
########## superset/mcp_service/report/tool/list_reports.py: ########## @@ -0,0 +1,243 @@ +# 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. + +""" +List reports (alerts & reports) FastMCP tool. +""" + +import logging +from typing import Any, List, TYPE_CHECKING + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +if TYPE_CHECKING: + from superset.reports.models import ReportSchedule + +from superset.daos.base import ColumnOperator +from superset.extensions import event_logger +from superset.mcp_service.common.schema_discovery import ( + get_all_column_names, + get_report_columns, + REPORT_DEFAULT_COLUMNS, + REPORT_SEARCH_COLUMNS, + REPORT_SORTABLE_COLUMNS, +) +from superset.mcp_service.mcp_core import ModelListCore Review Comment: **Suggestion:** When both self-lookup flags are enabled, this injects two separate filters, which are combined with AND by DAO filtering logic, so results are restricted to rows where the user is both creator and owner. The documented behavior across MCP list tools is OR/union semantics for these flags, so this implementation incorrectly drops valid rows; use a single OR-style synthetic filter (or equivalent OR expression) instead of two independent filters. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ list_reports with both flags omits valid owned-or-created reports. - ⚠️ Users cannot retrieve full union of owned and created reports. - ⚠️ Behavior contradicts documented MCP list filter semantics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The request schema `ListReportsRequest` in `superset/mcp_service/report/schemas.py:164-200` inherits `OwnedByMeMixin` and `CreatedByMeMixin` from `superset/mcp_service/common/cache_schemas.py:88-105,120-29`, whose docstring states that when both flags are set, the result should include items where the user is either creator OR owner (union, not intersection). 2. From an MCP client, send a `list_reports` request with both `created_by_me=true` and `owned_by_me=true` (per the tool description in `superset/mcp_service/app.py:135-139` and the schema in `report/schemas.py:164-175`), targeting the async `list_reports` function in `superset/mcp_service/report/tool/list_reports.py:87-98`. 3. `list_reports` constructs `ReportListCore` and calls `run_tool(...)` (list_reports.py:182-215); `ModelListCore.run_tool` in `superset/mcp_service/mcp_core.py:104-125` then delegates self-lookup filter handling to `self._prepend_self_lookup_filters(...)`. 4. Because `ReportListCore` overrides `_prepend_self_lookup_filters`, the implementation in `superset/mcp_service/report/tool/list_reports.py:21-50` is used: when `created_by_me and owned_by_me` is true (line 29), it constructs two separate `ColumnOperator` filters, `owners.id == user_id` and `created_by_fk == user_id` (lines 31-34), and returns them as `extra_list` combined with any existing filters (lines 35-40). 5. After fixing the DAO kwarg mismatch described in suggestion 1 so that the DAO call succeeds, `ModelListCore._call_dao_list` (or the subclass override) passes this list of `ColumnOperator` instances to `BaseDAO.list` as `column_operators`, which then calls `BaseDAO.apply_column_operators` in `superset/daos/base.py:472-37`. 6. `BaseDAO.apply_column_operators` iterates through `column_operators` and applies each via `query.filter(operator_enum.apply(column, value))` (base.py:14-33), which combines conditions with SQLAlchemy AND semantics, so the resulting query only returns schedules where the current user is both an owner and the creator. 7. This behavior contradicts the documented union semantics in `OwnedByMeMixin` ("either the creator OR an owner (union, not intersection)" at `cache_schemas.py:120-17`) and the generic `ModelListCore._prepend_self_lookup_filters` docstring ("a single combined OR filter" at `mcp_core.py:48-55`), causing valid reports where the user is only owner or only creator to be omitted when both flags are set. 8. Note: currently this logic is masked by the TypeError in suggestion 1, but once the DAO keyword mismatch is fixed as intended, enabling both flags reproduces this incorrect intersection filtering on every combined-flag call. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=19fd868a81884e59b086fc7b5058f158&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=19fd868a81884e59b086fc7b5058f158&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/report/tool/list_reports.py **Line:** 29:40 **Comment:** *Incorrect Condition Logic: When both self-lookup flags are enabled, this injects two separate filters, which are combined with AND by DAO filtering logic, so results are restricted to rows where the user is both creator and owner. The documented behavior across MCP list tools is OR/union semantics for these flags, so this implementation incorrectly drops valid rows; use a single OR-style synthetic filter (or equivalent OR expression) instead of two independent filters. 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%2F40348&comment_hash=3a32f32756d2b5226e297193527ab27cacf500cc415a3c9aad9fa7fd42e34f73&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40348&comment_hash=3a32f32756d2b5226e297193527ab27cacf500cc415a3c9aad9fa7fd42e34f73&reaction=dislike'>👎</a> ########## superset/mcp_service/report/tool/list_reports.py: ########## @@ -0,0 +1,243 @@ +# 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. + +""" +List reports (alerts & reports) FastMCP tool. +""" + +import logging +from typing import Any, List, TYPE_CHECKING + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +if TYPE_CHECKING: + from superset.reports.models import ReportSchedule + +from superset.daos.base import ColumnOperator +from superset.extensions import event_logger +from superset.mcp_service.common.schema_discovery import ( + get_all_column_names, + get_report_columns, + REPORT_DEFAULT_COLUMNS, + REPORT_SEARCH_COLUMNS, + REPORT_SORTABLE_COLUMNS, +) +from superset.mcp_service.mcp_core import ModelListCore +from superset.mcp_service.report.schemas import ( + ListReportsRequest, + ReportError, + ReportFilter, + ReportInfo, + ReportList, + serialize_report_object, +) + +logger = logging.getLogger(__name__) + + +class ReportListCore(ModelListCore[ReportList]): + """ModelListCore subclass for ReportSchedule. + + Overrides two behaviours that differ from the generic list tool: + + 1. The DAO is called with ``filters=`` instead of ``column_operators=`` + so that tests can inspect the kwarg by name. + 2. The self-lookup filter for ``owned_by_me`` uses the relationship + path ``owners.id`` (the real ReportSchedule filter column) rather + than the generic ``owner`` sentinel used by other list tools. + """ + + # Column name used by ReportScheduleDAO for the owners M2M filter. + _OWNED_BY_ME_COLUMN = "owners.id" + + @staticmethod + def _prepend_self_lookup_filters( + filters: Any, + created_by_me: bool, + owned_by_me: bool, Review Comment: **Suggestion:** The DAO call uses `filters=` but `BaseDAO.list`/`ReportScheduleDAO.list` expects `column_operators=`. This raises `TypeError: ... unexpected keyword argument 'filters'` at runtime, so `list_reports` fails for normal execution (tests miss it because they mock the DAO method). Call the DAO with the correct keyword used by the DAO contract. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP list_reports tool crashes with TypeError on every call. - ❌ Alerts and reports cannot be listed via MCP clients. - ⚠️ Test instructions for list_reports cannot succeed end-to-end. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start Superset with MCP service enabled so tools from `superset/mcp_service/app.py:650-689` are imported, including `list_reports` from `superset/mcp_service/report/tool/__init__.py` and `list_reports.py`. 2. From an MCP client, invoke the `list_reports` tool, which calls the async function `list_reports` in `superset/mcp_service/report/tool/list_reports.py:78-99` (registered via the `@tool` decorator at lines 78-86). 3. Inside `list_reports`, a `ReportListCore` instance is constructed (lines 182-202) with `dao_class=ReportScheduleDAO` and `filter_type=ReportFilter`, then `list_tool.run_tool(...)` is invoked with pagination, filters, and self-lookup flags (lines 203-215). 4. `ModelListCore.run_tool` in `superset/mcp_service/mcp_core.py:104-125` processes filters and then calls `self._call_dao_list(...)` at `mcp_core.py:305-313` to execute the DAO query. 5. Because `ReportListCore` overrides `_call_dao_list`, the implementation at `superset/mcp_service/report/tool/list_reports.py:52-61` is used, which calls `self.dao_class.list(filters=filters, order_column=..., ...)` as shown in lines 63-72 of the PR diff. 6. `ReportScheduleDAO` in `superset/daos/report.py:13-15` subclasses `BaseDAO[ReportSchedule]` and does not override `list`, so it inherits `BaseDAO.list` defined in `superset/daos/base.py:605-17`, whose signature expects `column_operators` and does not define a `filters` keyword argument. 7. At runtime, Python resolves `ReportScheduleDAO.list` to `BaseDAO.list` and the call `list(filters=filters, ...)` from `list_reports.py:63-72` raises `TypeError: list() got an unexpected keyword argument 'filters'`, causing every `list_reports` MCP invocation to fail before any rows are returned. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2806461e1aff466a94e8c1c74646f3b1&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=2806461e1aff466a94e8c1c74646f3b1&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/report/tool/list_reports.py **Line:** 63:72 **Comment:** *Api Mismatch: The DAO call uses `filters=` but `BaseDAO.list`/`ReportScheduleDAO.list` expects `column_operators=`. This raises `TypeError: ... unexpected keyword argument 'filters'` at runtime, so `list_reports` fails for normal execution (tests miss it because they mock the DAO method). Call the DAO with the correct keyword used by the DAO contract. 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%2F40348&comment_hash=1513bd65c2c46a09395d320c398a92430cddbd7e9cb90ef89cbe739e5db742bb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40348&comment_hash=1513bd65c2c46a09395d320c398a92430cddbd7e9cb90ef89cbe739e5db742bb&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]
