From 97bc0e6ccad8a24befdb6cb8a3228cb4c7d8f902 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 20 Oct 2026 21:40:23 -0600 Subject: [PATCH] fix: give each catalog merge error its own class in both packages The catalog-merge fixture names an error slug for its three failing cases, but both runners accepted any error, so an unrelated failure would pass. Each refusal now has a class (an R condition class, and a ValueError subclass in Python so existing handlers still catch it), and both runners map the slug to the class they expect. --- pkg-py/src/commons/_catalog/__init__.py | 6 ++++++ pkg-py/src/commons/_catalog/_core.py | 18 +++++++++++++++--- pkg-py/tests/test_catalog.py | 12 +++++++++++- pkg-r/NEWS.md | 2 ++ pkg-r/R/catalog.R | 3 +++ pkg-r/tests/testthat/test-catalog-merge.R | 8 +++++++- 6 files changed, 44 insertions(+), 5 deletions(-) diff --git a/pkg-py/src/commons/_catalog/__init__.py b/pkg-py/src/commons/_catalog/__init__.py index c7f12af47..950e85d73 100644 --- a/pkg-py/src/commons/_catalog/__init__.py +++ b/pkg-py/src/commons/_catalog/__init__.py @@ -13,6 +13,9 @@ from . import _databricks, _snowflake from ._core import ( + CatalogAmbiguousColumnError, + CatalogAmbiguousTableError, + CatalogClaimedRelationError, Manifest, MergedDictionary, Relation, @@ -40,7 +43,10 @@ __all__ = [ "CatalogAccessError", + "CatalogAmbiguousColumnError", + "CatalogAmbiguousTableError", "CatalogAuthorizationError", + "CatalogClaimedRelationError", "CatalogSessionChangedError", "CatalogTransientError", "Manifest", diff --git a/pkg-py/src/commons/_catalog/_core.py b/pkg-py/src/commons/_catalog/_core.py index 25b7e804b..94bb928bd 100644 --- a/pkg-py/src/commons/_catalog/_core.py +++ b/pkg-py/src/commons/_catalog/_core.py @@ -40,6 +40,18 @@ _GLOB_SPECIAL = re.compile(r"([][{}()+.^$|\\])") +class CatalogAmbiguousTableError(ValueError): + """An authored table name matches more than one selected relation.""" + + +class CatalogClaimedRelationError(ValueError): + """Two authored tables match the same selected relation.""" + + +class CatalogAmbiguousColumnError(ValueError): + """An authored column name matches more than one discovered column.""" + + @dataclass(frozen=True) class Selector: """One entry of a `tables` selection, naming a relation or a namespace. @@ -442,7 +454,7 @@ def _dictionary_matches( if label is None: continue if label in claimed: - raise ValueError( + raise CatalogClaimedRelationError( f"Authored tables {claimed[label]!r} and {authored_name!r} " f"both match selected relation {label!r}." ) @@ -504,7 +516,7 @@ def has_suffix( def _abort_ambiguous(authored_name: str, candidates: list[str]) -> None: listed = ", ".join(repr(item) for item in sorted(candidates)) - raise ValueError( + raise CatalogAmbiguousTableError( f"Authored table {authored_name!r} matches more than one selected " f"relation: {listed}. Use its fully qualified name in the data " f"dictionary." @@ -562,7 +574,7 @@ def _merge_columns( ] if len(candidates) > 1: listed = ", ".join(repr(name) for name in candidates) - raise ValueError( + raise CatalogAmbiguousColumnError( f"Authored column {authored_name!r} matches more than one " f"discovered column: {listed}." ) diff --git a/pkg-py/tests/test_catalog.py b/pkg-py/tests/test_catalog.py index 699395a49..72e1253da 100644 --- a/pkg-py/tests/test_catalog.py +++ b/pkg-py/tests/test_catalog.py @@ -14,6 +14,9 @@ import pytest from commons._catalog import ( + CatalogAmbiguousColumnError, + CatalogAmbiguousTableError, + CatalogClaimedRelationError, Manifest, MergedDictionary, Relation, @@ -508,13 +511,20 @@ def run_merge_case(case: dict) -> MergedDictionary: ) +_MERGE_ERRORS = { + "ambiguous": CatalogAmbiguousTableError, + "claimed": CatalogClaimedRelationError, + "ambiguous_column": CatalogAmbiguousColumnError, +} + + def test_the_dictionary_merge_matches_the_shared_contract(): cases = load_shared_fixture("catalog-merge")["merge"] assert cases for case in cases: expect = case["expect"] if "error" in expect: - with pytest.raises(ValueError): + with pytest.raises(_MERGE_ERRORS[expect["error"]]): run_merge_case(case) continue diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 079dfcda7..e8c91a033 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -8,6 +8,8 @@ * R code run by `run_r` can no longer send a signal to the application process's ID or to its process group. On Linux, the programs it starts can also no longer leave its process group, so they all stop when the R session ends. +* When a data dictionary cannot be matched to the warehouse catalog, the error now has a condition class you can catch: `commons_catalog_ambiguous_table_error` when an authored table name matches more than one selected relation, `commons_catalog_claimed_relation_error` when two authored tables match the same relation, and `commons_catalog_ambiguous_column_error` when an authored column name matches more than one discovered column. + * The CSS classes on `run_r`'s display are now `commons-run-display`, `commons-run-details`, `commons-run-code`, and `commons-run-plot`, without the `-r`, because the Python package's code tool uses the same classes. App CSS that targets the old `commons-run-r-*` names needs updating. # commons 0.1.1 diff --git a/pkg-r/R/catalog.R b/pkg-r/R/catalog.R index 6d09e00bf..1ee5948a5 100644 --- a/pkg-r/R/catalog.R +++ b/pkg-r/R/catalog.R @@ -444,6 +444,7 @@ catalog_dictionary_matches <- function( cli::cli_abort( "Authored tables {.val {c(other, authored_name)}} both match selected relation {.val {label}}.", + class = "commons_catalog_claimed_relation_error", call = call ) } @@ -530,6 +531,7 @@ catalog_abort_ambiguous_dictionary_table <- function( relation: {.val {matches}}.", "i" = "Use its fully qualified name in the data dictionary." ), + class = "commons_catalog_ambiguous_table_error", call = call ) } @@ -593,6 +595,7 @@ catalog_merge_dictionary_columns <- function( cli::cli_abort( "Authored column {.val {authored_name}} matches more than one discovered column: {.val {names(out)[candidates]}}.", + class = "commons_catalog_ambiguous_column_error", call = call ) } diff --git a/pkg-r/tests/testthat/test-catalog-merge.R b/pkg-r/tests/testthat/test-catalog-merge.R index 53bf898bc..710a3bf67 100644 --- a/pkg-r/tests/testthat/test-catalog-merge.R +++ b/pkg-r/tests/testthat/test-catalog-merge.R @@ -30,6 +30,12 @@ test_that("exclusion globs match the shared contract", { } }) +merge_error_class <- c( + ambiguous = "commons_catalog_ambiguous_table_error", + claimed = "commons_catalog_claimed_relation_error", + ambiguous_column = "commons_catalog_ambiguous_column_error" +) + test_that("the dictionary merge matches the shared contract", { cases <- shared_fixture("catalog-merge")$merge expect_gt(length(cases), 0L) @@ -70,7 +76,7 @@ test_that("the dictionary merge matches the shared contract", { ) } if (!is.null(case$expect$error)) { - expect_error(run()) + expect_error(run(), class = merge_error_class[[case$expect$error]]) next } merged <- run()