aminghadersohi commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3482255754
##########
superset/mcp_service/flask_singleton.py:
##########
@@ -51,6 +51,16 @@
logger.info("Reusing existing Flask app from app context for MCP
service")
# Use _get_current_object() to get the actual Flask app, not the
LocalProxy
app = current_app._get_current_object()
+
+ # Configure the chart plugin registry from the host app's config.
+ # This module is the registry's only configure site — core Superset
+ # startup must not import mcp_service (fastmcp is an optional extra).
+ from superset.mcp_service.chart import registry as _chart_registry
+
+ _chart_registry.configure(
+ disabled=app.config.get("MCP_DISABLED_CHART_PLUGINS"),
+ enabled_func=app.config.get("MCP_CHART_PLUGIN_ENABLED_FUNC"),
+ )
Review Comment:
The concern is addressed by the import chain. `server.py` imports
`create_mcp_app` from `app.py` at module-load time (line 34). Loading `app.py`
executes `mcp = create_mcp_app()` at module scope, which in turn does `from
superset.mcp_service.flask_singleton import app as flask_app` — triggering the
`flask_singleton` module and its `_chart_registry.configure(...)` call. This
happens before `run_server()` is ever called, so both the
`use_factory_config=True` and `use_factory_config=False` paths share the same
already-configured registry.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1517,25 +1555,29 @@ def reject_sql_expression_in_raw_mode(self) ->
"TableChartConfig":
@model_validator(mode="after")
def validate_unique_column_labels(self) -> "TableChartConfig":
"""Ensure all column labels are unique."""
- labels_seen = set()
+ # Key is (saved_metric, label) so a saved metric and a regular column
+ # with the same input name are not flagged as duplicates — saved
metrics
+ # resolve to their actual casing from the dataset during normalization.
+ labels_seen: dict[tuple[bool, str], str] = {}
duplicates = []
for i, col in enumerate(self.columns):
# Generate the label that will be used (same logic as
create_metric_object)
if col.sql_expression:
# SQL metrics carry a required label; use it verbatim.
- label = col.label
+ label = col.label or ""
elif col.saved_metric:
- label = col.label or col.name
+ label = col.label or col.name or ""
elif col.aggregate:
label = col.label or f"{col.aggregate}({col.name})"
else:
- label = col.label or col.name
+ label = col.label or col.name or ""
- if label in labels_seen:
+ key = (col.saved_metric, label)
+ if key in labels_seen:
duplicates.append(f"columns[{i}]: '{label}'")
else:
- labels_seen.add(label)
+ labels_seen[key] = f"columns[{i}]"
Review Comment:
The `(saved_metric, label)` key is intentional. A saved metric named
`Revenue` and an ad-hoc `SUM(revenue)` that the user also labels `Revenue` are
different backend objects and may resolve to different canonical names after
normalization (saved metrics get their casing from the dataset via
`_get_canonical_metric_name`). Keying by bare `label` would falsely reject
valid payloads where the user supplies the same string for both but the values
diverge post-normalization. The comment above the key explains this: 'saved
metrics resolve to their actual casing from the dataset during normalization.'
If two entries genuinely produce the same display label after normalization,
Superset's chart query layer surfaces a duplicate-label error at query time.
--
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]
