codeant-ai-for-open-source[bot] commented on code in PR #36497:
URL: https://github.com/apache/superset/pull/36497#discussion_r2608568975
##########
superset/mcp_service/auth.py:
##########
@@ -231,4 +232,63 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
finally:
_cleanup_session_finally()
- return sync_wrapper # type: ignore[return-value]
+ wrapper = sync_wrapper
+
+ # Merge original function's __globals__ into wrapper's __globals__
+ # This allows get_type_hints() to resolve type annotations from the
+ # original module (e.g., Context from fastmcp)
+ # FastMCP 2.13.2+ uses get_type_hints() which needs access to these types
+ merged_globals = {**wrapper.__globals__, **tool_func.__globals__} # type:
ignore[attr-defined]
+ new_wrapper = types.FunctionType(
+ wrapper.__code__, # type: ignore[attr-defined]
+ merged_globals,
+ wrapper.__name__,
+ wrapper.__defaults__, # type: ignore[attr-defined]
+ wrapper.__closure__, # type: ignore[attr-defined]
+ )
+ # Copy __dict__ but exclude __wrapped__
+ # NOTE: We intentionally do NOT preserve __wrapped__ here.
+ # Setting __wrapped__ causes inspect.signature() to follow the chain
+ # and find 'ctx' in the original function's signature, even after
+ # FastMCP's create_function_without_params removes it from annotations.
+ # This breaks Pydantic's TypeAdapter which expects signature params
+ # to match type_hints.
+ new_wrapper.__dict__.update(
+ {k: v for k, v in wrapper.__dict__.items() if k != "__wrapped__"}
+ )
+ new_wrapper.__module__ = wrapper.__module__
+ new_wrapper.__qualname__ = wrapper.__qualname__
+ new_wrapper.__annotations__ = wrapper.__annotations__
Review Comment:
**Suggestion:** Copying `wrapper.__dict__` into `new_wrapper.__dict__`
without preserving `__wrapped__` is intentional, but assigning
`new_wrapper.__annotations__ = wrapper.__annotations__` directly can make
`__annotations__` refer to a mutable dict shared between functions; mutating it
later could affect both; make a shallow copy when assigning to avoid accidental
cross-function mutation. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Make a shallow copy to avoid sharing the same annotations dict between
functions
new_wrapper.__annotations__ = dict(wrapper.__annotations__) if
wrapper.__annotations__ is not None else {}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Assigning the same mutable dict to both functions means later mutation of
annotations on one will affect the other. Making a shallow copy avoids
surprising cross-talk and is a low-risk correctness improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 261:261
**Comment:**
*Possible Bug: Copying `wrapper.__dict__` into `new_wrapper.__dict__`
without preserving `__wrapped__` is intentional, but assigning
`new_wrapper.__annotations__ = wrapper.__annotations__` directly can make
`__annotations__` refer to a mutable dict shared between functions; mutating it
later could affect both; make a shallow copy when assigning to avoid accidental
cross-function mutation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/caching.py:
##########
@@ -0,0 +1,137 @@
+# 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.
+
+"""
+MCP response caching using FastMCP's native ResponseCachingMiddleware.
+"""
+
+import logging
+from typing import Any, Dict
+
+from superset.mcp_service.storage import get_mcp_store
+
+logger = logging.getLogger(__name__)
+
+
+def _build_caching_settings(cache_config: Dict[str, Any]) -> Dict[str, Any]:
+ """
+ Build FastMCP caching settings from MCP_CACHE_CONFIG.
+
+ Maps our config format to FastMCP's settings objects:
+ - list_tools_ttl -> list_tools_settings
+ - list_resources_ttl -> list_resources_settings
+ - list_prompts_ttl -> list_prompts_settings
+ - read_resource_ttl -> read_resource_settings
+ - get_prompt_ttl -> get_prompt_settings
+ - call_tool_ttl + excluded_tools -> call_tool_settings
+
+ TTL values are already integers (Python evaluates '60 * 5' at config load
time).
+
+ Args:
+ cache_config: MCP_CACHE_CONFIG dict
+
+ Returns:
+ Dict of settings kwargs for ResponseCachingMiddleware
+ """
+ settings: Dict[str, Any] = {}
+
+ # List operations (default 5 min)
+ if "list_tools_ttl" in cache_config:
+ settings["list_tools_settings"] = {"ttl":
cache_config["list_tools_ttl"]}
+ if "list_resources_ttl" in cache_config:
+ settings["list_resources_settings"] = {
+ "ttl": cache_config["list_resources_ttl"]
+ }
+ if "list_prompts_ttl" in cache_config:
+ settings["list_prompts_settings"] = {"ttl":
cache_config["list_prompts_ttl"]}
+
+ # Individual item operations (default 1 hour)
+ if "read_resource_ttl" in cache_config:
+ settings["read_resource_settings"] = {"ttl":
cache_config["read_resource_ttl"]}
+ if "get_prompt_ttl" in cache_config:
+ settings["get_prompt_settings"] = {"ttl":
cache_config["get_prompt_ttl"]}
Review Comment:
**Suggestion:** Type/validation bug: TTL values are passed straight from
`MCP_CACHE_CONFIG` into settings without coercion or validation; if a TTL is
None, non-integer, or invalid, the downstream middleware may raise exceptions.
Coerce TTLs to int and log/skip invalid values to avoid runtime errors. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
try:
ttl = int(cache_config["read_resource_ttl"])
settings["read_resource_settings"] = {"ttl": ttl}
except (TypeError, ValueError):
logger.warning("Invalid 'read_resource_ttl' in MCP_CACHE_CONFIG;
must be an integer, ignoring setting")
if "get_prompt_ttl" in cache_config:
try:
ttl = int(cache_config["get_prompt_ttl"])
settings["get_prompt_settings"] = {"ttl": ttl}
except (TypeError, ValueError):
logger.warning("Invalid 'get_prompt_ttl' in MCP_CACHE_CONFIG;
must be an integer, ignoring setting")
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a sensible validation improvement. Passing unvalidated values from
config into the middleware can cause runtime exceptions. Coercing to int and
skipping/bubbling invalid values with a warning avoids runtime surprises and
makes behavior explicit.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/caching.py
**Line:** 64:66
**Comment:**
*Type Error: Type/validation bug: TTL values are passed straight from
`MCP_CACHE_CONFIG` into settings without coercion or validation; if a TTL is
None, non-integer, or invalid, the downstream middleware may raise exceptions.
Coerce TTLs to int and log/skip invalid values to avoid runtime errors.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/app.py:
##########
@@ -165,6 +167,8 @@ def _build_mcp_kwargs(
mcp_kwargs["include_tags"] = include_tags
if exclude_tags is not None:
mcp_kwargs["exclude_tags"] = exclude_tags
+ if middleware is not None:
+ mcp_kwargs["middleware"] = middleware
# Add any additional kwargs
mcp_kwargs.update(kwargs)
Review Comment:
**Suggestion:** The function first sets `mcp_kwargs["middleware"]` and then
calls `mcp_kwargs.update(kwargs)`, so a `middleware` value supplied inside
`kwargs` will silently override the explicit `middleware` parameter (or vice
versa depending on ordering); ensure a deterministic precedence by updating
with `kwargs` first and then applying the explicit `middleware` parameter.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
# Add any additional kwargs first so explicit parameters override them
mcp_kwargs.update(kwargs)
if middleware is not None:
mcp_kwargs["middleware"] = middleware
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current ordering allows a middleware key in kwargs to silently override
the explicit middleware parameter (surprising precedence). Updating kwargs
first, then setting the explicit middleware enforces the intuitive rule that
explicit function parameters take precedence and fixes a potential logic bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/app.py
**Line:** 170:174
**Comment:**
*Logic Error: The function first sets `mcp_kwargs["middleware"]` and
then calls `mcp_kwargs.update(kwargs)`, so a `middleware` value supplied inside
`kwargs` will silently override the explicit `middleware` parameter (or vice
versa depending on ordering); ensure a deterministic precedence by updating
with `kwargs` first and then applying the explicit `middleware` parameter.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/auth.py:
##########
@@ -231,4 +232,63 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
finally:
_cleanup_session_finally()
- return sync_wrapper # type: ignore[return-value]
+ wrapper = sync_wrapper
+
+ # Merge original function's __globals__ into wrapper's __globals__
+ # This allows get_type_hints() to resolve type annotations from the
+ # original module (e.g., Context from fastmcp)
+ # FastMCP 2.13.2+ uses get_type_hints() which needs access to these types
+ merged_globals = {**wrapper.__globals__, **tool_func.__globals__} # type:
ignore[attr-defined]
+ new_wrapper = types.FunctionType(
Review Comment:
**Suggestion:** The merged globals dictionary may lack a '__builtins__'
entry, which will cause runtime NameError/Name lookup failures when the newly
created function executes; ensure '__builtins__' is present in `merged_globals`
before passing it to `types.FunctionType`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Ensure builtins exist in the globals mapping required by FunctionType
if "__builtins__" not in merged_globals:
merged_globals["__builtins__"] =
wrapper.__globals__.get("__builtins__", __builtins__)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
FunctionType expects a globals mapping; if neither source contains
'__builtins__' (rare but possible in some constructed globals), name lookups
inside the new function can fail. Ensuring '__builtins__' exists is a
defensive, correct fix with minimal risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 242:242
**Comment:**
*Possible Bug: The merged globals dictionary may lack a '__builtins__'
entry, which will cause runtime NameError/Name lookup failures when the newly
created function executes; ensure '__builtins__' is present in `merged_globals`
before passing it to `types.FunctionType`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/auth.py:
##########
@@ -231,4 +232,63 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
finally:
_cleanup_session_finally()
- return sync_wrapper # type: ignore[return-value]
+ wrapper = sync_wrapper
+
+ # Merge original function's __globals__ into wrapper's __globals__
+ # This allows get_type_hints() to resolve type annotations from the
+ # original module (e.g., Context from fastmcp)
+ # FastMCP 2.13.2+ uses get_type_hints() which needs access to these types
+ merged_globals = {**wrapper.__globals__, **tool_func.__globals__} # type:
ignore[attr-defined]
+ new_wrapper = types.FunctionType(
+ wrapper.__code__, # type: ignore[attr-defined]
+ merged_globals,
+ wrapper.__name__,
+ wrapper.__defaults__, # type: ignore[attr-defined]
+ wrapper.__closure__, # type: ignore[attr-defined]
+ )
+ # Copy __dict__ but exclude __wrapped__
+ # NOTE: We intentionally do NOT preserve __wrapped__ here.
+ # Setting __wrapped__ causes inspect.signature() to follow the chain
+ # and find 'ctx' in the original function's signature, even after
+ # FastMCP's create_function_without_params removes it from annotations.
+ # This breaks Pydantic's TypeAdapter which expects signature params
+ # to match type_hints.
+ new_wrapper.__dict__.update(
+ {k: v for k, v in wrapper.__dict__.items() if k != "__wrapped__"}
+ )
+ new_wrapper.__module__ = wrapper.__module__
+ new_wrapper.__qualname__ = wrapper.__qualname__
+ new_wrapper.__annotations__ = wrapper.__annotations__
+
+ # Set __signature__ from the original function, but:
+ # 1. Remove ctx parameter - FastMCP tools don't expose it to clients
+ # 2. Skip if original has *args (parse_request output has its own handling)
+ from fastmcp import Context as FMContext
Review Comment:
**Suggestion:** Importing `Context` from `fastmcp` without guarding will
raise ImportError (or other errors) at decoration time if the `fastmcp` package
is not available or fails to import; this will break any module import or
decorator application that triggers `mcp_auth_hook`. Wrap the import in a
try/except and provide a safe fallback (e.g., set `FMContext = None`) so the
decorator can still be defined in environments where `fastmcp` is optional.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
try:
from fastmcp import Context as FMContext
except Exception:
# fastmcp may be optional in some environments (tests, minimal
installs).
# Fall back to None and handle this case below when comparing
annotations.
FMContext = None
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The import occurs at decorator run-time inside mcp_auth_hook and will raise
if fastmcp is missing, breaking any code paths that call the decorator.
Guarding the import (catching ImportError) is a small, safe hardening that lets
the module remain importable in minimal environments while the later annotation
checks still work (they already tolerate a non-matching FMContext).
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 266:266
**Comment:**
*Possible Bug: Importing `Context` from `fastmcp` without guarding will
raise ImportError (or other errors) at decoration time if the `fastmcp` package
is not available or fails to import; this will break any module import or
decorator application that triggers `mcp_auth_hook`. Wrap the import in a
try/except and provide a safe fallback (e.g., set `FMContext = None`) so the
decorator can still be defined in environments where `fastmcp` is optional.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_storage.py:
##########
@@ -0,0 +1,96 @@
+# 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 MCP storage factory."""
+
+from unittest.mock import MagicMock, patch
+
+
+def test_get_mcp_store_returns_none_when_disabled():
+ """Storage returns None when MCP_STORE_CONFIG.enabled is False."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.storage import get_mcp_store
+
+ result = get_mcp_store(prefix="test_")
+
+ assert result is None
+
+
+def test_get_mcp_store_returns_none_when_no_redis_url():
+ """Storage returns None when CACHE_REDIS_URL is not configured."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_REDIS_URL": None,
+ "WRAPPER_TYPE": "key_value.aio.wrappers.prefix_keys.PrefixKeysWrapper",
+ }
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.storage import get_mcp_store
+
+ result = get_mcp_store(prefix="test_")
+
+ assert result is None
+
+
+def test_get_mcp_store_creates_store_when_enabled():
+ """Storage creates wrapped RedisStore when properly configured."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_REDIS_URL": "redis://localhost:6379/0",
+ "WRAPPER_TYPE": "key_value.aio.wrappers.prefix_keys.PrefixKeysWrapper",
+ }
Review Comment:
**Suggestion:** Same issue in the third test: assigning the dict as
config.get.return_value will return that dict for any key lookup; switch to a
side_effect so only "MCP_STORE_CONFIG" returns the mapping, preventing
accidental masking of other config lookups (e.g., direct CACHE_REDIS_URL
queries). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
_mcp_store_cfg = {
"enabled": True,
"CACHE_REDIS_URL": "redis://localhost:6379/0",
"WRAPPER_TYPE":
"key_value.aio.wrappers.prefix_keys.PrefixKeysWrapper",
}
def _config_get(key, default=None):
return _mcp_store_cfg if key == "MCP_STORE_CONFIG" else default
mock_flask_app.config.get.side_effect = _config_get
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Identical to the prior two — the change is correct and improves test
robustness. While not strictly required for the current implementation (which
only queries "MCP_STORE_CONFIG"), using side_effect avoids future regressions
and makes the test more precise.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_storage.py
**Line:** 64:68
**Comment:**
*Possible Bug: Same issue in the third test: assigning the dict as
config.get.return_value will return that dict for any key lookup; switch to a
side_effect so only "MCP_STORE_CONFIG" returns the mapping, preventing
accidental masking of other config lookups (e.g., direct CACHE_REDIS_URL
queries).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/app.py:
##########
@@ -23,9 +23,10 @@
"""
import logging
-from typing import Any, Callable, Dict, List, Set
+from typing import Any, Callable, Dict, List, Sequence, Set
from fastmcp import FastMCP
+from fastmcp.server.middleware import Middleware
Review Comment:
**Suggestion:** Unconditional import of `Middleware` at module import time
can raise ImportError or make the entire module fail to import in environments
where `fastmcp.server.middleware` is not available; guard the runtime import
(use TYPE_CHECKING or try/except and fall back to a no-op/type alias) because
`Middleware` is only needed for type annotations. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
from typing import Any, Callable, Dict, List, Sequence, Set, TYPE_CHECKING
if TYPE_CHECKING:
from fastmcp.server.middleware import Middleware # type: ignore
else:
Middleware = Any # fallback at runtime when the real class isn't
importable
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion is reasonable: Middleware is only used in type annotations,
so guarding the import prevents import-time failures in environments where
fastmcp.server.middleware isn't present. The proposed TYPE_CHECKING fallback is
a correct, low-risk way to keep runtime imports light and avoid hard dependency
failures. Note: the current code will fail at import time if fastmcp isn't
installed, so this fixes a real fragility.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/app.py
**Line:** 26:29
**Comment:**
*Possible Bug: Unconditional import of `Middleware` at module import
time can raise ImportError or make the entire module fail to import in
environments where `fastmcp.server.middleware` is not available; guard the
runtime import (use TYPE_CHECKING or try/except and fall back to a no-op/type
alias) because `Middleware` is only needed for type annotations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_caching.py:
##########
@@ -0,0 +1,143 @@
+# 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 MCP response caching middleware."""
+
+from unittest.mock import MagicMock, patch
+
+from superset.mcp_service.caching import _build_caching_settings
+
+
+def test_build_caching_settings_empty_config():
+ """Empty config returns empty settings."""
+ result = _build_caching_settings({})
+ assert result == {}
+
+
+def test_build_caching_settings_list_ttls():
+ """List operation TTLs are mapped to settings."""
+ config = {
+ "list_tools_ttl": 300,
+ "list_resources_ttl": 300,
+ "list_prompts_ttl": 300,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["list_tools_settings"] == {"ttl": 300}
+ assert result["list_resources_settings"] == {"ttl": 300}
+ assert result["list_prompts_settings"] == {"ttl": 300}
+
+
+def test_build_caching_settings_item_ttls():
+ """Individual item TTLs are mapped to settings."""
+ config = {
+ "read_resource_ttl": 3600,
+ "get_prompt_ttl": 3600,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["read_resource_settings"] == {"ttl": 3600}
+ assert result["get_prompt_settings"] == {"ttl": 3600}
+
+
+def test_build_caching_settings_call_tool_with_exclusions():
+ """Call tool settings include TTL and exclusions."""
+ config = {
+ "call_tool_ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+ result = _build_caching_settings(config)
+
+ assert result["call_tool_settings"] == {
+ "ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+
+
+def test_create_response_caching_middleware_returns_none_when_disabled():
+ """Caching middleware returns None when disabled in config."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.caching import
create_response_caching_middleware
+
+ result = create_response_caching_middleware()
+
+ assert result is None
+
+
+def test_create_response_caching_middleware_returns_none_when_no_prefix():
+ """Caching middleware returns None when CACHE_KEY_PREFIX is not set."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_KEY_PREFIX": None,
+ }
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.caching import
create_response_caching_middleware
+
+ result = create_response_caching_middleware()
+
+ assert result is None
+
+
+def test_create_response_caching_middleware_creates_middleware():
+ """Caching middleware is created when properly configured."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_KEY_PREFIX": "mcp_cache_v1_",
+ "list_tools_ttl": 300,
+ }
Review Comment:
**Suggestion:** Using `config.get.return_value` returns the same map for any
key and can mask bugs where the code queries the wrong config key; replace with
a `side_effect` that returns the intended test configuration only when
`"MCP_CACHE_CONFIG"` is requested, otherwise return the provided default.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
mock_flask_app.config.get.side_effect = lambda key, default=None: (
{
"enabled": True,
"CACHE_KEY_PREFIX": "mcp_cache_v1_",
"list_tools_ttl": 300,
}
if key == "MCP_CACHE_CONFIG"
else default
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The proposed side_effect makes the test more robust by returning the config
only when the code requests "MCP_CACHE_CONFIG". This prevents false positives
where the code might be querying a different key and the test still passes due
to the blanket return_value.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_caching.py
**Line:** 111:115
**Comment:**
*Possible Bug: Using `config.get.return_value` returns the same map for
any key and can mask bugs where the code queries the wrong config key; replace
with a `side_effect` that returns the intended test configuration only when
`"MCP_CACHE_CONFIG"` is requested, otherwise return the provided default.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/mcp_service/server.py:
##########
@@ -101,7 +102,16 @@ def run_server(
except Exception as e:
logging.error("Failed to create auth provider: %s", e)
- mcp_instance = init_fastmcp_server(auth=auth_provider)
+ # Build middleware list
+ middleware_list = []
+ caching_middleware = create_response_caching_middleware()
Review Comment:
**Suggestion:** Calling `create_response_caching_middleware()` directly
without checks can raise exceptions or produce an awaitable; an exception here
will crash startup and a coroutine result would be appended incorrectly. Call
the factory only if it's present and callable, catch initialization errors, and
skip coroutine results. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
caching_middleware = None
if callable(create_response_caching_middleware):
try:
result = create_response_caching_middleware()
import asyncio
if asyncio.iscoroutine(result):
logging.warning("create_response_caching_middleware
returned a coroutine; skipping caching middleware")
else:
caching_middleware = result
except Exception as e:
logging.error("Failed to initialize caching middleware: %s",
e)
else:
# If the factory isn't available (import failed), skip caching
caching_middleware = None
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real robustness issue: calling the factory unguarded can raise,
return a coroutine, or be absent.
The improved code defends against non-callable imports, initialization
exceptions, and coroutine returns,
avoiding a hard crash and preventing invalid middleware values from being
appended.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 107:107
**Comment:**
*Possible Bug: Calling `create_response_caching_middleware()` directly
without checks can raise exceptions or produce an awaitable; an exception here
will crash startup and a coroutine result would be appended incorrectly. Call
the factory only if it's present and callable, catch initialization errors, and
skip coroutine results.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_storage.py:
##########
@@ -0,0 +1,96 @@
+# 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 MCP storage factory."""
+
+from unittest.mock import MagicMock, patch
+
+
+def test_get_mcp_store_returns_none_when_disabled():
+ """Storage returns None when MCP_STORE_CONFIG.enabled is False."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.storage import get_mcp_store
+
+ result = get_mcp_store(prefix="test_")
+
+ assert result is None
+
+
+def test_get_mcp_store_returns_none_when_no_redis_url():
+ """Storage returns None when CACHE_REDIS_URL is not configured."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_REDIS_URL": None,
+ "WRAPPER_TYPE": "key_value.aio.wrappers.prefix_keys.PrefixKeysWrapper",
+ }
Review Comment:
**Suggestion:** The test sets config.get.return_value to a dict, which will
be returned for any config key; this can incorrectly satisfy calls that expect
`CACHE_REDIS_URL` to be queried directly. Replace with a side_effect that
returns the mapping only when the "MCP_STORE_CONFIG" key is requested,
otherwise return the default. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
_mcp_store_cfg = {
"enabled": True,
"CACHE_REDIS_URL": None,
"WRAPPER_TYPE":
"key_value.aio.wrappers.prefix_keys.PrefixKeysWrapper",
}
def _config_get(key, default=None):
return _mcp_store_cfg if key == "MCP_STORE_CONFIG" else default
mock_flask_app.config.get.side_effect = _config_get
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same rationale as above — replacing return_value with a side_effect keyed on
"MCP_STORE_CONFIG" is a sensible hardening of the tests. It prevents accidental
masking if production code were to call config.get with different keys and
makes the test intent explicit.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_storage.py
**Line:** 43:47
**Comment:**
*Possible Bug: The test sets config.get.return_value to a dict, which
will be returned for any config key; this can incorrectly satisfy calls that
expect `CACHE_REDIS_URL` to be queried directly. Replace with a side_effect
that returns the mapping only when the "MCP_STORE_CONFIG" key is requested,
otherwise return the default.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_storage.py:
##########
@@ -0,0 +1,96 @@
+# 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 MCP storage factory."""
+
+from unittest.mock import MagicMock, patch
+
+
+def test_get_mcp_store_returns_none_when_disabled():
+ """Storage returns None when MCP_STORE_CONFIG.enabled is False."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
Review Comment:
**Suggestion:** Using MagicMock.config.get.return_value unconditionally
makes the mock return the same object for any config key; if production code
calls config.get with keys other than "MCP_STORE_CONFIG" (or with different
argument signatures), the mock will return the wrong value and tests become
fragile/incorrect. Use a side_effect that only returns the MCP_STORE_CONFIG
mapping when the key is "MCP_STORE_CONFIG" and otherwise returns the provided
default. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
_mcp_store_cfg = {"enabled": False}
def _config_get(key, default=None):
return _mcp_store_cfg if key == "MCP_STORE_CONFIG" else default
mock_flask_app.config.get.side_effect = _config_get
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion is valid: using return_value for config.get will make the
mock return that dict for any key, which can hide incorrect calls to config.get
elsewhere. In this code path get_mcp_store calls config.get("MCP_STORE_CONFIG",
...), so the test as written passes today, but switching to a side_effect that
only returns the mapping when the key is "MCP_STORE_CONFIG" makes the test
stricter and less fragile.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_storage.py
**Line:** 26:26
**Comment:**
*Possible Bug: Using MagicMock.config.get.return_value unconditionally
makes the mock return the same object for any config key; if production code
calls config.get with keys other than "MCP_STORE_CONFIG" (or with different
argument signatures), the mock will return the wrong value and tests become
fragile/incorrect. Use a side_effect that only returns the MCP_STORE_CONFIG
mapping when the key is "MCP_STORE_CONFIG" and otherwise returns the provided
default.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_caching.py:
##########
@@ -0,0 +1,143 @@
+# 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 MCP response caching middleware."""
+
+from unittest.mock import MagicMock, patch
+
+from superset.mcp_service.caching import _build_caching_settings
+
+
+def test_build_caching_settings_empty_config():
+ """Empty config returns empty settings."""
+ result = _build_caching_settings({})
+ assert result == {}
+
+
+def test_build_caching_settings_list_ttls():
+ """List operation TTLs are mapped to settings."""
+ config = {
+ "list_tools_ttl": 300,
+ "list_resources_ttl": 300,
+ "list_prompts_ttl": 300,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["list_tools_settings"] == {"ttl": 300}
+ assert result["list_resources_settings"] == {"ttl": 300}
+ assert result["list_prompts_settings"] == {"ttl": 300}
+
+
+def test_build_caching_settings_item_ttls():
+ """Individual item TTLs are mapped to settings."""
+ config = {
+ "read_resource_ttl": 3600,
+ "get_prompt_ttl": 3600,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["read_resource_settings"] == {"ttl": 3600}
+ assert result["get_prompt_settings"] == {"ttl": 3600}
+
+
+def test_build_caching_settings_call_tool_with_exclusions():
+ """Call tool settings include TTL and exclusions."""
+ config = {
+ "call_tool_ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+ result = _build_caching_settings(config)
+
+ assert result["call_tool_settings"] == {
+ "ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+
+
+def test_create_response_caching_middleware_returns_none_when_disabled():
+ """Caching middleware returns None when disabled in config."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
Review Comment:
**Suggestion:** Mocking `flask_app.config.get` with a fixed `return_value`
ignores the key argument passed to `config.get` and will return the same dict
for any key; this can mask bugs where the code requests the wrong configuration
key (e.g. not "MCP_CACHE_CONFIG"). Use `side_effect` so the mock only returns
the test config when the expected key is requested and otherwise returns the
provided default. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
mock_flask_app.config.get.side_effect = lambda key, default=None: (
{"enabled": False} if key == "MCP_CACHE_CONFIG" else default
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current test uses config.get.return_value which will return the same
dict for any key and can hide a bug where the code queries the wrong config
key. Replacing this with a side_effect that only returns the test dict for
"MCP_CACHE_CONFIG" makes the test stricter and more accurate without changing
production code.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_caching.py
**Line:** 74:74
**Comment:**
*Possible Bug: Mocking `flask_app.config.get` with a fixed
`return_value` ignores the key argument passed to `config.get` and will return
the same dict for any key; this can mask bugs where the code requests the wrong
configuration key (e.g. not "MCP_CACHE_CONFIG"). Use `side_effect` so the mock
only returns the test config when the expected key is requested and otherwise
returns the provided default.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_caching.py:
##########
@@ -0,0 +1,143 @@
+# 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 MCP response caching middleware."""
+
+from unittest.mock import MagicMock, patch
+
+from superset.mcp_service.caching import _build_caching_settings
+
+
+def test_build_caching_settings_empty_config():
+ """Empty config returns empty settings."""
+ result = _build_caching_settings({})
+ assert result == {}
+
+
+def test_build_caching_settings_list_ttls():
+ """List operation TTLs are mapped to settings."""
+ config = {
+ "list_tools_ttl": 300,
+ "list_resources_ttl": 300,
+ "list_prompts_ttl": 300,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["list_tools_settings"] == {"ttl": 300}
+ assert result["list_resources_settings"] == {"ttl": 300}
+ assert result["list_prompts_settings"] == {"ttl": 300}
+
+
+def test_build_caching_settings_item_ttls():
+ """Individual item TTLs are mapped to settings."""
+ config = {
+ "read_resource_ttl": 3600,
+ "get_prompt_ttl": 3600,
+ }
+ result = _build_caching_settings(config)
+
+ assert result["read_resource_settings"] == {"ttl": 3600}
+ assert result["get_prompt_settings"] == {"ttl": 3600}
+
+
+def test_build_caching_settings_call_tool_with_exclusions():
+ """Call tool settings include TTL and exclusions."""
+ config = {
+ "call_tool_ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+ result = _build_caching_settings(config)
+
+ assert result["call_tool_settings"] == {
+ "ttl": 3600,
+ "excluded_tools": ["execute_sql", "generate_chart"],
+ }
+
+
+def test_create_response_caching_middleware_returns_none_when_disabled():
+ """Caching middleware returns None when disabled in config."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {"enabled": False}
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=mock_flask_app,
+ ):
+ with patch("flask.has_app_context", return_value=True):
+ from superset.mcp_service.caching import
create_response_caching_middleware
+
+ result = create_response_caching_middleware()
+
+ assert result is None
+
+
+def test_create_response_caching_middleware_returns_none_when_no_prefix():
+ """Caching middleware returns None when CACHE_KEY_PREFIX is not set."""
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = {
+ "enabled": True,
+ "CACHE_KEY_PREFIX": None,
+ }
Review Comment:
**Suggestion:** The test sets `mock_flask_app.config.get.return_value` to a
dict, which will be returned for any config key and may hide issues where the
code asks for a different key; change to `side_effect` that returns the dict
only when the key is `"MCP_CACHE_CONFIG"` and otherwise returns the default.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
mock_flask_app.config.get.side_effect = lambda key, default=None: (
{"enabled": True, "CACHE_KEY_PREFIX": None} if key ==
"MCP_CACHE_CONFIG" else default
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue: returning the dict for any requested config key can mask
mistakes. Using a side_effect that checks for "MCP_CACHE_CONFIG" ensures the
code actually asks for the expected key and preserves the default behavior for
other keys.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_mcp_caching.py
**Line:** 91:94
**Comment:**
*Possible Bug: The test sets `mock_flask_app.config.get.return_value`
to a dict, which will be returned for any config key and may hide issues where
the code asks for a different key; change to `side_effect` that returns the
dict only when the key is `"MCP_CACHE_CONFIG"` and otherwise returns the
default.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
