Prevent setting generate_template: true on generics#8379
Open
gmazoyer wants to merge 10 commits intorelease-1.8from
Open
Prevent setting generate_template: true on generics#8379gmazoyer wants to merge 10 commits intorelease-1.8from
generate_template: true on generics#8379gmazoyer wants to merge 10 commits intorelease-1.8from
Conversation
Contributor
WalkthroughGRAPH_VERSION bumped to 62 and a new migration (Migration062) added to delete 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
436da5a to
0552323
Compare
github-actions
bot
added
the
type/documentation
Contributor
|
Is this attribute ever set to |
Contributor
|
FYI, this should target the |
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
c51c1f9 to
d7a4349
Compare
github-actions
bot
added
the
group/frontend
github-actions
bot
removed
the
group/frontend
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
1ae35c3 to
535cca0
Compare
github-actions
bot
added
the
group/frontend
gmazoyer
force-pushed
the
gma-20260213-8371
branch
3 times, most recently
from
09c52c5 to
91bfe79
Compare
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
91bfe79 to
2e70ab1
Compare
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
d27b6c6 to
966699d
Compare
gmazoyer
commented
Feb 17, 2026
| self.add_to_query(query) | ||
|
|
||
|
|
||
| class Migration062(GraphMigration): |
Contributor
Author
There was a problem hiding this comment.
Not sure if this migration is too brutal. I expected that I needed to use NodeAttributeRemoveMigration but that was unsuccessful when running infrahub upgrade.
gmazoyer
force-pushed
the
gma-20260213-8371
branch
from
04376ae to
8d28fe4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py (1)
60-69:validate_migrationis a no-op — consider adding a pre-check.The
validate_migrationmethod returns an emptyMigrationResultwithout checking the graph state. This is acceptable since the queries are idempotent, but a pre-check verifying whether the migration is needed (e.g., checking ifgenerate_templateattributes exist onSchemaGenericnodes) could provide better operational visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py` around lines 60 - 69, Migration062.validate_migration is currently a no-op; add a pre-check that queries the graph (via the provided db/InfrahubDatabase) for SchemaGeneric nodes that still have the generate_template attribute and use that to populate and return a meaningful MigrationResult (e.g., mark as not_needed when none found or include a warning/info when some exist) before running RemoveGenericGenerateTemplateQuery and RemoveGenericGenerateTemplateSchemaAttributeQuery; reference Migration062, validate_migration, MigrationResult, RemoveGenericGenerateTemplateQuery, RemoveGenericGenerateTemplateSchemaAttributeQuery, and the SchemaGeneric·generate_template attribute when implementing the check.
🤖 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/migrations/graph/m062_remove_generic_generate_template.py`:
- Around line 29-57: The DETACH DELETE in
RemoveGenericGenerateTemplateSchemaAttributeQuery.query_init currently removes
sa and rel but leaves child Attribute and AttributeValue nodes orphaned; update
the Cypher to explicitly MATCH the child Attribute nodes (e.g., via
sa-[:HAS_ATTRIBUTE]->(attr:Attribute)) and their value nodes
(attr-[:HAS_VALUE]->(val)) with the same active relationship filters, collect
them (attr and val) and include them in the DETACH DELETE so you remove sa, rel,
attr and val together; keep the existing variables (sa, rel) and the WHERE all(r
IN relationships(...) ... ) checks when adding the new MATCHes to ensure only
active nodes are deleted.
---
Nitpick comments:
In
`@backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py`:
- Around line 60-69: Migration062.validate_migration is currently a no-op; add a
pre-check that queries the graph (via the provided db/InfrahubDatabase) for
SchemaGeneric nodes that still have the generate_template attribute and use that
to populate and return a meaningful MigrationResult (e.g., mark as not_needed
when none found or include a warning/info when some exist) before running
RemoveGenericGenerateTemplateQuery and
RemoveGenericGenerateTemplateSchemaAttributeQuery; reference Migration062,
validate_migration, MigrationResult, RemoveGenericGenerateTemplateQuery,
RemoveGenericGenerateTemplateSchemaAttributeQuery, and the
SchemaGeneric·generate_template attribute when implementing the check.
backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py (1)
56-63: ConsiderOPTIONAL MATCHfor child attributes to avoid silent no-ops.Line 57 uses a required
MATCHfor child attributes and values. If anySchemaAttributechildAttributenode happens to lack aHAS_VALUEedge, the entireMATCHfails for that row, andsa/relwon't be deleted at all (the query silently does nothing). AnOPTIONAL MATCHwith a null-safe cleanup would be more defensive:Proposed fix
// Find child Attribute nodes and their value nodes -MATCH (sa)-[:HAS_ATTRIBUTE]->(attr:Attribute)-[:HAS_VALUE]->(val) -// Delete the SchemaAttribute, Relationship node, and child Attributes -DETACH DELETE sa, rel, attr -// Clean up orphaned value nodes (Boolean nodes are shared and will still have edges) -WITH val -WHERE NOT EXISTS { MATCH (val)-[]-() } -DELETE val +OPTIONAL MATCH (sa)-[:HAS_ATTRIBUTE]->(attr:Attribute) +OPTIONAL MATCH (attr)-[:HAS_VALUE]->(val) +// Delete the SchemaAttribute, Relationship node, and child Attributes +DETACH DELETE sa, rel +WITH attr, val +WHERE attr IS NOT NULL +DETACH DELETE attr +WITH val +WHERE val IS NOT NULL AND NOT EXISTS { MATCH (val)-[]-() } +DELETE valIn practice, every
Attributeshould have a value node, so the current code works for the expected data shape. This is a robustness suggestion only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py` around lines 56 - 63, The current required MATCH for child Attribute/value (MATCH (sa)-[:HAS_ATTRIBUTE]->(attr:Attribute)-[:HAS_VALUE]->(val)) can cause the row to be skipped if an Attribute lacks a HAS_VALUE edge; change it to an OPTIONAL MATCH for the attr->val relationship (keep the parent match for sa/rel/attr) so sa/rel/attr are still deleted even when val is null, then adjust the subsequent WITH/WHERE cleanup to only attempt deleting val when it is not null (e.g., pass val through the WITH and conditionally run the orphan check WHERE NOT EXISTS { MATCH (val)-[]-() } DELETE val).
🤖 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/infrahub/core/migrations/graph/m062_remove_generic_generate_template.py`:
- Around line 56-63: The current required MATCH for child Attribute/value (MATCH
(sa)-[:HAS_ATTRIBUTE]->(attr:Attribute)-[:HAS_VALUE]->(val)) can cause the row
to be skipped if an Attribute lacks a HAS_VALUE edge; change it to an OPTIONAL
MATCH for the attr->val relationship (keep the parent match for sa/rel/attr) so
sa/rel/attr are still deleted even when val is null, then adjust the subsequent
WITH/WHERE cleanup to only attempt deleting val when it is not null (e.g., pass
val through the WITH and conditionally run the orphan check WHERE NOT EXISTS {
MATCH (val)-[]-() } DELETE val).
ac371f1 to
e1ff1c9
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
ajtmccarty
approved these changes
Feb 17, 2026
Comment on lines
+23
to
+27
| MATCH (sg:SchemaGeneric)-[:HAS_ATTRIBUTE]->(attr:Attribute {name: "generate_template"})-[:HAS_VALUE]->(val) | ||
| DETACH DELETE attr | ||
| WITH val | ||
| WHERE NOT EXISTS { MATCH (val)-[]-() } | ||
| DELETE val |
Contributor
There was a problem hiding this comment.
I think you can just do
MATCH (sg:SchemaGeneric)-[:HAS_ATTRIBUTE]->(attr:Attribute {name: "generate_template"})
WITH DISTINCT attr
DETACH DELETE attr
b/c the value would always just be either True or False and I think you can assume those AttributeValue vertices are used elsewhere
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Setting
generate_template: trueon aGenericSchemacreates an invalid GraphQL schema, which breaks the frontend and prevents any GraphQL queries or mutations from running. This PR removes thegenerate_templateattribute fromGenericSchemaso that it cannot be set anymore.Important
We should probably backport this to
stabletoo.Closes #8371
What changed
generate_templateon a generic since the attribute no longer existsTestSchemaTemplateGenericInheritancethat exercised the now-blocked scenarioAuto-generated sub-templates for generics used as component peers (e.g.
InfraInterfacegeneric referenced via a component relationship) are not affected, that path relies already supported generics without requiring them to havegenerate_templateset totrue.How to review
Small diff. Focus on the validation method in
schema_branch.pybut it's mostly about addingisinstance(node, ...)for typing reasons and conditionals. There is also a migration to remove the attribute from the graph.How to test
Impact & rollout
generate_template: trueon a generic will get an error on next schema load but there should not be running instances out there using this, as they would be broken anyway.Checklist
uv run towncrier create ...)Summary by CodeRabbit
Bug Fixes
Migrations
Documentation
UI
Chores