Skip to content

Relationship cardinality declared on a concrete subtype is not enforced when the peer is a generic schema#9233

Merged
polmichel merged 10 commits into
stablefrom
pmi-20260512-relationship-cardinality
May 15, 2026
Merged

Relationship cardinality declared on a concrete subtype is not enforced when the peer is a generic schema#9233
polmichel merged 10 commits into
stablefrom
pmi-20260512-relationship-cardinality

Conversation

@polmichel

@polmichel polmichel commented May 12, 2026

Copy link
Copy Markdown
Contributor

Why

When a relationship's declared peer is a generic schema, but the cardinality constraint sits on a concrete subtype of that generic (not on the generic itself), the constraint check is silently skipped. The violation only surfaces later, on read, as ValueError: At most, one relationship expected.

Closes #8953 / IFC-2470

Example

Using the schema described in the issue

---
# yaml-language-server: $schema=https://schema.infrahub.app/infrahub/schema/latest.json
version: "1.0"

nodes:
  - name: UnicastIPAddress
    namespace: Network
    inherit_from:
      - BuiltinIPAddress
    relationships:
      - name: layer3
        peer: InterfaceLayer3
        kind: Generic
        optional: true
        cardinality: one
        identifier: ip__l3interface
        direction: inbound

  - name: AnycastIPAddress
    namespace: Network
    inherit_from:
      - BuiltinIPAddress
    relationships:
      - name: layer3
        peer: InterfaceLayer3
        kind: Generic
        optional: true
        cardinality: many
        identifier: ip__l3interface
        direction: inbound

  - name: Layer3
    namespace: Interface
    attributes:
      - name: name
        kind: Text
        unique: true
    relationships:
      - name: ip_addresses
        peer: BuiltinIPAddress
        kind: Attribute
        optional: true
        cardinality: many
        identifier: ip__l3interface
        direction: outbound

Scenario

Create a second Layer3 whose ip_addresses already point at a UnicastIPAddress that has a Layer3 peer -> This is silently accepted. The violation only surfaces later, at read time.

What this PR does

  • RelationshipCountConstraint: when the relationship's declared peer is a generic with no matching rel, falls back to resolving each peer's concrete kind and applying that subtype's cardinality / max_count / min_count.
  • lock_utils: when the declared peer is a generic, conservatively acquires the lock if any concrete
    subtype carries a count constraint

How to test

INFRAHUB_USE_TEST_CONTAINERS=true uv run pytest \
    backend/tests/component/core/constraint_validators/test_relationship_manager.py \
    backend/tests/component/core/test_get_kinds_lock.py::TestLockGenericPeerConcreteConstraint \
    backend/tests/component/core/test_get_kinds_lock.py::TestLockGenericPeerGenericRel -v

Summary by cubic

Fixes missing count enforcement when a relationship’s peer is generic and the constraint is declared on a concrete subtype. Constraints now run on mutation and locks cover these cases, closing #8953 / IFC-2470.

  • Bug Fixes
    • RelationshipCountConstraint: if the generic peer has no matching rel, resolve each changed peer’s concrete kind via NodeGetKindQuery and enforce that subtype’s cardinality/max_count/min_count (respects direction; supports mixed subtypes and generic-vs-concrete declarations).
    • lock_utils: when the declared peer is generic, acquire the count lock if any concrete subtype defines a count constraint for the same identifier (conservative; prevents under-locking).
    • Tests/docs: added tests.helpers.schema.room and expanded matrix for generic peers, max/min, mixed subtypes, and bidirectional cases; use the public relationship getter; removed a redundant same-direction test; clarified the changelog entry.

Written for commit 473756f. Summary will update on new commits.

@polmichel polmichel self-assigned this May 12, 2026
@github-actions github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 12, 2026
@codspeed-hq

codspeed-hq Bot commented May 12, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing pmi-20260512-relationship-cardinality (473756f) with stable (be7be1e)

Open in CodSpeed

@polmichel polmichel marked this pull request as ready for review May 12, 2026 21:59
@polmichel polmichel requested a review from a team as a code owner May 12, 2026 21:59
@ogenstad

Copy link
Copy Markdown
Contributor

If this gets included in 1.9.4 we might as well also include #9203 since we need to regenerate the release notes in #9234 regardless.

polmichel and others added 7 commits May 13, 2026 10:27
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adds class-grouped tests covering generic-vs-concrete cardinality across the
count constraint and lock_utils code paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
… generic peer (closes #8953)

When a relationship declares its peer as a generic but the cardinality constraint
(cardinality=one, max_count, or min_count) is declared on a concrete subtype,
both the runtime constraint check and the lock acquisition were silently
skipping the constraint. They now resolve the concrete kind of each peer
(constraint check) or walk the generic's used_by list (lock path) and enforce
the appropriate constraint.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@polmichel polmichel force-pushed the pmi-20260512-relationship-cardinality branch from 9ab8845 to 4c27c22 Compare May 13, 2026 08:28
Comment thread backend/tests/helpers/schema/room.py Outdated
kwargs["max_count"] = max_count
if min_count is not None:
kwargs["min_count"] = min_count
return RelationshipSchema(**kwargs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to create the RelationshipSchema using kwargs instead of a dict? not sure if you can pass min/max_count=None in to the constructor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I can manage something regarding None value

Comment on lines +274 to +284
async def test_schema_rejects_same_direction_same_identifier(
self, db: InfrahubDatabase, default_branch: Branch
) -> None:
with pytest.raises(ValueError, match=r"Incompatible direction"):
registry.schema.register_schema(
schema=build_room_schema(
occupant_direction="outbound",
rooms_direction="outbound",
),
branch=default_branch.name,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test looks like it belongs with other schema updating tests. it also might already exist

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_schema_branch_validate_identifiers_matching_direction at backend/tests/component/core/schema_manager/test_manager_schema.py is indeed verifying the same

Comment thread changelog/8953.fixed.md Outdated
@@ -0,0 +1 @@
Cardinality constraints declared on a concrete subtype are now enforced when the relationship's declared peer is a parent generic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this hard to understand. maybe something like

Fix relationship constraint validation to enforce minimum count, maximum count, and cardinality defined on the peer schema of the relationship being updated when the peer schema is a generic used by concrete subtypes that define their own relationships

... that's not much better. maybe this needs more than one sentence

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go for two sentences

Cardinality constraints (cardinality: one, min_count, max_count) were silently skipped when the relationship being updated declared its peer as a generic and the constraint was defined on a concrete subtype of that generic, letting data violate the schema. The constraint check and lock path now resolve each peer's concrete kind and enforce the constraint declared there.

If this is not clear enough, let me know and I will rework once this has been merged

polmichel and others added 2 commits May 15, 2026 09:22
Already covered by test_schema_branch_validate_identifiers_matching_direction
in schema_manager/test_manager_schema.py.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Shadow auto-approve: would require human review. These changes modify core constraint validation and lock acquisition logic for relationships with generic peers, which is a critical part of data integrity enforcement and concurrency control, so human review is required to ensure correctness and avoid introducing new bugs.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@polmichel polmichel force-pushed the pmi-20260512-relationship-cardinality branch from a985c54 to 473756f Compare May 15, 2026 07:43
@polmichel polmichel merged commit 1abb44e into stable May 15, 2026
132 of 137 checks passed
@polmichel polmichel deleted the pmi-20260512-relationship-cardinality branch May 15, 2026 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Relationship cardinality not enforced when using directions

3 participants