You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2020/12/04 11:36:48 UTC
[GitHub] [tinkerpop] FlorianHockmann opened a new pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
FlorianHockmann opened a new pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369
https://issues.apache.org/jira/browse/TINKERPOP-2472
This prepares Gremlin.Net for GraphBinary by decoupling the serialization format from the driver. Similar to the Java driver, it is now possible to provide the driver with a `IMessageSerializer` which it will use to serialize request messages sent to the server and response messages received from the server. Included in this PR are implementations of that new interface for GraphSON 2 and 3.
We can then add another implementation for GraphBinary in a follow up PR.
Before this PR, serialization with GraphSON / JSON was tightly integrated in the driver which made it hard to support a different IO format.
NOTE: The driver now always deserializes a result completely, e.g.,skipping serialization like it was implemented in TINKERPOP-2067 is no longer supported. However, users who need this functionality can simply implement their own `IMessageSerializer` with this functionality which should be trivial to do. This should mainly affect the [_ExRam.Gremlinq_ library](https://github.com/ExRam/ExRam.Gremlinq).
@danielcweber: Would be great if you could take a short look at these changes to see whether they produce bigger problems for Exram.Gremlinq then I anticipated.
VOTE +1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
danielcweber commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-748145504
@FlorianHockmann I had a look into [TINKERPOP-1696](https://issues.apache.org/jira/browse/TINKERPOP-1696) and I don't think it's an issue per se that GraphSONReader exposes the native serialization type (JsonElement), but rather that it does so from the main library. Now that you decoupled IMessageSerializer, it should be straightforward to put all things System.Text.Json into its own library (e.g. Gremlin.Net.GraphSon). Not sure how it would work otherwise since JsonElement is also over all the IGraphSonDeserializer interface. Is having a separate library a viable option? Did I miss something? I could tackle the change if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
danielcweber commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-738845913
Great abstraction and highly appreciated. From how I understand this change, and assuming this PR builds on the recent move towards System.Text.Json, this change would allow me to easily stick to Newtonsoft.Json for the foreseeable future, wouldn't it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-747631334
the change you describe here makes sense to me. that abstraction seems like an important change.
@FlorianHockmann are there any documentation changes necessary for this particular change or is it largely transparent to the user when they upgrade? or perhaps the documentation really isn't necessary until we have GraphBinary working and there are multiple options for serialization?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
danielcweber commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-739781707
>you can stick to Newtonsoft.Json with this change if you implement your own IMessageSerializer
Very nice.
>We have this issue for quite some time (TINKERPOP-1696) and I haven't found a way yet to get rid of this.
One possible solution I could think of is to generically parametrize these interface on the native serialization type (in this case, JsonElement), making the json-library pluggable. I'm not sure where it'll all come together though., and I'm also not sure whether this scenario is already tackled by your recent change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann closed pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
FlorianHockmann closed pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber edited a comment on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
danielcweber edited a comment on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-748145504
@FlorianHockmann I had a look into [TINKERPOP-1696](https://issues.apache.org/jira/browse/TINKERPOP-1696) and I don't think it's an issue per se that GraphSONReader exposes the native serialization type (JsonElement), but rather that it does so from the main library. Now that you decoupled IMessageSerializer, it should be straightforward to put all things System.Text.Json into its own library (e.g. Gremlin.Net.GraphSon). Not sure how it would work otherwise since JsonElement is also all over the IGraphSonDeserializer interface + implementations. Is having a separate library a viable option? Did I miss something? I could tackle the change if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] jorgebay commented on a change in pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
jorgebay commented on a change in pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#discussion_r546612915
##########
File path: gremlin-dotnet/src/Gremlin.Net/Driver/GremlinClient.cs
##########
@@ -35,43 +35,126 @@ namespace Gremlin.Net.Driver
/// </summary>
public class GremlinClient : IGremlinClient
{
- /// <summary>
- /// Defines the default mime type to use.
- /// </summary>
- public const string DefaultMimeType = "application/vnd.gremlin-v3.0+json";
+ private readonly ConnectionPool _connectionPool;
/// <summary>
- /// The GraphSON2 mime type to use.
+ /// Initializes a new instance of the <see cref="GremlinClient" /> class for the specified Gremlin Server.
/// </summary>
- public const string GraphSON2MimeType = "application/vnd.gremlin-v2.0+json";
+ /// <param name="gremlinServer">The <see cref="GremlinServer" /> the requests should be sent to.</param>
+ /// <param name="graphSONReader">A <see cref="GraphSONReader" /> instance to read received GraphSON data.</param>
+ /// <param name="graphSONWriter">a <see cref="GraphSONWriter" /> instance to write GraphSON data.</param>
+ /// <param name="connectionPoolSettings">The <see cref="ConnectionPoolSettings" /> for the connection pool.</param>
+ /// <param name="webSocketConfiguration">
+ /// A delegate that will be invoked with the <see cref="ClientWebSocketOptions" />
+ /// object used to configure WebSocket connections.
+ /// </param>
+ /// <param name="sessionId">The session Id if Gremlin Client in session mode, defaults to null as session-less Client.</param>
+ [Obsolete("This constructor is obsolete. Use the constructor that takes a IMessageSerializer instead.")]
Review comment:
Good idea to mark it as deprecated and documenting the upgrade path.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-739757074
> From how I understand this change, and assuming this PR builds on the recent move towards System.Text.Json, this change would allow me to easily stick to Newtonsoft.Json for the foreseeable future, wouldn't it?
Yes, this targets `master` which is already on `System.Text.Json` which will be released with 3.5.0. And right, you can stick to `Newtonsoft.Json` with this change if you implement your own `IMessageSerializer`. You just have to implement especially the deserialization completely on your own as [the `GraphSONReader` still uses a type](https://github.com/apache/tinkerpop/blob/master/gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/GraphSONReader.cs#L103) specific to our JSON library (`JsonElement`). We have this issue for quite some time ([TINKERPOP-1696](https://issues.apache.org/jira/browse/TINKERPOP-1696)) and I haven't found a way yet to get rid of this.
But if you want to do the deserialization on your own, then this doesn't affect you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-748064793
It sounds as though there is no real impact to users and that the real impact would be GraphBinary. I think that you can save upgrade documentation for then.
VOTE +1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] danielcweber edited a comment on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
danielcweber edited a comment on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-738845913
@FlorianHockmann Great abstraction and highly appreciated. From how I understand this change, and assuming this PR builds on the recent move towards System.Text.Json, this change would allow me to easily stick to Newtonsoft.Json for the foreseeable future, wouldn't it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-748005380
> are there any documentation changes necessary for this particular change or is it largely transparent to the user when they upgrade?
It is transparent to users unless they want to use GraphSON 2 or if they for some reason want to skip deserialization completely, but apart from the ExRam.Gremlinq library I don't know why people would do that and the feature to skip deserialization was also never communicated openly.
Configuration for GraphSON 2 changed from this:
```c#
var client = new GremlinClient(new GremlinServer("localhost", 8182), new GraphSON2Reader(),
new GraphSON2Writer(), GremlinClient.GraphSON2MimeType);
```
to this:
```c#
var client = new GremlinClient(new GremlinServer("localhost", 8182), new GraphSON2MessageSerializer());
```
which can be seen in the `GremlinVariantsTests` that are automatically included in the reference docs. I think this is also a nice little improvement for the API as users now only need to change a single parameter to change the GraphSON version instead of three.
I didn't put that into the upgrade docs as the old `GremlinClient` constructor is still there. I just deprecated it and point users to the new constructor.
Should I still put this also in the upgrade docs? I'm always unsure what kind of change is significant enough to warrant an entry there as I also don't want to clutter it with information that is irrelevant for probably >99% of our users.
Adding GraphBinary will of course make it more important for users how they can change the serialization format, but I would then just add another part to the reference docs where we already explain how to configure the client for GraphSON 2. That would of course then also deserve a section in the upgrade docs, just like [when we added it for Python](https://tinkerpop.apache.org/docs/3.4.9-SNAPSHOT/upgrade/#_python_graphbinary).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on a change in pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#discussion_r536038574
##########
File path: gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/GraphSONMessageSerializer.cs
##########
@@ -0,0 +1,100 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+using System;
+using System.Collections.Generic;
+using System.Text;
+using System.Text.Json;
+using Gremlin.Net.Driver;
+using Gremlin.Net.Driver.Messages;
+
+namespace Gremlin.Net.Structure.IO.GraphSON
+{
+ /// <summary>
+ /// Serializes data to and from Gremlin Server in GraphSON format.
+ /// </summary>
+ public abstract class GraphSONMessageSerializer : IMessageSerializer
+ {
+ private static readonly JsonSerializerOptions JsonDeserializingOptions = new JsonSerializerOptions
+ {PropertyNamingPolicy = JsonNamingPolicy.CamelCase};
+ private readonly string _mimeType;
+ private readonly GraphSONReader _graphSONReader;
+ private readonly GraphSONWriter _graphSONWriter;
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="GraphSONMessageSerializer" /> class.
+ /// </summary>
+ /// <param name="mimeType">The MIME type supported by this serializer.</param>
+ /// <param name="graphSONReader">The <see cref="GraphSONReader"/> used to deserialize from GraphSON.</param>
+ /// <param name="graphSonWriter">The <see cref="GraphSONWriter"/> used to serialize to GraphSON.</param>
+ protected GraphSONMessageSerializer(string mimeType, GraphSONReader graphSONReader,
+ GraphSONWriter graphSonWriter)
+ {
+ _mimeType = mimeType;
+ _graphSONReader = graphSONReader;
+ _graphSONWriter = graphSonWriter;
+ }
+
+ /// <inheritdoc />
+ public virtual byte[] SerializeMessage(RequestMessage requestMessage)
+ {
+ var graphSONMessage = _graphSONWriter.WriteObject(requestMessage);
+ return Encoding.UTF8.GetBytes(MessageWithHeader(graphSONMessage));
+ }
+
+ private string MessageWithHeader(string messageContent)
+ {
+ return $"{(char) _mimeType.Length}{_mimeType}{messageContent}";
+ }
+
+ /// <inheritdoc />
+ public virtual ResponseMessage<List<object>> DeserializeMessage(byte[] message)
Review comment:
This is the method users need to override if they want to skip deserialization with our GraphSON reader to get the functionality from TINKERPOP-2067 back.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] FlorianHockmann commented on pull request #1369: TINKERPOP-2472 Decouple the driver from the IO format
Posted by GitBox <gi...@apache.org>.
FlorianHockmann commented on pull request #1369:
URL: https://github.com/apache/tinkerpop/pull/1369#issuecomment-748196014
I think @jorgebay's intention with TINKERPOP-1696 was to avoid any external types in a publicly facing API as it would make it much harder for users to update to a newer version of Gremlin.Net when we switch the JSON library if they have used these interfaces, e.g., because they have implemented their own serializers.
That problem is of course still there. But I also don't see a good solution for this. I at least couldn't get the approach to work that @jorgebay suggested in the ticket to use `dynamic`. I also wouldn't want to put the serialization logic in a separate package for this.
But overall this problem doesn't have a high priority in my opinion and it's also completely independent of this PR basically. If we want to further discuss how to tackle it, then I'd suggest that we move the discussion into the comment section of that ticket.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org