bufbuild / protovalidate-go

Protocol Buffer Validation for Go
https://pkg.go.dev/github.com/bufbuild/protovalidate-go
Apache License 2.0
286 stars 19 forks source link

Make constraint resolution more flexible to different concrete extension types #57

Closed rodaine closed 1 year ago

rodaine commented 1 year ago

The go proto library struggles to resolve values for extensions if the concrete type associated with the generated source code on hand does not match that on the descriptor of the message in question (which happens in cases where the descriptor or message is produced at runtime). This manifested in the protovalidate resolver where the extension could not be found on options attached to a descriptor produced dynamically.

Unfortunately, I struggled to find a way to get the extension and its value directly. (I'm not even sure how to get the field/ext descriptor from the descriptor; protoreflect.Fields.ByNumber doesn't work). The most straightforward solution from a few iterations was to leverage proto.RangeExtensions.

There's room here for some mild optimization to short circuit in most vanilla situations but it makes it a bit less readable. Since this only happens once per descriptor, the optimization is overkill IMO.

jhump commented 1 year ago

Unfortunately, I struggled to find a way to get the extension and its value directly. (I'm not even sure how to get the field/ext descriptor from the descriptor; protoreflect.Fields.ByNumber doesn't work). The most straightforward solution from a few iterations was to leverage proto.RangeExtensions.

I think proto.RangeExtensions is fine. The alternative is to use options.ProtoReflect().Range(...), which is similar but gives you the value as a protoreflect.Value instead of any. (Not really an improvement.)

There is unfortunately no better way -- extensions are by their nature decoupled from the extended message. So the message's schema will have no knowledge of the extensions other than their ranges. protoreflect.Fields is only for directly declared non-extension fields.

Libraries typically should just use proto.{Has,Get}Extension or protoreflect.Message.{Has,Get}, as was done here before. This is the usually the appropriate way to implement this, and is why I was suggesting perhaps the complexity of dealing with dynamic schemas be pushed to the producer of the descriptors (i.e. the buf CLI), instead of having to deal with it in every library that might consume those descriptors (like this one).

In this case, since you don't have the actual extension descriptor used to populate the options, all you have is the field number, this does indeed look like the best way to handle it.