Repository: googleapis/api-linter
Pull request #252 (2019-10-22 20:30:14)

Refactor GetHTTPRules.
In writing an upcoming rule for AIP-156, I realized I have cases
where I *only* care about the URI, and not the method. Getting
this is a huge pain.

I have therefore refactored `GetHTTPRules` to return a custom
struct instead of the underlying proto annotation. The custom
struct has `Method` and `URI` fields that correspond to the
values on the annotation.

In the refactor to the standard method rules, the change clearly
does a better job of expressing intent: `if r.Method == "POST"`
is much clearer than `if r.GetPost() != ""`.


Patch:
diff --git a/rules/aip0131/method_rules.go b/rules/aip0131/method_rules.go
index e5822c8..3d7d391 100644
--- a/rules/aip0131/method_rules.go
+++ b/rules/aip0131/method_rules.go
@@ -77,7 +77,7 @@ var httpVerb = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that the RPC uses HTTP GET.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetGet() == "" {
+			if httpRule.Method != "GET" {
 				return []lint.Problem{{
 					Message:    "Get methods must use the HTTP GET verb.",
 					Descriptor: m,
@@ -97,13 +97,11 @@ var httpNameField = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if uri := httpRule.GetGet(); uri != "" {
-				if !getURINameRegexp.MatchString(uri) {
-					return []lint.Problem{{
-						Message:    "Get methods should include the `name` field in the URI.",
-						Descriptor: m,
-					}}
-				}
+			if !getURINameRegexp.MatchString(httpRule.URI) {
+				return []lint.Problem{{
+					Message:    "Get methods should include the `name` field in the URI.",
+					Descriptor: m,
+				}}
 			}
 		}
 
@@ -119,7 +117,7 @@ var httpBody = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetBody() != "" {
+			if httpRule.Body != "" {
 				return []lint.Problem{{
 					Message:    "Get methods should not have an HTTP body.",
 					Descriptor: m,
diff --git a/rules/aip0133/method_rules.go b/rules/aip0133/method_rules.go
index 6033808..ae33df7 100644
--- a/rules/aip0133/method_rules.go
+++ b/rules/aip0133/method_rules.go
@@ -31,7 +31,7 @@ var httpVerb = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that the RPC uses HTTP POST.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetPost() == "" {
+			if httpRule.Method != "POST" {
 				return []lint.Problem{{
 					Message:    "Create methods must use the HTTP POST verb.",
 					Descriptor: m,
@@ -51,13 +51,11 @@ var httpURIField = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC uri should include the `parent` field.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if uri := httpRule.GetPost(); uri != "" {
-				if !createURINameRegexp.MatchString(uri) {
-					return []lint.Problem{{
-						Message:    "Post methods should include the `parent` field in the URI.",
-						Descriptor: m,
-					}}
-				}
+			if !createURINameRegexp.MatchString(httpRule.URI) {
+				return []lint.Problem{{
+					Message:    "Create methods should include the `parent` field in the URI.",
+					Descriptor: m,
+				}}
 			}
 		}
 
@@ -84,7 +82,7 @@ var httpBody = &lint.MethodRule{
 
 		// Establish that HTTP body the RPC should map the resource field name in the request message.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if body := httpRule.GetBody(); body == "" {
+			if httpRule.Body == "" {
 				// Establish that the RPC should have HTTP body
 				return []lint.Problem{{
 					Message:    "Post methods should have an HTTP body.",
@@ -94,9 +92,13 @@ var httpBody = &lint.MethodRule{
 				// will not be triggered by the rule"core::0133::http-body". It will be
 				// triggered by another rule
 				// "core::0133::request-message::resource-field"
-			} else if resourceFieldName != "" && body != resourceFieldName {
+			} else if resourceFieldName != "" && httpRule.Body != resourceFieldName {
 				return []lint.Problem{{
-					Message:    fmt.Sprintf("The content of body %q must map to the resource field %q in the request message", body, resourceFieldName),
+					Message: fmt.Sprintf(
+						"The content of body %q must map to the resource field %q in the request message",
+						httpRule.Body,
+						resourceFieldName,
+					),
 					Descriptor: m,
 				}}
 			}
diff --git a/rules/aip0134/method_rules.go b/rules/aip0134/method_rules.go
index 6106533..a5c6e53 100644
--- a/rules/aip0134/method_rules.go
+++ b/rules/aip0134/method_rules.go
@@ -91,7 +91,7 @@ var httpVerb = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that the RPC uses HTTP PATCH.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetPatch() == "" {
+			if httpRule.Method != "PATCH" {
 				return []lint.Problem{{
 					Message:    "Update methods must use the HTTP PATCH verb.",
 					Descriptor: m,
@@ -112,14 +112,12 @@ var httpNameField = &lint.MethodRule{
 		fieldName := strcase.SnakeCase(m.GetName()[6:])
 		// Establish that the RPC has expected HTTP pattern.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if uri := httpRule.GetPatch(); uri != "" {
-				matches := updateURINameRegexp.FindStringSubmatch(uri)
-				if matches == nil || matches[1] != fieldName {
-					return []lint.Problem{{
-						Message:    fmt.Sprintf("Update methods should include the `%s.name` field in the URI.", fieldName),
-						Descriptor: m,
-					}}
-				}
+			matches := updateURINameRegexp.FindStringSubmatch(httpRule.URI)
+			if matches == nil || matches[1] != fieldName {
+				return []lint.Problem{{
+					Message:    fmt.Sprintf("Update methods should include the `%s.name` field in the URI.", fieldName),
+					Descriptor: m,
+				}}
 			}
 		}
 
@@ -136,7 +134,7 @@ var httpBody = &lint.MethodRule{
 		fieldName := strcase.SnakeCase(m.GetName()[6:])
 		// Establish that the RPC has HTTP body equal to fieldName.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetBody() != fieldName {
+			if httpRule.Body != fieldName {
 				return []lint.Problem{{
 					Message:    fmt.Sprintf("Update methods should have an HTTP body equal to `%q`.", fieldName),
 					Descriptor: m,
diff --git a/rules/aip0135/method_rules.go b/rules/aip0135/method_rules.go
index bd0d04b..e386be5 100644
--- a/rules/aip0135/method_rules.go
+++ b/rules/aip0135/method_rules.go
@@ -97,7 +97,7 @@ var httpVerb = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that the RPC uses HTTP DELETE.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetDelete() == "" {
+			if httpRule.Method != "DELETE" {
 				return []lint.Problem{{
 					Message:    "Delete methods must use the HTTP DELETE verb.",
 					Descriptor: m,
@@ -117,13 +117,11 @@ var httpNameField = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if uri := httpRule.GetDelete(); uri != "" {
-				if !deleteURINameRegexp.MatchString(uri) {
-					return []lint.Problem{{
-						Message:    "Delete methods should include the `name` field in the URI.",
-						Descriptor: m,
-					}}
-				}
+			if !deleteURINameRegexp.MatchString(httpRule.URI) {
+				return []lint.Problem{{
+					Message:    "Delete methods should include the `name` field in the URI.",
+					Descriptor: m,
+				}}
 			}
 		}
 
@@ -139,7 +137,7 @@ var httpBody = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if httpRule.GetBody() != "" {
+			if httpRule.Body != "" {
 				return []lint.Problem{{
 					Message:    "Delete methods should not have an HTTP body.",
 					Descriptor: m,
diff --git a/rules/internal/utils/http.go b/rules/internal/utils/http.go
index 74d5b80..b6d93c9 100644
--- a/rules/internal/utils/http.go
+++ b/rules/internal/utils/http.go
@@ -26,8 +26,8 @@ import (
 // and then flattens the values in `additional_bindings`.
 // This allows rule authors to simply range over all of the HTTP rules,
 // since the common case is to want to apply the checks to all of them.
-func GetHTTPRules(m *desc.MethodDescriptor) []*apb.HttpRule {
-	rules := []*apb.HttpRule{}
+func GetHTTPRules(m *desc.MethodDescriptor) []*HTTPRule {
+	rules := []*HTTPRule{}
 
 	// Get the method options.
 	opts := m.GetMethodOptions()
@@ -35,12 +35,56 @@ func GetHTTPRules(m *desc.MethodDescriptor) []*apb.HttpRule {
 	// Get the "primary" rule (the direct google.api.http annotation).
 	if x, err := proto.GetExtension(opts, apb.E_Http); err == nil {
 		httpRule := x.(*apb.HttpRule)
-		rules = append(rules, httpRule)
+		if parsedRule := parseRule(httpRule); parsedRule != nil {
+			rules = append(rules, parsedRule)
 
-		// Add any additional bindings and flatten them into `rules`.
-		rules = append(rules, httpRule.GetAdditionalBindings()...)
+			// Add any additional bindings and flatten them into `rules`.
+			for _, binding := range httpRule.GetAdditionalBindings() {
+				rules = append(rules, parseRule(binding))
+			}
+		}
 	}
 
 	// Done; return the rules.
 	return rules
 }
+
+func parseRule(rule *apb.HttpRule) *HTTPRule {
+	oneof := map[string]string{
+		"GET":    rule.GetGet(),
+		"POST":   rule.GetPost(),
+		"PUT":    rule.GetPut(),
+		"PATCH":  rule.GetPatch(),
+		"DELETE": rule.GetDelete(),
+	}
+	if custom := rule.GetCustom(); custom != nil {
+		oneof[custom.GetKind()] = custom.GetPath()
+	}
+	for method, uri := range oneof {
+		if uri != "" {
+			return &HTTPRule{
+				Method:       method,
+				URI:          uri,
+				Body:         rule.GetBody(),
+				ResponseBody: rule.GetResponseBody(),
+			}
+		}
+	}
+	return nil
+}
+
+// HTTPRule defines a parsed, easier-to-query equivalent to `apb.HttpRule`.
+type HTTPRule struct {
+	// The HTTP method. Guaranteed to be in all caps.
+	// This is set to "CUSTOM" if the Custom property is set.
+	Method string
+
+	// The HTTP URI (the value corresponding to the selected HTTP method).
+	URI string
+
+	// The `body` value forwarded from the generated proto's HttpRule.
+	Body string
+
+	// The `response_body` value forwarded from the generated proto's HttpRule.
+	ResponseBody string
+}


Test patch:
diff --git a/rules/internal/utils/http_test.go b/rules/internal/utils/http_test.go
index 5574eaa..4d32296 100644
--- a/rules/internal/utils/http_test.go
+++ b/rules/internal/utils/http_test.go
@@ -15,70 +15,93 @@
 package utils
 
 import (
-	"reflect"
+	"strings"
 	"testing"
 
-	"github.com/golang/protobuf/proto"
-	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
-	"github.com/jhump/protoreflect/desc/builder"
+	"github.com/googleapis/api-linter/rules/internal/testutils"
 	apb "google.golang.org/genproto/googleapis/api/annotations"
 )
 
 func TestGetHTTPRules(t *testing.T) {
-	// Create a MethodOptions with the annotation set.
-	opts := &dpb.MethodOptions{}
-	httpRule := &apb.HttpRule{
-		Pattern: &apb.HttpRule_Get{
-			Get: "/v1/{name=publishers/*/books/*}",
-		},
-		AdditionalBindings: []*apb.HttpRule{{
-			Pattern: &apb.HttpRule_Get{
-				Get: "/v1/{name=books/*}",
-			},
-		}},
-	}
+	for _, method := range []string{"GET", "POST", "PUT", "PATCH", "DELETE"} {
+		t.Run(method, func(t *testing.T) {
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/api/annotations.proto";
+				service Library {
+					rpc FrobBook(FrobBookRequest) returns (FrobBookResponse) {
+						option (google.api.http) = {
+							{{.M}}: "/v1/publishers/*/books/*"
+							additional_bindings {
+								{{.M}}: "/v1/books/*"
+							}
+						};
+					}
+				}
+				message FrobBookRequest {}
+				message FrobBookResponse {}
+			`, struct{ M string }{M: strings.ToLower(method)})
 
-	if err := proto.SetExtension(opts, apb.E_Http, httpRule); err != nil {
-		t.Fatalf("Failed to set google.api.http annotation.")
-	}
+			// Get the rules.
+			resp := GetHTTPRules(file.GetServices()[0].GetMethods()[0])
 
-	// Create a method with the options set.
-	service, err := builder.NewService("Library").AddMethod(
-		builder.NewMethod(
-			"WriteBook",
-			builder.RpcTypeMessage(builder.NewMessage("WriteBookRequest"), false),
-			builder.RpcTypeMessage(builder.NewMessage("WriteBookResponse"), false),
-		).SetOptions(opts),
-	).Build()
-	if err != nil {
-		t.Fatalf("Failed to build service.")
+			// Establish that we get back both HTTP rules, in order.
+			if got, want := resp[0].URI, "/v1/publishers/*/books/*"; got != want {
+				t.Errorf("Rule 1: Got URI %q, expected %q.", got, want)
+			}
+			if got, want := resp[1].URI, "/v1/books/*"; got != want {
+				t.Errorf("Rule 2: Got URI %q, expected %q.", got, want)
+			}
+			for _, httpRules := range resp {
+				if got, want := httpRules.Method, method; got != want {
+					t.Errorf("Got method %q, expected %q.", got, want)
+				}
+			}
+		})
 	}
+}
 
-	// Establish that we get back both HTTP rules.
-	resp := GetHTTPRules(service.GetMethods()[0])
-	if got, want := resp[0], httpRule; got != want {
-		t.Errorf("Expected the first rule to be %v; got %v.", want, got)
-	}
-	if got, want := resp[1], httpRule.GetAdditionalBindings()[0]; got != want {
-		t.Errorf("Expected the second rule ot be %v; got %v.", want, got)
+func TestGetHTTPRulesEmpty(t *testing.T) {
+	file := testutils.ParseProto3String(t, `
+		import "google/api/annotations.proto";
+		service Library {
+			rpc FrobBook(FrobBookRequest) returns (FrobBookResponse);
+		}
+		message FrobBookRequest {}
+		message FrobBookResponse {}
+	`)
+	if resp := GetHTTPRules(file.GetServices()[0].GetMethods()[0]); len(resp) > 0 {
+		t.Errorf("Got %v; expected no rules.", resp)
 	}
 }
 
-func TestGetHTTPRulesEmpty(t *testing.T) {
-	// Create a method with no actual HTTP rules.
-	service, err := builder.NewService("Library").AddMethod(
-		builder.NewMethod(
-			"WriteBook",
-			builder.RpcTypeMessage(builder.NewMessage("WriteBookRequest"), false),
-			builder.RpcTypeMessage(builder.NewMessage("WriteBookResponse"), false),
-		),
-	).Build()
-	if err != nil {
-		t.Fatalf("Failed to build service.")
+func TestParseRuleEmpty(t *testing.T) {
+	http := &apb.HttpRule{}
+	if got := parseRule(http); got != nil {
+		t.Errorf("Got %v, expected nil.", got)
 	}
+}
 
-	// Establish that we get back an empty list of rules.
-	if got, want := GetHTTPRules(service.GetMethods()[0]), []*apb.HttpRule{}; !reflect.DeepEqual(got, want) {
-		t.Errorf("Expected empty slice of rules; got %v.", got)
+func TestGetHTTPRulesCustom(t *testing.T) {
+	file := testutils.ParseProto3String(t, `
+		import "google/api/annotations.proto";
+		service Library {
+			rpc FrobBook(FrobBookRequest) returns (FrobBookResponse) {
+				option (google.api.http) = {
+					custom: {
+						kind: "HEAD"
+						path: "/v1/books/*"
+					}
+				};
+			}
+		}
+		message FrobBookRequest {}
+		message FrobBookResponse {}
+	`)
+	rule := GetHTTPRules(file.GetServices()[0].GetMethods()[0])[0]
+	if got, want := rule.Method, "HEAD"; got != want {
+		t.Errorf("Got %q; expected %q.", got, want)
+	}
+	if got, want := rule.URI, "/v1/books/*"; got != want {
+		t.Errorf("Got %q; expected %q.", got, want)
 	}
 }