mirror/protobuf-go
1
0
Fork 0
mirror of https://github.com/protocolbuffers/protobuf-go.git synced 2025-03-30 16:20:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

all: fix reflection behavior for empty lists and maps

Browse Source 
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>
This commit is contained in:
Damien Neil 2019-11-26 13:31:38 -08:00
parent 82886da2b9
commit 9f1165c3bf
5 changed files with 71 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
internal/impl/convert_list.go
View File

@ -48,7 +48,7 @@ func (c *listConverter) IsValidPB(v pref.Value) bool {
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 {

2
internal/impl/convert_map.go
View File

@ -43,7 +43,7 @@ func (c *mapConverter) IsValidPB(v pref.Value) bool {
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 {

18
internal/impl/message_reflect_field.go
View File

@ -138,11 +138,18 @@ func fieldInfoForMap(fd pref.FieldDescriptor, fs reflect.StructField, x exporter
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 @@ func fieldInfoForList(fd pref.FieldDescriptor, fs reflect.StructField, x exporte
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)

42
testing/prototest/prototest.go
View File

@ -19,7 +19,6 @@ import (
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 @@ func testField(t testing.TB, m pref.Message, fd pref.FieldDescriptor) {
// 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 @@ func testFieldMap(t testing.TB, m pref.Message, fd pref.FieldDescriptor) {
// 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 @@ func testFieldMap(t testing.TB, m pref.Message, fd pref.FieldDescriptor) {
}
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 @@ func testFieldList(t testing.TB, m pref.Message, fd pref.FieldDescriptor) {
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 @@ func newValue(m pref.Message, fd pref.FieldDescriptor, n seed, stack []pref.Mess
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 @@ func newValue(m pref.Message, fd pref.FieldDescriptor, n seed, stack []pref.Mess
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))

19
types/dynamicpb/dynamic.go
View File

@ -128,7 +128,18 @@ func (m *Message) Get(fd pref.FieldDescriptor) pref.Value {
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 typecheck(fd pref.FieldDescriptor, v pref.Value) {
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
}
}