aminghadersohi commented on code in PR #42841:
URL: https://github.com/apache/superset/pull/42841#discussion_r4409226410
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1046,6 +1046,41 @@ class AxisConfig(UnknownFieldCheckMixin,
OmittedMeansUnchanged):
)
+def _route_x_axis_key(data: Any, column_keys: tuple[str, ...]) -> Any:
+ """Route an ``x_axis`` payload to the column field or to axis styling.
+
+ Native ``form_data`` names the x-axis column ``x_axis``, and models that
+ accept it as a column alias also expose an ``x_axis`` styling field, so a
+ single input key would otherwise feed both. The payload's shape decides:
+ a bare string, or a non-empty mapping that names no styling field, is a
+ column reference and is moved to ``x``; anything else stays put and
+ configures the axis. Keying off the styling names rather than the column
+ names keeps a mistyped styling key ("sort_by") reported against
+ ``AxisConfig``, where the caller meant it, instead of against
``ColumnRef``.
+ """
+ if not isinstance(data, dict) or "x_axis" not in data:
+ return data
+ value = data["x_axis"]
+ is_column_ref = isinstance(value, str) or (
+ isinstance(value, dict)
+ and bool(value)
+ and not set(value) & set(AxisConfig.model_fields)
+ )
+ if not is_column_ref:
+ return data
+ data = dict(data)
+ column = data.pop("x_axis")
+ if any(data.get(key) is not None for key in column_keys):
+ raise ValueError(
+ "'x_axis' was read as the x-axis column, but "
+ f"{' / '.join(repr(key) for key in column_keys)} names it too. "
+ "Pass the column once, and use 'x_axis' only for axis styling "
+ "(title, scale, format)."
+ )
+ data["x"] = column
Review Comment:
`MixedTimeseriesChartConfig.x` has no string coercion (XY's `x` has
`coerce_x_column_name`), so the native form_data shape `x_axis: "ds"` is moved
here and still rejected on mixed_timeseries. Wrapping the string fixes both
configs.
```suggestion
data["x"] = {"name": column} if isinstance(column, str) else column
```
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -745,6 +747,83 @@ def
test_unknown_field_nested_one_level_down_is_rejected(self) -> None:
AxisConfig.model_validate({"title": "State", "sort_by": "metric"})
+class TestXAxisRouting:
+ """``x_axis`` names the x column and the axis styling; both must work.
+
+ Strict nested field checking makes the overlap load-bearing: a payload
+ routed to the wrong model is rejected rather than quietly half-applied.
+ """
+
+ XY_BASE = {"chart_type": "xy", "y": [{"name": "sales", "aggregate":
"SUM"}]}
+ MIXED_BASE = {
+ "chart_type": "mixed_timeseries",
+ "metrics": [{"name": "revenue", "aggregate": "SUM"}],
+ "metrics_b": [{"name": "orders", "aggregate": "COUNT"}],
+ }
+
+ @pytest.mark.parametrize(
+ "value", [{"name": "category"}, {"column_name": "category"},
"category"]
+ )
+ def test_column_shaped_x_axis_becomes_the_x_column(self, value: Any) ->
None:
+ config = XYChartConfig.model_validate({**self.XY_BASE, "x_axis":
value})
+ assert config.x is not None
+ assert config.x.name == "category"
+ assert config.x_axis is None
+
+ def test_styling_shaped_x_axis_configures_the_axis(self) -> None:
+ config = XYChartConfig.model_validate(
+ {**self.XY_BASE, "x_axis": {"title": "State", "scale": "log"}}
+ )
+ assert config.x is None
+ assert config.x_axis is not None
+ assert config.x_axis.title == "State"
+ assert config.x_axis.scale == "log"
+
+ def test_column_and_styling_both_supplied(self) -> None:
+ config = XYChartConfig.model_validate(
+ {**self.XY_BASE, "x": {"name": "category"}, "x_axis": {"title":
"State"}}
+ )
+ assert config.x is not None
+ assert config.x.name == "category"
+ assert config.x_axis is not None
+ assert config.x_axis.title == "State"
+
+ def test_column_named_twice_is_rejected(self) -> None:
+ """``x`` and a column-shaped ``x_axis`` together are ambiguous."""
+ with pytest.raises(ValidationError, match="Pass the column once"):
+ XYChartConfig.model_validate(
+ {**self.XY_BASE, "x": {"name": "a"}, "x_axis": {"name": "b"}}
+ )
+
+ def test_mistyped_styling_key_reported_against_the_axis(self) -> None:
+ """A styling payload stays a styling payload, typo and all."""
+ with pytest.raises(ValidationError, match="Unknown field 'sort_by'"):
+ XYChartConfig.model_validate(
+ {**self.XY_BASE, "x_axis": {"title": "State", "sort_by":
"metric"}}
+ )
+
+ def test_mistyped_column_key_reported_against_the_column(self) -> None:
+ with pytest.raises(ValidationError, match="Unknown field 'aggregat'"):
Review Comment:
Nit: Both models report `Unknown field 'aggregat'`, so this passes with the
production change reverted, when the typo lands on `AxisConfig`. Matching the
`ColumnRef` suggestion pins the routing.
```suggestion
with pytest.raises(ValidationError, match="did you mean
'aggregate'"):
```
--
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]
