Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proto: store extension values according to protobuf data model #746

Merged
merged 1 commit into from Nov 15, 2018
Merged

proto: store extension values according to protobuf data model #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 33 additions & 17 deletions proto/clone_test.go
View file
Open in desktop
Expand Up @@ -67,34 +67,50 @@ func init() {
if err := proto.SetExtension(cloneTestMessage, pb.E_Ext_More, ext); err != nil {
panic("SetExtension: " + err.Error())
}
if err := proto.SetExtension(cloneTestMessage, pb.E_Ext_Text, proto.String("hello")); err != nil {
panic("SetExtension: " + err.Error())
}
if err := proto.SetExtension(cloneTestMessage, pb.E_Greeting, []string{"one", "two"}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also need to verify that clone makes a deep copy on this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

panic("SetExtension: " + err.Error())
}
}

func TestClone(t *testing.T) {
// Create a clone using a marshal/unmarshal roundtrip.
vanilla := new(pb.MyMessage)
b, err := proto.Marshal(cloneTestMessage)
if err != nil {
t.Errorf("unexpected Marshal error: %v", err)
}
if err := proto.Unmarshal(b, vanilla); err != nil {
t.Errorf("unexpected Unarshal error: %v", err)
}

// Create a clone using Clone and verify that it is equal to the original.
m := proto.Clone(cloneTestMessage).(*pb.MyMessage)
if !proto.Equal(m, cloneTestMessage) {
t.Fatalf("Clone(%v) = %v", cloneTestMessage, m)
}

// Verify it was a deep copy.
*m.Inner.Port++
if proto.Equal(m, cloneTestMessage) {
t.Error("Mutating clone changed the original")
}
// Byte fields and repeated fields should be copied.
if &m.Pet[0] == &cloneTestMessage.Pet[0] {
t.Error("Pet: repeated field not copied")
// Mutate the clone, which should not affect the original.
x1, err := proto.GetExtension(m, pb.E_Ext_More)
if err != nil {
t.Errorf("unexpected GetExtension(%v) error: %v", pb.E_Ext_More.Name, err)
}
if &m.Others[0] == &cloneTestMessage.Others[0] {
t.Error("Others: repeated field not copied")
x2, err := proto.GetExtension(m, pb.E_Ext_Text)
if err != nil {
t.Errorf("unexpected GetExtension(%v) error: %v", pb.E_Ext_Text.Name, err)
}
if &m.Others[0].Value[0] == &cloneTestMessage.Others[0].Value[0] {
t.Error("Others[0].Value: bytes field not copied")
x3, err := proto.GetExtension(m, pb.E_Greeting)
if err != nil {
t.Errorf("unexpected GetExtension(%v) error: %v", pb.E_Greeting.Name, err)
}
if &m.RepBytes[0] == &cloneTestMessage.RepBytes[0] {
t.Error("RepBytes: repeated field not copied")
}
if &m.RepBytes[0][0] == &cloneTestMessage.RepBytes[0][0] {
t.Error("RepBytes[0]: bytes field not copied")
*m.Inner.Port++
*(x1.(*pb.Ext)).Data = "blah blah"
*(x2.(*string)) = "goodbye"
x3.([]string)[0] = "zero"
if !proto.Equal(cloneTestMessage, vanilla) {
t.Fatalf("mutation on original detected:\ngot %v\nwant %v", cloneTestMessage, vanilla)
}
}

Expand Down
3 changes: 2 additions & 1 deletion proto/equal.go
View file
Open in desktop
Expand Up @@ -246,7 +246,8 @@ func equalExtMap(base reflect.Type, em1, em2 map[int32]Extension) bool {
return false
}

m1, m2 := e1.value, e2.value
m1 := extensionAsLegacyType(e1.value)
m2 := extensionAsLegacyType(e2.value)

if m1 == nil && m2 == nil {
// Both have only encoded form.
Expand Down
76 changes: 70 additions & 6 deletions proto/extensions.go
View file
Open in desktop
Expand Up @@ -185,9 +185,25 @@ type Extension struct {
// extension will have only enc set. When such an extension is
// accessed using GetExtension (or GetExtensions) desc and value
// will be set.
desc *ExtensionDesc
desc *ExtensionDesc

// value is a concrete value for the extension field. Let the type of
// desc.ExtensionType be the "API type" and the type of Extension.value
// be the "storage type". The API type and storage type are the same except:
// * For scalars (except []byte), the API type uses *T,
// while the storage type uses T.
// * For repeated fields, the API type uses []T, while the storage type
// uses *[]T.
//
// The reason for the divergence is so that the storage type more naturally
// matches what is expected of when retrieving the values through the
// protobuf reflection APIs.
//
// The value may only be populated if desc is also populated.
value interface{}
enc []byte

// enc is the raw bytes for the extension field.
enc []byte
}

// SetRawExtension is for testing only.
Expand Down Expand Up @@ -334,7 +350,7 @@ func GetExtension(pb Message, extension *ExtensionDesc) (interface{}, error) {
// descriptors with the same field number.
return nil, errors.New("proto: descriptor conflict")
}
return e.value, nil
return extensionAsLegacyType(e.value), nil
}

if extension.ExtensionType == nil {
Expand All @@ -349,11 +365,11 @@ func GetExtension(pb Message, extension *ExtensionDesc) (interface{}, error) {

// Remember the decoded version and drop the encoded version.
// That way it is safe to mutate what we return.
e.value = v
e.value = extensionAsStorageType(v)
e.desc = extension
e.enc = nil
emap[extension.Field] = e
return e.value, nil
return extensionAsLegacyType(e.value), nil
}

// defaultExtensionValue returns the default value for extension.
Expand Down Expand Up @@ -500,7 +516,7 @@ func SetExtension(pb Message, extension *ExtensionDesc, value interface{}) error
}

extmap := epb.extensionsWrite()
extmap[extension.Field] = Extension{desc: extension, value: value}
extmap[extension.Field] = Extension{desc: extension, value: extensionAsStorageType(value)}
return nil
}

Expand Down Expand Up @@ -541,3 +557,51 @@ func RegisterExtension(desc *ExtensionDesc) {
func RegisteredExtensions(pb Message) map[int32]*ExtensionDesc {
return extensionMaps[reflect.TypeOf(pb).Elem()]
}

// extensionAsLegacyType converts an value in the storage type as the API type.
// See Extension.value.
func extensionAsLegacyType(v interface{}) interface{} {
neild marked this conversation as resolved.
Show resolved Hide resolved
switch rv := reflect.ValueOf(v); rv.Kind() {
case reflect.Bool, reflect.Int32, reflect.Int64, reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.String:
// Represent primitive types as a pointer to the value.
rv2 := reflect.New(rv.Type())
rv2.Elem().Set(rv)
v = rv2.Interface()
case reflect.Ptr:
// Represent slice types as the value itself.
switch rv.Type().Elem().Kind() {
case reflect.Slice:
if rv.IsNil() {
v = reflect.Zero(rv.Type().Elem()).Interface()
} else {
v = rv.Elem().Interface()
}
}
}
return v
}

// extensionAsStorageType converts an value in the API type as the storage type.
// See Extension.value.
func extensionAsStorageType(v interface{}) interface{} {
switch rv := reflect.ValueOf(v); rv.Kind() {
case reflect.Ptr:
// Represent slice types as the value itself.
switch rv.Type().Elem().Kind() {
case reflect.Bool, reflect.Int32, reflect.Int64, reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.String:
if rv.IsNil() {
v = reflect.Zero(rv.Type().Elem()).Interface()
} else {
v = rv.Elem().Interface()
}
}
case reflect.Slice:
// Represent slice types as a pointer to the value.
if rv.Type().Elem().Kind() != reflect.Uint8 {
rv2 := reflect.New(rv.Type())
rv2.Elem().Set(rv)
v = rv2.Interface()
}
}
return v
}
5 changes: 4 additions & 1 deletion proto/pointer_reflect.go
View file
Open in desktop
Expand Up @@ -79,10 +79,13 @@ func toPointer(i *Message) pointer {

// toAddrPointer converts an interface to a pointer that points to
// the interface data.
func toAddrPointer(i *interface{}, isptr bool) pointer {
func toAddrPointer(i *interface{}, isptr, deref bool) pointer {
v := reflect.ValueOf(*i)
u := reflect.New(v.Type())
u.Elem().Set(v)
if deref {
u = u.Elem()
}
return pointer{v: u}
}

Expand Down
15 changes: 10 additions & 5 deletions proto/pointer_unsafe.go
View file
Open in desktop
Expand Up @@ -85,16 +85,21 @@ func toPointer(i *Message) pointer {

// toAddrPointer converts an interface to a pointer that points to
// the interface data.
func toAddrPointer(i *interface{}, isptr bool) pointer {
func toAddrPointer(i *interface{}, isptr, deref bool) (p pointer) {
// Super-tricky - read or get the address of data word of interface value.
if isptr {
// The interface is of pointer type, thus it is a direct interface.
// The data word is the pointer data itself. We take its address.
return pointer{p: unsafe.Pointer(uintptr(unsafe.Pointer(i)) + ptrSize)}
p = pointer{p: unsafe.Pointer(uintptr(unsafe.Pointer(i)) + ptrSize)}
} else {
// The interface is not of pointer type. The data word is the pointer
// to the data.
p = pointer{p: (*[2]unsafe.Pointer)(unsafe.Pointer(i))[1]}
}
// The interface is not of pointer type. The data word is the pointer
// to the data.
return pointer{p: (*[2]unsafe.Pointer)(unsafe.Pointer(i))[1]}
if deref {
p.p = *(*unsafe.Pointer)(p.p)
}
return p
}

// valToPointer converts v to a pointer. v must be of pointer type.
Expand Down
34 changes: 22 additions & 12 deletions proto/table_marshal.go
View file
Open in desktop
Expand Up @@ -87,6 +87,7 @@ type marshalElemInfo struct {
sizer sizer
marshaler marshaler
isptr bool // elem is pointer typed, thus interface of this type is a direct interface (extension only)
deref bool // dereference the pointer before operating on it; implies isptr
}

var (
Expand Down Expand Up @@ -407,13 +408,22 @@ func (u *marshalInfo) getExtElemInfo(desc *ExtensionDesc) *marshalElemInfo {
panic("tag is not an integer")
}
wt := wiretype(tags[0])
if t.Kind() == reflect.Ptr && t.Elem().Kind() != reflect.Struct {
t = t.Elem()
}
sizer, marshaler := typeMarshaler(t, tags, false, false)
var deref bool
if t.Kind() == reflect.Slice && t.Elem().Kind() != reflect.Uint8 {
t = reflect.PtrTo(t)
deref = true
}
e = &marshalElemInfo{
wiretag: uint64(tag)<<3 | wt,
tagsize: SizeVarint(uint64(tag) << 3),
sizer: sizer,
marshaler: marshaler,
isptr: t.Kind() == reflect.Ptr,
deref: deref,
}

// update cache
Expand Down Expand Up @@ -2310,8 +2320,8 @@ func makeMapMarshaler(f *reflect.StructField) (sizer, marshaler) {
for _, k := range m.MapKeys() {
ki := k.Interface()
vi := m.MapIndex(k).Interface()
kaddr := toAddrPointer(&ki, false) // pointer to key
vaddr := toAddrPointer(&vi, valIsPtr) // pointer to value
kaddr := toAddrPointer(&ki, false, false) // pointer to key
vaddr := toAddrPointer(&vi, valIsPtr, false) // pointer to value
siz := keySizer(kaddr, 1) + valSizer(vaddr, 1) // tag of key = 1 (size=1), tag of val = 2 (size=1)
n += siz + SizeVarint(uint64(siz)) + tagsize
}
Expand All @@ -2329,8 +2339,8 @@ func makeMapMarshaler(f *reflect.StructField) (sizer, marshaler) {
for _, k := range keys {
ki := k.Interface()
vi := m.MapIndex(k).Interface()
kaddr := toAddrPointer(&ki, false) // pointer to key
vaddr := toAddrPointer(&vi, valIsPtr) // pointer to value
kaddr := toAddrPointer(&ki, false, false) // pointer to key
vaddr := toAddrPointer(&vi, valIsPtr, false) // pointer to value
b = appendVarint(b, tag)
siz := keySizer(kaddr, 1) + valCachedSizer(vaddr, 1) // tag of key = 1 (size=1), tag of val = 2 (size=1)
b = appendVarint(b, uint64(siz))
Expand Down Expand Up @@ -2399,7 +2409,7 @@ func (u *marshalInfo) sizeExtensions(ext *XXX_InternalExtensions) int {
// the last time this function was called.
ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
n += ei.sizer(p, ei.tagsize)
}
mu.Unlock()
Expand Down Expand Up @@ -2434,7 +2444,7 @@ func (u *marshalInfo) appendExtensions(b []byte, ext *XXX_InternalExtensions, de

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
b, err = ei.marshaler(b, p, ei.wiretag, deterministic)
if !nerr.Merge(err) {
return b, err
Expand Down Expand Up @@ -2465,7 +2475,7 @@ func (u *marshalInfo) appendExtensions(b []byte, ext *XXX_InternalExtensions, de

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
b, err = ei.marshaler(b, p, ei.wiretag, deterministic)
if !nerr.Merge(err) {
return b, err
Expand Down Expand Up @@ -2510,7 +2520,7 @@ func (u *marshalInfo) sizeMessageSet(ext *XXX_InternalExtensions) int {

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
n += ei.sizer(p, 1) // message, tag = 3 (size=1)
}
mu.Unlock()
Expand Down Expand Up @@ -2553,7 +2563,7 @@ func (u *marshalInfo) appendMessageSet(b []byte, ext *XXX_InternalExtensions, de

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
b, err = ei.marshaler(b, p, 3<<3|WireBytes, deterministic)
if !nerr.Merge(err) {
return b, err
Expand Down Expand Up @@ -2591,7 +2601,7 @@ func (u *marshalInfo) appendMessageSet(b []byte, ext *XXX_InternalExtensions, de

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
b, err = ei.marshaler(b, p, 3<<3|WireBytes, deterministic)
b = append(b, 1<<3|WireEndGroup)
if !nerr.Merge(err) {
Expand Down Expand Up @@ -2621,7 +2631,7 @@ func (u *marshalInfo) sizeV1Extensions(m map[int32]Extension) int {

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
n += ei.sizer(p, ei.tagsize)
}
return n
Expand Down Expand Up @@ -2656,7 +2666,7 @@ func (u *marshalInfo) appendV1Extensions(b []byte, m map[int32]Extension, determ

ei := u.getExtElemInfo(e.desc)
v := e.value
p := toAddrPointer(&v, ei.isptr)
p := toAddrPointer(&v, ei.isptr, ei.deref)
b, err = ei.marshaler(b, p, ei.wiretag, deterministic)
if !nerr.Merge(err) {
return b, err
Expand Down