Skip to content

fix: add labels to NodeNotFoundError for clearer messaging#1030

Open
DharmaBytesX wants to merge 2 commits into
opsmill:developfrom
DharmaBytesX:fix/5514-better-node-not-found-error
Open

fix: add labels to NodeNotFoundError for clearer messaging#1030
DharmaBytesX wants to merge 2 commits into
opsmill:developfrom
DharmaBytesX:fix/5514-better-node-not-found-error

Conversation

@DharmaBytesX
Copy link
Copy Markdown

@DharmaBytesX DharmaBytesX commented May 17, 2026

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.

Before:

        Unable to find the node in the database.
        main | InfraDevice | {'name__value': 'bad-device404'}

After:

        Unable to find the node in the database.
        Branch: main | Kind: InfraDevice | Identifier: {'name__value': 'bad-device404'}

Fixes opsmill/infrahub#5514


Summary by cubic

Add labels to NodeNotFoundError and require keyword-only branch_name, node_type, and identifier, producing consistent messages like "Branch: ... | Kind: ... | Identifier: ...".
Updated file download paths to pass branch and node ID, and store/CTL lookups to include branch (falling back to the client’s default), so all NodeNotFoundError instances carry full context.

Written for commit 1ab16ae. Summary will update on new commits. Review in cubic

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
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

@DharmaBytesX
Copy link
Copy Markdown
Author

Hi! A small fix here to improve the NodeNotFoundError message — could someone give it a quick review when you get a chance? Thanks!

Comment thread infrahub_sdk/exceptions.py Outdated
if self.branch_name:
detail_parts.append(f"Branch: {self.branch_name}")
if self.node_type:
detail_parts.append(f"Kind: {self.node_type}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have if-statements within the __str__() method of classes in general. I think the problem here is really that the parameters to the __init__() method are optional instead of being required. We should probably look at the places where we raise this error and check if we are always sending in all fields (or if we have enough information to do so) and then update the init method to require there parameters.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I've reworked the change along the lines you suggested: branch_name, node_type and identifier are now required keyword-only parameters of NodeNotFoundError.__init__, and __str__ no longer has any conditional logic.

I audited the call sites and updated them to always pass the three fields:

  • infrahub_sdk/client.py was already passing all three.
  • infrahub_sdk/ctl/object/utils.py now resolves the branch to client.default_branch when the caller did not provide one.
  • infrahub_sdk/file_handler.py: handle_response and handle_error_response now take branch and node_id, plumbed from FileHandler.download / _stream_to_file in both the async and sync paths.
  • infrahub_sdk/store.py: passes self.branch_name, and falls back to "unknown" only on the internal lookup paths where the caller genuinely never specified a kind.

Also added tests/unit/sdk/test_exceptions.py covering the new contract (keyword-only, all three required, rendered format).

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants