IHS-156 / #497 : support from_pool attributes on Python SDK queries #850
IHS-156 / #497 : support from_pool attributes on Python SDK queries #850
Conversation
WalkthroughThis pull request refactors the Python SDK's mutation payload handling to support pool-aware attributes. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
polmichel
changed the title
polmichel
changed the title
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #850 +/- ##
===========================================
+ Coverage 80.36% 80.41% +0.04%
===========================================
Files 115 115
Lines 9875 9883 +8
Branches 1504 1500 -4
===========================================
+ Hits 7936 7947 +11
- Misses 1417 1418 +1
+ Partials 522 518 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
polmichel
force-pushed
the
pmi-20260220-sdk-query-from-pool-attribute
branch
from
1c38182 to
1857ea0
Compare
Deploying infrahub-sdk-python with
|
| Latest commit: |
0ca55cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b77c6578.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pmi-20260220-sdk-query-from.infrahub-sdk-python.pages.dev |
polmichel
force-pushed
the
pmi-20260220-sdk-query-from-pool-attribute
branch
from
e4346d3 to
fc672b1
Compare
|
This GraphQL query generation algorithm, which considers different types of attributes, would require refactoring. It seems to be a potential source of bugs, and any addition, like mine, could cause regressions. |
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
left some comments on tests that I now see you just moved around, so feel free to ignore them if you want
infrahub_sdk/node/attribute.py
Outdated
| def _from_pool_attribute(self) -> bool | Any: | ||
| return (isinstance(self.value, CoreNodeBase) and self.value.is_resource_pool()) or self._from_pool is not None |
There was a problem hiding this comment.
I'd say force it to return a bool and call it _is_from_pool_attribute to make the name show that it returns a bool
infrahub_sdk/node/attribute.py
Outdated
| early_return = self._generate_value_data(data, variables) | ||
| if early_return is not None: | ||
| return early_return |
There was a problem hiding this comment.
I think naming a variable early_return that then returns early is a bit like including function in the name of a function.
_generate_input_data was not in a good state before your changes and I think that trying to move some of the logic to a different private method is a good idea, but we should avoid passing dictionaries to be modified in place. it's best practice to have a function return something instead of mutating an argument.
and it looks like the early return is really only when self.value is None, so maybe that can be made more explicit here
There was a problem hiding this comment.
I will do a refactor of this method to make it clearer!
There was a problem hiding this comment.
I tried to use an intermediate object and comment the different use cases.
I generated all unit tests covering the method in the file test_attribute_generate_input_data.py. I reviewed them and they seem good to me. This could be used as a "documentation" on which cases are handled in the method
|
also, please update to target |
I do not find release-1.8 |
polmichel
force-pushed
the
pmi-20260220-sdk-query-from-pool-attribute
branch
from
ab0bff1 to
3dde9ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
infrahub_sdk/node/attribute.py (1)
163-186: 🛠️ Refactor suggestion | 🟠 Major
_GraphQLPayloadAttribute:add_propertiesmutates in-place andto_dictis unusedTwo issues:
In-place mutation on a NamedTuple field.
add_propertiesdirectly modifiesself.payload_dict(a shared mutable dict). This contradicts the immutability implied byNamedTupleand the feedback that was given on the previous commit: "we should avoid passing dictionaries to be modified in place; it's best practice to have a function return something." The same principle applies to a method that mutatesself.payload_dict. Consider either using adataclass(which makes mutability explicit) or havingadd_propertiesreturn a new_GraphQLPayloadAttribute.
to_dict()is never called. Every consumer innode.pyaccesses.payload_dictand.variablesdirectly.to_dict()returns{"data": ..., "variables": ...}– a structure no caller uses – making it dead code that could mislead future readers.♻️ Option A – switch to a dataclass to make mutability explicit
-class _GraphQLPayloadAttribute(NamedTuple): - """Result of resolving an attribute value for a GraphQL mutation. - ... - """ - - payload_dict: dict[str, Any] - variables: dict[str, Any] - need_additional_properties: bool - - def to_dict(self) -> dict[str, Any]: - return {"data": self.payload_dict, "variables": self.variables} - - def add_properties(self, properties_flag: dict[str, Any], properties_object: dict[str, dict]) -> None: - if self.need_additional_properties: - for prop_name, prop in properties_flag.items(): - self.payload_dict[prop_name] = prop - - for prop_name, prop in properties_object.items(): - self.payload_dict[prop_name] = prop +from dataclasses import dataclass, field + +@dataclass +class _GraphQLPayloadAttribute: + """Result of resolving an attribute value for a GraphQL mutation.""" + + payload_dict: dict[str, Any] = field(default_factory=dict) + variables: dict[str, Any] = field(default_factory=dict) + need_additional_properties: bool = False + + def add_properties(self, properties_flag: dict[str, Any], properties_object: dict[str, dict]) -> None: + if self.need_additional_properties: + self.payload_dict.update(properties_flag) + self.payload_dict.update(properties_object)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/node/attribute.py` around lines 163 - 186, The _GraphQLPayloadAttribute currently mutates its payload_dict in add_properties which contradicts its NamedTuple immutability and the project's guideline to avoid in-place dict modification; change the design either by converting _GraphQLPayloadAttribute to a mutable dataclass (so add_properties may safely mutate payload_dict) or make add_properties return a new _GraphQLPayloadAttribute with merged payload_dict (do not mutate self.payload_dict); additionally remove or update the unused to_dict method (either delete it or change callers to use it) and update all references in node.py that access .payload_dict/.variables to use the new API (constructor/returned instance or to_dict) accordingly.tests/unit/sdk/pool/test_relationship_from_pool.py (1)
103-121:⚠️ Potential issue | 🟠 MajorSame
InfrahubNode/InfrahubNodeSyncmismatch intest_create_mutation_query_with_resource_pool_relationship.Line 117 creates
deviceasInfrahubNodein the sync branch, same as the bug at line 60 above.🐛 Proposed fix
- device = InfrahubNode( + device = InfrahubNodeSync( client=client, schema=simple_device_schema, data={"name": "device-01", "primary_address": ip_pool, "ip_address_pool": ip_pool}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/pool/test_relationship_from_pool.py` around lines 103 - 121, The test creates a synchronous object mismatch: in the sync branch you instantiate ip_prefix and ip_pool as InfrahubNodeSync but create device as InfrahubNode (see device variable in test_create_mutation_query_with_resource_pool_relationship); change the device instantiation to use InfrahubNodeSync with the same schema simple_device_schema and data payload so all related objects in the sync branch are InfrahubNodeSync instances and types are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 28-34: The code in the Attribute initializer uses
data.pop("from_pool") which mutates the caller's dict; change the branch
handling to avoid side-effects by first making a shallow copy of the incoming
dict (e.g., data = {**data}) before removing "from_pool", or alternatively read
via data.get("from_pool") and rebuild a new dict without that key (e.g., {k: v
for k, v in data.items() if k != "from_pool"}); ensure this happens where
self._from_pool is set so _from_pool still receives the value but the original
caller dict is not modified.
In `@tests/unit/sdk/pool/test_pool_queries.py`:
- Around line 21-29: The test function test_get_pool_allocated_resources has an
incorrect type for the fixture parameter mock_schema_query_ipam—this fixture is
a side-effect pytest fixture that doesn't return an HTTPXMock instance; update
the function signature to remove the HTTPXMock annotation for
mock_schema_query_ipam (either annotate it as None or use a generic type like
Any) so the parameter correctly reflects the fixture's behavior and avoids
misleading typing.
In `@tests/unit/sdk/pool/test_relationship_from_pool.py`:
- Around line 46-64: The device in the sync branch is incorrectly instantiated
as InfrahubNode instead of InfrahubNodeSync; change the creation of device to
use InfrahubNodeSync (the variable name device, instantiated with client=client,
schema=simple_device_schema, data={...}) so the sync parametrized test actually
exercises the sync client path, and apply the same replacement at the other
occurrence in test_create_mutation_query_with_resource_pool_relationship (the
second device instantiation around line 117).
---
Duplicate comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 163-186: The _GraphQLPayloadAttribute currently mutates its
payload_dict in add_properties which contradicts its NamedTuple immutability and
the project's guideline to avoid in-place dict modification; change the design
either by converting _GraphQLPayloadAttribute to a mutable dataclass (so
add_properties may safely mutate payload_dict) or make add_properties return a
new _GraphQLPayloadAttribute with merged payload_dict (do not mutate
self.payload_dict); additionally remove or update the unused to_dict method
(either delete it or change callers to use it) and update all references in
node.py that access .payload_dict/.variables to use the new API
(constructor/returned instance or to_dict) accordingly.
In `@tests/unit/sdk/pool/test_relationship_from_pool.py`:
- Around line 103-121: The test creates a synchronous object mismatch: in the
sync branch you instantiate ip_prefix and ip_pool as InfrahubNodeSync but create
device as InfrahubNode (see device variable in
test_create_mutation_query_with_resource_pool_relationship); change the device
instantiation to use InfrahubNodeSync with the same schema simple_device_schema
and data payload so all related objects in the sync branch are InfrahubNodeSync
instances and types are consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
AGENTS.mdchangelog/497.fixed.mdinfrahub_sdk/node/attribute.pyinfrahub_sdk/node/node.pytests/AGENTS.mdtests/unit/sdk/conftest.pytests/unit/sdk/pool/__init__.pytests/unit/sdk/pool/conftest.pytests/unit/sdk/pool/test_allocate.pytests/unit/sdk/pool/test_attribute_from_pool.pytests/unit/sdk/pool/test_pool_queries.pytests/unit/sdk/pool/test_relationship_from_pool.pytests/unit/sdk/test_attribute_generate_input_data.pytests/unit/sdk/test_client.pytests/unit/sdk/test_node.py
💤 Files with no reviewable changes (2)
- tests/unit/sdk/test_node.py
- tests/unit/sdk/test_client.py
oh duh |
polmichel
force-pushed
the
pmi-20260220-sdk-query-from-pool-attribute
branch
from
3dde9ee to
becef27
Compare
github-actions
bot
added
the
type/documentation
polmichel
force-pushed
the
pmi-20260220-sdk-query-from-pool-attribute
branch
from
becef27 to
0ca55cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/unit/sdk/pool/test_relationship_from_pool.py (1)
117-121:⚠️ Potential issue | 🟠 MajorSync branch still uses
InfrahubNodeinstead ofInfrahubNodeSyncfordevice.In the
else(sync) branch,ip_prefixandip_poolare correctlyInfrahubNodeSync, butdeviceon line 117 isInfrahubNode. This makes theclient_type == "sync"parametrization provide no sync-path coverage for mutation query generation.🐛 Proposed fix
- device = InfrahubNode( + device = InfrahubNodeSync( client=client, schema=simple_device_schema, data={"name": "device-01", "primary_address": ip_pool, "ip_address_pool": ip_pool}, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/sdk/pool/test_relationship_from_pool.py` around lines 117 - 121, In the sync branch replace the incorrect asynchronous class usage by instantiating device with InfrahubNodeSync instead of InfrahubNode so the sync path is exercised; locate the block where ip_prefix and ip_pool are created as InfrahubNodeSync and change the device assignment (variable device) to use InfrahubNodeSync with the same client, schema, and data parameters to match the sync branch behavior (this ensures client_type == "sync" covers mutation query generation).infrahub_sdk/node/attribute.py (1)
28-34:⚠️ Potential issue | 🟡 Minor
data.pop("from_pool")still mutates the caller's dict in-place.This was flagged in a prior review and remains unaddressed. A shallow copy before
.pop()avoids surprising side-effects for any caller holding a reference to the same dict.🛡️ Proposed fix
if isinstance(data, dict) and "from_pool" in data: - self._from_pool = data.pop("from_pool") - data.setdefault("value", None) + data = {**data} # shallow copy to avoid mutating caller's dict + self._from_pool = data.pop("from_pool") + data.setdefault("value", None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/node/attribute.py` around lines 28 - 34, The code currently mutates the caller's dict by calling data.pop("from_pool"); fix this in the Attribute initialization by making a shallow copy of the incoming dict before removing "from_pool" so callers' dicts are unchanged—e.g., when isinstance(data, dict) and "from_pool" in data, assign a copy (data_copy = data.copy()), extract the value from the copy into self._from_pool, then operate on that copy (setting default "value" on the copy) and continue using the copy for further processing; ensure all references in this branch use the copy instead of the original data so only the Attribute instance is mutated.
🧹 Nitpick comments (1)
infrahub_sdk/node/attribute.py (1)
169-192:NamedTuplewith in-place mutation viaadd_propertiesis misleading.
NamedTuplesignals immutable value semantics, butadd_propertiesmutatespayload_dictin place. This works at runtime (tuple immutability is shallow), but it's surprising for readers and type-checkers. A@dataclasswould more honestly communicate mutability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/node/attribute.py` around lines 169 - 192, The _GraphQLPayloadAttribute is declared as a NamedTuple but its add_properties method mutates payload_dict in place, so change the declaration to a mutable dataclass: replace class _GraphQLPayloadAttribute(NamedTuple) with `@dataclass` class _GraphQLPayloadAttribute and use field(default_factory=dict) for payload_dict and variables to avoid shared mutable defaults; keep the need_additional_properties: bool field, preserve the to_dict method signature, and keep the add_properties implementation semantics so callers see identical behavior while the type now correctly signals mutability; also add the necessary imports from dataclasses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx`:
- Around line 36-39: The "Returns" section in attribute.mdx currently splits a
single return description into two list items; combine the two bullets under the
Returns heading into one line such as "True if the attribute value is a resource
pool node or was explicitly allocated from a pool." so the return description is
a single list item; update the Returns block in attribute.mdx to replace the two
items with this single consolidated sentence.
---
Duplicate comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 28-34: The code currently mutates the caller's dict by calling
data.pop("from_pool"); fix this in the Attribute initialization by making a
shallow copy of the incoming dict before removing "from_pool" so callers' dicts
are unchanged—e.g., when isinstance(data, dict) and "from_pool" in data, assign
a copy (data_copy = data.copy()), extract the value from the copy into
self._from_pool, then operate on that copy (setting default "value" on the copy)
and continue using the copy for further processing; ensure all references in
this branch use the copy instead of the original data so only the Attribute
instance is mutated.
In `@tests/unit/sdk/pool/test_relationship_from_pool.py`:
- Around line 117-121: In the sync branch replace the incorrect asynchronous
class usage by instantiating device with InfrahubNodeSync instead of
InfrahubNode so the sync path is exercised; locate the block where ip_prefix and
ip_pool are created as InfrahubNodeSync and change the device assignment
(variable device) to use InfrahubNodeSync with the same client, schema, and data
parameters to match the sync branch behavior (this ensures client_type == "sync"
covers mutation query generation).
---
Nitpick comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 169-192: The _GraphQLPayloadAttribute is declared as a NamedTuple
but its add_properties method mutates payload_dict in place, so change the
declaration to a mutable dataclass: replace class
_GraphQLPayloadAttribute(NamedTuple) with `@dataclass` class
_GraphQLPayloadAttribute and use field(default_factory=dict) for payload_dict
and variables to avoid shared mutable defaults; keep the
need_additional_properties: bool field, preserve the to_dict method signature,
and keep the add_properties implementation semantics so callers see identical
behavior while the type now correctly signals mutability; also add the necessary
imports from dataclasses.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdxinfrahub_sdk/node/attribute.pyinfrahub_sdk/node/node.pytests/unit/sdk/pool/test_relationship_from_pool.pytests/unit/sdk/test_attribute_generate_input_data.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/sdk/test_attribute_generate_input_data.py
| **Returns:** | ||
|
|
||
| - True if the attribute value is a resource pool node or was explicitly | ||
| - allocated from a pool. |
There was a problem hiding this comment.
Return description is incorrectly split across two list items.
The second bullet ("allocated from a pool.") is a continuation of the first, not a separate return value. This renders oddly in MDX.
📝 Proposed fix
**Returns:**
-- True if the attribute value is a resource pool node or was explicitly
-- allocated from a pool.
+- `True` if the attribute value is a resource pool node or was explicitly allocated from a pool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx` around lines 36
- 39, The "Returns" section in attribute.mdx currently splits a single return
description into two list items; combine the two bullets under the Returns
heading into one line such as "True if the attribute value is a resource pool
node or was explicitly allocated from a pool." so the return description is a
single list item; update the Returns block in attribute.mdx to replace the two
items with this single consolidated sentence.
There was a problem hiding this comment.
I think the robot is right here
There was a problem hiding this comment.
@ajtmccarty Got it! I'll fix the formatting to merge those two bullets into a single line.
Task: Fix documentation formatting for is_from_pool_attribute return description
- Combine the two bullet points in the Returns section into a single consolidated sentence
- Add backticks around
Truefor consistency with documentation standards
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
| **Returns:** | ||
|
|
||
| - True if the attribute value is a resource pool node or was explicitly | ||
| - allocated from a pool. |
There was a problem hiding this comment.
I think the robot is right here
|
|
||
| payload_dict: dict[str, Any] | ||
| variables: dict[str, Any] | ||
| need_additional_properties: bool |
There was a problem hiding this comment.
I'd call this needs_metadata, that's generally what we call the source, owner, is_protected flags/properties
| return (isinstance(self.value, CoreNodeBase) and self.value.is_resource_pool()) or self._from_pool is not None | ||
|
|
||
|
|
||
| class _GraphQLPayloadAttribute(NamedTuple): |
There was a problem hiding this comment.
I'd say move it to the top of the file
| return {"data": self.payload_dict, "variables": self.variables} | ||
|
|
||
| def add_properties(self, properties_flag: dict[str, Any], properties_object: dict[str, dict]) -> None: | ||
| if self.need_additional_properties: |
There was a problem hiding this comment.
i personally prefer the early return to reduce indentation levels
There was a problem hiding this comment.
I think this is much easier to understand now
in the sdk we have to be careful to maintain backwards compatibility for the public API, but it looks like these are all internal changes, so I think it should be fine
2 participants
Why
Problem statement
Python SDK was wrongly generating the GraphQL queries containing from_pool attributes as
The generated query was encapsulating the from_pool attribute into a
value: {}when generated by aNumberPoolGoals
This PR aims to fix the query generation while not encapsulating the from_pool part into a
value: {}.Final query should look like:
Closes IHS-156 / #497
Non-goals
The pool-related unit tests were reorganized to provide a clearer view of the current test cases in the InfraHub SDK project ->
python_sdk/tests/unit/sdk/poolWhat changed
infrahub_sdk.node.attribute.Attribute.__init__infrahub_sdk.node.attribute.Attribute._generate_value_datainfrahub_sdk.node.attribute.Attribute._generate_mutation_querywhich now correctly handle the from_pool structure. Before this change, the from_pool structure was only managed when it was a part of an
InfrahubNodeobject.How to review
python_sdk/infrahub_sdk/node/attribute.pyHow to test
Checklist
uv run towncrier create ...)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests