Repository navigation
fix(node): refuse adding or removing a number pool range through the pool - #1469
Conversation
github-actions
Bot
added
the
type/documentation
Deploying infrahub-sdk-python with
|
| Latest commit: |
04bc1bc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54ca50ea.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://number-pool-ranges-add.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## infrahub-release-1.12 #1469 +/- ##
=========================================================
+ Coverage 87.74% 87.81% +0.07%
=========================================================
Files 155 157 +2
Lines 15517 15575 +58
Branches 2085 2094 +9
=========================================================
+ Hits 13615 13677 +62
+ Misses 1345 1341 -4
Partials 557 557
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
acf4338 to
2624fe1
Compare
…s fields The mutation of such a relationship takes the fields of a peer, not a reference to one, so add(), extend() and remove() could not reach Infrahub: add() and extend() dropped the peer without an error, and remove() sent the remaining peers as references, which Infrahub refused. They now raise FeatureNotSupportedError and name the ways out that work. The relationships concerned are listed as (name, peer) pairs, so another relationship to the same kind of node stays editable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A relationship whose mutation takes the peers' fields does reach Infrahub on a create, so object create refuses only a value that names peers by reference and lets a JSON list of their fields through. An update sends the peers the node was fetched with, so object update refuses every value and names the create that upserts the node. Both report the refusal before any change is applied, and the reason they give comes from the constant the SDK message is built from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d on their own The two tests fail: loading the file sends a CoreNumberPoolRange mutation of its own next to the pool, which Infrahub refuses as overlapping the range the pool holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A relationship whose mutation takes the peers' fields now travels in the payload of the node that declares it, in both forms an object file accepts for a list of objects. A list of references stays a list of references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2624fe1 to
93effe5
Compare
… them
The branch that sends the peers' fields with their node read the list under `data`
as it stands, while the validation pass runs it through the processors first. A file
with `parameters: {expand_range: true}` was therefore validated expanded and saved
unexpanded. Both passes now read the same peers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
93effe5 to
1292c6f
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx">
<violation number="1" location="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx:43">
P2: This says saving an existing node sends the peers it was fetched with, but the serializer omits an unchanged relationship from the update instead. Document that local peer-list edits are ignored on an existing node and Infrahub's existing peers remain unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| relationship. | ||
| relationship. For a relationship that takes its peers as fields instead of references, that | ||
| holds only until the node exists: a create sends the list as it stands, while a save of a node | ||
| Infrahub already holds sends the peers it was fetched with. |
There was a problem hiding this comment.
P2: This says saving an existing node sends the peers it was fetched with, but the serializer omits an unchanged relationship from the update instead. Document that local peer-list edits are ignored on an existing node and Infrahub's existing peers remain unchanged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/relationship.mdx, line 43:
<comment>This says saving an existing node sends the peers it was fetched with, but the serializer omits an unchanged relationship from the update instead. Document that local peer-list edits are ignored on an existing node and Infrahub's existing peers remain unchanged.</comment>
<file context>
@@ -38,7 +38,9 @@ peers(self) -> list[RelatedNode[PeerT] | RelatedNodeSync[PeerT]]
-relationship.
+relationship. For a relationship that takes its peers as fields instead of references, that
+holds only until the node exists: a create sends the list as it stands, while a save of a node
+Infrahub already holds sends the peers it was fetched with.
**Returns:**
</file context>
The create guard read a list of objects as the fields of the peers, so an object holding an id, an hfid or a node went through and failed against the server instead of reporting the refusal. The keys that name a peer now have one definition, which the node side reads as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch that sends the peers' fields read the parameters of the relationship alone, while the validation pass falls back to the parameters of the file. A file that sets expand_range for everything it holds was therefore validated expanded and saved unexpanded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Describing what each save path does with an edit invited readers to rely on one of them. The guide and the docstring now name the complete list as the way to set the ranges. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each one now carries only what the code does not say: why these peers do not go through the deferred branch, and why the parameters fall back to the file's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| async def test_changing_the_peer_list_before_the_pool_exists_sets_what_is_sent( | ||
| clients: BothClients, number_pool_schema: NodeSchemaAPI | ||
| ) -> None: |
There was a problem hiding this comment.
This test covers something the deprecation guide advises against: it shows that all the methods manipulating .peers do nothing. It passes, so I think it’s still good to keep it, even though the documentation tells users not to do this. Otherwise, we'd need to state that this does nothing for add, remove, ... but not for append.
|
|
||
| **Raises:** | ||
|
|
||
| - `FeatureNotSupportedError`: If the relationship takes its peers as fields instead of references. |
There was a problem hiding this comment.
Maybe naming this error differently would be more appropriate/accurate
There was a problem hiding this comment.
this isn't really fixed b/c it was never released
There was a problem hiding this comment.
Good point, I'll remove it
|
|
||
| # Relationships whose input in a mutation takes the peer's fields, not a reference to an existing peer, | ||
| # as (name, peer) pairs. | ||
| INLINE_INPUT_RELATIONSHIPS = frozenset({("ranges", "CoreNumberPoolRange")}) |
There was a problem hiding this comment.
can always change it later if we need to, but I would expect this to be dict[str, list/tuple[str]] (or the frozen version of that) to allow linking multiple relationships to a single kind. so {"CoreNumberPoolRange": ["ranges"]}
There was a problem hiding this comment.
I’ll do that! It’s definitely better to have some logic tying these strings together
| INLINE_INPUT_RELATIONSHIPS = frozenset({("ranges", "CoreNumberPoolRange")}) | ||
|
|
||
|
|
||
| def _peers_are_inline_input(schema: RelationshipSchemaAPI) -> bool: |
There was a problem hiding this comment.
should this lose the _? I know we use _ methods on the Node objects so we don't block attribute/relationship names, but I don't think we need it here. I could be missing something
There was a problem hiding this comment.
It was just to not expose this method to users through SDK documentation, but it's fine to expose it I guess, and it's also weird to have this method inside constants.py.
I'll change it
| await obj.process(client=client) | ||
| await obj.process(client=client) |
There was a problem hiding this comment.
is this to test idempotency?
| result = prepare_relationship_data(data, schema) | ||
| assert result == {"name": "router1", "site": ["DC1"], "tags": [["blue"]]} | ||
|
|
||
|
|
There was a problem hiding this comment.
these could probably move to their own test module. utils is a vague name to start with and test_utils is somehow worse
|
|
||
| @pytest.fixture | ||
| async def range_policy_schema() -> NodeSchemaAPI: | ||
| """A schema with its own relationship to CoreNumberPoolRange nodes.""" |
There was a problem hiding this comment.
this is not something that I had considered, but I guess CoreNumberPoolRange is just a schema that any other schema can link to, although CoreNumberPoolRange is branch-agnostic, so not sure how useful it would be
| assert 'ranges: [ { id: "range-1" }, ]' in query | ||
|
|
||
|
|
||
| async def test_object_file_sends_the_ranges_its_parameters_expand( |
There was a problem hiding this comment.
do we need a test for this? I don't think there's a scenario where using parameter expansion on pool ranges would actually work server-side
There was a problem hiding this comment.
That was mostly to prove that the code path doesn’t fork too early and still runs the code inside infrahub_sdk.spec.processors.factory.DataProcessorFactory.process_data, in case it contains behavior other than expansion. But I’m fine with removing it as it's hard to read why it is there.
The table, the message and the predicate that reads them now live in a module of their own, keyed by peer kind so a kind can have more than one such relationship. The predicate loses its leading underscore: four modules in three packages read it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both guards sat in a module named for everything the object commands share, and their tests in a test module named after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anges Nothing was released without it, so it is part of what the release adds rather than a fix. The integration test says why it processes the same file twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Expansion turns a start into a string, which the Number attribute of a range does not take, so the tests pinned a payload Infrahub has no reason to accept. The parameters still travel, so validating and saving read the same peers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/inline_peers.mdx">
<violation number="1" location="docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/inline_peers.mdx:12">
P3: This page exposes `peers_are_inline_input` as a public SDK API even though the helper is internal and is not exported from `infrahub_sdk.node`. Remove this page from the generated public API reference, or explicitly promote and support the helper as part of the public API.
(Based on your team's feedback about curating generated SDK API references.) [905c8e52-ba52-4980-8997-2766c3641fb0]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| ## Functions | ||
|
|
||
| ### `peers_are_inline_input` |
There was a problem hiding this comment.
P3: This page exposes peers_are_inline_input as a public SDK API even though the helper is internal and is not exported from infrahub_sdk.node. Remove this page from the generated public API reference, or explicitly promote and support the helper as part of the public API.
(Based on your team's feedback about curating generated SDK API references.) [905c8e52-ba52-4980-8997-2766c3641fb0]
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/inline_peers.mdx, line 12:
<comment>This page exposes `peers_are_inline_input` as a public SDK API even though the helper is internal and is not exported from `infrahub_sdk.node`. Remove this page from the generated public API reference, or explicitly promote and support the helper as part of the public API.
(Based on your team's feedback about curating generated SDK API references.) [905c8e52-ba52-4980-8997-2766c3641fb0]</comment>
<file context>
@@ -0,0 +1,18 @@
+
+## Functions
+
+### `peers_are_inline_input`
+
+```python
</file context>
… from an object file (#11370) The SDK commit 1.12 ships creates the pool first and each range afterwards as a node of its own, which Infrahub refuses as overlapping the range the pool already holds. The SDK now sends the ranges as the fields of the pool mutation (opsmill/infrahub-sdk-python#1469), so an object file that declares `ranges` loads whether or not the pool exists. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…1.12 The SDK now includes opsmill/infrahub-sdk-python#1470, which adds the shared argument to GitRepo, and opsmill/infrahub-sdk-python#1469. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
The
rangesof aCoreNumberPoolare bounds in its mutation input, not references to range nodes. Three paths did not account for that:add(),extend()andremove()on therangesrelationship did not change the pool: the first two dropped the range without an error, andremove()sent the remaining ranges as references, which Infrahub refused.infrahubctl object update --set ranges=...lost the edit in silence.rangesas objects failed. The loader created the pool without its ranges, so Infrahub gave it the default range, then created each declared range as a node of its own, which Infrahub refused as overlapping: the default range on a new pool, or the range it already held on an existing one. Reported by Wim.Close IFC-3543
What each entry point does
Nothing that produced the intended result is taken away. What was silent becomes explicit, and the object file starts working.
pool.ranges.add(),.extend(),.remove()FeatureNotSupportedError, naming the ways outpool.start_rangeandpool.end_rangeobject create CoreNumberPool --set ranges='[{"start": 1, "end": 10}]'object create CoreNumberPool --set ranges=range-1object update CoreNumberPool --set ranges=...object create --file,object update --file,object loadAn object file sets the ranges, on a new pool and on one that already exists:
The rule is keyed on the
(name, peer)pair, so another relationship toCoreNumberPoolRangekeeps references,add()andremove().Not covered
pool.ranges.peers: editing the list is not supported and the SDK does not report it.add_relationships(),remove_relationships()andinfrahubctl loadof an export reach Infrahub through other paths and bypass the refusal.The table of relationships is a stopgap. The lasting fix belongs in Infrahub: a schema that states the mutation takes the peer's fields, or a mutation that accepts references like every other relationship.
Tests
Unit tests cover each refused method for both clients, both CLI guards, and the object file in each form it accepts. The object-file failure lands red in its own commit before the fix.
Run against a live Infrahub 1.12.0.post1, with testcontainers:
What that covers: the ranges a create and an upsert declare, the way out the refusal names, a range node added, changed and deleted on its own, an object file loaded twice against the same pool, and attaching and detaching numbers.
🤖 Generated with Claude Code