From 095371a4e5a5c0b199f98898181864187236e92d Mon Sep 17 00:00:00 2001 From: Simon AURA--SCUSSEL Date: Sun, 17 May 2026 11:47:44 +0200 Subject: [PATCH 1/2] fix: add labels to NodeNotFoundError for clearer messaging The error message now shows Branch:, Kind:, and Identifier: labels instead of raw pipe-separated values, helping users understand which branch the lookup failed on. Fixes #5514 --- infrahub_sdk/exceptions.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/infrahub_sdk/exceptions.py b/infrahub_sdk/exceptions.py index b1f3e465..fefb9af9 100644 --- a/infrahub_sdk/exceptions.py +++ b/infrahub_sdk/exceptions.py @@ -84,9 +84,16 @@ def __init__( super().__init__(self.message) def __str__(self) -> str: + detail_parts = [] + if self.branch_name: + detail_parts.append(f"Branch: {self.branch_name}") + if self.node_type: + detail_parts.append(f"Kind: {self.node_type}") + detail_parts.append(f"Identifier: {self.identifier}") + detail = " | ".join(detail_parts) return f""" {self.message} - {self.branch_name} | {self.node_type} | {self.identifier} + {detail} """ From 1ab16ae8a91c65bd4993d024a745604882a23fbb Mon Sep 17 00:00:00 2001 From: Simon AURA--SCUSSEL Date: Wed, 20 May 2026 13:42:07 +0200 Subject: [PATCH 2/2] fix: make NodeNotFoundError parameters required Address review feedback: branch_name, node_type and identifier are now required keyword-only parameters of NodeNotFoundError.__init__, which removes the conditional branches from __str__. Call sites updated so every NodeNotFoundError carries the full context: - file_handler.handle_response and handle_error_response take branch and node_id, plumbed from FileHandler.download and _stream_to_file in both the async and sync paths. - store passes self.branch_name and only falls back to "unknown" on the internal lookup paths where the caller did not specify a kind. - ctl/object/utils resolves the branch to client.default_branch when the caller did not provide one. Added tests/unit/sdk/test_exceptions.py covering the new contract. --- infrahub_sdk/ctl/object/utils.py | 6 ++- infrahub_sdk/exceptions.py | 21 ++++------ infrahub_sdk/file_handler.py | 37 +++++++++++------ infrahub_sdk/store.py | 16 +++++++- tests/unit/sdk/test_exceptions.py | 62 +++++++++++++++++++++++++++++ tests/unit/sdk/test_file_handler.py | 33 ++++++++------- 6 files changed, 133 insertions(+), 42 deletions(-) create mode 100644 tests/unit/sdk/test_exceptions.py diff --git a/infrahub_sdk/ctl/object/utils.py b/infrahub_sdk/ctl/object/utils.py index 3fbcfd50..04772593 100644 --- a/infrahub_sdk/ctl/object/utils.py +++ b/infrahub_sdk/ctl/object/utils.py @@ -75,7 +75,11 @@ async def resolve_node( if node is not None: return node - raise NodeNotFoundError(node_type=kind, identifier={"id": [identifier]}, branch_name=branch) + raise NodeNotFoundError( + branch_name=branch or client.default_branch, + node_type=kind, + identifier={"id": [identifier]}, + ) def prepare_relationship_data(data: dict[str, Any], schema: MainSchemaTypesAPI) -> dict[str, Any]: diff --git a/infrahub_sdk/exceptions.py b/infrahub_sdk/exceptions.py index fefb9af9..f870d419 100644 --- a/infrahub_sdk/exceptions.py +++ b/infrahub_sdk/exceptions.py @@ -71,29 +71,22 @@ def __init__(self, message: str | None = None) -> None: class NodeNotFoundError(Error): def __init__( self, - identifier: Mapping[str, list[str]], + *, + branch_name: str, + node_type: str, + identifier: Mapping[str, list[str]] | str, message: str = "Unable to find the node in the database.", - branch_name: str | None = None, - node_type: str | None = None, ) -> None: - self.node_type = node_type or "unknown" - self.identifier = identifier self.branch_name = branch_name - + self.node_type = node_type + self.identifier = identifier self.message = message super().__init__(self.message) def __str__(self) -> str: - detail_parts = [] - if self.branch_name: - detail_parts.append(f"Branch: {self.branch_name}") - if self.node_type: - detail_parts.append(f"Kind: {self.node_type}") - detail_parts.append(f"Identifier: {self.identifier}") - detail = " | ".join(detail_parts) return f""" {self.message} - {detail} + Branch: {self.branch_name} | Kind: {self.node_type} | Identifier: {self.identifier} """ diff --git a/infrahub_sdk/file_handler.py b/infrahub_sdk/file_handler.py index 5d32441a..f906bc9b 100644 --- a/infrahub_sdk/file_handler.py +++ b/infrahub_sdk/file_handler.py @@ -95,11 +95,13 @@ def prepare_upload_sync(content: bytes | Path | BinaryIO | None, name: str | Non return PreparedFile(file_object=cast("BinaryIO", content), filename=filename, should_close=False) @staticmethod - def handle_error_response(exc: httpx.HTTPStatusError) -> None: + def handle_error_response(exc: httpx.HTTPStatusError, branch: str, node_id: str) -> None: """Handle HTTP error responses for file operations. Args: exc: The HTTP status error from httpx. + branch: The branch name used for the request. + node_id: The ID of the FileObject node being accessed. Raises: AuthenticationError: If authentication fails (401/403). @@ -114,15 +116,22 @@ def handle_error_response(exc: httpx.HTTPStatusError) -> None: if exc.response.status_code == 404: response = exc.response.json() detail = response.get("detail", "File not found") - raise NodeNotFoundError(node_type="FileObject", identifier=detail) from exc + raise NodeNotFoundError( + branch_name=branch, + node_type="FileObject", + identifier={"id": [node_id]}, + message=detail, + ) from exc raise exc @staticmethod - def handle_response(resp: httpx.Response) -> bytes: + def handle_response(resp: httpx.Response, branch: str, node_id: str) -> bytes: """Handle the HTTP response and return file content as bytes. Args: resp: The HTTP response from httpx. + branch: The branch name used for the request. + node_id: The ID of the FileObject node being accessed. Returns: The file content as bytes. @@ -134,7 +143,7 @@ def handle_response(resp: httpx.Response) -> bytes: try: resp.raise_for_status() except httpx.HTTPStatusError as exc: - FileHandlerBase.handle_error_response(exc=exc) + FileHandlerBase.handle_error_response(exc=exc, branch=branch, node_id=node_id) return resp.content @@ -198,7 +207,7 @@ async def download(self, node_id: str, branch: str | None, dest: Path | None = N url = self._build_url(node_id=node_id, branch=effective_branch) if dest is not None: - return await self._stream_to_file(url=url, dest=dest) + return await self._stream_to_file(url=url, dest=dest, branch=effective_branch, node_id=node_id) try: resp = await self._client._get(url=url) @@ -206,14 +215,16 @@ async def download(self, node_id: str, branch: str | None, dest: Path | None = N self._client.log.error(f"Unable to connect to {self._client.address}") raise - return self.handle_response(resp=resp) + return self.handle_response(resp=resp, branch=effective_branch, node_id=node_id) - async def _stream_to_file(self, url: str, dest: Path) -> int: + async def _stream_to_file(self, url: str, dest: Path, branch: str, node_id: str) -> int: """Stream download directly to a file without loading into memory. Args: url: The URL to download from. dest: The destination path to write to. + branch: The branch name used for the request. + node_id: The ID of the FileObject node being downloaded. Returns: The number of bytes written to the file. @@ -230,7 +241,7 @@ async def _stream_to_file(self, url: str, dest: Path) -> int: except httpx.HTTPStatusError as exc: # Need to read the response body for error details await resp.aread() - self.handle_error_response(exc=exc) + self.handle_error_response(exc=exc, branch=branch, node_id=node_id) bytes_written = 0 async with await anyio.Path(dest).open("wb") as f: @@ -303,7 +314,7 @@ def download(self, node_id: str, branch: str | None, dest: Path | None = None) - url = self._build_url(node_id=node_id, branch=effective_branch) if dest is not None: - return self._stream_to_file(url=url, dest=dest) + return self._stream_to_file(url=url, dest=dest, branch=effective_branch, node_id=node_id) try: resp = self._client._get(url=url) @@ -311,14 +322,16 @@ def download(self, node_id: str, branch: str | None, dest: Path | None = None) - self._client.log.error(f"Unable to connect to {self._client.address}") raise - return self.handle_response(resp=resp) + return self.handle_response(resp=resp, branch=effective_branch, node_id=node_id) - def _stream_to_file(self, url: str, dest: Path) -> int: + def _stream_to_file(self, url: str, dest: Path, branch: str, node_id: str) -> int: """Stream download directly to a file without loading into memory. Args: url: The URL to download from. dest: The destination path to write to. + branch: The branch name used for the request. + node_id: The ID of the FileObject node being downloaded. Returns: The number of bytes written to the file. @@ -335,7 +348,7 @@ def _stream_to_file(self, url: str, dest: Path) -> int: except httpx.HTTPStatusError as exc: # Need to read the response body for error details resp.read() - self.handle_error_response(exc=exc) + self.handle_error_response(exc=exc, branch=branch, node_id=node_id) bytes_written = 0 with dest.open("wb") as f: diff --git a/infrahub_sdk/store.py b/infrahub_sdk/store.py index 479badd1..025574f2 100644 --- a/infrahub_sdk/store.py +++ b/infrahub_sdk/store.py @@ -97,11 +97,15 @@ def get( if kind and found_invalid: raise NodeInvalidError( + branch_name=self.branch_name, + node_type=kind_name or "unknown", identifier={"key": [key] if isinstance(key, str) else key}, message=f"Found a node of a different kind instead of {kind} for key {key!r} in the store ({self.branch_name})", ) raise NodeNotFoundError( + branch_name=self.branch_name, + node_type=kind_name or "unknown", identifier={"key": [key] if isinstance(key, str) else key}, message=f"Unable to find the node {key!r} in the store ({self.branch_name})", ) @@ -111,6 +115,8 @@ def _get_by_internal_id( ) -> InfrahubNode | InfrahubNodeSync | CoreNode | CoreNodeSync: if internal_id not in self._objs: raise NodeNotFoundError( + branch_name=self.branch_name, + node_type=kind or "unknown", identifier={"internal_id": [internal_id]}, message=f"Unable to find the node {internal_id!r} in the store ({self.branch_name})", ) @@ -118,6 +124,7 @@ def _get_by_internal_id( node = self._objs[internal_id] if kind and kind not in node.get_all_kinds(): raise NodeInvalidError( + branch_name=self.branch_name, node_type=kind, identifier={"internal_id": [internal_id]}, message=f"Found a node of kind {node.get_kind()} instead of {kind} for internal_id {internal_id!r} in the store ({self.branch_name})", @@ -130,6 +137,8 @@ def _get_by_key( ) -> InfrahubNode | InfrahubNodeSync | CoreNode | CoreNodeSync: if key not in self._keys: raise NodeNotFoundError( + branch_name=self.branch_name, + node_type=kind or "unknown", identifier={"key": [key]}, message=f"Unable to find the node {key!r} in the store ({self.branch_name})", ) @@ -138,6 +147,7 @@ def _get_by_key( if kind and node.get_kind() != kind: raise NodeInvalidError( + branch_name=self.branch_name, node_type=kind, identifier={"key": [key]}, message=f"Found a node of kind {node.get_kind()} instead of {kind} for key {key!r} in the store ({self.branch_name})", @@ -148,6 +158,8 @@ def _get_by_key( def _get_by_id(self, id: str, kind: str | None = None) -> InfrahubNode | InfrahubNodeSync | CoreNode | CoreNodeSync: if id not in self._uuids: raise NodeNotFoundError( + branch_name=self.branch_name, + node_type=kind or "unknown", identifier={"id": [id]}, message=f"Unable to find the node {id!r} in the store ({self.branch_name})", ) @@ -155,6 +167,7 @@ def _get_by_id(self, id: str, kind: str | None = None) -> InfrahubNode | Infrahu node = self._get_by_internal_id(self._uuids[id]) if kind and kind not in node.get_all_kinds(): raise NodeInvalidError( + branch_name=self.branch_name, node_type=kind, identifier={"id": [id]}, message=f"Found a node of kind {node.get_kind()} instead of {kind} for id {id!r} in the store ({self.branch_name})", @@ -172,7 +185,8 @@ def _get_by_hfid( node_hfid = [hfid] if isinstance(hfid, str) else hfid exception_to_raise_if_not_found = NodeNotFoundError( - node_type=node_kind, + branch_name=self.branch_name, + node_type=node_kind or "unknown", identifier={"hfid": node_hfid}, message=f"Unable to find the node {hfid!r} in the store ({self.branch_name})", ) diff --git a/tests/unit/sdk/test_exceptions.py b/tests/unit/sdk/test_exceptions.py new file mode 100644 index 00000000..9a62529a --- /dev/null +++ b/tests/unit/sdk/test_exceptions.py @@ -0,0 +1,62 @@ +from __future__ import annotations + +import pytest + +from infrahub_sdk.exceptions import NodeInvalidError, NodeNotFoundError + + +def test_node_not_found_error_default_message_format() -> None: + error = NodeNotFoundError( + branch_name="main", + node_type="InfraDevice", + identifier={"name__value": ["bad-device404"]}, + ) + + rendered = str(error) + assert "Unable to find the node in the database." in rendered + assert "Branch: main" in rendered + assert "Kind: InfraDevice" in rendered + assert "Identifier: {'name__value': ['bad-device404']}" in rendered + + +def test_node_not_found_error_custom_message_format() -> None: + error = NodeNotFoundError( + branch_name="feature", + node_type="InfraInterface", + identifier={"id": ["abc123"]}, + message="Unable to find the node in the store.", + ) + + rendered = str(error) + assert "Unable to find the node in the store." in rendered + assert "Branch: feature | Kind: InfraInterface | Identifier: {'id': ['abc123']}" in rendered + + +def test_node_not_found_error_requires_keyword_arguments() -> None: + with pytest.raises(TypeError, match="positional argument"): + NodeNotFoundError("main", "InfraDevice", {"id": ["abc"]}) # type: ignore[misc] + + +def test_node_not_found_error_requires_all_fields() -> None: + with pytest.raises(TypeError, match="branch_name"): + NodeNotFoundError(node_type="InfraDevice", identifier={"id": ["abc"]}) # type: ignore[call-arg] + + with pytest.raises(TypeError, match="node_type"): + NodeNotFoundError(branch_name="main", identifier={"id": ["abc"]}) # type: ignore[call-arg] + + with pytest.raises(TypeError, match="identifier"): + NodeNotFoundError(branch_name="main", node_type="InfraDevice") # type: ignore[call-arg] + + +def test_node_invalid_error_inherits_signature() -> None: + error = NodeInvalidError( + branch_name="main", + node_type="InfraDevice", + identifier={"id": ["abc"]}, + message="Found a node of a different kind", + ) + + assert isinstance(error, NodeNotFoundError) + rendered = str(error) + assert "Branch: main | Kind: InfraDevice | Identifier: {'id': ['abc']}" in rendered + assert "Found a node of a different kind" in rendered diff --git a/tests/unit/sdk/test_file_handler.py b/tests/unit/sdk/test_file_handler.py index ae59c842..3c2b7262 100644 --- a/tests/unit/sdk/test_file_handler.py +++ b/tests/unit/sdk/test_file_handler.py @@ -171,10 +171,8 @@ def test_handle_error_response_401() -> None: response = httpx.Response(status_code=401, json={"errors": [{"message": "Invalid token"}]}) exc = httpx.HTTPStatusError(message="Unauthorized", request=httpx.Request("GET", "http://test"), response=response) - with pytest.raises(AuthenticationError) as excinfo: - FileHandlerBase.handle_error_response(exc=exc) - - assert "Invalid token" in str(excinfo.value) + with pytest.raises(AuthenticationError, match="Invalid token"): + FileHandlerBase.handle_error_response(exc=exc, branch="main", node_id=NODE_ID) def test_handle_error_response_403() -> None: @@ -182,10 +180,8 @@ def test_handle_error_response_403() -> None: response = httpx.Response(status_code=403, json={"errors": [{"message": "Access denied"}]}) exc = httpx.HTTPStatusError(message="Forbidden", request=httpx.Request("GET", "http://test"), response=response) - with pytest.raises(AuthenticationError) as excinfo: - FileHandlerBase.handle_error_response(exc=exc) - - assert "Access denied" in str(excinfo.value) + with pytest.raises(AuthenticationError, match="Access denied"): + FileHandlerBase.handle_error_response(exc=exc, branch="main", node_id=NODE_ID) def test_handle_error_response_404() -> None: @@ -193,10 +189,19 @@ def test_handle_error_response_404() -> None: response = httpx.Response(status_code=404, json={"detail": "File not found with ID abc123"}) exc = httpx.HTTPStatusError(message="Not Found", request=httpx.Request("GET", "http://test"), response=response) - with pytest.raises(NodeNotFoundError) as excinfo: - FileHandlerBase.handle_error_response(exc=exc) + with pytest.raises(NodeNotFoundError, match="File not found with ID abc123") as excinfo: + FileHandlerBase.handle_error_response(exc=exc, branch="main", node_id=NODE_ID) - assert "File not found with ID abc123" in str(excinfo.value) + error = excinfo.value + assert error.branch_name == "main" + assert error.node_type == "FileObject" + assert error.identifier == {"id": [NODE_ID]} + assert error.message == "File not found with ID abc123" + rendered = str(error) + assert "Branch: main" in rendered + assert "Kind: FileObject" in rendered + assert f"Identifier: {{'id': ['{NODE_ID}']}}" in rendered + assert "File not found with ID abc123" in rendered def test_handle_error_response_500() -> None: @@ -204,8 +209,8 @@ def test_handle_error_response_500() -> None: response = httpx.Response(status_code=500, json={"error": "Internal server error"}) exc = httpx.HTTPStatusError(message="Server Error", request=httpx.Request("GET", "http://test"), response=response) - with pytest.raises(httpx.HTTPStatusError): - FileHandlerBase.handle_error_response(exc=exc) + with pytest.raises(httpx.HTTPStatusError, match="Server Error"): + FileHandlerBase.handle_error_response(exc=exc, branch="main", node_id=NODE_ID) def test_handle_response_success() -> None: @@ -213,7 +218,7 @@ def test_handle_response_success() -> None: request = httpx.Request("GET", "http://test") response = httpx.Response(status_code=200, content=FILE_CONTENT_BYTES, request=request) - result = FileHandlerBase.handle_response(resp=response) + result = FileHandlerBase.handle_response(resp=response, branch="main", node_id=NODE_ID) assert result == FILE_CONTENT_BYTES