Prevent cascade delete from causing SDK tracking delete to fail #776
Conversation
WalkthroughAdds a changelog entry and updates infrahub_sdk/query_groups.py to import GraphQLError from .exceptions. In the async path of InfrahubGroupContext.delete_unused, deletion is wrapped in a try/except that catches GraphQLError: if the error message indicates the node cannot be found, the error is ignored (skipping the already-deleted node); otherwise the error is re-raised. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ogenstad
force-pushed
the
pog-early-error-return-IHS-98
branch
from
4ddc86c to
35f9935
Compare
Deploying infrahub-sdk-python with
|
| Latest commit: |
fda5fcf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f618136b.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-early-error-return-ihs-9.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #776 +/- ##
==========================================
- Coverage 80.39% 80.37% -0.03%
==========================================
Files 115 115
Lines 9869 9873 +4
Branches 1511 1512 +1
==========================================
+ Hits 7934 7935 +1
- Misses 1413 1416 +3
Partials 522 522
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrahub_sdk/query_groups.py (1)
108-122: Sync delete_unused still fails on cascade deletes.Line 112-122 handles GraphQLError only in the async path; the sync variant still calls delete directly, so sync tracking will continue to fail in the same cascade scenario. Please mirror the handling in
InfrahubGroupContextSync.delete_unused. As per coding guidelines, ...🛠️ Proposed fix for sync parity (InfrahubGroupContextSync.delete_unused)
def delete_unused(self) -> None: if self.previous_members and self.unused_member_ids: for member in self.previous_members: if member.id in self.unused_member_ids and member.typename: - self.client.delete(kind=member.typename, id=member.id) + try: + self.client.delete(kind=member.typename, id=member.id) + except GraphQLError as exc: + if not exc.message or "Unable to find the node" not in exc.message: + raise
infrahub_sdk/query_groups.py
Outdated
| # If the node already has been deleted, skip the error as it would have been deleted | ||
| # by the cascade delete of another node | ||
| # Note this is a quick fix and could be improved by the delete order based on relationship | ||
| # types along with cascade delete settings. Alternatively send a batch delete request. | ||
| # Also see INFP-468 for better error handling from the backend side to avoid having to | ||
| # parse the error message as above. |
There was a problem hiding this comment.
I'd say this comment should be Node has already been deleted, ignore error. everything else should be captured by Jira/GH issues
There was a problem hiding this comment.
Fair enough. I've shortened the comment.
ogenstad
force-pushed
the
pog-early-error-return-IHS-98
branch
from
35f9935 to
fda5fcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrahub_sdk/query_groups.py (1)
208-212: Missing cascade delete handling in sync version.The fix for cascade delete errors was only applied to the async
delete_unusedmethod. The sync versionInfrahubGroupContextSync.delete_unusedlacks the same try/except handling and will still fail when attempting to delete nodes already removed by cascade.Based on learnings and coding guidelines, the async/sync dual pattern should be followed for new features in the Python SDK.
🔧 Apply the same fix to the sync version
def delete_unused(self) -> None: if self.previous_members and self.unused_member_ids: for member in self.previous_members: if member.id in self.unused_member_ids and member.typename: - self.client.delete(kind=member.typename, id=member.id) + try: + self.client.delete(kind=member.typename, id=member.id) + except GraphQLError as exc: + # If the node already has been deleted by cascade delete of another node, + # skip the error; otherwise re-raise. + if not exc.message or "Unable to find the node" not in exc.message: + raise
🧹 Nitpick comments (1)
infrahub_sdk/query_groups.py (1)
112-118: Logic is correct for handling cascade-deleted nodes.The try/except correctly catches
GraphQLErrorand skips re-raising only when the message indicates the node was not found (already removed by cascade). Other GraphQL errors are properly re-raised.Minor suggestion: The comment placement inside the
ifblock is slightly confusing since it explains why we don't raise in the else case. Consider moving it above theifor after theraise:♻️ Suggested clarification
try: await self.client.delete(kind=member.typename, id=member.id) except GraphQLError as exc: + # If the node already has been deleted by cascade delete of another node, + # skip the error; otherwise re-raise. if not exc.message or "Unable to find the node" not in exc.message: - # If the node already has been deleted, skip the error as it would have been deleted - # by the cascade delete of another node raise
Currently there's an issue with deleting nodes using a generator and the SDK tracking feature if the impacted nodes have a schema that also uses the cascade delete feature.
For example if we have a number of devices and interfaces that should be deleted and we happen to delete one of the devices first we'd cause the backend cascade delete feature to remove all of the interfaces attached to that device. When the SDK then tries to remove such an interface we previously raised an error. The change here is that we instead catch those errors and ignore GraphQL errors if they are raised because a node was not found.
A larger refactoring could be warranted here. Both in terms of error management and enrichment from the backend but also the ability to send in a list of all of the nodes we want to delete in one transaction to the backend instead of calling a delete mutation for each entry.
Fixes #265
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.