veyron/services/store: Don't put values of the wrong type.
Previously, if the caller tried to put a value of the wrong type into a
top-level field, the value would be placed in the implicit directory of the object,
under the same name as that field.
This fix adds a new failure class to field.Set. If the value has the wrong type,
field.Set will return SetFailedWrongType, and Put() will return an error.
Change-Id: I8a6294c5a717434909e36d51e73b82a64bcb727c
diff --git a/services/store/memstore/field/reflect.go b/services/store/memstore/field/reflect.go
index 5a76e29..891cb31 100644
--- a/services/store/memstore/field/reflect.go
+++ b/services/store/memstore/field/reflect.go
@@ -2,9 +2,9 @@
// reflect.go uses reflection to access the fields in a value.
//
-// getField(v, path)
-// setField(v, path, x)
-// removeField(v, path)
+// Get(v, path)
+// Set(v, path, x)
+// Remove(v, path)
//
// The path is JSON-style, using field names. For example, consider the
// following value.
@@ -19,19 +19,19 @@
// Here are some possible paths:
//
// var x MyValue = ...
-// getField(x, storage.PathName{"A"}) == x.A
-// getField(x, storage.PathName{"B", "7"}) == x.B[7]
-// getField(x, storage.PathName{"C", "a"}) == x.C["a"]
-// getField(x, storage.PathName{"D", "a", "E"}) == x.D["a"].E
+// Get(x, storage.PathName{"A"}) == x.A
+// Get(x, storage.PathName{"B", "7"}) == x.B[7]
+// Get(x, storage.PathName{"C", "a"}) == x.C["a"]
+// Get(x, storage.PathName{"D", "a", "E"}) == x.D["a"].E
//
-// setField(x, storage.PathName{"A"}, 17)
-// getField(x, storage.PathName{"A"}) == 17
+// Set(x, storage.PathName{"A"}, 17)
+// Get(x, storage.PathName{"A"}) == 17
//
-// setField(x, storage.PathName{"D", "a"}, struct{E: 12})
-// getField(x, storage.PathName{"D", "a", "E"}) == 12
+// Set(x, storage.PathName{"D", "a"}, struct{E: 12})
+// Get(x, storage.PathName{"D", "a", "E"}) == 12
//
-// removeField(x, storage.PathName{"D", "a"}
-// getField(x, storage.PathName{"D", "a", "E"}) fails
+// Remove(x, storage.PathName{"D", "a"}
+// Get(x, storage.PathName{"D", "a", "E"}) fails
import (
"reflect"
@@ -43,7 +43,8 @@
type SetResult uint32
const (
- SetFailed SetResult = iota
+ SetFailedNotFound SetResult = iota
+ SetFailedWrongType
SetAsValue
SetAsID
)
@@ -135,26 +136,27 @@
//
// Here are the possible cases:
//
-// 1. SetFailed if the operation failed because the value has the wrong type
-// or the path doesn't exist.
+// 1. SetFailedNotFound if the operation failed because the path doesn't exist.
//
-// 2. SetAsValue if the operation was successful, and the value xval was
+// 2. SetFailedWrongType if the operation failed because the value has the wrong type.
+//
+// 3. SetAsValue if the operation was successful, and the value xval was
// stored. The returned storage.ID is null.
//
-// 3. SetAsId if the operation was successful, but the type of the field is
+// 4. 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).
//
-// The setFieldAsID case means that the value xval is to be stored as a separate
+// The setAsID case means that the value xval is to be stored as a separate
// value in the store, not as a subfield of the current value.
//
// As a special case, if the field type is storage.ID, and xval has type storage.ID,
-// then it is case #2, setFieldAsValue. The returned storage.ID is zero.
+// then it is case #2, setAsValue. The returned storage.ID is zero.
func Set(v reflect.Value, name string, xval interface{}) (SetResult, storage.ID) {
v = followPointers(v)
if !v.IsValid() {
- return SetFailed, nullID
+ return SetFailedNotFound, nullID
}
switch v.Type().Kind() {
case reflect.Map:
@@ -164,7 +166,7 @@
case reflect.Struct:
return setStructField(v, name, xval)
default:
- return SetFailed, nullID
+ return SetFailedNotFound, nullID
}
}
@@ -172,12 +174,12 @@
tyV := v.Type()
tyKey := tyV.Key()
if tyKey.Kind() != reflect.String {
- return SetFailed, nullID
+ return SetFailedNotFound, nullID
}
key := reflect.ValueOf(name).Convert(tyKey)
r, x, id := coerceValue(tyV.Elem(), v.MapIndex(key), xval)
- if r == SetFailed {
- return SetFailed, nullID
+ if r == SetFailedWrongType {
+ return SetFailedWrongType, nullID
}
v.SetMapIndex(key, x)
return r, id
@@ -186,8 +188,8 @@
func setSliceField(v reflect.Value, field string, xval interface{}) (SetResult, storage.ID) {
if field == SliceAppendSuffix {
r, x, id := coerceValue(v.Type().Elem(), nullValue, xval)
- if r == SetFailed {
- return SetFailed, nullID
+ if r == SetFailedWrongType {
+ return SetFailedWrongType, nullID
}
// This can panic if v is not settable. It is a requirement that users of this method
// ensure that it is settable.
@@ -197,11 +199,11 @@
l := v.Len()
i, err := strconv.Atoi(field)
if err != nil || i < 0 || i >= l {
- return SetFailed, nullID
+ return SetFailedNotFound, nullID
}
r, x, id := coerceValue(v.Type().Elem(), v.Index(i), xval)
- if r == SetFailed {
- return SetFailed, nullID
+ if r == SetFailedWrongType {
+ return SetFailedWrongType, nullID
}
v.Index(i).Set(x)
return r, id
@@ -210,12 +212,12 @@
func setStructField(v reflect.Value, name string, xval interface{}) (SetResult, storage.ID) {
field, found := v.Type().FieldByName(name)
if !found {
- return SetFailed, nullID
+ return SetFailedNotFound, nullID
}
fieldVal := v.FieldByName(name)
r, x, id := coerceValue(field.Type, fieldVal, xval)
- if r == SetFailed {
- return SetFailed, nullID
+ if r == SetFailedWrongType {
+ return SetFailedWrongType, nullID
}
fieldVal.Set(x)
return r, id
@@ -232,7 +234,7 @@
if prev.IsValid() {
var ok bool
if id, ok = prev.Interface().(storage.ID); !ok {
- return SetFailed, nullValue, nullID
+ return SetFailedWrongType, nullValue, nullID
}
} else {
id = storage.NewID()
@@ -243,7 +245,7 @@
case x.Type().ConvertibleTo(ty):
return SetAsValue, x.Convert(ty), nullID
default:
- return SetFailed, nullValue, nullID
+ return SetFailedWrongType, nullValue, nullID
}
}
diff --git a/services/store/memstore/state/mutable_snapshot.go b/services/store/memstore/state/mutable_snapshot.go
index 0845fcd..f640eb1 100644
--- a/services/store/memstore/state/mutable_snapshot.go
+++ b/services/store/memstore/state/mutable_snapshot.go
@@ -105,6 +105,7 @@
var (
errBadPath = verror.BadArgf("malformed path")
errBadRef = verror.BadArgf("value has dangling references")
+ errBadValue = verror.BadArgf("value has the wrong type")
errDuplicatePutMutation = verror.BadArgf("duplicate calls to PutMutation for the same ID")
errNotFound = verror.NotFoundf("not found")
errNotTagList = verror.BadArgf("not a TagList")
@@ -381,7 +382,7 @@
name := path[len(path)-1]
result, id := field.Set(p, name, v)
switch result {
- case field.SetFailed:
+ case field.SetFailedNotFound:
if len(suffix) != 0 {
return nil, errNotFound
}
@@ -389,6 +390,8 @@
return sn.putTags(checker, c, v)
}
return sn.putDirEntry(checker, c, name, v)
+ case field.SetFailedWrongType:
+ return nil, errBadValue
case field.SetAsID:
nc, err := sn.add(checker, id, v)
if err != nil {
@@ -417,8 +420,10 @@
name := path[len(path)-1]
result, id := field.Set(p, name, v)
switch result {
- case field.SetFailed:
+ case field.SetFailedNotFound:
return nil, errNotFound
+ case field.SetFailedWrongType:
+ return nil, errBadValue
case field.SetAsID:
nc, err := sn.add(checker, id, v)
if err != nil {
diff --git a/services/store/memstore/state/mutable_snapshot_test.go b/services/store/memstore/state/mutable_snapshot_test.go
index c65794c..34366c6 100644
--- a/services/store/memstore/state/mutable_snapshot_test.go
+++ b/services/store/memstore/state/mutable_snapshot_test.go
@@ -21,6 +21,11 @@
X int
}
+// Nest is a struct that nests a Value.
+type Nest struct {
+ V Value
+}
+
var (
root = &Dir{}
rootPath = storage.ParsePath("/")
@@ -335,3 +340,44 @@
t.Errorf("Expected 3, got %v", v)
}
}
+
+// Put a value of the wrong type to a subfield.
+func TestPutWrongType(t *testing.T) {
+ sn := newMutableSnapshot(rootPublicID)
+
+ _, err := sn.Put(rootPublicID, rootPath, Value{7})
+ if err != nil {
+ t.Errorf("/: %v", err)
+ }
+ _, err = sn.Put(rootPublicID, storage.ParsePath("/X"), "string")
+ if !verror.Is(err, verror.BadArg) {
+ t.Errorf("/X: %v", err)
+ }
+ v, err := sn.Get(rootPublicID, storage.ParsePath("/X"))
+ if err != nil {
+ t.Errorf("/X: %v", err)
+ }
+ if v.Value != 7 {
+ t.Errorf("Expected 7, got %v", v.Value)
+ }
+
+ _, err = sn.Put(rootPublicID, storage.ParsePath("/a"), Nest{Value{42}})
+ if err != nil {
+ t.Errorf("/a: %v", err)
+ }
+ _, err = sn.Put(rootPublicID, storage.ParsePath("/a/V"), "string")
+ if !verror.Is(err, verror.BadArg) {
+ t.Errorf("/a/V: %v", err)
+ }
+ _, err = sn.Put(rootPublicID, storage.ParsePath("/a/V/X"), "string")
+ if !verror.Is(err, verror.BadArg) {
+ t.Errorf("/a/V/X: %v", err)
+ }
+ v, err = sn.Get(rootPublicID, storage.ParsePath("/a/V/X"))
+ if err != nil {
+ t.Errorf("/a/V/X: %v", err)
+ }
+ if v.Value != 42 {
+ t.Errorf("Expected 42, got %v", v.Value)
+ }
+}
