Repository: googleapis/api-linter
Pull request #379 (2019-11-21 23:43:29)

[AIP-191] Enforce that Java package annotations are correct.


Patch:
diff --git a/docs/rules/0191/java-package.md b/docs/rules/0191/java-package.md
index 9d00c33..2de9098 100644
--- a/docs/rules/0191/java-package.md
+++ b/docs/rules/0191/java-package.md
@@ -31,8 +31,17 @@ package google.example.v1;
 // Needs `option java_package = "com.google.example.v1";`.
 option java_multiple_files = true;
 option java_outer_classname = "LibraryProto";
+```
+
+```proto
+// Incorrect.
+syntax = "proto3";
 
-message Book {}
+package google.example.v1;
+
+option java_package = "google.example";  // Should end with "google.example.v1".
+option java_multiple_files = true;
+option java_outer_classname = "LibraryProto";
 ```
 
 **Correct** code for this rule:
@@ -46,8 +55,6 @@ package google.example.v1;
 option java_package = "com.google.example.v1";
 option java_multiple_files = true;
 option java_outer_classname = "LibraryProto";
-
-message Book {}
 ```
 
 ## Disabling
@@ -64,8 +71,6 @@ package google.example.v1;
 
 option java_multiple_files = true;
 option java_outer_classname = "LibraryProto";
-
-message Book {}
 ```
 
 [aip-191]: https://aip.dev/191
diff --git a/locations/file_locations.go b/locations/file_locations.go
index f26a4a7..c7aeb14 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
 }
 
+// FileJavaPackage returns the location of the java_package file option
+// in a file descriptor.
+//
+// If the location can not be found (for example, because there is no
+// java_package option), it returns nil.
+func FileJavaPackage(f *desc.FileDescriptor) *dpb.SourceCodeInfo_Location {
+	return pathLocation(f, 8, 1) // 8 == options, 1 == java_package
+}
+
 // FilePhpNamespace returns the location of the php_namespace file option
 // in a file descriptor.
 //
diff --git a/rules/aip0191/java_package.go b/rules/aip0191/java_package.go
index 9f7ac19..db091d7 100644
--- a/rules/aip0191/java_package.go
+++ b/rules/aip0191/java_package.go
@@ -15,6 +15,9 @@
 package aip0191
 
 import (
+	"fmt"
+	"strings"
+
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
 	"github.com/jhump/protoreflect/desc"
@@ -24,11 +27,20 @@ var javaPackage = &lint.FileRule{
 	Name:   lint.NewRuleName(191, "java-package"),
 	OnlyIf: hasPackage,
 	LintFile: func(f *desc.FileDescriptor) []lint.Problem {
-		if f.GetFileOptions().GetJavaPackage() == "" {
+		javaPkg := f.GetFileOptions().GetJavaPackage()
+		if javaPkg == "" {
 			return []lint.Problem{{
+				Message:    "Proto files must set `option java_package`.",
 				Descriptor: f,
 				Location:   locations.FilePackage(f),
-				Message:    "Proto files must set `option java_package`.",
+			}}
+		}
+		if !strings.HasSuffix(javaPkg, f.GetPackage()) {
+			return []lint.Problem{{
+				Message:    "The Java Package should mirror the proto package.",
+				Suggestion: fmt.Sprintf(`option java_package = "com.%s";`, f.GetPackage()),
+				Descriptor: f,
+				Location:   locations.FileJavaPackage(f),
 			}}
 		}
 		return nil


Test patch:
diff --git a/locations/file_locations_test.go b/locations/file_locations_test.go
index dc3a745..97b4907 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 java_package = "com.google.api.linter";
 		option php_namespace = "Google\\Api\\Linter";
 		option ruby_package = "Google::Api::Linter";
 
@@ -49,8 +50,9 @@ 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";`))}},
-			{"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";`))}},
+			{"JavaPackage", FileJavaPackage, []int32{6, 0, int32(len(`option java_package = "com.google.api.linter";`))}},
+			{"PhpNamespace", FilePhpNamespace, []int32{7, 0, int32(len(`option php_namespace = "Google\\Api\\Linter";`))}},
+			{"RubyPackage", FileRubyPackage, []int32{8, 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/java_package_test.go b/rules/aip0191/java_package_test.go
index 871eb26..1079c1b 100644
--- a/rules/aip0191/java_package_test.go
+++ b/rules/aip0191/java_package_test.go
@@ -28,7 +28,10 @@ func TestJavaPackage(t *testing.T) {
 		problems   testutils.Problems
 	}{
 		{"Valid", []string{"package foo.v1;", `option java_package = "com.foo.v1";`}, testutils.Problems{}},
-		{"Invalid", []string{"package foo.v1;", ""}, testutils.Problems{{Message: "java_package"}}},
+		{"InvalidEmpty", []string{"package foo.v1;", ""}, testutils.Problems{{Message: "java_package"}}},
+		{"InvalidWrong", []string{"package foo.v1;", `option java_package = "something.else";`}, testutils.Problems{{
+			Suggestion: `option java_package = "com.foo.v1";`,
+		}}},
 		{"Ignored", []string{"", ""}, testutils.Problems{}},
 	} {
 		t.Run(test.name, func(t *testing.T) {