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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds 🚥 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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #850 +/- ##
===========================================
+ Coverage 80.36% 80.41% +0.04%
===========================================
Files 115 115
Lines 9875 9884 +9
Branches 1504 1500 -4
===========================================
+ Hits 7936 7948 +12
- 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:
|
1c38182 to
1857ea0
Compare
Deploying infrahub-sdk-python with
|
| Latest commit: |
f34027b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://14b7fb18.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pmi-20260220-sdk-query-from.infrahub-sdk-python.pages.dev |
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
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
|
also, please update to target |
I do not find release-1.8 |
ab0bff1 to
3dde9ee
Compare
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 |
3dde9ee to
becef27
Compare
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
infrahub_sdk/node/attribute.py
Outdated
| 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
There was a problem hiding this comment.
I usually prefer to hide implementation details at the bottom files. Since this is an internal class that is only used to build an object, I would rather have someone read the main attribute class and then read these implementation details if necessary.
By encapsulating the complexity in a dedicated object with clear variables and methods, we prevent the reader from having to open this Pandora's box if he is not interested in implementation details.
There was a problem hiding this comment.
(I have moved the class to the top of the file, but I am interested in the response)
There was a problem hiding this comment.
Matter of taste I guess. I prefer it top of the file too as I like to know that a class exists before stumbling upon an instance of it.
a488e50 to
8cde1bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 153-157: The annotation for properties_object is wrong: it
declares dict[str, dict] but the comprehension assigns values returned by
NodeProperty._generate_input_data(), which returns str | None (see class
NodeProperty and method _generate_input_data); change the type annotation of
properties_object to dict[str, str | None] (or dict[str, Optional[str]]) to
match actual values and keep the comprehension using getattr(self,
property_name)._generate_input_data() as-is.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdxinfrahub_sdk/node/attribute.pytests/unit/sdk/test_attribute_generate_input_data.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/sdk/test_attribute_generate_input_data.py
- docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx (1)
38-38: FormatTruewith backticks for consistency.The return value
Trueshould be wrapped in backticks to match standard documentation formatting for literal values.-- 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` at line 38, Update the return description in attribute.mdx so the literal True is formatted as code: replace "True if the attribute value is a resource pool node or was explicitly allocated from a pool." with " `True` if the attribute value is a resource pool node or was explicitly allocated from a pool." to match documentation formatting conventions for literal values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdx`:
- Line 38: Update the return description in attribute.mdx so the literal True is
formatted as code: replace "True if the attribute value is a resource pool node
or was explicitly allocated from a pool." with " `True` if the attribute value
is a resource pool node or was explicitly allocated from a pool." to match
documentation formatting conventions for literal values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/python-sdk/sdk_ref/infrahub_sdk/node/attribute.mdxinfrahub_sdk/node/attribute.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
There was a problem hiding this comment.
♻️ Duplicate comments (1)
infrahub_sdk/node/attribute.py (1)
57-59:⚠️ Potential issue | 🟡 MinorAvoid mutating caller-owned input in
Attribute.__init__.
data.pop("from_pool")mutates the original dict, which can leak side effects to callers (and shared node data). Prefer copying before mutation.Suggested fix
- if isinstance(data, dict) and "from_pool" in data: - self._from_pool = data.pop("from_pool") + if isinstance(data, dict) and "from_pool" in data: + data = dict(data) + 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 57 - 59, In Attribute.__init__, avoid mutating the caller-owned dict by copying the incoming data before popping "from_pool": instead of calling data.pop("from_pool") on the passed-in dict, make a shallow copy (e.g., data_copy = dict(data) or data.copy()), read and remove "from_pool" from that copy to set self._from_pool, and call data_copy.setdefault("value", None) so all subsequent uses in the constructor use the copied dict and the original caller dict is not modified.
🧹 Nitpick comments (1)
infrahub_sdk/node/attribute.py (1)
142-143: Update stale return-type wording in docstring.The method now returns
_GraphQLPayloadAttribute, but the docstring still saysResolvedValue.🤖 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 142 - 143, The docstring's return-type is stale: update the return description to reflect the actual return type _GraphQLPayloadAttribute (instead of ResolvedValue). Locate the method in infrahub_sdk/node/attribute.py that currently documents "Returns a ResolvedValue object" and change that sentence to state it returns a _GraphQLPayloadAttribute object (or equivalent brief description of that type), keeping the rest of the docstring intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 57-59: In Attribute.__init__, avoid mutating the caller-owned dict
by copying the incoming data before popping "from_pool": instead of calling
data.pop("from_pool") on the passed-in dict, make a shallow copy (e.g.,
data_copy = dict(data) or data.copy()), read and remove "from_pool" from that
copy to set self._from_pool, and call data_copy.setdefault("value", None) so all
subsequent uses in the constructor use the copied dict and the original caller
dict is not modified.
---
Nitpick comments:
In `@infrahub_sdk/node/attribute.py`:
- Around line 142-143: The docstring's return-type is stale: update the return
description to reflect the actual return type _GraphQLPayloadAttribute (instead
of ResolvedValue). Locate the method in infrahub_sdk/node/attribute.py that
currently documents "Returns a ResolvedValue object" and change that sentence to
state it returns a _GraphQLPayloadAttribute object (or equivalent brief
description of that type), keeping the rest of the docstring intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrahub_sdk/node/attribute.pyinfrahub_sdk/node/node.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
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
Chores