divijvaidya commented on a change in pull request #1555:
URL: https://github.com/apache/tinkerpop/pull/1555#discussion_r796027098
##########
File path: gremlin-test/features/sideEffect/Group.feature
##########
@@ -264,17 +264,17 @@ Feature: Step - group()
| m[{"software":"d[2.0].d", "person":"d[5.0].d"}] |
# The post-ordering really isn't really right but works around TINKERPOP-2600
- Scenario:
g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
- Given the modern graph
- And using the parameter xx1 defined as "m[{\"marko\":[\"666\"],
\"noone\":[\"blah\"]}]"
- And the traversal of
- """
- g.withSideEffect("a",
xx1).V().group("a").by("name").by(__.outE().label().fold()).cap("a").unfold().group().by(keys).by(select(values).order(Scope.local).by(Order.asc))
- """
- When iterated to list
- Then the result should be unordered
- | result |
- | m[{"ripple":[], "peter":["created"], "noone":["blah"], "vadas":[],
"josh":["created", "created"], "lop":[], "marko":["666", "created", "knows",
"knows"]}] |
+# Scenario:
g_withSideEffectXa__marko_666_noone_blahX_V_groupXaX_byXnameX_byXoutE_label_foldX_capXaX
Review comment:
please uncomment
##########
File path:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/reference/ReferenceVertex.java
##########
@@ -38,6 +38,10 @@ private ReferenceVertex() {
}
+ public ReferenceVertex(final Object id) {
Review comment:
An entity used for serialization such as ReferenceVertex should just be
a representation for underlying stored data. Such entities should not "assume"
or use defaults to fill up missing data entries.
If we are saying that a Reference Vertex cannot exist without both an ID and
a label present, then this constructor should not exit.
##########
File path:
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WsGremlinBinaryRequestDecoder.java
##########
@@ -76,6 +76,7 @@ protected void decode(final ChannelHandlerContext
channelHandlerContext, final B
try {
objects.add(serializer.deserializeRequest(messageBytes.discardReadBytes()));
} catch (SerializationException se) {
+ logger.warn(se.getMessage());
Review comment:
Message could contain PII so I am am apprehensive about logging it. It's
also not possible to sanitize it since the serialization itself has failed. Why
do we want to log the message in case of serialization error? wouldn't stack
trace be sufficient to figure out where the serialization failed? If not, I
would advocate for better stack trace from serialization instead of logging
here.
same comment for other place where we are logging the message itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
