Add test cases for examples and fix bugs found

bufbuild · Sep 27, 2024 · eae0426 · eae0426
1 parent 75898ac
commit eae0426
Show file tree

Hide file tree

Showing 12 changed files with 400 additions and 16 deletions.
diff --git a/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/buflintvalidate.go
@@ -58,8 +58,6 @@ func CheckMessage(
 // For a set of rules to be valid, it must
 //  1. permit _some_ value and all example values, if any
 //  2. have a type compatible with the field it validates.
-//
-// This also checks all predefined rule extensions fields to ensure they compile.
 func CheckField(
 	// addAnnotationFunc adds an annotation with the descriptor and location for check results.
 	addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),
@@ -69,6 +67,7 @@ func CheckField(
 	return checkField(addAnnotationFunc, field, extensionTypeResolver)
 }
 
+// CheckPredefinedRuleExtension checks that a predefined extension is valid, and any CEL expressions compile.
 func CheckPredefinedRuleExtension(
 	// addAnnotationFunc adds an annotation with the descriptor and location for check results.
 	addAnnotationFunc func(bufprotosource.Descriptor, bufprotosource.Location, []bufprotosource.Location, string, ...interface{}),

diff --git a/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/field.go b/private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/field.go
@@ -795,18 +795,28 @@ func checkExampleValues(
 	// and validate this message instance with protovalidate and filter the structured
 	// errors by field name to determine whether this example value fails rules defined
 	// on the same field.
-	v, err := protovalidate.New(
-		protovalidate.WithStandardConstraintInterceptor(
-			func(res protovalidate.StandardConstraintResolver) protovalidate.StandardConstraintResolver {
-				// Pass a constraint resolver interceptor so that constraints on other
-				// fields are not looked at by the validator.
-				return &constraintsResolverForTargetField{
-					StandardConstraintResolver: res,
-					targetField:                fieldDescriptor,
-				}
-			},
-		),
-	)
+	//
+	// Pass a constraint resolver interceptor so that constraints on other
+	// fields are not looked at by the validator.
+	constraintInterceptor := func(res protovalidate.StandardConstraintResolver) protovalidate.StandardConstraintResolver {
+		return &constraintsResolverForTargetField{
+			StandardConstraintResolver: res,
+			targetField:                fieldDescriptor,
+		}
+	}
+	// For map fields, we want to resolve the constraints on the parentMapFieldDescriptor rather
+	// than the MapEntry.
+	if parentMapFieldDescriptor != nil {
+		constraintInterceptor = func(res protovalidate.StandardConstraintResolver) protovalidate.StandardConstraintResolver {
+			// Pass a constraint resolver interceptor so that constraints on other
+			// fields are not looked at by the validator.
+			return &constraintsResolverForTargetField{
+				StandardConstraintResolver: res,
+				targetField:                parentMapFieldDescriptor,
+			}
+		}
+	}
+	validator, err := protovalidate.New(protovalidate.WithStandardConstraintInterceptor(constraintInterceptor))
 	if err != nil {
 		return err
 	}
@@ -878,10 +888,92 @@ func checkExampleValues(
 				return syserror.Newf("expected key or value as sythetic field name for map entry's field name, got %q", fieldDescriptor.Name())
 			}
 			messageToValidate.Set(parentMapFieldDescriptor, protoreflect.ValueOfMap(mapEntry))
+		case fieldDescriptor.Enum() != nil:
+			// We need to handle enum examples in a special way, since enum examples are set as
+			// int32, but we need to set it to the enum value to the field.
+			// So we cast exampleValue to an int32 and check that cast first before attempting
+			// to set it to the message field. This is because messageToValidate.Set will panic
+			// if an invalid type is attempted to be set.
+			exampleInt32, ok := exampleValue.Interface().(int32)
+			if !ok {
+				return syserror.Newf("expected enum example value to be int32 for field %q, got %T type instead", fieldDescriptor.FullName(), exampleValue)
+			}
+			messageToValidate.Set(fieldDescriptor, protoreflect.ValueOf(protoreflect.EnumNumber(exampleInt32)))
+		case fieldDescriptor.Message() != nil:
+			// We need to handle the case where the field is a wrapper type. We set the value directly base on the wrapper type.
+			switch string(fieldDescriptor.Message().FullName()) {
+			case string((&wrapperspb.FloatValue{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.FloatValue{Value: exampleValue.Interface().(float32)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.DoubleValue{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.DoubleValue{Value: exampleValue.Interface().(float64)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.Int32Value{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.Int32Value{Value: exampleValue.Interface().(int32)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.Int64Value{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.Int64Value{Value: exampleValue.Interface().(int64)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.UInt32Value{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.UInt32Value{Value: exampleValue.Interface().(uint32)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.UInt64Value{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.UInt64Value{Value: exampleValue.Interface().(uint64)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.BoolValue{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.BoolValue{Value: exampleValue.Interface().(bool)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.StringValue{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.StringValue{Value: exampleValue.Interface().(string)}).ProtoReflect(),
+					),
+				)
+			case string((&wrapperspb.BytesValue{}).ProtoReflect().Descriptor().FullName()):
+				messageToValidate.Set(
+					fieldDescriptor,
+					protoreflect.ValueOf(
+						(&wrapperspb.BytesValue{Value: exampleValue.Interface().([]byte)}).ProtoReflect(),
+					),
+				)
+			default:
+				// In the case where it is not a wrapper type (e.g. google.protobuf.Timestamp), we just set the example
+				// value directly.
+				messageToValidate.Set(fieldDescriptor, exampleValue)
+			}
 		default:
 			messageToValidate.Set(fieldDescriptor, exampleValue)
 		}
-		err := v.Validate(messageToValidate)
+		err := validator.Validate(messageToValidate)
 		if err == nil {
 			continue
 		}

diff --git a/private/bufpkg/bufcheck/lint_test.go b/private/bufpkg/bufcheck/lint_test.go
@@ -597,9 +597,11 @@ func TestRunProtovalidate(t *testing.T) {
 		bufanalysistesting.NewFileAnnotation(t, "bool.proto", 18, 51, 18, 84, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "bool.proto", 19, 31, 19, 69, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "bool.proto", 20, 50, 20, 88, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "bool.proto", 27, 5, 27, 46, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "bytes.proto", 21, 5, 21, 48, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "bytes.proto", 26, 5, 26, 45, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "bytes.proto", 31, 5, 31, 45, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "bytes.proto", 43, 5, 43, 65, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 10, 37, 14, 4, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 17, 5, 21, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 29, 5, 33, 6, "PROTOVALIDATE"),
@@ -633,7 +635,9 @@ func TestRunProtovalidate(t *testing.T) {
 		bufanalysistesting.NewFileAnnotation(t, "duration.proto", 105, 5, 108, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "duration.proto", 122, 5, 125, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "duration.proto", 127, 5, 130, 6, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "duration.proto", 155, 5, 158, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "enum.proto", 28, 5, 28, 40, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "enum.proto", 36, 5, 36, 42, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "extension.proto", 25, 7, 25, 43, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "extension.proto", 30, 7, 30, 47, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "extension.proto", 40, 5, 40, 41, "PROTOVALIDATE"),
@@ -650,6 +654,7 @@ func TestRunProtovalidate(t *testing.T) {
 		bufanalysistesting.NewFileAnnotation(t, "map.proto", 50, 5, 50, 57, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "map.proto", 53, 5, 53, 50, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "map.proto", 56, 41, 56, 80, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "map.proto", 69, 5, 69, 53, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "message.proto", 20, 3, 20, 49, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "message.proto", 27, 5, 27, 51, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "number.proto", 20, 5, 20, 42, "PROTOVALIDATE"),
@@ -670,6 +675,21 @@ func TestRunProtovalidate(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, "number.proto", 160, 5, 160, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 170, 5, 170, 45, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 180, 5, 180, 45, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 190, 5, 190, 43, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 200, 5, 200, 43, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 210, 5, 210, 46, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 220, 5, 220, 46, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 230, 5, 230, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 240, 5, 240, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 250, 5, 250, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 260, 5, 260, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 270, 5, 270, 43, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 280, 5, 280, 43, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 290, 5, 290, 44, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "number.proto", 300, 5, 300, 44, "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"),
@@ -679,6 +699,7 @@ func TestRunProtovalidate(t *testing.T) {
 		bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 51, 38, 51, 92, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 53, 26, 53, 74, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 55, 42, 55, 76, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 65, 5, 65, 62, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 31, 5, 31, 46, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 36, 5, 36, 44, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 41, 5, 41, 44, "PROTOVALIDATE"),
@@ -705,6 +726,8 @@ func TestRunProtovalidate(t *testing.T) {
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 122, 5, 122, 47, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 130, 5, 130, 46, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "string.proto", 133, 5, 133, 45, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "string.proto", 152, 5, 152, 51, "PROTOVALIDATE"),
+		bufanalysistesting.NewFileAnnotation(t, "string.proto", 154, 5, 154, 49, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "timestamp.proto", 57, 5, 60, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "timestamp.proto", 61, 5, 64, 6, "PROTOVALIDATE"),
 		bufanalysistesting.NewFileAnnotation(t, "timestamp.proto", 68, 5, 71, 6, "PROTOVALIDATE"),

diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/bool.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/bool.proto
@@ -18,4 +18,12 @@ message BoolTest {
   google.protobuf.BoolValue mismatch_wrapper = 8 [(buf.validate.field).int32.lt = 1];
   string string_mismatch = 9 [(buf.validate.field).bool.const = true];
   google.protobuf.Int32Value wrong_wrapper = 10 [(buf.validate.field).bool.const = true];
+  bool valid_example = 4 [
+    (buf.validate.field).bool.const = true,
+    (buf.validate.field).bool.example = true
+  ];
+  bool invalid_example = 5 [
+    (buf.validate.field).bool.const = true,
+    (buf.validate.field).bool.example = false
+  ];
 }
diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/bytes.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/bytes.proto
@@ -30,4 +30,16 @@ message BytesTest {
     (buf.validate.field).bytes.len = 1,
     (buf.validate.field).bytes.pattern = "["
   ];
+  bytes valid_example = 9 [
+    (buf.validate.field).bytes.prefix = "ÀÀÀÀÀ",
+    (buf.validate.field).bytes.example = "ÀÀÀÀÀÇÇÇÇÇÇÇÇÅÅÅÅÅÅÅÅ"
+  ];
+  bytes valid_and_invalid_example = 10 [
+    (buf.validate.field).bytes.prefix = "ÀÀÀÀÀ",
+    (buf.validate.field).bytes.max_len = 17,
+    // valid
+    (buf.validate.field).bytes.example = "ÀÀÀÀÀÀÀÀ",
+    // invalid, fails one of the rules
+    (buf.validate.field).bytes.example = "ÀÀÀÀÀÇÇÇÇÇÇÇÇÅÅÅÅÅÅÅÅ"
+  ];
 }
diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/duration.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/duration.proto
@@ -129,4 +129,32 @@ message DurationTest {
       nanos: 854775429
     }
   ];
+  google.protobuf.Duration valid_example = 20 [
+    (buf.validate.field).duration.lt = {
+      seconds: 17,
+      nanos: 25
+    },
+    (buf.validate.field).duration.gt = {
+      seconds: 5,
+      nanos: 1
+    },
+    (buf.validate.field).duration.example = {
+      seconds: 7,
+      nanos: 3
+    }
+  ];
+  google.protobuf.Duration invalid_example = 21 [
+    (buf.validate.field).duration.lt = {
+      seconds: 17,
+      nanos: 25
+    },
+    (buf.validate.field).duration.gt = {
+      seconds: 5,
+      nanos: 1
+    },
+    (buf.validate.field).duration.example = {
+      seconds: 2,
+      nanos: 3
+    }
+  ];
 }
diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/enum.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/enum.proto
@@ -27,4 +27,12 @@ message EnumTest {
     // const should be the only field
     (buf.validate.field).enum.const = 2
   ];
+  TestEnum valid_example = 6 [
+    (buf.validate.field).enum.defined_only = true,
+    (buf.validate.field).enum.example = 1
+  ];
+  TestEnum invalid_example = 7 [
+    (buf.validate.field).enum.defined_only = true,
+    (buf.validate.field).enum.example = 4
+  ];
 }
diff --git a/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/map.proto b/private/bufpkg/bufcheck/testdata/lint/protovalidate/proto/map.proto
@@ -54,4 +54,18 @@ message MapTest {
     (buf.validate.field).map.values.int32.gt = 1
   ];
   map<int64, string> non_map_rule = 15 [(buf.validate.field).string.min_len = 1];
+  map<int64, string> valid_keys_example = 11 [
+    (buf.validate.field).map.keys.int64.gt = 1,
+    (buf.validate.field).map.keys.int64.lt = 10,
+    (buf.validate.field).map.values.string.min_len = 1,
+    (buf.validate.field).map.values.string.max_len = 10,
+    (buf.validate.field).map.keys.int64.example = 5
+  ];
+  map<int64, string> invalid_keys_example = 12 [
+    (buf.validate.field).map.keys.int64.gt = 1,
+    (buf.validate.field).map.keys.int64.lt = 10,
+    (buf.validate.field).map.values.string.min_len = 1,
+    (buf.validate.field).map.values.string.max_len = 10,
+    (buf.validate.field).map.keys.int64.example = -1
+  ];
 }