aminghadersohi commented on code in PR #40357: URL: https://github.com/apache/superset/pull/40357#discussion_r3336156739
########## superset/mcp_service/plugin/tool/create_plugin.py: ########## @@ -0,0 +1,108 @@ +# 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 fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.plugin.schemas import ( + CreatePluginRequest, + CreatePluginResponse, +) + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="DynamicPlugin", + method_permission_name="write", + annotations=ToolAnnotations( + title="Register a dynamic plugin", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_plugin( + request: CreatePluginRequest, ctx: Context +) -> CreatePluginResponse: + """Register a new dynamic (custom) plugin in Superset. + + Requires the DYNAMIC_PLUGINS feature flag to be enabled and admin write + access to DynamicPlugin. The ``key`` must match the package name from the + plugin's package.json and be unique across all registered plugins. Review Comment: Fixed: the docstring now reads "Requires the DYNAMIC_PLUGINS feature flag to be enabled and DynamicPlugin write permission" — no mention of a specific role. Any role granted the DynamicPlugin/write permission can use the tool. ########## superset/mcp_service/app.py: ########## @@ -130,6 +130,9 @@ def get_default_instructions( - generate_dashboard: Create a dashboard from chart IDs (requires write access) - add_chart_to_existing_dashboard: Add a chart to an existing dashboard (requires write access) +Plugin Management: +- create_plugin: Register a new dynamic plugin by name, key, and bundle URL (requires admin write access and DYNAMIC_PLUGINS feature flag) Review Comment: Fixed: the DEFAULT_INSTRUCTIONS entry now reads "requires DynamicPlugin write permission and DYNAMIC_PLUGINS feature flag" — no role assumption. ########## superset/mcp_service/plugin/schemas.py: ########## @@ -0,0 +1,127 @@ +# 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 plugin-related MCP tool requests and responses.""" + +from pydantic import BaseModel, Field, field_validator, model_validator + + +class CreatePluginRequest(BaseModel): + """Request schema for create_plugin.""" + + name: str = Field( + ..., + min_length=1, + description="Human-friendly name for the plugin.", + ) + key: str = Field( + ..., + min_length=1, + description=( + "Unique plugin key. Should match the package name from the plugin's " + "package.json (e.g. '@my-org/my-chart-plugin')." + ), + ) + bundle_url: str = Field( + ..., + min_length=1, + description=( + "Full URL pointing to the built plugin bundle " + "(e.g. a CDN-hosted JavaScript file)." + ), + ) + + @field_validator("name", "key", "bundle_url") + @classmethod + def must_not_be_blank(cls, v: str) -> str: + if not v.strip(): + raise ValueError("Field must not be blank") + return v.strip() + + +class CreatePluginResponse(BaseModel): + """Response schema for create_plugin.""" + + id: int | None = Field( + None, + description="ID of the newly created plugin. None if creation failed.", + ) + name: str | None = Field(None, description="Plugin name.") + key: str | None = Field(None, description="Plugin key.") + bundle_url: str | None = Field(None, description="Plugin bundle URL.") + error: str | None = Field( + None, + description="Error message if creation failed, otherwise null.", + ) + + +class UpdatePluginRequest(BaseModel): + """Request schema for update_plugin.""" + + id: int = Field(..., description="ID of the plugin to update.") + name: str | None = Field( + None, + min_length=1, + description="New human-friendly name for the plugin.", + ) + key: str | None = Field( + None, + min_length=1, + description=( + "New unique plugin key. Should match the package name from the " + "plugin's package.json (e.g. '@my-org/my-chart-plugin')." + ), + ) + bundle_url: str | None = Field( + None, + min_length=1, + description=( + "New full URL pointing to the built plugin bundle " + "(e.g. a CDN-hosted JavaScript file)." + ), + ) + + @field_validator("name", "key", "bundle_url") + @classmethod + def must_not_be_blank(cls, v: str | None) -> str | None: + if v is not None and not v.strip(): + raise ValueError("Field must not be blank") + return v.strip() if v is not None else v Review Comment: The current validation order is intentional. When a caller supplies `name=""` (blank string), that IS an attempt to provide the `name` field — so the most informative error is "Field must not be blank", not "At least one of...must be provided". The model_validator fires after field validators, which is the correct Pydantic v2 layering: reject malformed values first, then verify that at least one meaningful value was given. No change 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: [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]
