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

APIv2: ability to distinguish truly invalid proto.Message values? #1013

Open
dsnet opened this issue Jan 13, 2020 · 3 comments
Open

APIv2: ability to distinguish truly invalid proto.Message values? #1013

dsnet opened this issue Jan 13, 2020 · 3 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Jan 13, 2020

Should there be a way for protobuf reflection to distinguish proto.Message values that are truly invalid?

Currently, we have protoreflect.Message.IsValid which indicates that the current Go value is functionally a read-only empty message. This implies that the current value has protobuf type information (i.e., Descriptor and Type methods are callable).

However, there are a number of Go types that possess a higher level of "invalid"-ness. For example, the zero value of dynamicpb.Message and ptypes.DynamicAny obviously cannot contain protobuf type information since they are both concrete Go types that may may represent any arbitrary protobuf message type.

The issue today is that a user of protobuf reflection cannot distinguish such messages and risk panicking whenever they call the methods on protoreflect.Message. For example, protocmp.Transform will choke on the zero value of dynamicpb.Message.

Some possibilities (some or all of these may be done) regarding truly invalid values:

  • Document that calling proto.Message.ProtoReflect on returns nil.
  • Document that calling protoreflect.Message.Type or protoreflect.Message.Descriptor returns nil.

(I define "truly invalid" as a concrete value that has no sensible protobuf message type associated with it at the present moment.)

@dsnet

This comment has been minimized.

Copy link
Member Author

@dsnet dsnet commented Jan 13, 2020

\cc @neild

@neild

This comment has been minimized.

Copy link
Contributor

@neild neild commented Jan 13, 2020

For dynamicpb.Message, we could define the zero value of the type as a read-only google.protobuf.Empty. Perhaps something similar for ptypes.DynamicAny.

@puellanivis

This comment has been minimized.

Copy link

@puellanivis puellanivis commented Jan 13, 2020

😕 hm… defining special magic values for these otherwise–invalid values means that they would become part of the public API, and once anyone ends up relying on that behavior, if there is any attempt to change the semantics, then we would break those that depends upon the semantics we set now.

I’m all for setting it to be documented undefined behavior, or otherwise stating that it’s otherwise invalid, and that it returns a non–useful value, then we have much more leeway to change it to a useful secondary–semantic like google.protobuf.Empty later, when we know this would actually be a useful and appropriate semantic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.