Repository: googleapis/api-linter
Pull request #562 (2020-06-16 18:36:09)

[AIP-133] Be more lenient on top-level resources.


Patch:
diff --git a/rules/aip0133/http_uri_parent.go b/rules/aip0133/http_uri_parent.go
index 0b1cd15..755f2e1 100644
--- a/rules/aip0133/http_uri_parent.go
+++ b/rules/aip0133/http_uri_parent.go
@@ -22,8 +22,10 @@ import (
 
 // Create methods should have a proper HTTP pattern.
 var httpURIField = &lint.MethodRule{
-	Name:   lint.NewRuleName(133, "http-uri-parent"),
-	OnlyIf: isCreateMethod,
+	Name: lint.NewRuleName(133, "http-uri-parent"),
+	OnlyIf: func(m *desc.MethodDescriptor) bool {
+		return isCreateMethod(m) && !hasNoParent(m.GetOutputType())
+	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC uri should include the `parent` field.
 		for _, httpRule := range utils.GetHTTPRules(m) {
diff --git a/rules/aip0133/method_signature.go b/rules/aip0133/method_signature.go
index b866747..e237d05 100644
--- a/rules/aip0133/method_signature.go
+++ b/rules/aip0133/method_signature.go
@@ -35,7 +35,11 @@ var methodSignature = &lint.MethodRule{
 		// Determine what signature we want. The {resource}_id is desired
 		// if and only if the field exists on the request.
 		resourceField := strcase.SnakeCase(getResourceMsgName(m))
-		want := []string{"parent", resourceField}
+		want := []string{}
+		if !hasNoParent(m.GetOutputType()) {
+			want = append(want, "parent")
+		}
+		want = append(want, resourceField)
 		if idField := resourceField + "_id"; m.GetInputType().FindFieldByName(idField) != nil {
 			want = append(want, idField)
 		}
diff --git a/rules/aip0133/request_parent_required.go b/rules/aip0133/request_parent_required.go
index 03a9947..483e964 100644
--- a/rules/aip0133/request_parent_required.go
+++ b/rules/aip0133/request_parent_required.go
@@ -23,12 +23,8 @@ var requestParentRequired = &lint.MessageRule{
 			// and then inspect the pattern there (oy!).
 			singular := getResourceMsgNameFromReq(m)
 			if field := m.FindFieldByName(strcase.SnakeCase(singular)); field != nil {
-				if resource := utils.GetResource(field.GetMessageType()); resource != nil {
-					for _, pattern := range resource.GetPattern() {
-						if strings.Count(pattern, "{") == 1 {
-							return nil
-						}
-					}
+				if hasNoParent(field.GetMessageType()) {
+					return nil
 				}
 			}
 
@@ -42,3 +38,14 @@ var requestParentRequired = &lint.MessageRule{
 		return nil
 	},
 }
+
+func hasNoParent(m *desc.MessageDescriptor) bool {
+	if resource := utils.GetResource(m); resource != nil {
+		for _, pattern := range resource.GetPattern() {
+			if strings.Count(pattern, "{") == 1 {
+				return true
+			}
+		}
+	}
+	return false
+}


Test patch:
diff --git a/rules/aip0133/http_uri_parent_test.go b/rules/aip0133/http_uri_parent_test.go
index 14ab387..eb15dfb 100644
--- a/rules/aip0133/http_uri_parent_test.go
+++ b/rules/aip0133/http_uri_parent_test.go
@@ -25,27 +25,34 @@ func TestHttpUriField(t *testing.T) {
 		testName   string
 		URI        string
 		MethodName string
+		Pattern    string
 		problems   testutils.Problems
 	}{
-		{"Valid", "/v1/{parent=publishers/*/books/*}", "CreateBook", testutils.Problems{}},
-		{"InvalidVarParent", "/v1/{book=publishers/*/books/*}", "CreateBook", testutils.Problems{{Message: "`parent` field"}}},
-		{"NoVarParent", "/v1/publishers/*/books/*", "CreateBook", testutils.Problems{{Message: "`parent` field"}}},
-		{"Irrelevant", "/v1/{book=publishers/*/books/*}", "BuildBook", testutils.Problems{}},
+		{"Valid", "/v1/{parent=publishers/*/books/*}", "CreateBook", "publishers/{publisher}/books/{book}", nil},
+		{"InvalidVarParent", "/v1/{book=publishers/*/books/*}", "CreateBook", "publishers/{publisher}/books/{book}", testutils.Problems{{Message: "`parent` field"}}},
+		{"NoVarParent", "/v1/publishers/*/books/*", "CreateBook", "publishers/{publisher}/books/{book}", testutils.Problems{{Message: "`parent` field"}}},
+		{"NoParent", "/v1/books/*", "CreateBook", "books/{book}", nil},
+		{"Irrelevant", "/v1/{book=publishers/*/books/*}", "BuildBook", "publishers/{publisher}/books/{book}", nil},
 	}
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
 				import "google/api/annotations.proto";
+				import "google/api/resource.proto";
 			  service Library {
-					rpc {{.MethodName}}({{.MethodName}}Request) returns ({{.MethodName}}Response) {
+					rpc {{.MethodName}}({{.MethodName}}Request) returns (Book) {
 						option (google.api.http) = {
-							get: "{{.URI}}"
+							post: "{{.URI}}"
 						};
 					}
 				}
 				message {{.MethodName}}Request {}
-				message {{.MethodName}}Response {}
+				message Book {
+					option (google.api.resource) = {
+						pattern: "{{.Pattern}}"
+					};
+				}
 			`, test)
 			method := f.GetServices()[0].GetMethods()[0]
 			if diff := test.problems.SetDescriptor(method).Diff(httpURIField.Lint(f)); diff != "" {
diff --git a/rules/aip0133/method_signature_test.go b/rules/aip0133/method_signature_test.go
index 607112d..5d396db 100644
--- a/rules/aip0133/method_signature_test.go
+++ b/rules/aip0133/method_signature_test.go
@@ -82,4 +82,30 @@ func TestMethodSignature(t *testing.T) {
 			}
 		})
 	}
+
+	// Add a separate test for the no-parent case rather than introducing yet
+	// another knob on the above test.
+	t.Run("NoParent", func(t *testing.T) {
+		file := testutils.ParseProto3String(t, `
+			import "google/api/client.proto";
+			import "google/api/resource.proto";
+			service Library {
+				rpc CreateBook(CreateBookRequest) returns (Book) {
+					option (google.api.method_signature) = "book,book_id";
+				}
+			}
+			message CreateBookRequest {
+				Book book = 1;
+				string book_id = 2;
+			}
+			message Book {
+				option (google.api.resource) = {
+					pattern: "books/{book}"
+				};
+			}
+		`)
+		if diff := (testutils.Problems{}).Diff(methodSignature.Lint(file)); diff != "" {
+			t.Errorf(diff)
+		}
+	})
 }