codeant-ai-for-open-source[bot] commented on PR #36527: URL: https://github.com/apache/superset/pull/36527#issuecomment-3641476508
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-78fca901911c0231ebaef0eb1a80278be62c40ef8fcc3ef0f19ca92efc18b58cR137-R156'><strong>Missing cache prefix enforcement</strong></a><br>If consumers enable MCP_STORE_CONFIG (Redis) but leave MCP_CACHE_CONFIG["CACHE_KEY_PREFIX"] as None, different features using the same Redis instance can conflict. The config currently allows a None prefix which risks key collisions or unexpected shared state. The code that creates/uses the store should validate or derive a safe default prefix when the store is enabled.<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-78fca901911c0231ebaef0eb1a80278be62c40ef8fcc3ef0f19ca92efc18b58cR130-R134'><strong>Store enabled without Redis URL</strong></a><br>The new config permits enabling the shared store via MCP_STORE_CONFIG["enabled"]. If a user enables the store but does not set "CACHE_REDIS_URL", the runtime code that instantiates a Redis client may fail. There should be a validation/warning and a clear fallback (e.g., refuse to enable or fall back to in-memory).<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-90302e46f36693a533e42092f84959ccdd142cbe9af4f07968075a1976b9dbffR95-R125'><strong>Possible AttributeError</strong></a><br>The code reads nested config objects via variables like `store_config = flask_app.config.get("MCP_STORE_CONFIG", {})` and later calls `store_config.get("enabled", False)`. If the config key exists but its value is `None` this will assign `None` to `store_config` and the subsequent `.get` call will raise an AttributeError. Consider defensive handling for config values that are present but falsy/None.<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-78fca901911c0231ebaef0eb1a80278be62c40ef8fcc3ef0f19ca92efc18b58cR133-R135'><strong>Wrapper type import risk</strong></a><br>The WRAPPER_TYPE string points to an import path for a wrapper class. If that path is incorrect or the package is not installed, runtime import will fail. Ensure code that uses WRAPPER_TYPE attempts to import it with a clear error message and fallbacks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-aac02cd191ffc37e46facc40ed73184793b484f04a5f6ad736ea4975503884a2R116-R120'><strong>Missing settings assertions</strong></a><br>The new tests assert that middleware is created and that `cache_storage` is `None` when falling back to in-memory store, but they do not assert that the per-operation settings (e.g., `list_tools_settings`) are passed through to the middleware. This leaves a gap where caching settings might be ignored while tests still pass.<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-aac02cd191ffc37e46facc40ed73184793b484f04a5f6ad736ea4975503884a2R105-R105'><strong>App context path untested</strong></a><br>All new tests patch `flask.has_app_context` to return `True`. The branch where no app context exists (so the code pushes one via `flask_app.app_context()`) is not exercised by these new tests. The context-push path could behave differently and should be covered.<br> - [ ] <a href='https://github.com/apache/superset/pull/36527/files#diff-aac02cd191ffc37e46facc40ed73184793b484f04a5f6ad736ea4975503884a2R91-R97'><strong>Brittle config mocking</strong></a><br>The tests configure `mock_flask_app.config.get` with a `side_effect` that looks up keys in `mock_configs`. This is fine, but it relies on the signature `get(key, default=None)` and returning `default` when key missing. If production code ever uses different keys or extra calls (e.g., without default), the tests may behave differently than a real Flask config dict. Consider using a dict-like object or setting `mock_flask_app.config` to a real dict to more closely mimic Flask behavior.<br> </td></tr> </table> -- 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]
