korbit-ai[bot] commented on code in PR #36038:
URL: https://github.com/apache/superset/pull/36038#discussion_r2501624149
##########
superset/mcp_service/__main__.py:
##########
@@ -123,7 +123,7 @@ def main() -> None:
if transport == "streamable-http":
host = os.environ.get("FASTMCP_HOST", "127.0.0.1")
port = int(os.environ.get("FASTMCP_PORT", "5008"))
- mcp.run(transport=transport, host=host, port=port)
+ mcp.run(transport=transport, host=host, port=port,
stateless_http=True)
Review Comment:
### Hardcoded stateless_http parameter without validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `stateless_http=True` parameter is hardcoded without validation that the
MCP library supports this parameter or understanding its implications for the
application's state management.
###### Why this matters
If the MCP library doesn't support the `stateless_http` parameter, this will
cause a runtime error. Additionally, enabling stateless HTTP mode may break
functionality that depends on maintaining session state or connection
persistence, potentially causing data loss or authentication issues.
###### Suggested change ∙ *Feature Preview*
Verify that the MCP library supports the `stateless_http` parameter and
consider making it configurable via environment variable:
```python
stateless = os.environ.get("FASTMCP_STATELESS_HTTP", "true").lower() ==
"true"
mcp.run(transport=transport, host=host, port=port, stateless_http=stateless)
```
Alternatively, wrap the call in a try-except block to handle unsupported
parameters gracefully.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e016c99-16ec-4e32-8896-a7b953c1618d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e016c99-16ec-4e32-8896-a7b953c1618d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e016c99-16ec-4e32-8896-a7b953c1618d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e016c99-16ec-4e32-8896-a7b953c1618d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e016c99-16ec-4e32-8896-a7b953c1618d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:15784762-53cb-4bbc-84e1-a7e831eee89a -->
[](15784762-53cb-4bbc-84e1-a7e831eee89a)
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -156,6 +163,31 @@ class DatasetInfo(BaseModel):
populate_by_name=True, # Allow both 'schema' (alias) and
'schema_name' (field)
)
+ @model_serializer(mode="wrap", when_used="json")
+ def _filter_fields_by_context(self, serializer: Any, info: Any) ->
Dict[str, Any]:
+ """Filter fields based on serialization context.
+
+ If context contains 'select_columns', only include those fields.
+ Otherwise, include all fields (default behavior).
+ """
+ # Get full serialization
+ data = serializer(self)
+
+ # Check if we have a context with select_columns
+ if info.context and isinstance(info.context, dict):
+ select_columns = info.context.get("select_columns")
+ if select_columns:
+ # Handle alias: 'schema' -> 'schema_name'
+ requested_fields = set(select_columns)
+ if "schema" in requested_fields:
+ requested_fields.add("schema_name")
Review Comment:
### Incomplete alias handling in field filtering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The alias handling logic only adds 'schema_name' when 'schema' is requested,
but doesn't handle the reverse case where 'schema_name' is requested but
'schema' should be included in the output.
###### Why this matters
When a user requests 'schema_name' directly, the serialized output will use
the alias 'schema' (due to the field alias configuration), but the filtering
logic won't recognize 'schema' as a valid field to include, potentially causing
the field to be filtered out incorrectly.
###### Suggested change ∙ *Feature Preview*
Handle both directions of the alias mapping:
```python
# Handle alias: 'schema' -> 'schema_name' and vice versa
requested_fields = set(select_columns)
if "schema" in requested_fields:
requested_fields.add("schema_name")
if "schema_name" in requested_fields:
requested_fields.add("schema")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee666f8-c411-4b89-bdfb-06bb60217d0b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee666f8-c411-4b89-bdfb-06bb60217d0b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee666f8-c411-4b89-bdfb-06bb60217d0b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee666f8-c411-4b89-bdfb-06bb60217d0b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bee666f8-c411-4b89-bdfb-06bb60217d0b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0cb2971-9e8b-4e9b-b10a-393237b33a23 -->
[](b0cb2971-9e8b-4e9b-b10a-393237b33a23)
##########
superset/mcp_service/app.py:
##########
@@ -236,7 +236,7 @@ def create_mcp_app(
# Create default MCP instance for backward compatibility
# Tool modules can import this and use @mcp.tool decorators
-mcp = create_mcp_app(stateless_http=True)
+mcp = create_mcp_app()
Review Comment:
### Global Mutable State Anti-pattern <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a global mutable state through a module-level MCP instance,
which can lead to issues with testing, multiple configurations, and dependency
management.
###### Why this matters
Global mutable state makes the code harder to test, can cause unexpected
side effects when multiple parts of the application modify it, and violates
dependency injection principles. This makes it difficult to have different
configurations for different environments or tests.
###### Suggested change ∙ *Feature Preview*
Instead of using a global MCP instance, consider using dependency injection:
```python
class SupersetMCP:
def __init__(self):
self.mcp = create_mcp_app()
def get_mcp(self):
return self.mcp
# Usage
superset_mcp = SupersetMCP()
mcp_instance = superset_mcp.get_mcp()
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8578b676-9c04-4044-a581-76ea1246fc05/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8578b676-9c04-4044-a581-76ea1246fc05?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8578b676-9c04-4044-a581-76ea1246fc05?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8578b676-9c04-4044-a581-76ea1246fc05?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8578b676-9c04-4044-a581-76ea1246fc05)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cb8bc41f-8e6d-4922-936a-412790840da7 -->
[](cb8bc41f-8e6d-4922-936a-412790840da7)
--
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]
