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 != "" {