From 0d6dccb7617818353e1dabee908645fb08c5c00c Mon Sep 17 00:00:00 2001 From: Andrew Paseltiner Date: Mon, 22 Mar 2021 13:31:58 -0400 Subject: [PATCH] fix: Check field cardinality for AIP-132, AIP-133, AIP-134, AIP-154, AIP-158, AIP-163 (#803) --- rules/aip0132/request_field_types.go | 15 +----- rules/aip0132/request_field_types_test.go | 46 +++++++++---------- rules/aip0133/request_id_field.go | 4 +- rules/aip0133/request_id_field_test.go | 1 + rules/aip0134/request_allow_missing_field.go | 4 +- .../request_allow_missing_field_test.go | 5 +- rules/aip0134/request_mask_field.go | 4 +- rules/aip0134/request_mask_field_test.go | 3 +- rules/aip0154/field_type.go | 4 +- rules/aip0154/field_type_test.go | 3 +- rules/aip0158/request_page_size_field.go | 4 +- .../aip0163/declarative_friendly_required.go | 4 +- .../declarative_friendly_required_test.go | 1 + 13 files changed, 44 insertions(+), 54 deletions(-) diff --git a/rules/aip0132/request_field_types.go b/rules/aip0132/request_field_types.go index 259986c05..7e6a256aa 100644 --- a/rules/aip0132/request_field_types.go +++ b/rules/aip0132/request_field_types.go @@ -15,8 +15,6 @@ package aip0132 import ( - "fmt" - "bitbucket.org/creachadair/stringset" "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/rules/internal/utils" @@ -25,20 +23,11 @@ import ( var knownFields = stringset.New("filter", "order_by") -// List methods should not have unrecognized fields. +// List request filter and order_by fields should be singular strings. var requestFieldTypes = &lint.FieldRule{ Name: lint.NewRuleName(132, "request-field-types"), OnlyIf: func(f *desc.FieldDescriptor) bool { return isListRequestMessage(f.GetOwner()) && knownFields.Contains(f.GetName()) }, - LintField: func(f *desc.FieldDescriptor) (problems []lint.Problem) { - // Establish that the field being checked is a string. - if utils.GetTypeName(f) != "string" { - return []lint.Problem{{ - Message: fmt.Sprintf("Field %q should be a string.", f.GetName()), - Descriptor: f, - }} - } - return - }, + LintField: utils.LintSingularStringField, } diff --git a/rules/aip0132/request_field_types_test.go b/rules/aip0132/request_field_types_test.go index a5ba4e441..f5f51ae20 100644 --- a/rules/aip0132/request_field_types_test.go +++ b/rules/aip0132/request_field_types_test.go @@ -18,42 +18,38 @@ import ( "testing" "github.com/googleapis/api-linter/rules/internal/testutils" - "github.com/jhump/protoreflect/desc/builder" ) func TestRequestFieldTypes(t *testing.T) { // Set up the testing permutations. tests := []struct { - testName string - messageName string - fieldName string - fieldType *builder.FieldType - problems testutils.Problems + testName string + Message string + Field string + problems testutils.Problems }{ - {"Filter", "ListBooksRequest", "filter", builder.FieldTypeString(), testutils.Problems{}}, - {"FilterInvalid", "ListBooksRequest", "filter", builder.FieldTypeBytes(), testutils.Problems{{Message: "string"}}}, - {"OrderBy", "ListBooksRequest", "order_by", builder.FieldTypeString(), testutils.Problems{}}, - {"OrderByInvalid", "ListBooksRequest", "order_by", builder.FieldTypeBytes(), testutils.Problems{{Message: "string"}}}, + {"Filter", "ListBooksRequest", "string filter", nil}, + {"FilterInvalid", "ListBooksRequest", "bytes filter", testutils.Problems{{Message: "singular string", Suggestion: "string"}}}, + {"FilterInvalidRepeated", "ListBooksRequest", "repeated string filter", testutils.Problems{{Message: "singular string", Suggestion: "string"}}}, + {"OrderBy", "ListBooksRequest", "string order_by", nil}, + {"OrderByInvalid", "ListBooksRequest", "bytes order_by", testutils.Problems{{Message: "singular string", Suggestion: "string"}}}, + {"OrderByInvalidRepeated", "ListBooksRequest", "repeated string order_by", testutils.Problems{{Message: "singular string", Suggestion: "string"}}}, + {"IrrelevantMessage", "Book", "bytes order_by", nil}, + {"IrrelevantField", "ListBooksRequest", "bytes foo", nil}, } // Run each test individually. for _, test := range tests { t.Run(test.testName, func(t *testing.T) { - // Create an appropriate message descriptor. - message, err := builder.NewMessage(test.messageName).AddField( - builder.NewField("parent", builder.FieldTypeString()), - ).AddField( - builder.NewField(test.fieldName, test.fieldType), - ).Build() - if err != nil { - t.Fatalf("Could not build GetBookRequest message.") - } - - // Run the lint rule, and establish that it returns the correct - // number of problems. - problems := requestFieldTypes.Lint(message.GetFile()) - if diff := test.problems.SetDescriptor(message.GetFields()[1]).Diff(problems); diff != "" { - t.Errorf(diff) + f := testutils.ParseProto3Tmpl(t, ` + message {{.Message}} { + {{.Field}} = 1; + } + `, test) + field := f.GetMessageTypes()[0].GetFields()[0] + problems := requestFieldTypes.Lint(f) + if diff := test.problems.SetDescriptor(field).Diff(problems); diff != "" { + t.Error(diff) } }) } diff --git a/rules/aip0133/request_id_field.go b/rules/aip0133/request_id_field.go index 4e643636e..1dd5e1334 100644 --- a/rules/aip0133/request_id_field.go +++ b/rules/aip0133/request_id_field.go @@ -31,9 +31,9 @@ var requestIDField = &lint.MessageRule{ }, LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { idField := strcase.SnakeCase(strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "Create")) + "_id" - if field := m.FindFieldByName(idField); field == nil || utils.GetTypeName(field) != "string" { + if field := m.FindFieldByName(idField); field == nil || utils.GetTypeName(field) != "string" || field.IsRepeated() { return []lint.Problem{{ - Message: fmt.Sprintf("Declarative-friendly create methods should contain a `string %s` field.", idField), + Message: fmt.Sprintf("Declarative-friendly create methods should contain a singular `string %s` field.", idField), Descriptor: m, }} } diff --git a/rules/aip0133/request_id_field_test.go b/rules/aip0133/request_id_field_test.go index 996ffcd0a..1b5483818 100644 --- a/rules/aip0133/request_id_field_test.go +++ b/rules/aip0133/request_id_field_test.go @@ -32,6 +32,7 @@ func TestRequestIDField(t *testing.T) { {"ValidClientSpecified", "style: DECLARATIVE_FRIENDLY", "string book_id = 3;", nil}, {"InvalidDF", "style: DECLARATIVE_FRIENDLY", "", problems}, {"InvalidType", "style: DECLARATIVE_FRIENDLY", "bytes book_id = 3;", problems}, + {"InvalidRepeated", "style: DECLARATIVE_FRIENDLY", "repeated string book_id = 3;", problems}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` diff --git a/rules/aip0134/request_allow_missing_field.go b/rules/aip0134/request_allow_missing_field.go index cd754b495..76768379f 100644 --- a/rules/aip0134/request_allow_missing_field.go +++ b/rules/aip0134/request_allow_missing_field.go @@ -27,13 +27,13 @@ var allowMissing = &lint.MessageRule{ }, LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { for _, field := range m.GetFields() { - if field.GetName() == "allow_missing" && utils.GetTypeName(field) == "bool" { + if field.GetName() == "allow_missing" && utils.GetTypeName(field) == "bool" && !field.IsRepeated() { return nil } } return []lint.Problem{{ Descriptor: m, - Message: "Update requests on declarative-friendly resources should include `bool allow_missing`.", + Message: "Update requests on declarative-friendly resources should include a singular `bool allow_missing` field.", }} }, } diff --git a/rules/aip0134/request_allow_missing_field_test.go b/rules/aip0134/request_allow_missing_field_test.go index f3cf324c0..095880b1d 100644 --- a/rules/aip0134/request_allow_missing_field_test.go +++ b/rules/aip0134/request_allow_missing_field_test.go @@ -21,7 +21,7 @@ import ( ) func TestAllowMissing(t *testing.T) { - problems := testutils.Problems{{Message: "include `bool allow_missing`"}} + problems := testutils.Problems{{Message: "include a singular `bool allow_missing`"}} for _, test := range []struct { name string Style string @@ -32,6 +32,7 @@ func TestAllowMissing(t *testing.T) { {"ValidIncluded", "style: DECLARATIVE_FRIENDLY", "bool allow_missing = 2;", nil}, {"Invalid", "style: DECLARATIVE_FRIENDLY", "", problems}, {"InvalidWrongType", "style: DECLARATIVE_FRIENDLY", "string allow_missing = 2;", problems}, + {"InvalidRepeated", "style: DECLARATIVE_FRIENDLY", "repeated bool allow_missing = 2;", problems}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` @@ -55,7 +56,7 @@ func TestAllowMissing(t *testing.T) { `, test) m := f.GetMessageTypes()[0] if diff := test.problems.SetDescriptor(m).Diff(allowMissing.Lint(f)); diff != "" { - t.Errorf(diff) + t.Error(diff) } }) } diff --git a/rules/aip0134/request_mask_field.go b/rules/aip0134/request_mask_field.go index 2000ae918..705126403 100644 --- a/rules/aip0134/request_mask_field.go +++ b/rules/aip0134/request_mask_field.go @@ -26,9 +26,9 @@ var requestMaskField = &lint.FieldRule{ return isUpdateRequestMessage(f.GetOwner()) && f.GetName() == "update_mask" }, LintField: func(f *desc.FieldDescriptor) []lint.Problem { - if t := f.GetMessageType(); t == nil || t.GetFullyQualifiedName() != "google.protobuf.FieldMask" { + if t := f.GetMessageType(); t == nil || t.GetFullyQualifiedName() != "google.protobuf.FieldMask" || f.IsRepeated() { return []lint.Problem{{ - Message: "The `update_mask` field should be a google.protobuf.FieldMask.", + Message: "The `update_mask` field should be a singular google.protobuf.FieldMask.", Suggestion: "google.protobuf.FieldMask", Descriptor: f, Location: locations.FieldType(f), diff --git a/rules/aip0134/request_mask_field_test.go b/rules/aip0134/request_mask_field_test.go index 6473575e2..5e2c23332 100644 --- a/rules/aip0134/request_mask_field_test.go +++ b/rules/aip0134/request_mask_field_test.go @@ -30,6 +30,7 @@ func TestRequestMaskField(t *testing.T) { }{ {"Valid", "UpdateBookRequest", "google.protobuf.FieldMask", "update_mask", nil}, {"InvalidType", "UpdateBookRequest", "string", "update_mask", testutils.Problems{{Suggestion: "google.protobuf.FieldMask"}}}, + {"InvalidRepeated", "UpdateBookRequest", "repeated google.protobuf.FieldMask", "update_mask", testutils.Problems{{Suggestion: "google.protobuf.FieldMask"}}}, {"IrrelevantMessage", "ModifyBookRequest", "string", "update_mask", nil}, {"IrrelevantField", "UpdateBookRequest", "string", "modify_mask", nil}, } @@ -45,7 +46,7 @@ func TestRequestMaskField(t *testing.T) { field := file.GetMessageTypes()[0].GetFields()[0] problems := requestMaskField.Lint(file) if diff := test.problems.SetDescriptor(field).Diff(problems); diff != "" { - t.Errorf(diff) + t.Error(diff) } }) } diff --git a/rules/aip0154/field_type.go b/rules/aip0154/field_type.go index a2108d00d..ea9e8874e 100644 --- a/rules/aip0154/field_type.go +++ b/rules/aip0154/field_type.go @@ -29,9 +29,9 @@ var fieldType = &lint.FieldRule{ return f.GetName() == "etag" }, LintField: func(f *desc.FieldDescriptor) []lint.Problem { - if t := utils.GetTypeName(f); t != "string" { + if t := utils.GetTypeName(f); t != "string" || f.IsRepeated() { return []lint.Problem{{ - Message: fmt.Sprintf("The etag field should be a string, not %s.", t), + Message: fmt.Sprintf("The etag field should be a singular string, not %s.", t), Descriptor: f, Location: locations.FieldType(f), Suggestion: "string", diff --git a/rules/aip0154/field_type_test.go b/rules/aip0154/field_type_test.go index 47431214b..f975b23e4 100644 --- a/rules/aip0154/field_type_test.go +++ b/rules/aip0154/field_type_test.go @@ -28,6 +28,7 @@ func TestFieldType(t *testing.T) { }{ {"ValidString", "string", nil}, {"Invalid", "bytes", testutils.Problems{{Suggestion: "string"}}}, + {"InvalidRepeated", "repeated string", testutils.Problems{{Suggestion: "string"}}}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` @@ -37,7 +38,7 @@ func TestFieldType(t *testing.T) { `, test) field := f.GetMessageTypes()[0].GetFields()[0] if diff := test.problems.SetDescriptor(field).Diff(fieldType.Lint(f)); diff != "" { - t.Errorf(diff) + t.Error(diff) } }) } diff --git a/rules/aip0158/request_page_size_field.go b/rules/aip0158/request_page_size_field.go index 270f1da20..aa042fb6e 100644 --- a/rules/aip0158/request_page_size_field.go +++ b/rules/aip0158/request_page_size_field.go @@ -37,9 +37,9 @@ var requestPaginationPageSize = &lint.MessageRule{ } // Rule check: Ensure that the name page_size is the correct type. - if pageSize.GetType() != builder.FieldTypeInt32().GetType() { + if pageSize.GetType() != builder.FieldTypeInt32().GetType() || pageSize.IsRepeated() { return []lint.Problem{{ - Message: "`page_size` field on List RPCs should be an int32", + Message: "`page_size` field on List RPCs should be a singular int32", Suggestion: "int32", Descriptor: pageSize, Location: locations.FieldType(pageSize), diff --git a/rules/aip0163/declarative_friendly_required.go b/rules/aip0163/declarative_friendly_required.go index cdb888099..9c9d1a2a1 100644 --- a/rules/aip0163/declarative_friendly_required.go +++ b/rules/aip0163/declarative_friendly_required.go @@ -40,9 +40,9 @@ var declarativeFriendlyRequired = &lint.MessageRule{ return false }, LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { - if vo := m.FindFieldByName("validate_only"); vo == nil || utils.GetTypeName(vo) != "bool" { + if vo := m.FindFieldByName("validate_only"); vo == nil || utils.GetTypeName(vo) != "bool" || vo.IsRepeated() { return []lint.Problem{{ - Message: "Declarative-friendly mutate requests should include `bool validate_only`.", + Message: "Declarative-friendly mutate requests should include a singular `bool validate_only` field.", Descriptor: m, }} } diff --git a/rules/aip0163/declarative_friendly_required_test.go b/rules/aip0163/declarative_friendly_required_test.go index 7604aea0b..b2aaead7b 100644 --- a/rules/aip0163/declarative_friendly_required_test.go +++ b/rules/aip0163/declarative_friendly_required_test.go @@ -31,6 +31,7 @@ func TestDeclarativeFriendlyRequired(t *testing.T) { {"ValidNotDF", "", "", nil}, {"ValidDF", "style: DECLARATIVE_FRIENDLY", "bool validate_only = 2;", nil}, {"InvalidDFWrongType", "style: DECLARATIVE_FRIENDLY", "int32 validate_only = 2;", problems}, + {"InvalidDFRepeated", "style: DECLARATIVE_FRIENDLY", "repeated bool validate_only = 2;", problems}, {"InvalidDFMissing", "style: DECLARATIVE_FRIENDLY", "", problems}, } { t.Run(test.name, func(t *testing.T) {