Allow recursive convertion to InfrahubNode objects#390
Allow recursive convertion to InfrahubNode objects#390ogenstad merged 1 commit intoinfrahub-developfrom
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
631f9c4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://48935c87.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://pog-recursive-relationships.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #390 +/- ##
====================================================
+ Coverage 75.56% 75.60% +0.04%
====================================================
Files 113 113
Lines 9514 9522 +8
Branches 1448 1452 +4
====================================================
+ Hits 7189 7199 +10
+ Misses 1838 1832 -6
- Partials 487 491 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ogenstad
force-pushed
the
pog-recursive-relationships-IHS-129
branch
from
b034f0e to
a25bbf7
Compare
ogenstad
force-pushed
the
pog-recursive-relationships-IHS-129
branch
from
a25bbf7 to
102ef44
Compare
WalkthroughAdds optional recursive traversal to relationship processing by introducing a Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrahub_sdk/node/node.py (2)
1-1: Fix formatting to pass ruff check.The pipeline indicates that
ruff formatwould reformat 1 file. Please runruff formatto fix the code style issues.
1091-1144: Add protection against infinite recursion from circular relationships.The recursive traversal does not guard against circular relationships. If node A references node B, and node B references node A (directly or through a chain), this will cause infinite recursion and a stack overflow.
Consider tracking visited node IDs to prevent reprocessing:
async def _process_relationships( self, node_data: dict[str, Any], branch: str, related_nodes: list[InfrahubNode], timeout: int | None = None, recursive: bool = False, + visited_ids: set[str] | None = None, ) -> None: """Processes the Relationships of a InfrahubNode and add Related Nodes to a list. Args: node_data (dict[str, Any]): The item from the GraphQL response corresponding to the node. branch (str): The branch name. related_nodes (list[InfrahubNode]): The list to which related nodes will be appended. timeout (int, optional): Overrides default timeout used when querying the graphql API. Specified in seconds. + recursive (bool): Whether to recursively process nested relationships. + visited_ids (set[str], optional): Set of already-visited node IDs to prevent circular traversal. """ + if visited_ids is None: + visited_ids = set() + + if self.id and self.id in visited_ids: + return + + if self.id: + visited_ids.add(self.id) + for rel_name in self._relationships: rel = getattr(self, rel_name) if rel and isinstance(rel, RelatedNode): relation = node_data["node"].get(rel_name, None) if relation and relation.get("node", None): related_node = await InfrahubNode.from_graphql( client=self._client, branch=branch, data=relation, timeout=timeout, ) related_nodes.append(related_node) if recursive: await related_node._process_relationships( node_data=relation, branch=branch, related_nodes=related_nodes, recursive=recursive, + visited_ids=visited_ids, ) elif rel and isinstance(rel, RelationshipManager): peers = node_data["node"].get(rel_name, None) if peers and peers["edges"]: for peer in peers["edges"]: related_node = await InfrahubNode.from_graphql( client=self._client, branch=branch, data=peer, timeout=timeout, ) related_nodes.append(related_node) if recursive: await related_node._process_relationships( node_data=peer, branch=branch, related_nodes=related_nodes, recursive=recursive, + visited_ids=visited_ids, )Apply the same pattern to the synchronous version at lines 1878-1932.
🧹 Nitpick comments (2)
changelog/389.changed.md (1)
1-1: Consider clarifying the warning about potential issues.The changelog warns that transforms or generators "could cause issues" but doesn't specify what kind of issues (e.g., missing nodes in the store, incomplete object trees, exceptions). Consider being more explicit about the failure mode to help users understand the impact.
Example revision:
-Updated behaviour for recursive lookups for the conversion of nested relationships. Note that this change could cause issues in transforms or generators that use the convert_query_response feature if "id" or "__typename" isn't requested for nested related objects. +Updated behaviour for recursive lookups for the conversion of nested relationships. Note that nested related objects will not be properly converted to InfrahubNode objects if "id" or "__typename" fields are not included in the GraphQL query for those nested objects when using the convert_query_response feature.infrahub_sdk/node/node.py (1)
1112-1125: Validate presence of required fields before recursive processing.The changelog warns that recursive processing may misbehave if "id" or "__typename" are missing from nested related objects, but there's no validation here. Consider adding a check and logging a warning or raising an informative error.
if relation and relation.get("node", None): + node_obj = relation.get("node", {}) + if recursive and (not node_obj.get("id") or not node_obj.get("__typename")): + # Log or raise a clear error about missing required fields + import logging + logger = logging.getLogger(__name__) + logger.warning( + f"Skipping recursive processing for relationship '{rel_name}' on node {self.id}: " + f"missing 'id' or '__typename' in nested node data. " + f"Ensure your GraphQL query includes these fields for nested relationships." + ) + continue related_node = await InfrahubNode.from_graphql(Apply similar validation at lines 1130-1143 for cardinality=many relationships and in the sync version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changelog/389.changed.md(1 hunks)infrahub_sdk/node/node.py(56 hunks)infrahub_sdk/operation.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
infrahub_sdk/operation.py (1)
infrahub_sdk/checks.py (1)
branch_name(134-144)
infrahub_sdk/node/node.py (5)
infrahub_sdk/exceptions.py (4)
FeatureNotSupportedError(154-155)NodeNotFoundError(71-90)ResourceNotDefinedError(97-102)SchemaNotFoundError(58-62)infrahub_sdk/node/relationship.py (10)
RelationshipManager(99-219)RelationshipManagerBase(21-96)has_update(56-57)fetch(147-181)fetch(270-304)_generate_query_data(67-96)_generate_mutation_query(62-64)_generate_input_data(59-60)extend(195-198)extend(318-321)infrahub_sdk/node/related_node.py (18)
display_label(111-114)get(216-226)get(263-273)typename(117-120)kind(123-126)is_resource_pool(101-104)initialized(107-108)RelatedNodeBase(17-179)RelatedNode(182-226)id(83-86)fetch(204-210)fetch(251-257)peer(213-214)peer(260-261)_generate_query_data(154-179)_generate_mutation_query(147-151)_generate_input_data(128-145)RelatedNodeSync(229-273)infrahub_sdk/client.py (27)
get(389-405)get(408-424)get(427-443)get(446-462)get(465-481)get(484-500)get(502-560)get(2198-2214)get(2217-2233)get(2236-2252)get(2255-2271)get(2274-2290)get(2293-2309)get(2311-2369)InfrahubClient(318-1688)create(347-353)create(356-362)create(364-379)create(1723-1729)create(1732-1738)create(1740-1754)filters(729-747)filters(750-768)filters(770-901)filters(2021-2039)filters(2042-2060)filters(2062-2195)infrahub_sdk/query_groups.py (4)
add_related_nodes(114-125)add_related_nodes(208-219)add_related_groups(127-138)add_related_groups(221-232)
🪛 GitHub Actions: CI
infrahub_sdk/node/node.py
[error] 1-1: ruff format check failed. 1 file would be reformatted. Run 'ruff format' (or 'ruff format --fix') to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
infrahub_sdk/node/node.py (2)
8-29: LGTM! Import formatting improved.The multi-line import formatting improves readability.
1097-1097: Good backward-compatible API design.The
recursiveparameter defaults toFalse, maintaining backward compatibility while enabling the new behavior when explicitly requested.infrahub_sdk/operation.py (1)
74-76: LGTM! Recursive processing now enabled for convert_query_response.This change correctly activates the recursive relationship processing for nodes when
convert_query_responseis enabled, which addresses issue #389.However, ensure that the circular relationship protection suggested in
infrahub_sdk/node/node.pyis implemented, as this call site will trigger recursive traversal without limits otherwise.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
infrahub_sdk/node/node.py (2)
944-997: Recursive relationship traversal works but needs cycle protection and timeout propagationThe new
recursive: bool = Falseflag and recursive calls in_process_relationshipscorrectly extend traversal to nested relationships for both cardinality=one and cardinality=many. For tree‑like graphs, this gives the expected behavior: withrecursive=True, all reachable related nodes are appended torelated_nodes, matching the new deep‑nesting tests.There are a couple of important edge cases to address:
- Cycles can cause unbounded recursion
Because recursion follows all relationships without tracking visited nodes, any schema/data that forms a cycle (e.g., a query that includes bothdevice.interfaces.device...or other bidirectional links) will cause_process_relationshipsto revisit the same logical nodes indefinitely until hitting recursion depth or blowing uprelated_nodes. That’s a real risk in a graph model.- Timeout is dropped on deeper levels
The initial call honors thetimeoutparameter when invokingfrom_graphql, but recursive calls do not passtimeoutto_process_relationships, so deeper levels will always seetimeout=None. This is inconsistent and could be surprising if schema lookups or future I/O inside_process_relationshipsstart relying on that parameter.- Duplicates in related_nodes
In shared‑subtree scenarios, the same logical node can be appended multiple times as you traverse different paths. That may be acceptable forconvert_query_response, but if consumers expect a set‑like list of unique nodes, they’ll see duplicates.To harden this:
- async def _process_relationships( - self, - node_data: dict[str, Any], - branch: str, - related_nodes: list[InfrahubNode], - timeout: int | None = None, - recursive: bool = False, - ) -> None: + async def _process_relationships( + self, + node_data: dict[str, Any], + branch: str, + related_nodes: list[InfrahubNode], + timeout: int | None = None, + recursive: bool = False, + _visited: set[tuple[str, str]] | None = None, + ) -> None: + if _visited is None: + _visited = set() @@ - related_node = await InfrahubNode.from_graphql( + related_node = await InfrahubNode.from_graphql( client=self._client, branch=branch, data=relation, timeout=timeout, ) - related_nodes.append(related_node) - if recursive: + key = (related_node.typename, related_node.id) + if key not in _visited: + _visited.add(key) + related_nodes.append(related_node) + if recursive and key not in _visited: await related_node._process_relationships( node_data=relation, branch=branch, related_nodes=related_nodes, - recursive=recursive, + timeout=timeout, + recursive=recursive, + _visited=_visited, ) @@ - related_node = await InfrahubNode.from_graphql( + related_node = await InfrahubNode.from_graphql( client=self._client, branch=branch, data=peer, timeout=timeout, ) - related_nodes.append(related_node) - if recursive: + key = (related_node.typename, related_node.id) + if key not in _visited: + _visited.add(key) + related_nodes.append(related_node) + if recursive and key not in _visited: await related_node._process_relationships( node_data=peer, branch=branch, related_nodes=related_nodes, - recursive=recursive, + timeout=timeout, + recursive=recursive, + _visited=_visited, )You can choose whether to suppress duplicates entirely or only use
_visitedto guard recursion, but having at least a recursion guard is important to avoid pathological graphs blowing up traversal.
1623-1676: Recursive traversal in sync_process_relationshipsshares async concernsThe sync
_process_relationshipsimplementation correctly mirrors the async version’s recursive behavior for bothRelatedNodeSyncandRelationshipManagerSync, enabling deep traversal underrecursive=True.The same caveats apply here:
- Potential infinite recursion / large blow‑ups on cyclic graphs (no visited set).
timeoutis accepted but not propagated into recursive calls, so deeper traversals ignore the caller’s timeout.related_nodesmay contain duplicates when the same node is reachable via multiple paths.I recommend applying the same visited‑set and timeout‑propagation pattern here as suggested for the async version, e.g.:
- def _process_relationships( + def _process_relationships( self, node_data: dict[str, Any], branch: str, related_nodes: list[InfrahubNodeSync], timeout: int | None = None, - recursive: bool = False, + recursive: bool = False, + _visited: set[tuple[str, str]] | None = None, ) -> None: + if _visited is None: + _visited = set() @@ - related_node = InfrahubNodeSync.from_graphql( + related_node = InfrahubNodeSync.from_graphql( client=self._client, branch=branch, data=relation, timeout=timeout, ) - related_nodes.append(related_node) - if recursive: + key = (related_node.typename, related_node.id) + if key not in _visited: + _visited.add(key) + related_nodes.append(related_node) + if recursive and key not in _visited: related_node._process_relationships( node_data=relation, branch=branch, related_nodes=related_nodes, - recursive=recursive, + timeout=timeout, + recursive=recursive, + _visited=_visited, ) @@ - related_node = InfrahubNodeSync.from_graphql( + related_node = InfrahubNodeSync.from_graphql( client=self._client, branch=branch, data=peer, timeout=timeout, ) - related_nodes.append(related_node) - if recursive: + key = (related_node.typename, related_node.id) + if key not in _visited: + _visited.add(key) + related_nodes.append(related_node) + if recursive and key not in _visited: related_node._process_relationships( node_data=peer, branch=branch, related_nodes=related_nodes, - recursive=recursive, + timeout=timeout, + recursive=recursive, + _visited=_visited, )Keeping the async/sync implementations structurally in sync will also help avoid future drift.
🧹 Nitpick comments (4)
tests/unit/sdk/conftest.py (1)
2703-2835: Deep-nesting fixtures look consistent with existing schema patternsThe new
interface_l3_schema,nested_device_with_interfaces_schema, andnested_device_datafixtures correctly model a 3‑level graph (InfraDevice→InfraInterfaceL3→InfraIPAddress) and match the__typename/relationship conventions used elsewhere (schemaname/namespace→Infra*kinds,edges/nodelayout). That makes them appropriate for exercising recursive relationship processing.If you want to reduce duplication later, you could consider reusing or deriving from the existing
device_schema/ipaddress_schemafixtures instead of hand‑crafting similar schema dicts, but it's not necessary for correctness here.infrahub_sdk/node/node.py (1)
421-431: Seeding query skeleton with core node identifiers is appropriateInitializing
generate_query_data_initwithedges.nodecontainingid,hfid,display_label, and__typenameensures these fields are always requested for top‑level nodes. That’s aligned with howfrom_graphqlandconvert_query_responserely on__typename/id, and matches the expectations in the tests that assert these keys are present.The extra fields slightly increase query size but are a good trade‑off for correctness and for supporting recursive relationship processing.
If you find yourself repeating this field set elsewhere (e.g., relationship query builders), consider centralizing the “core node selection” keys in a small helper or constant to avoid drift.
tests/unit/sdk/test_node.py (2)
1380-1712: Input-data tests now cover richer relationship/property casesAcross the
test_create_input_data*andtest_update_input_data*group:
- The updated
datapayloads and expectations exercise more combinations of attribute properties (is_protected,owner,source) and relationship property fields (_relation__*), including pool relationships and optional relationships being set toNoneor emptied.test_create_input_data_with_IPHost_attributenow asserts the presence of"interface": Nonein the generated data, which reflects the logic that optional cardinality=one relationships are explicitly cleared when uninitialized and matches howipaddress_schemadefines its parent link.- The various “include_unmodified” vs “exclude_unmodified” tests for rfile and location schemas are aligned with
_strip_unmodifiedbehavior, including relation properties where they exist.These expectations are consistent with the current implementation of
_generate_input_dataand_strip_unmodified, and they provide good regression coverage for subtle relationship/attribute encoding rules.If these patterns expand further, consider extracting small helpers for building the expected
datadicts (e.g.,expected_rfile_data(with_property: bool)) to reduce duplication and make it clearer what’s logically changing between variants.
2547-2745: Deep-nesting recursion test is well designed; a couple of small cleanups are possible
test_process_relationships_recursive_deep_nestingdoes a solid job of validating the new recursive behavior:
- It builds a minimal but realistic schema cache for Device → InterfaceL3 → IPAddress and injects it into the client’s schema cache using
set_cache, matching howfrom_graphqlresolves child schemas.- For each client type:
- It verifies that
recursive=Falseonly appends the two interface nodes torelated_nodes, not the IP addresses.- It verifies that
recursive=Trueappends both interfaces and all three IP addresses, with assertions on both presence and overall count.- The same logic is exercised for both
InfrahubNodeandInfrahubNodeSync, keeping async/sync behavior aligned.Two minor improvements you could consider:
- The
interface_l3_schemaandipaddress_schemafixtures are currently unused in this test. You can either remove them from the parameter list or use them to derive theschema_datayou push into the cache to reduce duplication.- The two
schema_datadicts for standard and sync are identical; factoring them into a small helper (or a local variable before theif/else) would DRY the test slightly.Functionally, though, this test is excellent and directly validates the recursive traversal feature added in the node implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
infrahub_sdk/node/node.py(22 hunks)tests/unit/sdk/conftest.py(1 hunks)tests/unit/sdk/test_node.py(29 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/sdk/conftest.py (1)
infrahub_sdk/schema/main.py (4)
NodeSchemaAPI(312-314)NodeSchema(307-309)convert_api(284-285)convert_api(308-309)
infrahub_sdk/node/node.py (3)
infrahub_sdk/node/relationship.py (5)
RelationshipManager(99-219)RelationshipManagerBase(21-96)RelationshipManagerSync(222-341)fetch(147-181)fetch(270-304)infrahub_sdk/node/related_node.py (7)
get(216-226)get(263-273)kind(123-126)fetch(204-210)fetch(251-257)peer(213-214)peer(260-261)infrahub_sdk/client.py (22)
get(389-405)get(408-424)get(427-443)get(446-462)get(465-481)get(484-500)get(502-560)get(2198-2214)get(2217-2233)get(2236-2252)get(2255-2271)get(2274-2290)get(2293-2309)get(2311-2369)create(347-353)create(356-362)create(364-379)create(1723-1729)create(1732-1738)create(1740-1754)execute_graphql(907-991)execute_graphql(1767-1852)
tests/unit/sdk/test_node.py (3)
tests/unit/sdk/conftest.py (7)
BothClients(25-28)client(32-33)clients(42-47)nested_device_with_interfaces_schema(2739-2763)interface_l3_schema(2704-2735)ipaddress_schema(900-920)nested_device_data(2767-2835)infrahub_sdk/node/node.py (6)
InfrahubNodeSync(1169-1845)InfrahubNode(485-1166)from_graphql(513-527)from_graphql(1197-1211)_process_relationships(944-996)_process_relationships(1622-1675)infrahub_sdk/schema/__init__.py (1)
set_cache(133-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (22)
infrahub_sdk/node/node.py (12)
8-29: Exception and relationship imports are consistent and explicitThe widened imports for exceptions and relationship managers improve clarity without changing behavior, and they correctly match the usages in this module.
529-597: Relationship initialization for cardinality=one remains correctThe restructured
RelatedNodeconstruction in_init_relationshipsand__setattr__keeps the same arguments (name, branch, client, schema, data) in a clearer multiline form, and therel_datanormalization fromRelatedNodeinstances into{id,hfid,__typename,kind,display_label}dicts is unchanged. This continues to give you a fully initializedRelatedNodewrapper around whatever shape came in (raw dict orRelatedNode), and throws an explicitSchemaNotFoundErrorwhen the relationship schema cannot be located.No behavior changes here beyond readability.
645-667: Save/create/update refactor is stylistic onlyThe multiline calls to
createandupdateinInfrahubNode.savepreserve the previous control flow and arguments (allow_upsert,timeout,request_context) while improving readability. Group‑context updates and store population are untouched.No functional concerns.
817-833: Relationship mutation helpers remain straightforwardThe updated formatting of
add_relationships/remove_relationshipskeeps the mutation payload identical (sameaction,relation_to_update, andrelated_nodeswiring) and the tracker naming convention unchanged, so existing behavior and telemetry remain intact.
878-885: Allocated resource handling correctly wraps new related nodesIn
_process_mutation_result, theRelatedNodeinstantiation for allocated pool resources is just reformatted; it still uses the existing relationship schema (rel.schema), client, branch, and rawallocated_resourcedata before fetching and assigning. That continues to keep the attribute/relationship metadata in sync after pool allocation.
887-943: Create/update signatures remain API-compatibleThe
createandupdatesignatures forInfrahubNodeare only reformatted; parameters (allow_upsert,do_full_update,timeout,request_context) and internal logic are unchanged, so callers and tests depending on these signatures stay compatible.
1018-1055: Resource-pool allocation query now explicitly requests identifiersIn
get_pool_allocated_resources, the query skeleton now includesid,kind,branch, andidentifierunderedges.node, which aligns with the downstream logic that buildsnode_ids_per_kindand later callsfilters(kind=kind, ids=node_ids, ...). This makes the query self‑consistent and matches the updated test expectations that include__typenameon the allocated nodes.Looks good and is necessary for robust conversion of the allocation results into
InfrahubNodeinstances.
1235-1277: Sync relationship initialization mirrors async behaviorThe
RelatedNodeSyncconstruction in_init_relationshipsand__setattr__matches the async variants (name, branch, client, schema, data normalization), so async/sync behavior stays aligned for cardinality=one relationships. No functional changes beyond formatting.
1326-1345: Sync save path remains aligned with async logicThe reformatted
createcall inInfrahubNodeSync.savekeeps the same branching (_existingvsallow_upsert) and parameters; group-context and store updates are untouched. This maintains parity with the async implementation.
1556-1563: Sync pool allocation result handling is consistentReformatting the
RelatedNodeSyncconstruction in the sync_process_mutation_resultkeeps the same semantics: wrap the allocated resource with the existing relationship schema and client context, then fetch & assign. Behavior matches the async side.
1565-1621: Sync create/update signatures preservedThe multiline signatures for
InfrahubNodeSync.createandInfrahubNodeSync.updatemaintain the same arguments and internals (notably the differentexclude_hfidbehavior for upsert vs create). No observable API change here.
1707-1715: Sync pool allocation query now fetches required node fieldsThe sync
get_pool_allocated_resourcesquery’sedges.nodenow explicitly selectsid,kind,branch, andidentifier, matching how the response is consumed and aligning with the async version and its tests.This ensures the kind/id bookkeeping is always possible from the GraphQL response.
tests/unit/sdk/test_node.py (10)
20-27: UsingBothClientsin tests is consistent with async/sync parity checksImporting
BothClientsfromconftestaligns this module with the shared client fixture pattern and allows parametrized tests to exercise both async and sync clients through a single interface. This keeps parity checks (like method signatures and behavior) straightforward.
124-131: HFID parsing test is precise and future-proofThe parametrized
test_parse_human_friendly_idfor"BuiltinLocation__JFK1"directly checks both the kind and the component list, which matches the expected behavior ofparse_human_friendly_idand will catch regressions if the HFID format ever changes.
147-172: HFID computation tests clearly separate location and rack behaviorThe explicit
location_datadict makestest_node_hfideasier to read, and the subsequent rack HFID checks (["facility_id__value", "location__name__value"]) confirm that cross-node HFIDs are computed correctly viaget_path_value. This is a good sanity test for the human‑friendly ID path logic.
232-279: RelatedNode/RelatedNodeSync construction in tests mirrors production usageThe explicit construction of
RelatedNode/RelatedNodeSyncintest_init_node_data_user_with_relationships_using_related_node(settingname,branch,client,schema,data) matches the production initialization path and confirms that nodes initialized with prebuilt related-node wrappers produce the same attribute/relationship state. Assertions onid,hfid,typename,kind, anddisplay_labelare thorough.All good here.
2097-2139: Flat-value tests correctly validate relationship cardinality constraintsThe enhanced
test_get_flat_valuenow:
- Uses a mocked GraphQL response to resolve the primary tag, ensuring attribute dereferencing for relationships is actually exercised.
- Asserts successful lookups for both attribute paths (
name__value) and relationship paths with custom separators (primary_tag__display_label,primary_tag.display_label).- Verifies that using a MANY relationship (
tags__display_label) raisesValueErrorwith a message matching the implementation ("Can only look up flat value for relationships of cardinality one").This tightly couples the test to the intended constraint that flat lookups only work on cardinality=one relationships.
2142-2163: Node.extract tests clearly exercise flat-value extraction for both async and sync
test_node_extract’sparamsdict and assertions verify thatextractcorrectly delegates toget_flat_valueand returns a simple mapping for both client types (id,name__value,description__value). This gives good coverage of the convenience API on top of flat-value resolution.
2178-2198: Read-only attribute test correctly ensures computed fields are excluded from input data
test_read_only_attrverifies that_generate_input_dataincludes only mutable attributes (street_number,street_name,postal_code) plusid, and excludes the read-onlycomputed_addresswhile still confirming its value is correctly computed in the in-memory node.That behavior matches expectations and guards against accidentally sending read-only fields back to the API.
2219-2412: Pool allocation tests now require enough data for proper node conversionThe changes in
test_create_input_data_with_resource_pool_relationship,test_create_mutation_query_with_resource_pool_relationship, andtest_get_pool_allocated_resources:
- Ensure the device payload includes both
primary_addressandip_address_pooltied to the same pool, matching production usage.- Adjust the mocked GraphQL response for
IpamIPAddressto include__typename, aligning it with the updated query skeleton inget_pool_allocated_resourcesand allowingfilters/from_graphqlto work as expected.- Keep assertions on input data and mutation query outputs intact, so resource pool semantics (allocating from pool vs referencing pool) remain covered.
These updates are consistent with the core changes in
InfrahubNode.get_pool_allocated_resourcesand maintain strong regression coverage.
2460-2530: Pool utilization test remains aligned with new query behavior
test_get_pool_resources_utilizationstill validates that exactly one utilization entry is returned with the expectedutilizationvalue for both async and sync clients. The mock response and tracker name match the updated query inget_pool_resources_utilization, so this continues to guard that code path effectively.
2533-2545: from_graphql test confirms async/sync parity after alias-stripping
test_from_graphqlstill ensures both async and syncfrom_graphqlpaths return a node with the expectedidwhen provided withlocation_data01and a schema fetched from cache. This remains an important sanity check for alias stripping and schema resolution.
ogenstad
force-pushed
the
pog-recursive-relationships-IHS-129
branch
from
a153e47 to
92d2487
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/sdk/test_node.py (1)
2547-2697: Comprehensive test for recursive relationship processing.The test thoroughly validates the new
recursiveparameter functionality across both async and sync clients, with clear assertions differentiating shallow (2 nodes) vs deep (5 nodes) processing.A few optional improvements to consider:
Add a clarifying comment at line 2565 explaining why
schema_datais manually constructed instead of using the fixtures directly (i.e., to populate a complete schema cache for peer lookups during recursive traversal).Consider verifying end-to-end accessibility: The test validates that
related_nodesis populated correctly, but doesn't verify that the relationships are actually accessible via node attributes (e.g.,device_node.interfaces[0].ip_addresses) or stored inclient.store. This would provide stronger confidence in the complete integration.Test length: At ~150 lines, the test is quite long. If you anticipate adding more recursive test scenarios, consider extracting a helper function for the common setup/assertion logic to reduce duplication between sync and async paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/sdk/test_node.py(29 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/sdk/test_node.py (3)
tests/unit/sdk/conftest.py (7)
BothClients(25-28)client(32-33)clients(42-47)nested_device_with_interfaces_schema(2739-2763)interface_l3_schema(2704-2735)ipaddress_schema(900-920)nested_device_data(2767-2835)infrahub_sdk/node/node.py (6)
InfrahubNodeSync(1169-1845)InfrahubNode(485-1166)from_graphql(513-527)from_graphql(1197-1211)_process_relationships(944-996)_process_relationships(1622-1675)infrahub_sdk/schema/__init__.py (1)
set_cache(133-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.12)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.9)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
tests/unit/sdk/test_node.py (2)
20-20: LGTM! Import addition is appropriate.The
BothClientsimport is correctly used by the new test function for type hints and accessing both sync and standard clients.
124-127: Formatting improvements throughout the file.The formatting changes (dict/list line breaks, function signature alignment, etc.) improve readability and consistency without altering functionality.
Also applies to: 149-153, 237-251, 289-296, 1380-1384, 1492-1497, 1538-1543, 1564-1568, 1607-1611, 1663-1667, 1694-1697, 1751-1754, 1763-1766, 1779-1786, 1855-1862, 2096-2103, 2106-2113, 2124-2128, 2135-2139, 2144-2148, 2181-2195, 2219-2226, 2244-2248, 2267-2271, 2284-2291, 2309-2313, 2332-2336, 2395-2407, 2461-2467, 2492-2496
ogenstad
force-pushed
the
pog-recursive-relationships-IHS-129
branch
from
92d2487 to
082b757
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/sdk/test_node.py (1)
2547-2697: Recursive relationships test thoroughly exercises deep conversionThe new
test_process_relationships_recursive_deep_nestinggives good coverage for both async and sync clients, clearly separating the non-recursive (interfaces only) and recursive (interfaces + IPs) behaviours and validating IDs rather than relying on object identity. The manually seededschema.set_cachemirrors the minimal schema needed and avoids external dependencies. One minor nit:interface_l3_schema/ipaddress_schemaparameters are unused in this test and could be dropped to avoid fixture churn, but that’s purely cosmetic.infrahub_sdk/node/node.py (2)
944-997: Recursive relationship traversal is correct but should propagate timeoutThe new
recursive: bool = Falseparameter and recursive calls in_process_relationshipscorrectly walk both ONE and MANY relationships and match the deep-nesting tests’ expectations. However, the recursive calls don’t passtimeoutthrough, so deeper levels always use the defaulttimeout=Nonewhen resolving schemas viafrom_graphql, even if the caller supplied a timeout for the top-level call.Propagating
timeoutpreserves the existing contract (“Overrides default timeout used when querying the graphql API”) across all nested levels.- if recursive: - await related_node._process_relationships( - node_data=relation, - branch=branch, - related_nodes=related_nodes, - recursive=recursive, - ) + if recursive: + await related_node._process_relationships( + node_data=relation, + branch=branch, + related_nodes=related_nodes, + timeout=timeout, + recursive=recursive, + ) @@ - if recursive: - await related_node._process_relationships( - node_data=peer, - branch=branch, - related_nodes=related_nodes, - recursive=recursive, - ) + if recursive: + await related_node._process_relationships( + node_data=peer, + branch=branch, + related_nodes=related_nodes, + timeout=timeout, + recursive=recursive, + )- if recursive: - related_node._process_relationships( - node_data=relation, - branch=branch, - related_nodes=related_nodes, - recursive=recursive, - ) + if recursive: + related_node._process_relationships( + node_data=relation, + branch=branch, + related_nodes=related_nodes, + timeout=timeout, + recursive=recursive, + ) @@ - if recursive: - related_node._process_relationships( - node_data=peer, - branch=branch, - related_nodes=related_nodes, - recursive=recursive, - ) + if recursive: + related_node._process_relationships( + node_data=peer, + branch=branch, + related_nodes=related_nodes, + timeout=timeout, + recursive=recursive, + )Also applies to: 1622-1676
1686-1687: Align sync error message with async variant for consistencyThe async version raises
ValueError("Allocated resources can only be fetched from resource pool nodes."), while the sync version still says"Allocate resources can only be fetched...". Behaviour is identical, but the wording mismatch is slightly confusing.Consider updating the sync message to match the async one for consistency.
- if not self.is_resource_pool(): - raise ValueError("Allocate resources can only be fetched from resource pool nodes.") + if not self.is_resource_pool(): + raise ValueError("Allocated resources can only be fetched from resource pool nodes.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog/389.changed.md(1 hunks)infrahub_sdk/node/node.py(22 hunks)infrahub_sdk/operation.py(1 hunks)tests/unit/sdk/conftest.py(1 hunks)tests/unit/sdk/test_node.py(29 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- infrahub_sdk/operation.py
- changelog/389.changed.md
- tests/unit/sdk/conftest.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/sdk/test_node.py (2)
tests/unit/sdk/conftest.py (7)
BothClients(25-28)client(32-33)clients(42-47)nested_device_with_interfaces_schema(2739-2763)interface_l3_schema(2704-2735)ipaddress_schema(900-920)nested_device_data(2767-2835)infrahub_sdk/schema/__init__.py (1)
set_cache(133-148)
infrahub_sdk/node/node.py (3)
infrahub_sdk/node/relationship.py (5)
RelationshipManager(99-219)RelationshipManagerBase(21-96)RelationshipManagerSync(222-341)fetch(147-181)fetch(270-304)infrahub_sdk/node/related_node.py (7)
get(216-226)get(263-273)kind(123-126)fetch(204-210)fetch(251-257)peer(213-214)peer(260-261)infrahub_sdk/client.py (22)
get(389-405)get(408-424)get(427-443)get(446-462)get(465-481)get(484-500)get(502-560)get(2198-2214)get(2217-2233)get(2236-2252)get(2255-2271)get(2274-2290)get(2293-2309)get(2311-2369)create(347-353)create(356-362)create(364-379)create(1723-1729)create(1732-1738)create(1740-1754)execute_graphql(907-991)execute_graphql(1767-1852)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests (3.13)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
tests/unit/sdk/test_node.py (2)
1380-1698: Expanded input-data tests for relationships and IPHost look consistentThe updated fixtures/expectations for
_generate_input_data(including richername/queryrelation metadata,repositoryrelation flags, and the explicit"interface": NoneforIPAddress) all align with the current_generate_input_datasemantics: optional ONE relationships defaulting toNone, and relation metadata being pushed into_relation__*fields. The assertions look coherent and should guard regressions around relationship serialization.
2095-2139: get_flat_value error-path test matches new cardinality checkThe test that asserts
ValueErrorfortags__display_labelnow matches the implementation’s message (RelationshipCardinality.ONE.value→"one"). This keeps behaviour well-specified for MANY relationships and ensures the error contract is covered for both async and sync variants.infrahub_sdk/node/node.py (1)
1018-1035: Pool allocation GraphQL shape matches usage and testsThe updated
Querydefinitions forInfrahubResourcePoolAllocatednow explicitly requestid,kind,branch, andidentifierunderedges.node, which aligns with howget_pool_allocated_resourcesbuildsnode_ids_per_kindand with the test responses intest_get_pool_allocated_resources. This is a clear improvement in correctness and makes the query self-documenting.Also applies to: 1697-1714
ogenstad
force-pushed
the
pog-recursive-relationships-IHS-129
branch
from
082b757 to
631f9c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/unit/sdk/test_node.py (4)
1492-1497: Relationship property expectations for RFile look correctThe updated expectations in the
rfile_*tests (02, 03, and update variants) now assert that:
- Attribute
namepreserves its own protection/owner/source flags.- Relationship fields (
query,repository) map attribute-levelowner/source/is_protectedinto_relation__*keys in the mutation payload.
This aligns with how relationship properties are meant to be encoded separately from node attributes.Also applies to: 1538-1543, 1607-1611, 1656-1667
1780-1785: Update-input tests for location relationships cover both full and trimmed payloadsThe
test_update_input_data_with_relationships_02and_exclude_unmodifiedvariants correctly distinguish:
- Full update payloads containing
name,primary_tag,tags, andtypewith and without properties.- Minimal payloads when
exclude_unmodified=True, where only changed relationship metadata (e.g.,primary_tagrelation properties) is retained.
This matches_strip_unmodifiedsemantics for attributes vs relationship data.Also applies to: 1856-1861
2220-2225: Resource pool relationship tests exercise both input and mutation queriesThe pool-related tests now assert that:
- Device
_generate_input_dataencodesprimary_addressasfrom_poolwith the pool ID and setsip_address_poolby ID._generate_mutation_queryincludes minimalobjectstructure with relationship nodes havingidslots.- Async and sync paths share the same expectations.
These changes align test fixtures with the intended resource-pool semantics and ensure both creation and mutation paths are covered.
Also applies to: 2245-2248, 2268-2271, 2285-2290, 2310-2313, 2333-2336
2547-2697: Deep-nesting recursive relationship test is well structuredThe new
test_process_relationships_recursive_deep_nesting:
- Builds a minimal schema cache for
Device,InterfaceL3, andIPAddressthat matches the shapes innested_device_data.- For both client types:
- With
recursive=False, confirms only the two interfaces are returned.- With
recursive=True, confirms all five nodes (2 interfaces + 3 IPs) are traversed and collected.This is a solid, focused validation of the new
recursiveparameter on_process_relationshipsfor both async and sync variants.
interface_l3_schemaandipaddress_schemafixtures are injected into the test but not used; you could drop them from the signature unless you intend to extend the assertions with schema-based checks.infrahub_sdk/node/node.py (2)
892-908: Recursive async_process_relationshipsis correct but lacks cycle protectionThe async
_process_relationshipsnow:
- Accepts a
recursive: bool = Falseflag with default preserving existing behavior.- For ONE and MANY relationships, constructs child nodes via
InfrahubNode.from_graphqland whenrecursive=Truecalls_process_relationshipson each child with the relevantnode_datafragment.This correctly enables deep traversal for response shapes like Device → interfaces → ip_addresses as exercised in the new tests. However, there is no visited set / depth guard; if a GraphQL query ever includes relationships that form a cycle in both directions, this could recurse indefinitely or accumulate many duplicates.
- Consider adding an optional
visitedset (e.g., of(kind, id)tuples) or a max-depth parameter to guard against cycles, especially for schemas where parent and child relationships can both be present in the same query.- Verify that current production queries do not include cyclic chains (e.g., Device → interfaces → device → …) before relying on
recursive=Truein general-purpose helpers.Also applies to: 910-927, 928-945
1545-1555: Sync_process_relationshipsmirrors async behavior consistentlyThe sync implementation of
_process_relationshipsmirrors the async one:
- Adds the same
recursive: bool = Falseparameter.- Recursively descends into ONE and MANY relationships using
InfrahubNodeSync.from_graphqland_process_relationshipswithout changing existing semantics whenrecursiveis left as default.Consistency between async and sync variants is good and matches the expectations in
test_process_relationships_recursive_deep_nesting. The same caveat about potential cycles applies here as well.
- If you introduce a visited/depth guard in the async variant, mirror it here to keep the two code paths aligned.
- Minor nit: the docstring line
recursive:(bool)is missing a space; you may want to fix this for readability.Also applies to: 1558-1575, 1576-1592
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog/389.changed.md(1 hunks)infrahub_sdk/node/node.py(4 hunks)infrahub_sdk/operation.py(1 hunks)tests/unit/sdk/conftest.py(1 hunks)tests/unit/sdk/test_node.py(29 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- infrahub_sdk/operation.py
- changelog/389.changed.md
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/sdk/conftest.py (1)
infrahub_sdk/schema/main.py (4)
NodeSchemaAPI(312-314)NodeSchema(307-309)convert_api(284-285)convert_api(308-309)
infrahub_sdk/node/node.py (2)
infrahub_sdk/node/related_node.py (6)
RelatedNode(182-226)get(216-226)get(263-273)peer(213-214)peer(260-261)RelatedNodeSync(229-273)infrahub_sdk/node/relationship.py (2)
RelationshipManager(99-219)RelationshipManagerSync(222-341)
tests/unit/sdk/test_node.py (2)
tests/unit/sdk/conftest.py (7)
BothClients(25-28)client(32-33)clients(42-47)nested_device_with_interfaces_schema(2739-2763)interface_l3_schema(2704-2735)ipaddress_schema(900-920)nested_device_data(2767-2835)infrahub_sdk/schema/__init__.py (1)
set_cache(133-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
tests/unit/sdk/conftest.py (1)
2703-2835: Deep-nesting fixtures look consistent and well-scopedThe new
interface_l3_schema,nested_device_with_interfaces_schema, andnested_device_datafixtures form a coherent 3-level Device → InterfaceL3 → IPAddress graph with consistent__typename/schema naming and shapes matching the rest of the test suite. They’re suitable for exercising recursive relationship traversal without impacting other tests.tests/unit/sdk/test_node.py (11)
20-20: ImportingBothClientsis appropriate for multi-client testsThe new
BothClientsimport is correctly wired for tests that need to exercise both async and sync clients (e.g., deep-nesting and pool-related tests). No issues.
124-127: HFID tests remain clear and targetedThe parametrized
test_parse_human_friendly_idand the explicitlocation_datasetup intest_node_hfidkeep the HFID behavior well specified without changing semantics. This still validates kind parsing and HFID composition properly.Also applies to: 149-153
290-296: Refactoringtest_init_node_data_graphqlsignature is neutralThe multi-line parameter formatting in
test_init_node_data_graphqlis purely cosmetic and does not affect test behavior. Safe change.
378-384: Explicitdatablocks in create-input tests aid readabilityThe extracted
datadictionaries intest_create_input_dataand nearby tests make the initial node payloads clearer without altering the assertions. This helps when reasoning about_generate_input_databehavior.Also applies to: 1380-1384
1694-1697: Includinginterface=Nonefor optional IPHost parent is consistentIn
test_create_input_data_with_IPHost_attribute, expecting"interface": Nonealongside theaddressdata matches_generate_input_data’s behavior of explicitly nulling optional cardinality-one relationships that were not initialized. This is consistent with how other optional ONE relationships are handled.
1752-1754: Tag list expectations now reflect updated tag membershipIn
test_update_input_data__with_relationships_01, the expectedtagslists explicitly contain the IDs for green and red after extend/remove operations. This matches the relationship mutations performed in the test and keeps the expectation aligned with the actual tag set.Also applies to: 1763-1766
2097-2103:get_flat_valuetests now enforce cardinality-one restrictionThe enhanced
test_get_flat_value:
- Sets up an HTTPX mock so relationship fetches are resolvable.
- Verifies the happy path for attribute and ONE-relationship lookups.
- Asserts that requesting
tags__display_label(cardinality MANY) raises aValueErrorwith the expected message, for both async and sync nodes.This tightly validates the intended constraint in
get_flat_value.Also applies to: 2106-2113, 2124-2128, 2136-2139
2144-2148:test_node_extractparam map is explicit and saneThe
paramsdict now clearly maps friendly keys (identifier,name,description) to their flat paths, which better documents whatextractis expected to return without changing behavior.
2181-2195: Read-only attribute test correctly excludescomputed_address
test_read_only_attrnow:
- Expects full property objects (is_protected/is_visible/value) for writable fields.
- Confirms
computed_addressis not present in the generated input data, but is still available on the node.This matches
_generate_input_data’s “skip read_only attributes” semantics and improves coverage.
2395-2405: Allocated resource IDs in IPAM tests are now explicitIn
test_get_pool_allocated_resources, the mockedIpamIPAddressedges now include explicitidand__typenamefields for each allocated resource. This makes the expectation on returned nodes clearer and matches howfilterswould populate the store.
2462-2467: Pool utilization test properly reuses existing prefixes and pools
test_get_pool_resources_utilizationnow:
- Constructs
ip_prefixandip_poolwith the shared fixtures and then callsget_pool_resources_utilizationon the pool.- Asserts the single utilization entry equals the mocked value
93.75for both async and sync clients.The setup matches the production usage pattern of this helper.
Also applies to: 2492-2496
3 participants
Adds a recursive parameter for the process relationship methods within the Node class. This is to enable nested conversions for the
convert_query_responsefeature that previously only went one level deep.Fixes #389
Summary by CodeRabbit
Changed
Tests
Documentation