Fix: Enforce cardinality on RelationshipFrom and bidirectional relationships

[billycalladine]

Context

This pull request addresses a critical data integrity issue where cardinality constraints on RelationshipFrom were not being enforced, leading to potential inconsistencies in the graph data. This change introduces bidirectional cardinality validation to ensure that relationship constraints are respected from both the source and target nodes' perspectives.

Problem

Previously, neomodel only validated the cardinality of a relationship from the source side (the node on which the .connect() method is called). This meant that a RelationshipTo with a specific cardinality (One, ZeroOrOne) would be correctly validated, but the corresponding RelationshipFrom on the target node would not be.

For example, if a Pet could only have one Owner (RelationshipFrom('Owner', 'OWNS', cardinality=One)), it was still possible to connect multiple Owner nodes to the same Pet, violating the constraint without raising an error.

This fixes issues #464 and #867 and #891

Solution

The core of the solution is to enforce cardinality checks on both ends of a relationship during the connect operation.

1. Bidirectional validation in connect:

   • The connect method in RelationshipManager has been updated to perform two checks:
     1. Local Cardinality: It validates the cardinality on the source relationship, as before.
     2. Remote Cardinality: It now inspects the target node to find the inverse relationship definition. If found, it validates the cardinality from the target node's perspective before creating the relationship.

2. _check_cardinality method:

   • A new method, _check_cardinality, has been introduced to the base RelationshipManager.
   • This method is implemented in the specific cardinality classes (One, ZeroOrOne) to encapsulate the validation logic.
   • The original validation code was moved from the connect methods of the cardinality classes into this new method, centralising the logic.

How to verify

A test case has been added to confirm the fix. The following scenario now works as expected:

class Owner(StructuredNode):
    name = StringProperty()
    pets = RelationshipTo('Pet', 'OWNS')

class Pet(StructuredNode):
    name = StringProperty()
    owner = RelationshipFrom('Owner', 'OWNS', cardinality=One)

owner1 = Owner(name="Alice").save()
owner2 = Owner(name="Bob").save()
pet = Pet(name="Fluffy").save()

# This connection succeeds
owner1.pets.connect(pet)

# This connection now correctly fails with AttemptedCardinalityViolation
with pytest.raises(AttemptedCardinalityViolation):
    owner2.pets.connect(pet)

Impact

• Data Integrity: This change significantly improves data integrity by preventing invalid relationship structures.
• Backward Compatibility: This is a breaking change for applications that implicitly relied on the buggy behaviour of unenforced RelationshipFrom cardinality. Code that created relationships violating these constraints will now raise an AttemptedCardinalityViolation. This is the correct and expected behaviour.
• Performance: The fix introduces a small overhead by requiring an additional lookup for the inverse relationship definition on the target node. This is a necessary trade-off for ensuring data correctness and is performed in-memory without extra database queries.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[billycalladine]

Tagging top contributors for visibility @robinedwards @aanastasiou @mariusconjeaud @pkatseas @tonioo

[mariusconjeaud]

Hello !

Thanks a ton for this very well documented and explained Issue & PR ! And also for the thorough research and linking with similar issues.

I have been away from neomodel issues for the past few months, so apologies for the delay in reviewing this.
I will approved and merge for the next minor release, which would happen very soon !

[billycalladine]

@mariusconjeaud thanks for the approval and merge! 🥳

On reflection, this could be a breaking change for unsuspecting users that are using RelationshipFrom but without the knowledge that it isn't currently working as expected. Having said that, if you are specifying cardinality in your code then you should expect this to work. So while this PR fixes cardinality not being enforced for RelationshipFrom, it may introduce breaking behaviour for existing users.

Specifically, applications that currently connect multiple nodes through a RelationshipFrom with cardinality=One or ZeroOrOne will start raising AttemptedCardinalityViolation. From a correctness perspective, this is the expected behaviour (and it matches the documentation as well as RelationshipTo). However, in practice, users who were unaware of the bug may experience errors after upgrading.

Nevertheless, since developers explicitly declare cardinality in their model, it’s reasonable they should expect enforcement. The fix makes the API consistent and aligns with the documented contract, but I wanted to flag the behavioural change so it’s not surprising to users.

[billycalladine]

my bad, I have just caught up with everything and read your comment here. You're already way ahead of me 😅

Thanks for resolving 🙌