diff --git a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py index 816db306c..a5913a791 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/scoring.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/scoring.py @@ -63,29 +63,46 @@ def uri_to_display_name(uri: str) -> str: def validate_cross_references(viz: CreatedVisualization) -> tuple[bool, list[str]]: - """Validate ranking-filter `using`/`attribute` resolve to correct URI prefixes.""" + """Validate ranking-filter `using`/`attribute` resolve to correct URI prefixes. + + Always returns `(ok, errors)` — a malformed filter produces an error entry, never an + exception. Anything unusable (None, empty, non-string) used to reach `.startswith()` + or `dict.get()` and blow up with AttributeError/TypeError mid-evaluation. + + `using` is required by the AAC schema, `attribute` is optional (see + `_normalize_ranking_filter`), so an absent/None/empty `attribute` is accepted silently. + """ errors: list[str] = [] fields = viz.query.fields for filter_key, filter_dict in viz.query.filter_by.items(): if filter_dict.get("type") != "ranking_filter": continue - using_val = filter_dict.get("using", "") - using_uri = _resolve_alias_to_uri(using_val, fields) - field_def = fields.get(using_val) - is_adhoc_agg = isinstance(field_def, AacQueryField) and bool(field_def.aggregation) - if not using_uri.startswith(("metric/", "fact/")) and not is_adhoc_agg: - errors.append( - f"ranking filter '{filter_key}': using='{using_val}' " - f"resolves to '{using_uri}' — expected a metric/ or fact/ URI" - ) - if "attribute" in filter_dict: - attr_val = filter_dict["attribute"] - attr_uri = _resolve_alias_to_uri(attr_val, fields) - if not attr_uri.startswith(("label/", "attribute/")): + using_val = filter_dict.get("using") + if not isinstance(using_val, str) or not using_val: + errors.append(f"ranking filter '{filter_key}': using={using_val!r} — a metric/ or fact/ URI is required") + else: + using_uri = _resolve_alias_to_uri(using_val, fields) + field_def = fields.get(using_val) + is_adhoc_agg = isinstance(field_def, AacQueryField) and bool(field_def.aggregation) + if not using_uri.startswith(("metric/", "fact/")) and not is_adhoc_agg: errors.append( - f"ranking filter '{filter_key}': attribute='{attr_val}' " - f"resolves to '{attr_uri}' — expected a label/ or attribute/ URI" + f"ranking filter '{filter_key}': using='{using_val}' " + f"resolves to '{using_uri}' — expected a metric/ or fact/ URI" ) + attr_val = filter_dict.get("attribute") + if attr_val is None or attr_val == "": + continue + if not isinstance(attr_val, str): + errors.append( + f"ranking filter '{filter_key}': attribute={attr_val!r} — expected a label/ or attribute/ URI" + ) + continue + attr_uri = _resolve_alias_to_uri(attr_val, fields) + if not attr_uri.startswith(("label/", "attribute/")): + errors.append( + f"ranking filter '{filter_key}': attribute='{attr_val}' " + f"resolves to '{attr_uri}' — expected a label/ or attribute/ URI" + ) return len(errors) == 0, errors @@ -99,11 +116,43 @@ def _normalize_date_filter(filter_dict: dict, _fields: dict) -> dict: } -def _normalize_ranking_filter(filter_dict: dict, fields: dict[str, AacQueryField | str]) -> dict: +def _sole_dimension_uri(viz: CreatedVisualization) -> str | None: + """URI of the visualization's only dimension, or None when it has zero or several.""" + dim_uris = get_dimension_uri_set(viz) + return next(iter(dim_uris)) if len(dim_uris) == 1 else None + + +def _normalize_ranking_filter( + filter_dict: dict, + fields: dict[str, AacQueryField | str], + sole_dim_uri: str | None = None, +) -> dict: + """Canonicalize a ranking filter so equivalent filters compare equal. + + `attribute` is optional in the AAC schema (gen-ai models it as `NotRequired[str]` / + `str | None`), and when it is omitted AFM ranks over every dimension of the result. For a + single-dimension visualization that is exactly "rank by that one dimension", so an omitted + attribute is filled in with `sole_dim_uri` instead of comparing as an empty string — the + agent and the dataset may legitimately express the same filter either way. + + The substitution is deliberately gated on there being exactly ONE dimension: with two or + more, omitting `attribute` ranks over the dimension *tuple*, which is a different filter, + so those stay strict. Callers pass the sole dimension of the visualization the filter + belongs to, which makes the comparison symmetric — it does not matter which side omitted it. + + Missing, None and "" are all treated as "not specified"; so is a non-string, which + `validate_cross_references` reports separately rather than crashing the comparison. + """ + attr_val = filter_dict.get("attribute") + if not isinstance(attr_val, str) or not attr_val: + dim_uri = sole_dim_uri or "" + else: + dim_uri = _resolve_alias_to_uri(attr_val, fields) + using_val = filter_dict.get("using") entry: dict = { "type": "ranking_filter", - "metric_uri": _resolve_alias_to_uri(filter_dict.get("using", ""), fields), - "dim_uri": _resolve_alias_to_uri(filter_dict.get("attribute", ""), fields), + "metric_uri": _resolve_alias_to_uri(using_val, fields) if isinstance(using_val, str) else "", + "dim_uri": dim_uri, } if "top" in filter_dict: entry["top"] = filter_dict["top"] @@ -127,12 +176,13 @@ def _split_and_normalize_filters(viz: CreatedVisualization) -> tuple[set[str], s ranking_set: set[str] = set() attr_set: set[str] = set() fields = viz.query.fields + sole_dim_uri = _sole_dimension_uri(viz) for filter_dict in viz.query.filter_by.values(): ft = filter_dict.get("type") if ft == "date_filter": date_set.add(json.dumps(_normalize_date_filter(filter_dict, fields), sort_keys=True)) elif ft == "ranking_filter": - ranking_set.add(json.dumps(_normalize_ranking_filter(filter_dict, fields), sort_keys=True)) + ranking_set.add(json.dumps(_normalize_ranking_filter(filter_dict, fields, sole_dim_uri), sort_keys=True)) elif ft == "attribute_filter": attr_set.add(json.dumps(_normalize_attribute_filter(filter_dict, fields), sort_keys=True)) return date_set, ranking_set, attr_set diff --git a/packages/gooddata-eval/tests/test_scoring.py b/packages/gooddata-eval/tests/test_scoring.py index f8bbeb4bb..15f16d383 100644 --- a/packages/gooddata-eval/tests/test_scoring.py +++ b/packages/gooddata-eval/tests/test_scoring.py @@ -64,3 +64,100 @@ def test_check_filters_exact_attribute_match(): actual = _viz(query={"fields": {}, "filter_by": f}) scores = check_filters(expected, actual) assert scores.all_ok is True + + +# --- ranking-filter `attribute` is optional on single-dimension visualizations (QA-28615) --- +# +# `attribute` is NotRequired in the AAC schema and AFM ranks over the whole result when it is +# absent, so on a one-dimension chart "omitted" and "the sole dimension" mean the same filter. +# The comparator used to demand an exact match and failed those as filters_correct=False. + +_M = {"m_sales": {"using": "metric/net_sales"}} +# same URI behind two different aliases — normalization must be alias-independent +_ONE_DIM_A = {**_M, "d_product_id": {"using": "label/product_id"}} +_ONE_DIM_B = {**_M, "d_product": {"using": "label/product_id"}} +_TWO_DIM = {**_M, "d_brand": {"using": "label/product_brand"}, "d_city": {"using": "label/customer_city"}} + + +def _rank_viz(fields, dims, **filter_overrides): + rank = {"type": "ranking_filter", "using": "m_sales", "top": 1, **filter_overrides} + return _viz( + type="bar_chart", + query={"fields": fields, "filter_by": {"f_rank": rank}}, + metrics=["m_sales"], + view_by=dims, + ) + + +def test_ranking_attribute_optional_on_single_dimension_viz(): + """Expected names the attribute, actual omits it — one dimension, so they are equivalent.""" + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + actual = _rank_viz(_ONE_DIM_B, ["d_product"]) + scores = check_filters(expected, actual) + assert scores.ranking_ok is True + assert scores.all_ok is True + + +def test_ranking_attribute_optional_is_symmetric(): + """Reverse direction: the dataset omits the attribute and the agent supplies it.""" + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"]) + actual = _rank_viz(_ONE_DIM_B, ["d_product"], attribute="d_product") + assert check_filters(expected, actual).ranking_ok is True + + +def test_ranking_attribute_none_and_empty_are_the_same_as_omitted(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + for omitted in ({"attribute": None}, {"attribute": ""}): + actual = _rank_viz(_ONE_DIM_B, ["d_product"], **omitted) + assert check_filters(expected, actual).ranking_ok is True, omitted + + +def test_ranking_attribute_still_required_on_multi_dimension_viz(): + """Two dimensions: omitting the attribute ranks over the tuple, so it stays strict.""" + expected = _rank_viz(_TWO_DIM, ["d_brand", "d_city"], attribute="d_brand") + actual = _rank_viz(_TWO_DIM, ["d_brand", "d_city"]) + assert check_filters(expected, actual).ranking_ok is False + + +def test_ranking_attribute_omitted_does_not_mask_a_wrong_top_n(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id", top=1) + actual = _rank_viz(_ONE_DIM_B, ["d_product"], top=5) + assert check_filters(expected, actual).ranking_ok is False + + +def test_ranking_attribute_omitted_does_not_mask_a_wrong_dimension(): + expected = _rank_viz(_ONE_DIM_A, ["d_product_id"], attribute="d_product_id") + actual = _rank_viz(_TWO_DIM, ["d_brand"]) # single dim, but a different one + assert check_filters(expected, actual).ranking_ok is False + + +def test_validate_cross_references_never_raises_on_empty_or_none_uris(): + """Each of these used to raise AttributeError/TypeError instead of returning a score. + + Every case carries its expected verdict: `attribute` is optional so None/"" are valid, + while a non-string attribute or a missing/None `using` must be reported as an error. + Asserting the verdict is what stops a malformed filter from silently passing as valid. + """ + cases = [ + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": None}, True), + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": ""}, True), + ({"type": "ranking_filter", "using": "m_sales", "top": 5, "attribute": []}, False), + ({"type": "ranking_filter", "using": None, "top": 5}, False), + ({"type": "ranking_filter", "top": 5}, False), + ] + for rank, expected_ok in cases: + viz = _viz(query={"fields": _M, "filter_by": {"f_rank": rank}}) + ok, errors = validate_cross_references(viz) + assert isinstance(ok, bool) and isinstance(errors, list), rank + assert ok is expected_ok, rank + assert bool(errors) is not expected_ok, rank + + +def test_validate_cross_references_accepts_omitted_attribute_but_flags_missing_using(): + omitted = _viz(query={"fields": _M, "filter_by": {"f": {"type": "ranking_filter", "using": "m_sales", "top": 5}}}) + assert validate_cross_references(omitted) == (True, []) + + no_using = _viz(query={"fields": _M, "filter_by": {"f": {"type": "ranking_filter", "top": 5}}}) + ok, errors = validate_cross_references(no_using) + assert ok is False + assert "is required" in errors[0] diff --git a/packages/gooddata-eval/tests/test_visualization_evaluator.py b/packages/gooddata-eval/tests/test_visualization_evaluator.py index 7e77b60ab..38d89e844 100644 --- a/packages/gooddata-eval/tests/test_visualization_evaluator.py +++ b/packages/gooddata-eval/tests/test_visualization_evaluator.py @@ -99,3 +99,31 @@ def test_evaluator_skill_not_activated_when_wrong_skill_name(): ) result = ev.evaluate(_item(_expected()), chat) assert result.detail["skill_activated"] is False + + +def _ranked(attribute: str | None, dim_alias: str = "d_q"): + """Single-dimension chart with a top-1 ranking filter, optionally naming the attribute.""" + rank = {"type": "ranking_filter", "using": "m_rev", "top": 1} + if attribute is not None: + rank["attribute"] = attribute + return { + "id": "x", + "type": "column_chart", + "query": { + "fields": {"m_rev": {"using": "metric/revenue"}, dim_alias: {"using": "label/date.quarter"}}, + "filter_by": {"f_rank": rank}, + }, + "metrics": ["m_rev"], + "view_by": [dim_alias], + } + + +def test_evaluator_passes_when_agent_omits_ranking_attribute_on_single_dim_viz(): + """QA-28615: the omitted attribute resolves to the sole dimension, so the case must pass.""" + ev = get_evaluator("visualization") + expected = _ranked("d_q") + actual = _ranked(None, dim_alias="d_quarter") # different alias, attribute omitted + result = ev.evaluate(_item(expected), _chat_result_with(actual)) + assert result.detail["filter_ranking_score"] is True + assert result.detail["filters_correct"] is True + assert result.passed is True