korbit-ai[bot] commented on code in PR #36078:
URL: https://github.com/apache/superset/pull/36078#discussion_r2516498381
##########
superset/mcp_service/server.py:
##########
@@ -82,9 +82,20 @@ def run_server(
factory_config = get_mcp_factory_config()
mcp_instance = create_mcp_app(**factory_config)
else:
- # Use default initialization
+ # Use default initialization with response caching middleware
logging.info("Creating MCP app with default configuration...")
- mcp_instance = init_fastmcp_server()
+
+ # Import here to avoid circular imports
+ from superset.mcp_service.mcp_config import
create_response_caching_middleware
+
+ # Create middleware list - response caching is enabled by default
+ middleware_list = []
+ caching_middleware = create_response_caching_middleware()
+ if caching_middleware:
+ middleware_list.append(caching_middleware)
+
+ # Create MCP instance with middleware
+ mcp_instance = create_mcp_app(middleware=middleware_list or None)
Review Comment:
### Ambiguous middleware parameter handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The middleware parameter is passed as `None` when middleware_list is empty,
but this may not be the intended behavior for the create_mcp_app function.
###### Why this matters
If create_mcp_app expects an empty list instead of None for no middleware,
this could cause unexpected behavior or errors. The function signature and
expected parameter types are not visible, making this a potential runtime issue.
###### Suggested change ∙ *Feature Preview*
Either pass an empty list consistently or verify that create_mcp_app
properly handles None values for middleware:
```python
# Option 1: Always pass a list
mcp_instance = create_mcp_app(middleware=middleware_list)
# Option 2: Only pass middleware if not empty
if middleware_list:
mcp_instance = create_mcp_app(middleware=middleware_list)
else:
mcp_instance = create_mcp_app()
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac0807a6-8327-494f-af97-59f00a94b28b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac0807a6-8327-494f-af97-59f00a94b28b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac0807a6-8327-494f-af97-59f00a94b28b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac0807a6-8327-494f-af97-59f00a94b28b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ac0807a6-8327-494f-af97-59f00a94b28b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d9cb6a44-4798-4dab-8da0-3d29a57a306b -->
[](d9cb6a44-4798-4dab-8da0-3d29a57a306b)
##########
superset/mcp_service/server.py:
##########
@@ -82,9 +82,20 @@ def run_server(
factory_config = get_mcp_factory_config()
mcp_instance = create_mcp_app(**factory_config)
else:
- # Use default initialization
+ # Use default initialization with response caching middleware
logging.info("Creating MCP app with default configuration...")
- mcp_instance = init_fastmcp_server()
+
+ # Import here to avoid circular imports
+ from superset.mcp_service.mcp_config import
create_response_caching_middleware
+
+ # Create middleware list - response caching is enabled by default
+ middleware_list = []
+ caching_middleware = create_response_caching_middleware()
+ if caching_middleware:
+ middleware_list.append(caching_middleware)
Review Comment:
### Inefficient middleware creation on startup <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The caching middleware is created and imported on every server startup in
the hot path, even when it might not be needed or when the server is restarted
frequently.
###### Why this matters
This causes unnecessary overhead during server initialization and repeated
imports/function calls that could be avoided through lazy loading or
module-level caching.
###### Suggested change ∙ *Feature Preview*
Move the middleware creation to module level or implement lazy
initialization. Consider caching the middleware instance or only creating it
when actually needed:
```python
# At module level
_cached_middleware = None
def get_cached_middleware():
global _cached_middleware
if _cached_middleware is None:
from superset.mcp_service.mcp_config import
create_response_caching_middleware
_cached_middleware = create_response_caching_middleware()
return _cached_middleware
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8e40839-e76e-44cc-bd31-bb5ec1f213e5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8e40839-e76e-44cc-bd31-bb5ec1f213e5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8e40839-e76e-44cc-bd31-bb5ec1f213e5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8e40839-e76e-44cc-bd31-bb5ec1f213e5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b8e40839-e76e-44cc-bd31-bb5ec1f213e5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f76571bb-f86b-4cbc-81fc-dd0799744f93 -->
[](f76571bb-f86b-4cbc-81fc-dd0799744f93)
##########
superset/mcp_service/server.py:
##########
@@ -82,9 +82,20 @@ def run_server(
factory_config = get_mcp_factory_config()
mcp_instance = create_mcp_app(**factory_config)
else:
- # Use default initialization
+ # Use default initialization with response caching middleware
logging.info("Creating MCP app with default configuration...")
- mcp_instance = init_fastmcp_server()
+
+ # Import here to avoid circular imports
+ from superset.mcp_service.mcp_config import
create_response_caching_middleware
+
+ # Create middleware list - response caching is enabled by default
+ middleware_list = []
+ caching_middleware = create_response_caching_middleware()
+ if caching_middleware:
+ middleware_list.append(caching_middleware)
Review Comment:
### Missing error handling for middleware creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes create_response_caching_middleware() can return None or a
falsy value, but there's no error handling if the function fails or raises an
exception.
###### Why this matters
If create_response_caching_middleware() raises an exception, the server
startup will fail. Additionally, if it returns an unexpected type that
evaluates to False but isn't None, the middleware won't be added when it should
be.
###### Suggested change ∙ *Feature Preview*
Add proper error handling and explicit None checking:
```python
try:
caching_middleware = create_response_caching_middleware()
if caching_middleware is not None:
middleware_list.append(caching_middleware)
except Exception as e:
logging.warning("Failed to create response caching middleware: %s", e)
# Continue without caching middleware
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6e02b8d-432d-4e2c-b82c-8568f4b1aae1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6e02b8d-432d-4e2c-b82c-8568f4b1aae1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6e02b8d-432d-4e2c-b82c-8568f4b1aae1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6e02b8d-432d-4e2c-b82c-8568f4b1aae1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6e02b8d-432d-4e2c-b82c-8568f4b1aae1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:80f7ff3a-8aed-4582-9c92-b9062b6c3246 -->
[](80f7ff3a-8aed-4582-9c92-b9062b6c3246)
--
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]
