support custom message validation in JSONPB when checking for required fields

[jhump]

Pull request #472 added new checks to ensure that messages are valid -- e.g. any field that is required is actually present.

I'm filing this issue primarily to support dynamic message implementations (such as this, which works fine in master, but does not work in dev).

Attempts to use jsonpb to marshal/unmarshal a dynamic message can now result in a panic:

panic: reflect.Value.Interface: cannot return value obtained from unexported field or method 

This happens if the message struct contains any unexported pointer values that are non-nil (including slice or map fields that contain a non-nil pointer element). When examining a message, it uses val.Interface().(proto.Message) to see if the pointer is a nested message which needs to be recursively checked.

1. I would like to be able to customize this check, for example by having the message provide a ValidateRecursive() method which can be called instead of using the default check.
2. It would be really nice if this check (as implemented, for generated messages) were an exported function in the proto package, such as:

proto.CheckRequiredFields(proto.Message) error

I have a fix that addresses the first bullet.

The second bullet would, of course, allow jsonpb to share code with the proto package for checking that required fields are present. But it would also make it easier for messages to implement ValidateRecursive() since they could call this exported function to apply default logic (if applicable), and they could also use it to apply normal validation for nested message fields.

[dsnet]

Adding ValidateRecursive would work in the short term, but it seems to be just a band-aid on a bigger problem. I believe that #364 is the proper solution for this. There needs to be a agreed-upon API for all messages can interact with the proto and jsonpb without baking in ad-hoc special cases.

I know I have been saying #364 is the solution for many problems, and I truly believe it is. I'm still working on the API design, and it's really tricky to get the details right with all the edge cases of the protobuf specification.

The being said, modifying the logic of jsonpb to at least not panic is fine.

[jhump]

Adding ValidateRecursive would work in the short term, but it seems to be just a band-aid on a bigger problem.

I agree. And a behaviorally complete message interface would be awesome!

I suppose an alternate resolution might be to just have jsonpb, when performing this check, ignore unexported fields and fields that don't have a protobuf tag (as is likely to happen with non-generated message implementations).

I think that would effectively make this check a no-op when marshaling and unmarshaling my dynamic message implementation. (My implementation already does this check internally anyway.)

[dsnet]

I suppose an alternate resolution might be to just have jsonpb, when performing this check, ignore unexported fields and fields that don't have a protobuf tag (as is likely to happen with non-generated message implementations).

A fix of this nature sounds fine. Want to send a PR taking that approach?

[jhump]

A fix of this nature sounds fine. Want to send a PR taking that approach?

Sure

[dsnet]

#510 should restore the prior behavior. We'll let #364 be the long term solution.