From 2aa02dc7978a18afd0eb72aa640c3cd90c932c61 Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Mon, 27 Jun 2022 11:08:18 -0700 Subject: [PATCH] feat(AIP-191): compare service casing to packaging (#984) A first step towards protecting services and packing annotations from having mismatched names e.g. `service Foobar` and `option c_sharp_namespace = "Google.FooBar.V1"` where `Foobar` and `FooBar` do not match in casing. @jskeet --- rules/aip0191/aip0191.go | 15 ++++++ rules/aip0191/csharp_namespace.go | 74 ++++++++++++++++---------- rules/aip0191/csharp_namespace_test.go | 4 ++ rules/aip0191/php_namespace.go | 66 ++++++++++++++--------- rules/aip0191/php_namespace_test.go | 4 ++ rules/aip0191/ruby_package.go | 52 ++++++++++++------ rules/aip0191/ruby_package_test.go | 5 +- 7 files changed, 150 insertions(+), 70 deletions(-) diff --git a/rules/aip0191/aip0191.go b/rules/aip0191/aip0191.go index f89f7d099..515f3cd22 100644 --- a/rules/aip0191/aip0191.go +++ b/rules/aip0191/aip0191.go @@ -17,6 +17,7 @@ package aip0191 import ( "regexp" + "strings" "github.com/googleapis/api-linter/lint" "github.com/jhump/protoreflect/desc" @@ -44,5 +45,19 @@ func hasPackage(f *desc.FileDescriptor) bool { return f.GetPackage() != "" } +func packagingServiceNameEquals(serv, pkg, sep string) bool { + segments := strings.Split(pkg, sep) + for _, segment := range segments { + // If a packaging annotation segment and a service name are equal in a + // case-insensitive comparison, they must also be equal using a + // case-sensitive comparison. + if strings.EqualFold(segment, serv) && segment != serv { + return false + } + } + + return true +} + var versionRegexp = regexp.MustCompile(`^v[0-9]+(p[0-9]+)?((alpha|beta)[0-9]*)?$`) var validCharacterRegexp = regexp.MustCompile(`^[a-z0-9\\_\\/]*$`) diff --git a/rules/aip0191/csharp_namespace.go b/rules/aip0191/csharp_namespace.go index 049abf650..695f66923 100644 --- a/rules/aip0191/csharp_namespace.go +++ b/rules/aip0191/csharp_namespace.go @@ -27,42 +27,60 @@ import ( var csharpNamespace = &lint.FileRule{ Name: lint.NewRuleName(191, "csharp-namespace"), + OnlyIf: func(f *desc.FileDescriptor) bool { + fops := f.GetFileOptions() + return fops != nil && fops.GetCsharpNamespace() != "" + }, LintFile: func(f *desc.FileDescriptor) []lint.Problem { - if ns := f.GetFileOptions().GetCsharpNamespace(); ns != "" { - // Check for invalid characters. - if !csharpValidChars.MatchString(ns) { - return []lint.Problem{{ - Message: "Invalid characters: C# namespaces only allow [A-Za-z0-9.].", - Descriptor: f, - Location: locations.FileCsharpNamespace(f), - }} - } + ns := f.GetFileOptions().GetCsharpNamespace() + delim := "." - // Check that upper camel case is used. - upperCamel := []string{} - for _, segment := range strings.Split(ns, ".") { - wantSegment := csharpVersionRegexp.ReplaceAllStringFunc( - strcase.UpperCamelCase(segment), - func(s string) string { - point := csharpVersionRegexp.FindStringSubmatch(s)[1] - if point != "" { - s = strings.ReplaceAll(s, point, strings.ToUpper(point)) - } - stability := csharpVersionRegexp.FindStringSubmatch(s)[2] - return strings.ReplaceAll(s, stability, strings.Title(stability)) - }, - ) - upperCamel = append(upperCamel, wantSegment) - } - if want := strings.Join(upperCamel, "."); ns != want { + // Check for invalid characters. + if !csharpValidChars.MatchString(ns) { + return []lint.Problem{{ + Message: "Invalid characters: C# namespaces only allow [A-Za-z0-9.].", + Descriptor: f, + Location: locations.FileCsharpNamespace(f), + }} + } + + // Check that upper camel case is used. + upperCamel := []string{} + for _, segment := range strings.Split(ns, delim) { + wantSegment := csharpVersionRegexp.ReplaceAllStringFunc( + strcase.UpperCamelCase(segment), + func(s string) string { + point := csharpVersionRegexp.FindStringSubmatch(s)[1] + if point != "" { + s = strings.ReplaceAll(s, point, strings.ToUpper(point)) + } + stability := csharpVersionRegexp.FindStringSubmatch(s)[2] + return strings.ReplaceAll(s, stability, strings.Title(stability)) + }, + ) + upperCamel = append(upperCamel, wantSegment) + } + if want := strings.Join(upperCamel, delim); ns != want { + return []lint.Problem{{ + Message: "C# namespaces use UpperCamelCase.", + Suggestion: fmt.Sprintf("option csharp_namespace = %q;", want), + Descriptor: f, + Location: locations.FileCsharpNamespace(f), + }} + } + + for _, s := range f.GetServices() { + n := s.GetName() + if !packagingServiceNameEquals(n, ns, delim) { + msg := fmt.Sprintf("Case of C# namespace and service name %q must match.", n) return []lint.Problem{{ - Message: "C# namespaces use UpperCamelCase.", - Suggestion: fmt.Sprintf("option csharp_namespace = %q;", want), + Message: msg, Descriptor: f, Location: locations.FileCsharpNamespace(f), }} } } + return nil }, } diff --git a/rules/aip0191/csharp_namespace_test.go b/rules/aip0191/csharp_namespace_test.go index cfd48393a..b9d224abb 100644 --- a/rules/aip0191/csharp_namespace_test.go +++ b/rules/aip0191/csharp_namespace_test.go @@ -31,6 +31,7 @@ func TestCsharpNamespace(t *testing.T) { {"ValidBeta", "Google.Example.V1Beta1", nil}, {"ValidMain", "Google.Example.V1Main", nil}, {"ValidPointBeta", "Google.Example.V1P1Beta1", nil}, + {"ValidServiceNamespaceCase", "Google.FooBar.V1", testutils.Problems{}}, {"InvalidBadChars", "Google:Example:V1", testutils.Problems{{Message: "Invalid characters"}}}, {"Invalid", "google.example.v1", testutils.Problems{{ Suggestion: fmt.Sprintf("option csharp_namespace = %q;", "Google.Example.V1"), @@ -44,12 +45,15 @@ func TestCsharpNamespace(t *testing.T) { {"InvalidPointBeta", "Google.Example.V1p1beta1", testutils.Problems{{ Suggestion: fmt.Sprintf("option csharp_namespace = %q;", "Google.Example.V1P1Beta1"), }}}, + {"ValidServiceNamespaceCase", "Google.Foobar.V1", testutils.Problems{{Message: "Case"}}}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` package google.example.v1; option csharp_namespace = "{{.CsharpNamespace}}"; + + service FooBar {} `, test) if diff := test.problems.SetDescriptor(f).Diff(csharpNamespace.Lint(f)); diff != "" { t.Errorf(diff) diff --git a/rules/aip0191/php_namespace.go b/rules/aip0191/php_namespace.go index b3f6cbb84..935b70c90 100644 --- a/rules/aip0191/php_namespace.go +++ b/rules/aip0191/php_namespace.go @@ -27,38 +27,56 @@ import ( var phpNamespace = &lint.FileRule{ Name: lint.NewRuleName(191, "php-namespace"), + OnlyIf: func(f *desc.FileDescriptor) bool { + fops := f.GetFileOptions() + return fops != nil && fops.GetPhpNamespace() != "" + }, 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), - }} - } + ns := f.GetFileOptions().GetPhpNamespace() + delim := `\` - // 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 { + // 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, delim) { + upperCamel = append(upperCamel, strcase.UpperCamelCase(segment)) + } + if want := strings.Join(upperCamel, delim); ns != want { + return []lint.Problem{{ + Message: "PHP namespaces use UpperCamelCase.", + Suggestion: fmt.Sprintf( + "option php_namespace = %s;", + // 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. We use %s to avoid additional escaping + // of backslashes by Sprintf. + strings.ReplaceAll(want, delim, `\\`), + ), + Descriptor: f, + Location: locations.FilePhpNamespace(f), + }} + } + + for _, s := range f.GetServices() { + n := s.GetName() + if !packagingServiceNameEquals(n, ns, delim) { + msg := fmt.Sprintf("Case of PHP namespace and service name %q must match.", n) return []lint.Problem{{ - Message: "PHP namespaces use UpperCamelCase.", - Suggestion: fmt.Sprintf( - "option php_namespace = %s;", - // 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. We use %s to avoid additional escaping - // of backslashes by Sprintf. - strings.ReplaceAll(want, `\`, `\\`), - ), + Message: msg, Descriptor: f, Location: locations.FilePhpNamespace(f), }} } } + return nil }, } diff --git a/rules/aip0191/php_namespace_test.go b/rules/aip0191/php_namespace_test.go index 7a224b079..aa1a8cac7 100644 --- a/rules/aip0191/php_namespace_test.go +++ b/rules/aip0191/php_namespace_test.go @@ -29,6 +29,7 @@ func TestPhpNamespace(t *testing.T) { }{ {"Valid", `Google\\Example\\V1`, testutils.Problems{}}, {"ValidBeta", `Google\\Example\\V1beta1`, testutils.Problems{}}, + {"ValidServiceNamespaceCase", `Google\\FooBar\\V1`, testutils.Problems{}}, {"InvalidBadChars", "Google:Example:V1", testutils.Problems{{Message: "Invalid characters"}}}, {"Invalid", `google\\example\\v1`, testutils.Problems{{ Suggestion: fmt.Sprintf("option php_namespace = %s;", `Google\\Example\\V1`), @@ -39,12 +40,15 @@ func TestPhpNamespace(t *testing.T) { {"InvalidBeta", `Google\\Example\\V1Beta1`, testutils.Problems{{ Suggestion: fmt.Sprintf("option php_namespace = %s;", `Google\\Example\\V1beta1`), }}}, + {"ValidServiceNamespaceCase", `Google\\Foobar\\V1`, testutils.Problems{{Message: "Case"}}}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` package google.example.v1; option php_namespace = "{{.PhpNamespace}}"; + + service FooBar {} `, test) if diff := test.problems.SetDescriptor(f).Diff(phpNamespace.Lint(f)); diff != "" { t.Errorf(diff) diff --git a/rules/aip0191/ruby_package.go b/rules/aip0191/ruby_package.go index c97804545..51d1d1ba3 100644 --- a/rules/aip0191/ruby_package.go +++ b/rules/aip0191/ruby_package.go @@ -27,31 +27,49 @@ import ( var rubyPackage = &lint.FileRule{ Name: lint.NewRuleName(191, "ruby-package"), + OnlyIf: func(f *desc.FileDescriptor) bool { + fops := f.GetFileOptions() + return fops != nil && fops.GetRubyPackage() != "" + }, LintFile: func(f *desc.FileDescriptor) []lint.Problem { - if ns := f.GetFileOptions().GetRubyPackage(); ns != "" { - // Check for invalid characters. - if !rubyValidChars.MatchString(ns) { - return []lint.Problem{{ - Message: "Invalid characters: Ruby packages only allow [A-Za-z0-9:].", - Descriptor: f, - Location: locations.FileRubyPackage(f), - }} - } + ns := f.GetFileOptions().GetRubyPackage() + delim := "::" - // 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 { + // Check for invalid characters. + if !rubyValidChars.MatchString(ns) { + return []lint.Problem{{ + Message: "Invalid characters: Ruby packages only allow [A-Za-z0-9:].", + Descriptor: f, + Location: locations.FileRubyPackage(f), + }} + } + + // Check that upper camel case is used. + upperCamel := []string{} + for _, segment := range strings.Split(ns, delim) { + upperCamel = append(upperCamel, strcase.UpperCamelCase(segment)) + } + if want := strings.Join(upperCamel, delim); ns != want { + return []lint.Problem{{ + Message: "Ruby packages use UpperCamelCase.", + Suggestion: fmt.Sprintf("option ruby_package = %q;", want), + Descriptor: f, + Location: locations.FileRubyPackage(f), + }} + } + + for _, s := range f.GetServices() { + n := s.GetName() + if !packagingServiceNameEquals(n, ns, delim) { + msg := fmt.Sprintf("Case of Ruby package and service name %q must match.", n) return []lint.Problem{{ - Message: "Ruby packages use UpperCamelCase.", - Suggestion: fmt.Sprintf("option ruby_package = %q;", want), + Message: msg, Descriptor: f, Location: locations.FileRubyPackage(f), }} } } + return nil }, } diff --git a/rules/aip0191/ruby_package_test.go b/rules/aip0191/ruby_package_test.go index e487c2d5d..59c7cb988 100644 --- a/rules/aip0191/ruby_package_test.go +++ b/rules/aip0191/ruby_package_test.go @@ -29,7 +29,7 @@ func TestRubyPackage(t *testing.T) { }{ {"Valid", "Google::Example::V1", testutils.Problems{}}, {"ValidBeta", "Google::Example::V1beta1", testutils.Problems{}}, - {"ValidTwoWords", "Google::LibraryExample::V1", testutils.Problems{}}, + {"ValidServiceNamespaceCase", "Google::FooBar::V1", testutils.Problems{}}, {"InvalidBadChars", "Google.Example.V1", testutils.Problems{{Message: "Invalid characters"}}}, {"Invalid", "google::example::v1", testutils.Problems{{ Suggestion: fmt.Sprintf("option ruby_package = %q;", "Google::Example::V1"), @@ -40,12 +40,15 @@ func TestRubyPackage(t *testing.T) { {"InvalidBeta", "Google::Example::V1Beta1", testutils.Problems{{ Suggestion: fmt.Sprintf("option ruby_package = %q;", "Google::Example::V1beta1"), }}}, + {"ValidServiceNamespaceCase", "Google::Foobar::V1", testutils.Problems{{Message: "Case"}}}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` package google.example.v1; option ruby_package = "{{.RubyPackage}}"; + + service FooBar {} `, test) if diff := test.problems.SetDescriptor(f).Diff(rubyPackage.Lint(f)); diff != "" { t.Errorf(diff)