[GitHub] [tinkerpop] divijvaidya commented on a change in pull request #1547: Add GraphBinary serialization support to gremlin-javascript

[GitBox <gi...@apache.org>]

divijvaidya commented on a change in pull request #1547:
URL: https://github.com/apache/tinkerpop/pull/1547#discussion_r784819118



##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/graphbinary-test.js
##########
@@ -0,0 +1,119 @@
+/*
+ *  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.
+ */
+
+/**
+ * @author Igor Ostapenko
+ */
+'use strict';
+
+const assert = require('assert');

Review comment:
       I know this PR is work in progress at this stage, so please consider the following comment as proactive caution.
   
   I would suggest adding a "round-tripping" test which serializes the object, deserializes the object and compares with the original version. For completeness of test coverage, we would want to add the following tests:
   
   1. Test for each data type defined at https://tinkerpop.apache.org/docs/3.5.1/dev/io/#_data_type_formats 
   2. For each data type in point 1 above, test for happy case and edge case e.g. null handling, unicode chars for string, +=inf/NaN/+-0.0 for numbers etc.
   3. Negative test cases where we expect serializer to throw an error e.g. if `parent` for vertex property is not `null` as defined in https://tinkerpop.apache.org/docs/3.5.1/dev/io/#_vertexproperty_4 , out of bound value for a number etc.

##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/graph-serializer.js
##########
@@ -98,6 +99,47 @@ class GraphSON2Writer {
   write(obj) {
     return JSON.stringify(this.adaptObject(obj));
   }
+

Review comment:
       I would suggest to create a separate PR for GraphSonV2 changes. It would simplify the GraphBinary PR and also possibly merge the graphsonV2 earlier than this much larger change. 
   

##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js
##########
@@ -40,6 +42,7 @@ const responseStatusCode = {
 
 const defaultMimeType = 'application/vnd.gremlin-v3.0+json';
 const graphSON2MimeType = 'application/vnd.gremlin-v2.0+json';
+const graphBinaryMimeType = 'application/vnd.graphbinary-v1.0';

Review comment:
       Note that graphBinary has two MimeTypes. `application/vnd.graphbinary-v1.0` and `application/vnd.graphbinary-v1.0-stringd`




-- 
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: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org