bito-code-review[bot] commented on code in PR #40357:
URL: https://github.com/apache/superset/pull/40357#discussion_r3331098949
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing imports for exported symbols</b></div>
<div id="fix">
The `__all__` list exports `create_plugin` and `update_plugin`, but there
are no corresponding `import` statements for these modules. Any caller
executing `from superset.mcp_service.plugin.tool import create_plugin` or
`import update_plugin` will hit `ImportError` at runtime. This is a
module-scope runtime failure that crashes the process on import.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/mcp_service/plugin/tool/__init__.py
+++ superset/mcp_service/plugin/tool/__init__.py
@@ -18,5 +18,7 @@
from .get_plugin_info import get_plugin_info
from .list_plugins import list_plugins
+from .create_plugin import create_plugin
+from .update_plugin import update_plugin
__all__ = [
"create_plugin",
```
</div>
</details>
</div>
<small><i>Code Review Run #5aae5a</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/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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing schema test coverage</b></div>
<div id="fix">
UpdatePluginRequest has a validator covering both name AND key fields
(schemas.py:311), but only name is tested for blank rejection. Add
test_update_plugin_request_blank_key_fails to prevent regression on key blank
validation.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -281,0 +282,13 @@
+
+def test_update_plugin_request_blank_key_fails() -> None:
+ with pytest.raises(ValidationError, match="must not be blank"):
+ UpdatePluginRequest(id=1, key=" ")
+
```
</div>
</details>
</div>
<small><i>Code Review Run #5aae5a</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: [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]
