Merge pull request #1707 from gooddata/fix-test

fix(gooddata-eval): make ranking attribute optional on 1-dim viz

Author: henrynguyengooddata

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

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]

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