Repository: googleapis/api-linter
Pull request #375 (2019-11-21 19:54:55)

[AIP-191] Enforce php_namespace idiomaticity.


Patch:
diff --git a/docs/rules/0191/php-namespace.md b/docs/rules/0191/php-namespace.md
new file mode 100644
index 0000000..3f3925b
--- /dev/null
+++ b/docs/rules/0191/php-namespace.md
@@ -0,0 +1,107 @@
+---
+rule:
+  aip: 191
+  name: [core, '0191', php-namespace]
+  summary: The `option php_namespace` annotation should be idiomatic if set.
+permalink: /191/php-namespace
+redirect_from:
+  - /0191/php-namespace
+---
+
+# PHP namespace annotation
+
+This rule enforces that if a proto file for a public API surface sets
+`option php_namespace`, that it uses language idiomatic conventions, as
+mandated in [AIP-191][].
+
+## Details
+
+This rule looks at each proto file, and complains if the `php_namespace` file
+annotation uses anything other than upper camel case, or includes characters
+other than letters, numbers, and `.`.
+
+It also ensures that versions with stability (e.g. `V1beta1`) are capitalized
+appropriately.
+
+## Examples
+
+### Case
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1;
+
+option php_namespace = "google\\example\\v1";  // Should be UpperCamelCase.
+```
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1;
+
+option php_namespace = "Google::Example::V1";  // Separator should be backslash.
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+syntax = "proto3";
+
+package google.example.v1;
+
+option php_namespace = "Google\\Example\\V1";
+```
+
+### Versions with stability
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+syntax = "proto3";
+
+package google.example.v1beta1;
+
+option php_namespace = "Google\\Example\\V1Beta1"; // Should be V1beta1.
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+syntax = "proto3";
+
+package google.example.v1beta1;
+
+option php_namespace = "Google\\Example\\V1beta1";
+```
+
+## Known issues
+
+This rule will improperly complain if it encounters an acronym. For example, it
+will complain about `Google\Cloud\AutoML\V1`, preferring `AutoMl`. This lint
+rule **should** be disabled in this case.
+
+## Disabling
+
+If you need to violate this rule, use a comment at the top of the file.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0191::php-namespace=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+syntax = "proto3";
+
+package google.example.v1;
+
+option php_namespace = "google\\example\\v1";
+```
+
+[aip-191]: https://aip.dev/191
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/locations/file_locations.go b/locations/file_locations.go
index 664107e..f26a4a7 100644
--- a/locations/file_locations.go
+++ b/locations/file_locations.go
@@ -44,6 +44,15 @@ func FileCsharpNamespace(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
 	return pathLocation(f, 8, 37) // 8 == options, 37 == csharp_namespace
 }
 
+// FilePhpNamespace returns the location of the php_namespace file option
+// in a file descriptor.
+//
+// If the location can not be found (for example, because there is no
+// php_namespace option), it returns nil.
+func FilePhpNamespace(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
+	return pathLocation(f, 8, 41) // 8 == options, 41 == php_namespace
+}
+
 // FileRubyPackage returns the location of the ruby_package file option
 // in a file descriptor.
 //
diff --git a/rules/aip0191/aip0191.go b/rules/aip0191/aip0191.go
index 8b0ebb1..b2c9ff1 100644
--- a/rules/aip0191/aip0191.go
+++ b/rules/aip0191/aip0191.go
@@ -31,6 +31,7 @@ func AddRules(r lint.RuleRegistry) error {
 		javaMultipleFiles,
 		javaOuterClassname,
 		javaPackage,
+		phpNamespace,
 		rubyPackage,
 		syntax,
 	)
diff --git a/rules/aip0191/php_namespace.go b/rules/aip0191/php_namespace.go
new file mode 100644
index 0000000..a260ea0
--- /dev/null
+++ b/rules/aip0191/php_namespace.go
@@ -0,0 +1,65 @@
+// 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 aip0191
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/locations"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/stoewer/go-strcase"
+)
+
+var phpNamespace = &lint.FileRule{
+	Name: lint.NewRuleName(191, "php-namespace"),
+	LintFile: func(f *desc.FileDescriptor) []lint.Problem {
+		if ns := f.GetFileOptions().GetPhpNamespace(); ns != "" {
+			// Check for invalid characters.
+			if !phpValidChars.MatchString(ns) {
+				return []lint.Problem{{
+					Message:    `Invalid characters: PHP namespaces only allow [A-Za-z0-9\].`,
+					Descriptor: f,
+					Location:   locations.FilePhpNamespace(f),
+				}}
+			}
+
+			// Check that upper camel case is used.
+			upperCamel := []string{}
+			for _, segment := range strings.Split(ns, `\`) {
+				upperCamel = append(upperCamel, strcase.UpperCamelCase(segment))
+			}
+			if want := strings.Join(upperCamel, `\`); ns != want {
+				return []lint.Problem{{
+					Message: "PHP namespaces use UpperCamelCase.",
+					Suggestion: fmt.Sprintf(
+						"option php_namespace = %q;",
+						// Even though the string value is a single backslash, we want
+						// to suggest two backslashes, because that is what should be
+						// typed into the editor.
+						strings.ReplaceAll(want, `\`, `\\`),
+					),
+					Descriptor: f,
+					Location:   locations.FilePhpNamespace(f),
+				}}
+			}
+		}
+		return nil
+	},
+}
+
+var phpValidChars = regexp.MustCompile(`^[A-Za-z0-9\\]+$`)


Test patch:
diff --git a/locations/file_locations_test.go b/locations/file_locations_test.go
index f0e9beb..dc3a745 100644
--- a/locations/file_locations_test.go
+++ b/locations/file_locations_test.go
@@ -31,6 +31,7 @@ func TestLocations(t *testing.T) {
 		package google.api.linter;
 
 		option csharp_namespace = "Google.Api.Linter";
+		option php_namespace = "Google\\Api\\Linter";
 		option ruby_package = "Google::Api::Linter";
 
 		message Foo {
@@ -48,7 +49,8 @@ func TestLocations(t *testing.T) {
 			{"Syntax", FileSyntax, []int32{1, 0, int32(len("syntax = \"proto3\";"))}},
 			{"Package", FilePackage, []int32{3, 0, int32(len("package google.api.linter;"))}},
 			{"CsharpNamespace", FileCsharpNamespace, []int32{5, 0, int32(len(`option csharp_namespace = "Google.Api.Linter";`))}},
-			{"RubyPackage", FileRubyPackage, []int32{6, 0, int32(len(`option ruby_package = "Google::Api::Linter";`))}},
+			{"PhpNamespace", FilePhpNamespace, []int32{6, 0, int32(len(`option php_namespace = "Google\\Api\\Linter";`))}},
+			{"RubyPackage", FileRubyPackage, []int32{7, 0, int32(len(`option ruby_package = "Google::Api::Linter";`))}},
 		}
 		for _, test := range tests {
 			t.Run(test.testName, func(t *testing.T) {
diff --git a/rules/aip0191/php_namespace_test.go b/rules/aip0191/php_namespace_test.go
new file mode 100644
index 0000000..b3f71c6
--- /dev/null
+++ b/rules/aip0191/php_namespace_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 aip0191
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestPhpNamespace(t *testing.T) {
+	for _, test := range []struct {
+		name         string
+		PhpNamespace string
+		problems     testutils.Problems
+	}{
+		{"Valid", `Google\\Example\\V1`, testutils.Problems{}},
+		{"ValidBeta", `Google\\Example\\V1beta1`, testutils.Problems{}},
+		{"InvalidBadChars", "Google:Example:V1", testutils.Problems{{Message: "Invalid characters"}}},
+		{"Invalid", `google\\example\\v1`, testutils.Problems{{
+			Suggestion: fmt.Sprintf("option php_namespace = %q;", `Google\\Example\\V1`),
+		}}},
+		{"InvalidVersion", `Google\\Example\\v1`, testutils.Problems{{
+			Suggestion: fmt.Sprintf("option php_namespace = %q;", `Google\\Example\\V1`),
+		}}},
+		{"InvalidBeta", `Google\\Example\\V1Beta1`, testutils.Problems{{
+			Suggestion: fmt.Sprintf("option php_namespace = %q;", `Google\\Example\\V1beta1`),
+		}}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				package google.example.v1;
+
+				option php_namespace = "{{.PhpNamespace}}";
+			`, test)
+			if diff := test.problems.SetDescriptor(f).Diff(phpNamespace.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}