Repository: googleapis/api-linter
Pull request #255 (2019-10-23 19:59:57)

[AIP-122] Keys must be snake case.
Also, make the tests better.

lukesneeringer: Marking `do not merge`; this still has one bug.
lukesneeringer: Fixed my bug.

Patch:
diff --git a/rules/aip0122/aip0122.go b/rules/aip0122/aip0122.go
index 81d35e5..ee40e57 100644
--- a/rules/aip0122/aip0122.go
+++ b/rules/aip0122/aip0122.go
@@ -23,6 +23,6 @@ import (
 // this AIP's rules to it.
 func AddRules(r lint.RuleRegistry) {
 	r.Register(
-		camelCase,
+		httpURICase,
 	)
 }
diff --git a/rules/aip0122/http_url_pattern.go b/rules/aip0122/http_url_pattern.go
index b583818..5ac0128 100644
--- a/rules/aip0122/http_url_pattern.go
+++ b/rules/aip0122/http_url_pattern.go
@@ -23,19 +23,59 @@ import (
 )
 
 // HTTP URL pattern shouldn't include underscore("_")
-var camelCase = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0122", "http-url-pattern"),
+var httpURICase = &lint.MethodRule{
+	Name: lint.NewRuleName("core", "0122", "http-uri-case"),
 	URI:  "https://aip.dev/122#guidance",
-	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+	LintMethod: func(m *desc.MethodDescriptor) (problems []lint.Problem) {
 		// Establish that the URI does not include a `_` character.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			if strings.Contains(httpRule.URI, "_") {
-				return []lint.Problem{{
-					Message:    "HTTP URL should use camel case, but not snake case.",
+			parsedURI := parseURI(httpRule.URI)
+			if strings.Contains(parsedURI.pattern, "_") {
+				problems = append(problems, lint.Problem{
+					Message:    "HTTP URI patterns should use camel case, not snake case.",
 					Descriptor: m,
-				}}
+				})
+			}
+			for _, v := range parsedURI.vars {
+				if strings.ToLower(v) != v {
+					problems = append(problems, lint.Problem{
+						Message:    "Variable names in URI patterns should use snake case, not camel case.",
+						Descriptor: m,
+					})
+				}
+			}
+
+			// FIXME: We intentionally only return at most one of each type of `Problem` here.
+			// When we can attach problems to the particular annotation, remove this.
+			if len(problems) > 0 {
+				return
 			}
 		}
-		return nil
+		return
 	},
 }
+
+// extractURI separates variable names from the URI pattern.
+//
+// It does not maintain the relationship between variable names and what
+// part of the pattern they apply to (we do not need that to lint here).
+func parseURI(uri string) (parsed struct {
+	vars    []string
+	pattern string
+}) {
+	parsed.pattern = uri
+	for strings.Contains(parsed.pattern, "{") && strings.Contains(parsed.pattern, "}") {
+		// Find the first {variable=pattern} segment and pull the variable out of it.
+		start, end := strings.Index(uri, "{"), strings.Index(uri, "}")
+		repl := ""
+		for i, segment := range strings.SplitN(parsed.pattern[start+1:end], "=", 2) {
+			if i == 0 {
+				parsed.vars = append(parsed.vars, segment)
+			} else {
+				repl = segment
+			}
+		}
+		parsed.pattern = strings.Replace(parsed.pattern, parsed.pattern[start:end+1], repl, 1)
+	}
+	return
+}
diff --git a/rules/rules.go b/rules/rules.go
index 7339add..cd720a5 100644
--- a/rules/rules.go
+++ b/rules/rules.go
@@ -52,6 +52,7 @@ package rules
 
 import (
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/aip0122"
 	"github.com/googleapis/api-linter/rules/aip0131"
 	"github.com/googleapis/api-linter/rules/aip0132"
 	"github.com/googleapis/api-linter/rules/aip0133"
@@ -70,6 +71,7 @@ import (
 )
 
 func init() {
+	aip0122.AddRules(coreRules)
 	aip0131.AddRules(coreRules)
 	aip0132.AddRules(coreRules)
 	aip0133.AddRules(coreRules)


Test patch:
diff --git a/rules/aip0122/http_url_pattern_test.go b/rules/aip0122/http_url_pattern_test.go
index 0268d9e..a5135ee 100644
--- a/rules/aip0122/http_url_pattern_test.go
+++ b/rules/aip0122/http_url_pattern_test.go
@@ -23,161 +23,35 @@ import (
 func TestHttpUriField(t *testing.T) {
 	tests := []struct {
 		testName string
-		src      string
+		URI      string
 		problems testutils.Problems
 	}{
-		{
-			testName: "Valid",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc CreateInstanceGroup(CreateInstanceGroupsRequest) returns (InstanceGroup) {
-		option (google.api.http) = {
-			post:"/v1/{parent=publishers/*/instanceGroups/*}"
-			body:"instanceGroup"
-		};
-	}
-}
-
-message CreateInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: nil,
-		},
-		{
-			testName: "InValid-Get",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc GetInstanceGroup(GetInstanceGroupsRequest) returns (InstanceGroup) {
-	 option (google.api.http) = {
-		 get: "/v1/{name=publishers/*/instance_groups/*}"
-	 };
+		{"Valid", "/v1/{name=publishers/*/books/*}:frob", testutils.Problems{}},
+		{"ValidCamelPattern", "/v1/{name=publishers/*/frobbableBooks/*}:frob", testutils.Problems{}},
+		{"InvalidSnakePattern", "/v1/{name=publishers/*/frobbable_books/*}:frob", testutils.Problems{{Message: "URI patterns"}}},
+		{"InvalidCamelVariable", "/v1/{bookName=publishers/*/books/*}:frob", testutils.Problems{{Message: "Variable names"}}},
+		{"ValidSnakeVariable", "/v1/{book_name=publishers/*/books/*}:frob", testutils.Problems{}},
+		{"ValidSnakeSoloVariable", "/v1/{book_name}:frob", testutils.Problems{}},
+		{"InvalidCamelSoloVariable", "/v1/{bookName}:frob", testutils.Problems{{Message: "Variable names"}}},
 	}
-}
-
-message GetInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-		{
-			testName: "InValid-Create",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc CreateInstanceGroup(CreateInstanceGroupsRequest) returns (InstanceGroup) {
-	 option (google.api.http) = {
-		 post: "/v1/{parent=publishers/*/instance_groups/*}"
-		 body: "instanceGroup"
-	 };
-	}
-}
-
-message CreateInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-		{
-			testName: "InValid-Delete",
-			//addionalImport:`import "google/protobuf/empty.proto";`,
-			src: `import "google/api/annotations.proto";
-import "google/protobuf/empty.proto";
-
-service InstanceGroupService {
-	rpc DeleteInstanceGroup(DeleteInstanceGroupsRequest) returns (google.protobuf.Empty) {
-	 option (google.api.http) = {
-		 delete: "/v1/{name=publishers/*/instance_groups/*}"
-	 };
-	}
-}
-
-message DeleteInstanceGroupsRequest {}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-		{
-			testName: "InValid-Update_Patch",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc UpdateInstanceGroup(UpdateInstanceGroupsRequest) returns (InstanceGroup) {
-	 option (google.api.http) = {
-		 patch: "/v1/{book.name=publishers/*/instance_groups/*}"
-		 body: "instanceGroup"
-	 };
-	}
-}
-
-message UpdateInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-		{
-			testName: "InValid-Update_Put",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc UpdateInstanceGroup(UpdateInstanceGroupsRequest) returns (InstanceGroup) {
-	 option (google.api.http) = {
-		 put: "/v1/{book.name=publishers/*/instance_groups/*}"
-		 body: "book"
-	 };
-	}
-}
-
-message UpdateInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-		{
-
-			testName: "InValid-Custom",
-			src: `import "google/api/annotations.proto";
-
-service InstanceGroupService {
-	rpc MoveInstanceGroup(MoveInstanceGroupsRequest) returns (InstanceGroup) {
-	 option (google.api.http) = {
-			custom: { path: "/v1/{book.name=publishers/*/instance_groups/*}" }
-	 };
-	}
-}
-
-message MoveInstanceGroupsRequest {}
-
-message InstanceGroup{}
-`,
-			problems: testutils.Problems{{
-				Message: "HTTP URL should use camel case, but not snake case.",
-			}},
-		},
-	}
-
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			file := testutils.ParseProto3String(t, test.src)
-
-			f := file.GetServices()[0].GetMethods()[0]
-			problems := camelCase.Lint(file)
-			if diff := test.problems.SetDescriptor(f).Diff(problems); diff != "" {
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/api/annotations.proto";
+				service Library {
+					rpc FrobBook(FrobBookRequest) returns (FrobBookResponse) {
+						option (google.api.http) = {
+							post: "{{.URI}}"
+							body: "*"
+						};
+					}
+				}
+				message FrobBookRequest {}
+				message FrobBookResponse {}
+			`, test)
+			method := file.GetServices()[0].GetMethods()[0]
+			problems := httpURICase.Lint(file)
+			if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
 				t.Errorf(diff)
 			}
 		})