IFC-1258: Fix relationship mutation constraints by solababs · Pull Request #9270 · opsmill/infrahub

solababs · 2026-05-18T08:28:37Z

Why

RelationshipAdd and RelationshipRemove mutations did not enforce two schema constraints:

RelationshipAdd allowed adding multiple peers to a cardinality-one relationship, silently violating the schema.
RelationshipRemove allowed removing the last peer from an optional=False relationship, leaving the node in an invalid state.

How to test

uv run pytest backend/tests/component/graphql/test_mutation_relationship.py -v

Impact & rollout

Backward compatibility: Callers that were relying on the ability to add a second peer to a cardinality-one relationship via RelationshipAdd, or to remove the last peer from an optional=False relationship via RelationshipRemove, will now receive a ValidationError. These operations were already schema violations — this PR makes the API consistent with the schema definition.
Performance: One additional get_peers() DB query per RelationshipAdd on cardinality-one relationships; one per RelationshipRemove on optional=False relationships. Negligible overhead.

Checklist

Tests added/updated
Changelog entry added (backend/changelog/5794.fixed.md)
External docs updated (if user-facing or ops-facing change)
Internal .md docs updated (internal knowledge and AI code tools knowledge)
I have reviewed AI generated content

Summary by cubic

Enforces schema constraints for GraphQL relationship mutations to fix IFC-1258. Supports cardinality-one relationships: the first add is allowed; violations return ValidationError.

Bug Fixes
- RelationshipAdd rejects adding a second peer to a cardinality-one relationship.
- RelationshipRemove rejects removing the last peer from an optional=False relationship; partial removals are allowed.

^{Written for commit 0895d2e. Summary will update on new commits. Review in cubic}

cubic-dev-ai

1 issue found across 3 files

Confidence score: 2/5

There is a high-confidence, high-severity logic gap in backend/infrahub/graphql/mutations/relationship.py: cardinality-one validation only checks pre-existing peers, so a single request can still create multiple new peers.
This can violate IFC-1258 and lead to concrete data-integrity/regression risk in relationship mutations, so this is not just a minor edge case.
Pay close attention to backend/infrahub/graphql/mutations/relationship.py - enforce cardinality checks against newly added peers within the same request.

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/infrahub/graphql/mutations/relationship.py">

<violation number="1" location="backend/infrahub/graphql/mutations/relationship.py:498">
P1: Cardinality-one validation is incomplete: it checks only pre-existing peers, so one request can still add multiple new peers and violate IFC-1258.</violation>
</file>

_{Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic}

cubic-dev-ai · 2026-05-18T08:31:34Z

+        if existing_peers:
+            raise ValidationError(
+                {"name": f"'{relationship_name}' is a cardinality-one relationship and already has a peer"}


P1: Cardinality-one validation is incomplete: it checks only pre-existing peers, so one request can still add multiple new peers and violate IFC-1258.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/graphql/mutations/relationship.py, line 498: <comment>Cardinality-one validation is incomplete: it checks only pre-existing peers, so one request can still add multiple new peers and violate IFC-1258.</comment> <file context> @@ -492,6 +490,30 @@ async def _collect_current_peers( + rel_schema = source_node.get_schema().get_relationship(name=relationship_name) + if rel_schema.cardinality == RelationshipCardinality.ONE: + existing_peers = await _collect_current_peers(info=info, data=data, source_node=source_node) + if existing_peers: + raise ValidationError( + {"name": f"'{relationship_name}' is a cardinality-one relationship and already has a peer"} </file context>

Suggested change

if existing_peers:

raise ValidationError(

{"name": f"'{relationship_name}' is a cardinality-one relationship and already has a peer"}

if existing_peers or len(data.get("nodes") or []) > 1:

raise ValidationError(

{"name": f"'{relationship_name}' is a cardinality-one relationship and already has a peer"}

)

codspeed-hq · 2026-05-18T08:35:40Z

Merging this PR will not alter performance

✅ 12 untouched benchmarks

_{Comparing fix-relationship-mutation-constraints-ifc-1258 (0895d2e) with stable (72c3033)}

ajtmccarty · 2026-05-18T16:44:05Z

        await _validate_permissions(info=info, source_node=source, peers=nodes)
        await _validate_peer_types(info=info, data=data, source_node=source, peers=nodes)
        await _validate_peer_parents(info=info, data=data, source_node=source, peers=nodes)
+        await _validate_cardinality_add(info=info, data=data, source_node=source)


looks like this could be called after the call to _collect_current_peers on line 101/102 and it could receive the rel_schema and existing_peers as arguments to avoid calling _collect_current_peers twice

ajtmccarty · 2026-05-18T16:45:14Z

    @classmethod
    @retry_db_transaction(name="relationship_add")
-    async def mutate(
+    async def mutate(  # noqa: PLR0915


is it possible to add this an an exception for this file in pyproject.toml somewhere so that it is included in our general list of "things to clean up?"

ajtmccarty · 2026-05-18T16:51:33Z

        nodes = await _validate_peers(info=info, data=data)
        await _validate_permissions(info=info, source_node=source, peers=nodes)
        await _validate_peer_types(info=info, data=data, source_node=source, peers=nodes)
+        await _validate_optional_remove(info=info, data=data, source_node=source)


same idea here to avoid multiple calls to _collect_current_peers

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic}

ajtmccarty

I think the robot might have a point with its comment. now that the cardinality-many check is removed, it actually might be possible to try to add multiple peers to a cardinality-one relationship. this might require a new test

ajtmccarty · 2026-05-19T14:34:49Z

+    """ % (
+        person_jack_main.id,
+        tag_red_main.id,
+    )


we generally try to do this with graphql variables now

ajtmccarty · 2026-05-19T14:35:56Z


    rel_schema = source.get_schema().get_relationship(name=relationship_name)
-    if rel_schema.cardinality != RelationshipCardinality.MANY:
-        raise ValidationError({"name": f"'{relationship_name}' must be a relationship of cardinality Many"})


this makes it look like the first bug this PR is fixing should have already been blocked

RelationshipAdd allowed adding multiple peers to a cardinality-one relationship, silently violating the schema.

ajtmccarty · 2026-05-19T14:38:52Z

    assert source2_final.id == profile.id
+
+
+async def test_relationship_add_cardinality_one_rejected(


this looks like it is already tested above in test_relationship_wrong_name.
I'd say that this test should cover successfully adding a card-one relationship via graphql and then failing to add a second

ajtmccarty · 2026-05-19T14:40:46Z

+    assert primary_tag.id == tag_blue_main.id
+
+
+async def test_relationship_remove_mandatory_rejected(


this test and the one below look like they can be combined into a class to share fixtures and reduce loading time

cubic-dev-ai

0 issues found across 1 file (changes from recent commits).

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic}

…1258

ajtmccarty · 2026-05-26T13:58:59Z

+        }) {
+            ok
+        }
+    }


optional. I think you can refactor this so that this query is defined in one place and uses graphql variables so that it does not have to be generated within the tests multiple times. it would make these tests easier to read

ajtmccarty · 2026-05-26T14:23:23Z

@@ -0,0 +1 @@
+RelationshipAdd now rejects additions that would violate a cardinality-one constraint. RelationshipRemove now rejects removals that would leave a mandatory (optional=False) relationship without any peer.


the mutation already rejected cardinality-one constraint violations b/c it rejected any request to change a cardinality-one relationship. I think the fix is 1) that it now allows updating a cardinality-one relationship and 2) that it respects a mandatory constraint on a cardinality many relationship

ajtmccarty · 2026-05-26T14:28:18Z

+        variable_values={"id": person_jack_main.id, "node_id": tag_red_main.id},
+    )
+    assert result.errors
+    assert result.errors[0].message == "'primary_tag' is a cardinality-one relationship and already has a peer at name"


this error looks wrong, specifically the at name piece at the end. does the error need to be raised in a different way to apply to the primary_tag field?

IFC-1258: Fix relationship mutation constraints

aee813d

solababs requested a review from a team as a code owner May 18, 2026 08:28

github-actions Bot added the group/backend Issue related to the backend (API Server, Git Agent) label May 18, 2026

cubic-dev-ai Bot reviewed May 18, 2026

View reviewed changes

ajtmccarty reviewed May 18, 2026

View reviewed changes

refactor logic

8e520b1

cubic-dev-ai Bot reviewed May 19, 2026

View reviewed changes

solababs requested a review from ajtmccarty May 19, 2026 10:03

ajtmccarty reviewed May 19, 2026

View reviewed changes

update tests

34dbb22

cubic-dev-ai Bot reviewed May 25, 2026

View reviewed changes

Merge branch 'stable' into fix-relationship-mutation-constraints-ifc-…

0895d2e

…1258

solababs requested a review from ajtmccarty May 25, 2026 07:37

ajtmccarty reviewed May 26, 2026

View reviewed changes

		assert source2_final.id == profile.id


		async def test_relationship_add_cardinality_one_rejected(

		assert primary_tag.id == tag_blue_main.id


		async def test_relationship_remove_mandatory_rejected(

		@@ -0,0 +1 @@
		RelationshipAdd now rejects additions that would violate a cardinality-one constraint. RelationshipRemove now rejects removals that would leave a mandatory (optional=False) relationship without any peer.

Conversation

solababs commented May 18, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How to test

Impact & rollout

Checklist

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot May 18, 2026

Choose a reason for hiding this comment

Uh oh!

codspeed-hq Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ajtmccarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

solababs commented May 18, 2026 •

edited by cubic-dev-ai Bot

Loading

codspeed-hq Bot commented May 18, 2026 •

edited

Loading