gabotorresruiz commented on code in PR #43679:
URL: https://github.com/apache/superset/pull/43679#discussion_r3982148829
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -689,9 +689,20 @@ def merge_chart_form_data( # noqa: C901
if not isinstance(config, GaugeChartConfig):
if dataset_rebind:
return dict(new_form_data)
+ fields_set = config.model_fields_set
+ if "filters" not in fields_set:
Review Comment:
Thanks for the follow-up Amin, the per-key rebind preservation is a clear
improvement over blanking everything. One problem remains though: the
`"filters" not in fields_set` gate (now in `merge_chart_form_data`) does not
survive column normalization. `update_chart` reassigns `parsed_config` from
`DatasetValidator.normalize_column_names`, and every plugin except gauge
round-trips the config through a full `model_dump()` then `model_validate()`,
which marks every field as set. Gauge dodges it because its
`normalize_column_refs` uses `model_dump(exclude_unset=True)`; XY, table,
mixed_timeseries and the rest do not. I verified it on this branch: passing
`_build_update_payload` a config round-tripped that way (the shape the tool
produces whenever the dataset resolves) skips `preserve_previous_adhoc_filters`
and drops a saved `country = US` predicate again, while the same call with the
un-normalized config preserves it. The tests all build configs from sparse
dicts, so they only exercis
e the un-normalized shape.
Two ways to close it: gate on the value instead, which is immune to
normalization,
```python
if getattr(config, "filters", None) is None:
preserve_previous_adhoc_filters(new_form_data, existing_form_data)
```
or switch the remaining plugins to `model_dump(exclude_unset=True)` like
gauge. Either way, a regression test that round-trips the config through
`model_validate(config.model_dump())` before calling `_build_update_payload`
would pin the production path. The explicit-empty pops are fine as-is since
they compare values.
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -211,6 +212,151 @@ def _merge_replacement_config(
)
+def _valid_dataset_reference(
+ value: Any,
+ columns: set[str],
+ metrics: set[str],
+ *,
+ allow_metric: bool = False,
+) -> bool:
+ """Return whether a form_data reference resolves against a dataset."""
+ if not isinstance(value, str):
+ return True
+ normalized = value.casefold()
+ return normalized in columns or (allow_metric and normalized in metrics)
+
+
+def _inherited_metrics_match_dataset(
+ existing_form_data: dict[str, Any],
+ columns: set[str],
+ metrics: set[str],
+) -> bool:
+ for metric in existing_form_data.get("metrics") or []:
+ if isinstance(metric, str) and not _valid_dataset_reference(
+ metric, columns, metrics, allow_metric=True
+ ):
+ return False
+ if isinstance(metric, dict):
+ column = metric.get("column")
+ if isinstance(column, dict) and not _valid_dataset_reference(
+ column.get("column_name"), columns, metrics
+ ):
+ return False
+ return True
+
+
+def _inherited_sort_matches_dataset(
+ order_by_cols: Any, columns: set[str], metrics: set[str]
+) -> bool:
+ for order_by in order_by_cols or []:
+ try:
+ column = json.loads(order_by)[0]
+ except (TypeError, ValueError, IndexError):
+ return False
+ if not _valid_dataset_reference(column, columns, metrics,
allow_metric=True):
+ return False
+ return True
+
+
+def _inherited_filters_match_dataset(
+ filters: Any, columns: set[str], metrics: set[str]
+) -> bool:
+ for filter_ in filters or []:
+ if not isinstance(filter_, dict):
+ return False
+ if filter_.get("expressionType") not in (None, "SIMPLE"):
+ return False
+ subject = filter_.get("subject") or filter_.get("col")
+ allow_metric = str(filter_.get("clause", "WHERE")).upper() == "HAVING"
+ if not _valid_dataset_reference(
+ subject, columns, metrics, allow_metric=allow_metric
+ ):
+ return False
+ return True
+
+
+#: form_data keys carrying query roles, mapped to the config field that
+#: sets them explicitly. An explicit field is the caller's stated intent,
+#: so it is never treated as inherited state.
+_INHERITED_QUERY_ROLE_FIELDS = {
+ "groupby": "group_by",
+ "groupby_b": "group_by_secondary",
+ "all_columns": None,
+ "columns": None,
+ "x_axis": None,
+ "granularity_sqla": None,
+ "metrics": None,
+ "order_by_cols": "sort_by",
+ "adhoc_filters": "filters",
+}
+
+_INHERITED_COLUMN_LIST_KEYS = frozenset(
+ {"groupby", "groupby_b", "all_columns", "columns"}
+)
+_INHERITED_COLUMN_SCALAR_KEYS = frozenset({"x_axis", "granularity_sqla"})
+
+
+def _inherited_state_invalid_keys(
+ existing_form_data: dict[str, Any],
+ new_form_data: dict[str, Any],
+ parsed_config: ChartConfig,
+ dataset_id: int,
+) -> set[str]:
+ """Return inherited query fields that are invalid for a new dataset.
+
+ A dataset rebind only has to discard the state that cannot resolve
+ against the replacement dataset; everything else stays valid and is
+ preserved so the update does not silently reset the chart.
+ """
+ fields_set = parsed_config.model_fields_set
Review Comment:
Same normalization issue reaches the rebind check:
`_inherited_state_invalid_keys` excludes a key from the inherited set whenever
its config field is in `model_fields_set`, and after `normalize_column_names`
every field is. I verified on this branch that a rebind whose saved params
carry only an incompatible adhoc filter never consults the target dataset and
still lands `old_ds_col` in the merged params with a round-tripped config, so
the "rejection names a column the caller never sent" failure mode is still
reachable. The `is None` value test (or the `exclude_unset` normalization fix)
closes this one too; the per-key preservation logic itself checks out,
including the fail-safe when the replacement dataset cannot be inspected.
--
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]
