sadpandajoe commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4202451039
##########
superset-frontend/plugins/plugin-chart-world-map/src/transformData.ts:
##########
@@ -66,7 +80,38 @@ export default function transformData(
typeof row.country === 'string' && fieldtype
? getCountry(fieldtype, row.country)
: undefined;
+ if (options.strict) {
+ if (!countryInfo || seen.has(countryInfo.cca3)) {
Review Comment:
Two rows like `{country: 'US', sales: 10}` and `{country: 'us', sales: 20}`
both resolve to `USA`, so this guard is what stops a typed world map from
silently overwriting one aggregate. I don't see a test that sends duplicate (or
case-variant) countries through `transformData`/`transformProps` with
`mcp_geographic: true` and expects the duplicate error; the strict cases in
`transformData.test.ts` all use single rows. Removing the `seen.has(...)`
check, or not forwarding `strict` from `transformProps`, would leave the
current suite green.
Could you add a case with two rows resolving to the same country and assert
it throws?
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -3738,6 +3738,195 @@ def validate_gantt_roles(self) -> "GanttChartConfig":
# Discriminated union for runtime validation (not exposed in JSON Schema)
+def _omit_inherited_descriptions(schema: dict[str, Any]) -> None:
+ """Drop field descriptions the published parent schema already carries.
+
+ The geographic references and filters only tighten bounds on the shared
+ ``ColumnRef``/``FilterConfig`` fields, whose descriptions appear in the
+ same tool schema.
+ """
+ for prop in schema.get("properties", {}).values():
+ prop.pop("description", None)
+
+
+class GeographicColumnRef(ColumnRef):
+ """Closed ColumnRef for geographic roles."""
+
+ model_config = ConfigDict(
+ extra="forbid",
+ populate_by_name=True,
+ strict=True,
+ json_schema_extra=_omit_inherited_descriptions,
+ )
+ dtype: str | None = Field(None, max_length=128)
+
+
+GeographicFilterValue = Annotated[str, Field(max_length=1000)] | int | float |
bool
+
+
+class GeographicFilterConfig(FilterConfig):
+ """Closed FilterConfig with bounded values."""
+
+ model_config = ConfigDict(
+ extra="forbid",
+ populate_by_name=True,
+ strict=True,
+ allow_inf_nan=False,
+ json_schema_extra=_omit_inherited_descriptions,
+ )
+ value: (
+ GeographicFilterValue
+ | Annotated[list[GeographicFilterValue], Field(max_length=1000)]
+ | None
+ ) = Field(None, validation_alias=AliasChoices("value", "val"))
+
+
+class GeographicChartConfig(BaseChartConfig):
+ """Bounded controls shared by geographic visualizations."""
+
+ model_config = ConfigDict(extra="forbid", strict=True)
+ filters: list[GeographicFilterConfig] | None = Field(None, max_length=100)
+ row_limit: int = Field(10000, ge=1, le=10000)
+ time_range: str | None = Field(None, max_length=1000)
+
+ @field_validator("time_range")
+ @classmethod
+ def validate_geographic_time_range(cls, value: str | None) -> str | None:
+ """Reject time expressions the query parser would silently ignore."""
+ return validate_time_range(value)
+
+ @model_validator(mode="after")
+ def validate_geographic_roles(self) -> "GeographicChartConfig":
+ """Reject metric dimensions and unaggregated metric roles."""
+ for field in ("entity", "latitude", "longitude", "dimension"):
+ ref = getattr(self, field, None)
+ if ref is not None and (not ref.name or ref.is_metric):
+ raise ValueError(
+ f"{field} requires a named dataset column, not a metric"
+ )
+ for field in ("metric", "secondary_metric", "radius_metric"):
+ ref = getattr(self, field, None)
+ if ref is not None and not ref.is_metric:
+ raise ValueError(
+ f"{field} requires aggregate, saved_metric, or
sql_expression"
+ )
+ # These helpers import chart schemas, so defer to avoid a cycle.
+ from superset.mcp_service.chart.chart_utils import create_metric_object
+ from superset.mcp_service.chart.query_result import metric_result_label
+
+ dimensions = {
+ ref.name
+ for field in ("entity", "latitude", "longitude", "dimension")
+ if (ref := getattr(self, field, None)) is not None
+ }
+ seen_metrics: dict[str | None, ColumnRef] = {}
+ for field in ("metric", "secondary_metric", "radius_metric"):
+ ref = getattr(self, field, None)
+ if ref is None:
+ continue
+ label = metric_result_label(create_metric_object(ref))
+ if self.chart_type == "deck_scatter" and label in {
+ "position",
+ "weight",
+ "extraProps",
+ }:
+ raise ValueError(
+ f"Metric alias {label!r} conflicts with a native spatial
field"
+ )
+ if label in dimensions:
+ raise ValueError(
+ f"Metric alias {label!r} conflicts with a geographic
column"
+ )
+ if label in seen_metrics and seen_metrics[label] != ref:
+ raise ValueError(f"Distinct geographic metrics share alias
{label!r}")
+ seen_metrics[label] = ref
+ return self
+
+
+class CountryMapChartConfig(GeographicChartConfig):
+ """Regional choropleth joined against the bundled country boundaries."""
+
+ chart_type: Literal["country_map"]
+ country: Literal["usa", "canada", "australia", "japan", "uk"] = Field(
+ ..., description="Bundled boundary set. Only these countries are
supported."
+ )
+ region_format: Literal["name", "abbreviation", "iso_3166_2"] = Field(
+ ...,
+ description="Explicit source format; abbreviation means the ISO
suffix. "
+ "Names match bundled boundary names, not geocoding or fuzzy matching.",
+ )
+ entity: GeographicColumnRef
+ metric: GeographicColumnRef
+ linear_color_scheme: str = Field("schemeBlues", min_length=1,
max_length=100)
+ number_format: str = Field("SMART_NUMBER", min_length=1, max_length=100)
+
+
+class WorldMapChartConfig(GeographicChartConfig):
+ """Country choropleth with optional metric-sized bubbles."""
+
+ chart_type: Literal["world_map"]
+ entity: GeographicColumnRef
+ country_format: Literal["name", "cca2", "cca3", "cioc"]
+ metric: GeographicColumnRef
+ secondary_metric: GeographicColumnRef | None = None
+ show_bubbles: bool = False
+ max_bubble_size: int = Field(25, ge=1, le=100)
+ sort_by_metric: bool = True
+ linear_color_scheme: str = Field("schemeBlues", min_length=1,
max_length=100)
+
+ @model_validator(mode="after")
+ def validate_bubble_metric(self) -> "WorldMapChartConfig":
+ """Require an explicit size metric when bubbles are requested."""
+ if self.show_bubbles and self.secondary_metric is None:
Review Comment:
This validator runs when the request is parsed, before `update_chart` loads
the saved chart and merges omitted controls. So if chart 123 already has a
`secondary_metric` and bubbles are off, sending
`{"chart_type":"world_map",...,"show_bubbles":true}` without re-supplying
`secondary_metric` fails with "show_bubbles requires secondary_metric", even
though the merged config already has the size metric (the post-merge check in
the plugin would pass). That contradicts the "omitted controls survive
same-type updates" behavior described in the PR, and `update_chart_preview`
hits the same wall.
Would it make sense to enforce this only for fresh generation and rely on
the post-merge check for updates?
--
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]
