142-feature/make-ids-optional #155
Conversation
| @@ -86,11 +87,14 @@ private Object getFieldValue(@NonNull Object domain, @NonNull String fieldName) | |||
| final ConvertingPropertyAccessor accessor = this.getPropertyAccessor(domain); | |||
| final GremlinPersistentEntity<?> persistentEntity = this.getPersistentEntity(domain.getClass()); | |||
| final PersistentProperty property = persistentEntity.getPersistentProperty(fieldName); | |||
| Assert.notNull(property, "persistence property should not be null"); | |||
| if (!fieldName.equals(Constants.PROPERTY_ID)) { | |||
There was a problem hiding this comment.
should not workaround the PROPERTY_ID, as the field which has @Id without id name. And also if you want to support Id optional, this may not the right place from the method name.
| Assert.isTrue(type == GremlinEntityType.EDGE || type == GremlinEntityType.VERTEX, "should be edge/vertex type"); | ||
|
|
||
| final String prefix = (type == GremlinEntityType.VERTEX) ? "V" : "E"; | ||
| if (id != null) { |
There was a problem hiding this comment.
the ID optional should not change the requiredId generation, should take care of id optional outside the caller.
|
@Incarnation-p-lee Thanks for the feedback. This was indeed a bit quick-and-dirty, and I see now that a much better approach would be to add an optional } else if (field.getAnnotation(EdgeFrom.class) != null) {
sourceEdge.setVertexIdFrom(this.getIdValue(object, converter));
} else if (field.getAnnotation(EdgeTo.class) != null) {
sourceEdge.setVertexIdTo(this.getIdValue(object, converter));Basically we'd have to throw exceptions if trying to write an edge for which the in and out vertices have not already been created in the datastore (and therefore still have null ids) |
|
@fabiencoppens |
|
@Incarnation-p-lee Here's what I suggest. As a first step, we could add the EDIT: We did a little bit of digging in neo4j's OGM, and we found the code where they handle this use case (vertex and edge creation in one transaction, where edges need the vertex ids to be created first). Here's the relevant class, line 63: https://github.com/neo4j/neo4j-ogm/blob/master/core/src/main/java/org/neo4j/ogm/session/request/RequestExecutor.java |
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
==========================================
- Coverage 90.22% 88.18% -2.04%
==========================================
Files 57 59 +2
Lines 1166 1261 +95
Branches 184 210 +26
==========================================
+ Hits 1052 1112 +60
- Misses 39 54 +15
- Partials 75 95 +20
Continue to review full report at Codecov.
|
|
@fabiencoppens |
|
@Incarnation-p-lee The annotation doesn't complicate anything, it's required in JPA, and a standard in ORMs and OGMs. See for example: |
|
@fabiencoppens |
|
@Incarnation-p-lee You do realize that spring data projects are initially inspired by JPA, right? The annotation is harmless as it is just an indication. Without an annotation, how do you indicate that the id field can be nullable at entity creation time? If we don't annotate, then we are back to removing the non-null constraint on the id field, which is what I had originally done. |
|
@fabiencoppens
We may need to talk about the detail implementation. |
|
@Incarnation-p-lee Those are excellent questions. In existing ORM/OGM frameworks like JPA and neo4j OGM, the |
|
@fabiencoppens For 2nd question, I prefer the generated Id when create entity. |
|
@Incarnation-p-lee This is still a work-in-progress. I'll be pushing a couple more pieces shortly. |
|
@fabiencoppens |
|
@fabiencoppens |
|
@fabiencoppens |
Strictly speaking, that's not really required, is it, if aliases are introduced, like: Never mind me if you are already working on this... |
|
@Incarnation-p-lee There is one small change that was missed in this PR, it's on the |
|
@fabiencoppens |
|
@fabiencoppens |
|
@Incarnation-p-lee I think I know what the problem is. In |
As I demonstrated above, you can pick "as"-aliases for each added vertex or edge without ID, ask the server to return both alias and assigned id, and just process them in whichever order they come. For this to work, you'd keep a Map of alias to each "id-less" instance as you write out each insert. I'd love to contribute, but I can't seem to make any of the integration test cases work against my local server, so I'm a little blocked... |
@jespersm What is blocking you with the ITs? What DB are you running them against? They rely on |
Ok, I got some green now, running against a fresh Gremlin Server with just the TinkerGraph (but with TLS on). But - I have 28 test errors in GremlinTemplateIT where you seem to have 5... I'm assuming you are running against Cosmos DB - I'll try that now.
I'm quite sure it does (at least with the serialization I tried, on Gremlin Server 3.3.3). I'm pretty sure it's also quite important for several of the more complex queries -- a bit like column aliases in SQL, I guess. |
The CI build runs the ITs against Tinkergraph, I believe. |
Oh, I see it now in the Travis files. I'll see if I get the same results if I use the same config files. Thanks. |
|
However, I see that you are seemingly hardcoding the textual representation of graph traversal and some of the GraphSON syntax into the library (and GraphSON 1.0 at that?), rather than using the API of the Client. |
That's a question for @Incarnation-p-lee . I'm just trying to contribute but have not participated in the project development so far. |
|
@jespersm |
|
@jespersm It is perfect if you can contribute this with PR, or I can do that later when I free, but it may take times. I file one issue for this. |
|
@fabiencoppens |
|
@fabiencoppens @Vertex("label-A")
Class A {
String name;
Long id;
};
@Vertex("label-A")
Class B {
String name;
Long id;
};For GraphRead, we may need to think about the type information about vertexes and edges when retrieve, is there any way to disable feature of no-required ID from this PR ? then we can merge this and working on the solution. |
|
@Incarnation-p-lee Yes this is a type erasure problem. When unmarshalling the results of a graph returned by the DB, we can't use reflection to determine the Java type of the vertices or edges, as the vertices or edges might have heterogeneous types, like in your ITs, where you have As far as this PR, it is really meant to make ids optional, which is very important for some of the Tinkerpop DBs such as JanusGraph. Otherwise, this PR is not very useful. One short-term workaround would be to rely on the labels, by making them mandatory (and unique, i.e. a one-to-one between label and Java type). At graph creation time, we would build a map of label to class, and use that to unmarshall the vertex and edge collections from the results. Thoughts? |
|
@fabiencoppens For aligning with gremlin syntax, configurable label is recommended I think. My idea for this problem is that add one |
|
@Incarnation-p-lee I Like your idea of an internal property that will contain the Vertex or Edge class's name. I think this needn't be exposed in the actual domain Java classes, but should be added to the entity's properties when it is saved to the DB, right? So it's a modification in the vertex and edge writers, which would add that internal property before the entity gets saved to the DB. And when we unmarshall a Graph, we'll get that property back for each instance in the vertices and edges, and de-serialize to that class. I like that idea. If we're ok on this, I'll get it done ASAP. |
|
@fabiencoppens |
|
@Incarnation-p-lee Changes made to incorporate the latest _classname internal property, and ITs are now green. Please review again and let me know. |
| GREMLIN_PRIMITIVE_GRAPH, // g | ||
| GREMLIN_PRIMITIVE_EDGE_ALL, // E() | ||
| GREMLIN_PRIMITIVE_GRAPH, // g | ||
| GREMLIN_PRIMITIVE_EDGE_ALL, // E() |
There was a problem hiding this comment.
please align the comments
| GREMLIN_PRIMITIVE_GRAPH, // g | ||
| GREMLIN_PRIMITIVE_VERTEX_ALL, // V() | ||
| GREMLIN_PRIMITIVE_GRAPH, // g | ||
| GREMLIN_PRIMITIVE_VERTEX_ALL, // V() |
There was a problem hiding this comment.
the same as above.
| @@ -62,7 +65,8 @@ public void write(@NonNull Object domain, @NonNull MappingGremlinConverter conve | |||
| } else if (field.getName().equals(GREMLIN_PROPERTY_CLASSNAME)) { | |||
| throw new GremlinEntityInformationException("Domain Cannot use pre-defined field name: " | |||
| + GREMLIN_PROPERTY_CLASSNAME); | |||
| } else if (field.getAnnotation(EdgeFrom.class) != null) { | |||
| } | |||
| else if (field.getAnnotation(EdgeFrom.class) != null) { | |||
There was a problem hiding this comment.
please put else after }
| try { | ||
| domainClass = Class.forName((String) source.getProperties().get(Constants.GREMLIN_PROPERTY_CLASSNAME)); | ||
| } | ||
| catch (ClassNotFoundException e) { |
|
@fabiencoppens |
|
@fabiencoppens |
Quick PR to allow ids to be optional when mutating the graph.