Skip to content

Commit

Permalink
Fix buf lint not catching non repeated rules defined on repeated fiel…
Browse files Browse the repository at this point in the history
…ds of matching type (#2647)

This PR fixes the bug where `buf lint` (PROTOVALIDATE) does not catch a
non repeated rule incorrectly defined on a repeated field of matching
type. For example,
```
repeated string foo = 1 [(buf.validate.field).string.min_len = 10];
```
wasn't caught.

Also updates `bufanalysistesting.AssertFileAnnotationsEqual` for more
descriptive error messages.
  • Loading branch information
oliversun9 authored Dec 5, 2023
1 parent 4ffb9f4 commit 44af737
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
package bufanalysistesting

import (
"fmt"
"testing"

"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
"github.com/bufbuild/buf/private/bufpkg/bufmodule/bufmoduleref"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -117,16 +117,11 @@ func AssertFileAnnotationsEqual(
) {
expected = normalizeFileAnnotations(t, expected)
actual = normalizeFileAnnotations(t, actual)
assert.Equal(t, len(expected), len(actual), fmt.Sprint(actual))
if len(expected) == len(actual) {
for i, a := range actual {
e := expected[i]
expectedFileInfo := e.FileInfo()
actualFileInfo := a.FileInfo()
assert.Equal(t, expectedFileInfo, actualFileInfo)
assert.Equal(t, e, a)
}
}
assert.Equal(
t,
slicesext.Map(expected, func(annotation bufanalysis.FileAnnotation) string { return annotation.String() }),
slicesext.Map(actual, func(annotation bufanalysis.FileAnnotation) string { return annotation.String() }),
)
}

func normalizeFileAnnotations(
Expand Down
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 @@ -615,6 +615,7 @@ func TestRunProtovalidateRules(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "map.proto", 46, 5, 46, 47, "PROTOVALIDATE"),
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, "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"),
Expand Down Expand Up @@ -642,6 +643,7 @@ func TestRunProtovalidateRules(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "repeated.proto", 49, 28, 49, 71, "PROTOVALIDATE"),
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, "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"),
Expand Down
21 changes: 17 additions & 4 deletions private/bufpkg/bufcheck/buflint/internal/buflintvalidate/field.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,15 @@ func checkForField(
},
constraints,
fieldDescriptor,
fieldDescriptor.Cardinality() == protoreflect.Repeated,
)
}

func checkConstraintsForField(
adder *adder,
fieldConstraints *validate.FieldConstraints,
fieldDescriptor protoreflect.FieldDescriptor,
expectRepeatedRule bool,
) error {
if fieldConstraints == nil {
return nil
Expand Down Expand Up @@ -211,7 +213,7 @@ func checkConstraintsForField(
if typeRulesFieldNumber == repeatedRulesFieldNumber {
return checkRepeatedRules(adder, fieldConstraints.GetRepeated(), fieldDescriptor)
}
typesMatch := checkRulesTypeMatchFieldType(adder, fieldDescriptor, typeRulesFieldNumber)
typesMatch := checkRulesTypeMatchFieldType(adder, fieldDescriptor, typeRulesFieldNumber, expectRepeatedRule)
if !typesMatch {
return nil
}
Expand Down Expand Up @@ -276,7 +278,18 @@ func checkRulesTypeMatchFieldType(
adder *adder,
fieldDescriptor protoreflect.FieldDescriptor,
ruleFieldNumber int32,
expectRepeatedRule bool,
) bool {
if expectRepeatedRule {
adder.addForPathf(
[]int32{ruleFieldNumber},
"Field %q is of type repeated %s but has %s rules.",
adder.fieldName(),
adder.fieldPrettyTypeName,
adder.getFieldRuleName(ruleFieldNumber),
)
return false
}
if expectedScalarType, ok := fieldNumberToAllowedScalarType[ruleFieldNumber]; ok &&
expectedScalarType == fieldDescriptor.Kind() {
return true
Expand Down Expand Up @@ -363,7 +376,7 @@ func checkRepeatedRules(
)
}
itemAdder := baseAdder.cloneWithNewBasePath(repeatedRulesFieldNumber, itemsFieldNumberInRepeatedRules)
return checkConstraintsForField(itemAdder, repeatedRules.Items, fieldDescriptor)
return checkConstraintsForField(itemAdder, repeatedRules.Items, fieldDescriptor, false)
}

func checkMapRules(
Expand Down Expand Up @@ -401,12 +414,12 @@ func checkMapRules(
)
}
keyAdder := baseAdder.cloneWithNewBasePath(mapRulesFieldNumber, keysFieldNumberInMapRules)
err := checkConstraintsForField(keyAdder, mapRules.Keys, fieldDescriptor.MapKey())
err := checkConstraintsForField(keyAdder, mapRules.Keys, fieldDescriptor.MapKey(), false)
if err != nil {
return err
}
valueAdder := baseAdder.cloneWithNewBasePath(mapRulesFieldNumber, valuesFieldNumberInMapRules)
return checkConstraintsForField(valueAdder, mapRules.Values, fieldDescriptor.MapValue())
return checkConstraintsForField(valueAdder, mapRules.Values, fieldDescriptor.MapValue(), false)
}

func checkStringRules(adder *adder, stringRules *validate.StringRules) error {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,5 @@ message MapTest {
(buf.validate.field).map.values.int32.lt = 10,
(buf.validate.field).map.values.int32.gt = 1
];
map<int64, string> non_map_rule = 15 [(buf.validate.field).string.min_len = 1];
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,6 @@ message RepeatedTest {
map<int32, string> map_field = 16 [(buf.validate.field).map.values.repeated.unique = true];
// int64 does not match int32
int32 wrong_type = 17 [(buf.validate.field).repeated.items.int64.lt = 1];
// non repeated
repeated int32 non_repeated_rule = 18 [(buf.validate.field).int32.gt = 10];
}

0 comments on commit 44af737

Please sign in to comment.