aminghadersohi commented on code in PR #40340:
URL: https://github.com/apache/superset/pull/40340#discussion_r3349893882
##########
superset/mcp_service/app.py:
##########
@@ -874,7 +880,7 @@ def init_fastmcp_server(
# Remove disabled tools BEFORE generating instructions so that the
# instructions never advertise tools that clients cannot actually call.
- disabled_tools: set[str] = flask_app.config.get("MCP_DISABLED_TOOLS",
set())
+ disabled_tools: set[str] = flask_app.config["MCP_DISABLED_TOOLS"]
Review Comment:
Confirmed: line 891 of `app.py` in the current HEAD already reads
`flask_app.config.get("MCP_DISABLED_TOOLS", set())`. The `[]` form you're
referencing was from commit `7e4b17f3f4`, which was subsequently corrected. No
further change needed.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -324,6 +322,48 @@ class GetDatasetInfoRequest(MetadataCacheControl):
]
+class CreateDatasetRequest(BaseModel):
+ """Request schema for create_dataset to register a physical table as a
dataset."""
+
+ database_id: Annotated[
+ int,
+ Field(
+ description="ID of the database connection to register the table
against"
+ ),
+ ]
+ schema: Annotated[
+ str | None,
+ Field(
+ default=None,
+ description="Schema (namespace) where the table lives, e.g.
'public'. "
+ "Omit or pass None for databases without schema namespaces (e.g.
SQLite).",
+ ),
+ ]
+ catalog: Annotated[
+ str | None,
+ Field(
+ default=None,
+ description="Catalog where the table lives. Omit for databases
without "
+ "catalog support.",
+ ),
+ ]
+ table_name: Annotated[
+ str,
+ Field(
+ min_length=1,
+ description="Name of the physical table to register as a dataset",
+ ),
+ ]
+ owners: Annotated[
+ List[int] | None,
+ Field(
+ default=None,
+ description="Optional list of owner user IDs. "
+ "Defaults to the calling user.",
+ ),
+ ]
+
Review Comment:
The `CreateDatasetRequest` schema is exercised by every test in
`test_create_dataset.py` — each test constructs the full request payload that
FastMCP deserializes into a `CreateDatasetRequest` instance before the tool
function is called. Required-field validation is covered by
`test_create_dataset_missing_required_fields` (raises `ToolError` when
`database_id` or `table_name` is absent). Dedicated Pydantic schema unit tests
(validating field types, min_length constraints, etc.) would duplicate that
coverage without adding signal; the tool-level tests are the right layer here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
