Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughThis pull request adds support for referencing resource pools by name in addition to UUID-based lookup. Core logic changes in node and relationship model files implement a fallback mechanism: when a pool identifier is not a valid UUID, the system queries the corresponding pool type by name instead. Two new comprehensive test suites validate pool lookup by name for both IP and number pool resources, covering creation, updates, and error scenarios. Additionally, test refactoring consolidates GraphQL mutation definitions into named constants for improved maintainability. 🚥 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 |
github-actions
bot
added
the
group/backend
| default_branch_scope_class: Branch, | ||
| register_core_models_schema_scope_class: SchemaBranch, | ||
| ) -> CoreNumberPool: | ||
| await load_schema(db=db, schema=SchemaRoot(nodes=[TICKET])) |
| ) | ||
| else: | ||
| results = await registry.manager.query( | ||
| db=db, schema=InfrahubKind.NUMBERPOOL, filters={"name__value": number_pool_id} |
There was a problem hiding this comment.
Not a blocker to make progress but number_pool would get different types compared to above where we are querying for CoreNumberPool instead of InfrahubKind.NUMBERPOOL. But the type checkers probably don't pick this up due to all of the ignore lines in pyproject.toml :(
| results = await registry.manager.query( | ||
| db=db, schema=InfrahubKind.RESOURCEPOOL, filters={"name__value": pool_id}, branch=self.branch | ||
| ) | ||
| results = cast("list[Node]", results) |
There was a problem hiding this comment.
I'm guessing that the problem here is that the type of the overload for NodeManager.query is incorrect and returns an Any instead of a Node. Once we change that (if it happens before the refactoring of those classes) I think this should resurface as a redundant cast, so we probably don't need to track it.
ajtmccarty
force-pushed
the
ajtm-02262026-pool-by-name
branch
from
1e1ac1c to
14e25c7
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: 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 `@backend/infrahub/core/node/__init__.py`:
- Around line 407-414: The UUID and name lookups for number pools (the
is_valid_uuid branch using registry.manager.get_one with CoreNumberPool and the
else branch using registry.manager.query with InfrahubKind.NUMBERPOOL) are
missing the branch filter; update both calls to include the current branch (same
pattern used by the relationship resolver around lines 564-574) so that get_one
and query are scoped by branch (pass the branch argument or include {"branch":
branch} in filters) to avoid cross-branch resolution.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/infrahub/core/node/__init__.pybackend/infrahub/core/relationship/model.pybackend/tests/component/graphql/resource_manager/__init__.pybackend/tests/component/graphql/resource_manager/test_ip_pool_lookup_by_name.pybackend/tests/component/graphql/resource_manager/test_number_pool_lookup_by_name.pybackend/tests/component/graphql/resource_manager/test_resource_manager.py
| if is_valid_uuid(number_pool_id): | ||
| number_pool = await registry.manager.get_one( | ||
| db=db, id=number_pool_id, kind=CoreNumberPool, raise_on_error=True | ||
| ) | ||
| else: | ||
| results = await registry.manager.query( | ||
| db=db, schema=InfrahubKind.NUMBERPOOL, filters={"name__value": number_pool_id} | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect handle_pool lookup path
echo "=== Lines 404-418 of backend/infrahub/core/node/__init__.py ==="
sed -n '404,418p' backend/infrahub/core/node/__init__.py
echo ""
echo "=== Relationship resolver pool lookup (lines 564-574) ==="
sed -n '564,574p' backend/infrahub/core/relationship/model.py 2>/dev/null || echo "(File or lines not found)"
echo ""
echo "=== NodeManager.query signature ==="
rg -nP 'async def query\(' backend/infrahub/core/manager.py -A10 -B2 | head -30
echo ""
echo "=== Check for get_branch_based_on_support_type method ==="
rg -n 'def get_branch_based_on_support_type' backend/infrahub/core/node/__init__.py -B2 -A8Repository: opsmill/infrahub
Length of output: 3036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are other calls to registry.manager.get_one for pools/numbers in the codebase
# to establish a pattern
echo "=== Other registry.manager.get_one calls for Node lookups in same file ==="
rg 'registry\.manager\.get_one' backend/infrahub/core/node/__init__.py -B1 -A3 | head -40
echo ""
echo "=== Check context around handle_pool method ==="
sed -n '390,425p' backend/infrahub/core/node/__init__.pyRepository: opsmill/infrahub
Length of output: 2350
Scope number-pool lookups to the current branch for both UUID and name paths.
Both the UUID lookup (line 409) and the name-based lookup (line 413) omit the branch parameter, causing potential cross-branch pool misresolution. The relationship resolver at lines 564-574 demonstrates the correct pattern with branch-scoped queries for both paths.
🔧 Proposed fix
try:
+ lookup_branch = self.get_branch_based_on_support_type()
if is_valid_uuid(number_pool_id):
number_pool = await registry.manager.get_one(
- db=db, id=number_pool_id, kind=CoreNumberPool, raise_on_error=True
+ db=db, id=number_pool_id, kind=CoreNumberPool, branch=lookup_branch, raise_on_error=True
)
else:
results = await registry.manager.query(
- db=db, schema=InfrahubKind.NUMBERPOOL, filters={"name__value": number_pool_id}
+ db=db,
+ schema=InfrahubKind.NUMBERPOOL,
+ filters={"name__value": number_pool_id},
+ branch=lookup_branch,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/infrahub/core/node/__init__.py` around lines 407 - 414, The UUID and
name lookups for number pools (the is_valid_uuid branch using
registry.manager.get_one with CoreNumberPool and the else branch using
registry.manager.query with InfrahubKind.NUMBERPOOL) are missing the branch
filter; update both calls to include the current branch (same pattern used by
the relationship resolver around lines 564-574) so that get_one and query are
scoped by branch (pass the branch argument or include {"branch": branch} in
filters) to avoid cross-branch resolution.
3 participants
IFC-2300
should follow #8482
allow specifying resource pools (IP Prefix/Address and NumberPool) by name in the
from_poolinput. thenameattribute is unique on theCoreResourcePoolgenericfor example
purposefully chose not to use
NodeManager.get_by_id_or_default_filterb/c we are trying to not usedefault_filterany longer. instead this usesis_valid_uuidand tries to get by name usingNodeManager.queryif the input is not an ID.added graphql component tests to cover the changes
also moved some of the test files around and removed string interpolation from some of the graphql query strings
Summary by CodeRabbit
New Features
Tests