Unreasonable cardinality constraints

[not-solis]

Hello all,

Before getting into this, I read the docs, and understand that the current capabilities surrounding the cardinality of relationships entails:

It is possible to (softly) enforce cardinality constraints on your relationships. Remember this needs to be declared on both sides of the relationship definition

I'm assuming that what is meant by "softly" is that this cardinality is only enforced on the connecting side of the relationship.

Take for example, my use case. I have a single node that has ZeroOrMore OUTGOING relationships, each to nodes that have One INCOMING relationship corresponding to that. Kinda looks like this:

Essentially, the One constraint should make this following case impossible:

Which it is, if I try to connect it from the StructuredNode that has the RelationshipDefinition constrained by One. But if I connect from the ZeroOrMore connected node, the other node's constraints aren't checked. Example code below:

class Test(StructuredNode):
    a = StringProperty(unique_index=True)
    other = RelationshipFrom('Test2', 'IS_UNDER', cardinality=One)

    def __init__(self, a, *args, **kwargs):
        kwargs['a'] = a
        super().__init__(*args, **kwargs)

class Test2(StructuredNode):
    b = StringProperty(unique_index=True)
    other = RelationshipTo('Test', 'IS_UNDER', cardinality=ZeroOrMore)

    def __init__(self, b, *args, **kwargs):
        kwargs['b'] = b
        super().__init__(*args, **kwargs)

install_all_labels()

testHub = Test2('a').save()
single1 = Test('b').save()
single2 = Test('c').save()
single3 = Test('d').save()

testHub2 = Test2('z').save()

testHub.other.connect(single1)
testHub.other.connect(single2)
testHub.other.connect(single3)

testHub2.other.connect(single1) # should fail, but doesn't
single1.other.connect(testHub2) # should fail, and does

Should the target of a relationship be able to establish connections? And either way, shouldn't both source and target be responsible for understanding the constraints of each side? This feels like a pretty common use case to me, so I'm curious to see if anyone else shares this concern.

[aanastasiou]

@mfboulos Thank you for bringing this to our attention. I need a little bit of time to go through it and let you know of either a clarification on current neomodel behaviour or possible modifications.

In the meantime, please note that a number of things around relationships are planned for revision. If any of those seems attractive or there is overlap with the issue you are raising please go ahead with a PR if you have the time and resources to spare.

All the best

[antipyrrhus]

Yup, this is something I've noticed previously and considered posting about but it slipped my mind. As you've noted, the way the .connect() method is currently implemented is that you must call it from the RelationshipManager attribute of one node (in your example, the other attribute) but not that of the the other node, thus making it hard(er) to check for constraints from the receiving node. I feel that the check can, however, be done in a pretty straightforward manner using a cypher query. I'll see if I can post a PR for it later.

[antipyrrhus]

Here is a commit that attempts to address the issue.

Basically it works by looking up the other node's relationship attributes and, once it finds an attribute whose type (in the OP's example, this would be "IS_UNDER") matches the original node's, then it checks whether this relationship has the constraint of either "One" or "ZeroOrOne", then checks whether the other node already has a connected neighbor pertaining to that relationship. If so, it throws an exception.

It also checks to ensure that the other node has a matching relationship attribute and ensures the direction of the edge is complementary (i.e. if one node's direction is incoming, the other should be outgoing, and vice versa; both directions being arbitrary is also acceptable).

I would welcome any comments / suggestions / corrections before considering putting this up for a PR.

[fsalmasri]

is this issue solved ?

[billycalladine]

@antipyrrhus any desire to make this PR? we're running into a similar issue raised here