aminghadersohi commented on code in PR #45242:
URL: https://github.com/apache/superset/pull/45242#discussion_r4265103904
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +179,313 @@ 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.
+
+ Reads go through the column-values API's ``data_cache`` entries (same key
+ and timeout), so a heavy virtual dataset is not re-queried per filter or
+ per tool call, and the UI and this tool warm each other. The key is
+ partitioned by the effective RLS, so entries are not shared across users
+ with different row-level security; the caller authorizes the dataset
+ before this runs. Searches are cached too, under the API's short
+ search-term timeout.
+ """
+ from superset.datasource.api import get_cached_column_values
+
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ if limit <= 0:
+ raise ValueError("Column-value validation requires a positive row
limit")
+ values, _ = get_cached_column_values(
+ dataset,
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search or None,
+ )
+ # Match the JSON scalar options the column-values API sends to the UI,
+ # rather than returning driver-specific date/decimal/numpy objects to the
+ # response schema or using their differing Python display labels.
+ candidates = json.loads(json.dumps(values))
+ if any(
+ v is not None and not isinstance(v, (str, int, float, bool)) for v in
candidates
+ ):
+ raise NotImplementedError("Structured column values are not scalar
selections")
+ return candidates, 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 return
+ ``_AMBIGUOUS`` instead of being treated as absent.
+ """
+ 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 = {
+ (type(candidate), candidate): candidate
+ for candidate in candidates
+ if candidate is not None and _normalize_value(candidate) == wanted
+ }
+ if len(matches) > 1:
+ return _AMBIGUOUS
+ return next(iter(matches.values())) if matches 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, dict[tuple[type, Any], Any]] = {}
+ for candidate in candidates:
+ if candidate is not None:
+ by_label.setdefault(_normalize_value(candidate), {})[
+ (type(candidate), 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 [
+ candidate
+ for label in dict.fromkeys(suggested)
+ for candidate in by_label[label].values()
+ ][:_MAX_VALUE_SUGGESTIONS]
+
+
+def _quote_values(values: Sequence[Any]) -> str:
+ """Render values as a quoted, comma-separated list."""
+ return ", ".join(
+ repr(value) if isinstance(value, str) else json.dumps(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}'. Unknown selections can produce empty chart "
+ "results. 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(candidates, 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 _unresolved_value_error(
+ conf: dict[str, Any],
+ value: FilterSelectValue,
+ candidates: Sequence[Any],
+ match: Any,
+) -> _FilterApplyError:
+ """Offer typed exact alternatives without claiming an unresolved value
absent."""
+ alternatives = {
+ (type(candidate), candidate): candidate
+ for candidate in candidates
+ if candidate is not None
+ and _normalize_value(candidate) == _normalize_value(value)
+ }
+ shown = ", ".join(
+ json.dumps(v) for v in
list(alternatives.values())[:_MAX_VALUE_SUGGESTIONS]
+ )
+ reason = (
+ "is ambiguous between distinct column values"
+ if match is _AMBIGUOUS
+ else "cannot be resolved uniquely because the value search is
truncated"
+ )
+ return _FilterApplyError(
+ f"Filter '{conf.get('name') or conf.get('id')}' value "
+ f"{json.dumps(value)} {reason}. No filter values were applied. "
+ f"Exact column values include: {shown}. Ask the user to choose "
+ "an exact value, preserving its spelling and JSON type; do not "
+ "report the value as absent or the filter as applied."
+ )
+
+
+def _search_value(
+ dataset: "SqlaTable",
+ conf: dict[str, Any],
+ column: str,
+ value: FilterSelectValue,
+ cache: dict[str, tuple[list[Any], bool]],
+) -> 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, native or non-ASCII values whose SQL
+ text representation or lowercasing may differ, a truncated search with no
+ match, or a failed query) is kept as given:
+ absence from a truncated page proves nothing. Normalized matches require
+ a complete search result to establish uniqueness; exact matches do not.
+ """
+ if not isinstance(value, str) or not value.isascii():
+ # NULL cannot be found with LIKE; native numbers and booleans need not
+ # have the same SQL text representation as their Python display label.
+ # SQL lower() also need not implement non-ASCII case folding.
+ return value, []
+ try:
+ target = dataset.get_column(column)
+ if target is not None and target.is_string is False:
+ # Numeric, boolean and date columns can be rendered differently by
+ # SQL casts and the JSON serializer (e.g. datetime's T separator).
+ return value, []
+ # SQL lower() is not Python casefold(). ASCII k/s can be stored as
+ # Kelvin sign/long s, and ff/fi/fl as ligatures. Exclude those pieces
+ # so every casefold-equivalent value contains the searched substring.
+ # Locale-sensitive lower() can map ASCII I to dotless ı as well.
+ # Splitting words also tolerates whitespace differences. If no safe
+ # substring remains, reuse the unfiltered page rather than claiming
+ # that a narrower query can establish absence or uniqueness.
+ pieces = [
+ part
+ for word in value.split()
+ for part in re.split(r"k|s|ffi|ffl|ff|fi|fl|i", word,
flags=re.IGNORECASE)
+ ]
+ term = max(pieces, key=len, default="")
+ key = term.lower()
+ if key not in cache:
+ if len(cache) - 1 >= _MAX_VALUE_SEARCHES:
+ return value, []
+ # Count failed attempts too, and reuse their unchecked result.
+ cache[key] = ([], False)
+ cache[key] = _column_values(dataset, column, search=term)
+ searched, complete = cache[key]
+ 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, []
+ match = _match_value(value, searched)
+ # Only an exact match is decisive when even the search results are
truncated.
+ # A lone normalized match on that page does not establish uniqueness.
+ if not complete and match is _NOT_FOUND:
+ # The literal value may be hidden beyond the narrowed result limit.
+ return value, searched
+ if not complete and match not in (_AMBIGUOUS, _NOT_FOUND) and match !=
value:
+ match = _UNRESOLVED
+ return match, 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 unknown values could silently empty chart results. 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
+ from superset.mcp_service.dashboard_scope import
get_request_dashboard_scope
+
+ scope = get_request_dashboard_scope()
+ try:
+ dataset = _target_dataset(conf)
+ # This tool creates links and remains scope-neutral. Column options
+ # cannot be constrained by values_for_column to the active dashboard
+ # view, so preserve the request without exposing unscoped suggestions.
+ if dataset is None or (scope is not None and scope.has_constraints):
Review Comment:
You are right: a time range alone made this skip validation. Fixed in
https://github.com/apache/superset/commit/600b983eb81b3c370bce863efb77a3eedc60623b.
Active dashboard scope no longer bypasses the dataset access check or column
lookup. Unknown values are rejected without creating a permalink;
`suggest=False` suppresses both suggestions/available values and exact
alternatives for ambiguous or truncated matches. A known column value still
does not certify that the active scoped subset contains matching rows.
The scoped regression cases failed on ede52e48, including bare time ranges,
native/adhoc filters, complete/truncated lookups, and suppressed alternatives.
The touched file passes 211 tests; dashboard and dashboard-scope suites pass
1,359 tests. Pre-commit passes on all branch-changed files.
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -158,6 +179,313 @@ 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.
+
+ Reads go through the column-values API's ``data_cache`` entries (same key
+ and timeout), so a heavy virtual dataset is not re-queried per filter or
+ per tool call, and the UI and this tool warm each other. The key is
+ partitioned by the effective RLS, so entries are not shared across users
+ with different row-level security; the caller authorizes the dataset
+ before this runs. Searches are cached too, under the API's short
+ search-term timeout.
+ """
+ from superset.datasource.api import get_cached_column_values
+
+ limit = apply_max_row_limit(current_app.config["FILTER_SELECT_ROW_LIMIT"])
+ if limit <= 0:
+ raise ValueError("Column-value validation requires a positive row
limit")
+ values, _ = get_cached_column_values(
+ dataset,
+ column_name=column,
+ limit=limit,
+ denormalize_column=not dataset.normalize_columns,
+ search=search or None,
+ )
+ # Match the JSON scalar options the column-values API sends to the UI,
+ # rather than returning driver-specific date/decimal/numpy objects to the
+ # response schema or using their differing Python display labels.
+ candidates = json.loads(json.dumps(values))
+ if any(
+ v is not None and not isinstance(v, (str, int, float, bool)) for v in
candidates
+ ):
+ raise NotImplementedError("Structured column values are not scalar
selections")
+ return candidates, 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 return
+ ``_AMBIGUOUS`` instead of being treated as absent.
+ """
+ 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
Review Comment:
Pinned in
https://github.com/apache/superset/commit/600b983eb81b3c370bce863efb77a3eedc60623b.
Added the two one-sided column cases you suggested, plus the corresponding
False/0 cases. Each asserts an unknown-value error and that no permalink state
is written.
Verified by temporarily removing the boolean-type guard: all four new cases
fail. Restoring the guard makes them pass; its existing behavior is unchanged.
The touched file passes 211 tests, dashboard/dashboard-scope suites pass 1,359
tests, and pre-commit passes on 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]
