dynamic.Message not using local extension registy?

[posener]

Hi,

In dynamic_message.go, the proto.RegisteredExtensions is used twice in the Message.mergeInto method. This, as far as I understand, takes the extensions that were registered in the global registry.

An issue that I see, is that extensions can be registered in the code that calls ParseFiles, and then used in the parsing process. This contaminates the parsing process, for example extensions could be used in the parsing process even if they were not defined in the parsed files.

I saw that the Message struct has a field of type ExtensionRegistry and wonder if it could be used instead of the proto.RegisteredExtensions call.

For the context, we have an issue that result in a panic very similar to what the user experienced in #380, and we worked around it by adding code that skips the extension registry for loops.

[jhump]

The issue you describe is unavoidable with the older protobuf runtime (v1.3 and older), which is when this library was originally written. The APIs in this old runtime (github.com/golang/protobuf) simply did not provide any way to do custom extensions. And the ExtensionRegistry type is bespoke to this repo, so can only be used with dynamic messages, not with generated messages.

However, this library has been updated to use v1.4 of the old runtime, which bridges to the new new runtime (google.golang.org/protobuf). And the new runtime does provide a way to use custom extensions.

I'm still trying to get my head around exactly what can be done here. I think we could convert/wrap the ExtensionRegistry so it supports the interface supported by the new runtime: protoregistry.ExtensionTypeResolver. And then I think we can use that, instead of the globally known extensions.

TBH, I'm not certain if that will work. I don't actually know what the old runtime does if a generated message contains an extension that the global registry doesn't know about. Maybe it will work fine? Or maybe you just can't use the old runtime to inspect those extensions, and calling code would have to use the new runtime for that?

I'll have to play with this a little.

[jhump]

Actually, wrapping ExtensionRegistry so it supports the new runtime's interface is no small task. It requires bridging the descriptor type introduced in this repo (desc.Descriptor) to the new runtime's descriptor type (protoreflect.Descriptor).

I actually have done a lot of that work in a branch, but just haven't merged it because I'm worried about how big of an impact it will have (mostly on performance, but I worry a little that it could potentially break people in other ways).

[posener]

Thanks @jhump ,

I would love to help if it is possible - but the solution seems to be very complicated (maybe you can reference also the pending PR that you mentioned?). I appreciate your explanation and help in any case!

[jhump]

FWIW, that pending PR was #354. And it has now been merged and released in v1.15.

There is still no direct interop support between dynamic.ExtensionRegistry and the newer runtime API stuff in google.golang.org/protobuf/reflect/protoregistry. Instead, I would recommend using google.golang.org/protobuf/types/dynamicpb, which supports dynamic messages and dynamic extension types. And you can safely¹ use a dynamic extension with generated messages.

[1] Caveat: using proto.GetExtension(msg, ext) with a dynamic extension may panic. That method was intended for use with generated extension descriptions. For dynamic ones, instead use msg.ProtoReflect().Get(ext).