Added automatic path inflation #716
Conversation
neomodel/util.py
Outdated
| new_relationships = [] | ||
| for relationship in object_to_resolve.relationships: | ||
| new_rel = self._object_resolution(relationship) | ||
| print(new_rel) |
There was a problem hiding this comment.
Please review the use of this print right here, probably a debugging remnant.
There was a problem hiding this comment.
@73VW Can you remove the print or resolve the conversation, whichever is right ?
neomodel/util.py
Outdated
| def __init__(self, nodes, *relationships): | ||
| assert all([isinstance(node, BaseNode) for node in nodes]) | ||
| for i, relationship in enumerate(relationships, start=1): | ||
| assert isinstance(relationship, BaseRel) |
There was a problem hiding this comment.
This check, along with the one on line 85 are possibly redundant. What else than nodes and relationships can the retrieved objects be here?
There was a problem hiding this comment.
I drew a lot of inspiration from the implementation of the neo4j Path class: https://github.com/neo4j/neo4j-python-driver/blob/5.0/src/neo4j/graph/__init__.py#L297. This might not be the best idea, I don't know.
On the other hand, I couldn't test that they were StructuredNode and StructuredRel because that was causing a cyclic import error. That's the reason I made base classes for both types.
I can remove them if you prefer.
There was a problem hiding this comment.
There was a problem hiding this comment.
@aanastasiou This is the last standing point for this PR, any opinion on this ?
There was a problem hiding this comment.
Sorry, I missed this indeed.
I believe it should be removed.
I have not tried to replicate the cyclic import error but creating a "pseudo-type" just for that assertion here does not sit well with me.
What I am saying is that there should be nothing else coming back from the database at this point other than nodes or relationships. Returning a Path is different than return this,that,the_other so why perform a check in the first place?
I am wondering if the driver uses this check because this behaviour is not uniform across different versions of the db?
But in this case, we can check the result of running this on different db versions through our tests. Is there anything in the test results to suggest that there are cases where NOT performing this assertion can lead to errors?
There was a problem hiding this comment.
Hey @aanastasiou, I will check that in depth during the week.
Regarding your last comment, it applies object resolution to path sub-objects, check this and this.
That makes the resolution recursive.
Or am I missing something?
There was a problem hiding this comment.
@73VW , the gist I used is here. This was used along with your PR.
When I examine the returned result, I am getting neo4j.graph.Node (i.e type(q[0][0][0].nodes)). This behaviour is common at least across Neo4j 4.4.18 and 5.9.
If this is not inline with what you are getting, then please post a detailed setup (neo4j server, neomodel version / commit, etc) along with a gist that demonstrates the bare minimum of the functionality you are contributing. Feel free to use mine as a basis from above. Chances are that this minimal gist will also constitute the basis for writing the pytest tests that test that part of the code. For more information, have a loot in this directory (See for example test_database_management.py).
There was a problem hiding this comment.
@73VW , any progress with this?
There was a problem hiding this comment.
Hey @aanastasiou, I'm not sure to understand what's wrong with this. Aren't we looking to obtain neo4j.graph.Node objects?
I might have missed something here.
Also, I didn't have time to look at the link you pasted. I have given a quick look but couldn't understand everything yet.
|
Hey @aanastasiou! Thanks for your review. Indeed that was a quick and dirty implementation because I needed it for one of my projects. I will take a moment to add a few tests and fix the points you've suggested. |
|
I resolved the merge conflicts for you. @73VW can you check that this is still OK ? Especially the Also waiting for the last two open points, then we can merge it. |
|
Hey @mariusconjeaud, I've removed the useless |
|
@73VW ...Was there a question I have missed? |
|
@aanastasiou yep, check this: #716 (comment) |
Based on discussion here: neo4j-contrib#716 (comment)
|
Kudos, SonarCloud Quality Gate passed!
|
|
Thanks @aanastasiou for fixing what was missing in my PR :) |
Implements #715
I am not really convinced about the implementation but I couldn't manage to do better without rewriting half of the package.
I welcome discussion and comments on this pull request.
I have not implemented tests but they will come as soon as I have time.
I've run all the tests except those for "Aura" because I don't know what it is. They finished succesfully.