IHS-119: Fix upsert not working for numberpool and attribute in hfid#1014
IHS-119: Fix upsert not working for numberpool and attribute in hfid#1014solababs wants to merge 5 commits into
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
da072d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41292ab9.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://sb-12052026-fix-upsert-numbe.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #1014 +/- ##
==========================================
+ Coverage 81.56% 81.58% +0.01%
==========================================
Files 134 134
Lines 11442 11490 +48
Branches 1730 1742 +12
==========================================
+ Hits 9333 9374 +41
- Misses 1564 1570 +6
- Partials 545 546 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| attr = self._get_attribute(attr_name) | ||
| except ResourceNotDefinedError: | ||
| continue | ||
| if attr.is_from_pool_attribute(): |
There was a problem hiding this comment.
I'm wondering if we should forbid this all together if the attribute can come from a pool. It could be that we should have
if attr.value is not None and attr.is_from_pool_attribute():I.e. that this would be problematic for upsert mutations if a value hadn't been assigned from a pool already. Was that something that you considered?
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
ogenstad
left a comment
There was a problem hiding this comment.
Looks good overall, but as we're testing with mocked data it could be nice either have an integration test or at least validate against a real server.
| timeout: int | None = None, | ||
| request_context: RequestContext | None = None, | ||
| ) -> None: | ||
| if allow_upsert and not self.id: |
There was a problem hiding this comment.
The bug report specifically mentions Node.save() as a problem area but I think we can have a similar situation in Node.create(). One additional comment here is that the code looks exactly the same for the sync and async version, with that in mind I think we can create a helper method that can be called from both of these to ensure that we keep the code in sync in the future.
Why
node.save(allow_upsert=True)on a node whose human-friendly identifier (HFID) contains aCoreNumberPool-sourced attribute crashes with"invalid literal for int() with base 10: 'VLAN ID Pool - Test'". The backend upsert resolver tries to use the unresolved pool reference as an HFID component, but the integer value is not assigned until after the server creates the node — making the upsert semantically impossible.This PR adds a client-side guard that detects the pattern and raises a descriptive
ValidationErrorbefore any network call is made.Closes #339
How to test
Checklist
python_sdk/changelog/339.fixed.md)docs/docs/python-sdk/guides/resource-manager.mdx)Summary by cubic
Adds a client-side guard to stop crashes when calling
save(allow_upsert=True)on nodes whose HFID includes aCoreNumberPool-sourced attribute; it now raises a clearValidationErrorbefore any request. Addresses IHS-119.InfrahubNode.save()to block upsert when an HFID component is an unresolved pool-backed attribute (either afrom_pooldict with no value or a pool node object); raisesValidationErrorearly.idis set or when the pool-backed attribute is not part of the HFID.is_unresolved_pool_attribute()for attributes; expanded tests; updated the Resource Manager guide with warning and alternatives and the SDK reference; added a changelog entry.Written for commit da072d3. Summary will update on new commits. Review in cubic