feat: IFC-2162 Add namespace restriction parameter to generic schemas#8319
feat: IFC-2162 Add namespace restriction parameter to generic schemas#8319
Conversation
WalkthroughAdds a new optional 🚥 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. 📝 Coding Plan
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
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
13b9350 to
7a77e8e
Compare
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
7a77e8e to
742f3f7
Compare
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
742f3f7 to
46a96db
Compare
github-actions
bot
added
the
type/documentation
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Added some comments but looks good to be overall.
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
...d/tests/integration/schema_lifecycle/core-schema/update/test_update_restricted_namespaces.py
Outdated
Show resolved
Hide resolved
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
46a96db to
e2df08c
Compare
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
|
I think we'll need some additional work with the schema integrity checks for this one. The overall feature looks good however we have a situation with merge operations when if you:
# yaml-language-server: $schema=https://schema.infrahub.app/infrahub/schema/latest.json
version: '1.0'
generics:
- name: Problem
namespace: Base
description: Generic Location, could be a country, city ...
label: Base Problem
icon: mingcute:location-line
include_in_menu: false
#restricted_namespaces:
# - Support
order_by:
- name__value
display_labels:
- name__value
attributes:
- name: name
kind: Text
order_weight: 1000
- name: description
kind: Text
optional: true
order_weight: 1200
- name: problem_number
kind: NumberPool
description: "Give me the number!"
unique: true
optional: false
read_only: true
nodes:
- name: Problem
namespace: Support
label: "Problem"
display_labels:
- name__value
human_friendly_id: ["name__value"]
generate_profile: true
include_in_menu: true
attributes:
- name: name
kind: Text
order_weight: 1000
inherit_from:
- BaseProblem
- name: Incident
namespace: Support
label: "Incident"
display_labels:
- name__value
human_friendly_id: ["name__value"]
generate_profile: true
include_in_menu: true
inherit_from:
- BaseProblem
attributes:
- name: name
kind: Text
order_weight: 1000
|
|
I'd say the cross-branch schema integrity issue that Patrick identified is a separate issue because there are a lot of other cases where we can end up with a broken schema after a merge a couple untested examples
|
![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
🤖 Fix all issues with AI agents
In `@backend/tests/component/core/schema_manager/test_manager_schema.py`:
- Line 4596: Fix the typo in the inline comment inside
backend/tests/component/core/schema_manager/test_manager_schema.py where the
comment reads "maje the namespaces allowed list empty" — update it to "make the
namespaces allowed list empty" so the comment correctly describes the action in
the surrounding test code (search for the comment text "maje the namespaces" to
locate the exact spot).
🧹 Nitpick comments (1)
backend/tests/component/core/schema_manager/test_manager_schema.py (1)
4611-4630: Consider simplifying the happy-path assertion.The try/except/pytest.fail pattern is verbose. Since any unhandled exception would already fail the test, you could just call the method directly. This is a minor style suggestion.
Simplified version
- # Act - try: - schema.validate_restricted_namespaces_from_generic() - - # Assert - no ValueError - except ValueError: - pytest.fail( - "A ValueError has been raised during process_restricted_namespaces check, however, the schema is correct" - ) + # Act - should not raise + schema.validate_restricted_namespaces_from_generic() +
backend/tests/component/core/schema_manager/test_manager_schema.py
Outdated
Show resolved
Hide resolved
|
This PR has been inactive for 2 weeks and has been marked as stale. If you're still working on this, please:
The stale label will be removed automatically when there's new activity. |
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
a8dadcd to
b6e4c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/infrahub/core/schema/schema_branch.py (1)
1165-1166: Please rewrite this new docstring in Google style.The current docstring is not in the project’s required format and is hard to parse quickly.
As per coding guidelines: "Follow Google-style docstring format in Python" and "Include brief one-line description in Python docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/core/schema/schema_branch.py` around lines 1165 - 1166, Replace the current free-form docstring """Ensure that every node which inherit from a generic node containing restricted namespaces are following on the rules""" with a Google-style docstring for the enclosing function or method (the docstring owner in schema_branch.py) that starts with a one-line summary, followed by a blank line and sections for Args, Returns, and Raises as needed; e.g., a concise summary line describing the check, an Args section listing parameters (e.g., node, generic_node, namespace restrictions), a Returns section indicating the boolean or list result, and a Raises section for any exceptions the function may raise—ensure wording is clear, use proper capitalization and punctuation, and keep the one-line description as the first line.
🤖 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/schema/schema_branch.py`:
- Around line 1170-1179: Validation only checks direct entries in
node.inherit_from, so transitive generic ancestors' restricted_namespaces (e.g.,
A <- B <- node) are skipped; update the check by walking the inheritance graph
for each generic in node.inherit_from using get_generic(name=...,
duplicate=False), performing a DFS/BFS with a visited set to avoid cycles, and
for each ancestor generic_node inspect restricted_namespaces and raise the same
ValueError if node.namespace is not allowed; integrate this logic where the
current direct-check exists and preserve the existing error message format.
- Around line 652-655: The call order in the validation sequence is wrong: move
validate_kinds() to run before validate_restricted_namespaces_from_generic() so
unknown generics trigger the normalized kind-validation path (use the existing
methods validate_names, validate_kinds,
validate_restricted_namespaces_from_generic, validate_python_keywords to locate
the block); update the sequence to call validate_names(), then validate_kinds(),
then validate_restricted_namespaces_from_generic(), then
validate_python_keywords(), keeping all other logic intact.
---
Nitpick comments:
In `@backend/infrahub/core/schema/schema_branch.py`:
- Around line 1165-1166: Replace the current free-form docstring """Ensure that
every node which inherit from a generic node containing restricted namespaces
are following on the rules""" with a Google-style docstring for the enclosing
function or method (the docstring owner in schema_branch.py) that starts with a
one-line summary, followed by a blank line and sections for Args, Returns, and
Raises as needed; e.g., a concise summary line describing the check, an Args
section listing parameters (e.g., node, generic_node, namespace restrictions), a
Returns section indicating the boolean or list result, and a Raises section for
any exceptions the function may raise—ensure wording is clear, use proper
capitalization and punctuation, and keep the one-line description as the first
line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75c829a8-5fd7-4275-875c-48c2abb6f1ce
⛔ Files ignored due to path filters (1)
backend/infrahub/core/schema/generated/genericnode_schema.pyis excluded by!**/generated/**
📒 Files selected for processing (6)
backend/infrahub/cli/db.pybackend/infrahub/core/schema/definitions/core/repository.pybackend/infrahub/core/schema/definitions/internal.pybackend/infrahub/core/schema/schema_branch.pybackend/tests/component/core/schema_manager/conftest.pybackend/tests/component/core/schema_manager/test_manager_schema.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/component/core/schema_manager/conftest.py
- backend/tests/component/core/schema_manager/test_manager_schema.py
- backend/infrahub/cli/db.py
…arameter at true, the ComponentDependencyRegistry is not initialized IFC-2164
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
e19ee52 to
044c713
Compare
| ) | ||
| restricted_namespaces: list[str] | None = Field( | ||
| default=None, | ||
| description="Namespaces that Nodes inheriting from this Generic are restricted to", |
There was a problem hiding this comment.
maybe "Nodes inheriting from this Generic schema must belong to one of the listed namespaces"?
There was a problem hiding this comment.
Yes this is better, thanks
| internal_kind=str, | ||
| description="Namespaces that Nodes inheriting from this Generic are restricted to", | ||
| optional=True, | ||
| extra={"update": UpdateSupport.ALLOWED}, |
There was a problem hiding this comment.
should this be UpdateSupport.VALIDATE_CONSTRAINT?
for example, if I have Generic schema PrimaryA and Node schema SecondaryB inherits from it and then I add ["Primary"] as the PrimaryA.restricted_namespaces, would that break the schema?
UPDATE: this is fine as is. the schema update in the example above would be prevented by validation during the update
| schema.load_schema(schema=test_schema) | ||
|
|
||
| # Act | ||
| with pytest.raises(ValueError) as error: |
There was a problem hiding this comment.
not required, but you should be able to do pytest.raises(ValueError, match="does not comply with this restriction as its namespace") and then you could skip the final assertion
There was a problem hiding this comment.
This was intended to be consistent with the other unit tests, but I guess this can be disturbing since it asserts only one element. Will do!
| except ValueError: | ||
| pytest.fail( | ||
| "A ValueError has been raised during process_restricted_namespaces check, however, the schema is correct" | ||
| ) |
There was a problem hiding this comment.
do you still get the traceback in the pytest output with this structure?
There was a problem hiding this comment.
I am not sure to understand what is expected there but I got this when I force the test to fail
correct_schema_generic_with_namespace_restriction = SchemaRoot(version=None, generics=[GenericSchema(id=None, state=<HashableModelState.PRESENT: 'present'>, name='Generic...nt=None, children=None)], extensions=SchemaExtension(id=None, state=<HashableModelState.PRESENT: 'present'>, nodes=[]))
async def test_generic_schema_with_restricted_namespace_pass_if_same_namespace(
self,
correct_schema_generic_with_namespace_restriction,
) -> None:
schema: SchemaBranch = SchemaBranch(cache={}, name="test")
schema.load_schema(schema=correct_schema_generic_with_namespace_restriction)
# Act
try:
schema.validate_restricted_namespaces_from_generic()
# Assert - no ValueError
except ValueError:
> pytest.fail(
"A ValueError has been raised during process_restricted_namespaces check, however, the schema is correct"
)
E Failed: A ValueError has been raised during process_restricted_namespaces check, however, the schema is correct
There was a problem hiding this comment.
I removed this assertion and have let the test fail by itself if an error occurs
There was a problem hiding this comment.
this test file is way too big, which is not your fault
could you move the new tests to a new file on their own so this one doesn't get any bigger?
|
|
||
| @pytest.fixture(scope="class") | ||
| def schema_cat_node() -> dict[str, Any]: | ||
| """Cat node in Cat namespace, inheriting from Animal generic (violates Dog restriction).""" |
There was a problem hiding this comment.
this doesn't depend on the schema, so I think the description is not quite accurate
There was a problem hiding this comment.
is this really an integration test? it looks like it just uses SchemaBranch and doesn't require the API or workers to be running. if it were using the client to load the schema, then it would be an integration or functional test, but I don't think that's really necessary. we have tests verifying that the schema is correctly validated when loaded on a branch. I think this is either a component test or we don't need it
There was a problem hiding this comment.
This was to illustrate the precedent issue regarding schema validation which was not taking in account the schema once merged and was only triggered on schema on both branches. I think we can remove it since a test already exists there https://github.com/opsmill/infrahub/pull/8428/changes.
And this should be a component test yes, my bad
| class TestRestrictedNamespacesValidation(TestSchemaLifecycleBase): | ||
| """Test that modifying restricted_namespaces is validated against existing nodes.""" | ||
|
|
||
| async def test_change_restriction_should_fail( |
There was a problem hiding this comment.
this looks like it does basically the same thing as the other new test file. this should be a component test if it is required
There was a problem hiding this comment.
The other one was to verify that, given one correct schema on branch A and another correct schema on main, which would be invalid once merged together, the schema validation engine would detect this.
This one is just testing to load an invalid schema on a branch, without any merge process. I will move it into a dedicated component tests folder
Relocate tests from integration/schema_lifecycle/namespace_restriction and component/schema_manager/test_namespace_restriction.py into component/schema_manager/namespace_restriction/ package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
e282988 to
6b85103
Compare
- Replace try/except + pytest.fail with direct call - Use pytest.raises(match=...) instead of assert on str(error.value) - Simplify list assertions and remove redundant type annotations - Split into InvalidCases / ValidCases test classes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move fixtures from parent conftest.py into the namespace_restriction package conftest.py. Extract inline schema dict to a dedicated fixture. Fix docstring descriptions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
polmichel
force-pushed
the
pmi-20260204-namespace-restriction-ifc-2162
branch
from
d44ff79 to
5712eed
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/component/core/schema_manager/namespace_restriction/test_namespace_restriction.py">
<violation number="1" location="backend/tests/component/core/schema_manager/namespace_restriction/test_namespace_restriction.py:70">
P2: `TestNamespaceRestrictionValidCases` is accidentally nested inside `TestNamespaceRestrictionInvalidCases`. While pytest will still discover the nested class, this is semantically wrong (valid cases grouped under invalid cases) and means the valid-case tests will only show up as sub-items of the invalid-case class in test output. Dedent this class (and its methods) to the module level.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| with pytest.raises(ValueError, match="Generic node 'CoreGenericRepository' has restricted namespaces"): | ||
| schema.validate_restricted_namespaces_from_generic() | ||
|
|
||
| class TestNamespaceRestrictionValidCases: |
There was a problem hiding this comment.
P2: TestNamespaceRestrictionValidCases is accidentally nested inside TestNamespaceRestrictionInvalidCases. While pytest will still discover the nested class, this is semantically wrong (valid cases grouped under invalid cases) and means the valid-case tests will only show up as sub-items of the invalid-case class in test output. Dedent this class (and its methods) to the module level.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/component/core/schema_manager/namespace_restriction/test_namespace_restriction.py, line 70:
<comment>`TestNamespaceRestrictionValidCases` is accidentally nested inside `TestNamespaceRestrictionInvalidCases`. While pytest will still discover the nested class, this is semantically wrong (valid cases grouped under invalid cases) and means the valid-case tests will only show up as sub-items of the invalid-case class in test output. Dedent this class (and its methods) to the module level.</comment>
<file context>
@@ -0,0 +1,80 @@
+ with pytest.raises(ValueError, match="Generic node 'CoreGenericRepository' has restricted namespaces"):
+ schema.validate_restricted_namespaces_from_generic()
+
+ class TestNamespaceRestrictionValidCases:
+ async def test_generic_schema_with_restricted_namespace_pass_if_same_namespace(
+ self,
</file context>
5 participants
Add namespace restriction support for Generic schemas
Why
Problem statement
The current data modeling of Git repositories in Infrahub allows users to create schemas that inherit from Git repositories. However, this is unexpected and could lead to further issues, as assumptions are made about the state of Git repositories when handling them in code.
Goal
To prevent this from happening, a new attribute has been created for generic schemas (and, by extension, Git repositories) called "restricted_namespaces". This attribute can limit the values that namespaces can take on inherited nodes.
In the case of Git repositories schema, we will define the value of this new parameter so that the namespaces can only be "Core". The schema of these repositories is part of the core package. This will prevent any extension of the schema.
This parameter is not only meant for this situation, but it can also serve as additional data validation or control for users in their schema hierarchy.
Non-goals
The
infrahub db update-core-schemacommand was not functioning properly when a modification was made to the core schema due to the non-initialization of theinfrahub.dependencies.component.registry.ComponentDependencyRegistry.This was not the case when calling
infrahub db upgradecommand, which contained the initialization.What changed
["Core"]which prevent any extension.How-to review
This PR is quite small: commit by commit, in the chronological order.
How to test
Test plan
Automated back-end tests: constraint validation logic, within existing or new nodes.
uv run pytest backend/tests/integration/schema_lifecycle/test_restricted_namespaces_validation.py backend/tests/component/core/schema_manager/test_manager_schema.py -v -k "restricted_namespace or change_restriction"Front-end UI change: the new parameter should be visible
Impact & rollout
Deployment notes: this PR must be also merged feat: IHS-190 (part of IFC-2162) Add namespace restriction parameter to generic schemas infrahub-sdk-python#807
Closes IFC-2162, IFC-2163, IFC-2164, IFC-2165, IFC-2166, IHS-190
Summary by CodeRabbit
New Features
Documentation
Frontend
Tests
Changelog