Repository: googleapis/api-linter
Pull request #631 (2020-09-25 14:16:49)

Fix erroneous logic for batch request-parent-field rules
Batch URIs reference a collection, not an individual resource, so a parent field should be required when there is at least one resource in the URI.

The parent field should be absent when there are no resources in the URI, but these rules don't yet enforce that.

#615


Patch:
diff --git a/rules/aip0231/request_parent_field.go b/rules/aip0231/request_parent_field.go
index c8fa8db..ba570a8 100644
--- a/rules/aip0231/request_parent_field.go
+++ b/rules/aip0231/request_parent_field.go
@@ -31,7 +31,7 @@ var requestParentField = &lint.MessageRule{
 	Name: lint.NewRuleName(231, "request-parent-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
 		// Sanity check: If the resource has a pattern, and that pattern
-		// contains only one variable, then a parent field is not expected.
+		// contains no variables, then a parent field is not expected.
 		//
 		// In order to parse out the pattern, we get the resource message
 		// from the response, then get the resource annotation from that,
@@ -41,7 +41,7 @@ var requestParentField = &lint.MessageRule{
 			if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil {
 				if resource := utils.GetResource(paged.GetMessageType()); resource != nil {
 					for _, pattern := range resource.GetPattern() {
-						if strings.Count(pattern, "{") == 1 {
+						if strings.Count(pattern, "{") == 0 {
 							return false
 						}
 					}
@@ -64,7 +64,7 @@ var requestParentField = &lint.MessageRule{
 		// Rule check: Establish that the parent field is a string.
 		if parentField.GetType() != builder.FieldTypeString().GetType() {
 			return []lint.Problem{{
-				Message:    "`parent` field on create request message must be a string",
+				Message:    "`parent` field on Batch Get request message must be a string",
 				Suggestion: "string",
 				Descriptor: parentField,
 				Location:   locations.FieldType(parentField),
diff --git a/rules/aip0233/request_parent_field.go b/rules/aip0233/request_parent_field.go
index 8d61f42..2d22e82 100644
--- a/rules/aip0233/request_parent_field.go
+++ b/rules/aip0233/request_parent_field.go
@@ -41,7 +41,7 @@ var requestParentField = &lint.MessageRule{
 			if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil {
 				if resource := utils.GetResource(paged.GetMessageType()); resource != nil {
 					for _, pattern := range resource.GetPattern() {
-						if strings.Count(pattern, "{") == 1 {
+						if strings.Count(pattern, "{") == 0 {
 							return false
 						}
 					}
@@ -64,7 +64,7 @@ var requestParentField = &lint.MessageRule{
 		// Rule check: Establish that the parent field is a string.
 		if parentField.GetType() != builder.FieldTypeString().GetType() {
 			return []lint.Problem{{
-				Message:    "`parent` field on create request message must be a string.",
+				Message:    "`parent` field on Batch Create request message must be a string.",
 				Descriptor: parentField,
 				Location:   locations.FieldType(parentField),
 				Suggestion: "string",
diff --git a/rules/aip0234/request_parent_field.go b/rules/aip0234/request_parent_field.go
index 3db8f5f..4365e16 100644
--- a/rules/aip0234/request_parent_field.go
+++ b/rules/aip0234/request_parent_field.go
@@ -31,7 +31,7 @@ var requestParentField = &lint.MessageRule{
 	Name: lint.NewRuleName(234, "request-parent-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
 		// Sanity check: If the resource has a pattern, and that pattern
-		// contains only one variable, then a parent field is not expected.
+		// contains no variables, then a parent field is not expected.
 		//
 		// In order to parse out the pattern, we get the resource message
 		// from the response, then get the resource annotation from that,
@@ -41,7 +41,7 @@ var requestParentField = &lint.MessageRule{
 			if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil {
 				if resource := utils.GetResource(paged.GetMessageType()); resource != nil {
 					for _, pattern := range resource.GetPattern() {
-						if strings.Count(pattern, "{") == 1 {
+						if strings.Count(pattern, "{") == 0 {
 							return false
 						}
 					}
@@ -64,7 +64,7 @@ var requestParentField = &lint.MessageRule{
 		// Rule check: Establish that the parent field is a string.
 		if parentField.GetType() != builder.FieldTypeString().GetType() {
 			return []lint.Problem{{
-				Message:    "`parent` field on Update request message must be a string.",
+				Message:    "`parent` field on Batch Update request message must be a string.",
 				Descriptor: parentField,
 				Location:   locations.FieldType(parentField),
 				Suggestion: "string",


Test patch:
diff --git a/rules/aip0231/request_parent_field_test.go b/rules/aip0231/request_parent_field_test.go
index 03e6441..c58ce28 100644
--- a/rules/aip0231/request_parent_field_test.go
+++ b/rules/aip0231/request_parent_field_test.go
@@ -29,11 +29,11 @@ func TestParentField(t *testing.T) {
 		Pattern  string
 		problems testutils.Problems
 	}{
-		{"Valid", "BatchGetBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil},
-		{"Missing", "BatchGetBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}},
-		{"InvalidType", "BatchGetBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}},
-		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil},
-		{"IrrelevantNoParent", "BatchGetBooks", "", "books/{b}", nil},
+		{"Valid", "BatchGetBooks", "string parent = 1;", "publishers/{p}/books", nil},
+		{"Missing", "BatchGetBooks", "", "publishers/{p}/books", testutils.Problems{{Message: "no `parent`"}}},
+		{"InvalidType", "BatchGetBooks", "int32 parent = 1;", "publishers/{p}/books", testutils.Problems{{Suggestion: "string"}}},
+		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books", nil},
+		{"IrrelevantNoParent", "BatchGetBooks", "", "books", nil},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
diff --git a/rules/aip0233/request_parent_field_test.go b/rules/aip0233/request_parent_field_test.go
index b18b8b8..bbbbba2 100644
--- a/rules/aip0233/request_parent_field_test.go
+++ b/rules/aip0233/request_parent_field_test.go
@@ -29,11 +29,11 @@ func TestParentField(t *testing.T) {
 		Pattern  string
 		problems testutils.Problems
 	}{
-		{"Valid", "BatchCreateBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil},
-		{"Missing", "BatchCreateBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}},
-		{"InvalidType", "BatchCreateBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}},
-		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil},
-		{"IrrelevantNoParent", "BatchCreateBooks", "", "books/{b}", nil},
+		{"Valid", "BatchCreateBooks", "string parent = 1;", "publishers/{p}/books", nil},
+		{"Missing", "BatchCreateBooks", "", "publishers/{p}/books", testutils.Problems{{Message: "no `parent`"}}},
+		{"InvalidType", "BatchCreateBooks", "int32 parent = 1;", "publishers/{p}/books", testutils.Problems{{Suggestion: "string"}}},
+		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books", nil},
+		{"IrrelevantNoParent", "BatchCreateBooks", "", "books", nil},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
diff --git a/rules/aip0234/request_parent_field_test.go b/rules/aip0234/request_parent_field_test.go
index 360e1c4..a032ba3 100644
--- a/rules/aip0234/request_parent_field_test.go
+++ b/rules/aip0234/request_parent_field_test.go
@@ -29,11 +29,11 @@ func TestParentField(t *testing.T) {
 		Pattern  string
 		problems testutils.Problems
 	}{
-		{"Valid", "BatchUpdateBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil},
-		{"Missing", "BatchUpdateBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}},
-		{"InvalidType", "BatchUpdateBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}},
-		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil},
-		{"IrrelevantNoParent", "BatchUpdateBooks", "", "books/{b}", nil},
+		{"Valid", "BatchUpdateBooks", "string parent = 1;", "publishers/{p}/books", nil},
+		{"Missing", "BatchUpdateBooks", "", "publishers/{p}/books", testutils.Problems{{Message: "no `parent`"}}},
+		{"InvalidType", "BatchUpdateBooks", "int32 parent = 1;", "publishers/{p}/books", testutils.Problems{{Suggestion: "string"}}},
+		{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books", nil},
+		{"IrrelevantNoParent", "BatchUpdateBooks", "", "books", nil},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `