aminghadersohi commented on code in PR #40357: URL: https://github.com/apache/superset/pull/40357#discussion_r3336159636
########## superset/mcp_service/plugin/tool/update_plugin.py: ########## @@ -0,0 +1,116 @@ +# 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 ( + UpdatePluginRequest, + UpdatePluginResponse, +) + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="DynamicPlugin", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update a dynamic plugin", + readOnlyHint=False, + destructiveHint=True, + ), +) +async def update_plugin( + request: UpdatePluginRequest, ctx: Context +) -> UpdatePluginResponse: + """Update an existing dynamic plugin's name, key, or bundle URL. + + Requires admin write access to DynamicPlugin and the DYNAMIC_PLUGINS + feature flag to be enabled. At least one of ``name``, ``key``, or + ``bundle_url`` must be provided; only the supplied fields are changed. + + Use ``create_plugin`` to look up the plugin ID if you only know the key. + """ Review Comment: Fixed: the docstring was updated in a previous commit. It now says "Use the plugin ID returned by `create_plugin`, or look up the ID in the Custom Plugins UI (Settings → Custom Plugins)." — no implication that `create_plugin` provides lookup functionality. ########## superset/mcp_service/plugin/tool/__init__.py: ########## @@ -19,6 +19,8 @@ from .list_plugins import list_plugins __all__ = [ - "list_plugins", + "create_plugin", "get_plugin_info", + "list_plugins", + "update_plugin", Review Comment: Fixed: added `from .create_plugin import create_plugin` and `from .update_plugin import update_plugin` to `superset/mcp_service/plugin/tool/__init__.py`. Also consolidated the `app.py` imports to use the package instead of the individual modules. ########## tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py: ########## @@ -170,3 +182,406 @@ def test_list_plugins_request_rejects_search_and_filters(): search="test", filters=[{"col": "name", "opr": "eq", "value": "x"}], ) + + +# --------------------------------------------------------------------------- +# CreatePluginRequest / UpdatePluginRequest schema tests +# --------------------------------------------------------------------------- + + +def test_create_plugin_request_valid() -> None: + req = CreatePluginRequest( + name="My Plugin", + key="@my-org/my-plugin", + bundle_url="https://cdn.example.com/bundle.js";, + ) + assert req.name == "My Plugin" + assert req.key == "@my-org/my-plugin" + assert req.bundle_url == "https://cdn.example.com/bundle.js"; + + +def test_create_plugin_request_blank_name_fails() -> None: + with pytest.raises(ValidationError, match="must not be blank"): + CreatePluginRequest( + name=" ", + key="@my-org/my-plugin", + bundle_url="https://cdn.example.com/bundle.js";, + ) + + +def test_create_plugin_request_name_too_long_fails() -> None: + with pytest.raises(ValidationError): + CreatePluginRequest( + name="a" * 51, + key="@my-org/my-plugin", + bundle_url="https://cdn.example.com/bundle.js";, + ) + + +def test_create_plugin_request_key_too_long_fails() -> None: + with pytest.raises(ValidationError): + CreatePluginRequest( + name="My Plugin", + key="k" * 51, + bundle_url="https://cdn.example.com/bundle.js";, + ) + + +def test_create_plugin_request_bundle_url_too_long_fails() -> None: + with pytest.raises(ValidationError): + CreatePluginRequest( + name="My Plugin", + key="@my-org/my-plugin", + bundle_url="https://cdn.example.com/"; + "a" * 980 + ".js", + ) + + +def test_create_plugin_request_invalid_url_scheme_fails() -> None: + with pytest.raises(ValidationError, match="http or https"): + CreatePluginRequest( + name="My Plugin", + key="@my-org/my-plugin", + bundle_url="ftp://cdn.example.com/bundle.js";, + ) + + +def test_create_plugin_request_javascript_url_fails() -> None: + with pytest.raises(ValidationError, match="http or https"): + CreatePluginRequest( + name="My Plugin", + key="@my-org/my-plugin", + bundle_url="javascript:alert(1)", + ) + + +def test_update_plugin_request_valid() -> None: + req = UpdatePluginRequest(id=1, name="Updated Name") + assert req.id == 1 + assert req.name == "Updated Name" + assert req.key is None + assert req.bundle_url is None + + +def test_update_plugin_request_no_fields_fails() -> None: + with pytest.raises(ValidationError, match="At least one of"): + UpdatePluginRequest(id=1) + + +def test_update_plugin_request_blank_name_fails() -> None: + with pytest.raises(ValidationError, match="must not be blank"): + UpdatePluginRequest(id=1, name=" ") + Review Comment: Fixed: added `test_update_plugin_request_blank_key_fails` to `tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py` to cover blank key rejection in `UpdatePluginRequest`. -- 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]
