simpler create/update for Template relationship pools#8535
simpler create/update for Template relationship pools#8535ajtmccarty wants to merge 10 commits intorelease-1.8from
Conversation
github-actions
bot
added
the
group/backend
WalkthroughIntroduces TemplatePoolHandler to route pool-backed relationships for template create/update and invokes it in GraphQL mutation logic when the active schema is a TemplateSchema. Marks the resource-pool RelationshipSchema created in schema_branch as 🚥 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 |
Merging this PR will degrade performance by 27.83%
Performance Changes
Comparing 
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/infrahub/graphql/mutations/template_pool_router.py (1)
83-86: This review comment addresses a speculative future scenario rather than a current issue.
GenericPoolInput.idis currently marked asrequired=Trueand the input type does not supporthfidlookups. Accessingfrom_pool["id"]is safe given the current schema constraints—aKeyErrorcannot occur unless the schema is extended to makeidoptional and addhfidsupport (as exists inRelatedNodeInput).If defensive programming for future schema extensibility is desired, the proposed fix is reasonable; however, it is not addressing a bug in the current codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/graphql/mutations/template_pool_router.py` around lines 83 - 86, The current access of from_pool["id"] in template_pool_router.py is safe given GenericPoolInput.id is required and hfid lookups are not supported; no code change is necessary—keep the existing assignment to data[pool_rel_name] = {"id": from_pool["id"]} and data[rel_name] = None; if you prefer future-proofing when extending the schema, replace the direct index with a defensive lookup (e.g., use from_pool.get("id") and fall back to handling an hfid path similar to RelatedNodeInput) but this is optional and not required to fix a bug now.
🤖 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/graphql/mutations/main.py`:
- Around line 250-252: The payload routing for template pool relationships is
mutating the public input (TemplatePoolHandler.route_relationships on
create_data) before user-input validation, which writes keys like
"<rel>_from_resource_pool" that are defined read_only=True in schema_branch.py
and then rejected by create_node()/obj.from_graphql(); change the flow so
routing doesn't mutate the validated input: either call
TemplatePoolHandler.route_relationships only after validation (move the call to
run after create_node()/obj.from_graphql input validation) or add an internal
handoff flag to route_relationships/create_node()/from_graphql (e.g.,
internal=True or bypass_readonly=True) so routed "<rel>_from_resource_pool"
entries are injected into the node state after validation and skip read-only
validation; update usages in TemplateSchema handling to use the chosen approach
and ensure schema_branch read_only semantics are preserved.
In
`@backend/tests/component/graphql/templates/test_template_pool_relationships.py`:
- Around line 422-428: The test is asserting against a stale `template` object
after reloading; replace the direct use of `template` with the refreshed
`reloaded` instance so the post-update state is validated correctly: change the
call `template.get_relationship("primary_ip").get_peer(db=db)` to use
`reloaded.get_relationship("primary_ip").get_peer(db=db)` (keeping the existing
awaits and assertions) so both relationship checks use the
NodeManager.get_one-refreshed object.
---
Nitpick comments:
In `@backend/infrahub/graphql/mutations/template_pool_router.py`:
- Around line 83-86: The current access of from_pool["id"] in
template_pool_router.py is safe given GenericPoolInput.id is required and hfid
lookups are not supported; no code change is necessary—keep the existing
assignment to data[pool_rel_name] = {"id": from_pool["id"]} and data[rel_name] =
None; if you prefer future-proofing when extending the schema, replace the
direct index with a defensive lookup (e.g., use from_pool.get("id") and fall
back to handling an hfid path similar to RelatedNodeInput) but this is optional
and not required to fix a bug now.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04282a37-1531-43a8-bcdb-cf639030f328
📒 Files selected for processing (11)
backend/infrahub/core/schema/schema_branch.pybackend/infrahub/graphql/mutations/main.pybackend/infrahub/graphql/mutations/template_pool_router.pybackend/infrahub/graphql/types/attribute.pybackend/tests/component/graphql/templates/__init__.pybackend/tests/component/graphql/templates/test_template_pool_relationships.pybackend/tests/functional/templates/test_template_resource_pool.pybackend/tests/integration/templates/test_template_resource_pool_lifecycle.pyfrontend/app/src/shared/api/graphql/graphql-env.d.tspython_sdkschema/schema.graphql
backend/tests/component/graphql/templates/test_template_pool_relationships.py
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/component/graphql/templates/test_template_pool_relationships.py (2)
85-92: Add a GraphQL negative case for the internal relationship.This suite covers the happy path through
primary_ip, but it never proves thatprimary_ip_from_resource_poolis actually read-only at the GraphQL layer. A small create/update mutation that tries to set the internal field directly and asserts validation failure would lock down the other half of this PR's contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/graphql/templates/test_template_pool_relationships.py` around lines 85 - 92, Add a negative GraphQL test method inside TestTemplatePoolRelationships that attempts to set the internal relationship field primary_ip_from_resource_pool directly via a create (and optionally update) mutation and asserts the operation is rejected; locate the GraphQL mutation helpers used elsewhere in this test file (e.g., the createTemplate/updateTemplate mutation calls) and reuse them to submit a payload containing primary_ip_from_resource_pool, then assert the response contains a validation/error entry (HTTP error or GraphQL errors) and that no object is created/modified. Ensure the test name clearly indicates it verifies primary_ip_from_resource_pool is read-only at the GraphQL layer.
368-382: The “by name” cases still exercisefrom_pool.id.Both tests pass
"tpl-test-address-pool"viaid, so they do not isolate the new explicitfrom_pool.hfid/name lookup branch. Ifidcontinues to accept friendly identifiers, these cases can stay green while the dedicated HFID path regresses. I'd add separate create/update mutations that populatefrom_pool.hfidinstead of reusing the...WITH_POOLdocuments.Also applies to: 396-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/graphql/templates/test_template_pool_relationships.py` around lines 368 - 382, The test currently exercises the id-based branch because it passes "tpl-test-address-pool" into the variable used for from_pool.id; add explicit HFID-path tests instead: create new test functions (e.g. test_create_template_with_pool_by_hfid and corresponding update test) that call the same mutation document or a small variant but supply a GraphQL input that sets from_pool.hfid (not from_pool.id) and pass the actual HFID value (from the ip_address_pool fixture) via variable_values; ensure the mutation document/variable names you use target the hfid field (refer to CREATE_TEMPLATE_WITH_POOL and from_pool.hfid/from_pool.id) so the code path for HFID lookup is exercised independently of the id-friendly-identifier branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/tests/component/graphql/templates/test_template_pool_relationships.py`:
- Around line 85-92: Add a negative GraphQL test method inside
TestTemplatePoolRelationships that attempts to set the internal relationship
field primary_ip_from_resource_pool directly via a create (and optionally
update) mutation and asserts the operation is rejected; locate the GraphQL
mutation helpers used elsewhere in this test file (e.g., the
createTemplate/updateTemplate mutation calls) and reuse them to submit a payload
containing primary_ip_from_resource_pool, then assert the response contains a
validation/error entry (HTTP error or GraphQL errors) and that no object is
created/modified. Ensure the test name clearly indicates it verifies
primary_ip_from_resource_pool is read-only at the GraphQL layer.
- Around line 368-382: The test currently exercises the id-based branch because
it passes "tpl-test-address-pool" into the variable used for from_pool.id; add
explicit HFID-path tests instead: create new test functions (e.g.
test_create_template_with_pool_by_hfid and corresponding update test) that call
the same mutation document or a small variant but supply a GraphQL input that
sets from_pool.hfid (not from_pool.id) and pass the actual HFID value (from the
ip_address_pool fixture) via variable_values; ensure the mutation
document/variable names you use target the hfid field (refer to
CREATE_TEMPLATE_WITH_POOL and from_pool.hfid/from_pool.id) so the code path for
HFID lookup is exercised independently of the id-friendly-identifier branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de8aa5cf-2833-411f-b48a-e6834e4c9ac5
📒 Files selected for processing (1)
backend/tests/component/graphql/templates/test_template_pool_relationships.py
| direct_peer = await reloaded.get_relationship("primary_ip").get_peer(db=db) | ||
| assert direct_peer is None | ||
|
|
||
| async def test_null_clears_both_relationships( |
There was a problem hiding this comment.
I feel this test could be misleading, as the 'primary_ip' relationship is never populated during the 'save' method.
I will modify this test in a commit so that it only explicitly checks both relationships emptiness while verifying the 'primary_ip_from_resource_pool' relationship is correctly populated during the save method.
I will add another one based on 'primary_ip' template creation
There was a problem hiding this comment.
Please tell me if I am wrong about the intention behind this test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/component/graphql/templates/test_template_pool_relationships.py (1)
109-111: Consider using public API instead of private attribute access.Line 109 accesses
registry._default_ipnamespace(private) while line 111 usesregistry.default_ipnamespace(public property). This asymmetry is fragile—if the internal implementation changes, the test may silently fail or behave unexpectedly.If there's a public getter available, prefer using it for consistency; otherwise, this pattern is acceptable for test setup code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/graphql/templates/test_template_pool_relationships.py` around lines 109 - 111, The test should avoid reading the private attribute registry._default_ipnamespace; use the public API consistently by checking the public property/ getter registry.default_ipnamespace (or calling the public method if one exists) before creating an IPAM namespace, and then assign via the public setter/attribute registry.default_ipnamespace = ip_namespace.id after create_ipam_namespace(db=db); update the conditional to reference registry.default_ipnamespace instead of registry._default_ipnamespace to keep usage symmetrical and resilient to internal changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/tests/component/graphql/templates/test_template_pool_relationships.py`:
- Around line 109-111: The test should avoid reading the private attribute
registry._default_ipnamespace; use the public API consistently by checking the
public property/ getter registry.default_ipnamespace (or calling the public
method if one exists) before creating an IPAM namespace, and then assign via the
public setter/attribute registry.default_ipnamespace = ip_namespace.id after
create_ipam_namespace(db=db); update the conditional to reference
registry.default_ipnamespace instead of registry._default_ipnamespace to keep
usage symmetrical and resilient to internal changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24b66d1b-f636-4515-8bfd-c647d7c26f0c
📒 Files selected for processing (2)
backend/tests/component/graphql/templates/test_template_pool_relationships.pypython_sdk
✅ Files skipped from review due to trivial changes (1)
- python_sdk
2 participants
goes with opsmill/infrahub-sdk-python#863 in the SDK
Why
Current approach to supporting relationships that provision from resource pools on templates is to use a
xyz_from_resource_poolrelationship that links to the CoreIPAddress/PrefixPool to use when provisioning from the template and wherexyzis the name of the relationship defined on the node schema's templatefor example, I create the
InfraInterfacenode schema that includes aprefixrelationship with a peer ofBuiltinIPPrefix. then I create a template forInfraInterfaceand I set theprefix_from_resource_poolrelationship to point to the CoreIPPrefixPool that I want to provision from when creating anInfraInterfaceinstance using this template.there is a good reason for using this approach in the backend (we don't have a way to save metadata about a relationship that does not yet exist), but the end user shouldn't have to deal with our implementation details. this approach is made even more confusing b/c we support NumberPools on attributes in templates in the regular way (i.e.
{from_pool: {id: "..."}})What changed
_from_resource_poolrelationships on templates to be read-only so that the user cannot try to edit themfrom_poolon a relationship that includes a_from_resource_poolrelationship will link the pool to the template using the_from_resource_poolrelationship, which is what the backend template provisioning logic expects_from_resource_poolrelationshipsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores