From 9e002e6adf42a3c3e709764b5237c982ccf94517 Mon Sep 17 00:00:00 2001
From: Noah Dietz <noahdietz@users.noreply.github.com>
Date: Fri, 14 Jul 2023 11:20:13 -0700
Subject: [PATCH] fix(AIP-135): allow etag & force in signature (#1197)

Allows `etag` and `force` in the Standard Delete `google.api.method_signature`. If there is a field that doesn't belong, it will recommend a `method_signature` that includes `etag` and/or `force` if they are present in the request message, but otherwise, will ensure that the first `method_signature` contains exclusively fields from the set `name` `etag` and `force`, as well as enforce that it always includes `name`.

Fixes #1140
---
 rules/aip0135/method_signature.go      | 27 ++++++++++++++++++++------
 rules/aip0135/method_signature_test.go | 18 +++++++++++++----
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/rules/aip0135/method_signature.go b/rules/aip0135/method_signature.go
index 9935dd43b..02cdfcfd3 100644
--- a/rules/aip0135/method_signature.go
+++ b/rules/aip0135/method_signature.go
@@ -16,8 +16,9 @@ package aip0135
 
 import (
 	"fmt"
-	"reflect"
+	"strings"
 
+	"bitbucket.org/creachadair/stringset"
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
 	"github.com/googleapis/api-linter/rules/internal/utils"
@@ -29,27 +30,41 @@ var methodSignature = &lint.MethodRule{
 	OnlyIf: isDeleteMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		signatures := utils.GetMethodSignatures(m)
+		in := m.GetInputType()
+
+		fields := []string{"name"}
+		if etag := in.FindFieldByName("etag"); etag != nil {
+			fields = append(fields, etag.GetName())
+		}
+		if force := in.FindFieldByName("force"); force != nil {
+			fields = append(fields, force.GetName())
+		}
+		want := strings.Join(fields, ",")
 
 		// Check if the signature is missing.
 		if len(signatures) == 0 {
 			return []lint.Problem{{
 				Message: fmt.Sprintf(
 					"Delete methods should include `(google.api.method_signature) = %q`",
-					"name",
+					want,
 				),
 				Descriptor: m,
 			}}
 		}
 
-		// Check if the signature is wrong.
-		if !reflect.DeepEqual(signatures[0], []string{"name"}) {
+		// Check if the signature contains a disallowed field or doesn't contain
+		// "name".
+		first := signatures[0]
+		fieldSet := stringset.New(fields...)
+		if !fieldSet.Contains(first...) || !stringset.New(first...).Contains("name") {
 			return []lint.Problem{{
-				Message:    `The method signature for Delete methods should be "name".`,
-				Suggestion: `option (google.api.method_signature) = "name";`,
+				Message:    fmt.Sprintf("The method signature for Delete methods should be %q.", want),
+				Suggestion: fmt.Sprintf("option (google.api.method_signature) = %q;", want),
 				Descriptor: m,
 				Location:   locations.MethodSignature(m, 0),
 			}}
 		}
+
 		return nil
 	},
 }
diff --git a/rules/aip0135/method_signature_test.go b/rules/aip0135/method_signature_test.go
index c201c2ca8..297c9cc62 100644
--- a/rules/aip0135/method_signature_test.go
+++ b/rules/aip0135/method_signature_test.go
@@ -25,17 +25,24 @@ func TestMethodSignature(t *testing.T) {
 		name       string
 		MethodName string
 		Signature  string
+		Etag       string
+		Force      string
 		problems   testutils.Problems
 	}{
-		{"Valid", "DeleteBook", `option (google.api.method_signature) = "name";`, testutils.Problems{}},
-		{"Missing", "DeleteBook", "", testutils.Problems{{Message: `(google.api.method_signature) = "name"`}}},
+		{"Valid", "DeleteBook", `option (google.api.method_signature) = "name";`, "", "", testutils.Problems{}},
+		{"Missing", "DeleteBook", "", "", "", testutils.Problems{{Message: `(google.api.method_signature) = "name"`}}},
 		{
 			"Wrong",
 			"DeleteBook",
 			`option (google.api.method_signature) = "book";`,
+			"", "",
 			testutils.Problems{{Suggestion: `option (google.api.method_signature) = "name";`}},
 		},
-		{"Irrelevant", "BurnBook", "", testutils.Problems{}},
+		{"Irrelevant", "RemoveBook", "", "", "", testutils.Problems{}},
+		{"WithEtag", "DeleteBook", `option (google.api.method_signature) = "name,etag";`, "string etag = 2;", "", testutils.Problems{}},
+		{"WithForce", "DeleteBook", `option (google.api.method_signature) = "name,force";`, "", "bool force = 3;", testutils.Problems{}},
+		{"WithBoth", "DeleteBook", `option (google.api.method_signature) = "name,etag,force";`, "string etag = 2;", "bool force = 3;", testutils.Problems{}},
+		{"MissingNameWithBoth", "DeleteBook", `option (google.api.method_signature) = "etag,force";`, "string etag = 2;", "bool force = 3;", testutils.Problems{{Suggestion: `option (google.api.method_signature) = "name,etag,force";`}}},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
@@ -46,7 +53,10 @@ func TestMethodSignature(t *testing.T) {
 						{{.Signature}}
 					}
 				}
-				message {{.MethodName}}Request {}
+				message {{.MethodName}}Request {
+					{{.Etag}}
+					{{.Force}}
+				}
 			`, test)
 			m := f.GetServices()[0].GetMethods()[0]
 			if diff := test.problems.SetDescriptor(m).Diff(methodSignature.Lint(f)); diff != "" {