Repository: googleapis/api-linter
Pull request #418 (2019-12-06 00:03:15)

[AIP-123] Add resource name pattern enforcement.
~Uses #417 as a base.~ #417 is merged.


Patch:
diff --git a/docs/rules/0123/resource-pattern.md b/docs/rules/0123/resource-pattern.md
new file mode 100644
index 0000000..d98f562
--- /dev/null
+++ b/docs/rules/0123/resource-pattern.md
@@ -0,0 +1,98 @@
+---
+rule:
+  aip: 123
+  name: [core, '0123', resource-pattern]
+  summary: Resource annotations should define a pattern.
+permalink: /123/resource-pattern
+redirect_from:
+  - /0123/resource-pattern
+---
+
+# Resource patterns
+
+This rule enforces that messages that appear to represent resources have a
+`pattern` defined on their `google.api.resource` annotation, as described in
+[AIP-123][].
+
+## Details
+
+This rule scans all messages with `google.api.resource` annotations, and
+complains if `pattern` is not provided at least once. It also complains if the
+segments outside of variable names contain underscores.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    // pattern should be here
+  }
+
+  string name = 1;
+}
+```
+
+```proto
+// Incorrect.
+message ElectronicBook {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/ElectronicBook"
+    // Should be: publishers/{publisher}/electronicBooks/{electronic_book}
+    pattern: "publishers/{publisher}/electronic_books/{electronic_book}"
+  }
+
+  string name = 1;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher}/books/{book}"
+  };
+
+  string name = 1;
+}
+```
+
+```proto
+// Correct.
+message ElectronicBook {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/ElectronicBook"
+    pattern: "publishers/{publisher}/electronicBooks/{electronic_book}"
+  };
+
+  string name = 1;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message.
+
+```proto
+// (-- api-linter: core::0123::resource-pattern=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+  }
+
+  string name = 1;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-123]: http://aip.dev/123
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/docs/rules/0123/resource-variables.md b/docs/rules/0123/resource-variables.md
new file mode 100644
index 0000000..8659987
--- /dev/null
+++ b/docs/rules/0123/resource-variables.md
@@ -0,0 +1,98 @@
+---
+rule:
+  aip: 123
+  name: [core, '0123', resource-variables]
+  summary: Resource patterns should use consistent variable naming.
+permalink: /123/resource-variables
+redirect_from:
+  - /0123/resource-variables
+---
+
+# Resource pattern variables
+
+This rule enforces that resource patterns use consistent variable naming
+conventions, as described in [AIP-123][].
+
+## Details
+
+This rule scans all messages with `google.api.resource` annotations, and
+complains if variables in a `pattern` use camel case, or end in `_id`.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    // Should be: publishers/{publisher}/books/{book}
+    pattern: "publishers/{publisher_id}/books/{book_id}"
+  }
+
+  string name = 1;
+}
+```
+
+```proto
+// Incorrect.
+message ElectronicBook {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/ElectronicBook"
+    // Should be: publishers/{publisher}/electronicBooks/{electronic_book}
+    pattern: "publishers/{publisher}/electronicBooks/{electronicBook}"
+  }
+
+  string name = 1;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher}/books/{book}"
+  };
+
+  string name = 1;
+}
+```
+
+```proto
+// Correct.
+message ElectronicBook {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/ElectronicBook"
+    pattern: "publishers/{publisher}/electronicBooks/{electronic_book}"
+  };
+
+  string name = 1;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message.
+
+```proto
+// (-- api-linter: core::0123::resource-variables=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher_id}/books/{book_id}"
+  }
+
+  string name = 1;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-123]: http://aip.dev/123
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/rules/aip0123/aip0123.go b/rules/aip0123/aip0123.go
index c25da3f..d878883 100644
--- a/rules/aip0123/aip0123.go
+++ b/rules/aip0123/aip0123.go
@@ -16,10 +16,14 @@
 package aip0123
 
 import (
+	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
+	"github.com/stoewer/go-strcase"
 )
 
 // AddRules accepts a register function and registers each of
@@ -28,9 +32,52 @@ func AddRules(r lint.RuleRegistry) error {
 	return r.Register(
 		123,
 		resourceAnnotation,
+		resourcePattern,
+		resourceVariables,
 	)
 }
 
 func isResourceMessage(m *desc.MessageDescriptor) bool {
 	return m.FindFieldByName("name") != nil && !strings.HasSuffix(m.GetName(), "Request")
 }
+
+func hasResourceAnnotation(m *desc.MessageDescriptor) bool {
+	return utils.GetResource(m) != nil
+}
+
+// getVariables returns a slice of variables declared in the pattern.
+//
+// For example, a pattern of "publishers/{publisher}/books/{book}" would
+// return []string{"publisher", "book"}.
+func getVariables(pattern string) []string {
+	answer := []string{}
+	for _, match := range varRegexp.FindAllStringSubmatch(pattern, -1) {
+		answer = append(answer, match[1])
+	}
+	return answer
+}
+
+// getPlainPattern returns the pattern with all variables replaced with "*".
+//
+// For example, a pattern of "publishers/{publisher}/books/{book}" would
+// return "publishers/*/books/*".
+func getPlainPattern(pattern string) string {
+	return varRegexp.ReplaceAllLiteralString(pattern, "*")
+}
+
+// getDesiredPattern returns the expected desired pattern, with errors we
+// lint for corrected.
+func getDesiredPattern(pattern string) string {
+	want := []string{}
+	for _, token := range strings.Split(pattern, "/") {
+		if strings.HasPrefix(token, "{") && strings.HasSuffix(token, "}") {
+			varname := token[1 : len(token)-1]
+			want = append(want, fmt.Sprintf("{%s}", strings.TrimSuffix(strcase.SnakeCase(varname), "_id")))
+		} else {
+			want = append(want, strcase.LowerCamelCase(token))
+		}
+	}
+	return strings.Join(want, "/")
+}
+
+var varRegexp = regexp.MustCompile(`\{([^}=]+)}`)
diff --git a/rules/aip0123/resource_pattern.go b/rules/aip0123/resource_pattern.go
new file mode 100644
index 0000000..fd8e60d
--- /dev/null
+++ b/rules/aip0123/resource_pattern.go
@@ -0,0 +1,57 @@
+// Copyright 2019 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 aip0123
+
+import (
+	"fmt"
+	"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 resourcePattern = &lint.MessageRule{
+	Name:   lint.NewRuleName(123, "resource-pattern"),
+	OnlyIf: hasResourceAnnotation,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		resource := utils.GetResource(m)
+		// Are any patterns declared at all? If not, complain.
+		if len(resource.GetPattern()) == 0 {
+			return []lint.Problem{{
+				Message:    "Resources should declare resource name pattern(s).",
+				Descriptor: m,
+				Location:   locations.MessageResource(m),
+			}}
+		}
+
+		// Ensure that the constant segments of the pattern uses camel case,
+		// not snake case.
+		for _, pattern := range resource.GetPattern() {
+			if strings.Contains(getPlainPattern(pattern), "_") {
+				return []lint.Problem{{
+					Message: fmt.Sprintf(
+						"Resource patterns should use camel case (apart from the variable names), such as %q.",
+						getDesiredPattern(pattern),
+					),
+					Descriptor: m,
+					Location:   locations.MessageResource(m),
+				}}
+			}
+		}
+		return nil
+	},
+}
diff --git a/rules/aip0123/resource_variables.go b/rules/aip0123/resource_variables.go
new file mode 100644
index 0000000..e40500b
--- /dev/null
+++ b/rules/aip0123/resource_variables.go
@@ -0,0 +1,58 @@
+// Copyright 2019 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 aip0123
+
+import (
+	"fmt"
+	"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 resourceVariables = &lint.MessageRule{
+	Name:   lint.NewRuleName(123, "resource-variables"),
+	OnlyIf: hasResourceAnnotation,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		resource := utils.GetResource(m)
+		for _, pattern := range resource.GetPattern() {
+			for _, variable := range getVariables(pattern) {
+				if strings.ToLower(variable) != variable {
+					return []lint.Problem{{
+						Message: fmt.Sprintf(
+							"Variable names in patterns should use snake case, such as %q.",
+							getDesiredPattern(pattern),
+						),
+						Descriptor: m,
+						Location:   locations.MessageResource(m),
+					}}
+				}
+				if strings.HasSuffix(variable, "_id") {
+					return []lint.Problem{{
+						Message: fmt.Sprintf(
+							"Variable names should omit the `_id` suffix, such as %q.",
+							getDesiredPattern(pattern),
+						),
+						Descriptor: m,
+						Location:   locations.MessageResource(m),
+					}}
+				}
+			}
+		}
+		return nil
+	},
+}


Test patch:
diff --git a/rules/aip0123/resource_pattern_test.go b/rules/aip0123/resource_pattern_test.go
new file mode 100644
index 0000000..6595655
--- /dev/null
+++ b/rules/aip0123/resource_pattern_test.go
@@ -0,0 +1,54 @@
+// Copyright 2019 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 aip0123
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestResourcePattern(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		Pattern  string
+		problems testutils.Problems
+	}{
+		{"Valid", `pattern: "publishers/{publisher}/books/{book}"`, testutils.Problems{}},
+		{"ValidCamel", `pattern: "publishers/{publisher}/electronicBooks/{electronic_book}"`, testutils.Problems{}},
+		{"Missing", "", testutils.Problems{{Message: "declare resource name pattern"}}},
+		{"SnakeCase", `pattern: "book_publishers/{book_publisher}/books/{book}"`, testutils.Problems{{
+			Message: "bookPublishers/{book_publisher}/books/{book}",
+		}}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/resource.proto";
+
+				message Book {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/Book"
+						{{.Pattern}}
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetMessageTypes()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(resourcePattern.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
diff --git a/rules/aip0123/resource_variables_test.go b/rules/aip0123/resource_variables_test.go
new file mode 100644
index 0000000..ef92dc7
--- /dev/null
+++ b/rules/aip0123/resource_variables_test.go
@@ -0,0 +1,54 @@
+// Copyright 2019 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 aip0123
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestResourceVariables(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		Pattern  string
+		problems testutils.Problems
+	}{
+		{"Valid", "publishers/{publisher}/electronicBooks/{electronic_book}", testutils.Problems{}},
+		{"CamelCase", "publishers/{publisher}/electronicBooks/{electronicBook}", testutils.Problems{{
+			Message: "publishers/{publisher}/electronicBooks/{electronic_book}",
+		}}},
+		{"ID", "publishers/{publisher}/electronicBooks/{electronic_book_id}", testutils.Problems{{
+			Message: "publishers/{publisher}/electronicBooks/{electronic_book}",
+		}}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/resource.proto";
+				message Book {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/Book"
+						pattern: "{{.Pattern}}"
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetMessageTypes()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(resourceVariables.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}