fix(AIP-133): use resource field in signature suggestion (#1439)

googleapis · Oct 22, 2024 · 20c96b6 · 20c96b6
1 parent 4256d75
commit 20c96b6
Show file tree

Hide file tree

Showing 2 changed files with 89 additions and 8 deletions.
diff --git a/rules/aip0133/method_signature.go b/rules/aip0133/method_signature.go
@@ -32,15 +32,21 @@ var methodSignature = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		signatures := utils.GetMethodSignatures(m)
 
-		// Determine what signature we want. The {resource}_id is desired
-		// if and only if the field exists on the request.
-		resourceField := strcase.SnakeCase(utils.GetResourceMessageName(m, "Create"))
+		// Determine what signature we want.
 		want := []string{}
-		if !hasNoParent(m.GetOutputType()) {
+		if !hasNoParent(utils.GetResponseType(m)) {
 			want = append(want, "parent")
 		}
-		want = append(want, resourceField)
-		if idField := resourceField + "_id"; m.GetInputType().FindFieldByName(idField) != nil {
+		for _, f := range m.GetInputType().GetFields() {
+			if mt := f.GetMessageType(); mt != nil && utils.IsResource(mt) {
+				want = append(want, f.GetName())
+				break
+			}
+		}
+		// The {resource}_id is desired if and only if the field exists on the
+		// request.
+		expectedResourceIDField := strcase.SnakeCase(utils.GetResourceMessageName(m, "Create"))
+		if idField := expectedResourceIDField + "_id"; m.GetInputType().FindFieldByName(idField) != nil {
 			want = append(want, idField)
 		}
 

diff --git a/rules/aip0133/method_signature_test.go b/rules/aip0133/method_signature_test.go
@@ -30,7 +30,6 @@ func TestMethodSignature(t *testing.T) {
 	}{
 		{"ValidNoID", "CreateBook", `option (google.api.method_signature) = "parent,book";`, "", testutils.Problems{}},
 		{"ValidID", "CreateBook", `option (google.api.method_signature) = "parent,book,book_id";`, "string book_id = 3;", testutils.Problems{}},
-		{"ValidOperation", "CreateUnitOperation", `option (google.api.method_signature) = "parent,unit_operation,unit_operation_id";`, "string unit_operation_id = 3;", testutils.Problems{}},
 		{"MissingNoID", "CreateBook", "", "", testutils.Problems{{Message: `(google.api.method_signature) = "parent,book"`}}},
 		{
 			"MissingID",
@@ -65,6 +64,7 @@ func TestMethodSignature(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
 				import "google/api/client.proto";
+				import "google/api/resource.proto";
 				service Library {
 					rpc {{.MethodName}}({{.MethodName}}Request) returns (Book) {
 						{{.Signature}}
@@ -75,7 +75,12 @@ func TestMethodSignature(t *testing.T) {
 					Book book = 2;
 					{{.IDField}}
 				}
-				message Book {}
+				message Book {
+				  option (google.api.resource) = {
+				    type: "library.googleapis.com/Book"
+					  pattern: "libraries/{library}/books/{book}"
+				  };
+				}
 			`, test)
 			m := f.GetServices()[0].GetMethods()[0]
 			if diff := test.problems.SetDescriptor(m).Diff(methodSignature.Lint(f)); diff != "" {
@@ -101,6 +106,7 @@ func TestMethodSignature(t *testing.T) {
 			}
 			message Book {
 				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
 					pattern: "books/{book}"
 				};
 			}
@@ -109,4 +115,73 @@ func TestMethodSignature(t *testing.T) {
 			t.Errorf(diff)
 		}
 	})
+	// Add a separate test for the LRO case rather than introducing yet
+	// another knob on the above test.
+	t.Run("Longrunning", func(t *testing.T) {
+		file := testutils.ParseProto3String(t, `
+			import "google/api/client.proto";
+			import "google/api/resource.proto";
+			import "google/longrunning/operations.proto";
+			service Library {
+				rpc CreateBook(CreateBookRequest) returns (google.longrunning.Operation) {
+					option (google.api.method_signature) = "book,book_id";
+					option (google.longrunning.operation_info) = {
+						response_type: "Book"
+						metadata_type: "Book"
+					};
+				}
+			}
+			message CreateBookRequest {
+				Book book = 1;
+				string book_id = 2;
+			}
+			message Book {
+				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
+					pattern: "books/{book}"
+				};
+			}
+		`)
+		if diff := (testutils.Problems{}).Diff(methodSignature.Lint(file)); diff != "" {
+			t.Errorf(diff)
+		}
+	})
+	// Add a separate test for the non-standard resource field case rather than
+	// introducing yet another knob on the above test.
+	t.Run("NonStandardResourceFieldName", func(t *testing.T) {
+		file := testutils.ParseProto3String(t, `
+			import "google/api/client.proto";
+			import "google/api/resource.proto";
+			import "google/longrunning/operations.proto";
+			service Library {
+				rpc CreateBook(CreateBookRequest) returns (google.longrunning.Operation) {
+					option (google.api.method_signature) = "book,book_id";
+					option (google.longrunning.operation_info) = {
+						response_type: "Book"
+						metadata_type: "Book"
+					};
+				}
+			}
+			message CreateBookRequest {
+				Book not_book = 1;
+				string book_id = 2;
+			}
+			message Book {
+				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
+					pattern: "books/{book}"
+				};
+			}
+		`)
+		want := testutils.Problems{
+			{
+				Message:    "not_book,book",
+				Suggestion: `option (google.api.method_signature) = "not_book,book_id";`,
+				Descriptor: file.GetServices()[0].GetMethods()[0],
+			},
+		}
+		if diff := want.Diff(methodSignature.Lint(file)); diff != "" {
+			t.Errorf(diff)
+		}
+	})
 }