codeant-ai-for-open-source[bot] commented on PR #36497: URL: https://github.com/apache/superset/pull/36497#issuecomment-3639370454
## 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/36497/files#diff-420249c5b9da86711c69396a53af4d19834ba519dc9b027433aea3269121acf1R266-R290'><strong>Signature removal may miss string-annotated ctx</strong></a><br>The code removes the `ctx` parameter by checking the parameter annotation against the concrete `FMContext` type or annotation `.__name__ == "Context"`. If the original function used a string forward reference (e.g. "Context") or other PEP 563-style annotations, this check will not match and `ctx` may remain in the signature/annotations, causing mismatches between signature and type hints (breaks Pydantic TypeAdapter/validation).<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-98674b133f5e4b72d13c28ae67fe0baa4eb555288135814aa24645da9eaaf7d9R34-R86'><strong>Syntax Compatibility</strong></a><br>The file uses PEP 604 union types (e.g. `str | Callable[[], str]`) which require Python 3.10+. Verify the project's supported Python versions; using this syntax on older runtimes will cause a SyntaxError at import time.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-0b1268c08fa98c96adafc6abfc3d24dcf863c08e1959c49410c48bc78e74ae9dR26-R31'><strong>Fragile internal import</strong></a><br>The code now imports `Middleware` from `fastmcp.server.middleware`. That path may be internal and could change between FastMCP versions; importing it at module import time will raise ImportError for incompatible versions and break runtime import of this module. Consider making the import optional or guarded so non-compatible environments fail more gracefully.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-98674b133f5e4b72d13c28ae67fe0baa4eb555288135814aa24645da9eaaf7d9R102-R119'><strong>Async Store Usage</strong></a><br>The code imports and instantiates `RedisStore` from `key_value.aio.stores.redis` (an "aio" package). That package is likely designed for async usage; creating and using an async-backed store in synchronous code can lead to unexpected behavior or missing event loop requirements. Confirm the store is usable synchronously or provide an adapter/factory that works in the sync context.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R87-R87'><strong>Import side-effects</strong></a><br>Importing `create_response_caching_middleware` at module/runtime scope before the Flask app is retrieved can trigger import-time side effects (dependency initialization, config access, redis connections, etc.). This may cause surprising failures or startup ordering issues. Consider delaying the import or making it resilient to failures.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R105-R114'><strong>Unhandled middleware errors</strong></a><br>The call to `create_response_caching_middleware()` is not protected by error handling. If it raises or returns an unexpected value, server startup could fail or an invalid middleware could be passed to `init_fastmcp_server`. Wrap construction in try/except and validate the returned middleware before appending.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-90302e46f36693a533e42092f84959ccdd142cbe9af4f07968075a1976b9dbffR122-R130'><strong>Middleware init exceptions</strong></a><br>ResponseCachingMiddleware is constructed directly. If the provided store or settings are incompatible (wrong interface, unexpected types), instantiation may raise exceptions which are not caught. This can break application startup.<br> - [ ] <a href='https://github.com/apache/superset/pull/36497/files#diff-def9087bc36aa52d2e008783f442cf3f267a098846a4828b5cb0d0508357ca75R434-R454'><strong>Missing defensive import handling</strong></a><br>The decorator imports and calls FastMCP's get_context (from fastmcp.server.dependencies) without fallback or explanatory error handling. If FastMCP isn't installed or the import fails at runtime, this will raise an ImportError at call time with a less-helpful stacktrace. Wrap the import/call in a try/except to surface a clear error and avoid crashing in environments where the feature is opt-in.<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]
