WKT do not appear to work with MessageDescriptors retrieved from FindMessage after CreateFileDescriptorFromSet

[bufdev]

I have a fully-valid FileDescriptorSet created from protoc with --include_imports, that contains google/protobuf/duration.proto in the files list, and the message I try to operate on contains a google.protobuf.Duration field named duration. When I create a DynamicMessage with dynamic.NewMessage (or using the default message factory with the WKT resolver and NewDynamicMessage) from a MessageDescriptor from FindMessage from something returned from CreateFileDescriptorFromSet where the message is contained in the last file. When I try to unmarshal '{"duration":"10.000s"}', I get the error Bad input. Expecting start of JSON object: '{'. Instead got: 10.000s.. I believe this is related to the fact that dynamic/message_factory.go relies on reflection to see if there is a registered WKT message in the github.com/golang/protobuf globals, when this library shouldn't rely on the global maps within github.com/golang/protobuf at all (ie calls to proto.MessageType) - it makes this unusable for github.com/gogo/protobuf users, and belies the purpose of being able to do everything dynamically with FileDescriptorSets in the first place :-)

I know this is a really weak explanation, I'll try to create a test that reproduces this, but wanted to kick off the conversation.

Great library btw!

[bufdev]

Update: I've created a test on a branch chained on top of #100, see here: https://github.com/peter-edge/protoreflect/compare/protoc-download...peter-edge:wkt-test?expand=1

In this test, I have the FileDescriptorSet for google/protobuf/duration.proto loaded from a file, and then I attempt to unmarshal valid JSON from a marshalled google.protobuf.Duration. This results in the error Bad input. Expecting start of JSON object: '{'. Instead got: 30303.000040404s.

[jhump]

I am pretty sure I understand the issue. It is because your program has not linked in the packages that contain the generated code for the well-known types, so proto.MessageName cannot find the types.

I see a couple of possible solutions:

1. The dynamic package could use link-only imports (i.e. _) to basically link in all of the applicable packages. These packages are actually quite small since they only contain the generated proto code, not the runtime support (which I believe is all in the protobuf/ptypes package). So this should not be much bloat and is very low effort.
2. The JSON implementation of *dynamic.Message could be updated to include logic for the custom JSON formats of the well-known types, forking/reproducing the logic in the protobuf/jsonpb package.

I'm personally strongly inclined to go with option 1. If this makes sense to you, and you have time to make the change (option 1 should be very small), and submit a pull request for it then I will merge it.

[bufdev]

Let me try that out, thanks so much for the quick response!

[jhump]

I like the test case. I'll include it in any PR that intends to fix this issue. (If you send a PR, please include the test with it!)

[bufdev]

I've looked into this, note this can't be the issue I don't think - dynamic/json_test.go (which is where I have that new test case) includes github.com/golang/protobuf/ptypes/duration, which would link in google.protobuf.Duration. Can you confirm I'm not crazy? Heh

[bufdev]

I did some more investigation:

• protoreflect/dynamic/json.go

  Line 372 in 8ade26d

  func (m *Message) unmarshalJson(r *jsReader, opts *jsonpb.Unmarshaler) error {

  is hit whenever you start unmarshalling an object, or your are unmarshalling a field that is a Message (which i.e. google.protobuf.Duration and other WKTs are)

• protoreflect/dynamic/json.go

  Line 390 in 8ade26d

  return err

  is where the error is returned.

  protoreflect/dynamic/json.go

  Line 707 in 8ade26d

  _, err := r.expect(func(t json.Token) bool { return t == json.Delim('{') }, nil, "start of JSON object: '{'")

  expects an opening bracket (and does not hit the resolver) for any Message.
• The resolver is never hit, there is no code path to the resolver for a field that is a WKT, or a WKT itself

I think this is an actual bug in the code, can you confirm?

[jhump]

Ah, I see. Yes, this is a legit bug.

One issue is the thing I mentioned, for programs that do not link in the well-known type packages. That causes an issue when a field has a well-known type here.

But when you already have a dynamic message that represents a well-known type, separate handling is needed. The fix is definitely a bit more involved. I have an idea; I think I can push a fix this weekend, maybe even later today.

[prasek]

Hey, thanks for a great library! I'm using gogo/protobuf with go2idl and doing some interceptor policy stuff that would benefit from grabbing options, so I followed your post grpc/grpc-go#1526 and made it work by porting the library to gogo here:
master...prasek:master.

[jhump]

@prasek, wow, that is great! Are you intending to maintain that gogo fork? If so, I will update my readme, referring users of gogo-protobuf to your repo.

[prasek]

@jhump To avoid maintaining a separate fork I've got an experimental branch where I introduced https://github.com/prasek/protoreflect/blob/grpc/proto/protoer.go to support both golang/protobuf and gogo/protobuf from a single repo. I have this working with a refactored subset of the code here: https://github.com/prasek/protoreflect/blob/grpc.

If you ignore my desc package refactoring, I think the proto package could be easily incorporated into the main repo. Note there are separate golang/protobuf and gogo/protobuf tests under the internal package. I think we could reduce some of the duplication, but the different package imports make it difficult. However, a handful of duplicate test files is much better than an entirely forked repo.

If this is something you're open to exploring I could take a stab at a PR with protoer support, so my fork doesn't have to be separately maintained.

[jhump]

@prasek, in my ideal world, this repo would not have any dependencies on the gogo stuff. Maybe instead, this repo could just offer the abstraction and the golang impl. Clients of the library can then install the gogo support from another place -- maybe another repo in your Github account? This is mainly due to my hesitance to put the burden of maintaining the Gogo-related code on myself. (I know, it's selfish. Sorry.)

I can actually see the abstraction (and golang impl) being a nice separate library -- there are likely other tools and libraries that want to be able to work with both. So carving out a small "proto-runtime adapter" library could be beneficial and used by things other than just protoreflect. Perhaps that separate library could provide both golang and gogo impls in a single repo.

I'll think about it a little more.

Also, FWIW, your grpc branch is a pretty major refactor -- I think it's going to make it pretty hard to keep your fork up-to-date with this repo since rebases will be hell. Some of those changes, like breaking up the desc package into multiple smaller files, would be welcome contributions here.

[prasek]

@jhump, maintaining a separate fork for gogo tests and splitting out protoer into a separate library makes sense. If you're open to the desc refactoring I can do that in a PR as well. Once this is done the fork should be easier to maintain.

Also note in my experimental grpc branch refactor I tried to fully encapsulate golang/protobuf so it didn't leak outside of the package and only protoreflect types were exposed. To allow gogo descriptor types to be passed into funcs like CreateFileDescriptor(*dpb.FileDescriptorProto), I changed the signature to accept proto.Message and used a golang protoer to EnsureNativeMessage():
https://github.com/prasek/protoreflect/blob/grpc/desc/builder.go#L21.

EnsureNativeMessage is essentially a no op when the types are the same, but Marshals when a different package is used. This approach could be used for all inbound/outbound conversion that exposes a golang/protobuf descriptor, but for my use I don't really need the functions that would require this, so something to think about.

[jhump]

Losing type safety for APIs that currently reference golang types in exported signatures is going to be a problem. I'll have to think more about it.

In the meantime, I am happy to accept the desc refactoring -- the code re-organization parts anyway.

[prasek]

Does the general layout of the desc refactoring look good to you? If so I can do a desc refactor PR followed by a protoer PR.

[prasek]

BTW the protoer PR would only change the internal use of proto for funcs like LoadFileDescriptor, but exported signatures would stay the same for funcs like CreateFileDescriptor.

[bufdev]

I need to make sure this is understood as I’m only half following here - if protoreflect were to depend on a gogo/protobuf fork, we would have to stop using it, and I think others would feel the same. I can’t see how it’s a maintainable world going forward, gogo/protobuf is widely adopted and few could trust depending on a fork for any reason or feature. If that’s not what’s happening, carry on!

[prasek]

The intention is to have zero additional dependencies along these lines: grpc/grpc-go#1873, so protoreflect wouldn't have any dependencies on gogo/protobuf.

[bufdev]

Sounds good!

[bufdev]

Hey @prasek and @jhump

With Golang Modules getting more and more popular every day, we might want to revisit this. In the old world, with Dep, we could do:

[[constraint]]
  name = "github.com/jhump/protoreflect"
  source = "https://github.com/prasek/protoreflect"
  branch = "master"

And be fine, but with Golang Modules, there's no equivalent to this unless I'm missing something.

If we keep the fork, we might have to update all the import paths in github.com/prasek/protoreflect by doing s/jump/prasek/ and updating the go.mod file. Otherwise, if we could have a golang/gogo combined world here, that would be great, but I know @jhump was against this.

[codyaray]

So this library doesn't support proto files generated with gogo/proto? Is that what ya'll are saying?

I wish I would've realized this library made it clear that it doesn't support gogo/proto before I spent several hours trying to figure out what was going on. In my case, I wasn't even able to get dynamic.AsDynamicMessage(msg) to work.