aminghadersohi commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4199436006
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -193,6 +307,12 @@ def _build_new_filter_config(
"defaultDataMask": _empty_data_mask(),
}
+ if spec.default_value is not None or spec.default_to_first_item:
Review Comment:
The split was not intentional; fixed in
3f78e8daf355fa8a40d91f8d841badd54c713d58. _build_new_filter_config also routes
enable_empty_filter=True through _default_data_mask when default_value is
omitted. For a required select filter without first-item mode, this stores
filterState.value=[] and the 1 = 0 adhoc predicate, just like the update path.
_extract_filter_extra_form_data therefore reports APPLIED instead of
NOT_APPLIED. First-item mode still takes precedence and stores filterState={} /
extraFormData={} so the UI can choose the first loaded option.
test_required_empty_default_applies_in_server_dashboard_context covers both
omitted and explicit-empty defaults; the omitted case failed before the fix and
passes afterward. All 891 dashboard unit tests pass; pre-commit passes on
touched files and all branch-changed files, including mypy. The behavior is
documented in docs/admin_docs/configuration/mcp-server.mdx.
##########
superset/mcp_service/dashboard/tool/manage_native_filters.py:
##########
@@ -293,6 +409,68 @@ def _merge_target(spec: NativeFilterUpdateSpec, merged:
dict[str, Any]) -> None:
merged["targets"] = [target]
+def _target_key(target: dict[str, Any]) -> tuple[Any, Any]:
+ """Identify a filter target by its dataset and column name."""
+ column = target.get("column")
+ return target.get("datasetId"), (
+ column.get("name") if isinstance(column, dict) else column
+ )
+
+
+def _stored_default_is_stale(
+ spec: NativeFilterUpdateSpec, existing: dict[str, Any], target_changed:
bool
+) -> bool:
+ """Whether an update without ``default_value`` invalidates the stored one.
+
+ A stored default goes stale when the filter is retargeted to another
+ column or dataset, when it becomes single-select while the default holds
+ several values, or when ``default_to_first_item`` is switched on (the
+ explicit default would otherwise win and the first item never applies).
+ """
+ if existing.get("filterType") != "filter_select":
+ return False
+ if spec.default_to_first_item is True:
+ return True
+ stored = ((existing.get("defaultDataMask") or {}).get("filterState") or
{}).get(
+ "value"
+ )
+ if stored is None:
+ return False
+ stored_count = len(stored) if isinstance(stored, list) else 1
+ return target_changed or (spec.multi_select is False and stored_count > 1)
+
+
+def _merge_select_default(
+ spec: NativeFilterUpdateSpec,
+ existing: dict[str, Any],
+ merged: dict[str, Any],
+ target_changed: bool,
+) -> None:
+ """Apply an explicit default_value, or drop a stored default gone stale."""
+ if spec.default_value is not None:
+ if (merged.get("controlValues") or {}).get("defaultToFirstItem"):
+ raise _FilterValidationError(
+ f"Filter '{spec.id}' has default_to_first_item enabled; "
+ "pass default_to_first_item=False in this same update "
+ "before setting an explicit default_value."
+ )
+ merged["defaultDataMask"] = _default_data_mask(merged,
spec.default_value)
+ elif (
+ existing.get("filterType") == "filter_select"
+ and spec.enable_empty_filter is not None
+ and not ((existing.get("defaultDataMask") or {}).get("filterState") or
{}).get(
+ "value"
+ )
+ ):
+ # Rebuild empty masks when the required control changes, including
+ # older masks that stored value=None alongside an impossible predicate.
+ merged["defaultDataMask"] = _default_data_mask(merged, [])
+ elif _stored_default_is_stale(spec, existing, target_changed):
Review Comment:
Agreed: leaving legacy rows dependent on a deliberate control toggle was
unnecessarily narrow. Fixed in 3f78e8daf355fa8a40d91f8d841badd54c713d58. After
explicit-default validation, _merge_select_default checks whether the merged
select controls still enable defaultToFirstItem and the stored filterState
contains a value key. Any update then resets the mask through
_default_data_mask to filterState={} / extraFormData={}, removing null or empty
selections that block automatic first-item selection. Explicit default_value
updates still require disabling first-item mode in the same update.
test_update_first_item_heals_legacy_selection covers all five updates you
listed with legacy null and [] masks and both required-control settings, and
checks that the original configuration is unchanged. All 20 cases failed before
the fix and pass afterward. The full dashboard unit suite passes 891 tests;
pre-commit, including mypy, passes on touched files and all branch-changed
files.
--
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]
