Repository: googleapis/api-linter
Pull request #1045 (2022-11-28 02:03:54)

feat: Validate that HTTP variable templates match resource pattern definitions (AIP-127)
Part of #972 

acamadeo: Thanks for the first pass! And whoops, sorry for flooding the chat by "Starting a review"! 🙃
noahdietz: Great job @acamadeo thank you for your hard work on this.

Patch:
diff --git a/docs/rules/0127/http-template-pattern.md b/docs/rules/0127/http-template-pattern.md
new file mode 100644
index 0000000..68a48d4
--- /dev/null
+++ b/docs/rules/0127/http-template-pattern.md
@@ -0,0 +1,97 @@
+---
+rule:
+  aip: 127
+  name: [core, '0127', http-template-pattern]
+  summary: |
+    HTTP template variable patterns should match the patterns defined by their resources.
+permalink: /127/http-template-pattern
+redirect_from:
+  - /127/http-template-pattern
+---
+
+# HTTP Pattern Variables
+
+This rule enforces that any HTTP annotations that reference a resource must
+match one of the pattern strings defined by that resource, as mandated in
+[AIP-127][].
+
+## Details
+
+This rule ensures that `google.api.http` path template variables that represent
+a resource name match one of the resource name patterns of the resource that the
+field being referenced represents.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+// The template for the `name` variable in the `google.api.http` annotation
+// is missing segments from the Book message's `pattern`.
+rpc GetBook(GetBookRequest) returns (Book) {
+    option (google.api.http) = {
+        get: "v1/{name=shelves/*}"
+    };
+}
+message GetBookRequest {
+    string name = 1 [
+        (google.api.resource_reference).type = "library.googleapis.com/Book"
+    ];
+}
+message Book {
+    option (google.api.resource) = {
+        type: "library.googleapis.com/Book"
+        pattern: "shelves/{shelf}/books/{book}"
+    };
+
+    // Book resource name.
+    string name = 1;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+rpc GetBook(GetBookRequest) returns (Book) {
+    option (google.api.http) = {
+        get: "v1/{name=shelves/*/books/*}"
+    };
+}
+message GetBookRequest {
+    string name = 1 [
+        (google.api.resource_reference).type = "library.googleapis.com/Book"
+    ];
+}
+message Book {
+    option (google.api.resource) = {
+        type: "library.googleapis.com/Book"
+        pattern: "shelves/{shelf}/books/{book}"
+    };
+
+    // Book resource name.
+    string name = 1;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the field.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0127::http-template-pattern=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+rpc GetBook(GetBookRequest) returns (Book) {
+    option (google.api.http) = {
+        get: "v1/{name=shelves/*}"
+    };
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-127]: https://aip.dev/127
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
\ No newline at end of file
diff --git a/rules/aip0127/aip0127.go b/rules/aip0127/aip0127.go
index 8744808..cd4f03e 100644
--- a/rules/aip0127/aip0127.go
+++ b/rules/aip0127/aip0127.go
@@ -24,6 +24,7 @@ func AddRules(r lint.RuleRegistry) error {
 	return r.Register(
 		127,
 		hasAnnotation,
+		httpTemplatePattern,
 		leadingSlash,
 		resourceNameExtraction,
 	)
diff --git a/rules/aip0127/http_template_pattern.go b/rules/aip0127/http_template_pattern.go
new file mode 100644
index 0000000..80867a4
--- /dev/null
+++ b/rules/aip0127/http_template_pattern.go
@@ -0,0 +1,136 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0127
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/locations"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
+)
+
+var (
+	resourcePatternSegment     = `[^/]+`
+	resourcePatternAnySegments = fmt.Sprintf("((%s/)*%s)?", resourcePatternSegment, resourcePatternSegment)
+	pathTemplateToRegex        = strings.NewReplacer("**", resourcePatternAnySegments, "*", resourcePatternSegment)
+)
+
+type resourceReference struct {
+	// The path of the field with the `google.api.resource_reference`. This is
+	// provided as a variable in the HTTPRule.
+	fieldPath string
+	// A template that the resource's pattern string must adhere to. This is
+	// provided by the variable's template in the HTTPRule.
+	pathTemplate string
+	// The name of the resource type. This is used to look up the resource
+	// message.
+	resourceRefName string
+}
+
+// Returns a list of resourceReferences for each variable in all the method's
+// HTTPRule's.
+func methodResourceReferences(m *desc.MethodDescriptor) []resourceReference {
+	resourceRefs := []resourceReference{}
+	for _, httpRule := range utils.GetHTTPRules(m) {
+		resourceRefs = append(resourceRefs, httpResourceReferences(httpRule, m.GetInputType())...)
+	}
+	return resourceRefs
+}
+
+// Returns a resourceReference for every variable in the given HTTPRule.
+func httpResourceReferences(httpRule *utils.HTTPRule, msg *desc.MessageDescriptor) []resourceReference {
+	resourceRefs := []resourceReference{}
+	for fieldPath, template := range httpRule.GetVariables() {
+		// Find the (sub-)field in the message corresponding to the variable's
+		// field path.
+		field := utils.FindFieldDotNotation(msg, fieldPath)
+		if field == nil {
+			continue
+		}
+
+		// Extract the name of the resource referenced by this field.
+		ref := utils.GetResourceReference(field)
+		if ref == nil || ref.GetChildType() != "" {
+			// TODO(#1047): Support the case where a resource has
+			// multiple parent resources.
+			continue
+		}
+
+		resourceRefs = append(resourceRefs, resourceReference{
+			fieldPath:       fieldPath,
+			pathTemplate:    template,
+			resourceRefName: ref.GetType(),
+		})
+	}
+	return resourceRefs
+}
+
+// Constructs a regex from the HTTPRule's path template representing resource
+// patterns that it will match against.
+func compilePathTemplateRegex(pathTemplate string) (*regexp.Regexp, error) {
+	pattern := fmt.Sprintf("^%s$", pathTemplateToRegex.Replace(pathTemplate))
+	return regexp.Compile(pattern)
+}
+
+func anyMatch(regex *regexp.Regexp, strs []string) bool {
+	for _, str := range strs {
+		if regex.MatchString(str) {
+			return true
+		}
+	}
+	return false
+}
+
+// Checks whether the HTTP pattern specified in `resourceRef` matches any of the
+// patterns defined for that resource.
+func checkHttpPatternMatchesResource(m *desc.MethodDescriptor, resourceRef resourceReference) []lint.Problem {
+	annotation := utils.FindResource(resourceRef.resourceRefName, m.GetFile())
+	if annotation == nil {
+		return []lint.Problem{}
+	}
+
+	pathRegex, err := compilePathTemplateRegex(resourceRef.pathTemplate)
+	if err != nil {
+		return []lint.Problem{}
+	}
+
+	if !anyMatch(pathRegex, annotation.GetPattern()) {
+		message := fmt.Sprintf("The HTTP pattern %q does not match any of the patterns for resource %q", resourceRef.pathTemplate, resourceRef.resourceRefName)
+		return []lint.Problem{{Message: message, Descriptor: m, Location: locations.MethodHTTPRule(m)}}
+	}
+
+	return []lint.Problem{}
+}
+
+var httpTemplatePattern = &lint.MethodRule{
+	Name: lint.NewRuleName(127, "http-template-pattern"),
+	OnlyIf: func(m *desc.MethodDescriptor) bool {
+		return len(methodResourceReferences(m)) > 0
+	},
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		problems := []lint.Problem{}
+
+		resourceRefs := methodResourceReferences(m)
+		for _, resourceRef := range resourceRefs {
+			problems = append(problems, checkHttpPatternMatchesResource(m, resourceRef)...)
+		}
+
+		return problems
+	},
+}
diff --git a/rules/internal/utils/http.go b/rules/internal/utils/http.go
index 2b03c52..f4b0386 100644
--- a/rules/internal/utils/http.go
+++ b/rules/internal/utils/http.go
@@ -99,6 +99,12 @@ type HTTPRule struct {
 }
 
 // GetVariables returns the variable segments in a URI as a map.
+//
+// For a given variable, the key is the variable's field path. The value is the
+// variable's template, which will match segment(s) of the URL.
+//
+// For more details on the path template syntax, see
+// https://github.com/googleapis/googleapis/blob/6e1a5a066659794f26091674e3668229e7750052/google/api/http.proto#L224.
 func (h *HTTPRule) GetVariables() map[string]string {
 	vars := map[string]string{}
 


Test patch:
diff --git a/rules/aip0127/http_template_pattern_test.go b/rules/aip0127/http_template_pattern_test.go
new file mode 100644
index 0000000..57b892e
--- /dev/null
+++ b/rules/aip0127/http_template_pattern_test.go
@@ -0,0 +1,204 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0127
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestHttpTemplatePattern_PatternMatching(t *testing.T) {
+	tests := []struct {
+		name            string
+		HttpAnnotation  string
+		ResourcePattern string
+		problems        testutils.Problems
+	}{
+		// HTTP variable uses literals
+		{"LiteralMatchesSameLiteralInPattern", "/v1/{name=shelves}", "shelves", nil},
+		{"LiteralDoesNotMatchDifferentLiteral", "/v1/{name=shelves}", "books", testutils.Problems{{Message: "does not match"}}},
+		{"SuffixAfterHttpVariableIgnoredForMatch", "/v1/{name=shelves}/books", "shelves", nil},
+
+		// HTTP variable uses single wildcard
+		{"SingleWildcardMatchesAnyLiteralSegment", "/v1/{name=*}", "shelves", nil},
+		{"SingleWildcardMatchesAnyVariableSegment", "/v1/{name=*}", "{shelf}", nil},
+		{"SingleWildcardDoesNotMatchMultipleUrlSegments", "/v1/{name=*}", "shelves/{shelf}", testutils.Problems{{Message: "does not match"}}},
+		{"LiteralAndWildcardMatch", "/v1/{name=shelves/*}", "shelves/{shelf}", nil},
+		// This case is only theoretical, as "{shelf}" represents a resource ID,
+		// rather than a resource name. It should not be observed in practice.
+		{"ImplicitWildcardMatches", "/v1/{name}/books", "{shelf}", nil},
+		{"MulitpleWildcardsMatches", "/v1/{name=shelves/*/books/*}", "shelves/{shelf}/books/{book}", nil},
+
+		// HTTP variable uses double wildcard
+		{"DoubleWildcardMatchesZeroSegments", "/v1/{name=**}", "", nil},
+		{"DoubleWildcardMatchesOneLiteralSegment", "/v1/{name=**}", "shelves", nil},
+		{"DoubleWildcardMatchesMultipleLiteralSegments", "/v1/{name=**}", "my/shelves", nil},
+		{"DoubleWildcardMatchesOneVariableSegment", "/v1/{name=**}", "{shelf}", nil},
+		{"DoubleWildcardMatchesMultipleVariableSegments", "/v1/{name=**}", "{shelf}/{book}", nil},
+		{"DoubleWildcardMatchesMixedLiteralVariableSegments", "/v1/{name=**}", "shelves/{shelf}", nil},
+		{"DoubleWildcardPrecededByLiteralMatches", "/v1/{name=shelves/**}", "shelves/{shelf}", nil},
+		{"DoubleWildcardFollowedByLiteralMatches", "/v1/{name=**/shelves/*}", "my/shelves/{shelf}", nil},
+		{"DoubleWildcardMissingLiteralDoesNotMatch", "/v1/{name=shelves/**}", "{shelf}", testutils.Problems{{Message: "does not match"}}},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/annotations.proto";
+				import "google/api/resource.proto";
+				service Library {
+					rpc GetBook(GetBookRequest) returns (Book) {
+						option (google.api.http) = {
+							get: "{{.HttpAnnotation}}"
+						};
+					}
+				}
+				message GetBookRequest {
+					// Format: shelves/{shelf}/books/{book}
+					string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"];
+				}
+				message Book {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/Book"
+						pattern: "{{.ResourcePattern}}"
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(httpTemplatePattern.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestHttpTemplatePattern_MultiplePatterns(t *testing.T) {
+	tests := []struct {
+		name             string
+		HttpAnnotation   string
+		ResourcePattern1 string
+		ResourcePattern2 string
+		problems         testutils.Problems
+	}{
+		{"MatchesIfFirstPatternMatches", "/v1/{name=shelves}", "shelves", "books", nil},
+		{"MatchesIfSecondPatternMatches", "/v1/{name=shelves}", "books", "shelves", nil},
+		{"FailsIfNeitherPatternMatches", "/v1/{name=shelves}", "books", "bins", testutils.Problems{{Message: "does not match"}}},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/annotations.proto";
+				import "google/api/resource.proto";
+				service Library {
+					rpc GetBook(GetBookRequest) returns (Book) {
+						option (google.api.http) = {
+							get: "{{.HttpAnnotation}}"
+						};
+					}
+				}
+				message GetBookRequest {
+					string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"];
+				}
+				message Book {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/Book"
+						pattern: "{{.ResourcePattern1}}"
+						pattern: "{{.ResourcePattern2}}"
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(httpTemplatePattern.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestHttpTemplatePattern_SkipCheckIfNoHTTPRules(t *testing.T) {
+	f := testutils.ParseProto3String(t, `
+			import "google/api/annotations.proto";
+			import "google/api/resource.proto";
+			service Library {
+				rpc GetBook(GetBookRequest) returns (Book) {}
+			}
+			message GetBookRequest {
+				string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"];
+			}
+			message Book {
+				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
+				};
+				string name = 1;
+			}
+		`)
+	if problems := httpTemplatePattern.Lint(f); len(problems) > 0 {
+		t.Errorf("%v", problems)
+	}
+}
+
+func TestHttpTemplatePattern_SkipCheckIfHTTPRuleHasNoVariables(t *testing.T) {
+	f := testutils.ParseProto3String(t, `
+			import "google/api/annotations.proto";
+			import "google/api/resource.proto";
+			service Library {
+				rpc GetBook(GetBookRequest) returns (Book) {
+					option (google.api.http) = {
+						get: "/v1/book"
+					};
+				}
+			}
+			message GetBookRequest {
+				string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"];
+			}
+			message Book {
+				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
+				};
+				string name = 1;
+			}
+		`)
+	if problems := httpTemplatePattern.Lint(f); len(problems) > 0 {
+		t.Errorf("%v", problems)
+	}
+}
+
+func TestHttpTemplatePattern_SkipCheckIfFieldPathMissingResourceAnnotation(t *testing.T) {
+	f := testutils.ParseProto3String(t, `
+			import "google/api/annotations.proto";
+			import "google/api/resource.proto";
+			service Library {
+				rpc GetBook(GetBookRequest) returns (Book) {
+					option (google.api.http) = {
+						get: "/v1/{name=shelves}"
+					};
+				}
+			}
+			message GetBookRequest {
+				string name = 1;
+			}
+			message Book {
+				option (google.api.resource) = {
+					type: "library.googleapis.com/Book"
+				};
+				string name = 1;
+			}
+		`)
+	if problems := httpTemplatePattern.Lint(f); len(problems) > 0 {
+		t.Errorf("%v", problems)
+	}
+}