fix: Do not require allow_missing Update request field for singleton …

…resources (#832)
googleapis · Apr 7, 2021 · eb7c453 · eb7c453
1 parent 2c6fcae
commit eb7c453
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 21 deletions.
diff --git a/rules/aip0134/request_allow_missing_field.go b/rules/aip0134/request_allow_missing_field.go
@@ -23,7 +23,11 @@ import (
 var allowMissing = &lint.MessageRule{
 	Name: lint.NewRuleName(134, "request-allow-missing-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
-		return isUpdateRequestMessage(m) && utils.IsDeclarativeFriendlyMessage(m)
+		if !isUpdateRequestMessage(m) {
+			return false
+		}
+		r := utils.DeclarativeFriendlyResource(m)
+		return r != nil && !utils.IsSingletonResource(r)
 	},
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
 		for _, field := range m.GetFields() {

diff --git a/rules/aip0134/request_allow_missing_field_test.go b/rules/aip0134/request_allow_missing_field_test.go
@@ -21,18 +21,22 @@ import (
 )
 
 func TestAllowMissing(t *testing.T) {
+	const singletonPattern = `books/{book}/settings`
+	const nonSingletonPattern = `books/{book}`
 	problems := testutils.Problems{{Message: "include a singular `bool allow_missing`"}}
 	for _, test := range []struct {
 		name         string
 		Style        string
+		Pattern      string
 		AllowMissing string
 		problems     testutils.Problems
 	}{
-		{"IgnoredNotDF", "", "", nil},
-		{"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},
+		{"IgnoredNotDF", "", nonSingletonPattern, "", nil},
+		{"ValidIncluded", "style: DECLARATIVE_FRIENDLY", nonSingletonPattern, "bool allow_missing = 2;", nil},
+		{"Invalid", "style: DECLARATIVE_FRIENDLY", nonSingletonPattern, "", problems},
+		{"InvalidWrongType", "style: DECLARATIVE_FRIENDLY", nonSingletonPattern, "string allow_missing = 2;", problems},
+		{"InvalidRepeated", "style: DECLARATIVE_FRIENDLY", nonSingletonPattern, "repeated bool allow_missing = 2;", problems},
+		{"IgnoredSingleton", "style: DECLARATIVE_FRIENDLY", singletonPattern, "", nil},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
@@ -50,6 +54,7 @@ func TestAllowMissing(t *testing.T) {
 				message Book {
 					option (google.api.resource) = {
 						type: "library.googleapis.com/Book"
+						pattern: "{{.Pattern}}"
 						{{.Style}}
 					};
 				}

diff --git a/rules/aip0148/declarative_friendly_fields.go b/rules/aip0148/declarative_friendly_fields.go
@@ -33,7 +33,7 @@ var declarativeFriendlyRequired = &lint.MessageRule{
 		return false
 	},
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		singleton := isSingletonResource(m)
+		singleton := utils.IsSingletonResource(m)
 		// Define the fields that are expected.
 		missingFields := stringset.New()
 		for name, typ := range reqFields {
@@ -79,17 +79,3 @@ var singletonExceptions = stringset.New(
 	"delete_time",
 	"uid",
 )
-
-func isSingletonResource(m *desc.MessageDescriptor) bool {
-	// If the pattern ends in something other than "}", that indicates that this is a singleton.
-	//
-	// For example:
-	//   publishers/{publisher}/books/{book} -- not a singleton, many books
-	//   publishers/*/settings -- a singleton; one settings object per publisher
-	for _, pattern := range utils.GetResource(m).GetPattern() {
-		if !strings.HasSuffix(pattern, "}") {
-			return true
-		}
-	}
-	return false
-}
diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go
@@ -80,6 +80,22 @@ func IsResource(m *desc.MessageDescriptor) bool {
 	return false
 }
 
+// IsSingletonResource returns true if the given message is a singleton
+// resource according to its pattern.
+func IsSingletonResource(m *desc.MessageDescriptor) bool {
+	// If the pattern ends in something other than "}", that indicates that this is a singleton.
+	//
+	// For example:
+	//   publishers/{publisher}/books/{book} -- not a singleton, many books
+	//   publishers/*/settings -- a singleton; one settings object per publisher
+	for _, pattern := range GetResource(m).GetPattern() {
+		if !strings.HasSuffix(pattern, "}") {
+			return true
+		}
+	}
+	return false
+}
+
 // GetResourceDefinitions returns the google.api.resource_definition annotations
 // for a file.
 func GetResourceDefinitions(f *desc.FileDescriptor) []*apb.ResourceDescriptor {