Relationship cardinality declared on a concrete subtype is not enforced when the peer is a generic schema#9233
Conversation
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]>
9ab8845 to
4c27c22
Compare
| kwargs["max_count"] = max_count | ||
| if min_count is not None: | ||
| kwargs["min_count"] = min_count | ||
| return RelationshipSchema(**kwargs) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You are right, I can manage something regarding None value
| 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, | ||
| ) |
There was a problem hiding this comment.
this test looks like it belongs with other schema updating tests. it also might already exist
There was a problem hiding this comment.
test_schema_branch_validate_identifiers_matching_direction at backend/tests/component/core/schema_manager/test_manager_schema.py is indeed verifying the same
| @@ -0,0 +1 @@ | |||
| Cardinality constraints declared on a concrete subtype are now enforced when the relationship's declared peer is a parent generic. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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]>
There was a problem hiding this comment.
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]>
a985c54 to
473756f
Compare
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
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.subtype carries a count constraint
How to test
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.
NodeGetKindQueryand enforce that subtype’scardinality/max_count/min_count(respects direction; supports mixed subtypes and generic-vs-concrete declarations).tests.helpers.schema.roomand 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.