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

Improve required standard constraint docs + conformance tests #124

Merged
merged 1 commit into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ proto_library(
"numbers.proto",
"oneofs.proto",
"repeated.proto",
"required_field_proto2.proto",
"required_field_proto3.proto",
"strings.proto",
"wkt_any.proto",
"wkt_duration.proto",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ message OneofRequired {
}
}

message OneofRequiredWithRequiredField {
oneof o {
option (buf.validate.oneof).required = true;

string a = 1 [(buf.validate.field).required = true];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I only included it to be exhaustive! I believe @oliversun9 is covering that in the linting work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is covered in bufbuild/buf#2528

string b = 2;
}
}

message OneofIgnoreEmpty {
oneof o {
string x = 1 [
Expand All @@ -66,8 +75,8 @@ message OneofIgnoreEmpty {
];
int32 z = 3 [
(buf.validate.field).int32 = {
lte: 128,
gte: 256,
gte: 128,
lte: 256,
Comment on lines +78 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

},
(buf.validate.field).ignore_empty = true
];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2023 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto2";

package buf.validate.conformance.cases;

import "buf/validate/validate.proto";

message RequiredProto2ScalarOptional {
optional string val = 1 [(buf.validate.field).required = true];
}

message RequiredProto2ScalarOptionalDefault {
optional string val = 1 [
(buf.validate.field).required = true,
default = "foo"
];
}

message RequiredProto2ScalarRequired {
required string val = 1 [(buf.validate.field).required = true];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

message RequiredProto2Message {
optional Msg val = 1 [(buf.validate.field).required = true];
message Msg {
optional string val = 1;
}
}

message RequiredProto2Oneof {
oneof val {
string a = 1 [(buf.validate.field).required = true];
string b = 2;
}
}

message RequiredProto2Repeated {
repeated string val = 1 [(buf.validate.field).required = true];
}

message RequiredProto2Map {
map<string, string> val = 1 [(buf.validate.field).required = true];
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2023 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package buf.validate.conformance.cases;

import "buf/validate/validate.proto";

message RequiredProto3Scalar {
string val = 1 [(buf.validate.field).required = true];
}

message RequiredProto3OptionalScalar {
optional string val = 1 [(buf.validate.field).required = true];
}

message RequiredProto3Message {
Msg val = 1 [(buf.validate.field).required = true];
message Msg {
string val = 1;
}
}

message RequiredProto3OneOf {
oneof val {
string a = 1 [(buf.validate.field).required = true];
string b = 2;
}
}

message RequiredProto3Repeated {
repeated string val = 1 [(buf.validate.field).required = true];
}

message RequiredProto3Map {
map<string, string> val = 1 [(buf.validate.field).required = true];
}
50 changes: 30 additions & 20 deletions proto/protovalidate/buf/validate/validate.proto
Original file line number Diff line number Diff line change
Expand Up @@ -91,21 +91,22 @@ message MessageConstraints {
}

// The `OneofConstraints` message type enables you to manage constraints for
// oneof fields in your protobuf messages. Use the `required` constraint to ensure
// that exactly one of the fields within a oneof is set; validation will fail
// if none of the fields in the oneof are set:
// oneof fields in your protobuf messages.
message OneofConstraints {
// `required` is an optional boolean attribute that ensures that
// exactly one of the field options in a oneof is set; validation fails if
// no fields in the oneof are set.
// If `required` is true, exactly one field of the oneof must be present. A
// validation error is returned if no fields in the oneof are present. The
// field itself may still be a default value; further constraints
// should be placed on the fields themselves to ensure they are valid values,
// such as `min_len` or `gt`.
//
// ```proto
// message MyMessage {
// oneof value {
// // The field `a` or `b` must be set.
// // Either `a` or `b` must be set. If `a` is set, it must also be
// // non-empty; whereas if `b` is set, it can still be an empty string.
// option (buf.validate.oneof).required = true;
// optional string a = 1;
// optional string b = 2;
// string a = 1 [(buf.validate.field).string.min_len = 1];
// string b = 2;
// }
// }
// ```
Expand Down Expand Up @@ -141,28 +142,37 @@ message FieldConstraints {
// }
// ```
bool skipped = 24;
// `required` is an optional boolean attribute that specifies that
// this field must be set. If required is set to true, the field value must
// not be empty; otherwise, an error message will be generated.
// If `required` is true, the field must be populated. Field presence can be
// described as "serialized in the wire format," which follows the following rules:
//
// Note that `required` validates that `repeated` fields are non-empty, that is
// setting a `repeated` field as `required` is equivalent to `repeated.min_items = 1`.
// - the following "nullable" fields must be explicitly set to be considered present:
// - singular message fields (may be their empty value)
// - member fields of a oneof (may be their default value)
// - proto3 optional fields (may be their default value)
// - proto2 scalar fields
// - proto3 scalar fields must be non-zero to be considered present
// - repeated and map fields must be non-empty to be considered present
//
// ```proto
// message MyMessage {
// // The field `value` must be set.
// // The field `value` must be set to a non-null value.
// optional MyOtherMessage value = 1 [(buf.validate.field).required = true];
// }
// ```
bool required = 25;
// `ignore_empty` specifies that the validation rules of this field should be
// evaluated only if the field isn't empty. If the field is empty, no validation
// rules are applied.
// If `ignore_empty` is true and applied to a non-nullable field (see
// `required` for more details), validation is skipped on the field if it is
// the default or empty value. Adding `ignore_empty` to a "nullable" field is
// a noop as these unset fields already skip validation (with the exception
// of `required`).
//
// ```proto
// message MyRepeated {
// // The field `value` validation rules should be evaluated only if the field isn't empty.
// repeated string value = 1 [(buf.validate.field).ignore_empty = true];
// // The field `value` min_len rule is only applied if the field isn't empty.
// repeated string value = 1 [
// (buf.validate.field).ignore_empty = true,
// (buf.validate.field).min_len = 5
// ];
// }
// ```
bool ignore_empty = 26;
Expand Down
Loading