fix: Print clear error when fetching/editing a relationship on an id-less node#1155
fix: Print clear error when fetching/editing a relationship on an id-less node#1155PhillSimonds wants to merge 1 commit into
Conversation
… node Fetching or editing a cardinality-many relationship on a node that has no ID (e.g. created with client.create() but never saved) produced confusing errors: fetch() surfaced the generic "At least one filter must be provided to get()" from the internal client.get(id=None) call, and add()/remove() told the user to call fetch() — which could not succeed either, sending them in a circle. fetch(), add(), and remove() now detect the missing node ID up front and raise a clear UninitializedError that nudges the user to call .save() first. When the node does have an ID, the existing "call fetch()" guidance is preserved. Applied to both RelationshipManager and RelationshipManagerSync. Fixes #1153 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying infrahub-sdk-python with
|
| Latest commit: |
df30f27
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7d7bc26f.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://sts-fix-1153-rel-fetch-no-id.infrahub-sdk-python.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #1155 +/- ##
===========================================
- Coverage 82.35% 82.32% -0.03%
===========================================
Files 138 138
Lines 12065 12056 -9
Branches 1805 1811 +6
===========================================
- Hits 9936 9925 -11
- Misses 1573 1576 +3
+ Partials 556 555 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
PhillSimonds
changed the title
PhillSimonds
changed the title
| A relationship can only be fetched or edited once the parent node has an ID to look it up | ||
| by. When it does not, the underlying ``client.get()`` call would otherwise fail with a | ||
| generic "At least one filter must be provided to get()" message that gives the caller no | ||
| hint about the real cause. |
There was a problem hiding this comment.
if this documentation is needed, it should go somewhere else. the first line of the docstring and the text of the error message cover the actual description of this function's responsibility
| from .node import InfrahubNode, InfrahubNodeSync | ||
|
|
||
|
|
||
| def _raise_missing_identifier(node: InfrahubNode | InfrahubNodeSync, name: str) -> NoReturn: |
There was a problem hiding this comment.
I don't think this function really needs to access the whole node object. I think passing in object_kind: str would be enough. and you can get it using node.get_kind() instead of accessing the private-ish ._schema
| @@ -440,6 +465,8 @@ def fetch(self) -> None: | |||
|
|
|||
| """ | |||
| if not self.initialized: | |||
There was a problem hiding this comment.
haven't tested this, but it looks like if I create a Node with no ID AND relationship data, then self.initialized is set to True, which would skip your new error message below. not sure if that is a valid use case that should be covered by this change.
There was a problem hiding this comment.
what happens if a node is initialized with no UUID, but an HFID? would an error still be raised in that case? probably worth adding as a test to confirm whatever the expected behavior should be
2 participants
What
Fixes #1153.
Fetching or editing a cardinality-many relationship on a node with no ID (e.g. one built with
client.create()but neversave()d) produced confusing errors:node.rel.fetch()→ValueError: At least one filter must be provided to get()— becausefetch()re-queries the parent viaclient.get(id=self.node.id)withid=None.node.rel.add(...)/remove(...)→Must call fetch() on RelationshipManager before editing members— butfetch()can't succeed either, so the two guards send the caller in a circle.Change
fetch(),add(), andremove()(bothRelationshipManagerandRelationshipManagerSync) now detect the missing node ID up front and raise a clearUninitializedErrorthat nudges the user to.save()first, via a shared_raise_missing_identifier()helper. When the node does have an ID, the existing"Must call fetch()"guidance is preserved.The message is deliberately phrased around "no ID to look it up by / likely not saved yet" rather than asserting the node doesn't exist in Infrahub — at that point only a local attribute has been inspected, no server call has been made.
Tests
Added to
tests/unit/sdk/test_node.py(async + sync):test_fetch_relationship_without_node_id_raises_clear_errortest_edit_relationship_without_node_id_raises_clear_errortest_edit_relationship_with_node_id_still_requires_fetch(regression: id-bearing node still gets "call fetch()")uv run invoke format lint-codeclean;uv run pytest tests/unit/sdk/test_node.py→ 228 passed.Related
Sibling PR for #1152 (assigning to a cardinality-many relationship).
Summary by cubic
Show a clear error when fetching or editing a relationship on a node without an ID, guiding users to save the node first. Prevents the generic
get()error and the circular “call fetch()” guidance.RelationshipManagerandRelationshipManagerSync,fetch(),add(), andremove()now detect a missing node ID and raiseUninitializedErrorwith a hint to call.save()first.Written for commit df30f27. Summary will update on new commits.