aminghadersohi commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4129946199
##########
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:
Fixed in 70611d12b4f566cd690093318d5159cf28caa57c: removed the duplicate
unguarded parse and reused the guarded saved-params path, so malformed legacy
params no longer block a valid saved query context. The regression test covers
malformed JSON.
##########
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:
Fixed in 70611d12b4f566cd690093318d5159cf28caa57c: resolved the data plugin
from the effective form data so cached type changes use the queried plugin’s
empty-result and row-shape contract. Added coverage for an empty Bullet preview
overriding a saved table chart.
##########
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:
Fixed in 70611d12b4f566cd690093318d5159cf28caa57c: deferred Bullet sort
cross-checks until the saved hierarchy is resolved before mapping in both
update tools. Tests cover omitted/null dimensions, unknown targets, rebinds,
and physical-dimension precedence over metric aliases.
##########
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:
Fixed in 70611d12b4f566cd690093318d5159cf28caa57c: set Gauge’s
owns_update_merge flag so generic preservation cannot restore intentionally
cleared bindings. Regression tests cover null time_range and granularity_sqla
in save, preview-first, and cached-preview updates.
##########
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:
Fixed in 70611d12b4f566cd690093318d5159cf28caa57c: removed the unused
NORMALIZATION_EXCEPTIONS constant and its stale shared-handling comment.
--
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]
