bito-code-review[bot] commented on code in PR #40357:
URL: https://github.com/apache/superset/pull/40357#discussion_r3336161848
##########
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:
<!-- Bito Reply -->
The suggestion in the PR comment is valid and appropriate to apply. It
addresses a missing test case for the `UpdatePluginRequest` validation logic,
specifically ensuring that the `key` field is tested for blank input rejection,
just like the `name` field. The test implementation in the suggestion is
correct and aligns with the existing validation logic in the schema.
**tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py**
```
def test_update_plugin_request_blank_key_fails() -> None:
with pytest.raises(ValidationError, match="must not be blank"):
UpdatePluginRequest(id=1, key=" ")
```
##########
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:
<!-- Bito Reply -->
The suggestion is valid and should be applied. The `__all__` list in the
`__init__.py` file exports `create_plugin` and `update_plugin`, but the
corresponding import statements for these modules are missing. This will cause
an `ImportError` at runtime when someone tries to import these functions. The
suggested fix adds the necessary import statements to ensure that the exported
symbols are properly defined and available for use.
**superset/mcp_service/plugin/tool/__init__.py**
```
from .create_plugin import create_plugin
from .update_plugin import update_plugin
__all__ = [
"create_plugin",
"update_plugin",
]
```
--
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]
