Skip to content

Commit

Permalink
net/proto2: remove <message>.ExtensionMap() from generated messages
Browse files Browse the repository at this point in the history 
Turn generated message struct field XXX_Extensions map[int32]Extension
into an embedded proto.InternalExtensions  struct

InternalExtensions is a struct without any exported fields and methods.
This effectively makes the representation of the extension map private.
The proto package can access InternalExtensions by checking that the
generated struct has the method 'extmap() proto.InternalExtensions'.

Also lock accesses to the extension map.

This change bumps the Go protobuf generated code version number. Any
.pb.go files generated with this version of the proto package or later
will require this version or later of the proto package to compile.
  • Loading branch information
@matloob
matloob committed May 23, 2016
1 parent cd85f19 commit e51d002
Show file tree
Hide file tree
Showing 15 changed files with 274 additions and 82 deletions.
17 changes: 6 additions & 11 deletions jsonpb/jsonpb.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,14 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU
}

// Handle proto2 extensions.
if ep, ok := v.(extendableProto); ok {
if ep, ok := v.(proto.Message); ok {
extensions := proto.RegisteredExtensions(v)
extensionMap := ep.ExtensionMap()
// Sort extensions for stable output.
ids := make([]int32, 0, len(extensionMap))
for id := range extensionMap {
ids := make([]int32, 0, len(extensions))
for id, desc := range extensions {
if !proto.HasExtension(ep, desc) {
continue
}
ids = append(ids, id)
}
sort.Sort(int32Slice(ids))
Expand Down Expand Up @@ -767,13 +769,6 @@ func acceptedJSONFieldNames(prop *proto.Properties) fieldNames {
return opts
}

// extendableProto is an interface implemented by any protocol buffer that may be extended.
type extendableProto interface {
proto.Message
ExtensionRangeArray() []proto.ExtensionRange
ExtensionMap() map[int32]proto.Extension
}

// Writer wrapper inspired by https://blog.golang.org/errors-are-values
type errWriter struct {
writer io.Writer
Expand Down
12 changes: 9 additions & 3 deletions proto/clone.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,15 @@ func mergeStruct(out, in reflect.Value) {
mergeAny(out.Field(i), in.Field(i), false, sprop.Prop[i])
}

if emIn, ok := in.Addr().Interface().(extendableProto); ok {
emOut := out.Addr().Interface().(extendableProto)
mergeExtension(emOut.ExtensionMap(), emIn.ExtensionMap())
if emIn, ok := extendable(in.Addr().Interface()); ok {
emOut, _ := extendable(out.Addr().Interface())
mIn, muIn := emIn.extensionsRead()
if mIn != nil {
mOut := emOut.extensionsWrite()
muIn.Lock()
mergeExtension(mOut, mIn)
muIn.Unlock()
}
}

uf := in.FieldByName("XXX_unrecognized")
Expand Down
7 changes: 4 additions & 3 deletions proto/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,11 +390,12 @@ func (o *Buffer) unmarshalType(st reflect.Type, prop *StructProperties, is_group
if !ok {
// Maybe it's an extension?
if prop.extendable {
if e := structPointer_Interface(base, st).(extendableProto); isExtensionField(e, int32(tag)) {
if e, _ := extendable(structPointer_Interface(base, st)); isExtensionField(e, int32(tag)) {
if err = o.skip(st, tag, wire); err == nil {
ext := e.ExtensionMap()[int32(tag)] // may be missing
extmap := e.extensionsWrite()
ext := extmap[int32(tag)] // may be missing
ext.enc = append(ext.enc, o.buf[oi:o.index]...)
e.ExtensionMap()[int32(tag)] = ext
extmap[int32(tag)] = ext
}
continue
}
Expand Down
28 changes: 24 additions & 4 deletions proto/encode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1073,10 +1073,25 @@ func size_slice_struct_group(p *Properties, base structPointer) (n int) {

// Encode an extension map.
func (o *Buffer) enc_map(p *Properties, base structPointer) error {
v := *structPointer_ExtMap(base, p.field)
if err := encodeExtensionMap(v); err != nil {
exts := structPointer_ExtMap(base, p.field)
if err := encodeExtensionsMap(*exts); err != nil {
return err
}

return o.enc_map_body(*exts)
}

func (o *Buffer) enc_exts(p *Properties, base structPointer) error {
exts := structPointer_Extensions(base, p.field)
if err := encodeExtensions(exts); err != nil {
return err
}
v, _ := exts.extensionsRead()

return o.enc_map_body(v)
}

func (o *Buffer) enc_map_body(v map[int32]Extension) error {
// Fast-path for common cases: zero or one extensions.
if len(v) <= 1 {
for _, e := range v {
Expand All @@ -1099,8 +1114,13 @@ func (o *Buffer) enc_map(p *Properties, base structPointer) error {
}

func size_map(p *Properties, base structPointer) int {
v := *structPointer_ExtMap(base, p.field)
return sizeExtensionMap(v)
v := structPointer_ExtMap(base, p.field)
return extensionsMapSize(*v)
}

func size_exts(p *Properties, base structPointer) int {
v := structPointer_Extensions(base, p.field)
return extensionsSize(v)
}

// Encode a map field.
Expand Down
12 changes: 7 additions & 5 deletions proto/equal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@ func equalStruct(v1, v2 reflect.Value) bool {
}
}

if em1 := v1.FieldByName("XXX_extensions"); em1.IsValid() {
em2 := v2.FieldByName("XXX_extensions")
if !equalExtensions(v1.Type(), em1.Interface().(map[int32]Extension), em2.Interface().(map[int32]Extension)) {
if em1 := v1.FieldByName("XXX_InternalExtensions"); em1.IsValid() {
em2 := v2.FieldByName("XXX_InternalExtensions")
if !equalExtensions(v1.Type(), em1.Interface().(XXX_InternalExtensions), em2.Interface().(XXX_InternalExtensions)) {
return false
}
}
Expand Down Expand Up @@ -223,8 +223,10 @@ func equalAny(v1, v2 reflect.Value, prop *Properties) bool {
}

// base is the struct type that the extensions are based on.
// em1 and em2 are extension maps.
func equalExtensions(base reflect.Type, em1, em2 map[int32]Extension) bool {
// x1 and x2 are InternalExtensions.
func equalExtensions(base reflect.Type, x1, x2 XXX_InternalExtensions) bool {
em1, _ := x1.extensionsRead()
em2, _ := x2.extensionsRead()
if len(em1) != len(em2) {
return false
}
Expand Down
Loading

19 comments on commit e51d002

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 20, 2016

Choose a reason for hiding this comment

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

With this change, there does not appear to be enough plumbing to generically/reflectively iterate through extensions: HasExtension works fine if the only populated field in the ExtensionDesc passed in is Field, but you cannot iterate through extensions. And GetExtension requires a fully (and correctly) populated ExtensionDesc; you could pass dummy data, but it would blow up the first time someone called it correctly. GetExtensions too requires a slice of populated ExtensionDesc objects.

We have a couple of small forks to protoc-gen-go that look for boolean field option extensions. We were simply iterating through field.GetOptions().ExtensionMap(), looking for matching extension number, and hand-decoding the boolean value. We actually change the generated code: for example, our "redacted" extension causes the given field to be redacted from the default string representation, so we don't accidentally log PII.

Would it be possible to add a method that returns a representation of extensions that can be programmatically manipulated/queried without access to the proper extension protos?

@matloob
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me make sure I understand: You want to be able to iterate through all extensions, including those whose generated proto code are not linked into the binary, right? And that's why you can't iterate through proto.RegisteredExtensions(pb), right?

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

Yeah, more or less. I've actually solved the first of my problems caused by this change by just creating fake ExtensionDesc objects - since they're only used in our protoc plugin, nothing else is going to use them.

The other uses I'm trying to fix now are (a) code that does serialization, which obviously needs to iterate over all present extensions, registered or not, and (b) logging code that iterates through present extensions rather than all registered extensions for efficiency: we'd wind up calling HasExtension 100 times when only zero or one are probably present.

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

@matloob @dsymonds A quick question: are we doing something wrong? Are we using parts of the exported protobuf API that we shouldn't be using? Changes like this are incredibly and repeatedly painful for us, but now that we can't update GRPC without also updating protobuf, we're forced to deal with protobuf API churn :-(

Is there a way to stay abreast of or participate in the discussion that happens before changes like this?

@matloob
Copy link
Contributor Author

@matloob matloob commented on e51d002 Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The change we made did remove the ability to examine extensions that are not registered. To fix that, we'll add a function to the proto library that looks like this:

package proto

// GetAllExtensionDescs returns a slice of the extensions present in pb.
// The returned slice is not guaranteed to be in any given order.
func GetAllExtensionDescs(pb Message) (extensions []*ExtensionDesc{}, err error)

Would that solve uses (a) and (b)?

To address your second question: I wouldn't say that you're doing something wrong. It looks to more like an engineering tradeoff: the flexibility of custom generated and serialization code can come with headaches and pain when the protobuf upstream library changes.

It was our mistake to drop support for extracting unregistered extensions, and we'll try to get a fix in soon. On the other hand, we don't and can't support custom serialization code or changes in the generated code. The protobuf library and the generated code it produces are built to work together. The protobuf library supports the code it generates, and (for a reasonable transition period) code generated by earlier versions of the library. It would be too much of a strain on on us to make changes in the library or generated format that would be guaranteed not to break any fork or other unanticipated use of the protobuf library.

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

Hmmmm. It depends on what's actually inside that []interface{}. If it's just the extension value, which might be (say) bool, then that would give no way to get the extension number, so you wouldn't know which boolean extension you were looking at.

Also, what is "es" ("which are also listed in es"), and how could you get an error?

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I have a branch (and can send a PR) that adds proto.ExtensionIDs(pb Message) []int32, which would allow less wasteful iteration…

@matloob
Copy link
Contributor Author

@matloob matloob commented on e51d002 Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah ExtensionIDs is a better solution. An alternative, what do you think of something that returns a []*proto.ExtensionDesc, with the actual ExtensionDesc present if the extension is registered, or a dummy ExtensionDesc{Field: extensionId} otherwise?

@matloob
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like this:


// GetAllExtensionDescs returns a slice of all the descriptors extensions present in pb.
// If an extension is not registered, a descriptor with only the 'field' value set will
// be returned instead of a full descriptor.
// The returned slice is not guaranteed to be in any given order.
func GetAllExtensionDescs(pb Message) (extensions []*ExtensionDesc, err error) {
    registeredExtensions := RegisteredExtensions(pb)
    epb, ok := pb.(extendableProto)
    if !ok {
        return nil, errors.New("proto: not an extendable proto")
    }
    extensions = make([]*ExtensionDesc, 0, len(epb.ExtensionMap()))
    for extid := range epb.ExtensionMap() {
        desc, ok := registeredExtensions[extid]
        if !ok {
            desc = &ExtensionDesc{Field: extid}
        }
        extensions = append(extensions, desc)
    }
    return extensions, nil
}

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

lgtm… although we'll only use Field :-)

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

For completeness, it should probably pass back out any ExtensionDesc objects that were cached by GetExtension too…

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

Ideally, I still feel like there should be a way to programmatically work with unknown (and unregistered) extensions… which you can't do right now, since you'll never guess an appropriate ExtensionDesc and you can't access the raw bytes. But when I look at our code that was doing that, it was using reflection to access the lowercase enc field… at least this change prompted me to get rid of that bit of ugliness! :-)

@matloob
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... you're probably right, but I'm wary of exposing too much of a proto's internals, so we need to make sure it's the right API.

Could you give me an example use case where you'd want to work with the extension, but you don't know what type it is? I'll try to think about how we can suupport it.

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

I wouldn't worry about it now.

But for instance if one wanted to properly serialize protos oneself, it would be useful…

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 21, 2016

Choose a reason for hiding this comment

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

One last question: do you expect GetAllExtensionDescs to land soon, or should I cobble together a workaround for now?

@matloob
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should get in over the next few days

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 28, 2016

Choose a reason for hiding this comment

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

Any news on this? I can put together a pull request if that would help…

@matloob
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, it's in now!

@zellyn
Copy link
Contributor

@zellyn zellyn commented on e51d002 Jun 28, 2016

Choose a reason for hiding this comment

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

Thanks!

Please sign in to comment.