Improve required standard constraint docs + conformance tests #124
Conversation
| oneof o { | ||
| option (buf.validate.oneof).required = true; | ||
|
|
||
| string a = 1 [(buf.validate.field).required = true]; |
There was a problem hiding this comment.
Aside: this seems like a thing to lint for, no? This effectively makes it impossible to ever use field b and the message be valid, right?
There was a problem hiding this comment.
Yep, I only included it to be exhaustive! I believe @oliversun9 is covering that in the linting work.
There was a problem hiding this comment.
Yep, this is covered in bufbuild/buf#2528
| gte: 128, | ||
| lte: 256, |
There was a problem hiding this comment.
FWIW, it was legal the other way, too. Alfred was explaining this to me the other day, that when the low and high are flipped, it effectively enforces that the value is not in the range in between (i.e. "gte" and "lte" end up combined with OR instead of AND).
There was a problem hiding this comment.
That's correct, however in context of testing the ignore_empty constraint, having it the other way around made the ignore_empty uninteresting (0 would always be valid in the constraints)
| } | ||
|
|
||
| message RequiredProto2ScalarRequired { | ||
| required string val = 1 [(buf.validate.field).required = true]; |
There was a problem hiding this comment.
Intentional to use required label on this one? With this here, you can't even deserialize the message if the field absent (so we'd never even get to the validate call).
There was a problem hiding this comment.
Correct, this is mainly to be exhaustive. At least in protobuf-go, you can set the (un)marshaler options to allow partials and let unset required fields to pass through.
Following #124, this adds conformance tests that ensure the `ignore_empty` rules is consistent with `required`. Mainly, if a field cannot differentiate between unset and the zero value (i.e., is not nullable), then this rule applies; effectively this is just repeated and map fields, as well as non-optional proto3 scalar outside of a oneof. Running against `protovalidate-go`, a few places where `ignore_empty` should be a noop ends up disabling evaluations. This will be a minor follow-up fix in that library. Patches for the other libraries will follow to bring them into conformance with these tests and those in #124.
With #124 et al, `ignore_empty` semantics have been made consistent with `required`, but it's still limiting (and confusing) in situations where skipping validation rules is still desired when a field is either unpopulated _or_ set to the default value (for instance, within an update request that allows conditionally updating a field on a resource that is itself a nullable field). This patch deprecates `ignore_empty`, expanding into `ignore` which is now an enum that supports the original semantics, as well as this expanded~looser definition. `ignore_empty` will be removed from the proto with a v1 release. Conformance + examples will be added after this is released (due to the two-phased managed module) --------- Co-authored-by: Edward McFarlane <3036610+emcfarlane@users.noreply.github.com>
This patch improves behavioral documentation of the
OneOfConstraints.required,FieldConstraints.required, andFieldConstraints.ignore_emptystandard constraints. The conformance tests have also been expanded to check the behavior forrequired(withignore_emptyto follow in a follow up).Local conformance testing has verified that the current
protovalidate-gobehavior matches the expectations of these constraints.Partially addresses: #115