Make put of nil maps (and a few other cases) work pre-VOM2
This involves more tests of the field package, a rewrite of deep equals, a small hack to make slice settable (which will go away for VOM2) and a few other changes.
Change-Id: Ie2048a934570caa0b823c3d0cb149b0c5fc13fdd
diff --git a/services/store/memstore/field/reflect.go b/services/store/memstore/field/reflect.go
index c12f667..5a76e29 100644
--- a/services/store/memstore/field/reflect.go
+++ b/services/store/memstore/field/reflect.go
@@ -48,6 +48,10 @@
SetAsID
)
+const (
+ SliceAppendSuffix = "@"
+)
+
var (
nullID storage.ID
nullValue reflect.Value
@@ -131,13 +135,13 @@
//
// Here are the possible cases:
//
-// 1. setFieldFailed if the operation failed because the value has the wrong type
+// 1. SetFailed if the operation failed because the value has the wrong type
// or the path doesn't exist.
//
-// 2. setFieldAsValue if the operation was successful, and the value xval was
+// 2. SetAsValue if the operation was successful, and the value xval was
// stored. The returned storage.ID is null.
//
-// 3. setFieldAsId if the operation was successful, but the type of the field is
+// 3. SetAsId if the operation was successful, but the type of the field is
// storage.ID and xval does not have type storage.ID. In this case, the value
// xval is not stored; the storage.ID is returned instead. If the field does
// not already exist, a new storage.ID is created (and returned).
@@ -157,8 +161,10 @@
return setMapField(v, name, xval)
case reflect.Array, reflect.Slice:
return setSliceField(v, name, xval)
+ case reflect.Struct:
+ return setStructField(v, name, xval)
default:
- return setRegularField(v, name, xval)
+ return SetFailed, nullID
}
}
@@ -168,9 +174,6 @@
if tyKey.Kind() != reflect.String {
return SetFailed, nullID
}
- if v.IsNil() {
- v.Set(reflect.MakeMap(tyV))
- }
key := reflect.ValueOf(name).Convert(tyKey)
r, x, id := coerceValue(tyV.Elem(), v.MapIndex(key), xval)
if r == SetFailed {
@@ -181,11 +184,13 @@
}
func setSliceField(v reflect.Value, field string, xval interface{}) (SetResult, storage.ID) {
- if field == "@" {
+ if field == SliceAppendSuffix {
r, x, id := coerceValue(v.Type().Elem(), nullValue, xval)
if r == SetFailed {
return SetFailed, nullID
}
+ // This can panic if v is not settable. It is a requirement that users of this method
+ // ensure that it is settable.
v.Set(reflect.Append(v, x))
return r, id
}
@@ -202,16 +207,17 @@
return r, id
}
-func setRegularField(v reflect.Value, name string, xval interface{}) (SetResult, storage.ID) {
- v = findNextField(v, name)
- if !v.CanSet() {
+func setStructField(v reflect.Value, name string, xval interface{}) (SetResult, storage.ID) {
+ field, found := v.Type().FieldByName(name)
+ if !found {
return SetFailed, nullID
}
- r, x, id := coerceValue(v.Type(), v, xval)
+ fieldVal := v.FieldByName(name)
+ r, x, id := coerceValue(field.Type, fieldVal, xval)
if r == SetFailed {
return SetFailed, nullID
}
- v.Set(x)
+ fieldVal.Set(x)
return r, id
}
diff --git a/services/store/memstore/field/reflect_test.go b/services/store/memstore/field/reflect_test.go
index 517bff7..5c85253 100644
--- a/services/store/memstore/field/reflect_test.go
+++ b/services/store/memstore/field/reflect_test.go
@@ -129,7 +129,132 @@
}
}
-func TestSetField(t *testing.T) {
+func TestSetSliceField(t *testing.T) {
+ v := &[]string{"a", "b", "c"}
+ rv := reflect.ValueOf(v)
+
+ // Test simple get and set.
+ b, _ := field.Get(v, storage.PathName{"1"})
+ if "b" != b.Interface() {
+ t.Errorf(`Expected "b", got %v`, b.Interface())
+ }
+ if ok, _ := field.Set(rv, "1", "other"); ok != field.SetAsValue {
+ t.Errorf("field.Set failed on slice: %v", ok)
+ }
+ b, _ = field.Get(v, storage.PathName{"1"})
+ if "other" != b.Interface() {
+ t.Errorf(`Expected "a", got %v`, b.Interface())
+ }
+
+ // Test get on a non-existant field.
+ ne, _ := field.Get(v, storage.PathName{"4"})
+ if ne.Kind() != reflect.Slice {
+ t.Errorf("Expected to get a top level slice, got: %v", ne.Interface())
+ }
+ ne, _ = field.Get(v, storage.PathName{"-1"})
+ if ne.Kind() != reflect.Slice {
+ t.Errorf("Expected to get a top level slice, got: %v", ne.Interface())
+ }
+ nepath := storage.PathName{"X"}
+ ne, s := field.Get(v, nepath)
+ if ne.Kind() != reflect.Slice {
+ t.Errorf("Expected to get a top level slice, got: %v", ne.Interface())
+ }
+ if !reflect.DeepEqual(s, nepath) {
+ t.Errorf("Expected path %v, got %v.", nepath, s)
+ }
+
+ // Test adding a value.
+ if ok, _ := field.Set(rv, "@", "AppendedVal"); ok != field.SetAsValue {
+ t.Errorf("Expected to succeed in appending value: %v", ok)
+ }
+ appended, _ := field.Get(v, storage.PathName{"3"})
+ if "AppendedVal" != appended.Interface() {
+ t.Errorf(`Expected "AppendedVal", got %v`, appended.Interface())
+ }
+
+ // Test set of an incompatible value fails.
+ if ok, _ := field.Set(rv, "1", true); ok == field.SetAsValue {
+ t.Errorf("Expected field.Set to fail when an incompatible value is being set.")
+ }
+}
+
+func TestSetEmptySliceField(t *testing.T) {
+ v := &[]string{}
+ rv := reflect.ValueOf(v)
+
+ ne, _ := field.Get(v, storage.PathName{"0"})
+ if ne.Kind() != reflect.Slice {
+ t.Errorf("Expected to get a top level slice, got: %v", ne.Interface())
+ }
+ if ok, _ := field.Set(rv, "0", "a"); ok == field.SetAsValue {
+ t.Errorf("Expected field.Set to fail")
+ }
+}
+
+func TestSetMapField(t *testing.T) {
+ v := &map[string]string{
+ "A": "a",
+ "B": "b",
+ }
+ rv := reflect.ValueOf(v)
+
+ // Test simple get and set.
+ a, _ := field.Get(v, storage.PathName{"A"})
+ if "a" != a.Interface() {
+ t.Errorf(`Expected "a", got %v`, a.Interface())
+ }
+ if ok, _ := field.Set(rv, "A", "other"); ok != field.SetAsValue {
+ t.Errorf("field.Set failed on map: %v", ok)
+ }
+ a, _ = field.Get(v, storage.PathName{"A"})
+ if "other" != a.Interface() {
+ t.Errorf(`Expected "a", got %v`, a.Interface())
+ }
+
+ // Test get on a non-existant field.
+ nepath := storage.PathName{"NonExistant"}
+ ne, s := field.Get(v, nepath)
+ if !reflect.DeepEqual(s, nepath) {
+ t.Errorf("Expected path %v, got %v.", nepath, s)
+ }
+ if ne.Kind() != reflect.Map {
+ t.Errorf("Expected to get a top level map, got: %v", ne.Interface())
+ }
+
+ // Test that set on a non-existant field adds the field.
+ if ok, _ := field.Set(rv, "C", "c"); ok != field.SetAsValue {
+ t.Errorf("Expected field.Set to succeed: %v", ok)
+ }
+ c, _ := field.Get(v, storage.PathName{"C"})
+ if "c" != c.Interface() {
+ t.Errorf(`Expected "c", got %v`, c.Interface())
+ }
+
+ // Test set of an incompatible value fails.
+ if ok, _ := field.Set(rv, "A", true); ok == field.SetAsValue {
+ t.Errorf("Expected field.Set to fail when an incompatible value is being set.")
+ }
+}
+
+func TestSetEmptyMapField(t *testing.T) {
+ v := &map[string]interface{}{}
+ rv := reflect.ValueOf(v)
+
+ ne, _ := field.Get(v, storage.PathName{"A"})
+ if ne.Kind() != reflect.Map {
+ t.Errorf("Expected to get a top level map, got: %v", ne.Interface())
+ }
+ if ok, _ := field.Set(rv, "A", "a"); ok != field.SetAsValue {
+ t.Errorf("Expected field.Set to succeed: %v", ok)
+ }
+ a, _ := field.Get(v, storage.PathName{"A"})
+ if "a" != a.Interface() {
+ t.Errorf(`Expected "a", got %v`, a.Interface())
+ }
+}
+
+func TestSetStructField(t *testing.T) {
a := &A{
B: 5,
C: []int{6, 7},
diff --git a/services/store/memstore/state/mutable_snapshot.go b/services/store/memstore/state/mutable_snapshot.go
index c95d9cd..7b7c675 100644
--- a/services/store/memstore/state/mutable_snapshot.go
+++ b/services/store/memstore/state/mutable_snapshot.go
@@ -400,7 +400,7 @@
return sn.putTagsValue(checker, path, suffix[1:], c, v)
}
value := deepcopy(c.Value)
- p, s := field.Get(value, suffix)
+ p, s := field.Get(makeInnerReference(value), suffix)
if len(s) != 0 {
return nil, ErrNotFound
}
diff --git a/services/store/memstore/state/mutable_snapshot_test.go b/services/store/memstore/state/mutable_snapshot_test.go
index abeea27..62ace5d 100644
--- a/services/store/memstore/state/mutable_snapshot_test.go
+++ b/services/store/memstore/state/mutable_snapshot_test.go
@@ -31,6 +31,7 @@
stat, err := sn.Put(rootPublicID, storage.ParsePath(path), dir)
if err != nil || stat == nil {
t.Errorf("%s(%d): mkdir %s: %s", file, line, path, err)
+ return storage.ID{}, dir
}
m, ok := sn.mutations.Delta[stat.ID]
if !ok {
@@ -264,3 +265,27 @@
expectNotExists(t, sn, id5)
checkInRefs(t, sn)
}
+
+// Tests that nil maps are converted to empty maps.
+func TestPutToNilMap(t *testing.T) {
+ sn := newMutableSnapshot(rootPublicID)
+
+ var m map[string]interface{}
+ if _, err := sn.Put(rootPublicID, storage.PathName{}, m); err != nil {
+ t.Error("failure during nil map put: ", err)
+ }
+ if _, err := sn.Put(rootPublicID, storage.PathName{"z"}, "z"); err != nil {
+ t.Error("failure during put of child of nil map: ", err)
+ }
+}
+
+// Tests that slices are settable so that we can append.
+func TestAppendToSlice(t *testing.T) {
+ sn := newMutableSnapshot(rootPublicID)
+ if _, err := sn.Put(rootPublicID, storage.ParsePath("/"), []int{}); err != nil {
+ t.Error("failure during put of empty slice: ", err)
+ }
+ if _, err := sn.Put(rootPublicID, storage.ParsePath("/@"), 1); err != nil {
+ t.Error("failure during append to slice: ", err)
+ }
+}
diff --git a/services/store/memstore/state/util.go b/services/store/memstore/state/util.go
index 42e07fe..cb72db1 100644
--- a/services/store/memstore/state/util.go
+++ b/services/store/memstore/state/util.go
@@ -7,22 +7,81 @@
"sort"
"veyron2/storage"
- "veyron2/vom"
)
+// This function returns an interface with a pointer to the element in val.
+// Note that this is different from returning &val which returns a pointer to
+// val. This is required temporarily before we switch to VOM value so that
+// we can append to slices.
+// TODO(bprosnitz) This is hacky -- remove this when we switch to VOM Value
+func makeInnerReference(val interface{}) interface{} {
+ rv := reflect.ValueOf(val)
+ ptr := reflect.New(rv.Type())
+ ptr.Elem().Set(rv)
+ return ptr.Interface()
+}
+
+func deepcopyReflect(rv reflect.Value) reflect.Value {
+ switch rv.Kind() {
+ case reflect.Array:
+ arr := reflect.New(rv.Type()).Elem()
+ for i := 0; i < rv.Len(); i++ {
+ valcopy := deepcopyReflect(rv.Index(i))
+ arr.Index(i).Set(valcopy)
+ }
+ return arr
+ case reflect.Slice:
+ s := reflect.MakeSlice(rv.Type(), rv.Len(), rv.Cap())
+ for i := 0; i < rv.Len(); i++ {
+ valcopy := deepcopyReflect(rv.Index(i))
+ s.Index(i).Set(valcopy)
+ }
+ ptr := reflect.New(rv.Type())
+ ptr.Elem().Set(s)
+ return ptr.Elem()
+ case reflect.Map:
+ m := reflect.MakeMap(rv.Type())
+ keys := rv.MapKeys()
+ for _, key := range keys {
+ val := rv.MapIndex(key)
+ keycopy := deepcopyReflect(key)
+ valcopy := deepcopyReflect(val)
+ m.SetMapIndex(keycopy, valcopy)
+ }
+ return m
+ case reflect.Struct:
+ s := reflect.New(rv.Type()).Elem()
+ for i := 0; i < rv.NumField(); i++ {
+ valcopy := deepcopyReflect(rv.Field(i))
+ s.Field(i).Set(valcopy)
+ }
+ return s
+ case reflect.Ptr:
+ ptr := reflect.New(rv.Type()).Elem()
+ elem := reflect.New(rv.Type().Elem())
+ ptr.Set(elem)
+ ptr.Elem().Set(deepcopyReflect(rv.Elem()))
+ return ptr
+ case reflect.Interface:
+ intr := reflect.New(rv.Type()).Elem()
+ intr.Set(deepcopyReflect(rv.Elem()))
+ return intr
+ case reflect.Chan, reflect.Func, reflect.UnsafePointer:
+ panic(fmt.Sprintf("deepcopy of kind %v not supported", rv.Kind()))
+ default:
+ // Primitives (copy it so we can't set the original)
+ return reflect.ValueOf(rv.Interface())
+ }
+}
+
// deepcopy performs a deep copy of a value. We need this to simulate secondary
// storage where each time a value is stored, it is copied to secondary storage;
// and when it is retrieved, it is copied out of secondary storage.
func deepcopy(v interface{}) interface{} {
- var buf bytes.Buffer
- if err := vom.NewEncoder(&buf).Encode(v); err != nil {
- panic(fmt.Sprintf("Can't encode %v: %s", v, err))
+ if v == nil {
+ return nil
}
- cp := reflect.New(reflect.TypeOf(v)).Elem().Interface()
- if err := vom.NewDecoder(&buf).Decode(&cp); err != nil {
- panic(fmt.Sprintf("Can't decode %v: %s", v, err))
- }
- return cp
+ return deepcopyReflect(reflect.ValueOf(v)).Interface()
}
// addIDToSort adds a storage.ID to a sorted array.
diff --git a/services/store/memstore/state/util_test.go b/services/store/memstore/state/util_test.go
index 25fca8a..38653ef 100644
--- a/services/store/memstore/state/util_test.go
+++ b/services/store/memstore/state/util_test.go
@@ -6,6 +6,7 @@
package state
import (
+ "reflect"
"runtime"
"testing"
@@ -50,3 +51,66 @@
t.Errorf("%s(%d): can't remove %s: %s", file, line, path, err)
}
}
+
+func TestDeepcopy(t *testing.T) {
+ type basicTestStruct struct {
+ X int
+ }
+ type compositeHoldingTestStruct struct {
+ A [2]int
+ Sl []string
+ M map[string]int
+ St basicTestStruct
+ }
+
+ var intr interface{} = []string{"X"}
+
+ tests := []interface{}{
+ nil,
+ 0,
+ true,
+ "str",
+ [3]int{4, 5, 6},
+ []string{"A", "B"},
+ map[string]int{"A": 4, "B": 3},
+ basicTestStruct{7},
+ &basicTestStruct{7},
+ compositeHoldingTestStruct{
+ [2]int{3, 4},
+ []string{"A"},
+ map[string]int{"A": 5},
+ basicTestStruct{X: 3},
+ },
+ intr,
+ }
+
+ for _, test := range tests {
+ copiedVal := deepcopy(test)
+ if !reflect.DeepEqual(copiedVal, test) {
+ t.Errorf("failure in deepcopy. Expected %v, got %v", test, copiedVal)
+ }
+ }
+}
+
+func TestDeepcopySliceSettability(t *testing.T) {
+ rvSliceCopy := deepcopyReflect(reflect.ValueOf([]int{3, 4}))
+ if !rvSliceCopy.CanSet() {
+ t.Errorf("can't set slice. This is required for appending to slices")
+ }
+}
+
+func TestDeepcopyNilMap(t *testing.T) {
+ var nilMap map[int]int
+ mapCopy := deepcopy(nilMap)
+ if !reflect.DeepEqual(mapCopy, map[int]int{}) {
+ t.Errorf("expected an empty map, got %v", mapCopy)
+ }
+
+ type structWithMap struct {
+ M map[int]int
+ }
+ s := deepcopy(&structWithMap{})
+ if !reflect.DeepEqual(s, &structWithMap{map[int]int{}}) {
+ t.Errorf("expected an empty map in the struct, got %v", s)
+ }
+}
