TINKERPOP-1730 Gremlin .NET: add support for GraphSON3 #710
Conversation
| .ToList<ISet<string>>(); | ||
| // "objects" is an object[] | ||
| object[] objectsProperty = reader.ToObject(graphsonObject["objects"]); | ||
| var objects = objectsProperty.ToArray(); |
There was a problem hiding this comment.
Isn't ToArray() redundant here when objectsProperty is already of type object[]?
| @@ -102,9 +102,9 @@ public void g_V_HasXname_markoX_ValueMap_Next() | |||
| var connection = _connectionFactory.CreateRemoteConnection(); | |||
| var g = graph.Traversal().WithRemote(connection); | |||
|
|
|||
| var receivedValueMap = g.V().Has("name", "marko").ValueMap<string, object>().Next(); | |||
| var receivedValueMap = g.V().Has("name", "marko").ValueMap<object, object>().Next(); | |||
There was a problem hiding this comment.
Shouldn't this also work when string is used as the generic type parameter?
There was a problem hiding this comment.
The serialization format for maps changed (see JIRA tickets 1427 and 1658), the result from this traversal was originally a js object:
{ "marko": { "@type": "g:Int32", "@value": 29 } }And changed in GraphSON3 to a type "g:Map" for which we use IDictionary<object, object>.
I've started a discussion on the mailing list regarding collections child types: https://lists.apache.org/thread.html/3918353aaa63aa07b69214da24fa7aa0760004227fc57fa2b3bcae86@%3Cdev.tinkerpop.apache.org%3E
There was a problem hiding this comment.
Ah ok, I saw the thread on the mailing list, but somehow I didn't draw the connection to this change.
| Assert.Equal(3, n["lop"]); | ||
| Assert.Equal(1, n["ripple"]); | ||
| var n = (IList<object>) t.SideEffects.Get("n"); | ||
| Assert.Equal(n.Select(tr => ((Traverser)tr).Object), new[] {"lop", "ripple"}); |
There was a problem hiding this comment.
Please switch the sides of the arguments here as the expected value should be the first argument and the actual value the second. Otherwise the messages for a failed test can become misleading.
|
|
||
| namespace Gremlin.Net.Driver.Messages | ||
| { | ||
| internal class ResponseResult<T> | ||
| { | ||
| [JsonProperty(PropertyName = "data")] | ||
| public List<T> Data { get; set; } | ||
| public JToken Data { get; set; } |
There was a problem hiding this comment.
You can completely remove the type parameter T from the class when you change the type here to JToken.
|
Thanks @FlorianHockmann for the feedback. About |
|
VOTE: +1 |
|
Unfortunately, you'll need to rebase because the Gremlin Server test server script moved out of the pom.xml, but your changes are easy enough. All tests pass with VOTE +1 |
asfgit
force-pushed
the
TINKERPOP-1730
branch
from
f9bfe3d
to
80f3dd3
Compare
|
Rebased onto master, tests pass with VOTE +1 I'll merge it once CI jobs finish. |
https://issues.apache.org/jira/browse/TINKERPOP-1730