Repository: gocql/gocql
Pull request #1458 (2020-06-15 15:46:03)

Fix tuple marshaling
Hello,

this PR fixes the marshaling of null values in tuples.

Right now for a null value gocql writes a length of `0` but a null value should be encoded as a `-1` [as per the CQL spec](https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v4.spec#L208-L209).

Writing `0` breaks cqlsh sometimes:
```
Traceback (most recent call last):
  File "/home/vincent/dev/Batch/Ops/cassandra/bin/cqlsh.py", line 1050, in perform_simple_statement
    result = future.result()
  File "/home/vincent/dev/Batch/Ops/cassandra/bin/../lib/cassandra-driver-internal-only-3.11.0-bb96859b.zip/cassandra-driver-3.11.0-bb96859b/cassandra/cluster.py", line 3925, in result
    raise self._final_exception
DriverException: Failed decoding result column "prev_generation_id" of type set<frozen<tuple<timeuuid, timestamp>>>: can't compare datetime.datetime to EmptyValue
```

And the datastax java driver:
```
Exception in thread "main" java.lang.NullPointerException
	at com.datastax.driver.core.AbstractAddressableByIndexData.hashCode(AbstractAddressableByIndexData.java:288)
	at com.datastax.driver.core.TupleValue.hashCode(TupleValue.java:69)
	at java.base/java.util.HashMap.hash(HashMap.java:339)
	at java.base/java.util.HashMap.put(HashMap.java:612)
	at java.base/java.util.HashSet.add(HashSet.java:220)
	at com.datastax.driver.core.TypeCodec$AbstractCollectionCodec.deserialize(TypeCodec.java:1804)
	at com.datastax.driver.core.TypeCodec$AbstractCollectionCodec.deserialize(TypeCodec.java:1756)
	at com.datastax.driver.core.AbstractGettableByIndexData.getSet(AbstractGettableByIndexData.java:269)
	at com.datastax.driver.core.AbstractGettableData.getSet(AbstractGettableData.java:29)
	at com.datastax.driver.core.AbstractGettableByIndexData.getSet(AbstractGettableByIndexData.java:260)
	at com.datastax.driver.core.AbstractGettableData.getSet(AbstractGettableData.java:29)
	at com.batch.testguava.App.main(App.java:87)
```

gocql can unmarshal these values correctly, so it's possible the python and java driver are also buggy.

I also added tests which cover most of the code in `marshalTuple`.

Here is a [gist](https://gist.github.com/vrischmann/df074f124121970f5918c3879a017038) with the Go code to write a tuple and the Java code which exhibits the exception when unmarshaling that tuple data.
That code doesn't break cqlsh, I'm not sure why yet.


Patch:
diff --git a/marshal.go b/marshal.go
index dd1d5e6..e95c1c8 100644
--- a/marshal.go
+++ b/marshal.go
@@ -1839,6 +1839,11 @@ func marshalTuple(info TypeInfo, value interface{}) ([]byte, error) {
 
 		var buf []byte
 		for i, elem := range v {
+			if elem == nil {
+				buf = appendInt(buf, int32(-1))
+				continue
+			}
+
 			data, err := Marshal(tuple.Elems[i], elem)
 			if err != nil {
 				return nil, err
@@ -1864,7 +1869,14 @@ func marshalTuple(info TypeInfo, value interface{}) ([]byte, error) {
 
 		var buf []byte
 		for i, elem := range tuple.Elems {
-			data, err := Marshal(elem, rv.Field(i).Interface())
+			field := rv.Field(i)
+
+			if field.Kind() == reflect.Ptr && field.IsNil() {
+				buf = appendInt(buf, int32(-1))
+				continue
+			}
+
+			data, err := Marshal(elem, field.Interface())
 			if err != nil {
 				return nil, err
 			}
@@ -1883,7 +1895,14 @@ func marshalTuple(info TypeInfo, value interface{}) ([]byte, error) {
 
 		var buf []byte
 		for i, elem := range tuple.Elems {
-			data, err := Marshal(elem, rv.Index(i).Interface())
+			item := rv.Index(i)
+
+			if item.Kind() == reflect.Ptr && item.IsNil() {
+				buf = appendInt(buf, int32(-1))
+				continue
+			}
+
+			data, err := Marshal(elem, item.Interface())
 			if err != nil {
 				return nil, err
 			}


Test patch:
diff --git a/marshal_test.go b/marshal_test.go
index 5809d3e..8410018 100644
--- a/marshal_test.go
+++ b/marshal_test.go
@@ -1708,31 +1708,139 @@ func TestMarshalTuple(t *testing.T) {
 		},
 	}
 
-	expectedData := []byte("\x00\x00\x00\x03foo\x00\x00\x00\x03bar")
-	value := []interface{}{"foo", "bar"}
-
-	data, err := Marshal(info, value)
-	if err != nil {
-		t.Errorf("marshalTest: %v", err)
-		return
+	stringToPtr := func(s string) *string { return &s }
+	checkString := func(t *testing.T, exp string, got string) {
+		if got != exp {
+			t.Errorf("expected string to be %v, got %v", exp, got)
+		}
 	}
 
-	if !bytes.Equal(data, expectedData) {
-		t.Errorf("marshalTest: expected %x (%v), got %x (%v)",
-			expectedData, decBigInt(expectedData), data, decBigInt(data))
-		return
+	type tupleStruct struct {
+		A string
+		B *string
 	}
-
-	var s1, s2 string
-	val := []interface{}{&s1, &s2}
-	err = Unmarshal(info, expectedData, val)
-	if err != nil {
-		t.Errorf("unmarshalTest: %v", err)
-		return
+	var (
+		s1 *string
+		s2 *string
+	)
+
+	testCases := []struct {
+		name       string
+		expected   []byte
+		value      interface{}
+		checkValue interface{}
+		check      func(*testing.T, interface{})
+	}{
+		{
+			name:       "interface-slice:two-strings",
+			expected:   []byte("\x00\x00\x00\x03foo\x00\x00\x00\x03bar"),
+			value:      []interface{}{"foo", "bar"},
+			checkValue: []interface{}{&s1, &s2},
+			check: func(t *testing.T, v interface{}) {
+				checkString(t, "foo", *s1)
+				checkString(t, "bar", *s2)
+			},
+		},
+		{
+			name:       "interface-slice:one-string-one-nil-string",
+			expected:   []byte("\x00\x00\x00\x03foo\xff\xff\xff\xff"),
+			value:      []interface{}{"foo", nil},
+			checkValue: []interface{}{&s1, &s2},
+			check: func(t *testing.T, v interface{}) {
+				checkString(t, "foo", *s1)
+				if s2 != nil {
+					t.Errorf("expected string to be nil, got %v", *s2)
+				}
+			},
+		},
+		{
+			name:     "struct:two-strings",
+			expected: []byte("\x00\x00\x00\x03foo\x00\x00\x00\x03bar"),
+			value: tupleStruct{
+				A: "foo",
+				B: stringToPtr("bar"),
+			},
+			checkValue: &tupleStruct{},
+			check: func(t *testing.T, v interface{}) {
+				got := v.(*tupleStruct)
+				if got.A != "foo" {
+					t.Errorf("expected A string to be %v, got %v", "foo", got.A)
+				}
+				if got.B == nil {
+					t.Errorf("expected B string to be %v, got nil", "bar")
+				}
+				if *got.B != "bar" {
+					t.Errorf("expected B string to be %v, got %v", "bar", got.B)
+				}
+			},
+		},
+		{
+			name:       "struct:one-string-one-nil-string",
+			expected:   []byte("\x00\x00\x00\x03foo\xff\xff\xff\xff"),
+			value:      tupleStruct{A: "foo", B: nil},
+			checkValue: &tupleStruct{},
+			check: func(t *testing.T, v interface{}) {
+				got := v.(*tupleStruct)
+				if got.A != "foo" {
+					t.Errorf("expected A string to be %v, got %v", "foo", got.A)
+				}
+				if *got.B != "" {
+					t.Errorf("expected B string to be empty, got %v", *got.B)
+				}
+			},
+		},
+		{
+			name:     "arrayslice:two-strings",
+			expected: []byte("\x00\x00\x00\x03foo\x00\x00\x00\x03bar"),
+			value: [2]*string{
+				stringToPtr("foo"),
+				stringToPtr("bar"),
+			},
+			checkValue: &[2]*string{},
+			check: func(t *testing.T, v interface{}) {
+				got := v.(*[2]*string)
+				checkString(t, "foo", *(got[0]))
+				checkString(t, "bar", *(got[1]))
+			},
+		},
+		{
+			name:     "arrayslice:one-string-one-nil-string",
+			expected: []byte("\x00\x00\x00\x03foo\xff\xff\xff\xff"),
+			value: [2]*string{
+				stringToPtr("foo"),
+				nil,
+			},
+			checkValue: &[2]*string{},
+			check: func(t *testing.T, v interface{}) {
+				got := v.(*[2]*string)
+				checkString(t, "foo", *(got[0]))
+				checkString(t, "", *(got[1]))
+			},
+		},
 	}
 
-	if s1 != "foo" || s2 != "bar" {
-		t.Errorf("unmarshalTest: expected [foo, bar], got [%s, %s]", s1, s2)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			data, err := Marshal(info, tc.value)
+			if err != nil {
+				t.Errorf("marshalTest: %v", err)
+				return
+			}
+
+			if !bytes.Equal(data, tc.expected) {
+				t.Errorf("marshalTest: expected %x (%v), got %x (%v)",
+					tc.expected, decBigInt(tc.expected), data, decBigInt(data))
+				return
+			}
+
+			err = Unmarshal(info, data, tc.checkValue)
+			if err != nil {
+				t.Errorf("unmarshalTest: %v", err)
+				return
+			}
+
+			tc.check(t, tc.checkValue)
+		})
 	}
 }