neo4j-contrib / neomodel

An Object Graph Mapper (OGM) for the Neo4j graph database.
https://neomodel.readthedocs.io
MIT License
939 stars 231 forks source link

Add RelationshipManager.reversed() as a shortcut to reversed relationships #637

Closed AntonLydike closed 1 year ago

AntonLydike commented 2 years ago

This PR wants to introduce .reversed() as a shortcut to get a new relationship manager for edges in the reversed direction of the original manager. For example:

class Animal(StructuredNode):
  ate = RelationshipTo('Animal', 'ATE')

a = Animal().save()
b = Animal().save()

a.ate.connect(b)

# get "eaten" relation
b.ate.reversed().all()

While this example is quite contrived, I found myself wanting to traverse some relationships backwards without having to declare them twice. This method seems like a nice way of achieving this.

aanastasiou commented 1 year ago

Hello @AntonLydike , this is an interesting feature and I am wondering if it is possible to create queries that contain loops with it :/

Can I please ask you to put something in the documentation as well along the lines of what reversed() is doing?

mariusconjeaud commented 1 year ago

It shouldn't create loops since Cypher will by default only ever traverse a given relationship once in the same MATCH group. So it cannot from A to B then back to A using the same relationship.

That being said, I think deciding on this feature's relevance comes down to the question of "should a relationship in an OGM be defined on both sides, or on a single side ?". Because if we allow this, then why not allow the case of :

class Animal(StructuredNode):
  ate = RelationshipTo('Plant', 'ATE')

class Plant(StructuredNode):
   # Not defining rel because defined on the other side

animal = Animal().save()
plant = Animal().save()

animal.ate.connect(plant)

# pseudo-code : 
plant.eaten_by(defined_by=Animal.ate).all()

Not saying I have on opinion on this ; just that the two are linked in my opinion.

aanastasiou commented 1 year ago

It shouldn't create loops since Cypher will by default only ever traverse a given relationship once in the same MATCH group.

@mariusconjeaud At the CYPHER level it probably does not create problems but what I am wondering is if it creates a problem when you do an animal.ate.reversed().ate.all(). In other words, the doubt is whether it would create a problem at the OGM code level, not its queries.

That being said, I think deciding on this feature's relevance comes down to the question of "should a relationship in an OGM be defined on both sides, or on a single side ?". Because if we allow this, then why not allow the case of :

As far as I can tell, this is not an OGM concern but a data modelling concern. Personally, I do not think that it is adding anything because it should be possible to re-express the query to reach the required entity from the "other side" of the reversed relationship.

It would be nice to have a few use case examples to support feature requests such as this one. I am not saying it is useless.

mariusconjeaud commented 1 year ago

It is a data modelling concern I agree ; but as an OGM library, we can provide guidance regarding this question of symmetry in relationship declaration. That is what I mean here.

It is useful in the sense that it has use cases, since it saves defining one more relationship in the class definitions. But maybe the tradeoff between usefulness (limited since there is a perfectly valid workaround) and the open questions (potential loop side effect, readability of the syntax, validating the concept of asymetric relationship definition) is too low for approving this ?

aanastasiou commented 1 year ago

...it saves defining one more relationship in the class definitions...

@mariusconjeaud but then that would become an undirected relationship, no? It is not advisable to have single direction relationships in both directions (except of course if they bare different data and even in that case, they probably can be modelled as an undirected edge).

Overall, I would agree too with your comment about the tradeoff being too low for approval.

mariusconjeaud commented 1 year ago

Closing for housekeeping and based on the comments and recent activity on this PR. Feel free to re-open with more examples to back the arguments ?