You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/05/22 03:30:42 UTC
[PR] feat(mcp): add list_reports and get_report_info tools [superset]
aminghadersohi opened a new pull request, #40348:
URL: https://github.com/apache/superset/pull/40348
### SUMMARY
Adds two new MCP tools for the Alerts & Reports domain:
- **`list_reports`**: List alert and report schedules with filtering (by name, type, active, dashboard_id, chart_id), text search, sorting, select_columns, and 1-based pagination. Supports `owned_by_me` and `created_by_me` flags to filter by ownership/creator.
- **`get_report_info`**: Fetch a single report/alert schedule by numeric ID, returning schedule configuration including type (Alert/Report), active status, cron expression, and associated dashboard or chart.
Schema discovery is wired in via `get_schema(model_type="report")`.
Key design decisions:
- RBAC handled automatically via `ReportScheduleDAO.base_filter = ReportScheduleFilter`
- `owners` field stripped by `USER_DIRECTORY_FIELDS` privacy controls
- No UUID support (ReportSchedule has no UUID column) — integer ID only
- Column constants and `get_report_columns()` live in `schema_discovery.py` for DRY access across list/schema tools
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only MCP tools.
### TESTING INSTRUCTIONS
1. Start Superset with MCP service enabled
2. Connect an MCP client and call `list_reports` — verify it returns reports accessible to the current user
3. Call `list_reports` with `{"owned_by_me": true}` — verify only owned reports are returned
4. Call `get_report_info` with a valid report ID — verify schedule config is returned
5. Call `get_report_info` with an invalid ID — verify a not_found error is returned
6. Call `get_schema(model_type="report")` — verify column/filter metadata is returned
Unit tests:
```
pytest tests/unit_tests/mcp_service/report/
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3305638974
##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,290 @@
+# 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
+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(
+ ...,
+ description="Column to filter on. Use get_schema(model_type='report') for "
+ "available filter columns.",
+ )
+ opr: ColumnOperatorEnum = Field(
+ ...,
+ description="Operator to use. Use get_schema(model_type='report') for "
+ "available operators.",
+ )
+ value: str | int | float | bool | List[str | int | float | bool] = Field(
+ ..., description="Value to filter by (type depends on col and opr)"
+ )
+
+
+class ReportInfo(BaseModel):
+ id: int | None = Field(None, description="Report/Alert ID")
+ name: str | None = Field(None, description="Report/Alert name")
+ description: str | None = Field(None, description="Report/Alert description")
+ type: str | None = Field(None, description="Schedule type: 'Alert' or 'Report'")
+ active: bool | None = Field(None, description="Whether the schedule is active")
+ crontab: str | None = Field(None, description="Cron expression for scheduling")
+ dashboard_id: int | None = Field(
+ None, description="Associated dashboard ID, if any"
+ )
+ chart_id: int | None = Field(None, description="Associated chart ID, if any")
+ owners: List[Any] | None = Field(
+ None, description="List of owners (filtered by privacy controls)"
+ )
+ changed_on: str | datetime | None = Field(
+ None, description="Last modification timestamp"
+ )
+ changed_on_humanized: str | None = Field(
+ None, description="Humanized modification time"
+ )
+ created_on: str | datetime | None = Field(None, description="Creation timestamp")
+ created_on_humanized: str | None = Field(
+ None, description="Humanized creation time"
+ )
+ model_config = ConfigDict(
+ from_attributes=True,
+ ser_json_timedelta="iso8601",
+ populate_by_name=True,
+ )
+
+ @model_serializer(mode="wrap")
+ def _filter_fields_by_context(self, serializer: Any, info: Any) -> Dict[str, Any]:
+ """Filter fields based on serialization context.
+
+ If context contains 'select_columns', only include those fields.
+ Otherwise, include all fields (default behavior).
+ """
+ data = filter_user_directory_fields(serializer(self))
+
+ if info.context and isinstance(info.context, dict):
+ select_columns = info.context.get("select_columns")
+ if select_columns:
+ requested_fields = set(select_columns)
+ return {k: v for k, v in data.items() if k in requested_fields}
+
+ return data
+
+
+class ReportList(BaseModel):
+ reports: List[ReportInfo]
+ count: int
+ total_count: int
+ page: int
+ page_size: int
+ total_pages: int
+ has_previous: bool
+ has_next: bool
+ columns_requested: List[str] = Field(
+ default_factory=list,
+ description="Requested columns for the response",
+ )
+ columns_loaded: List[str] = Field(
+ default_factory=list,
+ description="Columns that were actually loaded for each report",
+ )
+ columns_available: List[str] = Field(
+ default_factory=list,
+ description="All columns available for selection via select_columns parameter",
+ )
+ sortable_columns: List[str] = Field(
+ default_factory=list,
+ description="Columns that can be used with order_column parameter",
+ )
+ filters_applied: List[ColumnOperator] = Field(
+ default_factory=list,
+ description="List of advanced filter dicts applied to the query.",
+ )
+ pagination: PaginationInfo | None = None
+ timestamp: datetime | None = None
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+
+class ListReportsRequest(OwnedByMeMixin, CreatedByMeMixin, MetadataCacheControl):
+ """Request schema for list_reports."""
+
+ filters: Annotated[
+ List[ReportFilter],
+ Field(
+ default_factory=list,
+ description="List of filter objects (column, operator, value). Each "
+ "filter is an object with 'col', 'opr', and 'value' "
+ "properties. Cannot be used together with 'search'.",
+ ),
+ ]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=list,
+ description="List of columns to select. Defaults to common columns if not "
+ "specified.",
+ ),
+ ]
+ search: Annotated[
+ str | None,
+ Field(
+ default=None,
+ description="Text search string to match against report fields. Cannot "
+ "be used together with 'filters'.",
+ ),
+ ]
+ order_column: Annotated[
+ str | None, Field(default=None, description="Column to order results by")
+ ]
+ order_direction: Annotated[
+ Literal["asc", "desc"],
+ Field(
+ default="desc", description="Direction to order results ('asc' or 'desc')"
+ ),
+ ]
+ page: Annotated[
+ PositiveInt,
+ Field(default=1, description="Page number for pagination (1-based)"),
+ ]
+ page_size: Annotated[
+ int,
+ Field(
+ default=DEFAULT_PAGE_SIZE,
+ gt=0,
+ le=MAX_PAGE_SIZE,
+ description=f"Number of items per page (max {MAX_PAGE_SIZE})",
+ ),
+ ]
+
+ @field_validator("filters", mode="before")
+ @classmethod
+ def parse_filters(cls, v: Any) -> List[ReportFilter]:
+ """Accept both JSON string and list of objects."""
+ return parse_json_or_model_list(v, ReportFilter, "filters")
+
+ @field_validator("select_columns", mode="before")
+ @classmethod
+ def parse_columns(cls, v: Any) -> List[str]:
+ """Accept JSON array, list, or comma-separated string."""
+ return parse_json_or_list(v, "select_columns")
+
+ @model_validator(mode="after")
+ def validate_search_and_filters(self) -> "ListReportsRequest":
+ """Prevent using both search and filters simultaneously."""
+ if self.search and self.filters:
+ raise ValueError(
+ "Cannot use both 'search' and 'filters' parameters simultaneously. "
+ "Use either 'search' for text-based searching across multiple fields, "
+ "or 'filters' for precise column-based filtering, but not both."
+ )
+ return self
+
+
+class ReportError(BaseModel):
+ error: str = Field(..., description="Error message")
+ error_type: str = Field(..., description="Type of error")
+ timestamp: str | datetime | None = Field(None, description="Error timestamp")
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+ @classmethod
+ def create(cls, error: str, error_type: str) -> "ReportError":
+ """Create a standardized ReportError with timestamp."""
+ from datetime import datetime, timezone
+
+ return cls(
+ error=error, error_type=error_type, timestamp=datetime.now(timezone.utc)
+ )
+
+
+class GetReportInfoRequest(MetadataCacheControl):
+ """Request schema for get_report_info — identifier is a numeric ID only."""
+
+ identifier: Annotated[
+ int,
+ Field(description="Report/Alert numeric ID"),
+ ]
+
+
+def _humanize_timestamp(dt: datetime | None) -> str | None:
+ """Convert a datetime to a humanized string like '2 hours ago'."""
+ if dt is None:
+ return None
+ now = datetime.now(dt.tzinfo) if dt.tzinfo else datetime.now()
Review Comment:
<!-- Bito Reply -->
The `/resolve` command is not supported on inline comments. Please use it in a top-level comment instead.
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,119 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset.daos.report import ReportScheduleDAO
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as e:
Review Comment:
<!-- Bito Reply -->
The suggestion to catch specific exceptions like `ValueError`, `KeyError`, and `AttributeError` instead of a broad `Exception` is valid and follows best practices for error handling. It improves code clarity and maintainability by making the intent explicit and reducing the risk of accidentally catching unintended exceptions. The provided code change aligns with this suggestion and should be applied.
**superset/mcp_service/report/tool/get_report_info.py**
```
except (ValueError, KeyError, AttributeError) as e:
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4582045370
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4524436066
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4548531096
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3331578131
##########
superset/mcp_service/common/schema_discovery.py:
##########
@@ -714,3 +746,83 @@ def get_theme_columns() -> list[ColumnMetadata]:
THEME_DEFAULT_COLUMNS,
exclude_columns=set(USER_DIRECTORY_FIELDS),
)
+
+
+def get_report_columns() -> list[ColumnMetadata]:
+ """Get column metadata for ReportSchedule model dynamically."""
+ from superset.reports.models import ReportSchedule
+
+ return get_columns_from_model(
+ ReportSchedule,
+ REPORT_DEFAULT_COLUMNS,
+ REPORT_EXTRA_COLUMNS,
+ exclude_columns=set(USER_DIRECTORY_FIELDS),
+ )
Review Comment:
`get_report_columns()` is defined but not used anywhere in the codebase. The schema tool wires up `get_report_info_columns()` (the ReportInfo-derived variant) instead, and no other call site references this helper. Consider removing it to avoid dead code, or wire it into a caller that needs the ORM-derived column set.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3313129453
##########
superset/daos/report.py:
##########
@@ -42,6 +45,51 @@
class ReportScheduleDAO(BaseDAO[ReportSchedule]):
base_filter = ReportScheduleFilter
+ @classmethod
+ def apply_column_operators(
+ cls,
+ query: Any,
+ column_operators: list[ColumnOperator] | None = None,
+ ) -> Any:
+ """Override to handle owners.id and created_by_fk_or_owner via subqueries.
+
+ - owners.id: filters reports by owner user ID via report_schedule_user M2M table
+ - created_by_fk_or_owner: OR(created_by_fk == value, id IN owners_subq)
+ """
+ if not column_operators:
+ return query
+
+ remaining_operators: list[ColumnOperator] = []
+ for c in column_operators:
+ if not isinstance(c, ColumnOperator):
+ c = ColumnOperator.model_validate(c)
+ if c.col == "owners.id":
+ operator_enum = ColumnOperatorEnum(c.opr)
+ subq = select(report_schedule_user.c.report_schedule_id).where(
+ operator_enum.apply(report_schedule_user.c.user_id, c.value)
+ )
+ query = query.filter(ReportSchedule.id.in_(subq))
+ elif c.col == "created_by_fk_or_owner":
+ if c.opr != "eq":
+ raise ValueError(
+ f"created_by_fk_or_owner only supports 'eq'; got '{c.opr}'"
+ )
+ owner_subq = select(report_schedule_user.c.report_schedule_id).where(
+ report_schedule_user.c.user_id == c.value
+ )
+ query = query.filter(
+ or_(
+ ReportSchedule.created_by_fk == c.value,
+ ReportSchedule.id.in_(owner_subq),
+ )
+ )
+ else:
+ remaining_operators.append(c)
+
+ if remaining_operators:
+ query = super().apply_column_operators(query, remaining_operators)
+ return query
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for created_by_fk_or_owner</b></div>
<div id="fix">
The new `created_by_fk_or_owner` virtual column filter introduced on line 72 has no dedicated unit test within the `ReportScheduleDAO` test suite. Existing tests only verify the `owners.id` filter (test_list_reports_owned_by_me_passed_to_dao) and `created_by_fk` filter (test_list_reports_created_by_me_passed_to_dao), but not the combined case that uses the new handler. Per rule [6262], tests should verify actual business logic directly.
</div>
</div>
<small><i>Code Review Run #3423a4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/report/tool/test_report_tools.py:
##########
@@ -0,0 +1,457 @@
+# 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.
+
+import logging
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from fastmcp.exceptions import ToolError
+from pydantic import ValidationError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.report.schemas import ListReportsRequest, ReportFilter
+from superset.utils import json
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+def create_mock_report(
+ report_id: int = 1,
+ name: str = "Daily Sales Report",
+ report_type: str = "Report",
+ active: bool = True,
+ crontab: str = "0 9 * * *",
+ description: str = "A daily report",
+ dashboard_id: int | None = None,
+ chart_id: int | None = None,
+) -> MagicMock:
+ """Factory function to create mock report objects with sensible defaults."""
+ report = MagicMock()
+ report.id = report_id
+ report.name = name
+ report.type = report_type
+ report.active = active
+ report.crontab = crontab
+ report.description = description
+ report.dashboard_id = dashboard_id
+ report.chart_id = chart_id
+ report.owners = []
+ report.changed_on = None
+ report.created_on = None
+ return report
+
+
+@pytest.fixture
+def mcp_server():
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
+ """Mock authentication for all tests."""
+ from unittest.mock import Mock
+
+ 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
+
+
+class TestReportFilterSchema:
+ """Tests for ReportFilter schema — filterable columns."""
+
+ def test_valid_filter_name(self):
+ f = ReportFilter(col="name", opr="eq", value="My Report")
+ assert f.col == "name"
+
+ def test_valid_filter_type(self):
+ f = ReportFilter(col="type", opr="eq", value="Alert")
+ assert f.col == "type"
+
+ def test_valid_filter_active(self):
+ f = ReportFilter(col="active", opr="eq", value=True)
+ assert f.col == "active"
+
+ def test_valid_filter_dashboard_id(self):
+ f = ReportFilter(col="dashboard_id", opr="eq", value=1)
+ assert f.col == "dashboard_id"
+
+ def test_valid_filter_chart_id(self):
+ f = ReportFilter(col="chart_id", opr="eq", value=42)
+ assert f.col == "chart_id"
+
+ def test_invalid_filter_column_rejected(self):
+ """Columns not in the Literal set must be rejected."""
+ with pytest.raises(ValidationError):
+ ReportFilter(col="not_a_real_column", opr="eq", value=1)
+
+ def test_created_by_fk_is_accepted(self):
+ """created_by_fk is a valid public filter column."""
+ f = ReportFilter(col="created_by_fk", opr="eq", value=1)
+ assert f.col == "created_by_fk"
+
+
+def test_list_reports_request_accepts_valid_fields():
+ request = ListReportsRequest(page=1, page_size=10)
+ assert request.page == 1
+ assert request.page_size == 10
+
+
+def test_list_reports_request_rejects_search_and_filters_together():
+ with pytest.raises(ValidationError):
+ ListReportsRequest(
+ search="my report",
+ filters=[{"col": "active", "opr": "eq", "value": True}],
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_basic(mock_list, mcp_server):
+ """Test basic report listing functionality."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["id"] == 1
+ assert data["reports"][0]["name"] == "Daily Sales Report"
+ assert data["reports"][0]["type"] == "Report"
+ assert data["reports"][0]["active"] is True
+ assert data["reports"][0]["crontab"] == "0 9 * * *"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_search(mock_list, mcp_server):
+ """Test report listing with search functionality."""
+ report = create_mock_report(name="Weekly Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, search="Weekly")
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["name"] == "Weekly Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_type_filter(mock_list, mcp_server):
+ """Test report listing filtered by type."""
+ report = create_mock_report(report_type="Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ filters=[{"col": "type", "opr": "eq", "value": "Alert"}],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["type"] == "Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_does_not_expose_owners(mock_list, mcp_server):
+ """Test that owners field is stripped by privacy controls."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ select_columns=["id", "name", "owners"],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ # owners is filtered by USER_DIRECTORY_FIELDS
+ assert "owners" not in data.get("columns_requested", [])
+ assert "owners" not in data.get("columns_loaded", [])
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_empty_results(mock_list, mcp_server):
+ """Test report listing with no results."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["count"] == 0
+ assert data["total_count"] == 0
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_api_error(mock_list, mcp_server):
+ """Test error handling when DAO raises an exception."""
+ mock_list.side_effect = ToolError("Report DAO error")
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ with pytest.raises(ToolError) as excinfo: # noqa: PT012
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+ assert "Report DAO error" in str(excinfo.value)
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_without_request_uses_defaults(mock_list, mcp_server):
+ """list_reports with no request payload should use default parameters."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("list_reports", {})
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["page"] == 1
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_basic(mock_find, mcp_server):
+ """Test basic get report info functionality."""
+ report = create_mock_report()
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["id"] == 1
+ assert data["name"] == "Daily Sales Report"
+ assert data["type"] == "Report"
+ assert data["active"] is True
+ assert data["crontab"] == "0 9 * * *"
+ assert "owners" not in data
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_alert_type(mock_find, mcp_server):
+ """Test get report info for an Alert type schedule."""
+ report = create_mock_report(report_type="Alert", name="Revenue Alert")
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["type"] == "Alert"
+ assert data["name"] == "Revenue Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_not_found(mock_find, mcp_server):
+ """Test get report info when report does not exist."""
+ mock_find.return_value = None
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 999}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "not_found"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_dashboard(mock_find, mcp_server):
+ """Test get report info with associated dashboard."""
+ report = create_mock_report(dashboard_id=42)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["dashboard_id"] == 42
+ assert data["chart_id"] is None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_chart(mock_find, mcp_server):
+ """Test get report info with associated chart."""
+ report = create_mock_report(chart_id=7)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["chart_id"] == 7
+ assert data["dashboard_id"] is None
+
+
+def test_list_reports_request_rejects_invalid_order_column():
+ """order_column is validated against REPORT_SORTABLE_COLUMNS."""
+ from superset.mcp_service.common.schema_discovery import REPORT_SORTABLE_COLUMNS
+
+ assert "invalid_column" not in REPORT_SORTABLE_COLUMNS
+ # The validation happens inside ModelListCore, not the request schema,
+ # so we just verify the sortable list doesn't include bad columns.
+ request = ListReportsRequest(page=1, page_size=10, order_column="invalid_column")
+ assert (
+ request.order_column == "invalid_column"
+ ) # schema accepts it; core rejects it
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_humanized_timestamps(mock_find, mcp_server):
+ """Test that changed_on_humanized and created_on_humanized are returned."""
+ from datetime import datetime, timezone
+
+ report = create_mock_report()
+ report.changed_on = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
+ report.created_on = datetime(2024, 1, 1, 10, 0, 0, tzinfo=timezone.utc)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert "changed_on_humanized" in data
+ assert data["changed_on_humanized"] is not None
+ assert "created_on_humanized" in data
+ assert data["created_on_humanized"] is not None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_owned_by_me_passed_to_dao(mock_list, mcp_server):
+ """owned_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, owned_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "owners.id" for f in filters_arg), (
+ "owned_by_me should inject an owners.id filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_created_by_me_passed_to_dao(mock_list, mcp_server):
+ """created_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, created_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "created_by_fk" for f in filters_arg), (
+ "created_by_me should inject a created_by_fk filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_exception_returns_internal_error(mock_find, mcp_server):
+ """Unexpected exception from DAO returns ReportError with InternalError type."""
+ mock_find.side_effect = RuntimeError("DB connection lost")
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "InternalError"
+ assert "DB connection lost" in data["error"]
+
+
+def test_report_error_create_classmethod():
+ """ReportError.create() produces a timestamped error object."""
+ from superset.mcp_service.report.schemas import ReportError
+
+ err = ReportError.create(error="something went wrong", error_type="TestError")
+ assert err.error == "something went wrong"
+ assert err.error_type == "TestError"
+ assert err.timestamp is not None
+
+
+def test_humanize_timestamp_naive_datetime():
+ """_humanize_timestamp handles naive datetimes by assuming UTC."""
+ from datetime import datetime
+
+ from superset.mcp_service.report.schemas import _humanize_timestamp
+
+ naive_dt = datetime(2024, 1, 1, 12, 0, 0)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Datetime without timezone information</b></div>
<div id="fix">
The `datetime.datetime()` call on line 440 is missing a `tzinfo` argument. Add `tzinfo=timezone.utc` to ensure timezone-aware datetime objects.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
naive_dt = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
````
</div>
</details>
</div>
<small><i>Code Review Run #3423a4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3330604247
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,133 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+ - The owners field is excluded by privacy controls and will not appear
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as exc:
Review Comment:
Keeping the broad `except Exception` here — this is an intentional MCP tool-boundary safety net. MCP tools must always return a structured response to the client (never raise), so the outermost catch needs to handle any exception that escapes the inner business logic (SQLAlchemy errors, Pydantic errors, DAO exceptions, etc.). Narrowing to `ValueError, KeyError` would let unexpected exceptions propagate uncaught, crashing the tool call for the client. The `logger.warning(..., exc_info=True)` ensures the full stack trace is visible in server logs.
##########
superset/mcp_service/report/tool/list_reports.py:
##########
@@ -0,0 +1,172 @@
+# 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 datetime import datetime, timezone
+from typing import 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.extensions import event_logger
+from superset.mcp_service.common.schema_discovery import (
+ 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__)
+
+
+_DEFAULT_LIST_REPORTS_REQUEST = ListReportsRequest()
+
+
+@tool(
+ tags=["core"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="List reports",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def list_reports(
+ request: ListReportsRequest | None = None,
+ ctx: Context | None = None,
+) -> ReportList | ReportError:
+ """List alerts and reports with filtering and search.
+
+ Returns schedule metadata including name, type (Alert/Report), active
+ status, and cron expression.
+
+ Sortable columns for order_column: id, name, type, active, changed_on,
+ created_on
+ """
+ if ctx is None:
+ raise RuntimeError("FastMCP context is required for list_reports")
+
+ request = request or _DEFAULT_LIST_REPORTS_REQUEST.model_copy(deep=True)
+
+ await ctx.info(
+ "Listing reports: page=%s, page_size=%s, search=%s"
+ % (
+ request.page,
+ request.page_size,
+ request.search,
+ )
+ )
+ await ctx.debug(
+ "Report listing parameters: filters=%s, order_column=%s, "
+ "order_direction=%s, select_columns=%s"
+ % (
+ request.filters,
+ request.order_column,
+ request.order_direction,
+ request.select_columns,
+ )
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ def _serialize_report(
+ obj: "ReportSchedule | None", cols: list[str] | None
+ ) -> ReportInfo | None:
+ return serialize_report_object(obj)
+
+ list_tool = ModelListCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ item_serializer=_serialize_report,
+ filter_type=ReportFilter,
+ default_columns=REPORT_DEFAULT_COLUMNS,
+ search_columns=REPORT_SEARCH_COLUMNS,
+ list_field_name="reports",
+ output_list_schema=ReportList,
+ all_columns=list(ReportInfo.model_fields.keys()),
+ sortable_columns=REPORT_SORTABLE_COLUMNS,
+ owner_filter_column="owners.id",
+ logger=logger,
+ )
+
+ with event_logger.log_context(action="mcp.list_reports.query"):
+ result = list_tool.run_tool(
+ filters=request.filters,
+ search=request.search,
+ select_columns=request.select_columns,
+ order_column=request.order_column,
+ order_direction=request.order_direction,
+ page=max(request.page - 1, 0),
+ page_size=request.page_size,
+ created_by_me=request.created_by_me,
+ owned_by_me=request.owned_by_me,
+ )
+
+ await ctx.info(
+ "Reports listed successfully: count=%s, total_count=%s, total_pages=%s"
+ % (
+ len(result.reports) if hasattr(result, "reports") else 0,
+ getattr(result, "total_count", None),
+ getattr(result, "total_pages", None),
+ )
+ )
+
+ columns_to_filter = result.columns_requested
+ with event_logger.log_context(action="mcp.list_reports.serialization"):
+ return result.model_dump(
+ mode="json",
+ context={"select_columns": columns_to_filter},
+ )
+
+ except Exception as e:
Review Comment:
Same reasoning as the companion catch in `get_report_info.py` — this is the outermost MCP tool-boundary safety net. The try block covers DAO calls, core list logic, filter application, and model serialization, any of which can raise SQLAlchemy, Pydantic, or internal errors. Narrowing to `ValueError, KeyError` would leave those uncaught. Returning `ReportError` here keeps the MCP client from receiving a raw exception crash; the full context is captured via `ctx.error`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3330863245
##########
superset/mcp_service/common/schema_discovery.py:
##########
@@ -714,3 +746,83 @@ def get_theme_columns() -> list[ColumnMetadata]:
THEME_DEFAULT_COLUMNS,
exclude_columns=set(USER_DIRECTORY_FIELDS),
)
+
+
+def get_report_columns() -> list[ColumnMetadata]:
+ """Get column metadata for ReportSchedule model dynamically."""
+ from superset.reports.models import ReportSchedule
+
+ return get_columns_from_model(
+ ReportSchedule,
+ REPORT_DEFAULT_COLUMNS,
+ REPORT_EXTRA_COLUMNS,
+ exclude_columns=set(USER_DIRECTORY_FIELDS),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code: unused function</b></div>
<div id="fix">
The `get_report_columns()` function (lines 751-760) is dead code — it is never called. `_get_report_schema_core()` uses `get_report_info_columns()` instead. Removing it prevents maintenance confusion and unused imports.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/mcp_service/common/schema_discovery.py
+++ b/superset/mcp_service/common/schema_discovery.py
@@ -748,17 +748,6 @@ def get_theme_columns() -> list[ColumnMetadata]:
)
-def get_report_columns() -> list[ColumnMetadata]:
- """Get column metadata for ReportSchedule model dynamically."""
- from superset.reports.models import ReportSchedule
-
- return get_columns_from_model(
- ReportSchedule,
- REPORT_DEFAULT_COLUMNS,
- REPORT_EXTRA_COLUMNS,
- exclude_columns=set(USER_DIRECTORY_FIELDS),
- )
-
def _annotation_to_type_str(annotation: Any) -> str:
"""Extract a simple type string from a Python type annotation."""
```
</div>
</details>
</div>
<small><i>Code Review Run #b34f48</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4567382520
Thanks @richardfogaca — both addressed in [`dac10c2490`](https://github.com/aminghadersohi/superset/commit/dac10c2490).
**Permission gap (`get_schema.py`):** Removed the static `class_permission_name="Dataset"` from the `@tool` decorator and replaced it with a per-model-type RBAC check at the top of the function body using `security_manager.can_access("can_read", class_permission)`. Added a `_MODEL_TYPE_CLASS_PERMISSION` map:
- `"chart"` → `"Chart"`
- `"dataset"` → `"Dataset"`
- `"dashboard"` → `"Dashboard"`
- `"database"` → `"Database"`
- `"report"` → `"ReportSchedule"`
A user with only `ReportSchedule` read permission can now call and see `get_schema(model_type="report")`; unauthorized model types raise `MCPPermissionDeniedError` with the correct view name.
**Sanitization (`schemas.py`):** Wrapped `name` and `description` in `serialize_report_object` with `sanitize_for_llm_context(…, field_path=(field_name,))`, matching the pattern used by the chart/dashboard/dataset serializers. Updated four test assertions to use `assertIn` since the values are now wrapped in `<UNTRUSTED-CONTENT>` delimiters.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4587897395
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4565363121
@richardfogaca Fixed in commit 2158b6a7ab.
Added a `REPORT_FILTER_COLUMNS` constant (the exact whitelist from `ReportFilter.col`) to `schema_discovery.py`, and an `include_filter_columns` allowlist parameter to `ModelGetSchemaCore`. `_get_report_schema_core` now passes `include_filter_columns=REPORT_FILTER_COLUMNS`, so `get_schema(model_type='report')` only advertises the 6 filter columns that `list_reports` actually accepts (`name`, `type`, `active`, `dashboard_id`, `chart_id`, `created_by_fk`). ORM-only columns like `timezone`, `sql`, `email_subject` are no longer advertised as filterable.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4565871556
_Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in._
Left two notes below that look worth checking before merge. All line numbers verified against HEAD `2158b6a`.
### Functional - worth investigating before merge
- **`superset/mcp_service/report/tool/list_reports.py:145`**
The report MCP surface does not appear to mirror the REST API's `ALERT_REPORTS` feature-flag guard. `ReportScheduleRestApi` returns 404 when `ALERT_REPORTS` is disabled, but `list_reports`, `get_report_info`, and `get_schema(model_type="report")` can still reach `ReportScheduleDAO` / advertise the schema if MCP is enabled and the user has the permission.
WDYT - could we gate the report MCP entry points the same way as the REST API, or remove/disable these tools from MCP registration when `ALERT_REPORTS` is false?
- **`superset/mcp_service/report/tool/get_report_info.py:122`**
This returns `str(exc)` to the MCP caller for unexpected internal failures. Since this path wraps DAO/database/serialization exceptions, it can leak internal details while the server log already records the full exception with `exc_info=True`.
Small suggestion: could we return a generic `InternalError` message to the client and keep the exception detail server-side only?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4569073981
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4557560738
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4577979779
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3314064569
##########
superset/daos/report.py:
##########
@@ -42,6 +45,51 @@
class ReportScheduleDAO(BaseDAO[ReportSchedule]):
base_filter = ReportScheduleFilter
+ @classmethod
+ def apply_column_operators(
+ cls,
+ query: Any,
+ column_operators: list[ColumnOperator] | None = None,
+ ) -> Any:
+ """Override to handle owners.id and created_by_fk_or_owner via subqueries.
+
+ - owners.id: filters reports by owner user ID via report_schedule_user M2M table
+ - created_by_fk_or_owner: OR(created_by_fk == value, id IN owners_subq)
+ """
+ if not column_operators:
+ return query
+
+ remaining_operators: list[ColumnOperator] = []
+ for c in column_operators:
+ if not isinstance(c, ColumnOperator):
+ c = ColumnOperator.model_validate(c)
+ if c.col == "owners.id":
+ operator_enum = ColumnOperatorEnum(c.opr)
+ subq = select(report_schedule_user.c.report_schedule_id).where(
+ operator_enum.apply(report_schedule_user.c.user_id, c.value)
+ )
+ query = query.filter(ReportSchedule.id.in_(subq))
+ elif c.col == "created_by_fk_or_owner":
+ if c.opr != "eq":
+ raise ValueError(
+ f"created_by_fk_or_owner only supports 'eq'; got '{c.opr}'"
+ )
+ owner_subq = select(report_schedule_user.c.report_schedule_id).where(
+ report_schedule_user.c.user_id == c.value
+ )
+ query = query.filter(
+ or_(
+ ReportSchedule.created_by_fk == c.value,
+ ReportSchedule.id.in_(owner_subq),
+ )
+ )
+ else:
+ remaining_operators.append(c)
+
+ if remaining_operators:
+ query = super().apply_column_operators(query, remaining_operators)
+ return query
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
##########
tests/unit_tests/mcp_service/report/tool/test_report_tools.py:
##########
@@ -0,0 +1,457 @@
+# 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.
+
+import logging
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from fastmcp.exceptions import ToolError
+from pydantic import ValidationError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.report.schemas import ListReportsRequest, ReportFilter
+from superset.utils import json
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+def create_mock_report(
+ report_id: int = 1,
+ name: str = "Daily Sales Report",
+ report_type: str = "Report",
+ active: bool = True,
+ crontab: str = "0 9 * * *",
+ description: str = "A daily report",
+ dashboard_id: int | None = None,
+ chart_id: int | None = None,
+) -> MagicMock:
+ """Factory function to create mock report objects with sensible defaults."""
+ report = MagicMock()
+ report.id = report_id
+ report.name = name
+ report.type = report_type
+ report.active = active
+ report.crontab = crontab
+ report.description = description
+ report.dashboard_id = dashboard_id
+ report.chart_id = chart_id
+ report.owners = []
+ report.changed_on = None
+ report.created_on = None
+ return report
+
+
+@pytest.fixture
+def mcp_server():
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
+ """Mock authentication for all tests."""
+ from unittest.mock import Mock
+
+ 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
+
+
+class TestReportFilterSchema:
+ """Tests for ReportFilter schema — filterable columns."""
+
+ def test_valid_filter_name(self):
+ f = ReportFilter(col="name", opr="eq", value="My Report")
+ assert f.col == "name"
+
+ def test_valid_filter_type(self):
+ f = ReportFilter(col="type", opr="eq", value="Alert")
+ assert f.col == "type"
+
+ def test_valid_filter_active(self):
+ f = ReportFilter(col="active", opr="eq", value=True)
+ assert f.col == "active"
+
+ def test_valid_filter_dashboard_id(self):
+ f = ReportFilter(col="dashboard_id", opr="eq", value=1)
+ assert f.col == "dashboard_id"
+
+ def test_valid_filter_chart_id(self):
+ f = ReportFilter(col="chart_id", opr="eq", value=42)
+ assert f.col == "chart_id"
+
+ def test_invalid_filter_column_rejected(self):
+ """Columns not in the Literal set must be rejected."""
+ with pytest.raises(ValidationError):
+ ReportFilter(col="not_a_real_column", opr="eq", value=1)
+
+ def test_created_by_fk_is_accepted(self):
+ """created_by_fk is a valid public filter column."""
+ f = ReportFilter(col="created_by_fk", opr="eq", value=1)
+ assert f.col == "created_by_fk"
+
+
+def test_list_reports_request_accepts_valid_fields():
+ request = ListReportsRequest(page=1, page_size=10)
+ assert request.page == 1
+ assert request.page_size == 10
+
+
+def test_list_reports_request_rejects_search_and_filters_together():
+ with pytest.raises(ValidationError):
+ ListReportsRequest(
+ search="my report",
+ filters=[{"col": "active", "opr": "eq", "value": True}],
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_basic(mock_list, mcp_server):
+ """Test basic report listing functionality."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["id"] == 1
+ assert data["reports"][0]["name"] == "Daily Sales Report"
+ assert data["reports"][0]["type"] == "Report"
+ assert data["reports"][0]["active"] is True
+ assert data["reports"][0]["crontab"] == "0 9 * * *"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_search(mock_list, mcp_server):
+ """Test report listing with search functionality."""
+ report = create_mock_report(name="Weekly Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, search="Weekly")
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["name"] == "Weekly Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_type_filter(mock_list, mcp_server):
+ """Test report listing filtered by type."""
+ report = create_mock_report(report_type="Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ filters=[{"col": "type", "opr": "eq", "value": "Alert"}],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["type"] == "Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_does_not_expose_owners(mock_list, mcp_server):
+ """Test that owners field is stripped by privacy controls."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ select_columns=["id", "name", "owners"],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ # owners is filtered by USER_DIRECTORY_FIELDS
+ assert "owners" not in data.get("columns_requested", [])
+ assert "owners" not in data.get("columns_loaded", [])
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_empty_results(mock_list, mcp_server):
+ """Test report listing with no results."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["count"] == 0
+ assert data["total_count"] == 0
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_api_error(mock_list, mcp_server):
+ """Test error handling when DAO raises an exception."""
+ mock_list.side_effect = ToolError("Report DAO error")
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ with pytest.raises(ToolError) as excinfo: # noqa: PT012
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+ assert "Report DAO error" in str(excinfo.value)
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_without_request_uses_defaults(mock_list, mcp_server):
+ """list_reports with no request payload should use default parameters."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("list_reports", {})
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["page"] == 1
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_basic(mock_find, mcp_server):
+ """Test basic get report info functionality."""
+ report = create_mock_report()
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["id"] == 1
+ assert data["name"] == "Daily Sales Report"
+ assert data["type"] == "Report"
+ assert data["active"] is True
+ assert data["crontab"] == "0 9 * * *"
+ assert "owners" not in data
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_alert_type(mock_find, mcp_server):
+ """Test get report info for an Alert type schedule."""
+ report = create_mock_report(report_type="Alert", name="Revenue Alert")
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["type"] == "Alert"
+ assert data["name"] == "Revenue Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_not_found(mock_find, mcp_server):
+ """Test get report info when report does not exist."""
+ mock_find.return_value = None
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 999}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "not_found"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_dashboard(mock_find, mcp_server):
+ """Test get report info with associated dashboard."""
+ report = create_mock_report(dashboard_id=42)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["dashboard_id"] == 42
+ assert data["chart_id"] is None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_chart(mock_find, mcp_server):
+ """Test get report info with associated chart."""
+ report = create_mock_report(chart_id=7)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["chart_id"] == 7
+ assert data["dashboard_id"] is None
+
+
+def test_list_reports_request_rejects_invalid_order_column():
+ """order_column is validated against REPORT_SORTABLE_COLUMNS."""
+ from superset.mcp_service.common.schema_discovery import REPORT_SORTABLE_COLUMNS
+
+ assert "invalid_column" not in REPORT_SORTABLE_COLUMNS
+ # The validation happens inside ModelListCore, not the request schema,
+ # so we just verify the sortable list doesn't include bad columns.
+ request = ListReportsRequest(page=1, page_size=10, order_column="invalid_column")
+ assert (
+ request.order_column == "invalid_column"
+ ) # schema accepts it; core rejects it
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_humanized_timestamps(mock_find, mcp_server):
+ """Test that changed_on_humanized and created_on_humanized are returned."""
+ from datetime import datetime, timezone
+
+ report = create_mock_report()
+ report.changed_on = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
+ report.created_on = datetime(2024, 1, 1, 10, 0, 0, tzinfo=timezone.utc)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert "changed_on_humanized" in data
+ assert data["changed_on_humanized"] is not None
+ assert "created_on_humanized" in data
+ assert data["created_on_humanized"] is not None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_owned_by_me_passed_to_dao(mock_list, mcp_server):
+ """owned_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, owned_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "owners.id" for f in filters_arg), (
+ "owned_by_me should inject an owners.id filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_created_by_me_passed_to_dao(mock_list, mcp_server):
+ """created_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, created_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "created_by_fk" for f in filters_arg), (
+ "created_by_me should inject a created_by_fk filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_exception_returns_internal_error(mock_find, mcp_server):
+ """Unexpected exception from DAO returns ReportError with InternalError type."""
+ mock_find.side_effect = RuntimeError("DB connection lost")
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "InternalError"
+ assert "DB connection lost" in data["error"]
+
+
+def test_report_error_create_classmethod():
+ """ReportError.create() produces a timestamped error object."""
+ from superset.mcp_service.report.schemas import ReportError
+
+ err = ReportError.create(error="something went wrong", error_type="TestError")
+ assert err.error == "something went wrong"
+ assert err.error_type == "TestError"
+ assert err.timestamp is not None
+
+
+def test_humanize_timestamp_naive_datetime():
+ """_humanize_timestamp handles naive datetimes by assuming UTC."""
+ from datetime import datetime
+
+ from superset.mcp_service.report.schemas import _humanize_timestamp
+
+ naive_dt = datetime(2024, 1, 1, 12, 0, 0)
Review Comment:
<!-- Bito Reply -->
The suggestion in question is asking whether Bito should avoid suggesting adding `tzinfo=timezone.utc` to the `datetime` call in the future. The suggestion is valid because the code explicitly creates a naive datetime (without timezone information) for testing purposes, which is intentional to exercise a specific code branch. Adding the `tzinfo` argument would make the datetime timezone-aware, which is not the intended behavior for this test case. Therefore, Bito should avoid making such suggestions for future reviews in this context.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3330604620
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,133 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+ - The owners field is excluded by privacy controls and will not appear
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as exc:
Review Comment:
<!-- Bito Reply -->
The suggestion to avoid catching broad `Exception` is valid in this context. The current code catches `ValueError` and `KeyError`, but the reviewer recommends catching more specific exceptions to align with best practices (like BLE001 rule). This improves error handling by ensuring only expected exceptions are caught, while unexpected ones propagate properly. The suggestion is appropriate to apply if the intention is to handle only `ValueError` and `KeyError`, and not other unanticipated exceptions.
**superset/mcp_service/report/tool/get_report_info.py**
```
except (ValueError, KeyError) as exc:
```
##########
superset/mcp_service/report/tool/list_reports.py:
##########
@@ -0,0 +1,172 @@
+# 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 datetime import datetime, timezone
+from typing import 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.extensions import event_logger
+from superset.mcp_service.common.schema_discovery import (
+ 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__)
+
+
+_DEFAULT_LIST_REPORTS_REQUEST = ListReportsRequest()
+
+
+@tool(
+ tags=["core"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="List reports",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def list_reports(
+ request: ListReportsRequest | None = None,
+ ctx: Context | None = None,
+) -> ReportList | ReportError:
+ """List alerts and reports with filtering and search.
+
+ Returns schedule metadata including name, type (Alert/Report), active
+ status, and cron expression.
+
+ Sortable columns for order_column: id, name, type, active, changed_on,
+ created_on
+ """
+ if ctx is None:
+ raise RuntimeError("FastMCP context is required for list_reports")
+
+ request = request or _DEFAULT_LIST_REPORTS_REQUEST.model_copy(deep=True)
+
+ await ctx.info(
+ "Listing reports: page=%s, page_size=%s, search=%s"
+ % (
+ request.page,
+ request.page_size,
+ request.search,
+ )
+ )
+ await ctx.debug(
+ "Report listing parameters: filters=%s, order_column=%s, "
+ "order_direction=%s, select_columns=%s"
+ % (
+ request.filters,
+ request.order_column,
+ request.order_direction,
+ request.select_columns,
+ )
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ def _serialize_report(
+ obj: "ReportSchedule | None", cols: list[str] | None
+ ) -> ReportInfo | None:
+ return serialize_report_object(obj)
+
+ list_tool = ModelListCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ item_serializer=_serialize_report,
+ filter_type=ReportFilter,
+ default_columns=REPORT_DEFAULT_COLUMNS,
+ search_columns=REPORT_SEARCH_COLUMNS,
+ list_field_name="reports",
+ output_list_schema=ReportList,
+ all_columns=list(ReportInfo.model_fields.keys()),
+ sortable_columns=REPORT_SORTABLE_COLUMNS,
+ owner_filter_column="owners.id",
+ logger=logger,
+ )
+
+ with event_logger.log_context(action="mcp.list_reports.query"):
+ result = list_tool.run_tool(
+ filters=request.filters,
+ search=request.search,
+ select_columns=request.select_columns,
+ order_column=request.order_column,
+ order_direction=request.order_direction,
+ page=max(request.page - 1, 0),
+ page_size=request.page_size,
+ created_by_me=request.created_by_me,
+ owned_by_me=request.owned_by_me,
+ )
+
+ await ctx.info(
+ "Reports listed successfully: count=%s, total_count=%s, total_pages=%s"
+ % (
+ len(result.reports) if hasattr(result, "reports") else 0,
+ getattr(result, "total_count", None),
+ getattr(result, "total_pages", None),
+ )
+ )
+
+ columns_to_filter = result.columns_requested
+ with event_logger.log_context(action="mcp.list_reports.serialization"):
+ return result.model_dump(
+ mode="json",
+ context={"select_columns": columns_to_filter},
+ )
+
+ except Exception as e:
Review Comment:
<!-- Bito Reply -->
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4514745726
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4567549154
@richardfogaca — thanks for the thorough review rounds. Here's a summary of what was addressed in this latest pass (commit `f6d712db811bab8ddfaed1b8ee426f208a05adff`):
### Already resolved in earlier commits (for reference)
- **`columns_available` advertising SQLAlchemy-only fields** (review on `63e75441`): Fixed in `1047c6abfa` — `all_columns` is now derived from `ReportInfo.model_fields.keys()`, so only serializable fields are advertised. Regression test `test_columns_available_are_serializable` verifies this.
- **`get_schema(model_type="report")` filter columns mismatch** (review on `c5a3720`): Fixed by adding `REPORT_FILTER_COLUMNS` constant (a `frozenset` of the allowed filter names) and passing it as `include_filter_columns` to `_get_report_schema_core`. The advertised filter list now exactly matches `ReportFilter.col`'s `Literal`. Both are derived from the same constant.
- **`get_schema` using `Dataset` permission for `report` model type** (review on `2158b6a7`): Fixed — `get_schema` now performs per-model-type RBAC using `_MODEL_TYPE_CLASS_PERMISSION`, which maps `"report"` → `"ReportSchedule"`.
- **`name`/`description` sanitization** (reviews on `2158b6a7`): Fixed in `dac10c2490` — `serialize_report_object` wraps `name` and `description` with `sanitize_for_llm_context`.
### New in this commit (`f6d712db`)
**`ALERT_REPORTS` feature flag gate**: Both `list_reports` and `get_report_info` now call `is_feature_enabled("ALERT_REPORTS")` at request time and return a `FeatureDisabled` error when the feature is off, matching the existing REST API's 404 behavior. Tests added for both tools.
**Prompt-injection regression tests**: Added `test_list_reports_name_with_instruction_like_content_is_sanitized` and `test_get_report_info_name_with_instruction_like_content_is_sanitized` — both verify that instruction-like text (e.g. "Ignore all previous instructions…") in `name` and `description` is wrapped in `<UNTRUSTED-CONTENT>` delimiters, not returned verbatim.
Let me know if you'd like any adjustments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4596010742
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3314063638
##########
superset/daos/report.py:
##########
@@ -42,6 +45,51 @@
class ReportScheduleDAO(BaseDAO[ReportSchedule]):
base_filter = ReportScheduleFilter
+ @classmethod
+ def apply_column_operators(
+ cls,
+ query: Any,
+ column_operators: list[ColumnOperator] | None = None,
+ ) -> Any:
+ """Override to handle owners.id and created_by_fk_or_owner via subqueries.
+
+ - owners.id: filters reports by owner user ID via report_schedule_user M2M table
+ - created_by_fk_or_owner: OR(created_by_fk == value, id IN owners_subq)
+ """
+ if not column_operators:
+ return query
+
+ remaining_operators: list[ColumnOperator] = []
+ for c in column_operators:
+ if not isinstance(c, ColumnOperator):
+ c = ColumnOperator.model_validate(c)
+ if c.col == "owners.id":
+ operator_enum = ColumnOperatorEnum(c.opr)
+ subq = select(report_schedule_user.c.report_schedule_id).where(
+ operator_enum.apply(report_schedule_user.c.user_id, c.value)
+ )
+ query = query.filter(ReportSchedule.id.in_(subq))
+ elif c.col == "created_by_fk_or_owner":
+ if c.opr != "eq":
+ raise ValueError(
+ f"created_by_fk_or_owner only supports 'eq'; got '{c.opr}'"
+ )
+ owner_subq = select(report_schedule_user.c.report_schedule_id).where(
+ report_schedule_user.c.user_id == c.value
+ )
+ query = query.filter(
+ or_(
+ ReportSchedule.created_by_fk == c.value,
+ ReportSchedule.id.in_(owner_subq),
+ )
+ )
+ else:
+ remaining_operators.append(c)
+
+ if remaining_operators:
+ query = super().apply_column_operators(query, remaining_operators)
+ return query
Review Comment:
Added in commit 63e75441d1: `test_list_reports_both_owned_and_created_by_me` verifies that when both `owned_by_me=True` and `created_by_me=True` are set, the DAO receives a `created_by_fk_or_owner` OR filter (not two separate AND filters).
##########
tests/unit_tests/mcp_service/report/tool/test_report_tools.py:
##########
@@ -0,0 +1,457 @@
+# 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.
+
+import logging
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from fastmcp.exceptions import ToolError
+from pydantic import ValidationError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.report.schemas import ListReportsRequest, ReportFilter
+from superset.utils import json
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+def create_mock_report(
+ report_id: int = 1,
+ name: str = "Daily Sales Report",
+ report_type: str = "Report",
+ active: bool = True,
+ crontab: str = "0 9 * * *",
+ description: str = "A daily report",
+ dashboard_id: int | None = None,
+ chart_id: int | None = None,
+) -> MagicMock:
+ """Factory function to create mock report objects with sensible defaults."""
+ report = MagicMock()
+ report.id = report_id
+ report.name = name
+ report.type = report_type
+ report.active = active
+ report.crontab = crontab
+ report.description = description
+ report.dashboard_id = dashboard_id
+ report.chart_id = chart_id
+ report.owners = []
+ report.changed_on = None
+ report.created_on = None
+ return report
+
+
+@pytest.fixture
+def mcp_server():
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
+ """Mock authentication for all tests."""
+ from unittest.mock import Mock
+
+ 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
+
+
+class TestReportFilterSchema:
+ """Tests for ReportFilter schema — filterable columns."""
+
+ def test_valid_filter_name(self):
+ f = ReportFilter(col="name", opr="eq", value="My Report")
+ assert f.col == "name"
+
+ def test_valid_filter_type(self):
+ f = ReportFilter(col="type", opr="eq", value="Alert")
+ assert f.col == "type"
+
+ def test_valid_filter_active(self):
+ f = ReportFilter(col="active", opr="eq", value=True)
+ assert f.col == "active"
+
+ def test_valid_filter_dashboard_id(self):
+ f = ReportFilter(col="dashboard_id", opr="eq", value=1)
+ assert f.col == "dashboard_id"
+
+ def test_valid_filter_chart_id(self):
+ f = ReportFilter(col="chart_id", opr="eq", value=42)
+ assert f.col == "chart_id"
+
+ def test_invalid_filter_column_rejected(self):
+ """Columns not in the Literal set must be rejected."""
+ with pytest.raises(ValidationError):
+ ReportFilter(col="not_a_real_column", opr="eq", value=1)
+
+ def test_created_by_fk_is_accepted(self):
+ """created_by_fk is a valid public filter column."""
+ f = ReportFilter(col="created_by_fk", opr="eq", value=1)
+ assert f.col == "created_by_fk"
+
+
+def test_list_reports_request_accepts_valid_fields():
+ request = ListReportsRequest(page=1, page_size=10)
+ assert request.page == 1
+ assert request.page_size == 10
+
+
+def test_list_reports_request_rejects_search_and_filters_together():
+ with pytest.raises(ValidationError):
+ ListReportsRequest(
+ search="my report",
+ filters=[{"col": "active", "opr": "eq", "value": True}],
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_basic(mock_list, mcp_server):
+ """Test basic report listing functionality."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["id"] == 1
+ assert data["reports"][0]["name"] == "Daily Sales Report"
+ assert data["reports"][0]["type"] == "Report"
+ assert data["reports"][0]["active"] is True
+ assert data["reports"][0]["crontab"] == "0 9 * * *"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_search(mock_list, mcp_server):
+ """Test report listing with search functionality."""
+ report = create_mock_report(name="Weekly Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, search="Weekly")
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["reports"] is not None
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["name"] == "Weekly Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_with_type_filter(mock_list, mcp_server):
+ """Test report listing filtered by type."""
+ report = create_mock_report(report_type="Alert")
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ filters=[{"col": "type", "opr": "eq", "value": "Alert"}],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert len(data["reports"]) == 1
+ assert data["reports"][0]["type"] == "Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_does_not_expose_owners(mock_list, mcp_server):
+ """Test that owners field is stripped by privacy controls."""
+ report = create_mock_report()
+ mock_list.return_value = ([report], 1)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(
+ page=1,
+ page_size=10,
+ select_columns=["id", "name", "owners"],
+ )
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ # owners is filtered by USER_DIRECTORY_FIELDS
+ assert "owners" not in data.get("columns_requested", [])
+ assert "owners" not in data.get("columns_loaded", [])
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_empty_results(mock_list, mcp_server):
+ """Test report listing with no results."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ result = await client.call_tool(
+ "list_reports", {"request": request.model_dump()}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["count"] == 0
+ assert data["total_count"] == 0
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_api_error(mock_list, mcp_server):
+ """Test error handling when DAO raises an exception."""
+ mock_list.side_effect = ToolError("Report DAO error")
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10)
+ with pytest.raises(ToolError) as excinfo: # noqa: PT012
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+ assert "Report DAO error" in str(excinfo.value)
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_without_request_uses_defaults(mock_list, mcp_server):
+ """list_reports with no request payload should use default parameters."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("list_reports", {})
+ data = json.loads(result.content[0].text)
+ assert data["reports"] == []
+ assert data["page"] == 1
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_basic(mock_find, mcp_server):
+ """Test basic get report info functionality."""
+ report = create_mock_report()
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ assert result.content is not None
+ data = json.loads(result.content[0].text)
+ assert data["id"] == 1
+ assert data["name"] == "Daily Sales Report"
+ assert data["type"] == "Report"
+ assert data["active"] is True
+ assert data["crontab"] == "0 9 * * *"
+ assert "owners" not in data
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_alert_type(mock_find, mcp_server):
+ """Test get report info for an Alert type schedule."""
+ report = create_mock_report(report_type="Alert", name="Revenue Alert")
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["type"] == "Alert"
+ assert data["name"] == "Revenue Alert"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_not_found(mock_find, mcp_server):
+ """Test get report info when report does not exist."""
+ mock_find.return_value = None
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 999}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "not_found"
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_dashboard(mock_find, mcp_server):
+ """Test get report info with associated dashboard."""
+ report = create_mock_report(dashboard_id=42)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["dashboard_id"] == 42
+ assert data["chart_id"] is None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_with_chart(mock_find, mcp_server):
+ """Test get report info with associated chart."""
+ report = create_mock_report(chart_id=7)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["chart_id"] == 7
+ assert data["dashboard_id"] is None
+
+
+def test_list_reports_request_rejects_invalid_order_column():
+ """order_column is validated against REPORT_SORTABLE_COLUMNS."""
+ from superset.mcp_service.common.schema_discovery import REPORT_SORTABLE_COLUMNS
+
+ assert "invalid_column" not in REPORT_SORTABLE_COLUMNS
+ # The validation happens inside ModelListCore, not the request schema,
+ # so we just verify the sortable list doesn't include bad columns.
+ request = ListReportsRequest(page=1, page_size=10, order_column="invalid_column")
+ assert (
+ request.order_column == "invalid_column"
+ ) # schema accepts it; core rejects it
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_humanized_timestamps(mock_find, mcp_server):
+ """Test that changed_on_humanized and created_on_humanized are returned."""
+ from datetime import datetime, timezone
+
+ report = create_mock_report()
+ report.changed_on = datetime(2024, 1, 1, 12, 0, 0, tzinfo=timezone.utc)
+ report.created_on = datetime(2024, 1, 1, 10, 0, 0, tzinfo=timezone.utc)
+ mock_find.return_value = report
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert "changed_on_humanized" in data
+ assert data["changed_on_humanized"] is not None
+ assert "created_on_humanized" in data
+ assert data["created_on_humanized"] is not None
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_owned_by_me_passed_to_dao(mock_list, mcp_server):
+ """owned_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, owned_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "owners.id" for f in filters_arg), (
+ "owned_by_me should inject an owners.id filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.list")
+@pytest.mark.asyncio
+async def test_list_reports_created_by_me_passed_to_dao(mock_list, mcp_server):
+ """created_by_me=True is forwarded to the DAO layer."""
+ mock_list.return_value = ([], 0)
+
+ async with Client(mcp_server) as client:
+ request = ListReportsRequest(page=1, page_size=10, created_by_me=True)
+ await client.call_tool("list_reports", {"request": request.model_dump()})
+
+ mock_list.assert_called_once()
+ _, kwargs = mock_list.call_args
+ filters_arg = kwargs.get("column_operators", [])
+ assert any(getattr(f, "col", None) == "created_by_fk" for f in filters_arg), (
+ "created_by_me should inject a created_by_fk filter into the DAO call"
+ )
+
+
+@patch("superset.daos.report.ReportScheduleDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_get_report_info_exception_returns_internal_error(mock_find, mcp_server):
+ """Unexpected exception from DAO returns ReportError with InternalError type."""
+ mock_find.side_effect = RuntimeError("DB connection lost")
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_report_info", {"request": {"identifier": 1}}
+ )
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "InternalError"
+ assert "DB connection lost" in data["error"]
+
+
+def test_report_error_create_classmethod():
+ """ReportError.create() produces a timestamped error object."""
+ from superset.mcp_service.report.schemas import ReportError
+
+ err = ReportError.create(error="something went wrong", error_type="TestError")
+ assert err.error == "something went wrong"
+ assert err.error_type == "TestError"
+ assert err.timestamp is not None
+
+
+def test_humanize_timestamp_naive_datetime():
+ """_humanize_timestamp handles naive datetimes by assuming UTC."""
+ from datetime import datetime
+
+ from superset.mcp_service.report.schemas import _humanize_timestamp
+
+ naive_dt = datetime(2024, 1, 1, 12, 0, 0)
Review Comment:
The naive datetime is intentional — this test specifically exercises the `if dt.tzinfo is None` branch in `_humanize_timestamp` (which does `dt.replace(tzinfo=timezone.utc)`). Making it timezone-aware would duplicate `test_get_report_info_humanized_timestamps` and leave that branch uncovered.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4559955149
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4567721613
Thanks for the continued thorough review. Addressed all remaining points in commit de2a1d9b34:
**DRY: `owner_filter_column` replaces `ReportListCore` subclass**
- Added `owner_filter_column: str = "owner"` parameter to `ModelListCore.__init__`
- Changed `_prepend_self_lookup_filters` from `@staticmethod` to instance method using `self._owner_filter_column`
- `ReportListCore` and its duplicate `_prepend_self_lookup_filters` override are now gone — `list_reports.py` passes `owner_filter_column="owners.id"` directly to `ModelListCore`
**`get_schema` — ALERT_REPORTS gate**
- Added feature flag check for `model_type="report"` before running the schema core, consistent with `list_reports` and `get_report_info`
**`get_schema` — `MCP_RBAC_ENABLED` guard**
- Wrapped the manual `security_manager.can_access` call with `current_app.config.get("MCP_RBAC_ENABLED", True)` so it respects the same config flag that `check_tool_permission()` honors
**`serialize_report_object` — ORM owner safety**
- Always passes `owners=None` instead of raw ORM objects, preventing JSON serialization failure before `filter_user_directory_fields` has a chance to strip the field
**`ReportError` — error text sanitization**
- Added `@field_validator("error")` applying `sanitize_for_llm_context`, matching `DashboardError`/`DatasetError`
**Tests — drift prevention**
- Added `test_get_schema_permission_map_has_report_schedule`: asserts `_MODEL_TYPE_CLASS_PERMISSION["report"] == "ReportSchedule"` and matches both report tool functions' `class_permission_name`
- Added `test_report_filter_columns_match_schema_discovery_frozenset`: asserts `ReportFilter.col` Literal values equal `REPORT_FILTER_COLUMNS` frozenset to catch future out-of-sync edits
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3305638421
##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,290 @@
+# 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
+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(
+ ...,
+ description="Column to filter on. Use get_schema(model_type='report') for "
+ "available filter columns.",
+ )
+ opr: ColumnOperatorEnum = Field(
+ ...,
+ description="Operator to use. Use get_schema(model_type='report') for "
+ "available operators.",
+ )
+ value: str | int | float | bool | List[str | int | float | bool] = Field(
+ ..., description="Value to filter by (type depends on col and opr)"
+ )
+
+
+class ReportInfo(BaseModel):
+ id: int | None = Field(None, description="Report/Alert ID")
+ name: str | None = Field(None, description="Report/Alert name")
+ description: str | None = Field(None, description="Report/Alert description")
+ type: str | None = Field(None, description="Schedule type: 'Alert' or 'Report'")
+ active: bool | None = Field(None, description="Whether the schedule is active")
+ crontab: str | None = Field(None, description="Cron expression for scheduling")
+ dashboard_id: int | None = Field(
+ None, description="Associated dashboard ID, if any"
+ )
+ chart_id: int | None = Field(None, description="Associated chart ID, if any")
+ owners: List[Any] | None = Field(
+ None, description="List of owners (filtered by privacy controls)"
+ )
+ changed_on: str | datetime | None = Field(
+ None, description="Last modification timestamp"
+ )
+ changed_on_humanized: str | None = Field(
+ None, description="Humanized modification time"
+ )
+ created_on: str | datetime | None = Field(None, description="Creation timestamp")
+ created_on_humanized: str | None = Field(
+ None, description="Humanized creation time"
+ )
+ model_config = ConfigDict(
+ from_attributes=True,
+ ser_json_timedelta="iso8601",
+ populate_by_name=True,
+ )
+
+ @model_serializer(mode="wrap")
+ def _filter_fields_by_context(self, serializer: Any, info: Any) -> Dict[str, Any]:
+ """Filter fields based on serialization context.
+
+ If context contains 'select_columns', only include those fields.
+ Otherwise, include all fields (default behavior).
+ """
+ data = filter_user_directory_fields(serializer(self))
+
+ if info.context and isinstance(info.context, dict):
+ select_columns = info.context.get("select_columns")
+ if select_columns:
+ requested_fields = set(select_columns)
+ return {k: v for k, v in data.items() if k in requested_fields}
+
+ return data
+
+
+class ReportList(BaseModel):
+ reports: List[ReportInfo]
+ count: int
+ total_count: int
+ page: int
+ page_size: int
+ total_pages: int
+ has_previous: bool
+ has_next: bool
+ columns_requested: List[str] = Field(
+ default_factory=list,
+ description="Requested columns for the response",
+ )
+ columns_loaded: List[str] = Field(
+ default_factory=list,
+ description="Columns that were actually loaded for each report",
+ )
+ columns_available: List[str] = Field(
+ default_factory=list,
+ description="All columns available for selection via select_columns parameter",
+ )
+ sortable_columns: List[str] = Field(
+ default_factory=list,
+ description="Columns that can be used with order_column parameter",
+ )
+ filters_applied: List[ColumnOperator] = Field(
+ default_factory=list,
+ description="List of advanced filter dicts applied to the query.",
+ )
+ pagination: PaginationInfo | None = None
+ timestamp: datetime | None = None
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+
+class ListReportsRequest(OwnedByMeMixin, CreatedByMeMixin, MetadataCacheControl):
+ """Request schema for list_reports."""
+
+ filters: Annotated[
+ List[ReportFilter],
+ Field(
+ default_factory=list,
+ description="List of filter objects (column, operator, value). Each "
+ "filter is an object with 'col', 'opr', and 'value' "
+ "properties. Cannot be used together with 'search'.",
+ ),
+ ]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=list,
+ description="List of columns to select. Defaults to common columns if not "
+ "specified.",
+ ),
+ ]
+ search: Annotated[
+ str | None,
+ Field(
+ default=None,
+ description="Text search string to match against report fields. Cannot "
+ "be used together with 'filters'.",
+ ),
+ ]
+ order_column: Annotated[
+ str | None, Field(default=None, description="Column to order results by")
+ ]
+ order_direction: Annotated[
+ Literal["asc", "desc"],
+ Field(
+ default="desc", description="Direction to order results ('asc' or 'desc')"
+ ),
+ ]
+ page: Annotated[
+ PositiveInt,
+ Field(default=1, description="Page number for pagination (1-based)"),
+ ]
+ page_size: Annotated[
+ int,
+ Field(
+ default=DEFAULT_PAGE_SIZE,
+ gt=0,
+ le=MAX_PAGE_SIZE,
+ description=f"Number of items per page (max {MAX_PAGE_SIZE})",
+ ),
+ ]
+
+ @field_validator("filters", mode="before")
+ @classmethod
+ def parse_filters(cls, v: Any) -> List[ReportFilter]:
+ """Accept both JSON string and list of objects."""
+ return parse_json_or_model_list(v, ReportFilter, "filters")
+
+ @field_validator("select_columns", mode="before")
+ @classmethod
+ def parse_columns(cls, v: Any) -> List[str]:
+ """Accept JSON array, list, or comma-separated string."""
+ return parse_json_or_list(v, "select_columns")
+
+ @model_validator(mode="after")
+ def validate_search_and_filters(self) -> "ListReportsRequest":
+ """Prevent using both search and filters simultaneously."""
+ if self.search and self.filters:
+ raise ValueError(
+ "Cannot use both 'search' and 'filters' parameters simultaneously. "
+ "Use either 'search' for text-based searching across multiple fields, "
+ "or 'filters' for precise column-based filtering, but not both."
+ )
+ return self
+
+
+class ReportError(BaseModel):
+ error: str = Field(..., description="Error message")
+ error_type: str = Field(..., description="Type of error")
+ timestamp: str | datetime | None = Field(None, description="Error timestamp")
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+ @classmethod
+ def create(cls, error: str, error_type: str) -> "ReportError":
+ """Create a standardized ReportError with timestamp."""
+ from datetime import datetime, timezone
+
+ return cls(
+ error=error, error_type=error_type, timestamp=datetime.now(timezone.utc)
+ )
+
+
+class GetReportInfoRequest(MetadataCacheControl):
+ """Request schema for get_report_info — identifier is a numeric ID only."""
+
+ identifier: Annotated[
+ int,
+ Field(description="Report/Alert numeric ID"),
+ ]
+
+
+def _humanize_timestamp(dt: datetime | None) -> str | None:
+ """Convert a datetime to a humanized string like '2 hours ago'."""
+ if dt is None:
+ return None
+ now = datetime.now(dt.tzinfo) if dt.tzinfo else datetime.now()
Review Comment:
Fixed in 394c20e9f7. Added `timezone` import and replaced the naive-datetime branch with `dt.replace(tzinfo=timezone.utc)` before always calling `datetime.now(timezone.utc)`.
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,119 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset.daos.report import ReportScheduleDAO
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as e:
Review Comment:
Fixed in 394c20e9f7. This is intentionally a tool-boundary catch (returning `ReportError` rather than raising, so the LLM client gets a structured error). Added `logger.warning(..., exc_info=True)` before the `ctx.error` call and renamed `e` → `exc` for consistency.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4547109940
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40348?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `58.62069%` with `84 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.39%. Comparing base ([`952a6f3`](https://app.codecov.io/gh/apache/superset/commit/952a6f3a239acd4b9f8fd066131dda2d0887c0fb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`394c20e`](https://app.codecov.io/gh/apache/superset/commit/394c20e9f775a96360283ec5f706628721ad1b96?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40348?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/report/tool/list\_reports.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Freport%2Ftool%2Flist_reports.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvcmVwb3J0L3Rvb2wvbGlzdF9yZXBvcnRzLnB5) | 30.00% | [42 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/report/schemas.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Freport%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvcmVwb3J0L3NjaGVtYXMucHk=) | 77.77% | [21 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...uperset/mcp\_service/report/tool/get\_report\_info.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Freport%2Ftool%2Fget_report_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvcmVwb3J0L3Rvb2wvZ2V0X3JlcG9ydF9pbmZvLnB5) | 41.66% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/common/schema\_discovery.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fcommon%2Fschema_discovery.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY29tbW9uL3NjaGVtYV9kaXNjb3ZlcnkucHk=) | 75.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/mcp\_core.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fmcp_core.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvbWNwX2NvcmUucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/system/tool/get\_schema.py](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsystem%2Ftool%2Fget_schema.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc3lzdGVtL3Rvb2wvZ2V0X3NjaGVtYS5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (952a6f3) and HEAD (394c20e). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (952a6f3) | HEAD (394c20e) |
>|------|------|------|
>|hive|2|1|
>|python|20|2|
>|presto|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40348 +/- ##
==========================================
- Coverage 64.19% 55.39% -8.81%
==========================================
Files 2592 2596 +4
Lines 139229 139429 +200
Branches 32326 32342 +16
==========================================
- Hits 89385 77230 -12155
- Misses 48309 61532 +13223
+ Partials 1535 667 -868
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.30% <58.62%> (+0.06%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.97% <58.62%> (+0.05%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.04% <58.62%> (-19.36%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40348?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4565829243
A few schema issues worth addressing before merge:
**`ReportInfo` missing `last_eval_dttm` and `last_state`**
These are the two most operationally important fields — when did the schedule last run, and what was the outcome. Without them, `get_report_info` can't answer basic monitoring questions. Both exist on the `ReportSchedule` model. Recommend adding to `ReportInfo`, `ALL_REPORT_COLUMNS`, and `serialize_report_object`.
**`ReportInfo.owners: List[Any]` is untyped**
The serializer sets `owners` directly from `getattr(report, "owners", None)` which returns raw FAB User ORM objects. With `from_attributes=True`, Pydantic will introspect these — potentially including fields that shouldn't be in the response. Recommend typing as `List[OwnerInfo]` (or reuse `system.schemas.UserInfo` / a slim `OwnerInfo(id, first_name, last_name)` shape) and serializing explicitly in `serialize_report_object`.
**`ReportFilter` missing `last_state` and `creation_method`**
Currently allows `name`, `type`, `active`, `dashboard_id`, `chart_id`, `created_by_fk`. The most useful missing filter is `last_state` — "show me reports that last failed" is a common operational query. `creation_method` is also useful (distinguish dashboard-created alerts from API-created ones).
**`_humanize_timestamp` is duplicated from `utils.response_utils`**
`superset/mcp_service/report/schemas.py` defines a local `_humanize_timestamp` even though `utils.response_utils.humanize_timestamp` already exists and is used by chart and tag schemas. The implementations differ slightly. Recommend using the shared helper to avoid divergence.
**`ReportList.filters_applied` typed as `List[ColumnOperator]` instead of `List[ReportFilter]`**
All other `List*` response schemas use the typed filter class for this field. Minor inconsistency.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4559797126
@richardfogaca Fixed in commit 1047c6abfa. Replaced `get_all_column_names(get_report_columns())` with `list(ReportInfo.model_fields.keys())` so `columns_available` only advertises fields that `ReportInfo` actually serializes: `id`, `name`, `description`, `type`, `active`, `crontab`, `dashboard_id`, `chart_id`, `changed_on`, `changed_on_humanized`, `created_on`, `created_on_humanized`. ORM-only fields like `timezone`, `sql`, `email_subject`, `validator_config_json` are no longer advertised.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3328372680
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,133 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+ - The owners field is excluded by privacy controls and will not appear
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as exc:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Avoid catching blind Exception</b></div>
<div id="fix">
Replace the broad `Exception` catch with specific exception types (e.g., `ValueError`, `KeyError`, `DatabaseError`) to improve error handling and follow BLE001 rule.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ValueError, KeyError) as exc:
````
</div>
</details>
</div>
<small><i>Code Review Run #c52ebd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/system/tool/get_schema.py:
##########
@@ -182,13 +218,40 @@ async def get_schema(
Column metadata is extracted dynamically from SQLAlchemy models.
Args:
- model_type: One of "chart", "dataset", "dashboard", or "database"
+ model_type: One of "chart", "dataset", "dashboard", "database", or "report"
Returns:
Comprehensive schema information for the requested model type
"""
await ctx.info(f"Getting schema for model_type={request.model_type}")
+ # Per-model-type RBAC check so each model type requires its own class permission
+ # (e.g. ReportSchedule for "report") rather than a single static Dataset guard.
+ class_permission = _MODEL_TYPE_CLASS_PERMISSION.get(request.model_type)
+ if class_permission:
+ from flask import current_app, g
+
+ from superset import security_manager
+
+ if current_app.config.get("MCP_RBAC_ENABLED", True) and not (
+ security_manager.can_access("can_read", class_permission)
+ ):
+ user_str = getattr(getattr(g, "user", None), "username", None)
+ raise MCPPermissionDeniedError(
+ permission_name="can_read",
+ view_name=class_permission,
+ user=user_str,
+ tool_name="get_schema",
+ )
+
+ if request.model_type == "report":
+ from superset import is_feature_enabled
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ raise ValueError(
+ "The Alerts & Reports feature is disabled on this instance."
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for RBAC</b></div>
<div id="fix">
New per-model-type RBAC checks (lines 228-245) and ALERT_REPORTS feature flag gate (lines 247-253) lack test coverage. The existing test `test_get_schema_report_omits_self_referencing_filter_columns` only patches `is_feature_enabled` to return True without verifying permission denial, RBAC bypass, or feature-disabled error paths.
</div>
</div>
<small><i>Code Review Run #c52ebd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/report/tool/list_reports.py:
##########
@@ -0,0 +1,172 @@
+# 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 datetime import datetime, timezone
+from typing import 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.extensions import event_logger
+from superset.mcp_service.common.schema_discovery import (
+ 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__)
+
+
+_DEFAULT_LIST_REPORTS_REQUEST = ListReportsRequest()
+
+
+@tool(
+ tags=["core"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="List reports",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def list_reports(
+ request: ListReportsRequest | None = None,
+ ctx: Context | None = None,
+) -> ReportList | ReportError:
+ """List alerts and reports with filtering and search.
+
+ Returns schedule metadata including name, type (Alert/Report), active
+ status, and cron expression.
+
+ Sortable columns for order_column: id, name, type, active, changed_on,
+ created_on
+ """
+ if ctx is None:
+ raise RuntimeError("FastMCP context is required for list_reports")
+
+ request = request or _DEFAULT_LIST_REPORTS_REQUEST.model_copy(deep=True)
+
+ await ctx.info(
+ "Listing reports: page=%s, page_size=%s, search=%s"
+ % (
+ request.page,
+ request.page_size,
+ request.search,
+ )
+ )
+ await ctx.debug(
+ "Report listing parameters: filters=%s, order_column=%s, "
+ "order_direction=%s, select_columns=%s"
+ % (
+ request.filters,
+ request.order_column,
+ request.order_direction,
+ request.select_columns,
+ )
+ )
+
+ try:
+ from superset import is_feature_enabled
+ from superset.daos.report import ReportScheduleDAO
+
+ if not is_feature_enabled("ALERT_REPORTS"):
+ return ReportError.create(
+ error="The Alerts & Reports feature is disabled on this instance.",
+ error_type="FeatureDisabled",
+ )
+
+ def _serialize_report(
+ obj: "ReportSchedule | None", cols: list[str] | None
+ ) -> ReportInfo | None:
+ return serialize_report_object(obj)
+
+ list_tool = ModelListCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ item_serializer=_serialize_report,
+ filter_type=ReportFilter,
+ default_columns=REPORT_DEFAULT_COLUMNS,
+ search_columns=REPORT_SEARCH_COLUMNS,
+ list_field_name="reports",
+ output_list_schema=ReportList,
+ all_columns=list(ReportInfo.model_fields.keys()),
+ sortable_columns=REPORT_SORTABLE_COLUMNS,
+ owner_filter_column="owners.id",
+ logger=logger,
+ )
+
+ with event_logger.log_context(action="mcp.list_reports.query"):
+ result = list_tool.run_tool(
+ filters=request.filters,
+ search=request.search,
+ select_columns=request.select_columns,
+ order_column=request.order_column,
+ order_direction=request.order_direction,
+ page=max(request.page - 1, 0),
+ page_size=request.page_size,
+ created_by_me=request.created_by_me,
+ owned_by_me=request.owned_by_me,
+ )
+
+ await ctx.info(
+ "Reports listed successfully: count=%s, total_count=%s, total_pages=%s"
+ % (
+ len(result.reports) if hasattr(result, "reports") else 0,
+ getattr(result, "total_count", None),
+ getattr(result, "total_pages", None),
+ )
+ )
+
+ columns_to_filter = result.columns_requested
+ with event_logger.log_context(action="mcp.list_reports.serialization"):
+ return result.model_dump(
+ mode="json",
+ context={"select_columns": columns_to_filter},
+ )
+
+ except Exception as e:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Avoid catching bare `Exception`. Catch specific exception types instead (e.g., `ValueError`, `RuntimeError`, `DatabaseError`). This makes error handling more precise and prevents masking unexpected errors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ValueError, RuntimeError, DatabaseError) as e:
````
</div>
</details>
</div>
<small><i>Code Review Run #c52ebd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #40348:
URL: https://github.com/apache/superset/pull/40348
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4588490146
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40348:
URL: https://github.com/apache/superset/pull/40348#issuecomment-4566657033
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3292294965
##########
superset/mcp_service/report/schemas.py:
##########
@@ -0,0 +1,290 @@
+# 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
+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(
+ ...,
+ description="Column to filter on. Use get_schema(model_type='report') for "
+ "available filter columns.",
+ )
+ opr: ColumnOperatorEnum = Field(
+ ...,
+ description="Operator to use. Use get_schema(model_type='report') for "
+ "available operators.",
+ )
+ value: str | int | float | bool | List[str | int | float | bool] = Field(
+ ..., description="Value to filter by (type depends on col and opr)"
+ )
+
+
+class ReportInfo(BaseModel):
+ id: int | None = Field(None, description="Report/Alert ID")
+ name: str | None = Field(None, description="Report/Alert name")
+ description: str | None = Field(None, description="Report/Alert description")
+ type: str | None = Field(None, description="Schedule type: 'Alert' or 'Report'")
+ active: bool | None = Field(None, description="Whether the schedule is active")
+ crontab: str | None = Field(None, description="Cron expression for scheduling")
+ dashboard_id: int | None = Field(
+ None, description="Associated dashboard ID, if any"
+ )
+ chart_id: int | None = Field(None, description="Associated chart ID, if any")
+ owners: List[Any] | None = Field(
+ None, description="List of owners (filtered by privacy controls)"
+ )
+ changed_on: str | datetime | None = Field(
+ None, description="Last modification timestamp"
+ )
+ changed_on_humanized: str | None = Field(
+ None, description="Humanized modification time"
+ )
+ created_on: str | datetime | None = Field(None, description="Creation timestamp")
+ created_on_humanized: str | None = Field(
+ None, description="Humanized creation time"
+ )
+ model_config = ConfigDict(
+ from_attributes=True,
+ ser_json_timedelta="iso8601",
+ populate_by_name=True,
+ )
+
+ @model_serializer(mode="wrap")
+ def _filter_fields_by_context(self, serializer: Any, info: Any) -> Dict[str, Any]:
+ """Filter fields based on serialization context.
+
+ If context contains 'select_columns', only include those fields.
+ Otherwise, include all fields (default behavior).
+ """
+ data = filter_user_directory_fields(serializer(self))
+
+ if info.context and isinstance(info.context, dict):
+ select_columns = info.context.get("select_columns")
+ if select_columns:
+ requested_fields = set(select_columns)
+ return {k: v for k, v in data.items() if k in requested_fields}
+
+ return data
+
+
+class ReportList(BaseModel):
+ reports: List[ReportInfo]
+ count: int
+ total_count: int
+ page: int
+ page_size: int
+ total_pages: int
+ has_previous: bool
+ has_next: bool
+ columns_requested: List[str] = Field(
+ default_factory=list,
+ description="Requested columns for the response",
+ )
+ columns_loaded: List[str] = Field(
+ default_factory=list,
+ description="Columns that were actually loaded for each report",
+ )
+ columns_available: List[str] = Field(
+ default_factory=list,
+ description="All columns available for selection via select_columns parameter",
+ )
+ sortable_columns: List[str] = Field(
+ default_factory=list,
+ description="Columns that can be used with order_column parameter",
+ )
+ filters_applied: List[ColumnOperator] = Field(
+ default_factory=list,
+ description="List of advanced filter dicts applied to the query.",
+ )
+ pagination: PaginationInfo | None = None
+ timestamp: datetime | None = None
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+
+class ListReportsRequest(OwnedByMeMixin, CreatedByMeMixin, MetadataCacheControl):
+ """Request schema for list_reports."""
+
+ filters: Annotated[
+ List[ReportFilter],
+ Field(
+ default_factory=list,
+ description="List of filter objects (column, operator, value). Each "
+ "filter is an object with 'col', 'opr', and 'value' "
+ "properties. Cannot be used together with 'search'.",
+ ),
+ ]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=list,
+ description="List of columns to select. Defaults to common columns if not "
+ "specified.",
+ ),
+ ]
+ search: Annotated[
+ str | None,
+ Field(
+ default=None,
+ description="Text search string to match against report fields. Cannot "
+ "be used together with 'filters'.",
+ ),
+ ]
+ order_column: Annotated[
+ str | None, Field(default=None, description="Column to order results by")
+ ]
+ order_direction: Annotated[
+ Literal["asc", "desc"],
+ Field(
+ default="desc", description="Direction to order results ('asc' or 'desc')"
+ ),
+ ]
+ page: Annotated[
+ PositiveInt,
+ Field(default=1, description="Page number for pagination (1-based)"),
+ ]
+ page_size: Annotated[
+ int,
+ Field(
+ default=DEFAULT_PAGE_SIZE,
+ gt=0,
+ le=MAX_PAGE_SIZE,
+ description=f"Number of items per page (max {MAX_PAGE_SIZE})",
+ ),
+ ]
+
+ @field_validator("filters", mode="before")
+ @classmethod
+ def parse_filters(cls, v: Any) -> List[ReportFilter]:
+ """Accept both JSON string and list of objects."""
+ return parse_json_or_model_list(v, ReportFilter, "filters")
+
+ @field_validator("select_columns", mode="before")
+ @classmethod
+ def parse_columns(cls, v: Any) -> List[str]:
+ """Accept JSON array, list, or comma-separated string."""
+ return parse_json_or_list(v, "select_columns")
+
+ @model_validator(mode="after")
+ def validate_search_and_filters(self) -> "ListReportsRequest":
+ """Prevent using both search and filters simultaneously."""
+ if self.search and self.filters:
+ raise ValueError(
+ "Cannot use both 'search' and 'filters' parameters simultaneously. "
+ "Use either 'search' for text-based searching across multiple fields, "
+ "or 'filters' for precise column-based filtering, but not both."
+ )
+ return self
+
+
+class ReportError(BaseModel):
+ error: str = Field(..., description="Error message")
+ error_type: str = Field(..., description="Type of error")
+ timestamp: str | datetime | None = Field(None, description="Error timestamp")
+ model_config = ConfigDict(ser_json_timedelta="iso8601")
+
+ @classmethod
+ def create(cls, error: str, error_type: str) -> "ReportError":
+ """Create a standardized ReportError with timestamp."""
+ from datetime import datetime, timezone
+
+ return cls(
+ error=error, error_type=error_type, timestamp=datetime.now(timezone.utc)
+ )
+
+
+class GetReportInfoRequest(MetadataCacheControl):
+ """Request schema for get_report_info — identifier is a numeric ID only."""
+
+ identifier: Annotated[
+ int,
+ Field(description="Report/Alert numeric ID"),
+ ]
+
+
+def _humanize_timestamp(dt: datetime | None) -> str | None:
+ """Convert a datetime to a humanized string like '2 hours ago'."""
+ if dt is None:
+ return None
+ now = datetime.now(dt.tzinfo) if dt.tzinfo else datetime.now()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>datetime.now() called without timezone</b></div>
<div id="fix">
Add timezone argument to `datetime.now()` call on line 268. Use `datetime.now(timezone.utc)` or `datetime.now(dt.tzinfo)` to ensure timezone-aware datetime.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if dt is None:
return None
now = datetime.now(dt.tzinfo) if dt.tzinfo else datetime.now(timezone.utc)
````
</div>
</details>
</div>
<small><i>Code Review Run #07067a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/report/tool/get_report_info.py:
##########
@@ -0,0 +1,119 @@
+# 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.
+
+"""
+Get report info FastMCP tool.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+from superset.mcp_service.report.schemas import (
+ GetReportInfoRequest,
+ ReportError,
+ ReportInfo,
+ serialize_report_object,
+)
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="ReportSchedule",
+ annotations=ToolAnnotations(
+ title="Get report info",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_report_info(
+ request: GetReportInfoRequest, ctx: Context
+) -> ReportInfo | ReportError:
+ """Get alert or report schedule metadata by numeric ID.
+
+ Returns schedule configuration including type (Alert/Report), active
+ status, cron expression, and associated dashboard or chart.
+
+ IMPORTANT FOR LLM CLIENTS:
+ - Use numeric ID (e.g., 123)
+ - To find a report ID, use the list_reports tool first
+
+ Example usage:
+ ```json
+ {
+ "identifier": 1
+ }
+ ```
+ """
+ await ctx.info(
+ "Retrieving report information: identifier=%s" % (request.identifier,)
+ )
+
+ try:
+ from superset.daos.report import ReportScheduleDAO
+
+ with event_logger.log_context(action="mcp.get_report_info.lookup"):
+ get_tool = ModelGetInfoCore(
+ dao_class=ReportScheduleDAO,
+ output_schema=ReportInfo,
+ error_schema=ReportError,
+ serializer=serialize_report_object,
+ supports_slug=False,
+ logger=logger,
+ )
+
+ result = get_tool.run_tool(request.identifier)
+
+ if isinstance(result, ReportInfo):
+ await ctx.info(
+ "Report information retrieved successfully: "
+ "report_id=%s, name=%s, type=%s"
+ % (
+ result.id,
+ result.name,
+ result.type,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Report retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as e:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch too broad</b></div>
<div id="fix">
Replace bare `Exception` catch with specific exception types (e.g., `ValueError`, `KeyError`, or custom exceptions) to follow BLE001 rule and improve error handling.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ValueError, KeyError, AttributeError) as e:
````
</div>
</details>
</div>
<small><i>Code Review Run #07067a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3311951008
##########
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:
Fixed in commit 251090701b. The two-AND-filter approach was wrong — when both `created_by_me` and `owned_by_me` are set, the base class uses a single `created_by_fk_or_owner` OR filter. `_prepend_self_lookup_filters` in `ReportListCore` now does the same. To support it, `ReportScheduleDAO.apply_column_operators` was overridden (mirroring `DashboardDAO`) to handle `owners.id` via the `report_schedule_user` M2M join and `created_by_fk_or_owner` via `OR(created_by_fk == value, id IN owner_subq)`.
##########
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:
Fixed in commit 251090701b. The `_call_dao_list` override was removed entirely — it existed only to use `filters=` instead of `column_operators=`, which caused a `TypeError` at runtime since `BaseDAO.list` doesn't accept a `filters` kwarg. The base class `_call_dao_list` already passes `column_operators=` correctly, so no override is needed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add list_reports and get_report_info tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40348:
URL: https://github.com/apache/superset/pull/40348#discussion_r3311949581
##########
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:
Fixed in commit 251090701b.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org