bito-code-review[bot] commented on code in PR #40340:
URL: https://github.com/apache/superset/pull/40340#discussion_r3349895540
##########
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:
<!-- Bito Reply -->
The suggestion to use `.get("MCP_DISABLED_TOOLS", set())` is appropriate for
this code. Using `.get()` with a default value ensures that the configuration
key is handled safely if it is missing, which prevents potential errors during
test execution when the configuration is mocked.
**superset/mcp_service/app.py**
```
- disabled_tools: set[str] = flask_app.config["MCP_DISABLED_TOOLS"]
+ disabled_tools: set[str] = flask_app.config.get("MCP_DISABLED_TOOLS",
set())
```
##########
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:
<!-- Bito Reply -->
The suggestion to add dedicated unit tests for the `CreateDatasetRequest`
schema is not necessary in this case. As noted, the schema is fully exercised
by the existing tool-level tests in `test_create_dataset.py`, which cover both
successful request construction and required-field validation. Adding redundant
Pydantic schema unit tests would not provide additional signal.
--
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]
