gabotorresruiz commented on code in PR #44985:
URL: https://github.com/apache/superset/pull/44985#discussion_r4198783252
##########
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:
Not a blocker, and it is about rows that already exist rather than about
this change. Master's create path wrote `{"filterState": {"value": null}}` for
`default_to_first_item: true` (I confirmed that on the merge base), and on this
branch a filter carrying that mask is only healed when the update passes
`default_to_first_item` or `enable_empty_filter`. I ran `name`, `description`,
`scope_chart_ids`, `sort_ascending` and `search_all_options` updates against
one and the stale `{"value": null}` survived all five, so the first item stays
blocked for those filters.
Would it be worth resetting whenever `merged`'s `defaultToFirstItem` is on
and the stored `filterState` still has a `value` key, so any update heals them?
Or do you prefer leaving existing rows to a deliberate `default_to_first_item:
true` call? I might be missing a reason to keep the narrower trigger.
##########
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:
Not a blocker, and more a question than a finding. The update path now
treats "required with nothing chosen" as a real stored default:
`{"enable_empty_filter": true}` on a select filter whose mask is `{"value":
null}` writes `{"filterState": {"value": []}, "extraFormData":
{"adhoc_filters": [1 = 0]}}`, and `_extract_filter_extra_form_data` reports
that one as `APPLIED`. Creating the same filter here with `enable_empty_filter:
true` and no `default_value` keeps `_empty_data_mask()`, which the same
function reports as `NOT_APPLIED`, so server side dashboard chart queries for
it come back unfiltered until some later update happens to touch
`enable_empty_filter`.
Was that split deliberate, with `default_value: []` being the way to ask for
the required empty default at create time? If not, adding `or
spec.enable_empty_filter` to this condition would line the two paths up.
--
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]
