Copilot commented on code in PR #11621:
URL: https://github.com/apache/gravitino/pull/11621#discussion_r3401204010
##########
mcp-server/mcp_server/client/plain/plain_rest_client_topic_operation.py:
##########
@@ -46,3 +49,46 @@ async def load_topic(
f"/topics/{encode_path_segment(topic_name)}"
)
return response.json().get("topic", {})
+
Review Comment:
`load_topic` returns `response.json().get(...)` directly, which bypasses
`_handle_gravitino_exception` (authorization/REST errors can be silently
ignored) and also returns a dict while the method is declared `-> str`. Use
`extract_content_from_response` to both surface Gravitino errors and return a
JSON-formatted string consistently.
##########
mcp-server/mcp_server/client/plain/plain_rest_client_fileset_operation.py:
##########
@@ -62,3 +65,54 @@ async def list_files_in_fileset(
params={"sub_path": sub_path, "location_name": location_name},
)
return response.json().get("files", [])
Review Comment:
`list_files_in_fileset` returns `response.json().get(...)` directly, which
bypasses `_handle_gravitino_exception` (errors/authz denials can be silently
turned into an empty list) and returns a Python list while the method is
declared `-> str`. Use `extract_content_from_response` for consistent
JSON-string output and proper error handling.
##########
mcp-server/mcp_server/tools/fileset.py:
##########
@@ -143,3 +143,97 @@ async def list_files_in_fileset(
return await client.as_fileset_operation().list_files_in_fileset(
catalog_name, schema_name, fileset_name, location_name, sub_path
)
+
+ # pylint:disable=too-many-positional-arguments
Review Comment:
The pylint disable directive is malformed (`# pylint:disable=...`); pylint
expects `# pylint: disable=...` with a space after the colon. As written, the
rule suppression may not take effect and CI linting can fail.
##########
mcp-server/mcp_server/tools/model.py:
##########
@@ -230,3 +230,120 @@ async def load_model_version_by_alias(
return await client.as_model_operation().load_model_version_by_alias(
catalog_name, schema_name, model_name, alias
)
+
+ @mcp.tool(tags={"model"})
+ async def register_model(
+ ctx: Context,
+ catalog_name: str,
+ schema_name: str,
+ name: str,
+ comment: str,
+ properties: dict,
+ ) -> str:
+ """
+ Register a new model within a schema.
+
+ Authorization is enforced by Gravitino: a principal without the
+ required grant receives an authorization denial.
+
+ Args:
+ ctx (Context): The request context object.
+ catalog_name (str): Name of the catalog.
+ schema_name (str): Name of the schema.
+ name (str): Name of the model to register.
+ comment (str): Human-readable description.
+ properties (dict): Model configuration properties.
+
+ Returns:
+ str: JSON-formatted string containing the registered model.
+ """
+ client = ctx.request_context.lifespan_context.rest_client()
+ return await client.as_model_operation().register_model(
+ catalog_name, schema_name, name, comment, properties
+ )
+
+ @mcp.tool(tags={"model"})
+ async def delete_model(
+ ctx: Context, catalog_name: str, schema_name: str, model_name: str
+ ) -> str:
+ """
+ Delete a model by its name.
+
+ Args:
+ ctx (Context): The request context object.
+ catalog_name (str): Name of the catalog.
+ schema_name (str): Name of the schema.
+ model_name (str): Name of the model to delete.
+
+ Returns:
+ str: JSON-formatted string indicating whether the model was
deleted.
+ """
+ client = ctx.request_context.lifespan_context.rest_client()
+ return await client.as_model_operation().delete_model(
+ catalog_name, schema_name, model_name
+ )
+
+ # pylint:disable=too-many-positional-arguments
Review Comment:
The pylint disable directive is malformed (`# pylint:disable=...`); pylint
expects `# pylint: disable=...` with a space after the colon. As written, the
suppression may not apply and linting can fail.
##########
mcp-server/mcp_server/client/plain/plain_rest_client_model_operation.py:
##########
@@ -81,3 +84,66 @@ async def load_model_version_by_alias(
f"/aliases/{encode_path_segment(alias)}"
)
return response.json().get("modelVersion", {})
Review Comment:
`load_model_version_by_alias` returns `response.json().get(...)` directly,
bypassing `_handle_gravitino_exception` and returning a dict while the method
is declared `-> str`. Use `extract_content_from_response` so Gravitino error
payloads (including authz denials) are handled consistently and the return type
matches the interface.
##########
mcp-server/mcp_server/client/plain/plain_rest_client_model_operation.py:
##########
@@ -16,7 +16,10 @@
# under the License.
from mcp_server.client import ModelOperation
-from mcp_server.client.plain.utils import encode_path_segment
+from mcp_server.client.plain.utils import (
+ encode_path_segment,
+ extract_content_from_response,
+)
Review Comment:
`list_of_models` returns `response.json().get(...)` directly, which bypasses
`_handle_gravitino_exception` and can silently convert authz/REST errors into
an empty list. It also returns a Python list while the method is declared `->
str`. Use `extract_content_from_response` to keep error handling and output
format consistent with the new write operations.
##########
mcp-server/mcp_server/tools/tag.py:
##########
@@ -364,8 +364,3 @@ async def list_metadata_by_tag(ctx: Context, tag_name: str)
-> str:
"""
client = ctx.request_context.lifespan_context.rest_client()
return await client.as_tag_operation().list_metadata_by_tag(tag_name)
Review Comment:
Since tag write tools are now enabled (the `mcp.disable(...)` block is
removed), the earlier inline comments in this module that say alter/delete are
"disabled by default" become inaccurate/misleading. Please update or remove
those comments so the documentation matches the actual behavior (tools are
exposed and authorization is enforced by Gravitino).
##########
mcp-server/mcp_server/client/plain/plain_rest_client_fileset_operation.py:
##########
@@ -16,7 +16,10 @@
# under the License.
from mcp_server.client.fileset_operation import FilesetOperation
-from mcp_server.client.plain.utils import encode_path_segment
+from mcp_server.client.plain.utils import (
+ encode_path_segment,
+ extract_content_from_response,
+)
Review Comment:
`list_of_filesets` returns `response.json().get(...)` directly, which
bypasses `_handle_gravitino_exception` and can silently turn authz/REST errors
into an empty list. It also returns a Python list while the method is declared
`-> str`. Use `extract_content_from_response` for consistent JSON-string output
and proper error handling.
##########
mcp-server/mcp_server/client/model_operation.py:
##########
@@ -106,3 +106,39 @@ async def load_model_version_by_alias(
str: JSON-formatted string containing full model version metadata
"""
pass
+
+ @abstractmethod
+ async def register_model(
+ self,
+ catalog_name: str,
+ schema_name: str,
+ name: str,
+ comment: str,
+ properties: dict,
+ ) -> str:
+ pass
+
+ @abstractmethod
+ async def delete_model(
+ self, catalog_name: str, schema_name: str, model_name: str
+ ) -> str:
+ pass
+
+ @abstractmethod
+ async def link_model_version(
+ self,
+ catalog_name: str,
+ schema_name: str,
+ model_name: str,
+ uri: str,
+ aliases: list,
+ comment: str,
+ properties: dict,
+ ) -> str:
+ pass
Review Comment:
The PR description/issue scope mentions a write operation to update model
version aliases, but this interface (and corresponding tools/client
implementation) only adds register/delete model and link/delete version. If
alias updates are intended to be part of this PR, a dedicated operation/tool
should be added (e.g., `update_model_version_aliases`) and wired through the
REST client and mocks/tests.
##########
mcp-server/mcp_server/client/plain/plain_rest_client_topic_operation.py:
##########
@@ -15,7 +15,10 @@
# specific language governing permissions and limitations
# under the License.
-from mcp_server.client.plain.utils import encode_path_segment
+from mcp_server.client.plain.utils import (
+ encode_path_segment,
+ extract_content_from_response,
+)
Review Comment:
`list_of_topics` currently returns `response.json().get(...)` directly,
which bypasses `_handle_gravitino_exception` and can silently convert
authz/REST errors into an empty list. It also returns a Python list while the
method is declared `-> str`. Use `extract_content_from_response` for consistent
JSON-string output and proper error handling (same pattern as the new write
methods).
--
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]
