Skip to content

Commit

Permalink
Add check for required on oneof fields in buf lint (#2528)
Browse files Browse the repository at this point in the history
This adds a check in `PROTOVALIDATE` rules in `buf lint` that checks
oneof fields do not have `(buf.validate.field).required`.
  • Loading branch information
oliversun9 authored Nov 2, 2023
1 parent dad51a5 commit 5b912f4
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 0 deletions.
2 changes: 2 additions & 0 deletions private/bufpkg/bufcheck/buflint/buflint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,8 @@ func TestRunProtovalidateRules(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "number.proto", 134, 5, 134, 56, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "number.proto", 139, 5, 139, 50, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "number.proto", 142, 5, 142, 52, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 13, 7, 13, 43, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "oneof.proto", 19, 7, 19, 43, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 25, 5, 25, 48, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 27, 5, 27, 48, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 45, 5, 45, 48, "PROTOVALIDATE"),
Expand Down
10 changes: 10 additions & 0 deletions private/bufpkg/bufcheck/buflint/internal/buflintvalidate/field.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,16 @@ func checkConstraintsForField(
if fieldDescriptor.IsExtension() {
checkConstraintsForExtension(adder, fieldConstraints)
}
if fieldDescriptor.ContainingOneof() != nil && fieldConstraints.GetRequired() {
adder.addForPathf(
[]int32{requiredFieldNumber},
"Field %q has %s but is in a oneof (%s). Oneof fields must not have %s.",
adder.fieldName(),
adder.getFieldRuleName(requiredFieldNumber),
fieldDescriptor.ContainingOneof().Name(),
adder.getFieldRuleName(requiredFieldNumber),
)
}
checkFieldFlags(adder, fieldConstraints)
if err := checkCELForField(
adder,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
syntax = "proto3";

package what;

import "buf/validate/validate.proto";
import "google/protobuf/duration.proto";

message OneofTest {
oneof foo {
int32 v1 = 1 [
(buf.validate.field).int32.lt = 3,
// must not have required
(buf.validate.field).required = true
];
string v2 = 2;
google.protobuf.Duration v3 = 3 [
(buf.validate.field).duration.gt = {seconds: 5},
// must not have required
(buf.validate.field).required = true,
(buf.validate.field).duration.lt = {seconds: 10}
];
}
// required is OK for non-oneof fields
string f4 = 4 [(buf.validate.field).required = true];
}

0 comments on commit 5b912f4

Please sign in to comment.