[GitHub] [tinkerpop] lyndonbauto commented on a diff in pull request #1648: Serialization refactoring

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

lyndonbauto commented on code in PR #1648:
URL: https://github.com/apache/tinkerpop/pull/1648#discussion_r870780609


##########
gremlin-go/driver/serializer.go:
##########
@@ -165,135 +173,147 @@ func uuidToBigInt(requestID uuid.UUID) big.Int {
 	return bigInt
 }
 
-func readUUID(buffer *bytes.Buffer) (uuid.UUID, error) {
-	var nullable byte
-	err := binary.Read(buffer, binary.LittleEndian, &nullable)
-	if err != nil {
-		return uuid.UUID{}, err
-	}
-	uuidBytes := make([]byte, 16)
-	err = binary.Read(buffer, binary.LittleEndian, uuidBytes)
-	if err != nil {
-		return uuid.UUID{}, err
-	}
-	return uuid.FromBytes(uuidBytes)
-}
-
-func readMap(buffer *bytes.Buffer, gs *graphBinarySerializer) (map[string]interface{}, error) {
-	var mapSize uint32
-	err := binary.Read(buffer, binary.BigEndian, &mapSize)
-	if err != nil {
-		return nil, err
-	}
-	var mapData = map[string]interface{}{}
-	for i := uint32(0); i < mapSize; i++ {
-		var keyType dataType
-		err = binary.Read(buffer, binary.BigEndian, &keyType)
-		if err != nil {
-			return nil, err
-		} else if keyType != stringType {
-			return nil, newError(err0703ReadMapNonStringKeyError)
-		}
-		var nullable byte
-		err = binary.Read(buffer, binary.BigEndian, &nullable)
-		if err != nil {
-			return nil, err
-		}
-		if nullable != 0 {
-			return nil, newError(err0701ReadMapNullKeyError)
-		}
-
-		k, err := readString(buffer)
-		if err != nil {
-			return nil, err
-		}
-		mapData[k], err = gs.ser.read(buffer)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return mapData, nil
-}
-
-func readString(buffer *bytes.Buffer) (string, error) {
-	var strLength uint32
-	err := binary.Read(buffer, binary.BigEndian, &strLength)
-	if err != nil {
-		return "", err
-	}
-
-	strBytes := make([]byte, strLength)
-	err = binary.Read(buffer, binary.BigEndian, strBytes)
-	if err != nil {
-		return "", err
-	}
-	return string(strBytes[:]), nil
-}
-
 // deserializeMessage deserializes a response message.
-func (gs graphBinarySerializer) deserializeMessage(responseMessage []byte) (response, error) {
+func (gs graphBinarySerializer) deserializeMessage(message []byte) (response, error) {
 	var msg response
-	buffer := bytes.Buffer{}
-	buffer.Write(responseMessage)
 
-	// Version
-	_, err := buffer.ReadByte()
-	if err != nil {
-		return msg, err
+	if message == nil || len(message) == 0 {
+		gs.ser.logHandler.log(Error, nullInput)
+		return msg, newError(err0405ReadValueInvalidNullInputError)
 	}
 
-	// Response uuid
-	msgUUID, err := readUUID(&buffer)
-	if err != nil {
-		return msg, err
-	}
+	initDeserializers()
 
-	// Status Code
-	var statusCode uint32
-	err = binary.Read(&buffer, binary.BigEndian, &statusCode)
+	// Skip version and nullable byte.
+	i := 2
+	id, err := readUuid(&message, &i)
 	if err != nil {
 		return msg, err
 	}
-	statusCode = statusCode & 0xFF
-
-	// Nullable Status message
-	var statusMessageNull byte
-	var statusMessage string
-	err = binary.Read(&buffer, binary.LittleEndian, &statusMessageNull)
-	if err != nil {
-		return msg, err
-	}
-	if statusMessageNull == 0 {
-		statusMessage, err = readString(&buffer)
+	msg.responseID = id.(uuid.UUID)
+	msg.responseStatus.code = uint16(readUint32Safe(&message, &i) & 0xFF)
+	isMessageValid := readByteSafe(&message, &i)
+	if isMessageValid == 0 {
+		message, err := readString(&message, &i)
 		if err != nil {
 			return msg, err
 		}
+		msg.responseStatus.message = message.(string)
 	}
-
-	// Status Attributes
-	statusAttributes, err := readMap(&buffer, &gs)
+	attr, err := readMapUnqualified(&message, &i)
 	if err != nil {
 		return msg, err
 	}
-
-	// Meta Attributes
-	metaAttributes, err := readMap(&buffer, &gs)
+	msg.responseStatus.attributes = attr.(map[string]interface{})
+	meta, err := readMapUnqualified(&message, &i)
 	if err != nil {
 		return msg, err
 	}
-
-	// Result data
-	data, err := gs.ser.read(&buffer)
+	msg.responseResult.meta = meta.(map[string]interface{})
+	msg.responseResult.data, err = readFullyQualifiedNullable(&message, &i, true)
 	if err != nil {
 		return msg, err
 	}
+	return msg, nil
+}
 
-	msg.responseID = msgUUID
-	msg.responseStatus.code = uint16(statusCode)
-	msg.responseStatus.message = statusMessage
-	msg.responseStatus.attributes = statusAttributes
-	msg.responseResult.meta = metaAttributes
-	msg.responseResult.data = data
+func initSerializers() {
+	// todo: lock or don't care?
+	// todo: lazy init or don't care?
+	if serializers == nil || len(serializers) == 0 {
+		serializers = map[dataType]writer{
+			bytecodeType:   bytecodeWriter,
+			stringType:     stringWriter,
+			bigIntegerType: bigIntWriter,
+			longType:       longWriter,
+			intType:        intWriter,
+			shortType:      shortWriter,
+			byteType: func(value interface{}, buffer *bytes.Buffer, typeSerializer *graphBinaryTypeSerializer) ([]byte, error) {
+				err := binary.Write(buffer, binary.BigEndian, value.(uint8))
+				return buffer.Bytes(), err
+			},
+			booleanType: func(value interface{}, buffer *bytes.Buffer, typeSerializer *graphBinaryTypeSerializer) ([]byte, error) {
+				err := binary.Write(buffer, binary.BigEndian, value.(bool))
+				return buffer.Bytes(), err
+			},
+			uuidType: func(value interface{}, buffer *bytes.Buffer, typeSerializer *graphBinaryTypeSerializer) ([]byte, error) {
+				err := binary.Write(buffer, binary.BigEndian, value)
+				return buffer.Bytes(), err
+			},
+			floatType: func(value interface{}, buffer *bytes.Buffer, typeSerializer *graphBinaryTypeSerializer) ([]byte, error) {
+				err := binary.Write(buffer, binary.BigEndian, value)
+				return buffer.Bytes(), err
+			},
+			doubleType: func(value interface{}, buffer *bytes.Buffer, typeSerializer *graphBinaryTypeSerializer) ([]byte, error) {
+				err := binary.Write(buffer, binary.BigEndian, value)
+				return buffer.Bytes(), err
+			},
+			vertexType:            vertexWriter,
+			edgeType:              edgeWriter,
+			propertyType:          propertyWriter,
+			vertexPropertyType:    vertexPropertyWriter,
+			lambdaType:            lambdaWriter,
+			traversalStrategyType: traversalStrategyWriter,
+			pathType:              pathWriter,
+			setType:               setWriter,
+			dateType:              timeWriter,
+			durationType:          durationWriter,
+			cardinalityType:       enumWriter,
+			columnType:            enumWriter,
+			directionType:         enumWriter,
+			operatorType:          enumWriter,
+			orderType:             enumWriter,
+			pickType:              enumWriter,
+			popType:               enumWriter,
+			tType:                 enumWriter,
+			barrierType:           enumWriter,
+			scopeType:             enumWriter,
+			pType:                 pWriter,
+			textPType:             textPWriter,
+			bindingType:           bindingWriter,
+			mapType:               mapWriter,
+			listType:              listWriter,
+		}
+	}
+}
 
-	return msg, nil
+func initDeserializers() {
+	// todo: lock or don't care?

Review Comment:
   Good question. 
   
   I think worst case we have a situation of overwriting the deserializers with an equivalent object, so effectively no change.
   
   I don't see a need to lock.



-- 
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