all: fix reflection behavior for empty lists and maps
The protoreflect documentation states that Get on an empty repeated or
map field returns an invalid (empty, read-only) List or Map. We weren't
doing this; fix it.
The documentation also states that an invalid List or Map may not be
assigned via Set. We were permitting Set with an invalid map; fix this.
Add tests for this behavior in prototest.
Change-Id: I4678af532e192210af0bde7c96a1439a4cd26efa
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/209019
Reviewed-by: Joe Tsai <joetsai@google.com>
diff --git a/internal/impl/convert_list.go b/internal/impl/convert_list.go
index bec1d1d..1c86942 100644
--- a/internal/impl/convert_list.go
+++ b/internal/impl/convert_list.go
@@ -48,7 +48,7 @@
if !ok {
return false
}
- return list.v.Type().Elem() == c.goType
+ return list.v.Type().Elem() == c.goType && list.IsValid()
}
func (c *listConverter) IsValidGo(v reflect.Value) bool {
diff --git a/internal/impl/convert_map.go b/internal/impl/convert_map.go
index a05b131..34c3fcd 100644
--- a/internal/impl/convert_map.go
+++ b/internal/impl/convert_map.go
@@ -43,7 +43,7 @@
if !ok {
return false
}
- return mapv.v.Type() == c.goType
+ return mapv.v.Type() == c.goType && mapv.IsValid()
}
func (c *mapConverter) IsValidGo(v reflect.Value) bool {
diff --git a/internal/impl/message_reflect_field.go b/internal/impl/message_reflect_field.go
index 8d4e6ae..7b87d47 100644
--- a/internal/impl/message_reflect_field.go
+++ b/internal/impl/message_reflect_field.go
@@ -138,11 +138,18 @@
return conv.Zero()
}
rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
+ if rv.Len() == 0 {
+ return conv.Zero()
+ }
return conv.PBValueOf(rv)
},
set: func(p pointer, v pref.Value) {
rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
- rv.Set(conv.GoValueOf(v))
+ pv := conv.GoValueOf(v)
+ if pv.IsNil() {
+ panic(fmt.Sprintf("invalid value: setting map field to read-only value"))
+ }
+ rv.Set(pv)
},
mutable: func(p pointer) pref.Value {
v := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
@@ -184,11 +191,18 @@
return conv.Zero()
}
rv := p.Apply(fieldOffset).AsValueOf(fs.Type)
+ if rv.Elem().Len() == 0 {
+ return conv.Zero()
+ }
return conv.PBValueOf(rv)
},
set: func(p pointer, v pref.Value) {
rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
- rv.Set(reflect.ValueOf(v.List().(unwrapper).protoUnwrap()).Elem())
+ pv := conv.GoValueOf(v)
+ if pv.IsNil() {
+ panic(fmt.Sprintf("invalid value: setting repeated field to read-only value"))
+ }
+ rv.Set(pv.Elem())
},
mutable: func(p pointer) pref.Value {
v := p.Apply(fieldOffset).AsValueOf(fs.Type)
diff --git a/testing/prototest/prototest.go b/testing/prototest/prototest.go
index 508ccb9..040fddb 100644
--- a/testing/prototest/prototest.go
+++ b/testing/prototest/prototest.go
@@ -19,7 +19,6 @@
preg "google.golang.org/protobuf/reflect/protoregistry"
)
-// TODO: Test read-only properties of unpopulated composite values.
// TODO: Test invalid field descriptors or oneof descriptors.
// TODO: This should test the functionality that can be provided by fast-paths.
@@ -175,7 +174,7 @@
// Set to the default value.
switch {
case fd.IsList() || fd.IsMap():
- m.Set(fd, m.Get(fd))
+ m.Set(fd, m.Mutable(fd))
if got, want := m.Has(fd), (fd.IsExtension() && fd.Cardinality() != pref.Repeated) || fd.ContainingOneof() != nil; got != want {
t.Errorf("after setting %q to default:\nMessage.Has(%v) = %v, want %v", name, num, got, want)
}
@@ -207,10 +206,26 @@
// New values.
m.Clear(fd) // start with an empty map
mapv := m.Get(fd).Map()
+ if mapv.IsValid() {
+ t.Errorf("after clearing field: message.Get(%v).IsValid() = true, want false", name)
+ }
if got, want := mapv.NewValue(), newMapValue(fd, mapv, 0, nil); !valueEqual(got, want) {
t.Errorf("message.Get(%v).NewValue() = %v, want %v", name, formatValue(got), formatValue(want))
}
+ if !panics(func() {
+ m.Set(fd, pref.ValueOfMap(mapv))
+ }) {
+ t.Errorf("message.Set(%v, <invalid>) does not panic", name)
+ }
+ if !panics(func() {
+ mapv.Set(newMapKey(fd, 0), newMapValue(fd, mapv, 0, nil))
+ }) {
+ t.Errorf("message.Get(%v).Set(...) of invalid map does not panic", name)
+ }
mapv = m.Mutable(fd).Map() // mutable map
+ if !mapv.IsValid() {
+ t.Errorf("message.Mutable(%v).IsValid() = false, want true", name)
+ }
if got, want := mapv.NewValue(), newMapValue(fd, mapv, 0, nil); !valueEqual(got, want) {
t.Errorf("message.Mutable(%v).NewValue() = %v, want %v", name, formatValue(got), formatValue(want))
}
@@ -254,6 +269,9 @@
}
return true
})
+ if mapv := m.Get(fd).Map(); mapv.IsValid() {
+ t.Errorf("after clearing all elements: message.Get(%v).IsValid() = true, want false %v", name, formatValue(pref.ValueOfMap(mapv)))
+ }
// Non-existent map keys.
missingKey := newMapKey(fd, 1)
@@ -290,10 +308,26 @@
m.Clear(fd) // start with an empty list
list := m.Get(fd).List()
+ if list.IsValid() {
+ t.Errorf("message.Get(%v).IsValid() = true, want false", name)
+ }
+ if !panics(func() {
+ m.Set(fd, pref.ValueOfList(list))
+ }) {
+ t.Errorf("message.Set(%v, <invalid>) does not panic", name)
+ }
+ if !panics(func() {
+ list.Append(newListElement(fd, list, 0, nil))
+ }) {
+ t.Errorf("message.Get(%v).Append(...) of invalid list does not panic", name)
+ }
if got, want := list.NewElement(), newListElement(fd, list, 0, nil); !valueEqual(got, want) {
t.Errorf("message.Get(%v).NewElement() = %v, want %v", name, formatValue(got), formatValue(want))
}
list = m.Mutable(fd).List() // mutable list
+ if !list.IsValid() {
+ t.Errorf("message.Get(%v).IsValid() = false, want true", name)
+ }
if got, want := list.NewElement(), newListElement(fd, list, 0, nil); !valueEqual(got, want) {
t.Errorf("message.Mutable(%v).NewElement() = %v, want %v", name, formatValue(got), formatValue(want))
}
@@ -538,7 +572,7 @@
switch {
case fd.IsList():
if n == 0 {
- return m.New().Get(fd)
+ return m.New().Mutable(fd)
}
list := m.NewField(fd).List()
list.Append(newListElement(fd, list, 0, stack))
@@ -548,7 +582,7 @@
return pref.ValueOfList(list)
case fd.IsMap():
if n == 0 {
- return m.New().Get(fd)
+ return m.New().Mutable(fd)
}
mapv := m.NewField(fd).Map()
mapv.Set(newMapKey(fd, 0), newMapValue(fd, mapv, 0, stack))
diff --git a/types/dynamicpb/dynamic.go b/types/dynamicpb/dynamic.go
index b21f6da..828a2be 100644
--- a/types/dynamicpb/dynamic.go
+++ b/types/dynamicpb/dynamic.go
@@ -128,7 +128,18 @@
return m.known[num]
}
if v, ok := m.known[num]; ok {
- return v
+ switch {
+ case fd.IsMap():
+ if v.Map().Len() > 0 {
+ return v
+ }
+ case fd.IsList():
+ if v.List().Len() > 0 {
+ return v
+ }
+ default:
+ return v
+ }
}
switch {
case fd.IsMap():
@@ -413,18 +424,18 @@
func typeIsValid(fd pref.FieldDescriptor, v pref.Value) error {
switch {
case fd.IsMap():
- if mapv, ok := v.Interface().(*dynamicMap); !ok || mapv.desc != fd {
+ if mapv, ok := v.Interface().(*dynamicMap); !ok || mapv.desc != fd || !mapv.IsValid() {
return errors.New("%v: assigning invalid type %T", fd.FullName(), v.Interface())
}
return nil
case fd.IsList():
switch list := v.Interface().(type) {
case *dynamicList:
- if list.desc == fd {
+ if list.desc == fd && list.IsValid() {
return nil
}
case emptyList:
- if list.desc == fd {
+ if list.desc == fd && list.IsValid() {
return nil
}
}
