codeant-ai-for-open-source[bot] commented on code in PR #40348: URL: https://github.com/apache/superset/pull/40348#discussion_r3305670506
########## 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, + user: Any, + ) -> Any: + """Inject report-specific self-lookup filters. + + Uses ``owners.id`` for ``owned_by_me`` (instead of the generic + ``owner`` column) to match what ``ReportScheduleDAO.list`` expects. + """ + if not (created_by_me or owned_by_me): + return filters + + if not user or not getattr(user, "is_authenticated", False): + raise ValueError("This operation requires an authenticated user") + + user_id: int = user.id + extra: ColumnOperator + if created_by_me and owned_by_me: + # Inject both filters separately so each assertion in tests passes. + owners_filter = ColumnOperator(col="owners.id", opr="eq", value=user_id) + created_filter = ColumnOperator( + col="created_by_fk", opr="eq", value=user_id + ) + extra_list = [owners_filter, created_filter] + if filters is None: + return extra_list + if isinstance(filters, list): + return extra_list + filters + return extra_list + [filters] + elif created_by_me: + extra = ColumnOperator(col="created_by_fk", opr="eq", value=user_id) + else: + extra = ColumnOperator(col="owners.id", opr="eq", value=user_id) + + if filters is None: + return [extra] + if isinstance(filters, list): + return [extra] + filters + return [extra, filters] + + def _call_dao_list( + self, + filters: Any, + order_column: str, + order_direction: str, + page: int, + page_size: int, + search: str | None, + columns_to_load: List[str], + ) -> tuple[List[Any], int]: + """Call the DAO with ``filters=`` kwarg (report-specific convention).""" + return self.dao_class.list( # type: ignore[call-arg] + filters=filters, + order_column=order_column, Review Comment: **🔴 Architect Review — CRITICAL** ReportListCore._call_dao_list calls ReportScheduleDAO.list(filters=...) even though BaseDAO.list only accepts column_operators= and ReportScheduleDAO does not override list, so any runtime call to list_reports will raise a TypeError for an unexpected keyword argument and the tool cannot execute successfully. **Suggestion:** Update ReportListCore._call_dao_list to use the existing DAO contract (pass filters as column_operators or introduce a compatible list implementation on ReportScheduleDAO) and add an end-to-end test that exercises list_reports against the real DAO, without patching list. [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2ee1756bb9ec4de69a2a9e513972c0fe&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=2ee1756bb9ec4de69a2a9e513972c0fe&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 an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood. **Path:** superset/mcp_service/report/tool/list_reports.py **Line:** 121:124 **Comment:** *CRITICAL: ReportListCore._call_dao_list calls ReportScheduleDAO.list(filters=...) even though BaseDAO.list only accepts column_operators= and ReportScheduleDAO does not override list, so any runtime call to list_reports will raise a TypeError for an unexpected keyword argument and the tool cannot execute successfully. 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. If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding. 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> ########## superset/mcp_service/report/schemas.py: ########## @@ -0,0 +1,292 @@ +# 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. + +""" +Pydantic schemas for report (alerts & reports) related responses. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from typing import Annotated, Any, Dict, List, Literal + +import humanize +from pydantic import ( + BaseModel, + ConfigDict, + Field, + field_validator, + model_serializer, + model_validator, + PositiveInt, +) + +from superset.daos.base import ColumnOperator, ColumnOperatorEnum +from superset.mcp_service.common.cache_schemas import ( + CreatedByMeMixin, + MetadataCacheControl, + OwnedByMeMixin, +) +from superset.mcp_service.constants import DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE +from superset.mcp_service.privacy import filter_user_directory_fields +from superset.mcp_service.system.schemas import PaginationInfo +from superset.mcp_service.utils.schema_utils import ( + parse_json_or_list, + parse_json_or_model_list, +) + + +class ReportFilter(ColumnOperator): + """ + Filter object for report listing. + col: The column to filter on. Must be one of the allowed filter fields. + opr: The operator to use. Must be one of the supported operators. + value: The value to filter by (type depends on col and opr). + """ + + col: Literal[ + "name", + "type", + "active", + "dashboard_id", + "chart_id", + ] = Field( Review Comment: **🟠Architect Review — HIGH** ReportFilter.col only allows name, type, active, dashboard_id, and chart_id, while get_schema(model_type="report") uses ModelGetSchemaCore with generic DAO filter discovery that exposes created_by_fk as a valid filter column, so schema discovery can advertise a created_by_fk filter that list_reports then rejects at request validation. **Suggestion:** Align report filter validation with schema discovery by either adding created_by_fk to ReportFilter.col or excluding it from the report filter_columns in _get_report_schema_core so that clients can safely trust get_schema output as the source of truth. [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9e385bed576949a0998f139fb8f87565&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=9e385bed576949a0998f139fb8f87565&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 an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood. **Path:** superset/mcp_service/report/schemas.py **Line:** 61:67 **Comment:** *HIGH: ReportFilter.col only allows name, type, active, dashboard_id, and chart_id, while get_schema(model_type="report") uses ModelGetSchemaCore with generic DAO filter discovery that exposes created_by_fk as a valid filter column, so schema discovery can advertise a created_by_fk filter that list_reports then rejects at request validation. 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. If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding. 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> -- 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]
