Copilot commented on code in PR #14208:
URL: https://github.com/apache/gravitino/pull/14208#discussion_r4401073987
##########
mcp-server/mcp_server/client/plain/utils.py:
##########
@@ -37,6 +39,29 @@ def extract_content_from_response(response, field: str,
default="") -> str:
return json.dumps(response_json.get(field, default))
+def extract_list_from_response(response, field: str) -> str:
+ """Extract a required JSON array without rounding fractional numbers.
+
+ Propagate server errors and reject malformed envelopes instead of returning
+ an empty result for a failed discovery request.
+ """
+ try:
+ payload = simplejson.loads(response.content, use_decimal=True)
+ except simplejson.JSONDecodeError:
+ response.raise_for_status()
+ raise
+ if isinstance(payload, dict):
+ _handle_gravitino_exception(payload)
+ response.raise_for_status()
+ if not isinstance(payload, dict) or "code" not in payload:
+ raise GravitinoException("Invalid Gravitino response: missing code")
+ if not isinstance(payload.get(field), list):
+ raise GravitinoException(
+ f"Invalid Gravitino response: {field} must be a JSON array"
+ )
+ return simplejson.dumps(payload[field], use_decimal=True)
Review Comment:
For HTTP 200 responses with invalid JSON, this re-raises
`simplejson.JSONDecodeError`, which can leak a low-level parsing exception to
callers with a less actionable message. Consider translating the decode failure
into a `GravitinoException` with a consistent, user-facing message (e.g.,
\"Invalid Gravitino response: invalid JSON\"), while still preserving
`raise_for_status()` behavior for non-2xx responses.
##########
mcp-server/pyproject.toml:
##########
@@ -31,6 +31,8 @@ dependencies = [
"pydantic>=2.12.0,<3",
# httpx.Auth plugin for hop-2 client_credentials fetch/cache.
"httpx-auth>=0.22,<0.24",
+ # Preserve exact JSON numbers in Semantic Model metric discovery responses.
+ "simplejson==3.20.1",
Review Comment:
Pinning `simplejson` to an exact version can make installs brittle and
prevent receiving compatible bugfix releases. Consider using a bounded range
(e.g., `>=3.20.1,<4`) unless deterministic builds are a hard requirement for
this project.
##########
mcp-server/tests/integration/test_semantic_model_metric_discovery.py:
##########
@@ -0,0 +1,343 @@
+# 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.
+
+"""Discover metrics through the live MCP HTTP server and Gravitino REST API."""
+
+import asyncio
+from decimal import Decimal
+from uuid import uuid4
+
+import httpx
+import pytest
+import simplejson
+from fastmcp import Client
+from fastmcp.client.transports import StreamableHttpTransport
+from fastmcp.exceptions import ToolError
+
+from tests.integration.gravitino_setup import basic_auth_header
+
+# Pytest injects fixtures by argument name.
+# pylint: disable=redefined-outer-name
+
+
+class MetricDiscoveryFixture:
+ """Isolated Semantic Models and role grants for each live-server test."""
+
+ def __init__(self, env, rest_client):
+ self.env = env
+ self.rest_client = rest_client
+ self.metalake = "metric_mcp_" + uuid4().hex
+ self.base = f"/api/metalakes/{self.metalake}"
+ self.models_path = (
+ self.base
+ + "/catalogs/metrics_catalog/schemas/semantic/semantic-models"
+ )
+ self.revenue = self.metric("Revenue")
+ self.lower_revenue = self.metric("revenue")
+
+ def request(self, method, path, body=None):
+ response = self.rest_client.request(
+ method,
+ path,
+ content=(
+ None
+ if body is None
+ else simplejson.dumps(body, use_decimal=True)
+ ),
+ )
+ response.raise_for_status()
+ return simplejson.loads(response.content, use_decimal=True)
+
+ def provision(self):
+ self.request(
+ "POST",
+ self.base + "/catalogs",
+ {
+ "name": "metrics_catalog",
+ "type": "RELATIONAL",
+ "provider": "lakehouse-generic",
+ "comment": "",
+ "properties": {},
+ },
+ )
+ schemas_path = self.base + "/catalogs/metrics_catalog/schemas"
+ for schema in ("source", "semantic", "other", "empty"):
+ self.request(
+ "POST",
+ schemas_path,
+ {"name": schema, "comment": "", "properties": {}},
+ )
+ self.request(
+ "POST",
+ schemas_path + "/source/tables",
+ {
+ "name": "orders",
+ "columns": [{"name": "amount", "type": "long"}],
+ "properties": {
+ "format": "delta",
+ "external": "true",
+ "location": f"file:///tmp/{self.metalake}/orders",
+ },
+ },
+ )
+ for model, metrics in (
+ ("sales", [self.lower_revenue, self.revenue]),
+ ("hidden", [self.revenue]),
+ ("no_metrics", []),
+ ):
+ self.request(
+ "POST",
+ self.models_path,
+ {
+ "name": model,
+ "definition": self.definition(metrics),
+ "properties": {},
+ },
+ )
+ self.request(
+ "POST",
+ schemas_path + "/other/semantic-models",
+ {
+ "name": "sales",
+ "definition": self.definition([self.metric("other_schema")]),
+ },
+ )
+ self.request("POST", self.base + "/users", {"name": "metric_reader"})
+ self.request(
+ "POST",
+ self.base + "/roles",
+ {
+ "name": "metric_reader",
+ "properties": {},
+ "securableObjects": [
+ {
+ "fullName": "metrics_catalog",
+ "type": "CATALOG",
+ "privileges": [
+ {"name": "USE_CATALOG", "condition": "ALLOW"}
+ ],
+ },
+ {
+ "fullName": "metrics_catalog.semantic",
+ "type": "SCHEMA",
+ "privileges": [
+ {"name": "USE_SCHEMA", "condition": "ALLOW"}
+ ],
+ },
+ ],
+ },
+ )
+ self.request(
+ "PUT",
+ self.base + "/permissions/users/metric_reader/grant",
+ {"roleNames": ["metric_reader"]},
+ )
+
+ def call(self, tool, principal="admin", schema="semantic"):
+ async def invoke():
+ transport = StreamableHttpTransport(
+ url=self.env["mcp_url"],
+ headers={"Authorization": basic_auth_header(principal)},
+ )
+ async with Client(transport) as client:
+ result = await client.call_tool(
+ tool,
+ {
+ "metalake": self.metalake,
+ "catalog_name": "metrics_catalog",
+ "schema_name": schema,
+ },
+ )
+ return simplejson.loads(
+ result.content[0].text, use_decimal=True
+ )
+
+ return asyncio.run(invoke())
+
+ def assert_discovery(self, expected, principal="admin"):
+ assert self.call("list_metric_infos", principal) == expected
+ assert self.call("list_metrics", principal) == [
Review Comment:
Using `asyncio.run()` inside tests makes these helpers incompatible with
environments that already have a running event loop (it raises `RuntimeError`),
and it makes future async fixtures / async test refactors harder. Prefer making
`call()` async and using `pytest.mark.asyncio` (or the repo’s existing async
test pattern) to await the tool calls directly.
--
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]
