Copilot commented on code in PR #11621:
URL: https://github.com/apache/gravitino/pull/11621#discussion_r3421853142
##########
mcp-server/mcp_server/client/topic_operation.py:
##########
@@ -53,3 +53,31 @@ async def load_topic(
str: JSON-formatted string containing full topic metadata
"""
pass
+
+ @abstractmethod
+ # pylint: disable=too-many-positional-arguments
+ async def create_topic(
+ self,
+ catalog_name: str,
+ schema_name: str,
+ name: str,
+ comment: str,
+ properties: dict,
+ ) -> str:
+ pass
+
+ @abstractmethod
+ async def alter_topic(
+ self,
+ catalog_name: str,
+ schema_name: str,
+ topic_name: str,
+ updates: list,
+ ) -> str:
+ pass
+
+ @abstractmethod
+ async def delete_topic(
+ self, catalog_name: str, schema_name: str, topic_name: str
+ ) -> str:
+ pass
Review Comment:
These new operation interfaces declare `-> str` return types, but the plain
REST implementations return Python objects (e.g., dict/list/bool via
`extract_content_from_response`). This mismatch will confuse tool consumers and
can cause runtime/serialization inconsistencies. Recommendation (mandatory):
standardize return types across the client interface + implementations. Either
(a) update these interfaces (and corresponding tool annotations) to return
structured types like `dict | list | bool` (or `Any`) and adjust docstrings
accordingly, or (b) ensure implementations consistently return JSON strings
(e.g., `json.dumps(...)`) so `-> str` is accurate.
##########
mcp-server/mcp_server/tools/catalog.py:
##########
@@ -108,3 +108,79 @@ async def get_list_of_catalogs(ctx: Context) -> str:
"""
client = ctx.request_context.lifespan_context.rest_client()
return await client.as_catalog_operation().get_list_of_catalogs()
+
+ @mcp.tool(tags={"catalog"})
+ # pylint: disable=too-many-positional-arguments
+ async def create_catalog(
+ ctx: Context,
+ name: str,
+ catalog_type: str,
+ provider: str,
+ comment: str,
+ properties: dict,
+ ) -> str:
+ """
+ Create a new catalog within the metalake.
+
+ Authorization is enforced by Gravitino: a principal without the
+ required grant receives an authorization denial.
+
+ Args:
+ ctx (Context): The request context object.
+ name (str): Name of the catalog to create.
+ catalog_type (str): Catalog type, one of "relational", "fileset",
+ "messaging", "model".
+ provider (str): Provider implementation, e.g. "hive",
+ "lakehouse-iceberg", "jdbc-postgresql", "kafka". May be empty
+ for model/fileset catalogs that have a single provider.
+ comment (str): Human-readable description.
+ properties (dict): Catalog configuration properties.
+
+ Returns:
+ str: JSON-formatted string containing the created catalog.
Review Comment:
The tool docstring claims the return value is a \"JSON-formatted string\",
but the underlying REST client implementations return deserialized Python
objects (dict/list/bool) via `extract_content_from_response`. Update the
docstrings (and preferably the type annotations) to reflect the actual return
type so MCP tool consumers know what shape they receive.
##########
mcp-server/tests/unit/tools/test_tag.py:
##########
@@ -99,14 +99,17 @@ async def _test_disassociate_tag_from_metadata(mcp_server):
asyncio.run(_test_disassociate_tag_from_metadata(self.mcp))
- def test_destructive_tag_tools_disabled_by_default(self):
- async def _test_destructive_tag_tools_disabled_by_default(mcp_server):
- tool_names = {tool.name for tool in await mcp_server.list_tools()}
+ def test_write_tag_tools_are_exposed(self):
+ """Tag write tools are registered/exposed (not hidden).
+
+ Authorization enforcement is delegated to Gravitino and covered by the
+ live integration test, not asserted here; this only checks exposure.
+ """
- self.assertIn("get_tag_by_name", tool_names)
- self.assertIn("list_of_tags", tool_names)
- self.assertNotIn("create_tag", tool_names)
- self.assertNotIn("alter_tag", tool_names)
- self.assertNotIn("delete_tag", tool_names)
+ async def _test(mcp_server):
+ tool_names = {tool.name for tool in await mcp_server.list_tools()}
+ self.assertIn("create_tag", tool_names)
+ self.assertIn("alter_tag", tool_names)
+ self.assertIn("delete_tag", tool_names)
- asyncio.run(_test_destructive_tag_tools_disabled_by_default(self.mcp))
+ asyncio.run(_test(self.mcp))
Review Comment:
This test only verifies tool exposure, but this PR changes behavior by
making tag write tools callable by default (previously disabled). To cover the
new behavior at the unit level (not authorization), add tests that call
`create_tag`, `alter_tag`, and `delete_tag` via `client.call_tool(...)` and
assert the mock operation outputs—matching the pattern used for
table/schema/topic/fileset/model write tool tests.
##########
mcp-server/mcp_server/tools/table.py:
##########
@@ -257,3 +257,96 @@ async def get_table_metadata_details(
return await client.as_table_operation().load_table(
catalog_name, schema_name, table_name
)
+
+ @mcp.tool(tags={"table"})
+ # pylint: disable=too-many-positional-arguments
+ async def create_table(
+ ctx: Context,
+ catalog_name: str,
+ schema_name: str,
+ name: str,
+ comment: str,
+ columns: list,
+ properties: dict,
+ ) -> str:
Review Comment:
Several new tools require disabling `too-many-positional-arguments`, which
increases the chance of parameter-order mistakes and makes future evolution
harder. Optional improvement: switch to keyword-only parameters after `ctx`
(e.g., using `*`), or define a single request object type (dataclass / pydantic
model / TypedDict) for the payload (e.g., `CreateTableRequest`) to reduce
positional arguments while keeping tool schemas explicit.
--
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]
