Conversation
WalkthroughThe pull request refactors node feature flag initialization and hierarchical relationship setup across schema and node classes. In 🚥 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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #910 +/- ##
====================================================
- Coverage 81.09% 81.06% -0.03%
====================================================
Files 121 121
Lines 10592 10644 +52
Branches 1581 1586 +5
====================================================
+ Hits 8590 8629 +39
- Misses 1487 1498 +11
- Partials 515 517 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrahub_sdk/schema/main.py`:
- Around line 344-350: The supports_artifacts and supports_file_object checks
currently live only on NodeSchemaAPI causing ProfileSchemaAPI and
TemplateSchemaAPI (which also have inherit_from) to incorrectly return False;
move these property implementations to a shared base or mixin used by
MainSchemaTypesAPI (e.g., create an InheritFromSupportMixin with
supports_artifacts and supports_file_object that check "CoreArtifactTarget" and
"CoreFileObject" in inherit_from) or alternatively implement the same properties
on ProfileSchemaAPI and TemplateSchemaAPI so they mirror NodeSchemaAPI's
behavior; update references to supports_artifacts and supports_file_object
across MainSchemaTypesAPI and node/node.py to use the new shared implementation
to avoid FeatureNotSupportedError regressions.
- Around line 352-359: supports_hierarchy uses "self.hierarchy is not None" but
hierarchical_relationship_schemas uses "if not self.hierarchy", causing
inconsistent treatment of empty-string hierarchy; update
hierarchical_relationship_schemas to use the same predicate as
supports_hierarchy (i.e., check "self.hierarchy is not None") so both
supports_hierarchy and hierarchical_relationship_schemas (and any related
_hierarchy_support logic) treat an empty string consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de4b62fb-d38d-4b74-9fdc-36ae1126c1dc
📒 Files selected for processing (2)
infrahub_sdk/node/node.pyinfrahub_sdk/schema/main.py
| @property | ||
| def supports_artifacts(self) -> bool: | ||
| return "CoreArtifactTarget" in self.inherit_from | ||
|
|
||
| @property | ||
| def supports_file_object(self) -> bool: | ||
| return "CoreFileObject" in self.inherit_from |
There was a problem hiding this comment.
Share inherit_from capability checks with profile/template schemas.
Lines 68-69 in infrahub_sdk/node/node.py now read these properties for every MainSchemaTypesAPI, but only NodeSchemaAPI overrides them here. ProfileSchemaAPI and TemplateSchemaAPI still carry inherit_from, so any of those kinds inheriting CoreArtifactTarget or CoreFileObject now regress to False and can start raising FeatureNotSupportedError. Please move these checks onto a shared inherit_from-aware base/mixin, or override them on the profile/template schema classes as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrahub_sdk/schema/main.py` around lines 344 - 350, The supports_artifacts
and supports_file_object checks currently live only on NodeSchemaAPI causing
ProfileSchemaAPI and TemplateSchemaAPI (which also have inherit_from) to
incorrectly return False; move these property implementations to a shared base
or mixin used by MainSchemaTypesAPI (e.g., create an InheritFromSupportMixin
with supports_artifacts and supports_file_object that check "CoreArtifactTarget"
and "CoreFileObject" in inherit_from) or alternatively implement the same
properties on ProfileSchemaAPI and TemplateSchemaAPI so they mirror
NodeSchemaAPI's behavior; update references to supports_artifacts and
supports_file_object across MainSchemaTypesAPI and node/node.py to use the new
shared implementation to avoid FeatureNotSupportedError regressions.
| @property | ||
| def supports_hierarchy(self) -> bool: | ||
| return self.hierarchy is not None | ||
|
|
||
| @property | ||
| def hierarchical_relationship_schemas(self) -> list[RelationshipSchemaAPI]: | ||
| if not self.hierarchy: | ||
| return [] |
There was a problem hiding this comment.
Keep the hierarchy enablement predicate consistent.
Line 354 treats hierarchy="" as enabled, while Line 358 treats the same value as disabled. That can leave _hierarchy_support=True but produce no hierarchical wrappers. Use the same predicate in both properties.
🛠️ Minimal fix
`@property`
def supports_hierarchy(self) -> bool:
- return self.hierarchy is not None
+ return bool(self.hierarchy)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrahub_sdk/schema/main.py` around lines 352 - 359, supports_hierarchy uses
"self.hierarchy is not None" but hierarchical_relationship_schemas uses "if not
self.hierarchy", causing inconsistent treatment of empty-string hierarchy;
update hierarchical_relationship_schemas to use the same predicate as
supports_hierarchy (i.e., check "self.hierarchy is not None") so both
supports_hierarchy and hierarchical_relationship_schemas (and any related
_hierarchy_support logic) treat an empty string consistently.
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
coderabbit might have a couple legitimate comments, but, other than that, it looks great. big improvement
| GenericSchemaAPI, | ||
| ProfileSchemaAPI, | ||
| RelationshipCardinality, | ||
| RelationshipKind, | ||
| RelationshipSchemaAPI, | ||
| TemplateSchemaAPI, | ||
| ) |
2 participants
Why
The node initialization code in InfrahubNodeBase was determining capability flags (_artifact_support, _file_object_support, _hierarchy_support, _artifact_definition_support) by inspecting schema internals directly — using getattr to reach into inherit_from, string comparisons against hardcoded kind names, and isinstance checks to exclude schema types that don't carry certain fields. This logic belonged on the schema layer, not the node layer.
The change moves these capability checks to where the relevant data lives: BaseSchema provides safe defaults (False / []) for all schema types, and NodeSchemaAPI overrides them with real logic using its own fields (inherit_from, hierarchy). The same pattern is applied to hierarchical_relationship_schemas — rather than constructing four hardcoded RelationshipSchemaAPI objects inline in _init_relationships (duplicated across the async and sync classes), the schema itself now produces the correct pseudo-schemas for hierarchical nodes.
The result is that node.py no longer needs to know anything about the structure of the schema to determine what a node supports. Four uniform assignments replace a mix of getattr hacks, isinstance guards, and inline schema construction. The _init_relationships hierarchical block drops from ~70 lines (×2 for async/sync) to a single loop.
What changed
Summary by CodeRabbit
New Features
Improvements