Fix filters include parameter
#513
Conversation
WalkthroughThe PR changes SDK relationship prefetch and include handling. infrahub_sdk/client.py adds an include parameter to _process_nodes_and_relationships (async and sync) and triggers node._process_relationships when prefetch_relationships is true or include is provided; process_page forwards include. infrahub_sdk/node/node.py updates generate_query_data_node to prefetch relationships when prefetch_relationships or include contains the relationship name, with different nested prefetch propagation between async and sync paths. Type hints refined and tests added/updated for include semantics and sync client coverage. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
LucasG0
force-pushed
the
lgu-fix-filters-include
branch
from
14a114f to
575c6a9
Compare
Deploying infrahub-sdk-python with
|
| Latest commit: |
25e8f9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5ac9b069.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://lgu-fix-filters-include.infrahub-sdk-python.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## stable #513 +/- ##
==========================================
+ Coverage 75.74% 75.82% +0.08%
==========================================
Files 100 100
Lines 8846 8861 +15
Branches 1732 1738 +6
==========================================
+ Hits 6700 6719 +19
+ Misses 1670 1667 -3
+ Partials 476 475 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
LucasG0
force-pushed
the
lgu-fix-filters-include
branch
from
0f2eb5a to
2254f5f
Compare
LucasG0
force-pushed
the
lgu-fix-filters-include
branch
2 times, most recently
from
1913614 to
645c7e6
Compare
LucasG0
force-pushed
the
lgu-fix-filters-include
branch
from
645c7e6 to
f344e76
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrahub_sdk/node/node.py (1)
1368-1374: Sync nested prefetch passes include/exclude unfiltered; scope them to the peer’s schema to prevent accidental overfetch.If the peer has a same-named relationship (e.g., "tags"), passing the parent’s raw lists can include unintended fields.
Apply:
- peer_schema = self._client.schema.get(kind=rel_schema.peer, branch=self._branch) - peer_node = InfrahubNodeSync(client=self._client, schema=peer_schema, branch=self._branch) - peer_data = peer_node.generate_query_data_node(include=include, exclude=exclude, property=property) + peer_schema = self._client.schema.get(kind=rel_schema.peer, branch=self._branch) + peer_node = InfrahubNodeSync(client=self._client, schema=peer_schema, branch=self._branch) + peer_rel_names = getattr(peer_schema, "relationship_names", [r.name for r in peer_schema.relationships]) + child_include = [n for n in (include or []) if n in peer_rel_names] or None + child_exclude = [n for n in (exclude or []) if n in peer_rel_names] or None + peer_data = peer_node.generate_query_data_node( + include=child_include, + exclude=child_exclude, + property=property, + )
🧹 Nitpick comments (6)
tests/unit/sdk/conftest.py (1)
2651-2666: Make helper synchronous (no await inside) to simplify tests.The function performs no async I/O. Making it sync avoids unnecessary
awaitin tests.Apply:
-async def set_builtin_tag_schema_cache(client) -> None: +def set_builtin_tag_schema_cache(client) -> None: # Set tag schema in cache to avoid needed to request the server.Follow-up: drop
awaitat call sites.tests/integration/test_node.py (2)
112-114: Relax brittle error-message assertion.Asserting the exact string ties the test to message wording. Prefer checking only exception type or a stable substring.
Apply:
- with pytest.raises(NodeNotFoundError, match=f"Unable to find the node '{person_joe.id}' in the store"): + with pytest.raises(NodeNotFoundError): _ = node_after.owner.peer
108-111: Add a public clear() method to NodeStore and use it instead of mutating_branchesdirectly.NodeStore (infrahub_sdk/store.py) doesn’t expose a clear API, so tests shouldn’t do
client.store._branches = {}– add in NodeStore:
def clear(self): self._branches = {} # reset other internal state if neededand update tests to call
client.store.clear().infrahub_sdk/client.py (3)
548-559: Fix async docstring: missing include param and wrong return type.Document the new
includeparameter and change return type toProcessRelationsNode.Args: response (dict[str, Any]): The response from the GraphQL query. schema_kind (str): The kind of schema being queried. branch (str): The branch name. prefetch_relationships (bool): Flag to indicate whether to prefetch relationship data. + include (list[str] | None): If provided, process relationships present in the GraphQL response for these names. timeout (int, optional): Overrides default timeout used when querying the graphql API. Specified in seconds. Returns: - ProcessRelationsNodeSync: A TypedDict containing two lists: + ProcessRelationsNode: A TypedDict containing two lists: - 'nodes': A list of InfrahubNode objects representing the nodes processed. - 'related_nodes': A list of InfrahubNode objects representing the related nodes
561-563: Convert note into actionable TODO with issue reference.The comment signals tech debt. Make it a TODO referencing a tracking issue to avoid drift.
- # Ideally, include and relationships wouldn't be parameters of this method, they should only - # be used to build the request for the server, and this method would build node according to the response. + # TODO(lgu): include/prefetch should influence only request construction; this method should rely solely + # on the server response to build nodes. Track in <link to issue>.
1847-1853: Sync docstring: add include param.Document the new
includeparameter.Args: response (dict[str, Any]): The response from the GraphQL query. schema_kind (str): The kind of schema being queried. branch (str): The branch name. prefetch_relationships (bool): Flag to indicate whether to prefetch relationship data. + include (list[str] | None): If provided, process relationships present in the GraphQL response for these names. timeout (int, optional): Overrides default timeout used when querying the graphql API. Specified in seconds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
infrahub_sdk/client.py(6 hunks)infrahub_sdk/node/node.py(3 hunks)tests/integration/test_node.py(1 hunks)tests/unit/sdk/conftest.py(2 hunks)tests/unit/sdk/test_node.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
tests/integration/test_node.pytests/unit/sdk/conftest.pyinfrahub_sdk/client.pytests/unit/sdk/test_node.pyinfrahub_sdk/node/node.py
tests/integration/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write integration tests under tests/integration/ (tests against real Infrahub instances)
Files:
tests/integration/test_node.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/integration/test_node.pytests/unit/sdk/conftest.pytests/unit/sdk/test_node.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/conftest.pytests/unit/sdk/test_node.py
infrahub_sdk/client.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh
Files:
infrahub_sdk/client.py
🧠 Learnings (2)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to tests/**/*.py : Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Applied to files:
tests/unit/sdk/conftest.py
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Maintain a dual client pattern: InfrahubClient (async) and InfrahubClientSync (sync) must provide identical interfaces
Applied to files:
tests/unit/sdk/conftest.py
🧬 Code graph analysis (4)
tests/integration/test_node.py (4)
infrahub_sdk/client.py (21)
InfrahubClient(290-1548)create(322-328)create(331-337)create(339-354)create(1585-1591)create(1594-1600)create(1602-1616)get(364-380)get(383-399)get(402-418)get(421-437)get(440-456)get(459-475)get(477-535)get(2050-2066)get(2069-2085)get(2088-2104)get(2107-2123)get(2126-2142)get(2145-2161)get(2163-2221)infrahub_sdk/node/node.py (4)
create(835-863)create(1461-1488)save(597-624)save(1226-1249)infrahub_sdk/node/related_node.py (4)
kind(123-126)id(83-86)peer(213-214)peer(260-261)infrahub_sdk/exceptions.py (1)
NodeNotFoundError(69-88)
tests/unit/sdk/conftest.py (2)
infrahub_sdk/client.py (1)
InfrahubClientSync(1551-2765)infrahub_sdk/schema/__init__.py (1)
set_cache(113-128)
infrahub_sdk/client.py (1)
infrahub_sdk/node/node.py (5)
InfrahubNode(452-1081)from_graphql(480-494)from_graphql(1112-1126)_process_relationships(887-918)_process_relationships(1513-1545)
tests/unit/sdk/test_node.py (1)
tests/unit/sdk/conftest.py (4)
set_builtin_tag_schema_cache(2651-2665)client(32-33)client_sync(37-38)location_schema(145-177)
⏰ 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: integration-tests-latest-infrahub
🔇 Additional comments (8)
infrahub_sdk/node/node.py (1)
685-706: Behavior check: include=[] triggers relationship processing; cardinality=one peers may still be prefetched.With client-side processing gated by “include is not None,” an explicitly empty include can still populate store for ONE relationships (since the query always includes them). Confirm this is desired; otherwise we should either skip processing when include==[] or restrict ONE relationships similarly.
Also applies to: 728-743, 753-771
tests/unit/sdk/conftest.py (1)
36-39: LGTM: Added sync client fixture mirrors async client config.tests/unit/sdk/test_node.py (3)
20-20: LGTM: Import of schema cache helper.
1060-1072: LGTM: Include+property tests cover async and sync paths with schema cache primed.
1190-1202: LGTM: Include tests (no property) for both clients.tests/integration/test_node.py (1)
88-121: LGTM: Integration test validates include-driven relationship loading for one and many.infrahub_sdk/client.py (2)
820-827: Propagating include into processing path looks good.Passing
includedown ensures related nodes are collected when explicitly requested.
1986-1993: Sync callsite propagation is correct.Named arg usage avoids ordering pitfalls after the signature change.
| peer_data: dict[str, Any] = {} | ||
| if rel_schema and prefetch_relationships: | ||
| should_fetch_relationship = prefetch_relationships or (include is not None and rel_name in include) | ||
| if rel_schema and should_fetch_relationship: | ||
| peer_schema = await self._client.schema.get(kind=rel_schema.peer, branch=self._branch) | ||
| peer_node = InfrahubNode(client=self._client, schema=peer_schema, branch=self._branch) | ||
| peer_data = await peer_node.generate_query_data_node( | ||
| include=include, | ||
| exclude=exclude, | ||
| property=property, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Async nested prefetch should propagate include/exclude (filtered) to match sync semantics and avoid over/under-fetch.
Currently only property is passed; this diverges from sync and can omit requested nested fields.
Apply:
- peer_data = await peer_node.generate_query_data_node(
- property=property,
- )
+ # Propagate only relationship names relevant to the peer schema
+ peer_rel_names = getattr(peer_schema, "relationship_names", [r.name for r in peer_schema.relationships])
+ child_include = [n for n in (include or []) if n in peer_rel_names] or None
+ child_exclude = [n for n in (exclude or []) if n in peer_rel_names] or None
+ peer_data = await peer_node.generate_query_data_node(
+ include=child_include,
+ exclude=child_exclude,
+ property=property,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| peer_data: dict[str, Any] = {} | |
| if rel_schema and prefetch_relationships: | |
| should_fetch_relationship = prefetch_relationships or (include is not None and rel_name in include) | |
| if rel_schema and should_fetch_relationship: | |
| peer_schema = await self._client.schema.get(kind=rel_schema.peer, branch=self._branch) | |
| peer_node = InfrahubNode(client=self._client, schema=peer_schema, branch=self._branch) | |
| peer_data = await peer_node.generate_query_data_node( | |
| include=include, | |
| exclude=exclude, | |
| property=property, | |
| ) | |
| peer_data: dict[str, Any] = {} | |
| should_fetch_relationship = prefetch_relationships or (include is not None and rel_name in include) | |
| if rel_schema and should_fetch_relationship: | |
| peer_schema = await self._client.schema.get(kind=rel_schema.peer, branch=self._branch) | |
| peer_node = InfrahubNode(client=self._client, schema=peer_schema, branch=self._branch) | |
| # Propagate only relationship names relevant to the peer schema | |
| peer_rel_names = getattr(peer_schema, "relationship_names", [r.name for r in peer_schema.relationships]) | |
| child_include = [n for n in (include or []) if n in peer_rel_names] or None | |
| child_exclude = [n for n in (exclude or []) if n in peer_rel_names] or None | |
| peer_data = await peer_node.generate_query_data_node( | |
| include=child_include, | |
| exclude=child_exclude, | |
| property=property, | |
| ) |
infrahub_sdk/client.py
Outdated
| """ | ||
|
|
||
| # Ideally, include and relationships wouldn't be parameters of this method, they should only | ||
| # be used to build the request for the server, and this method would build node according to the response. |
There was a problem hiding this comment.
I agree with this comment and I think you can do this differently and then remove the comment
what about instead of passing include in here, you update the prefetch_relationships parameter to be a do_process_relationships Boolean flag and then update the two places where _process_nodes_and_relationships is called to set do_process_relationships based on prefetch_relationships and if include is not empty (or maybe if include actually includes the name of a relationship)
There was a problem hiding this comment.
This is about the fact that we should rely on these parameters only to fetch the data from the server and not while building the nodes client side.
The current logic is:
if prefetch_relationships / include has a rel_name:
fetch_rel_from_server
if prefetch_relationships / include has a rel_name:
process_relationships() # build peers nodes
The comment means that we should get rid of the second if condition, ie, while building the nodes we should just build it ouf of the payload that we previously fetched. Does this make sense? I tried to do so but it led to some complicated bugs that should be treated out of this PR if we want to clean that.
I still made a minor change following up your comment so that include checks on whether it contains a relationship name.
Using a do_prefetch_relationships as you suggested might not fit well here because we have access to node relationships once it is built inside the _process_nodes_and_relationships method, and we need relationships names to check whether include contains some.
There was a problem hiding this comment.
I was just suggesting making the below small changes. I didn't realize that this comment was related to existing functionality. This sounds like something that deserves its own issue in GH/Jira instead of a comment that will be ignored / forgotten
diff --git a/infrahub_sdk/client.py b/infrahub_sdk/client.py
index f9a380b..cae970a 100644
--- a/infrahub_sdk/client.py
+++ b/infrahub_sdk/client.py
@@ -539,8 +539,7 @@ class InfrahubClient(BaseClient):
response: dict[str, Any],
schema_kind: str,
branch: str,
- prefetch_relationships: bool,
- include: list[str] | None,
+ do_process_relationships: bool,
timeout: int | None = None,
) -> ProcessRelationsNode:
"""Processes InfrahubNode and their Relationships from the GraphQL query response.
@@ -549,7 +548,7 @@ class InfrahubClient(BaseClient):
response (dict[str, Any]): The response from the GraphQL query.
schema_kind (str): The kind of schema being queried.
branch (str): The branch name.
- prefetch_relationships (bool): Flag to indicate whether to prefetch relationship data.
+ do_process_relationships (bool): Flag to indicate whether to fetch relationship data.
timeout (int, optional): Overrides default timeout used when querying the graphql API. Specified in seconds.
Returns:
@@ -568,7 +567,7 @@ class InfrahubClient(BaseClient):
node = await InfrahubNode.from_graphql(client=self, branch=branch, data=item, timeout=timeout)
nodes.append(node)
- if prefetch_relationships or include is not None:
+ if do_process_relationships:
await node._process_relationships(
node_data=item,
branch=branch,
@@ -821,9 +820,8 @@ class InfrahubClient(BaseClient):
response=response,
schema_kind=schema.kind,
branch=branch,
- prefetch_relationships=prefetch_relationships,
+ do_process_relationships=prefetch_relationships or include is not None,
timeout=timeout,
- include=include,
)
return response, process_result
There was a problem hiding this comment.
I understood that this was the change you suggested, and I think we cannot do that because
do_process_relationships=prefetch_relationships or include is not None is missing checking relationships names as you nicely pointed out, and we need the node being built to do that, cf my previous comment
Using a do_prefetch_relationships as you suggested might not fit well here because we have access to node relationships once it is built inside the _process_nodes_and_relationships method, and we need relationships names to check whether include contains some.
infrahub_sdk/client.py
Outdated
| nodes.append(node) | ||
|
|
||
| if prefetch_relationships: | ||
| if prefetch_relationships or include is not None: |
There was a problem hiding this comment.
I wonder if we should have an extra check here. I.e. if "include" just consists of a list of attribute names we don't really need to run the code for process_relationships?
infrahub_sdk/client.py
Outdated
| nodes.append(node) | ||
|
|
||
| if prefetch_relationships: | ||
| if prefetch_relationships or include is not None: |
|
|
||
| peer_data: dict[str, Any] = {} | ||
| if rel_schema and prefetch_relationships: | ||
| should_fetch_relationship = prefetch_relationships or (include is not None and rel_name in include) |
There was a problem hiding this comment.
It was something like this I was thinking about in the comments above.
There was a problem hiding this comment.
I had actually addressed these changes following Aaron's comment but didn't push them
LucasG0
force-pushed
the
lgu-fix-filters-include
branch
from
31aeaa0 to
25e8f9f
Compare
3 participants
Help to fix opsmill/infrahub#6976
Fix
client.filters(include=[...])that was not fetching relationshipsSummary by CodeRabbit
New Features
Bug Fixes
Tests