jerryshao commented on code in PR #14208:
URL: https://github.com/apache/gravitino/pull/14208#discussion_r4409589089
##########
mcp-server/mcp_server/client/plain/utils.py:
##########
@@ -37,6 +39,31 @@ 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 as exc:
Review Comment:
[Nit] A body that is not valid UTF-8 escapes this normalization.
`simplejson.loads()` on a `bytes` argument decodes before parsing, so invalid
bytes raise `UnicodeDecodeError`, not `simplejson.JSONDecodeError`. That
exception misses this `except`, so the caller gets a raw `UnicodeDecodeError`
instead of the `Invalid Gravitino response: invalid JSON` message added in
b71162e7 and, because `response.raise_for_status()` on line 51 is skipped too,
a non-2xx response with such a body reports the decode error rather than its
HTTP status.
Widening the clause covers both: `except (simplejson.JSONDecodeError,
UnicodeDecodeError) as exc:`. (`extract_content_from_response` is not affected
the same way, since `response.json()` applies httpx charset handling.)
Verified by: ran `simplejson.loads(b'\xff\xfe{"code":0}', use_decimal=True)`
in this PR's `uv --locked` environment and got `UnicodeDecodeError: 'utf-8'
codec can't decode byte 0xff in position 0`, which the current clause does not
catch.
##########
mcp-server/mcp_server/client/plain/utils.py:
##########
@@ -37,6 +39,31 @@ 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 as exc:
+ response.raise_for_status()
+ raise GravitinoException(
+ "Invalid Gravitino response: invalid JSON"
+ ) from exc
+ 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:
[Question] Is the precision guarantee meant to stay limited to these two
tools? `extract_content_from_response` and `extract_response` (lines 36-39 and
67-71, unchanged) still use stdlib `json`, so `load_semantic_model` returns the
*same* metric definitions with `aiContext` numbers rounded to float, while
`list_metric_infos` returns them exactly. An agent that lists metrics and then
loads the containing model sees two different values for one stored number.
The server keeps the full precision in both cases
(`SemanticDTOUtils.java:88-91` reads them as `BigInteger`/`BigDecimal`), so
this is purely a client-side difference. Routing the other two extractors
through `simplejson.loads(..., use_decimal=True)` would make it uniform; if you
would rather keep this PR to metric discovery, a follow-up issue would be
enough.
Verified by: read all three extractor functions in `utils.py` on this branch
and confirmed `load_semantic_model` / `create_semantic_model` still call the
stdlib-`json` extractors at
`plain_rest_client_semantic_model_operation.py:67,87,100,108`.
##########
mcp-server/mcp_server/tools/semantic_model.py:
##########
@@ -43,6 +44,65 @@ async def list_of_semantic_models(
)
)
+ @mcp.tool(
+ tags={"semantic_model"},
+ annotations=ToolAnnotations(
+ readOnlyHint=True, destructiveHint=False, idempotentHint=True
Review Comment:
[Nit] These are the only two tools in the whole MCP server carrying
`ToolAnnotations`. A client that filters or auto-approves on `readOnlyHint`
will treat the equally read-only `list_of_semantic_models` and
`load_semantic_model` (and every `list_of_*` / `load_*` tool in the other
modules) as unannotated, which the spec says to read as the conservative
default. The asymmetry makes the hint less useful than if nothing were
annotated.
Worth either annotating the other read-only semantic-model tools in this PR,
or filing a follow-up to annotate the read-only tools repo-wide so the hint
means something consistent.
Verified by: `grep -rn "ToolAnnotations\|readOnlyHint"
mcp-server/mcp_server/` returns only these two tool definitions (lines 49-50
and 78-79 of this file).
##########
docs/gravitino-mcp-server.md:
##########
@@ -139,6 +139,8 @@ Gravitino MCP server supports the following tools, and you
could export tool by
| `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` |
+| `list_metrics` | List visible metric identifiers across
Semantic Models in a schema. | `semantic_model` |
+| `list_metric_infos` | List complete visible metrics across
Semantic Models in a schema. | `semantic_model` |
Review Comment:
[Nit] One more doc page enumerates these tools and was not updated.
`docs/manage-semantic-model-metadata-using-gravitino.md:592-594` still reads
"provides `list_of_semantic_models`, `load_semantic_model`,
`create_semantic_model`, `alter_semantic_model`, and `drop_semantic_model`" and
links into this page's `#semantic-models` anchor, so it now contradicts the
table above. Adding the two new tool names there keeps the two pages consistent
(this PR already dropped the matching "these five tools" wording on line 154 of
this file).
Verified by: grepped the repo for `list_of_semantic_models` outside
`mcp-server/`; that file is the only other doc enumerating the tool set, and
`git diff --stat origin/main...HEAD` shows it is untouched by this PR.
--
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]
