Repository: edgexfoundry/device-sdk-go
Pull request #397 (2019-11-07 16:46:02)

binary NewCommandValue consistency (fuji)
Duplicate of #346 to address #345 for Fuji release.


Patch:
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
index 232b6e96..4d75a293 100644
--- a/RELEASE-NOTES.txt
+++ b/RELEASE-NOTES.txt
@@ -39,5 +39,4 @@ Known issues:
 =======
 o ISSUE#261:  The OnChange option for AutoEvents is not effective on Binary data. So in the case where two successive Binary readings are identical, both will be sent to core-data regardless of the OnChange setting.
 o ISSUE#342:  --overwrite flag (re-upload text-based config to the registry) is not implemented.
-o ISSUE#345:  NewCommandValue function makes an unnecessary CBOR encoding.
 o ISSUE#354:  Query parameters whose names start with "ds-" will be allowed. They should not be and they will be blocked in future versions.
diff --git a/pkg/models/commandvalue.go b/pkg/models/commandvalue.go
index 537433b6..568fc6bb 100644
--- a/pkg/models/commandvalue.go
+++ b/pkg/models/commandvalue.go
@@ -8,7 +8,6 @@
 package models
 
 import (
-	"bufio"
 	"bytes"
 	"encoding/base64"
 	"encoding/binary"
@@ -18,7 +17,6 @@ import (
 	"strings"
 
 	contract "github.com/edgexfoundry/go-mod-core-contracts/models"
-	"github.com/ugorji/go/codec"
 )
 
 // ValueType indicates the type of value being passed back
@@ -228,7 +226,7 @@ func NewCommandValue(DeviceResourceName string, origin int64, value interface{},
 	switch t {
 	case Binary:
 		// assign cv.BinValue
-		err = encodeBinaryValue(cv, value)
+		cv.BinValue = value.([]byte)
 	case String:
 		cv.stringValue = value.(string)
 	default:
@@ -539,28 +537,7 @@ func (cv *CommandValue) Float64Value() (float64, error) {
 func (cv *CommandValue) BinaryValue() ([]byte, error) {
 	var value []byte
 	if cv.Type != Binary {
-		return value, fmt.Errorf("the CommandValue (%s) data type (%v) is not binary!", cv.String(), cv.Type)
+		return value, fmt.Errorf("the CommandValue (%s) data type (%v) is not binary", cv.String(), cv.Type)
 	}
-	err := decodeBinaryValue(bytes.NewReader(cv.BinValue), &value)
-	return value, err
-}
-
-func encodeBinaryValue(cv *CommandValue, value interface{}) error {
-	buf := new(bytes.Buffer)
-	hCbor := new(codec.CborHandle)
-	enc := codec.NewEncoder(buf, hCbor)
-	err := enc.Encode(value)
-	if err == nil {
-		cv.BinValue = buf.Bytes()
-	}
-	return err
-}
-
-func decodeBinaryValue(reader io.Reader, value interface{}) error {
-	// Provide a buffered reader for go-codec performance
-	var bufReader = bufio.NewReader(reader)
-	var h codec.Handle = new(codec.CborHandle)
-	var dec *codec.Decoder = codec.NewDecoder(bufReader, h)
-	var err error = dec.Decode(value)
-	return err
+	return cv.BinValue, nil
 }


Test patch:
diff --git a/pkg/models/commandvalue_test.go b/pkg/models/commandvalue_test.go
index 5888bced..b376c48b 100644
--- a/pkg/models/commandvalue_test.go
+++ b/pkg/models/commandvalue_test.go
@@ -21,6 +21,153 @@ import (
 	"github.com/ugorji/go/codec"
 )
 
+// Test NewCommandValue function
+func TestNewCommandValue(t *testing.T) {
+	// Test Bool
+	cv, err := NewCommandValue("resource", 0, true, Bool)
+	test, err2 := NewBoolValue("resource", 0, true)
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewBoolValue")
+	}
+
+	// Test int8
+	cv, err = NewCommandValue("resource", 0, int8(5), Int8)
+	test, err2 = NewInt8Value("resource", 0, int8(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewInt8Value")
+	}
+
+	// Test int16
+	cv, err = NewCommandValue("resource", 0, int16(5), Int16)
+	test, err2 = NewInt16Value("resource", 0, int16(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewInt16Value")
+	}
+
+	// Test int32
+	cv, err = NewCommandValue("resource", 0, int32(5), Int32)
+	test, err2 = NewInt32Value("resource", 0, int32(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewInt32Value")
+	}
+
+	// Test uint8
+	cv, err = NewCommandValue("resource", 0, uint8(5), Uint8)
+	test, err2 = NewUint8Value("resource", 0, uint8(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewUint8Value")
+	}
+
+	// Test uint16
+	cv, err = NewCommandValue("resource", 0, uint16(5), Uint16)
+	test, err2 = NewUint16Value("resource", 0, uint16(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewUint16Value")
+	}
+
+	// Test uint32
+	cv, err = NewCommandValue("resource", 0, uint32(5), Uint32)
+	test, err2 = NewUint32Value("resource", 0, uint32(5))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewInt8Value")
+	}
+
+	// Test float32
+	cv, err = NewCommandValue("resource", 0, float32(5.8), Float32)
+	test, err2 = NewFloat32Value("resource", 0, float32(5.8))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewFloat32Value")
+	}
+
+	// Test float64
+	cv, err = NewCommandValue("resource", 0, float64(5.8), Float64)
+	test, err2 = NewFloat64Value("resource", 0, float64(5.8))
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewFloat64Value")
+	}
+
+	// Test string
+	cv, err = NewCommandValue("resource", 0, "test value", String)
+	test = NewStringValue("resource", 0, "test value")
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewStringValue")
+	}
+
+	// Test binary
+	cv, err = NewCommandValue("resource", 0, []byte{1, 2, 3, 5, 8, 13}, Binary)
+	test, err2 = NewBinaryValue("resource", 0, []byte{1, 2, 3, 5, 8, 13})
+	if err != nil || err2 != nil {
+		t.Errorf("Error creating command value")
+	}
+
+	if !reflect.DeepEqual(cv, test) {
+		fmt.Println(cv)
+		fmt.Println(test)
+		t.Errorf("CommandValue returned from NewCommandValue doesn't match NewBinaryValue")
+	}
+
+}
+
 // Test NewBoolValue function.
 func TestNewBoolValue(t *testing.T) {
 	var value bool
@@ -599,19 +746,18 @@ func TestNewBinaryValue(t *testing.T) {
 	var origin int64 = time.Now().UnixNano()
 	// assign instance of mockStructB as a CBOR encoded CommandValue payload
 	var mock1 contract.Event
-	var mock2 contract.Event
 	mock1.Device = "Device01234567890"
 	mock1.Created = origin
 	mock1.ID = "MyStringIdentifier"
 	mock1.Modified = origin + 123
 	mock1.Pushed = 12345
-	// To extend coverage cborMock1 becomes encoded byte array.
+	// To extend coverage cborMock becomes encoded byte array.
 	// We will then confirm CommandValue particulars of binary payload are valid
-	cborMock1, err := encodeMockEvent(mock1)
+	cborMock, err := encodeMockEvent(mock1)
 	if err != nil {
 		t.Errorf("NewBinaryValue: Error encoding struct as binary value")
 	}
-	cv, errAssign := NewBinaryValue("resource", origin, cborMock1)
+	cv, errAssign := NewBinaryValue("resource", origin, cborMock)
 	if errAssign != nil {
 		t.Errorf("NewBinaryValue: Error invoking NewBinaryValue [%v]", errAssign)
 	}
@@ -622,28 +768,12 @@ func TestNewBinaryValue(t *testing.T) {
 	if cv.Origin != origin {
 		t.Errorf("Expected matching value! invalid Origin: %d != %d", cv.Origin, origin)
 	}
-	// Populate cv.BinValue as CBOR encoded CommandValue and decode back
-	err1 := encodeBinaryValue(cv, mock1)
-	if err1 != nil {
-		t.Errorf("NewBinaryValue: Error encoding binary value")
-	}
-	// decode the original encoded value, compare original raw struct to decoded mock2 and mock3
-	mock2 = contract.Event{}
-	err2 := decodeBinaryValue(bytes.NewReader(cborMock1), &mock2)
-	if err2 != nil {
-		t.Errorf("NewBinaryValue: Error decoding binary value")
-	}
-	if !reflect.DeepEqual(mock1, mock2) {
-		t.Errorf("NewBinaryValue: cv.BinValue: %v doesn't match value: %v", mock2, mock1)
-	}
-
-	mock3 := contract.Event{}
-	err3 := decodeBinaryValue(bytes.NewReader(cv.BinValue), &mock3)
-	if err3 != nil {
-		t.Errorf("NewBinaryValue: Error decoding binary value")
+	val, err := cv.BinaryValue()
+	if err != nil {
+		t.Errorf("BinaryValue: error retrieving binary value from command value")
 	}
-	if !reflect.DeepEqual(mock2, mock3) {
-		t.Errorf("NewBinaryValue: cv.BinValue: %v doesn't match value: %v", mock2, mock3)
+	if !reflect.DeepEqual(val, cborMock) {
+		t.Errorf("BinaryValue() result doesn't match expected payload")
 	}
 }