aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4202686468
##########
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:
Fixed in c66908a76ee81fdcaea829bff2fdd9920d8788ab. Removed the request-time
bubble-metric validator; the existing plugin result-contract check validates
the final merged form data. Saved updates, unsaved updates, and cached-preview
updates can enable bubbles while retaining an omitted secondary metric. All
three public FastMCP regressions failed before the fix and pass after it,
checking returned/persisted controls. Additional regressions confirm fresh
generation still rejects missing size metrics and explicit clearing is rejected
when bubbles are enabled. Chart unit suite: 3,610 passed, 3 skipped; world-map
Jest: 61 passed; staged-file pre-commit passed.
##########
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:
Added in c66908a76ee81fdcaea829bff2fdd9920d8788ab. The existing duplicate
guard is correct; new parameterized tests cover both identical US/US and
case-variant US/us aggregates through transformData and through transformProps
with mcp_geographic: true. All four new cases fail when seen.has(...) is
temporarily removed; the two transformProps cases also fail when strict
forwarding is disabled. Production code is restored and all 61 world-map tests
pass. Chart unit suite: 3,610 passed, 3 skipped; staged-file pre-commit passed.
--
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]
