codeant-ai-for-open-source[bot] commented on code in PR #36527:
URL: https://github.com/apache/superset/pull/36527#discussion_r2610223594
##########
superset/mcp_service/caching.py:
##########
@@ -95,22 +96,12 @@ def create_response_caching_middleware() -> Any | None:
def _create_middleware() -> Any | None:
cache_config = flask_app.config.get("MCP_CACHE_CONFIG", {})
+ store_config = flask_app.config.get("MCP_STORE_CONFIG", {})
Review Comment:
**Suggestion:** The PR added `store_config =
flask_app.config.get("MCP_STORE_CONFIG", {})` but if the app config explicitly
sets `MCP_STORE_CONFIG` to None (or a non-dict), subsequent calls like
`store_config.get(...)` will raise an AttributeError; normalize the value
(e.g., use `or {}`) so it's always a dict. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
store_config = flask_app.config.get("MCP_STORE_CONFIG") or {}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good catch — if the config key exists but is set to None (or some non-dict),
store_config.get(...) will raise. Normalizing with `or {}` prevents an
AttributeError and is a small, safe hardening change.
</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:** 99:99
**Comment:**
*Possible Bug: The PR added `store_config =
flask_app.config.get("MCP_STORE_CONFIG", {})` but if the app config explicitly
sets `MCP_STORE_CONFIG` to None (or a non-dict), subsequent calls like
`store_config.get(...)` will raise an AttributeError; normalize the value
(e.g., use `or {}`) so it's always a dict.
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:
##########
@@ -119,9 +110,23 @@ def _create_middleware() -> Any | None:
)
return None
+ # Determine which store to use
+ store = None
+ if store_config.get("enabled", False):
+ # Redis store requires a prefix
+ cache_prefix = cache_config.get("CACHE_KEY_PREFIX")
+ if not cache_prefix:
+ logger.warning(
+ "MCP_STORE_CONFIG enabled but no CACHE_KEY_PREFIX
configured - "
+ "falling back to in-memory store"
+ )
+ else:
+ store = get_mcp_store(prefix=cache_prefix)
Review Comment:
**Suggestion:** The code calls `get_mcp_store(prefix=cache_prefix)` without
validating the runtime type of `cache_prefix`; if an invalid type (e.g., dict)
is configured this may raise errors inside the store factory. Validate that
`cache_prefix` is a string or callable before passing it to `get_mcp_store` and
log + fall back to in-memory store otherwise. [type error]
**Severity Level:** Minor ⚠️
```suggestion
if isinstance(cache_prefix, str) or callable(cache_prefix):
store = get_mcp_store(prefix=cache_prefix)
else:
logger.warning(
"MCP_STORE_CONFIG has invalid CACHE_KEY_PREFIX type;
falling back to in-memory store"
)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Reasonable defensive check: validating that cache_prefix is a str or
callable before passing it into the store factory prevents obscure runtime
errors from misconfigured values and lets us log a clearer warning and fall
back safely.
</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:** 124:124
**Comment:**
*Type Error: The code calls `get_mcp_store(prefix=cache_prefix)`
without validating the runtime type of `cache_prefix`; if an invalid type
(e.g., dict) is configured this may raise errors inside the store factory.
Validate that `cache_prefix` is a string or callable before passing it to
`get_mcp_store` and log + fall back to in-memory store otherwise.
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:
##########
@@ -85,24 +85,74 @@ def
test_create_response_caching_middleware_returns_none_when_disabled():
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."""
+def
test_create_response_caching_middleware_falls_back_to_memory_when_no_prefix():
+ """Caching middleware uses in-memory store when CACHE_KEY_PREFIX is not
set."""
mock_flask_app = MagicMock()
- mock_flask_app.config.get.return_value = {
- "enabled": True,
- "CACHE_KEY_PREFIX": None,
+ mock_configs = {
+ "MCP_CACHE_CONFIG": {"enabled": True, "list_tools_ttl": 300},
+ "MCP_STORE_CONFIG": {"enabled": True}, # Store enabled but no
CACHE_KEY_PREFIX
}
+ mock_flask_app.config.get.side_effect = lambda key, default=None:
mock_configs.get(
+ key, default
+ )
+
+ mock_middleware = MagicMock()
Review Comment:
**Suggestion:** Fragile mocking of Flask app config: the tests set
`mock_flask_app.config.get.side_effect = lambda ...` which relies on `config`
being a MagicMock attribute; using a plain dict for `config` (assigning
`mock_flask_app.config = mock_configs`) is simpler and more faithful to how
Flask's `app.config` behaves and avoids accidental MagicMock call semantics
interfering with `get` arguments. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Use a plain dict for config so .get behaves like Flask's config mapping
mock_flask_app.config = mock_configs
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct and practical. Assigning a plain dict to app.config more closely
mirrors Flask's API (dict.get) and avoids surprising MagicMock behavior.
It improves test realism 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:** 100:100
**Comment:**
*Possible Bug: Fragile mocking of Flask app config: the tests set
`mock_flask_app.config.get.side_effect = lambda ...` which relies on `config`
being a MagicMock attribute; using a plain dict for `config` (assigning
`mock_flask_app.config = mock_configs`) is simpler and more faithful to how
Flask's `app.config` behaves and avoids accidental MagicMock call semantics
interfering with `get` arguments.
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]
