Closed diegopso closed 2 months ago
@tonioo or @owinogradow, do you think you could have a look ? fetch_relations is something we merged back from the NN fork right ?
@diegopso Coming back to this, I think that :
id
as its internal identifier, and Neo4j 5 now uses element_id
; but since we still support Neo4j 4, neomodel has to keep both in the node definition. It will then use the right id
/element_id
depending on the version of the Neo4j server you are connected to (so in your case, element_id). So you can ignore that traceback.Note on id
/element_id
: we do provide the possibility to users to use id
and element_id
in whatever way they want, but we align with the Neo4j guideline, which is that those are internal ids that should only be used by the database itself; they are not guaranteed to be immutable. To identify your nodes in a robust way, you should apply your own unique identifier system.
OK, I compared your example with the one in the documentation, and I can confirm that the problem is "just" that the documentation was very bad on that one. Sorry about that.
I will update accordingly, and before I do that, here is what is wrong/missing in the documentation:
MyNode.nodes.fetch_relations('myrel__myotherrel').all()
in this order, and not all
before fetch_relations
StructuredRel
. That is because fetch_relations will always try to do the object resolution, as it does when running db.cypher_query('MATCH (n) RETURN n', resolve_objects=true)
; and that relies on the NODE_CLASS_REGISTRY
object, in which relationships are added only if they have a model attached to them. So in this case, "IS_FROM" does not have a model => not in the node class registry => neomodel does not know what object to resolve it intoSorry for those two oversights; we had to integrate this back from an end-user branch, and it required quite some structural changes, and so we missed a few points (in that branch, which is actually a full application, relationships always have a model attached to them, so I missed that this was required)
@diegopso Please check the linked PR. It does not change the behaviour (because it makes sense that neomodel expects a model to be defined for a relationship it is trying to resolve into a model object); but it fixes and improves the documentation, as well as the content of the exception, so it is clearer what is expected.
Thanks a ton for raising this up ! This is the latest big feature (except for async), and I feel we never truly completed it. That part definitely deserves more attention (it makes neomodel way more graphy after all)
@mariusconjeaud thank you for the updates. I think now the docs look good.
I agree that a model for a relationship should be defined to be resolved. Still, when implementing a workaround on my side, I extended RelationshipTo
and RelationshipFrom
to change the default model argument in the init from None
to StructuredRel
. This, at least for me, made such that there is always a model to resolve the relationship to. This should be enough for all relationships that have no extra properties, and just custom relationships require writing specific classes.
I am mentioning it because you also have something similar for your passing fetch_relations
tests in these classes: https://github.com/neo4j-contrib/neomodel/blob/795-cant-retrieve-additional-relations-using-fetch_relations/test/sync_/test_match_api.py#L35-L46
Used in this test case: https://github.com/neo4j-contrib/neomodel/blob/795-cant-retrieve-additional-relations-using-fetch_relations/test/sync_/test_match_api.py#L531
Still, as is now is much easier to understand and use it.
Your solution is interesting. I actually thought about that while working on this, but I was looking at a different place where to add StructuredRel
as default, and it was not suitable.
But your solution looks better !
It would need testing though, to make sure it's not filling the node class registry
with many relationship classes.
I mean that if you define two distinct relationships with the same type
string (like, IS_FROM
), then it might be trying to create two entries with that key in the registry and trigger a conflict.
Expected Behavior (Mandatory)
I expect that I can define models without defining
StructuredRel
and use the methodfetch_relations
to retrieve relationships of my model as described in https://neomodel.readthedocs.io/en/latest/getting_started.html#retrieving-additional-relationsActual Behavior (Mandatory)
An exception is thrown:
How to Reproduce the Problem
Simple Example
For instance, the code below from docs (with adjusts) should work given the correct DB URL.
Work around
If I manually define the model of the relationship to be
StructuredRel
classes, the error is not triggered.Adding:
And changing the relation to:
Results in:
Although this works, it brings some weird trace info if looking in a debugger:
Screenshots (where it's possibile)
The error on my terminal:
Specifications (Mandatory)
Currently used versions
Versions