Repository: googleapis/api-linter
Pull request #188 (2019-09-19 14:49:31)

[AIP-131] Add rules to check the annotation.


Patch:
diff --git a/rules/aip0131/aip0131.go b/rules/aip0131/aip0131.go
index f33c7f6..bd1abbe 100644
--- a/rules/aip0131/aip0131.go
+++ b/rules/aip0131/aip0131.go
@@ -26,15 +26,19 @@ import (
 // this AIP's rules to it.
 func AddRules(r lint.RuleRegistry) {
 	r.Register(
+		httpBody,
+		httpNameField,
+		httpVerb,
+		responseMessageName,
 		requestMessageName,
 		standardFields,
 		unknownFields,
-		responseMessageName,
 	)
 }
 
 var getMethodRegexp = regexp.MustCompile("^Get(?:[A-Z]|$)")
 var getReqMessageRegexp = regexp.MustCompile("^Get[A-Za-z0-9]*Request$")
+var getURINameRegexp = regexp.MustCompile("\\{name=[a-zA-Z/*]+\\}$")
 
 // Returns true if this is a AIP-131 Get method, false otherwise.
 func isGetMethod(m *desc.MethodDescriptor) bool {
diff --git a/rules/aip0131/method_rules.go b/rules/aip0131/method_rules.go
index ecd27f6..84bb1ec 100644
--- a/rules/aip0131/method_rules.go
+++ b/rules/aip0131/method_rules.go
@@ -18,6 +18,7 @@ import (
 	"fmt"
 
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
 )
 
@@ -76,3 +77,80 @@ var responseMessageName = &lint.MethodRule{
 		return nil
 	},
 }
+
+// Get methods should use the HTTP GET verb.
+var httpVerb = &lint.MethodRule{
+	Name: lint.NewRuleName("core", "0131", "http-verb"),
+	URI:  "https://aip.dev/131#guidance",
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		// We only care about Get methods for the purpose of this rule;
+		// ignore everything else.
+		if !isGetMethod(m) {
+			return nil
+		}
+
+		// Rule check: Establish that the RPC uses HTTP GET.
+		for _, httpRule := range utils.GetHTTPRules(m) {
+			if httpRule.GetGet() == "" {
+				return []lint.Problem{{
+					Message:    "Get methods must use the HTTP GET verb.",
+					Descriptor: m,
+				}}
+			}
+		}
+
+		return nil
+	},
+}
+
+// Get methods should have a proper HTTP pattern.
+var httpNameField = &lint.MethodRule{
+	Name: lint.NewRuleName("core", "0131", "http-body"),
+	URI:  "https://aip.dev/131#guidance",
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		// We only care about Get methods for the purpose of this rule;
+		// ignore everything else.
+		if !isGetMethod(m) {
+			return nil
+		}
+
+		// 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,
+					}}
+				}
+			}
+		}
+
+		return nil
+	},
+}
+
+// Get methods should not have an HTTP body.
+var httpBody = &lint.MethodRule{
+	Name: lint.NewRuleName("core", "0131", "http-name"),
+	URI:  "https://aip.dev/131#guidance",
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		// We only care about Get methods for the purpose of this rule;
+		// ignore everything else.
+		if !isGetMethod(m) {
+			return nil
+		}
+
+		// Establish that the RPC has no HTTP body.
+		for _, httpRule := range utils.GetHTTPRules(m) {
+			if httpRule.GetBody() != "" {
+				return []lint.Problem{{
+					Message:    "Get methods should not have an HTTP body.",
+					Descriptor: m,
+				}}
+			}
+		}
+
+		return nil
+	},
+}


Test patch:
diff --git a/rules/aip0131/method_rules_test.go b/rules/aip0131/method_rules_test.go
index 3702714..c5eb095 100644
--- a/rules/aip0131/method_rules_test.go
+++ b/rules/aip0131/method_rules_test.go
@@ -15,9 +15,13 @@
 package aip0131
 
 import (
+	"strings"
 	"testing"
 
+	"github.com/golang/protobuf/proto"
+	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/jhump/protoreflect/desc/builder"
+	"google.golang.org/genproto/googleapis/api/annotations"
 )
 
 func TestRequestMessageName(t *testing.T) {
@@ -92,3 +96,154 @@ func TestResponseMessageName(t *testing.T) {
 		})
 	}
 }
+
+func TestHttpVerb(t *testing.T) {
+	// Set up GET and POST HTTP annotations.
+	httpGet := &annotations.HttpRule{
+		Pattern: &annotations.HttpRule_Get{
+			Get: "/v1/{name=publishers/*/books/*}",
+		},
+	}
+	httpPost := &annotations.HttpRule{
+		Pattern: &annotations.HttpRule_Post{
+			Post: "/v1/{name=publishers/*/books/*}",
+		},
+	}
+
+	// Set up testing permutations.
+	tests := []struct {
+		testName   string
+		httpRule   *annotations.HttpRule
+		methodName string
+		msg        string
+	}{
+		{"Valid", httpGet, "GetBook", ""},
+		{"Invalid", httpPost, "GetBook", "HTTP GET"},
+		{"Irrelevant", httpPost, "AcquireBook", ""},
+	}
+
+	// Run each test.
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			// Create a MethodOptions with the annotation set.
+			opts := &dpb.MethodOptions{}
+			if err := proto.SetExtension(opts, annotations.E_Http, test.httpRule); err != nil {
+				t.Fatalf("Failed to set google.api.http annotation.")
+			}
+
+			// Create a minimal service with a AIP-131 Get method
+			// (or with a different method, in the "Irrelevant" case).
+			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
+				builder.RpcTypeMessage(builder.NewMessage("GetBookRequest"), false),
+				builder.RpcTypeMessage(builder.NewMessage("Book"), false),
+			).SetOptions(opts)).Build()
+			if err != nil {
+				t.Fatalf("Could not build %s method.", test.methodName)
+			}
+
+			// Run the method, ensure we get what we expect.
+			problems := httpVerb.Lint(service.GetFile())
+			if test.msg == "" && len(problems) > 0 {
+				t.Errorf("Got %v, expected no problems.", problems)
+			} else if test.msg != "" && !strings.Contains(problems[0].Message, test.msg) {
+				t.Errorf("Got %q, expected message containing %q", problems[0].Message, test.msg)
+			}
+		})
+	}
+}
+
+func TestHttpBody(t *testing.T) {
+	tests := []struct {
+		testName   string
+		body       string
+		methodName string
+		msg        string
+	}{
+		{"Valid", "", "GetBook", ""},
+		{"Invalid", "*", "GetBook", "HTTP body"},
+		{"Irrelevant", "*", "AcquireBook", ""},
+	}
+
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			// Create a MethodOptions with the annotation set.
+			opts := &dpb.MethodOptions{}
+			httpRule := &annotations.HttpRule{
+				Pattern: &annotations.HttpRule_Get{
+					Get: "/v1/{name=publishers/*/books/*}",
+				},
+				Body: test.body,
+			}
+			if err := proto.SetExtension(opts, annotations.E_Http, httpRule); err != nil {
+				t.Fatalf("Failed to set google.api.http annotation.")
+			}
+
+			// Create a minimal service with a AIP-131 Get method
+			// (or with a different method, in the "Irrelevant" case).
+			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
+				builder.RpcTypeMessage(builder.NewMessage("GetBookRequest"), false),
+				builder.RpcTypeMessage(builder.NewMessage("Book"), false),
+			).SetOptions(opts)).Build()
+			if err != nil {
+				t.Fatalf("Could not build %s method.", test.methodName)
+			}
+
+			// Run the method, ensure we get what we expect.
+			problems := httpBody.Lint(service.GetFile())
+			if test.msg == "" && len(problems) > 0 {
+				t.Errorf("Got %v, expected no problems.", problems)
+			} else if test.msg != "" && !strings.Contains(problems[0].Message, test.msg) {
+				t.Errorf("Got %q, expected message containing %q", problems[0].Message, test.msg)
+			}
+		})
+	}
+}
+
+func TestHttpNameField(t *testing.T) {
+	tests := []struct {
+		testName   string
+		uri        string
+		methodName string
+		msg        string
+	}{
+		{"Valid", "/v1/{name=publishers/*/books/*}", "GetBook", ""},
+		{"InvalidVarName", "/v1/{book=publishers/*/books/*}", "GetBook", "`name` field"},
+		{"NoVarName", "/v1/publishers/*/books/*", "GetBook", "`name` field"},
+		{"Irrelevant", "/v1/{book=publishers/*/books/*}", "AcquireBook", ""},
+	}
+
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			// Create a MethodOptions with the annotation set.
+			opts := &dpb.MethodOptions{}
+			httpRule := &annotations.HttpRule{
+				Pattern: &annotations.HttpRule_Get{
+					Get: test.uri,
+				},
+			}
+			if err := proto.SetExtension(opts, annotations.E_Http, httpRule); err != nil {
+				t.Fatalf("Failed to set google.api.http annotation.")
+			}
+
+			// Create a minimal service with a AIP-131 Get method
+			// (or with a different method, in the "Irrelevant" case).
+			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
+				builder.RpcTypeMessage(builder.NewMessage("GetBookRequest"), false),
+				builder.RpcTypeMessage(builder.NewMessage("Book"), false),
+			).SetOptions(opts)).Build()
+			if err != nil {
+				t.Fatalf("Could not build %s method.", test.methodName)
+			}
+
+			// Run the method, ensure we get what we expect.
+			problems := httpNameField.Lint(service.GetFile())
+			if test.msg == "" && len(problems) > 0 {
+				t.Errorf("Got %v, expected no problems.", problems)
+			} else if test.msg != "" && len(problems) == 0 {
+				t.Errorf("Got no problems, expected 1.")
+			} else if test.msg != "" && !strings.Contains(problems[0].Message, test.msg) {
+				t.Errorf("Got %q, expected message containing %q", problems[0].Message, test.msg)
+			}
+		})
+	}
+}