jerryshao commented on code in PR #13804: URL: https://github.com/apache/gravitino/pull/13804#discussion_r4257088325
########## mcp-server/mcp_server/client/plain/plain_rest_client_semantic_model_operation.py: ########## @@ -0,0 +1,113 @@ +# 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. + +from typing import Optional + +from httpx import AsyncClient + +from mcp_server.client.plain.utils import ( + encode_path_segment, + extract_content_from_response, +) +from mcp_server.client.semantic_model_operation import SemanticModelOperation + + +class PlainRESTClientSemanticModelOperation(SemanticModelOperation): + """Access Semantic Models through Gravitino's native REST API.""" + + def __init__(self, metalake_name: str, rest_client: AsyncClient): + self.metalake_name = metalake_name + self.rest_client = rest_client + + async def list_of_semantic_models( + self, catalog_name: str, schema_name: str + ) -> str: + response = await self.rest_client.get( + self._models_path(catalog_name, schema_name) + ) + return extract_content_from_response(response, "identifiers", []) + + async def load_semantic_model( + self, catalog_name: str, schema_name: str, semantic_model_name: str + ) -> str: + response = await self.rest_client.get( + self._model_path(catalog_name, schema_name, semantic_model_name) + ) + return extract_content_from_response(response, "semanticModel", {}) + + # pylint: disable=too-many-positional-arguments + async def create_semantic_model( + self, + catalog_name: str, + schema_name: str, + name: str, + definition: dict, + comment: Optional[str] = None, + properties: Optional[dict[str, str]] = None, + ) -> str: + body = {"name": name, "definition": definition} + if comment is not None: + body["comment"] = comment + if properties is not None: + body["properties"] = properties + response = await self.rest_client.post( + self._models_path(catalog_name, schema_name), json=body + ) + return extract_content_from_response(response, "semanticModel", {}) + + async def alter_semantic_model( + self, + catalog_name: str, + schema_name: str, + semantic_model_name: str, + updates: list[dict], + ) -> str: + response = await self.rest_client.put( + self._model_path(catalog_name, schema_name, semantic_model_name), + json={"updates": updates}, + ) + return extract_content_from_response(response, "semanticModel", {}) + + async def drop_semantic_model( + self, catalog_name: str, schema_name: str, semantic_model_name: str + ) -> str: + response = await self.rest_client.delete( + self._model_path(catalog_name, schema_name, semantic_model_name) + ) + return extract_content_from_response(response, "dropped", False) + + def _models_path(self, catalog_name: str, schema_name: str) -> str: + return ( + f"/api/metalakes/{self._encode_path_name(self.metalake_name)}" + f"/catalogs/{self._encode_path_name(catalog_name)}" + f"/schemas/{self._encode_path_name(schema_name)}/semantic-models" + ) + + def _model_path( + self, catalog_name: str, schema_name: str, semantic_model_name: str + ) -> str: + return ( + f"{self._models_path(catalog_name, schema_name)}" + f"/{self._encode_path_name(semantic_model_name)}" + ) + + @staticmethod + def _encode_path_name(name: str) -> str: + # HTTP clients normalize dot segments, which can target another resource. + if name in ("", ".", ".."): + raise ValueError("REST path names must not be empty, '.' or '..'") Review Comment: [Important] This guard is the right fix, but it only protects the Semantic Model operations, while every sibling `PlainRESTClient*Operation` still interpolates `encode_path_segment(...)` with no `.`/`..` check - and some of those are destructive. Concrete case: `drop_table` builds `/api/metalakes/{m}/catalogs/{c}/schemas/{s}/tables/{name}` (`mcp-server/mcp_server/client/plain/plain_rest_client_table_operation.py:108-112`). With `table_name=".."` httpx collapses the path to `/api/metalakes/{m}/catalogs/{c}/schemas/{s}` and issues a DELETE against it - the exact endpoint `plain_rest_client_schema_operation.py:63-66` uses to drop a schema. A read-side variant: `load_model` with `model_name="."` (`plain_rest_client_model_operation.py:46-52`) collapses to the collection GET, and `extract_content_from_response(response, "model", {})` then returns `{}` instead of an error. Suggestion: move this check into `encode_path_segment` in `mcp-server/mcp_server/client/plain/utils.py:25-27` and call it directly here. That is a three-line change which fixes every operation at once, and keeps the new tests passing (they only assert the message contains "REST path names"). If you would rather keep this PR narrow, a follow-up issue referencing this helper would work too - what matters is that the knowledge does not stay local to this one file. Verified by: ran httpx 0.28.1 locally - `httpx.URL('http://h/a/b/..').raw_path` is `/a` and `.../models/.` resolves to `.../models`; then read the table/schema/model plain-client path builders and the matching server DELETE routes. ########## mcp-server/tests/unit/tools/mock_operation.py: ########## @@ -73,6 +76,9 @@ def as_partition_operation(self) -> PartitionOperation: def as_view_operation(self) -> ViewOperation: return MockViewOperation() + def as_semantic_model_operation(self) -> SemanticModelOperation: + return AsyncMock(spec=SemanticModelOperation) Review Comment: [Nit] Every other accessor in this file returns a deterministic stub whose methods return canned JSON strings (e.g. `MockViewOperation` just above). This one returns a fresh `AsyncMock(spec=SemanticModelOperation)` on each call, so a future test driving these tools through `MockOperation` gets `MagicMock` objects where the tools declare `-> str`, and `assert_awaited_with` is impossible because every call builds a new mock. Nothing is broken today - `test_semantic_model.py` goes through the real REST client with a mocked transport - so this is only about keeping the mock file uniform. A small `MockSemanticModelOperation` returning JSON strings would match the surrounding code. Verified by: read `tests/unit/tools/mock_operation.py` in full and grepped the test tree; no test currently calls a Semantic Model tool via `MockOperation`. ########## docs/gravitino-mcp-server.md: ########## @@ -138,7 +138,56 @@ Gravitino MCP server supports the following tools, and you could export tool by | `get_partition` | Retrieve a partition's metadata. Only for catalogs with a partition API. | `partition` | | `list_of_views` | Retrieve a list of views for a schema. Only for catalogs supporting views. | `view` | | `load_view` | Retrieve a view's metadata. Only for catalogs supporting views. | `view` | +| `list_of_semantic_models` | List the Semantic Models the caller can access in a schema. | `semantic_model` | +| `load_semantic_model` | Load a Semantic Model's metadata and definition. | `semantic_model` | +| `create_semantic_model` | Create a Semantic Model from a native REST definition. | `semantic_model` | +| `alter_semantic_model` | Apply Semantic Model updates atomically. | `semantic_model` | +| `drop_semantic_model` | Drop a Semantic Model's metadata. | `semantic_model` | +### Semantic Models + +Semantic Model tools manage schema-scoped semantic metadata through the Gravitino REST API. +Use the `semantic_model` tag with `--include-tool-tags` to expose these five tools. +They use the same per-call `metalake` selection and authentication as other tools; +Gravitino filters listings and enforces authorization, including access to referenced source tables. +Names used in REST paths must be non-empty and cannot be `.` or `..`, which HTTP clients +normalize as path navigation. Names must also satisfy the server's naming rules. + +For example, call `create_semantic_model` with the following arguments after the +`sales.mart.orders` source table exists in the selected metalake: + +```json +{ + "metalake": "prod", + "catalog_name": "sales", + "schema_name": "mart", + "name": "sales_model", + "definition": { + "datasets": [ + { + "name": "orders", + "source": {"namespace": ["sales", "mart"], "name": "orders"}, + "fields": [] + } + ] + }, + "comment": "Governed sales definitions", + "properties": {"domain": "sales"} +} +``` + +`definition` uses the native camelCase [Semantic Model REST schema](./open-api/semantic-models.yaml). Review Comment: [Nit] This links a raw OpenAPI fragment. `docs/open-api/semantic-models.yaml` is not a standalone spec - it is `$ref`-ed into `docs/open-api/openapi.yaml:172-181`, and no other page under `docs/` links to a local `open-api/*.yaml` file (the Iceberg page links to the published spec URL instead). On the rendered docs site that relative target is not a page, so the link is likely to dead-end for readers even though the in-repo link checker is happy because the file exists. Consider pointing at the published REST API page for Semantic Models instead, or describing the schema inline. Verified by: grepped `docs/*.md` for `open-api/` references, read `docs/open-api/openapi.yaml:172-181`, and read the asset-existence rule in `.claude/skills/gravitino-docs-refine/scripts/check_links.py:97-128`. -- 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]
