IHS-119: Fix upsert not working for numberpool and attribute in hfid by solababs · Pull Request #1014 · opsmill/infrahub-sdk-python

solababs · 2026-05-12T10:12:49Z

Why

node.save(allow_upsert=True) on a node whose human-friendly identifier (HFID) contains a CoreNumberPool-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 ValidationError before any network call is made.

Closes #339

How to test

cd python_sdk

# All four upsert guard tests
uv run pytest tests/unit/sdk/pool/test_attribute_from_pool.py -v -k "upsert"

Checklist

Tests added/updated
Changelog entry added (python_sdk/changelog/339.fixed.md)
External docs updated (docs/docs/python-sdk/guides/resource-manager.mdx)
Internal .md docs updated (internal knowledge and AI code tools knowledge)

Summary by cubic

Prevents crashes when upserting nodes whose HFID includes a CoreNumberPool attribute by failing fast with a clear ValidationError. Addresses IHS-119.

Bug Fixes
- Added _validate_upsert() and call it from create() (async and sync) so both save(allow_upsert=True) and create(allow_upsert=True) raise early when any HFID component is an unresolved pool-backed attribute (a from_pool dict with no value or a pool node object).
- Upsert still works when id is set or when the pool-backed attribute is not in the HFID. Added is_unresolved_pool_attribute(), tests for async/sync paths, updated docs with alternatives, and a changelog entry.

^{Written for commit 5ac552f. Summary will update on new commits.}

cloudflare-workers-and-pages · 2026-05-12T10:15:55Z

Deploying infrahub-sdk-python with Cloudflare Pages

Latest commit:	`5ac552f`
Status:	✅ Deploy successful!
Preview URL:	https://c450c72e.infrahub-sdk-python.pages.dev
Branch Preview URL:	https://sb-12052026-fix-upsert-numbe.infrahub-sdk-python.pages.dev

View logs

codecov · 2026-05-12T10:19:15Z

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
infrahub_sdk/node/node.py	87.50%	2 Missing ⚠️

@@            Coverage Diff             @@
##           stable    #1014      +/-   ##
==========================================
+ Coverage   81.56%   81.78%   +0.21%     
==========================================
  Files         134      135       +1     
  Lines       11442    11643     +201     
  Branches     1730     1761      +31     
==========================================
+ Hits         9333     9522     +189     
- Misses       1564     1572       +8     
- Partials      545      549       +4

Flag	Coverage Δ
integration-tests	`41.62% <52.63%> (+0.12%)`	⬆️
python-3.10	`55.20% <47.36%> (+0.61%)`	⬆️
python-3.11	`55.18% <47.36%> (+0.59%)`	⬆️
python-3.12	`55.18% <47.36%> (+0.61%)`	⬆️
python-3.13	`55.20% <47.36%> (+0.61%)`	⬆️
python-3.14	`55.18% <47.36%> (+0.59%)`	⬆️
python-filler-3.12	`22.36% <21.05%> (-0.35%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
infrahub_sdk/node/attribute.py	`100.00% <100.00%> (ø)`
infrahub_sdk/node/node.py	`87.42% <87.50%> (+0.53%)`	⬆️

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ogenstad · 2026-05-25T13:50:23Z

+                    attr = self._get_attribute(attr_name)
+                except ResourceNotDefinedError:
+                    continue
+                if attr.is_from_pool_attribute():


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?

cubic-dev-ai

1 issue found across 5 files

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

ogenstad

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.

ogenstad · 2026-06-03T10:41:32Z

        timeout: int | None = None,
        request_context: RequestContext | None = None,
    ) -> None:
+        if allow_upsert and not self.id:


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.

polmichel

The coverage is great and the fix is nicely implemented.
That said, there is a test that is just noisy and, in my opinion, does not bring any value to our test suite. Maybe it would be better to remove it.

Optional nitpick comments about test descriptions and test assertions have also been added.

polmichel · 2026-06-11T12:32:54Z

+    vlan_schema_with_pool_hfid: NodeSchemaAPI,
+    httpx_mock: HTTPXMock,
+) -> None:
+    """FAILS before the guard is added: ValidationError must be raised when HFID attr is NumberPool-sourced."""


Nitpick and optional comment: test description should in my opinion describe what's the expected behavior from the system, and not stating what has been done in order to reach this state (like fixing an older bug)

polmichel · 2026-06-11T12:35:48Z

+    vlan_schema_with_pool_hfid: NodeSchemaAPI,
+    httpx_mock: HTTPXMock,
+) -> None:
+    """Guard is bypassed when an explicit node id is already set."""


Nitpick and optional comment: I do not think knowing there is a "guard" is important. What's important is the use case that this test describes.

polmichel · 2026-06-11T12:37:49Z

+    with pytest.raises(ValidationError, match="vlan_id"):
+        await node.save(allow_upsert=True)


Optional comment: after some discussions with @gmazoyer and @ajtmccarty, we ended up agreeing that exact error message matches would create better assertions in our test suites.

polmichel · 2026-06-11T12:38:09Z

+        data={"name": "Test VLAN", "vlan_id": {"from_pool": {"id": POOL_ID}}},
+    )
+
+    with pytest.raises(ValidationError, match="vlan_id"):


Same optional comment about error message assertions

Same inside other tests

polmichel · 2026-06-11T12:42:29Z

+@pytest.mark.httpx_mock(assert_all_responses_were_requested=False)
+async def test_save_upsert_no_error_before_fix(
+    client: InfrahubClient,
+    vlan_schema_with_pool_hfid: NodeSchemaAPI,
+    httpx_mock: HTTPXMock,
+) -> None:
+    """FAILS before the guard is added: ValidationError must be raised when HFID attr is NumberPool-sourced."""
+    httpx_mock.add_response(method="POST", json=UPSERT_MOCK_RESPONSE)
+    node = InfrahubNode(
+        client=client,
+        schema=vlan_schema_with_pool_hfid,
+        data={"name": "Test VLAN", "vlan_id": {"from_pool": {"id": POOL_ID}}},
+    )
+
+    with pytest.raises(ValidationError, match="vlan_id"):
+        await node.save(allow_upsert=True)


I am not sure what's the purpose of this test: mocking the API behavior to make sure that if we remove the fix, the application is still failing?

I think we should remove this test

polmichel · 2026-06-11T12:47:03Z

+    httpx_mock: HTTPXMock,
+) -> None:
+    """FAILS before the guard is added: ValidationError must be raised when HFID attr is NumberPool-sourced."""
+    httpx_mock.add_response(method="POST", json=UPSERT_MOCK_RESPONSE)


The mock response is not even used -> @pytest.mark.httpx_mock(assert_all_responses_were_requested=False)

polmichel · 2026-06-11T12:48:09Z

+    vlan_schema: NodeSchemaAPI,
+    httpx_mock: HTTPXMock,
+) -> None:
+    """Guard does not fire when the pool-sourced attribute is not part of the HFID."""


Same comment about test description

Same inside other tests

IHS-119: Fix upsert not working for numberpool and attribute in hfid

687de40

solababs requested a review from a team as a code owner May 12, 2026 10:12

github-actions Bot added the type/documentation Improvements or additions to documentation label May 12, 2026

update docs

98047b9

ogenstad reviewed May 25, 2026

View reviewed changes

update condition

e15070c

solababs requested a review from ogenstad May 26, 2026 10:14

cubic-dev-ai Bot reviewed May 26, 2026

View reviewed changes

Comment thread infrahub_sdk/node/node.py Outdated

solababs added 2 commits May 26, 2026 11:26

update test

679f7a9

update docs

da072d3

ogenstad reviewed Jun 3, 2026

View reviewed changes

ogenstad added 2 commits June 8, 2026 08:56

Consolidate validation

da84a50

Move validation to create

f1790c8

ogenstad requested a review from a team June 11, 2026 08:18

polmichel reviewed Jun 11, 2026

View reviewed changes

address feedback

5ac552f

polmichel approved these changes Jun 11, 2026

View reviewed changes

ogenstad merged commit 05ac997 into stable Jun 11, 2026
21 checks passed

ogenstad deleted the sb-12052026-fix-upsert-numberpool-hfid-ihs-119 branch June 11, 2026 17:00

		with pytest.raises(ValidationError, match="vlan_id"):
		await node.save(allow_upsert=True)

Conversation

solababs commented May 12, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How to test

Checklist

Summary by cubic

Uh oh!

cloudflare-workers-and-pages Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying infrahub-sdk-python with Cloudflare Pages

Uh oh!

codecov Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ogenstad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmichel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

solababs commented May 12, 2026 •

edited by cubic-dev-ai Bot

Loading

cloudflare-workers-and-pages Bot commented May 12, 2026 •

edited

Loading

codecov Bot commented May 12, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading