fix(AIP-135): allow etag & force in signature

googleapis · Jul 12, 2023 · cd87d7d · cd87d7d
1 parent 5402a46
commit cd87d7d
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 10 deletions.
diff --git 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,40 @@ var methodSignature = &lint.MethodRule{
 	OnlyIf: isDeleteMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		signatures := utils.GetMethodSignatures(m)
+		in := m.GetInputType()
+
+		fields := stringset.New("name")
+		if etag := in.FindFieldByName("etag"); etag != nil {
+			fields.Add(etag.GetName())
+		}
+		if force := in.FindFieldByName("force"); force != nil {
+			fields.Add(force.GetName())
+		}
+		want := strings.Join(fields.Unordered(), ",")
 
 		// 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]
+		if !fields.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
@@ -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 != "" {