Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3917: Add package openapi naming strategy. #4372

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions protoc-gen-openapiv2/internal/genopenapi/naming.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package genopenapi

import (
"reflect"
"regexp"
"strings"
)

Expand All @@ -17,6 +18,8 @@ func LookupNamingStrategy(strategyName string) func([]string) map[string]string
return resolveNamesLegacy
case "simple":
return resolveNamesSimple
case "package":
return resolveNamesPackage
}
return nil
}
Expand All @@ -41,7 +44,7 @@ func resolveNamesFQN(messages []string) map[string]string {
// E.g., if the fully qualified name is `.a.b.C.D`, and there are other messages with fully
// qualified names ending in `.D` but not in `.C.D`, it assigns the unique name `bCD`.
func resolveNamesLegacy(messages []string) map[string]string {
return resolveNamesUniqueWithContext(messages, 1, "")
return resolveNamesUniqueWithContext(messages, 1, "", false)
}

// resolveNamesSimple takes the names of all proto messages and generates unique references by using a simple
Expand All @@ -52,20 +55,46 @@ func resolveNamesLegacy(messages []string) map[string]string {
// E.g., if the fully qualified name is `.a.b.C.D`, and there are other messages with
// fully qualified names ending in `.D` but not in `.C.D`, it assigns the unique name `C.D`.
func resolveNamesSimple(messages []string) map[string]string {
return resolveNamesUniqueWithContext(messages, 0, ".")
return resolveNamesUniqueWithContext(messages, 0, ".", false)
}

// resolveNamesPackage takes the names of all proto messages and generates unique references by
// starting with the package-scoped name (with nested message types qualified by their containing
// "parent" types), and then following the "simple" heuristic above to add package name components
// until each message has a unique name with a "." between each component.
//
// E.g., if the fully qualified name is `.a.b.C.D`, the name is `C.D` unless there is another
// package-scoped name ending in "C.D", in which case it would be `b.C.D` (unless that also
// conflicted, in which case the name would be the fully-qualified `a.b.C`).
func resolveNamesPackage(messages []string) map[string]string {
return resolveNamesUniqueWithContext(messages, 0, ".", true)
}

// Take the names of every proto message and generates a unique reference by:
// first, separating each message name into its components by splitting at dots. Then,
// take the shortest suffix slice from each components slice that is unique among all
// messages, and convert it into a component name by taking extraContext additional
// components into consideration and joining all components with componentSeparator.
func resolveNamesUniqueWithContext(messages []string, extraContext int, componentSeparator string) map[string]string {
func resolveNamesUniqueWithContext(messages []string, extraContext int, componentSeparator string, qualifyNestedMessages bool) map[string]string {
packagesByDepth := make(map[int][][]string)
uniqueNames := make(map[string]string)

hierarchy := func(pkg string) []string {
return strings.Split(pkg, ".")
if !qualifyNestedMessages {
return strings.Split(pkg, ".")
}
// We rely on the convention that package names are lowercase but message names are
// capitalized.
pkgEnd := regexp.MustCompile(`\.[A-Z]`).FindStringIndex(pkg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this regexp compilation into a global variable?

Copy link
Contributor Author

@jgiles jgiles May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

I actually had it as a global var initially, as is often done for regexp compilation... but I moved it into the function to have the pattern defined in the same context as it's used for readability. Generally name resolution only happens once per process anyways, so I don't think the performance optimization of doing the regexp compilation one time up-front really gains us anything here.

I've moved it out though into a global var though, let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I appreciate the explanation. All the same, I think anyone reading an inline regexp.MustCompile will be surprised, so lets leave it as a package level variable.

if pkgEnd == nil {
// Fall back to non-qualified behavior if search based on convention fails.
return strings.Split(pkg, ".")
}
// Return each package component as an element, followed by the full message name
// (potentially qualified, if nested) as a single component.
qualifiedPkgName := pkg[:pkgEnd[0]]
nestedTypeName := pkg[pkgEnd[0]+1:]
return append(strings.Split(qualifiedPkgName, "."), nestedTypeName)
}

for _, p := range messages {
Expand Down
24 changes: 17 additions & 7 deletions protoc-gen-openapiv2/internal/genopenapi/naming_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import "testing"

func TestNaming(t *testing.T) {
type expectedNames struct {
fqn, legacy, simple string
fqn, legacy, simple, pkg string
}
messageNameToExpected := map[string]expectedNames{
".A": {"A", "A", "A"},
".a.B.C": {"a.B.C", "aBC", "B.C"},
".a.D.C": {"a.D.C", "aDC", "D.C"},
".a.E.F": {"a.E.F", "aEF", "a.E.F"},
".b.E.F": {"b.E.F", "bEF", "b.E.F"},
".c.G.H": {"c.G.H", "GH", "H"},
".A": {"A", "A", "A", "A"},
".a.B.C": {"a.B.C", "aBC", "B.C", "B.C"},
".a.D.C": {"a.D.C", "aDC", "D.C", "D.C"},
".a.E.F": {"a.E.F", "aEF", "a.E.F", "a.E.F"},
".b.E.F": {"b.E.F", "bEF", "b.E.F", "b.E.F"},
".c.G.H": {"c.G.H", "GH", "H", "G.H"},
}

allMessageNames := make([]string, 0, len(messageNameToExpected))
Expand Down Expand Up @@ -50,4 +50,14 @@ func TestNaming(t *testing.T) {
}
}
})
t.Run("package", func(t *testing.T) {
uniqueNames := resolveNamesPackage(allMessageNames)
for _, msgName := range allMessageNames {
expected := messageNameToExpected[msgName].pkg
actual := uniqueNames[msgName]
if expected != actual {
t.Errorf("package unique name %q does not match expected name %q", actual, expected)
}
}
})
}
2 changes: 1 addition & 1 deletion protoc-gen-openapiv2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ var (
_ = flag.Bool("allow_repeated_fields_in_body", true, "allows to use repeated field in `body` and `response_body` field of `google.api.http` annotation option. DEPRECATED: the value is ignored and always behaves as `true`.")
includePackageInTags = flag.Bool("include_package_in_tags", false, "if unset, the gRPC service name is added to the `Tags` field of each operation. If set and the `package` directive is shown in the proto file, the package name will be prepended to the service name")
useFQNForOpenAPIName = flag.Bool("fqn_for_openapi_name", false, "if set, the object's OpenAPI names will use the fully qualified names from the proto definition (ie my.package.MyMessage.MyInnerMessage). DEPRECATED: prefer `openapi_naming_strategy=fqn`")
openAPINamingStrategy = flag.String("openapi_naming_strategy", "", "use the given OpenAPI naming strategy. Allowed values are `legacy`, `fqn`, `simple`. If unset, either `legacy` or `fqn` are selected, depending on the value of the `fqn_for_openapi_name` flag")
openAPINamingStrategy = flag.String("openapi_naming_strategy", "", "use the given OpenAPI naming strategy. Allowed values are `legacy`, `fqn`, `simple`, `package`. If unset, either `legacy` or `fqn` are selected, depending on the value of the `fqn_for_openapi_name` flag")
useGoTemplate = flag.Bool("use_go_templates", false, "if set, you can use Go templates in protofile comments")
goTemplateArgs = utilities.StringArrayFlag(flag.CommandLine, "go_template_args", "provide a custom value that can override a key in the Go template. Requires the `use_go_templates` option to be set")
ignoreComments = flag.Bool("ignore_comments", false, "if set, all protofile comments are excluded from output")
Expand Down
Loading