From 4a10af2169fe3c4b9cbf47ea0841e8196a304bd2 Mon Sep 17 00:00:00 2001 From: chienyuanchang Date: Wed, 8 Jul 2026 14:54:52 -0700 Subject: [PATCH] fix(contentunderstanding): --schema-dir picks natural version max MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Copilot review comment from the sibling .NET PR (azure-sdk-for-net#60394, second-round review). The bug came from Python and was faithfully ported to Java (#49672) / JS (#39137) / .NET (#60394) — this PR fixes the source-of-truth so the four languages stay in lockstep. .NET is already fixed (commit 61bda1abd16), JS is queued (commit d2a463cde18 on the open PR), Java is queued (commit 7b07a7564d3 on the open PR). Before: _discover_inner_from_dir did 'sorted(schema_dir.glob("*.json"))' and took the last element as the 'newest version'. That breaks the moment any alias reaches double-digit versions: invoice_v9.json vs invoice_v10.json → sorted lexicographically, '1' < '9' at position 9, so invoice_v10 sorts BEFORE invoice_v9, 'alphabetical last' picks invoice_v9, and the tool silently loads the wrong (older) schema. After: sort by a 'version key' tuple, highest wins: - group 0 → bare '.json' (no suffix; oldest) - group 1 → '_v.json' or (numeric suffix; '_.json' sorted by N) - group 2 → any other '_.json' (lex tiebreaker) So invoice_v10 beats invoice_v9 beats invoice, and 'invoice_draft' still beats bare 'invoice.json' but loses to any versioned file. New _version_sort_key helper is module-level (leading-underscore is just 'implementation detail', not test-invisible) so tests can pin the key extractor directly. Test coverage (+11 pytest cases in test_skills_classify_route_router.py): - test_version_sort_key_bare_alias_returns_group_zero - test_version_sort_key_v_prefixed_numeric_returns_group_one (v10 > v9) - test_version_sort_key_bare_numeric_returns_group_one (_42) - test_version_sort_key_non_numeric_suffix_returns_group_two - test_discover_inner_from_dir_resolves_exact_match_stem - test_discover_inner_from_dir_picks_natural_version_max_not_alphabetical (the exact regression the .NET Copilot review flagged) - test_discover_inner_from_dir_prefers_versioned_over_bare_alias - test_discover_inner_from_dir_skips_prebuilt_aliases - test_discover_inner_from_dir_skips_categories_without_analyzer_id - test_discover_inner_from_dir_missing_aliases_raises - test_discover_inner_from_dir_unrelated_json_files_ignored - test_discover_inner_from_dir_non_existent_dir_raises CHANGELOG.md: added a 'Bugs Fixed' entry to the existing 1.2.0b3 (Unreleased) section (no version bump needed — b3 has not shipped yet). Verified: pytest tests/test_skills_classify_route_router.py → 19/19 passing (was 8; +11 new cases). --- .../scripts/create_and_test_router.py | 40 ++++- .../CHANGELOG.md | 7 + .../test_skills_classify_route_router.py | 155 ++++++++++++++++++ 3 files changed, 198 insertions(+), 4 deletions(-) diff --git a/sdk/contentunderstanding/azure-ai-contentunderstanding/.github/skills/cu-sdk-author-analyzer-classify-route/scripts/create_and_test_router.py b/sdk/contentunderstanding/azure-ai-contentunderstanding/.github/skills/cu-sdk-author-analyzer-classify-route/scripts/create_and_test_router.py index 18cb4e2af131..f2289b1f6d32 100644 --- a/sdk/contentunderstanding/azure-ai-contentunderstanding/.github/skills/cu-sdk-author-analyzer-classify-route/scripts/create_and_test_router.py +++ b/sdk/contentunderstanding/azure-ai-contentunderstanding/.github/skills/cu-sdk-author-analyzer-classify-route/scripts/create_and_test_router.py @@ -49,6 +49,7 @@ import argparse import importlib.util import json +import re import sys import time from pathlib import Path @@ -123,6 +124,34 @@ def _parse_inner_arg(values: List[str]) -> Dict[str, Path]: return result +def _version_sort_key(stem: str, alias: str) -> Tuple[int, int, str]: + """Sort key for ``[_suffix]`` filename stems. Higher tuple = newer. + + * Group 0: bare ```` (no suffix) — oldest baseline. + * Group 1: numeric suffix ``_v`` or ``_`` — sorted + by N as an integer, so ``v10`` beats ``v9``. + * Group 2: any other suffix ``_`` — lexicographic tiebreaker. + + Public (module-level) so tests can exercise the key extractor directly. + """ + + if stem == alias: + return (0, 0, "") + # We only get here for stems that already matched the ``_`` filter, + # so the underscore is guaranteed to be at index len(alias). + suffix = stem[len(alias) + 1:] + m = _VERSION_SUFFIX.fullmatch(suffix) + if m is not None: + try: + return (1, int(m.group(1)), "") + except ValueError: + pass # fall through to group 2 + return (2, 0, suffix) + + +_VERSION_SUFFIX = re.compile(r"v?(\d+)") + + def _discover_inner_from_dir( outer_schema: Dict[str, Any], schema_dir: Path ) -> Dict[str, Path]: @@ -131,8 +160,9 @@ def _discover_inner_from_dir( For every category in the outer schema whose ``analyzerId`` is a non- prebuilt alias, find a matching JSON file in ``schema_dir``. The matching rule is: filename stem == alias, or stem startswith - ``_`` (picks the alphabetically last match, so ``invoice_v2.json`` - wins over ``invoice_v1.json``). + ``_``. When multiple files match, pick the highest version — so + ``invoice_v10.json`` beats ``invoice_v9.json`` (plain alphabetical + sort broke as soon as version numbers hit two digits). """ if not schema_dir.is_dir(): @@ -148,7 +178,7 @@ def _discover_inner_from_dir( continue aliases.append(alias) - json_files = sorted(schema_dir.glob("*.json")) + json_files = list(schema_dir.glob("*.json")) resolved: Dict[str, Path] = {} missing: List[str] = [] for alias in aliases: @@ -159,7 +189,9 @@ def _discover_inner_from_dir( if not matches: missing.append(alias) continue - resolved[alias] = matches[-1] # alphabetically last → newest version + # Highest version key wins: v10 > v9 > bare baseline. + matches.sort(key=lambda p: _version_sort_key(p.stem, alias)) + resolved[alias] = matches[-1] if missing: raise SystemExit( diff --git a/sdk/contentunderstanding/azure-ai-contentunderstanding/CHANGELOG.md b/sdk/contentunderstanding/azure-ai-contentunderstanding/CHANGELOG.md index ddc87ad8535c..d8cd7f478961 100644 --- a/sdk/contentunderstanding/azure-ai-contentunderstanding/CHANGELOG.md +++ b/sdk/contentunderstanding/azure-ai-contentunderstanding/CHANGELOG.md @@ -2,6 +2,13 @@ ## 1.2.0b3 (Unreleased) +### Bugs Fixed + +- `cu-sdk-author-analyzer-classify-route`: `--schema-dir` now picks the + highest-numbered version when multiple files match an alias — so + `invoice_v10.json` beats `invoice_v9.json` (previous alphabetical + sort silently picked v9 because `'1' < '9'` char-by-char). + ### Other Changes - Added GitHub Copilot skills under `.github/skills/` to help users diff --git a/sdk/contentunderstanding/azure-ai-contentunderstanding/tests/test_skills_classify_route_router.py b/sdk/contentunderstanding/azure-ai-contentunderstanding/tests/test_skills_classify_route_router.py index 9125e975cdef..c39e4039ef0c 100644 --- a/sdk/contentunderstanding/azure-ai-contentunderstanding/tests/test_skills_classify_route_router.py +++ b/sdk/contentunderstanding/azure-ai-contentunderstanding/tests/test_skills_classify_route_router.py @@ -178,5 +178,160 @@ def test_parse_inner_arg_rejects_missing_equals(): router._parse_inner_arg(["invoice/tmp/inv.json"]) +# --------------------------------------------------------------------------- +# _version_sort_key — pure key extractor +# --------------------------------------------------------------------------- + + +def test_version_sort_key_bare_alias_returns_group_zero(): + key = router._version_sort_key("invoice", "invoice") + assert key == (0, 0, "") + + +def test_version_sort_key_v_prefixed_numeric_returns_group_one(): + v9 = router._version_sort_key("invoice_v9", "invoice") + v10 = router._version_sort_key("invoice_v10", "invoice") + assert v9 == (1, 9, "") + assert v10 == (1, 10, "") + # The whole point of the fix. + assert v10 > v9, "v10 must sort higher than v9 by numeric version" + + +def test_version_sort_key_bare_numeric_returns_group_one(): + """`_` (no `v` prefix) is also a numeric version.""" + assert router._version_sort_key("invoice_42", "invoice") == (1, 42, "") + + +def test_version_sort_key_non_numeric_suffix_returns_group_two(): + assert router._version_sort_key("invoice_draft", "invoice") == (2, 0, "draft") + + +# --------------------------------------------------------------------------- +# _discover_inner_from_dir — filesystem-touching resolution +# --------------------------------------------------------------------------- + + +def _outer_with_aliases(*aliases): + """Build an outer classifier schema whose category_i has analyzerId=aliases[i]. + + ``None`` in the list produces a category with no ``analyzerId`` at all + (classification-only bucket). + """ + categories = {} + for i, alias in enumerate(aliases): + entry = {"description": f"d{i}"} + if alias is not None: + entry["analyzerId"] = alias + categories[f"cat_{i}"] = entry + return { + "baseAnalyzerId": "prebuilt-document", + "config": {"enableSegment": True, "contentCategories": categories}, + } + + +def test_discover_inner_from_dir_resolves_exact_match_stem(tmp_path): + (tmp_path / "invoice.json").write_text("{}") + (tmp_path / "bank_statement.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice", "bank_statement"), tmp_path + ) + + assert resolved == { + "invoice": tmp_path / "invoice.json", + "bank_statement": tmp_path / "bank_statement.json", + } + + +def test_discover_inner_from_dir_picks_natural_version_max_not_alphabetical(tmp_path): + """Regression: the previous impl did ``sorted(schema_dir.glob("*.json"))`` + and took the last element as "newest". But ``'1' < '9'`` char-by-char, + so ``invoice_v10.json`` sorted BEFORE ``invoice_v9.json`` and + "alphabetical last" silently picked v9 — the older schema. Copilot + flagged this on the sibling .NET PR (azure-sdk-for-net#60394); the + natural version sort fix brings all four languages back in lockstep. + """ + for name in ("invoice_v1", "invoice_v2", "invoice_v9", "invoice_v10"): + (tmp_path / f"{name}.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice"), tmp_path + ) + + assert resolved == {"invoice": tmp_path / "invoice_v10.json"}, ( + "v10 must beat v9 (natural version order, not alphabetical)" + ) + + +def test_discover_inner_from_dir_prefers_versioned_over_bare_alias(tmp_path): + """A bare ``.json`` is group 0 (baseline); any versioned file + is group 1 or 2 and beats the baseline as "newer". + """ + (tmp_path / "invoice.json").write_text("{}") + (tmp_path / "invoice_v1.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice"), tmp_path + ) + + assert resolved == {"invoice": tmp_path / "invoice_v1.json"} + + +def test_discover_inner_from_dir_skips_prebuilt_aliases(tmp_path): + """``prebuilt-invoice`` is a service alias; no local file required.""" + (tmp_path / "invoice.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice", "prebuilt-invoice"), tmp_path + ) + + assert list(resolved) == ["invoice"] + + +def test_discover_inner_from_dir_skips_categories_without_analyzer_id(tmp_path): + """A category without ``analyzerId`` is a classification-only bucket.""" + (tmp_path / "invoice.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice", None), tmp_path + ) + + assert list(resolved) == ["invoice"] + + +def test_discover_inner_from_dir_missing_aliases_raises(tmp_path): + """Every unresolved alias must appear in the SystemExit message.""" + (tmp_path / "invoice.json").write_text("{}") + + with pytest.raises(SystemExit) as excinfo: + router._discover_inner_from_dir( + _outer_with_aliases("invoice", "bank_statement", "loan_application"), + tmp_path, + ) + + msg = str(excinfo.value) + assert "bank_statement" in msg + assert "loan_application" in msg + + +def test_discover_inner_from_dir_unrelated_json_files_ignored(tmp_path): + (tmp_path / "invoice.json").write_text("{}") + (tmp_path / "notes.json").write_text("{}") + (tmp_path / "settings.json").write_text("{}") + + resolved = router._discover_inner_from_dir( + _outer_with_aliases("invoice"), tmp_path + ) + + assert resolved == {"invoice": tmp_path / "invoice.json"} + + +def test_discover_inner_from_dir_non_existent_dir_raises(tmp_path): + missing = tmp_path / "definitely-not-there" + with pytest.raises(SystemExit) as excinfo: + router._discover_inner_from_dir(_outer_with_aliases("invoice"), missing) + assert "--schema-dir is not a directory" in str(excinfo.value) + + if __name__ == "__main__": sys.exit(pytest.main([__file__, "-v"]))