fix: Check field cardinality for AIP-132, AIP-133, AIP-134, AIP-154, A…

…IP-158, AIP-163 (#803)
googleapis · Mar 22, 2021 · 0d6dccb · 0d6dccb
1 parent 459634d
commit 0d6dccb
Show file tree

Hide file tree

Showing 13 changed files with 44 additions and 54 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {