diff --git a/doc/specs/2026-05-26-routing-error-design.md b/doc/specs/2026-05-26-routing-error-design.md new file mode 100644 index 0000000..1a61f2b --- /dev/null +++ b/doc/specs/2026-05-26-routing-error-design.md @@ -0,0 +1,224 @@ +# Design: `RoutingError` Typed Exception + +**Date:** 2026-05-26 +**Status:** Approved +**Related todo:** `doc/todo/error-routing-typed-exception.md` + +--- + +## Problem + +When `upsert`, `add`, or `merge` tries to write through a key path and an +intermediate node is not a mapping (it is a scalar or a list), yamlpatch +raises a `PatchError` whose message contains one of three Rust-layer +substrings: + +| Rust substring | Triggering scenario | +|---|---| +| `"non-mapping route"` | `Op.add` with a scalar or list as the parent | +| `"expected mapping containing key"` | `Op.merge_into` with a list as the parent | +| `"unexpected node"` | `Op.merge_into` with a scalar as the parent | + +There is currently no typed exception for this condition. Callers who need +to distinguish a routing failure from other `PatchError`s must match raw +strings — fragile and not part of the public API contract. + +`merge()` already works around this by catching `_PatchErrorKind.UNEXPECTED_NODE` +and re-raising as `NodeTypeError`. That is an approximation: routing errors +and target-type errors are distinct failure modes and should have distinct +exception types. + +--- + +## Goals + +- Introduce `RoutingError` so callers can `except yamltrip.RoutingError` + without any string matching. +- Confine all three routing-related Rust substrings to the existing + `_PatchErrorKind` / `_classify_patch_error` / `_is_routing_error` layer. +- Produce human-readable messages (`"Route passes through a non-mapping node + at a > b"`) rather than leaking Rust internals. +- Fix `merge()` to raise `RoutingError` instead of `NodeTypeError` for + routing failures (breaking change; `NodeTypeError` was an approximation). + +--- + +## Exception Hierarchy + +```python +# errors.py — added after KeyMissingError, before NodeTypeError +class RoutingError(PatchError): + """Raised when a key path passes through a non-mapping node.""" +``` + +`RoutingError` is a direct subclass of `PatchError`. It is **not** a subclass +of `NodeTypeError`: routing errors and target-type errors are semantically +distinct, and `NodeTypeError` carries a `TypeError` mixin that is not +appropriate here. + +`RoutingError` is exported in `__init__.py` alongside the other `PatchError` +subclasses. + +--- + +## Detection Layer (`_PatchErrorKind`, `_classify_patch_error`, `_is_routing_error`) + +Two new members are added to `_PatchErrorKind` (after `BLOCK_SEQUENCE_EXPECTED`, +before `UNEXPECTED_NODE`): + +```python +NON_MAPPING_ROUTE = "non-mapping route" +EXPECTED_MAPPING = "expected mapping containing key" +``` + +`UNEXPECTED_NODE = "unexpected node"` already exists and is also a routing +error in all contexts where yamlpatch emits it. + +`_classify_patch_error` gains two new branches (inserted before the +`UNEXPECTED_NODE` check): + +```python +if _PatchErrorKind.NON_MAPPING_ROUTE.value in msg: + return _PatchErrorKind.NON_MAPPING_ROUTE +if _PatchErrorKind.EXPECTED_MAPPING.value in msg: + return _PatchErrorKind.EXPECTED_MAPPING +``` + +A new module-level helper (placed after `_classify_patch_error`) unifies the +three routing kinds: + +```python +def _is_routing_error(kind: _PatchErrorKind) -> bool: + """Return True if kind represents a routing failure.""" + return kind in ( + _PatchErrorKind.NON_MAPPING_ROUTE, + _PatchErrorKind.EXPECTED_MAPPING, + _PatchErrorKind.UNEXPECTED_NODE, + ) +``` + +`_is_routing_error` is the single place in the codebase that knows which Rust +error substrings constitute a routing failure. + +--- + +## Raise Sites (Option A: catch in private helpers) + +### `_create_at(parent_keys, child_keys, value)` + +`_create_at` is the common creation path for both `upsert` and `add` (when the +document is empty). Each `_apply_patches` call that uses `parent_keys` as the +route is wrapped: + +```python +try: + return self._apply_patches([patch]) # or [add_patch, replace_patch] +except PatchError as e: + if _is_routing_error(_classify_patch_error(e)): + msg = f"Route passes through a non-mapping node at {format_path(parent_keys)}" + raise RoutingError(msg) from None + raise +``` + +The bootstrap branch (`not parent_keys and self._is_empty_document()`) creates +a `Document` from scratch and never calls `_apply_patches`, so no catch is +needed there. + +### `add(*keys, key, value)` + +The non-empty-document path calls `_apply_patches` directly (not via +`_create_at`). That call is wrapped identically, using `keys` as the path: + +```python +try: + return self._apply_patches([patch]) +except PatchError as e: + if _is_routing_error(_classify_patch_error(e)): + msg = f"Route passes through a non-mapping node at {format_path(keys)}" + raise RoutingError(msg) from None + raise +``` + +### `merge()` + +The existing `_classify_patch_error(e) == _PatchErrorKind.UNEXPECTED_NODE` +block is removed entirely. `upsert` (via `_create_at`) now raises `RoutingError` +directly with a good message; `merge()` just lets it propagate. + +The ancestor-finding loop in `merge()` (previously used to construct the +`NodeTypeError` message) is no longer needed. + +### Methods that get `RoutingError` for free + +`upsert`, `sync`, and any future method that calls `_create_at` raise +`RoutingError` without any further changes. + +--- + +## Public API + +### `errors.py` + +```python +class RoutingError(PatchError): + """Raised when a key path passes through a non-mapping node.""" +``` + +### `__init__.py` + +`RoutingError` added to the import from `.errors` and to `__all__`, placed +adjacent to the other `PatchError` subclasses. + +### `editor.py` + +No changes required. `RoutingError` propagates through `Editor`'s delegation +layer automatically. + +--- + +## Tests + +### `TestPatchErrorStringPins` additions (in `test_edge_cases.py`) + +Two new substring pin tests with their `test_classify_*` counterparts: + +- `test_non_mapping_route_substring` — triggers `Op.add` on a scalar parent, + asserts `_PatchErrorKind.NON_MAPPING_ROUTE.value in msg` +- `test_expected_mapping_substring` — triggers `Op.merge_into` on a list parent, + asserts `_PatchErrorKind.EXPECTED_MAPPING.value in msg` +- `test_classify_non_mapping_route` — verifies classifier round-trips to `NON_MAPPING_ROUTE` +- `test_classify_expected_mapping` — verifies classifier round-trips to `EXPECTED_MAPPING` + +### New `TestRoutingError` class (in `test_errors.py` or `test_edge_cases.py`) + +Behaviour tests that verify the public exception type and message: + +- `upsert("a", "b", value=...)` on `a: scalar` raises `RoutingError` with `"at a"` in message +- `add("a", key="b", value=...)` on `a: scalar` raises `RoutingError` +- `merge("a", "b", value=...)` on `a: scalar` raises `RoutingError` (not `NodeTypeError`) +- `isinstance(err, PatchError)` — confirms `RoutingError` IS-A `PatchError` +- `upsert("a", "b", value=...)` on `items: [x, y]` raises `RoutingError` + (covers the `EXPECTED_MAPPING` path) + +### Existing tests + +`NodeTypeError` tests for `append`, `insert`, `extend_list` on wrong-type +targets are unaffected. Any existing test that asserts `NodeTypeError` from +`merge()` for routing failures must be updated to expect `RoutingError`. + +### Public API / stubs + +`test_public_api.py` or `test_stubs.py` must be updated to include `RoutingError` +in the expected exports. + +--- + +## Breaking Changes + +| Method | Old exception | New exception | +|---|---|---| +| `merge()` routing through non-mapping | `NodeTypeError` | `RoutingError` | + +All other methods (`upsert`, `add`, `sync`) previously raised raw `PatchError` +for routing failures; they now raise the more specific `RoutingError`, which is +a `PatchError` subclass — `except PatchError` continues to work. diff --git a/src/yamltrip/__init__.py b/src/yamltrip/__init__.py index eb80f05..6ee9278 100644 --- a/src/yamltrip/__init__.py +++ b/src/yamltrip/__init__.py @@ -14,6 +14,7 @@ ParseError, PatchError, QueryError, + RoutingError, YAMLTripError, ) @@ -52,6 +53,7 @@ def edit(path: str | Path) -> Editor: "PatchError", "QueryError", "Route", + "RoutingError", "YAMLTripError", "edit", "load", diff --git a/src/yamltrip/document.py b/src/yamltrip/document.py index 959b1b2..555d598 100644 --- a/src/yamltrip/document.py +++ b/src/yamltrip/document.py @@ -26,6 +26,7 @@ ParseError, PatchError, QueryError, + RoutingError, ) if TYPE_CHECKING: @@ -197,7 +198,13 @@ def add(self, *keys: KeyPart, key: str, value: Any) -> Document: route = _make_route(keys) op = Op.add(key, value) patch = Patch(route=route, operation=op) - return self._apply_patches([patch]) + try: + return self._apply_patches([patch]) + except PatchError as e: + if _is_routing_error(_classify_patch_error(e)): + msg = f"Route passes through a non-mapping node at {format_path(keys)}" + raise RoutingError(msg) from None + raise def upsert(self, *keys: KeyPart, value: Any) -> Document: """Replace if exists, create (with intermediate mappings) if not.""" @@ -253,24 +260,30 @@ def _create_at( for k in reversed(child_keys[1:]): nested_value = {k: nested_value} route = _make_route(parent_keys) - if isinstance(nested_value, dict) and any( - isinstance(v, (dict, list, tuple)) for v in nested_value.values() - ): - # Op.merge_into is scoped to flat mappings (uniform indent); for - # nested values, add a placeholder then replace via complex-replace - # which preserves relative indentation. - add_op = Op.add(first_key, None) - add_patch = Patch(route=route, operation=add_op) - replace_route = _make_route((*parent_keys, first_key)) - replace_op = Op.replace(nested_value) - replace_patch = Patch(route=replace_route, operation=replace_op) - return self._apply_patches([add_patch, replace_patch]) - elif isinstance(nested_value, dict): - op = Op.merge_into(first_key, nested_value) - else: - op = Op.add(first_key, nested_value) - patch = Patch(route=route, operation=op) - return self._apply_patches([patch]) + try: + if isinstance(nested_value, dict) and any( + isinstance(v, (dict, list, tuple)) for v in nested_value.values() + ): + # Op.merge_into is scoped to flat mappings (uniform indent); for + # nested values, add a placeholder then replace via complex-replace + # which preserves relative indentation. + add_op = Op.add(first_key, None) + add_patch = Patch(route=route, operation=add_op) + replace_route = _make_route((*parent_keys, first_key)) + replace_op = Op.replace(nested_value) + replace_patch = Patch(route=replace_route, operation=replace_op) + return self._apply_patches([add_patch, replace_patch]) + elif isinstance(nested_value, dict): + op = Op.merge_into(first_key, nested_value) + else: + op = Op.add(first_key, nested_value) + patch = Patch(route=route, operation=op) + return self._apply_patches([patch]) + except PatchError as e: + if _is_routing_error(_classify_patch_error(e)): + msg = f"Route passes through a non-mapping node at {format_path(parent_keys)}" + raise RoutingError(msg) from None + raise def _is_empty_document(self) -> bool: """True if the document has no root data node.""" @@ -500,20 +513,7 @@ def merge(self, *keys: KeyPart, value: Any) -> Document: if normalized: route = _make_route(normalized) if not self._core_doc.query_exists(route): - try: - return self.upsert(*normalized, value=value) - except PatchError as e: - if _classify_patch_error(e) == _PatchErrorKind.UNEXPECTED_NODE: - # Find deepest existing ancestor to report - failing = normalized - for i in range(len(normalized), 0, -1): - sub = normalized[:i] - if self._core_doc.query_exists(_make_route(sub)): - failing = sub - break - msg = f"Value at {format_path(failing)} is not a mapping" - raise NodeTypeError(msg) from None - raise + return self.upsert(*normalized, value=value) # Get current value and diff try: @@ -640,6 +640,8 @@ class _PatchErrorKind(enum.Enum): FLOW_SEQUENCE = "flow sequence" NOT_A_SEQUENCE = "only permitted against sequence" BLOCK_SEQUENCE_EXPECTED = "expected BlockSequence" + NON_MAPPING_ROUTE = "non-mapping route" + EXPECTED_MAPPING = "expected mapping containing key" UNEXPECTED_NODE = "unexpected node" UNKNOWN = "" @@ -649,14 +651,30 @@ def _classify_patch_error(err: PatchError) -> _PatchErrorKind: All yamlpatch error-message substrings are confined here so that callers can branch on the enum rather than matching raw strings. + + The tuple is ordered explicitly. More specific substrings must appear + before any that are a prefix of them (e.g. EXPECTED_MAPPING before a + hypothetical EXPECTED) so that substring matching is unambiguous. """ msg = str(err) - if _PatchErrorKind.FLOW_SEQUENCE.value in msg: - return _PatchErrorKind.FLOW_SEQUENCE - if _PatchErrorKind.NOT_A_SEQUENCE.value in msg: - return _PatchErrorKind.NOT_A_SEQUENCE - if _PatchErrorKind.BLOCK_SEQUENCE_EXPECTED.value in msg: - return _PatchErrorKind.BLOCK_SEQUENCE_EXPECTED - if _PatchErrorKind.UNEXPECTED_NODE.value in msg: - return _PatchErrorKind.UNEXPECTED_NODE + _ordered = ( + _PatchErrorKind.FLOW_SEQUENCE, + _PatchErrorKind.NOT_A_SEQUENCE, + _PatchErrorKind.BLOCK_SEQUENCE_EXPECTED, + _PatchErrorKind.NON_MAPPING_ROUTE, + _PatchErrorKind.EXPECTED_MAPPING, + _PatchErrorKind.UNEXPECTED_NODE, + ) + for kind in _ordered: + if kind.value in msg: + return kind return _PatchErrorKind.UNKNOWN + + +def _is_routing_error(kind: _PatchErrorKind) -> bool: + """Return True if kind represents a routing failure through a non-mapping node.""" + return kind in ( + _PatchErrorKind.NON_MAPPING_ROUTE, + _PatchErrorKind.EXPECTED_MAPPING, + _PatchErrorKind.UNEXPECTED_NODE, + ) diff --git a/src/yamltrip/errors.py b/src/yamltrip/errors.py index 9a6f14b..6b722f7 100644 --- a/src/yamltrip/errors.py +++ b/src/yamltrip/errors.py @@ -25,5 +25,9 @@ class KeyMissingError(PatchError): """Raised by replace() when the key doesn't exist.""" +class RoutingError(PatchError): + """Raised when a key path passes through a non-mapping node.""" + + class NodeTypeError(PatchError, TypeError): """Raised when a node is not the expected type for the operation.""" diff --git a/tests/test_edge_cases.py b/tests/test_edge_cases.py index 655edd2..6459d04 100644 --- a/tests/test_edge_cases.py +++ b/tests/test_edge_cases.py @@ -178,6 +178,32 @@ def test_classify_unexpected_node(self): msg = self._raw_error(doc, "name", op=Op.merge_into("nested", {"foo": "bar"})) assert _classify_patch_error(PatchError(msg)) == _PatchErrorKind.UNEXPECTED_NODE + def test_non_mapping_route_substring(self): + """Op.add on a scalar node raises an error containing 'non-mapping route'.""" + doc = Document("name: scalar\n") + msg = self._raw_error(doc, "name", op=Op.add("child", "val")) + assert _PatchErrorKind.NON_MAPPING_ROUTE.value in msg + + def test_classify_non_mapping_route(self): + doc = Document("name: scalar\n") + msg = self._raw_error(doc, "name", op=Op.add("child", "val")) + assert ( + _classify_patch_error(PatchError(msg)) == _PatchErrorKind.NON_MAPPING_ROUTE + ) + + def test_expected_mapping_substring(self): + """Op.merge_into on a list node raises an error containing 'expected mapping containing key'.""" + doc = Document("items:\n - a\n") + msg = self._raw_error(doc, "items", op=Op.merge_into("k", {"a": 1})) + assert _PatchErrorKind.EXPECTED_MAPPING.value in msg + + def test_classify_expected_mapping(self): + doc = Document("items:\n - a\n") + msg = self._raw_error(doc, "items", op=Op.merge_into("k", {"a": 1})) + assert ( + _classify_patch_error(PatchError(msg)) == _PatchErrorKind.EXPECTED_MAPPING + ) + def test_classify_unknown(self): assert ( _classify_patch_error(PatchError("some unrelated error")) diff --git a/tests/test_errors.py b/tests/test_errors.py index aa81d9b..ec3df30 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -1,5 +1,6 @@ import pytest +from yamltrip import Document from yamltrip.errors import ( KeyExistsError, KeyMissingError, @@ -7,6 +8,7 @@ ParseError, PatchError, QueryError, + RoutingError, YAMLTripError, ) @@ -38,3 +40,55 @@ def test_raise_and_catch_base(self): msg = "key already exists" with pytest.raises(YAMLTripError, match=msg): raise KeyExistsError(msg) + + +class TestRoutingErrorHierarchy: + def test_routing_error_is_patch_error(self): + assert issubclass(RoutingError, PatchError) + + def test_routing_error_is_not_type_error(self): + assert not issubclass(RoutingError, TypeError) + + def test_routing_error_is_not_node_type_error(self): + assert not issubclass(RoutingError, NodeTypeError) + + def test_raise_and_catch_as_patch_error(self): + msg = "route through scalar" + with pytest.raises(PatchError): + raise RoutingError(msg) + + def test_message(self): + err = RoutingError("Route passes through a non-mapping node at a") + assert "a" in str(err) + + +class TestRoutingErrorBehaviour: + def test_upsert_through_scalar_raises_routing_error(self): + doc = Document("a: scalar\n") + with pytest.raises(RoutingError, match="non-mapping node at a"): + doc.upsert("a", "b", value="foo") + + def test_upsert_through_list_raises_routing_error(self): + doc = Document("items:\n - x\n") + with pytest.raises(RoutingError, match="non-mapping node at items"): + doc.upsert("items", "b", value="foo") + + def test_add_through_scalar_raises_routing_error(self): + doc = Document("name: scalar\n") + with pytest.raises(RoutingError, match="non-mapping node at name"): + doc.add("name", key="child", value="foo") + + def test_routing_error_is_catchable_as_patch_error(self): + doc = Document("a: scalar\n") + with pytest.raises(PatchError): + doc.upsert("a", "b", value="foo") + + def test_add_through_list_raises_routing_error(self): + doc = Document("items:\n - a\n") + with pytest.raises(RoutingError, match="non-mapping node at items"): + doc.add("items", key="child", value="foo") + + def test_upsert_scalar_root_raises_routing_error(self): + doc = Document("just_a_scalar\n") + with pytest.raises(RoutingError, match=r"non-mapping node at "): + doc.upsert("y", value="v") diff --git a/tests/test_merge.py b/tests/test_merge.py index 9e27d2f..2f5abbf 100644 --- a/tests/test_merge.py +++ b/tests/test_merge.py @@ -2,7 +2,7 @@ from yamltrip import edit from yamltrip.document import Document -from yamltrip.errors import NodeTypeError +from yamltrip.errors import RoutingError class TestMergeMapping: @@ -108,7 +108,13 @@ def test_editor_merge(self, tmp_path): class TestMergeErrors: def test_merge_through_scalar_raises(self): - """Merging through a scalar path raises NodeTypeError.""" + """Merging through a scalar path raises RoutingError.""" doc = Document("a:\n b: 1\n") - with pytest.raises(NodeTypeError, match="Value at a > b"): + with pytest.raises(RoutingError, match="non-mapping node at a > b"): doc.merge("a", "b", "c", value={"x": 1}) + + def test_merge_through_list_raises(self): + """Merging through a list node raises RoutingError.""" + doc = Document("items:\n - a\n - b\n") + with pytest.raises(RoutingError, match="non-mapping node at items"): + doc.merge("items", "child", value={"x": 1}) diff --git a/tests/test_public_api.py b/tests/test_public_api.py index 035e610..4ddeff1 100644 --- a/tests/test_public_api.py +++ b/tests/test_public_api.py @@ -39,6 +39,7 @@ def test_error_classes(self): assert issubclass(yamltrip.PatchError, yamltrip.YAMLTripError) assert issubclass(yamltrip.KeyExistsError, yamltrip.PatchError) assert issubclass(yamltrip.KeyMissingError, yamltrip.PatchError) + assert issubclass(yamltrip.RoutingError, yamltrip.PatchError) def test_core_types(self): assert yamltrip.Location is not None diff --git a/tests/test_sync.py b/tests/test_sync.py index fdde31d..a03f762 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -1,5 +1,8 @@ +import pytest + from yamltrip.document import Document from yamltrip.editor import Editor +from yamltrip.errors import RoutingError class TestSyncMappingAddKey: @@ -300,3 +303,11 @@ def test_multi_level_sync(self): doc2 = doc.sync("ci", value={"autofix_prs": False, "skip": ["codespell"]}) assert doc2["ci", "autofix_prs"] is False assert doc2["ci", "skip"] == ["codespell"] + + +class TestSyncRoutingError: + def test_sync_through_scalar_raises_routing_error(self): + """sync() through a scalar node raises RoutingError via upsert delegation.""" + doc = Document("a: scalar\n") + with pytest.raises(RoutingError, match="non-mapping node at a"): + doc.sync("a", "b", value={"x": 1})