Copilot commented on code in PR #11622:
URL: https://github.com/apache/gravitino/pull/11622#discussion_r3403149071
##########
mcp-server/mcp_server/server.py:
##########
@@ -109,8 +163,29 @@ def _run_stdio(self):
def _run_http(self):
_host, _port, _path = _parse_mcp_url(self.setting.mcp_url)
- asyncio.run(
- self.mcp.run_async(
- transport="http", host=_host, port=_port, path=_path
- )
+ # FastMCP accepts "http" and "streamable-http" as equivalent aliases.
Review Comment:
When `mcp_url` uses `https://` but `--tls-cert/--tls-key` are not provided
(or vice versa), the server currently still starts and may end up serving plain
HTTP on port 443 or serving TLS behind an `http://` URL. This is a
security/operational foot-gun; the scheme and TLS settings should be validated
and rejected when inconsistent.
##########
mcp-server/mcp_server/core/context.py:
##########
@@ -15,15 +15,86 @@
# specific language governing permissions and limitations
# under the License.
+from collections import OrderedDict
+
from mcp_server.client.factory import RESTClientFactory
from mcp_server.core.setting import Setting
+# Upper bound on the number of per-principal REST clients kept alive at once.
+# Each client owns an httpx connection pool; caching by Authorization header
lets
+# repeated calls from the same principal reuse a pool instead of opening a new
one
+# per tool call, while the LRU bound keeps memory/sockets in check as
principals
+# (e.g. rotating tokens) come and go.
+_MAX_CACHED_CLIENTS = 128
+
+
+def _get_request_authorization() -> str:
+ """Return the raw ``Authorization`` header of the current HTTP request.
+
+ The header is forwarded to Gravitino verbatim so the auth scheme chosen by
+ the agent (``Basic`` for simple auth, ``Bearer`` for OAuth2, ``Negotiate``
+ for Kerberos) is preserved. Returns an empty string in stdio mode or when
+ the header is absent.
+ """
+ try:
+ # Imported lazily: only available within an HTTP request context.
+ # pylint: disable=import-outside-toplevel
+ from fastmcp.server.dependencies import get_http_request
+
+ return get_http_request().headers.get("authorization", "")
+ except (LookupError, RuntimeError):
+ # No active HTTP request: stdio mode (get_http_request raises
+ # RuntimeError) or missing request context (LookupError).
+ return ""
+
class GravitinoContext:
def __init__(self, setting: Setting):
- self.gravitino_client = RESTClientFactory.create_rest_client(
- setting.metalake, setting.gravitino_uri
+ self._setting = setting
+ # Static startup identity: the --token CLI value is treated as a Bearer
+ # token (OAuth2). Used in stdio mode or as the fallback when an HTTP
+ # request carries no Authorization header.
+ default_authorization = (
+ f"Bearer {setting.token}" if setting.token else ""
+ )
+ self._default_client = RESTClientFactory.create_rest_client(
+ setting.metalake, setting.gravitino_uri, default_authorization
)
+ # LRU cache of per-principal clients keyed by the raw Authorization
header.
+ # Safe without locking: rest_client() runs on the single asyncio event
+ # loop and never awaits between lookup and insert.
+ self._clients_by_auth: "OrderedDict[str, object]" = OrderedDict()
def rest_client(self):
- return self.gravitino_client
+ """Return a REST client carrying the correct identity for this request.
+
+ In HTTP transport mode the incoming request's ``Authorization`` header
is
+ forwarded verbatim to Gravitino, taking priority over the static
startup
+ token. This keeps concurrent sessions with different principals fully
+ isolated — one principal's identity never leaks into another's calls.
+
+ Falls back to the shared default client (static startup token) when:
+ - running in stdio mode (no HTTP request context), or
+ - the incoming request carries no Authorization header.
+
+ Per-principal clients are cached (and their connection pools reused)
so a
+ new pool is not opened on every tool call.
+ """
+ authorization = _get_request_authorization()
+ if not authorization:
+ return self._default_client
+
+ cached = self._clients_by_auth.get(authorization)
+ if cached is not None:
+ self._clients_by_auth.move_to_end(authorization)
+ return cached
+
+ client = RESTClientFactory.create_rest_client(
+ self._setting.metalake,
+ self._setting.gravitino_uri,
+ authorization,
+ )
+ self._clients_by_auth[authorization] = client
+ if len(self._clients_by_auth) > _MAX_CACHED_CLIENTS:
+ self._clients_by_auth.popitem(last=False)
+ return client
Review Comment:
The per-principal REST client LRU evicts entries with `popitem()` but never
closes the evicted client's underlying `httpx.AsyncClient` connection pool.
With rotating tokens or many principals this can leak open sockets/file
descriptors until process exit.
##########
mcp-server/tests/integration/test_authz_e2e.py:
##########
@@ -0,0 +1,161 @@
+# 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.
+
+"""End-to-end authorization integration test through the live MCP HTTP server.
+
+Validates the three demo acceptance moments against a real Gravitino with
+authorization enabled:
+
+ 1. Two principals run the same discovery call and get correctly different,
+ authorization-scoped results.
+ 2. A read-only principal attempts a write through MCP and is denied by
+ Gravitino authorization.
+ 3. Both the reads and the denied write appear as audit records attributed to
+ the correct principal.
+
+These tests only run when GRAVITINO_URI / MCP_URL / MCP_METALAKE are set (see
+conftest.py); otherwise the suite is skipped.
+"""
+
+import asyncio
+import json
+import os
+import time
+
+import pytest
+from fastmcp import Client
+from fastmcp.client.transports import StreamableHttpTransport
+
+from tests.integration.gravitino_setup import basic_auth_header
+
+ADMIN = "admin"
+BOB = "bob"
+CATALOG_ALLOWED = "cat_allowed"
+CATALOG_DENIED = "cat_denied"
+
+
+def _client_for(principal: str, mcp_url: str) -> Client:
+ """Build an MCP client that authenticates as ``principal`` (simple
auth)."""
+ transport = StreamableHttpTransport(
+ url=mcp_url,
+ headers={"Authorization": basic_auth_header(principal)},
+ )
+ return Client(transport)
+
+
+async def _list_catalog_names(principal: str, mcp_url: str) -> set:
+ async with _client_for(principal, mcp_url) as client:
+ result = await client.call_tool("get_list_of_catalogs")
+ payload = json.loads(result.content[0].text)
+ return {entry["name"] for entry in payload}
+
+
+def test_moment1_authorization_scoped_discovery(
+ gravitino_fixture, integration_env
+):
+ """Moment 1: admin and bob get different, correctly scoped catalog
lists."""
+ mcp_url = integration_env["mcp_url"]
+
+ admin_catalogs = asyncio.run(_list_catalog_names(ADMIN, mcp_url))
+ bob_catalogs = asyncio.run(_list_catalog_names(BOB, mcp_url))
+
+ # Admin owns the metalake and sees both catalogs.
+ assert CATALOG_ALLOWED in admin_catalogs
+ assert CATALOG_DENIED in admin_catalogs
+
+ # Bob was granted USE_CATALOG on the allowed catalog only.
+ assert CATALOG_ALLOWED in bob_catalogs
+ assert CATALOG_DENIED not in bob_catalogs
+
+ # The two principals must receive different results.
+ assert admin_catalogs != bob_catalogs
+
+
+def test_moment2_write_denied_for_readonly_principal(
+ gravitino_fixture, integration_env
+):
+ """Moment 2: bob (no write grant) is denied when creating a tag through
MCP."""
+ mcp_url = integration_env["mcp_url"]
+
+ async def _attempt_write():
+ async with _client_for(BOB, mcp_url) as client:
+ await client.call_tool(
+ "create_tag",
+ {
+ "name": "denied_tag",
+ "comment": "should be denied",
+ "properties": {},
+ },
+ )
+
+ with pytest.raises(Exception): # noqa: B017 – any tool error means denial
+ asyncio.run(_attempt_write())
Review Comment:
`pytest.raises(Exception)` will pass for any failure (including transport
errors, missing tools, or server crashes), so this test may not actually verify
that the failure is an authorization denial. It should assert that the error
indicates a forbidden/unauthorized response.
##########
mcp-server/tests/unit/test_per_request_token.py:
##########
@@ -0,0 +1,194 @@
+# 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.
+
+"""Tests for per-request identity isolation (Task 6).
+
+GravitinoContext.rest_client() must forward the current HTTP request's raw
+Authorization header (any scheme) to Gravitino, not the shared startup token,
+so concurrent multi-principal sessions stay fully isolated in HTTP mode.
+"""
+
+import unittest
+from unittest.mock import MagicMock, patch
+
+from mcp_server.core import context as context_module
+from mcp_server.core.context import (
+ GravitinoContext,
+ _get_request_authorization,
+)
+from mcp_server.core.setting import Setting
+
+# Tests intentionally exercise context/client internals (e.g. _default_client,
+# _catalog_operation) to assert per-request isolation; protected access is
expected.
+# pylint: disable=protected-access
+
+
+class TestGetRequestAuthorization(unittest.TestCase):
+ """Unit tests for _get_request_authorization() (HTTP context
extraction)."""
+
+ def test_returns_raw_bearer_header(self):
+ mock_request = MagicMock()
+ mock_request.headers.get.return_value = "Bearer request-token-xyz"
+
+ with patch(
+ "fastmcp.server.dependencies.get_http_request",
+ return_value=mock_request,
+ ):
+ authorization = _get_request_authorization()
+
+ self.assertEqual(authorization, "Bearer request-token-xyz")
+
+ def test_returns_raw_basic_header_verbatim(self):
+ """Basic (simple auth) headers must pass through unchanged, not be
dropped."""
+ mock_request = MagicMock()
+ mock_request.headers.get.return_value = "Basic YWxpY2U6ZHVtbXk="
+
+ with patch(
+ "fastmcp.server.dependencies.get_http_request",
+ return_value=mock_request,
+ ):
+ authorization = _get_request_authorization()
+
+ self.assertEqual(authorization, "Basic YWxpY2U6ZHVtbXk=")
+
+ def test_returns_empty_when_no_http_context(self):
+ """Simulates stdio mode where get_http_request raises LookupError."""
+ with patch(
+ "fastmcp.server.dependencies.get_http_request",
+ side_effect=LookupError("no request context"),
+ ):
+ authorization = _get_request_authorization()
+
+ self.assertEqual(authorization, "")
+
+ def test_returns_empty_when_no_authorization_header(self):
+ mock_request = MagicMock()
+ mock_request.headers.get.return_value = ""
+
+ with patch(
+ "fastmcp.server.dependencies.get_http_request",
+ return_value=mock_request,
+ ):
+ authorization = _get_request_authorization()
+
+ self.assertEqual(authorization, "")
+
+
+class TestGravitinoContextPerRequestAuthorization(unittest.TestCase):
+ """GravitinoContext.rest_client() isolates per-request identities."""
+
+ def _make_context(self, startup_token: str = "") -> GravitinoContext:
+ return GravitinoContext(
+ Setting(
+ metalake="ml",
+ gravitino_uri="http://localhost:8090";,
+ token=startup_token,
Review Comment:
These tests assume `GravitinoContext.rest_client()` returns a real
`PlainRESTClientOperation` instance (they inspect
`client._catalog_operation.rest_client`). Because other unit tests globally
swap `RESTClientFactory` to `MockOperation` without restoring it, the test can
become order-dependent. Set the factory explicitly in `setUp()`.
##########
mcp-server/tests/unit/test_auth_flow.py:
##########
@@ -0,0 +1,180 @@
+# 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.
+
+import asyncio
+import sys
+import unittest
+from unittest import mock
+
+from mcp_server.client.plain.plain_rest_client_operation import (
+ PlainRESTClientOperation,
+)
+from mcp_server.core.context import GravitinoContext
+from mcp_server.core.setting import Setting
+from mcp_server.main import _parse_args
+
+
+def _shared_rest_client(operation: PlainRESTClientOperation):
+ # pylint: disable=protected-access
+ return operation._catalog_operation.rest_client
+
+
+def _headers_of(operation: PlainRESTClientOperation):
+ return _shared_rest_client(operation).headers
+
+
+def _close(operation: PlainRESTClientOperation):
+ asyncio.run(_shared_rest_client(operation).aclose())
+
+
+class TestAuthorizationInjection(unittest.TestCase):
+ """Verify the Authorization header is forwarded verbatim to the httpx
client."""
+
+ def test_bearer_authorization_header(self):
+ """A Bearer authorization value is forwarded unchanged."""
+ client = PlainRESTClientOperation(
+ "my_metalake",
+ "http://localhost:8090";,
+ authorization="Bearer my-secret-token",
+ )
+ try:
+ self.assertEqual(
+ _headers_of(client).get("Authorization"),
+ "Bearer my-secret-token",
+ )
+ finally:
+ _close(client)
+
+ def test_basic_authorization_header(self):
+ """A Basic authorization value (simple auth) is forwarded unchanged."""
+ client = PlainRESTClientOperation(
+ "my_metalake",
+ "http://localhost:8090";,
+ authorization="Basic YWxpY2U6ZHVtbXk=",
+ )
+ try:
+ self.assertEqual(
+ _headers_of(client).get("Authorization"),
+ "Basic YWxpY2U6ZHVtbXk=",
+ )
+ finally:
+ _close(client)
+
+ def test_empty_authorization_no_header(self):
+ """When authorization is empty, no Authorization header is added."""
+ client = PlainRESTClientOperation(
+ "my_metalake", "http://localhost:8090";, authorization=""
+ )
+ try:
+ self.assertIsNone(_headers_of(client).get("Authorization"))
+ finally:
+ _close(client)
+
+ def test_no_authorization_argument_no_header(self):
+ """When authorization argument is omitted, no Authorization header is
added."""
+ client = PlainRESTClientOperation(
+ "my_metalake", "http://localhost:8090";
+ )
+ try:
+ self.assertIsNone(_headers_of(client).get("Authorization"))
+ finally:
+ _close(client)
+
+
+class TestSettingTokenMasking(unittest.TestCase):
+ """Verify that the token is not exposed in Setting string
representations."""
+
+ def test_token_masked_in_str(self):
+ """Token value must not appear in Setting.__str__."""
+ setting = Setting(metalake="ml", token="super-secret-token-value")
+ self.assertNotIn("super-secret-token-value", str(setting))
+ self.assertIn("***", str(setting))
+
+ def test_token_not_in_repr(self):
+ """Token value must not appear in Setting.__repr__ either."""
+ setting = Setting(metalake="ml", token="super-secret-token-value")
+ self.assertNotIn("super-secret-token-value", repr(setting))
+
+ def test_empty_token_shows_empty_in_str(self):
+ """When no token is set, __str__ shows empty placeholder."""
+ setting = Setting(metalake="ml", token="")
+ self.assertNotIn("***", str(setting))
+
+
+class TestTokenArgParsing(unittest.TestCase):
+ """Verify --token CLI argument and GRAVITINO_TOKEN env var precedence."""
+
+ def test_env_var_used_when_token_omitted(self):
+ """GRAVITINO_TOKEN is used when --token is not passed."""
+ with mock.patch.dict(
+ "os.environ", {"GRAVITINO_TOKEN": "env-token"}
+ ), mock.patch.object(sys, "argv", ["prog", "--metalake", "ml"]):
+ args = _parse_args()
+ self.assertEqual(args.token, "env-token")
+
+ def test_cli_token_overrides_env_var(self):
+ """--token takes precedence over GRAVITINO_TOKEN when both are set."""
+ with mock.patch.dict(
+ "os.environ", {"GRAVITINO_TOKEN": "env-token"}
+ ), mock.patch.object(
+ sys, "argv", ["prog", "--metalake", "ml", "--token", "cli-token"]
+ ):
+ args = _parse_args()
+ self.assertEqual(args.token, "cli-token")
+
+ def test_no_token_anywhere_defaults_to_empty(self):
+ """Without --token and GRAVITINO_TOKEN, token defaults to empty
string."""
+ with mock.patch.dict("os.environ", {}, clear=True), mock.patch.object(
+ sys, "argv", ["prog", "--metalake", "ml"]
+ ):
+ args = _parse_args()
+ self.assertEqual(args.token, "")
+
+
+class TestGravitinoContextTokenPropagation(unittest.TestCase):
+ """Verify GravitinoContext passes token from Setting to the REST client."""
+
+ def test_context_propagates_token(self):
Review Comment:
These context propagation tests rely on `RESTClientFactory` using the real
`PlainRESTClientOperation`, but other unit tests change the factory globally to
`MockOperation` without resetting it. Explicitly setting the factory in
`setUp()` makes the test robust regardless of execution order.
##########
mcp-server/mcp_server/core/audit.py:
##########
@@ -0,0 +1,76 @@
+# 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.
+
+import base64
+import binascii
+import json
+import logging
+from datetime import datetime, timezone
+
+_audit_logger = logging.getLogger("gravitino.mcp.audit")
+
+
+def _extract_principal(authorization: str) -> str:
+ """Derive a display principal from a raw Authorization header value.
+
+ - "Basic <base64(user:secret)>" → "<user>" (Gravitino simple auth)
+ - "Bearer <token>" → "bearer:<first-8-chars-of-token>"
+ - empty / missing / unparsable → "anonymous"
+ """
+ if not authorization:
+ return "anonymous"
+ parts = authorization.split()
+ if len(parts) != 2:
+ return "anonymous"
+ scheme, credential = parts[0].lower(), parts[1]
+ if scheme == "basic":
+ try:
+ decoded = base64.b64decode(credential).decode("utf-8")
+ except (binascii.Error, UnicodeDecodeError, ValueError):
+ return "anonymous"
+ user = decoded.split(":", 1)[0]
+ return user if user else "anonymous"
Review Comment:
`_extract_principal()` uses `base64.b64decode()` without `validate=True`,
which can accept some malformed inputs and potentially produce confusing
principals. Using strict validation makes parsing safer and more predictable
for audit attribution.
##########
mcp-server/mcp_server/core/audit.py:
##########
@@ -0,0 +1,76 @@
+# 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.
+
+import base64
+import binascii
+import json
+import logging
+from datetime import datetime, timezone
+
+_audit_logger = logging.getLogger("gravitino.mcp.audit")
+
+
+def _extract_principal(authorization: str) -> str:
+ """Derive a display principal from a raw Authorization header value.
+
+ - "Basic <base64(user:secret)>" → "<user>" (Gravitino simple auth)
+ - "Bearer <token>" → "bearer:<first-8-chars-of-token>"
+ - empty / missing / unparsable → "anonymous"
+ """
+ if not authorization:
+ return "anonymous"
+ parts = authorization.split()
+ if len(parts) != 2:
+ return "anonymous"
+ scheme, credential = parts[0].lower(), parts[1]
+ if scheme == "basic":
+ try:
+ decoded = base64.b64decode(credential).decode("utf-8")
+ except (binascii.Error, UnicodeDecodeError, ValueError):
+ return "anonymous"
+ user = decoded.split(":", 1)[0]
+ return user if user else "anonymous"
+ if scheme == "bearer":
+ return f"bearer:{credential[:8]}"
+ return "anonymous"
+
+
+def emit(
+ *,
+ principal: str,
+ tool: str,
+ outcome: str,
+ error_type: str = "",
+) -> None:
+ """Write one structured JSON audit record to the audit logger.
+
+ Args:
+ principal: Identity derived from the request (e.g. "bearer:abc12345"
or "anonymous").
+ tool: MCP tool name that was invoked.
+ outcome: "allow" for successful calls, "deny" for authorization
failures.
+ error_type: Exception class name when outcome is "deny", empty
otherwise.
Review Comment:
The `emit()` docstring says `outcome="deny"` is for authorization failures,
but `AuditMiddleware` currently emits `deny` for any exception type. The
documentation should match the actual semantics so downstream consumers don't
misinterpret failures as authorization denials.
##########
mcp-server/tests/unit/test_transport_tls.py:
##########
@@ -0,0 +1,124 @@
+# 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.
+
+"""Tests for HTTP transport URL parsing, the streamable-http alias, and TLS
wiring."""
+
+import unittest
+from unittest.mock import patch
+
+from mcp_server.client.factory import RESTClientFactory
+from mcp_server.core.setting import Setting
+from mcp_server.server import GravitinoMCPServer, _parse_mcp_url
+from tests.unit.tools import MockOperation
+
+
+class TestParseMcpUrl(unittest.TestCase):
+ """_parse_mcp_url accepts http and https and rejects other schemes."""
+
+ def test_http_url(self):
+ self.assertEqual(
+ _parse_mcp_url("http://127.0.0.1:8000/mcp";),
+ ("127.0.0.1", 8000, "/mcp"),
+ )
+
+ def test_https_url(self):
+ self.assertEqual(
+ _parse_mcp_url("https://mcphost:9443/mcp";),
+ ("mcphost", 9443, "/mcp"),
+ )
+
+ def test_https_default_port(self):
+ _, port, _ = _parse_mcp_url("https://mcphost/mcp";)
+ self.assertEqual(port, 443)
+
+ def test_http_default_port(self):
+ _, port, _ = _parse_mcp_url("http://mcphost/mcp";)
+ self.assertEqual(port, 80)
+
+ def test_unsupported_scheme_rejected(self):
+ with self.assertRaises(ValueError):
+ _parse_mcp_url("ftp://mcphost/mcp";)
+
+
+class TestRunHttpTransport(unittest.TestCase):
+ """GravitinoMCPServer.run() wires transport name and TLS config
correctly."""
+
+ def setUp(self):
+ RESTClientFactory.set_rest_client(MockOperation)
+
Review Comment:
This test mutates the global `RESTClientFactory` client class in `setUp()`
but never restores it, which can make other unit tests order-dependent/flaky
(especially tests that expect the default `PlainRESTClientOperation`). Add a
`tearDown()` to reset the factory.
##########
mcp-server/.inspector-demo-inspector.out:
##########
@@ -0,0 +1,34 @@
+Starting MCP inspector...
+⚙️ Proxy server listening on localhost:6277
+⚠️ WARNING: Authentication is disabled. This is not recommended.
+
+🚀 MCP Inspector is up and running at:
+ http://localhost:6274
+
+New StreamableHttp connection request
+Query parameters:
{"url":"http://127.0.0.1:8000/mcp","transportType":"streamable-http"}
+Created StreamableHttp client transport
+Client <-> Proxy sessionId: 257a7f2e-5aba-47eb-8fb9-1704738bd68a
+Proxy <-> Server sessionId: a1836f818e0e49cd9b6609f31d5aebfc
+Received POST message for sessionId 257a7f2e-5aba-47eb-8fb9-1704738bd68a
+Received GET message for sessionId 257a7f2e-5aba-47eb-8fb9-1704738bd68a
+Received POST message for sessionId 257a7f2e-5aba-47eb-8fb9-1704738bd68a
+Error:
+🚀 MCP Inspector is up and running at:
+ http://localhost:6274
+
+
+Failed with exit code: null
+ at Object.error
(/Users/yuqi/.npm/_npx/5a9d879542beca3a/node_modules/spawn-rx/lib/src/index.js:314:27)
+ at ConsumerObserver.error
(/Users/yuqi/.npm/_npx/5a9d879542beca3a/node_modules/rxjs/dist/cjs/internal/Subscriber.js:124:33)
+ at Subscriber._error
(/Users/yuqi/.npm/_npx/5a9d879542beca3a/node_modules/rxjs/dist/cjs/internal/Subscriber.js:84:30)
+ at Subscriber.error
(/Users/yuqi/.npm/_npx/5a9d879542beca3a/node_modules/rxjs/dist/cjs/internal/Subscriber.js:60:18)
Review Comment:
This is a captured local Inspector run log that includes a user-specific
filesystem path (`/Users/...`) and runtime errors. Log/output artifacts like
this should not be committed; remove it from the PR and ignore it going forward.
##########
mcp-server/.inspector-demo-mcp.out:
##########
@@ -0,0 +1,57 @@
+
+
+╭──────────────────────────────────────────────────────────────────────────────╮
+│
│
+│
│
+│ ▄▀▀ ▄▀█ █▀▀ ▀█▀ █▀▄▀█ █▀▀ █▀█
│
+│ █▀ █▀█ ▄▄█ █ █ ▀ █ █▄▄ █▀▀
│
+│
│
+│
│
+│ FastMCP 3.2.0
│
+│ https://gofastmcp.com
│
+│
│
+│ 🖥 Server: Gravitino MCP Server, 3.2.0
│
+│ 🚀 Deploy free: https://horizon.prefect.io │
+│
│
+╰──────────────────────────────────────────────────────────────────────────────╯
+╭──────────────────────────────────────────────────────────────────────────────╮
+│ 🎉 Update available: 3.4.2 │
+│ Run: pip install --upgrade fastmcp
│
+╰──────────────────────────────────────────────────────────────────────────────╯
+
+
+[06/12/26 11:03:27] INFO Starting MCP server 'Gravitino MCP
transport.py:299
+ Server' with transport 'http' on
+ http://127.0.0.1:8000/mcp
Review Comment:
This looks like a captured runtime log/output file (including
interactive/terminal rendering) and includes environment-specific noise.
Generated `.out` artifacts shouldn't be committed to the repository; remove it
from the PR and add it to `.gitignore` if it is produced by local scripts.
--
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]
