Repository: googleapis/api-linter
Pull request #338 (2019-11-12 21:16:17)

[feat] Move URI parsing into HTTPRule.
This makes URI parsing in HTTPRule better, and moves it from
a utility method in a single API into something that can be used
elsewhere.


Patch:
diff --git a/rules/aip0122/camel_case_uris.go b/rules/aip0122/camel_case_uris.go
index 20ff6a5..2793b0f 100644
--- a/rules/aip0122/camel_case_uris.go
+++ b/rules/aip0122/camel_case_uris.go
@@ -28,14 +28,13 @@ var httpURICase = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) (problems []lint.Problem) {
 		// Establish that the URI does not include a `_` character.
 		for _, httpRule := range utils.GetHTTPRules(m) {
-			parsedURI := parseURI(httpRule.URI)
-			if strings.Contains(parsedURI.pattern, "_") {
+			if strings.Contains(httpRule.GetPlainURI(), "_") {
 				problems = append(problems, lint.Problem{
 					Message:    "HTTP URI patterns should use camel case, not snake case.",
 					Descriptor: m,
 				})
 			}
-			for _, v := range parsedURI.vars {
+			for v := range httpRule.GetVariables() {
 				if strings.ToLower(v) != v {
 					problems = append(problems, lint.Problem{
 						Message:    "Variable names in URI patterns should use snake case, not camel case.",
@@ -53,28 +52,3 @@ var httpURICase = &lint.MethodRule{
 		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/internal/utils/http.go b/rules/internal/utils/http.go
index b6d93c9..f8dc74b 100644
--- a/rules/internal/utils/http.go
+++ b/rules/internal/utils/http.go
@@ -15,6 +15,8 @@
 package utils
 
 import (
+	"regexp"
+
 	"github.com/golang/protobuf/proto"
 	"github.com/jhump/protoreflect/desc"
 	apb "google.golang.org/genproto/googleapis/api/annotations"
@@ -88,3 +90,23 @@ type HTTPRule struct {
 	// The `response_body` value forwarded from the generated proto's HttpRule.
 	ResponseBody string
 }
+
+// GetVariables returns the variable segments in a URI as a map.
+func (h *HTTPRule) GetVariables() map[string]string {
+	vars := map[string]string{}
+	for _, match := range plainVar.FindAllStringSubmatch(h.URI, -1) {
+		vars[match[1]] = "*"
+	}
+	for _, match := range varSegment.FindAllStringSubmatch(h.URI, -1) {
+		vars[match[1]] = match[2]
+	}
+	return vars
+}
+
+// GetPlainURI returns the URI with variable segment information removed.
+func (h *HTTPRule) GetPlainURI() string {
+	return plainVar.ReplaceAllString(varSegment.ReplaceAllString(h.URI, "$2"), "*")
+}
+
+var plainVar = regexp.MustCompile(`\{([^}=]+)}`)
+var varSegment = regexp.MustCompile(`\{([^}=]+)=([^}]+)\}`)


Test patch:
diff --git a/rules/internal/utils/http_test.go b/rules/internal/utils/http_test.go
index 4d32296..d7c85a6 100644
--- a/rules/internal/utils/http_test.go
+++ b/rules/internal/utils/http_test.go
@@ -18,6 +18,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/googleapis/api-linter/rules/internal/testutils"
 	apb "google.golang.org/genproto/googleapis/api/annotations"
 )
@@ -105,3 +106,43 @@ func TestGetHTTPRulesCustom(t *testing.T) {
 		t.Errorf("Got %q; expected %q.", got, want)
 	}
 }
+
+func TestGetPlainURI(t *testing.T) {
+	tests := []struct {
+		name     string
+		uri      string
+		plainURI string
+	}{
+		{"KeyOnly", "/v1/publishers/{pub_id}/books/{book_id}", "/v1/publishers/*/books/*"},
+		{"KeyValue", "/v1/{name=publishers/*/books/*}", "/v1/publishers/*/books/*"},
+		{"MultiKeyValue", "/v1/{publisher=publishers/*}/{book=books/*}", "/v1/publishers/*/books/*"},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			rule := &HTTPRule{URI: test.uri}
+			if diff := cmp.Diff(rule.GetPlainURI(), test.plainURI); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestGetVariables(t *testing.T) {
+	tests := []struct {
+		name string
+		uri  string
+		vars map[string]string
+	}{
+		{"KeyOnly", "/v1/publishers/{pub_id}/books/{book_id}", map[string]string{"pub_id": "*", "book_id": "*"}},
+		{"KeyValue", "/v1/{name=publishers/*/books/*}", map[string]string{"name": "publishers/*/books/*"}},
+		{"MultiKeyValue", "/v1/{publisher=publishers/*}/{book=books/*}", map[string]string{"publisher": "publishers/*", "book": "books/*"}},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			rule := &HTTPRule{URI: test.uri}
+			if diff := cmp.Diff(rule.GetVariables(), test.vars); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}