sadpandajoe commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4129641723
##########
superset/mcp_service/chart/plugins/gauge.py:
##########
@@ -44,6 +49,12 @@ class GaugeChartPlugin(BaseChartPlugin):
native_viz_types: ClassVar[Mapping[str, str]] = {
"gauge_chart": "Gauge Chart",
}
+ requires_compile_check = True
+ strict_dataset_rebind = True
Review Comment:
Gauge implements its own `merge_update_form_data` (returning
`merge_gauge_update_form_data`) but doesn't set `owns_update_merge = True` the
way Gantt/Treemap do. Both update tools therefore still run the generic
same-viz preservation pass afterward, which restores any key Gauge's own merge
just removed -- because that pass only checks whether a key is present in the
merged output, not whether it was intentionally cleared. So explicitly setting
`time_range` or `granularity_sqla` to `null` to clear a saved Gauge threshold
binding doesn't stick. Should `owns_update_merge = True` be set here?
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -897,7 +975,6 @@ async def update_chart( # noqa: C901
if validation_config is not None and effective_norm_dataset_id is not
None:
from superset.mcp_service.chart.validation.dataset_validator
import (
DatasetValidator,
Review Comment:
This import list no longer includes `NORMALIZATION_EXCEPTIONS` -- and after
checking, it now has no remaining callers anywhere in
`superset/mcp_service/chart/` (the sibling `update_chart_preview.py` switched
to inline exception tuples the same way). Its definition and comment in
`dataset_validator.py` still describe it as shared by the validation pipeline
and tools, which could mislead a future change to normalization exception
handling. Could that dead constant and its comment be removed?
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -792,11 +833,21 @@ async def execute_chart_data( # noqa: C901
command.validate()
result = command.run()
- if form_data.get("viz_type") == "treemap_v2":
+ if _normalizes_data_results(form_data):
result = normalize_chart_query_result(result, form_data)
if isinstance(result, ChartError):
return result
- if query_failure := query_result_failure(result):
+ data_plugin = _data_plugin(chart_viz_type)
Review Comment:
`data_plugin` is resolved from the saved chart's `chart_viz_type` here, not
from `effective_form_data`'s viz_type. When a `form_data_key` override supplies
cached preview data for a different chart type (for example, previewing a type
change to Bullet on a saved chart of another type), `allows_empty_result` and
the row-shape flags below are taken from the old saved type's plugin instead of
the type actually being queried, so a legitimately empty result can be rejected
as `NoData` under the wrong contract. Should this resolve the plugin from the
effective form data's `viz_type` instead?
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -85,6 +98,10 @@ async def generate_chart( # noqa: C901
- Set save_chart=True to permanently save the chart
- LLM clients MUST display returned chart URL to users
- Use numeric dataset ID or UUID (NOT schema.table_name format)
+ - MUST include chart_type in config (one of: 'xy', 'table', 'pie',
'bullet',
Review Comment:
This adds a second "MUST include chart_type" list that contradicts the
existing one a few lines below: it says `'gauge_chart'` where the schema's
discriminator is `'gauge'` (`GaugeChartConfig.chart_type: Literal["gauge"]`),
and it's missing `'treemap_v2'`, `'bubble_v2'`, and `'gantt'`, which the
existing list already has. A client following this list for Gauge would submit
a value the schema rejects. Could `'bullet'` be added to the existing correct
list below instead, and this duplicate list removed?
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -72,6 +78,13 @@
logger = logging.getLogger(__name__)
+def _bounded_update_response(payload: object) -> GenerateChartResponse:
Review Comment:
`_bounded_update_response` never forwards a `persisted_chart_id` to
`preflight_generate_chart_response`, unlike `generate_chart`'s equivalent
wrapper, which passes `chart_id if request.save_chart else None`. So when an
update's combined form data/previews exceed the response-size limit after the
chart was already persisted, the response falls back to `chart: null, success:
false` with nothing to indicate the update actually saved -- a client can't
tell this apart from a failed update and may retry unnecessarily. Should this
pass the chart's id through the same way `generate_chart` does?
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -581,6 +614,12 @@ async def execute_chart_data( # noqa: C901
# The query_context contains all the information needed to
reproduce
# the chart's data exactly as shown in the visualization
query_context_json = None
+ form_data: dict[str, Any] = {}
+ if chart_params:
+ parsed_form_data = utils_json.loads(chart_params)
Review Comment:
This `utils_json.loads(chart_params)` isn't wrapped in a try/except, unlike
the identical parse a few lines below it. A chart with malformed legacy
`params` (e.g. `"{"`) now raises here instead of falling through to that
guarded fallback, even when the chart has a valid saved `query_context` that
would otherwise satisfy the request. Could this parse reuse the same `except
(TypeError, ValueError)` guard as the block below?
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2656,6 +2740,426 @@ def validate_unique_column_labels(self) ->
"XYChartConfig":
return self
+class BulletChartConfig(BaseChartConfig):
+ """Config for bullet charts (viz_type ``bullet``)."""
+
+ # Semantic field names are exposed to MCP clients; validation aliases and
the
+ # native adapter accept saved Explore ``form_data`` without weakening the
+ # unknown-field checks that catch misspelled controls.
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["bullet"] = "bullet"
+ metric: ColumnRef = Field(
+ ...,
+ description="Numeric measure shown by each bullet bar",
+ )
+ dimensions: List[ColumnRef] | None = Field(
+ None,
+ validation_alias=AliasChoices("dimensions", "groupby"),
+ description=(
+ "Category hierarchy; one bullet row per unique combination. Omit
to "
+ "keep the saved hierarchy on update; [] clears it."
+ ),
+ max_length=20,
+ )
+ filters: List[FilterConfig] | None = Field(None, max_length=100)
+ time_range: str | None = Field(
+ None,
+ min_length=1,
+ max_length=1000,
+ description=(
+ "Superset time range, e.g. 'Last 30 days' or '2025-01-01 :
2025-12-31'"
+ ),
+ )
+ row_limit: int = Field(
+ 10000,
+ ge=1,
+ le=50000,
+ description="Maximum bullet rows",
+ )
+ order_by: List[SortByConfig] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("order_by", "orderby", "order_by_cols"),
+ max_length=20,
+ description="Row order by a dimension name or the metric output
label/name",
+ )
+
+ # Presentation fields map one-for-one onto Bullet/transformProps.ts
controls.
+ ranges: List[float] = Field(
+ default_factory=list,
+ max_length=100,
+ description="Qualitative range thresholds shaded behind the measure",
+ )
+ range_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("range_labels", "rangeLabels"),
+ max_length=100,
+ )
+ markers: List[float] = Field(
+ default_factory=list,
+ max_length=100,
+ description="Target values drawn as point markers",
+ )
+ marker_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_labels", "markerLabels"),
+ max_length=100,
+ )
+ marker_lines: List[float] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_lines", "markerLines"),
+ max_length=100,
+ description="Reference values drawn as vertical lines",
+ )
+ marker_line_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_line_labels",
"markerLineLabels"),
+ max_length=100,
+ )
+ y_axis_format: str = Field(
+ "SMART_NUMBER",
+ validation_alias=AliasChoices("y_axis_format", "yAxisFormat"),
+ max_length=100,
+ )
+ show_labels: bool = Field(
+ False,
+ validation_alias=AliasChoices("show_labels", "showLabels"),
+ )
+ show_legend: bool = Field(
+ False,
+ validation_alias=AliasChoices("show_legend", "showLegend"),
+ )
+
+ @staticmethod
+ def _adapt_native_metric(value: Any) -> Any:
+ """Translate QueryFormMetric shapes into the shared ColumnRef
contract."""
+ if isinstance(value, str):
+ return {"name": value, "saved_metric": True}
+ if not isinstance(value, dict):
+ return value
+ if "expressionType" not in value:
+ # QueryObject's documented legacy saved-metric representation is a
+ # label-only object. Keep this adapter deliberately narrow: objects
+ # carrying ad-hoc fields must declare expressionType explicitly,
and
+ # semantic ColumnRef objects continue through normal validation.
+ if set(value) == {"label"}:
+ label = value["label"]
+ if not isinstance(label, str) or not label or len(label) > 255:
+ raise ValueError(
+ "legacy saved metric label must be a non-empty string
of "
+ "at most 255 characters"
+ )
+ return {"name": label, "saved_metric": True}
+ return value
+ expression_type = value.get("expressionType")
+ if expression_type == "SQL":
+ return {
+ "sql_expression": value.get("sqlExpression"),
+ "label": value.get("label"),
+ }
+ if expression_type != "SIMPLE":
+ raise ValueError("metric.expressionType must be 'SIMPLE' or 'SQL'")
+ column = value.get("column")
+ if isinstance(column, dict):
+ name = column.get("column_name")
+ else:
+ name = column
+ return {
+ "name": name,
+ "aggregate": value.get("aggregate"),
+ "label": value.get("label"),
+ }
+
+ @staticmethod
+ def _canonical_dimension_alias(value: Any, field_name: str) -> list[str]:
+ """Canonicalize semantic/native dimension aliases for conflict
checks."""
+ if not isinstance(value, list):
+ raise ValueError(f"{field_name} must be an array")
+ canonical: list[str] = []
+ for index, item in enumerate(value):
+ name: str | None
+ if isinstance(item, str):
+ name = item
+ elif isinstance(item, ColumnRef):
+ name = item.name
+ elif isinstance(item, dict):
+ name = next(
+ (
+ item[key]
+ for key in ("name", "column_name", "column")
+ if isinstance(item.get(key), str)
+ ),
+ None,
+ )
+ else:
+ name = None
+ if not name:
+ raise ValueError(
+ f"{field_name}[{index}] must identify a physical column"
+ )
+ canonical.append(name)
+ return canonical
+
+ @staticmethod
+ def _adapt_native_order_by(value: Any) -> Any: # noqa: C901
+ if value is None:
+ return []
+ if not isinstance(value, list):
+ raise ValueError("order_by must be an array")
+ result: list[Any] = []
+ for index, entry in enumerate(value):
+ if isinstance(entry, str):
+ if len(entry) > 2000:
+ raise ValueError(f"order_by[{index}] is too long")
+ try:
+ entry = json.loads(entry)
+ except json.JSONDecodeError:
+ # A bare output/column name is the ergonomic typed form.
+ result.append({"column": entry, "ascending": False})
+ continue
+ if isinstance(entry, dict):
+ result.append(entry)
+ continue
+ if not isinstance(entry, (list, tuple)) or len(entry) != 2:
+ raise ValueError(
+ f"order_by[{index}] must be [column, ascending_boolean]"
+ )
+ target, ascending = entry
+ if isinstance(target, dict):
+ target = target.get("label") or target.get("metric_name")
+ if not isinstance(target, str) or not target:
+ raise ValueError(f"order_by[{index}] needs a column or metric
label")
+ if not isinstance(ascending, bool):
+ raise ValueError(f"order_by[{index}] ascending value must be
boolean")
+ result.append({"column": target, "ascending": ascending})
+ return result
+
+ @staticmethod
+ def _adapt_native_filters(data: dict[str, Any]) -> None: # noqa: C901
+ if "adhoc_filters" not in data:
+ return
+ if "filters" in data:
+ raise ValueError("Use either filters or native adhoc_filters, not
both")
+ raw_filters = data.pop("adhoc_filters")
+ if not isinstance(raw_filters, list):
+ raise ValueError("adhoc_filters must be an array")
+ filters: list[dict[str, Any]] = []
+ for index, raw_filter in enumerate(raw_filters):
+ if not isinstance(raw_filter, dict):
+ raise ValueError(f"adhoc_filters[{index}] must be an object")
+ if raw_filter.get("expressionType") != "SIMPLE":
+ raise ValueError(
+ f"adhoc_filters[{index}] must use expressionType='SIMPLE'"
+ )
+ if raw_filter.get("clause") not in (None, "WHERE"):
+ raise ValueError(f"adhoc_filters[{index}] must use
clause='WHERE'")
+ subject = raw_filter.get("subject")
+ operator = raw_filter.get("operator")
+ comparator = raw_filter.get("comparator")
+ if operator == "TEMPORAL_RANGE":
+ if not isinstance(subject, str) or not subject:
+ raise ValueError(
+ f"adhoc_filters[{index}] temporal filter needs subject"
+ )
+ data.setdefault("temporal_column", subject)
+ if isinstance(comparator, str) and comparator.casefold() !=
"no filter":
+ data.setdefault("time_range", comparator)
+ continue
+ if not isinstance(operator, str):
+ raise ValueError(f"adhoc_filters[{index}] needs an operator")
+ operator_map = {
+ "==": "=",
+ "EQUALS": "=",
+ "NOT_EQUALS": "!=",
+ "LESS_THAN": "<",
+ "LESS_THAN_OR_EQUAL": "<=",
+ "GREATER_THAN": ">",
+ "GREATER_THAN_OR_EQUAL": ">=",
+ "NOT_IN": "NOT IN",
+ "IS_NULL": "IS NULL",
+ "IS_NOT_NULL": "IS NOT NULL",
+ }
+ operator = operator_map.get(operator, operator)
+ filters.append({"column": subject, "op": operator, "value":
comparator})
+ data["filters"] = filters
+
+ @model_validator(mode="before")
+ @classmethod
+ def adapt_native_form_data(cls, raw: Any) -> Any: # noqa: C901
+ """Accept recognized saved Bullet form_data and reject ambiguous
state."""
+ if not isinstance(raw, dict):
+ return raw
+ data = dict(raw)
+ # Null means no hierarchy was supplied, not a conflicting alias or a
+ # request to clear saved dimensions. Only an explicit [] clears them.
+ for key in ("dimensions", "groupby"):
+ if data.get(key) is None:
+ data.pop(key, None)
+ if "dimensions" in data and "groupby" in data:
+ dimensions = cls._canonical_dimension_alias(
+ data["dimensions"], "dimensions"
+ )
+ groupby = cls._canonical_dimension_alias(data["groupby"],
"groupby")
+ if dimensions != groupby:
+ raise ValueError(
+ "Conflicting Bullet dimension aliases: 'dimensions' and "
+ "native 'groupby' must identify the same physical columns
in "
+ "the same order; provide only one or make them equivalent"
+ )
+ # Avoid relying on AliasChoices precedence or JSON key order.
+ data.pop("groupby")
+ if data.get("viz_type") == "bullet":
+ data.setdefault("chart_type", "bullet")
+ data.pop("viz_type", None)
+ for key in (
+ "annotation_layers",
+ "dashboards",
+ "datasource",
+ "datasource_id",
+ "datasource_type",
+ "extra_form_data",
+ "slice_id",
+ "slice_name",
+ ):
+ data.pop(key, None)
+
+ if (marker_key := "_mcp_dashboard_time_filter_subject") in data:
+ marker = data.pop(marker_key)
+ if not isinstance(marker, str) or not marker:
+ raise ValueError(f"{marker_key} must be a physical column
name")
+ raw_filters = data.get("adhoc_filters")
+ if not isinstance(raw_filters, list):
+ raise ValueError(
+ f"{marker_key} requires an adhoc_filters array containing
its "
+ "generated binding"
+ )
+ provenance_matches = [
+ filter_
+ for filter_ in raw_filters
+ if isinstance(filter_, dict)
+ and filter_.get("subject") == marker
+ and filter_.get("operator") == "TEMPORAL_RANGE"
+ ]
+ if len(provenance_matches) != 1:
+ raise ValueError(
+ f"{marker_key} must match exactly one TEMPORAL_RANGE
filter "
+ f"for subject {marker!r}; found {len(provenance_matches)}"
+ )
+ data.setdefault("temporal_column", marker)
+
+ if "metric" in data:
+ data["metric"] = cls._adapt_native_metric(data["metric"])
+ for key in ("groupby", "dimensions"):
+ if key in data:
+ if not isinstance(data[key], list):
+ raise ValueError(f"{key} must be an array")
+ data[key] = [
+ {"name": item} if isinstance(item, str) else item
+ for item in data[key]
+ ]
+ for key in ("order_by", "orderby", "order_by_cols"):
+ if key in data:
+ data[key] = cls._adapt_native_order_by(data[key])
+ cls._adapt_native_filters(data)
+ return data
+
+ @field_validator("ranges", "markers", "marker_lines", mode="before")
+ @classmethod
+ def tokenize_native_numeric_lists(cls, value: Any) -> Any:
+ """Parse numeric controls without creating values for empty tokens."""
+ if value is None:
+ return []
+ if isinstance(value, str):
+ return [token.strip() for token in value.split(",") if
token.strip()]
+ return value
+
+ @field_validator(
+ "range_labels", "marker_labels", "marker_line_labels", mode="before"
+ )
+ @classmethod
+ def tokenize_native_label_lists(cls, value: Any) -> Any:
+ """Parse label controls while preserving positional empty tokens."""
+ if value is None or value == "":
+ return []
+ if isinstance(value, str):
+ return [token.strip() for token in value.split(",")]
+ return value
+
+ @field_validator("ranges", "markers", "marker_lines")
+ @classmethod
+ def reject_non_finite_values(cls, values: List[float]) -> List[float]:
+ if any(not math.isfinite(value) for value in values):
+ raise ValueError("Bullet thresholds and markers must be finite
numbers")
+ return values
+
+ @field_validator("range_labels", "marker_labels", "marker_line_labels")
+ @classmethod
+ def validate_presentation_labels(cls, labels: List[str]) -> List[str]:
+ result: list[str] = []
+ for label in labels:
+ if "," in label:
+ raise ValueError(
+ "Bullet labels cannot contain commas because the frontend "
+ "comma-separated controls have no escaping"
+ )
+ if label == "":
+ result.append("")
+ continue
+ sanitized = sanitize_user_input(
+ label, "Bullet label", max_length=200, allow_empty=True
+ )
+ if sanitized is not None:
+ result.append(sanitized)
+ return result
+
+ @field_validator("time_range")
+ @classmethod
+ def sanitize_time_range(cls, value: str | None) -> str | None:
+ return sanitize_user_input(
+ value, "Time range", max_length=1000, allow_empty=True
+ )
+
+ @model_validator(mode="after")
+ def validate_roles_and_outputs(self) -> "BulletChartConfig": # noqa: C901
+ dimensions = self.dimensions or []
+ seen_names: set[str] = set()
+ for index, dimension in enumerate(dimensions):
+ _reject_sql_expression_on_dimension(dimension,
f"dimensions[{index}]")
+ if dimension.saved_metric or dimension.aggregate:
+ raise ValueError(
+ f"dimensions[{index}] must be a physical dimension, not a
metric"
+ )
+ name = dimension.name or ""
+ if name in seen_names:
+ raise ValueError(f"Duplicate Bullet dimension: {name!r}")
+ seen_names.add(name)
+
+ # ``map_bullet_config`` deliberately emits physical groupby names. The
+ # frontend therefore reads dimension results under those names even
when
+ # a friendly ``ColumnRef.label`` was supplied. Only the metric is
emitted
+ # with an output alias. Keep validation aligned with those actual
result
+ # fields instead of treating dimension display labels as SQL aliases.
+ if (metric_output := _bullet_metric_output_label(self.metric)) in
seen_names:
+ raise ValueError(
+ f"Bullet metric output label {metric_output!r} conflicts with
a "
+ "dimension (its physical output name); provide a unique metric
label"
+ )
+
+ resolved_order: list[tuple[str, int | None]] = []
+ for item in self.order_by:
Review Comment:
`order_by` is checked against `self.dimensions` here, before the saved
dimension hierarchy is merged in by `update_chart`/`update_chart_preview`.
Updating a saved Bullet chart's `order_by` while omitting `dimensions` -- the
documented way to preserve the saved hierarchy -- fails with "order_by must
reference a Bullet dimension or metric output" even though the referenced
column exists in the chart's saved dimensions. Could this cross-check be
deferred to the merge step, where the saved hierarchy is available?
--
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]
