Skip to content

Commit

Permalink
feat(AIP-191): compare service casing to packaging (#984)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@noahdietz
noahdietz authored Jun 27, 2022
1 parent 5076b1a commit 2aa02dc
Show file tree
Hide file tree
Showing 7 changed files with 150 additions and 70 deletions.
15 changes: 15 additions & 0 deletions rules/aip0191/aip0191.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package aip0191

import (
"regexp"
"strings"

"github.com/googleapis/api-linter/lint"
"github.com/jhump/protoreflect/desc"
Expand Down Expand Up @@ -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\\_\\/]*$`)
74 changes: 46 additions & 28 deletions rules/aip0191/csharp_namespace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.].",

This comment has been minimized.

Sign in to view

Copy link
@jskeet

jskeet Jun 28, 2022

Well, other characters are valid in C# namespaces, but we don't want them there :) (In particular, we don't want underscores.)
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)

This comment has been minimized.

Sign in to view

Copy link
@jskeet

jskeet Jun 28, 2022

I don't know what this is testing for, but would this give us problems with (say) "google.firestore.v1" as the API proto package, but "Google.Cloud.Firestore.V1" as the C# namespace?
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
},
}
Expand Down
4 changes: 4 additions & 0 deletions rules/aip0191/csharp_namespace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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)
Expand Down
66 changes: 42 additions & 24 deletions rules/aip0191/php_namespace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
Expand Down
4 changes: 4 additions & 0 deletions rules/aip0191/php_namespace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`),
Expand All @@ -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)
Expand Down
52 changes: 35 additions & 17 deletions rules/aip0191/ruby_package.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
Expand Down
5 changes: 4 additions & 1 deletion rules/aip0191/ruby_package_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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)
Expand Down

0 comments on commit 2aa02dc

Please sign in to comment.