bufbuild / protovalidate

Protocol Buffer Validation - Go, Java, Python, and C++ Beta Releases!
https://buf.build/bufbuild/protovalidate
Apache License 2.0
908 stars 37 forks source link

[BUG] field constraints exhibit differeint behaviour in oneofs #244

Closed anu-tiernan closed 2 months ago

anu-tiernan commented 2 months ago

In most contexts (buf.validate.oneof).required and (buf.validate.field).string.min_len = 1 would achieve the same result for a string field.

In a oneof however required is always checked on all fields including not selected fields while string.min_len is only checked for the selected field which is unexpected.

For example:

message Test {
  oneof id {
    option (buf.validate.oneof).required = true;
    string id1 = 1 [(buf.validate.field).required = true];
    string id2 = 2 [(buf.validate.field).required = true];
  }
}

My expected interpretation of this is that you must set either id1 or id2 in the id oneof, and that the selected id can not be empty matching the semantic if the selected field was not a part of the oneof while the non-selected field has no validators run against it.

Further adding to the confusion, required is described as proto3 scalar fields must be non-zero to be considered populated, which for a string means it can't be a zero length string, but once it's in a oneof an empty string will happily validate fine when the field is marked as required.

I'm not 100% sure if this is an implementation bug in protovalidate-go, or if it's working as intended and maybe just the protovalidate docs need updating to better clarify the expected behaviour. From my perspective having the semantics remain the name for a field in a oneof vs outside of a oneof would be the least surprising.

rodaine commented 2 months ago

Hi @anu-tiernan! Transferred this to the protovalidate project since it's not Go specific. The short of it is, this is behaving as expected. Check out https://github.com/bufbuild/protovalidate/issues/228 for the specifics. If you have any ideas for improving the documentation to make this more clear, please feel free to open a PR.