aminghadersohi commented on code in PR #45242:
URL: https://github.com/apache/superset/pull/45242#discussion_r4257907765
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ from superset.daos.dataset import DatasetDAO
+
+ targets = [target for target in (conf.get("targets") or []) if target]
+ dataset_id = targets[0].get("datasetId") if targets else None
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
Review Comment:
Fixed in 44ab996c7aae9c50a5ff9d3f88c90223a0e8ea5a. Normalized candidates are
now keyed by type and value, so numeric 1 and text '1' remain distinct and an
inexact request such as ' 1 ' is rejected as ambiguous. Regression tests cover
both candidate orders and verify exact numeric/text matches still win. The new
ambiguity tests failed on the previous head; dashboard and related dataset
tests pass (1,447 tests), as do ruff, pylint, and pre-commit.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ from superset.daos.dataset import DatasetDAO
+
+ targets = [target for target in (conf.get("targets") or []) if target]
+ dataset_id = targets[0].get("datasetId") if targets else None
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
+ return next(iter(matches.values())) if len(matches) == 1 else _NOT_FOUND
+
+
+def _suggest_values(value: FilterSelectValue, candidates: Sequence[Any]) ->
list[Any]:
+ """Return column values that look like what ``value`` was meant to be."""
+ wanted = _normalize_value(value)
+ by_label: dict[str, Any] = {}
+ for candidate in candidates:
+ if candidate is not None:
+ by_label.setdefault(_normalize_value(candidate), candidate)
+ suggested = [label for label in by_label if wanted and wanted in label]
+ suggested += difflib.get_close_matches(
+ wanted, list(by_label), n=_MAX_VALUE_SUGGESTIONS, cutoff=0.6
+ )
+ return [by_label[label] for label in dict.fromkeys(suggested)][
+ :_MAX_VALUE_SUGGESTIONS
+ ]
+
+
+def _quote_values(values: Sequence[Any]) -> str:
+ """Render values as a quoted, comma-separated list."""
+ return ", ".join(f"'{_value_label(value)}'" for value in values)
+
+
+def _unknown_values_error(
+ conf: dict[str, Any],
+ column: str,
+ unknown: list[tuple[FilterSelectValue, list[Any]]],
+ candidates: list[Any],
+ complete: bool,
+) -> _FilterApplyError:
+ """Build the error for requested values that are absent from the data."""
+ name = conf.get("name") or conf.get("id")
+ noun = "value" if len(unknown) == 1 else "values"
+ message = [
+ f"Filter '{name}' has no {noun} {_quote_values([v for v, _ in
unknown])} "
+ f"in column '{column}', so applying it would leave every chart it "
+ "filters empty. No filter values were applied."
+ ]
+ for value, suggestions in unknown:
+ if suggestions:
+ message.append(
+ f"Closest matches for '{_value_label(value)}': "
+ f"{_quote_values(suggestions)}."
+ )
+ if not any(suggestions for _, suggestions in unknown):
+ shown = sorted(
+ (value for value in candidates if value is not None),
key=_value_label
+ )[:_MAX_VALUE_SUGGESTIONS]
+ if not shown:
+ message.append(
+ "The column has no values to filter on. Tell the user the "
+ "value was not found instead of reporting the filter as
applied."
+ )
+ return _FilterApplyError(" ".join(message))
+ total = f" ({len(candidates)} in total)" if complete else ""
+ prefix = "Available values" if complete else "Available values include"
+ message.append(f"{prefix}: {_quote_values(shown)}{total}.")
+ message.append(
+ "Tell the user the value was not found and offer these values instead "
+ "of reporting the filter as applied."
+ )
+ return _FilterApplyError(" ".join(message))
+
+
+def _search_value(
+ dataset: "SqlaTable",
+ conf: dict[str, Any],
+ column: str,
+ value: FilterSelectValue,
+) -> tuple[Any, list[Any]]:
+ """Look ``value`` up across the whole column, past a truncated first page.
+
+ Returns the matched value (or ``_NOT_FOUND``) and the values the search
+ returned, for suggestions. A value the search cannot settle (NULL, which
+ a containment search cannot find, or a failed query) is kept as given:
+ absence from a truncated page proves nothing.
+ """
+ if value is None:
+ return value, []
+ try:
+ searched, _ = _column_values(dataset, column,
search=_value_label(value))
Review Comment:
Fixed in 44ab996c7aae9c50a5ff9d3f88c90223a0e8ea5a. The containment search
uses the longest whitespace-delimited word rather than the unchanged request
phrase, then validates the returned values with the existing case/whitespace
normalization. This finds stored single-space, repeated-space, and tab variants
without inserting wildcard patterns. Added failing regression cases plus real
SQLite query coverage; dashboard and related dataset tests pass (1,447 tests),
as do ruff, pylint, and pre-commit.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +172,208 @@ def _filter_target_column(conf: dict[str, Any]) -> str:
return column
+def _target_dataset(conf: dict[str, Any]) -> "SqlaTable | None":
+ """Return the dataset a filter's first target points at, if it resolves."""
+ from superset.daos.dataset import DatasetDAO
+
+ targets = [target for target in (conf.get("targets") or []) if target]
+ dataset_id = targets[0].get("datasetId") if targets else None
+ return DatasetDAO.find_by_id(dataset_id) if dataset_id is not None else
None
+
+
+def _column_values(
+ dataset: "SqlaTable", column: str, search: str | None = None
+) -> tuple[list[Any], bool]:
+ """Return ``column``'s distinct values and whether that list is complete.
+
+ Uses the same bounded, RLS-aware query that feeds the dashboard's select
+ filter options, so the values checked here are the ones a viewer could
+ pick. ``search`` narrows it to values containing the term,
+ case-insensitively.
+ """
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ values = dataset.values_for_column(
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search,
+ )
+ return values, len(values) < limit
+
+
+def _normalize_value(value: FilterSelectValue) -> str:
+ """Fold a value to the text it is compared by when not matched exactly."""
+ return " ".join(_value_label(value).split()).casefold()
+
+
+def _match_value(value: FilterSelectValue, candidates: Sequence[Any]) -> Any:
+ """Return the column value ``value`` refers to, or ``_NOT_FOUND``.
+
+ An exact match keeps the requested value. Otherwise a single column
+ value whose text matches ignoring case and whitespace (``"emea"`` for
+ ``"EMEA"``, ``"2024"`` for ``2024``) is used instead, since filtering on
+ the requested spelling would match no rows. Several such values are
+ ambiguous and count as no match.
+ """
+ for candidate in candidates:
+ # bool is an int subclass, so True would otherwise match 1.
+ if candidate == value and isinstance(candidate, bool) == isinstance(
+ value, bool
+ ):
+ return value
+ if value is None:
+ return _NOT_FOUND
+ wanted = _normalize_value(value)
+ matches = {
+ _value_label(candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
+ return next(iter(matches.values())) if len(matches) == 1 else _NOT_FOUND
+
+
+def _suggest_values(value: FilterSelectValue, candidates: Sequence[Any]) ->
list[Any]:
+ """Return column values that look like what ``value`` was meant to be."""
+ wanted = _normalize_value(value)
+ by_label: dict[str, Any] = {}
+ for candidate in candidates:
+ if candidate is not None:
+ by_label.setdefault(_normalize_value(candidate), candidate)
+ suggested = [label for label in by_label if wanted and wanted in label]
+ suggested += difflib.get_close_matches(
+ wanted, list(by_label), n=_MAX_VALUE_SUGGESTIONS, cutoff=0.6
+ )
+ return [by_label[label] for label in dict.fromkeys(suggested)][
+ :_MAX_VALUE_SUGGESTIONS
+ ]
+
+
+def _quote_values(values: Sequence[Any]) -> str:
+ """Render values as a quoted, comma-separated list."""
+ return ", ".join(f"'{_value_label(value)}'" for value in values)
+
+
+def _unknown_values_error(
+ conf: dict[str, Any],
+ column: str,
+ unknown: list[tuple[FilterSelectValue, list[Any]]],
+ candidates: list[Any],
+ complete: bool,
+) -> _FilterApplyError:
+ """Build the error for requested values that are absent from the data."""
+ name = conf.get("name") or conf.get("id")
+ noun = "value" if len(unknown) == 1 else "values"
+ message = [
+ f"Filter '{name}' has no {noun} {_quote_values([v for v, _ in
unknown])} "
+ f"in column '{column}', so applying it would leave every chart it "
+ "filters empty. No filter values were applied."
+ ]
+ for value, suggestions in unknown:
+ if suggestions:
+ message.append(
+ f"Closest matches for '{_value_label(value)}': "
+ f"{_quote_values(suggestions)}."
+ )
+ if not any(suggestions for _, suggestions in unknown):
+ shown = sorted(
+ (value for value in candidates if value is not None),
key=_value_label
+ )[:_MAX_VALUE_SUGGESTIONS]
+ if not shown:
+ message.append(
+ "The column has no values to filter on. Tell the user the "
+ "value was not found instead of reporting the filter as
applied."
+ )
+ return _FilterApplyError(" ".join(message))
+ total = f" ({len(candidates)} in total)" if complete else ""
+ prefix = "Available values" if complete else "Available values include"
+ message.append(f"{prefix}: {_quote_values(shown)}{total}.")
+ message.append(
+ "Tell the user the value was not found and offer these values instead "
+ "of reporting the filter as applied."
+ )
+ return _FilterApplyError(" ".join(message))
+
+
+def _search_value(
+ dataset: "SqlaTable",
+ conf: dict[str, Any],
+ column: str,
+ value: FilterSelectValue,
+) -> tuple[Any, list[Any]]:
+ """Look ``value`` up across the whole column, past a truncated first page.
+
+ Returns the matched value (or ``_NOT_FOUND``) and the values the search
+ returned, for suggestions. A value the search cannot settle (NULL, which
+ a containment search cannot find, or a failed query) is kept as given:
+ absence from a truncated page proves nothing.
+ """
+ if value is None:
+ return value, []
+ try:
+ searched, _ = _column_values(dataset, column,
search=_value_label(value))
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning(
+ "Could not search column %s for filter %s",
+ column,
+ conf.get("id"),
+ exc_info=True,
+ )
+ return value, []
+ return _match_value(value, searched), searched
+
+
+def _verify_select_values(
+ conf: dict[str, Any], column: str, values: list[FilterSelectValue]
+) -> list[FilterSelectValue]:
+ """Check requested select values against the filter column's data.
+
+ Returns the values to apply, with near matches replaced by the column
+ value they match, and raises when a value matches nothing: an ``IN``
+ predicate on a value the column lacks would silently empty every chart
+ in the filter's scope. When the values cannot be read (unresolvable or
+ inaccessible dataset, query failure, unsupported datasource) they are
+ applied unchecked, as before, rather than blocking the request.
+ """
+ from superset.exceptions import SupersetSecurityException
+
+ try:
+ dataset = _target_dataset(conf)
+ if dataset is None:
+ return values
+ # The column-values API gates on the same check before reading values.
+ dataset.raise_for_access()
+ candidates, complete = _column_values(dataset, column)
+ except SupersetSecurityException:
+ return values
+ except Exception: # noqa: BLE001 pylint: disable=broad-except
+ logger.warning(
+ "Could not read values of column %s for filter %s; applying the "
+ "requested values unchecked",
+ column,
+ conf.get("id"),
+ exc_info=True,
+ )
+ return values
+
+ resolved: list[FilterSelectValue] = []
+ # Keyed by type too: True == 1, but they are different selections.
+ seen: set[tuple[type, FilterSelectValue]] = set()
+ unknown: list[tuple[FilterSelectValue, list[Any]]] = []
+ for value in values:
+ match = _match_value(value, candidates)
+ searched: list[Any] = []
+ if match is _NOT_FOUND and not complete:
+ match, searched = _search_value(dataset, conf, column, value)
Review Comment:
Fixed in 44ab996c7aae9c50a5ff9d3f88c90223a0e8ea5a. A normalized match on a
truncated first page now triggers the column search; only an exact match may
skip it. If the search itself is truncated, a non-exact match is not accepted
as uniquely determined. Regression tests cover later case/spacing collisions,
truncated search results, and a later exact match taking precedence. The new
first-page ambiguity tests failed on the previous head; dashboard and related
dataset tests pass (1,447 tests), as do ruff, pylint, and pre-commit.
--
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]
