fix: Respect google.api.resource singular annotation in batch method rules #1629
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ package utils | |
|
|
||
| import ( | ||
| "github.com/gertd/go-pluralize" | ||
| "github.com/stoewer/go-strcase" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
| ) | ||
|
|
||
| var pluralizeClient = pluralize.NewClient() | ||
|
|
@@ -32,3 +34,70 @@ func ToPlural(s string) string { | |
| func ToSingular(s string) string { | ||
| return pluralizeClient.Singular(s) | ||
| } | ||
|
|
||
| // ResourceSingular returns the singular form of a resource name extracted from | ||
| // a batch method message name (e.g. "Books" from "BatchUpdateBooksRequest"). | ||
| // | ||
| // It searches for a resource message with a google.api.resource annotation | ||
| // whose singular matches the expected singular of pluralName. This search | ||
| // covers messages in the same file as well as directly imported files, since | ||
| // the resource message is commonly defined in a separate file from the service | ||
| // and request/response messages. | ||
| // | ||
| // If a matching resource annotation is found, its singular is returned (in | ||
| // UpperCamelCase). Otherwise, it falls back to the go-pluralize library. | ||
| // | ||
| // This avoids incorrect singularization of words like "Metadata" (which | ||
| // go-pluralize converts to "Metadatum" using Latin grammar rules). | ||
| func ResourceSingular(pluralName string, m protoreflect.MessageDescriptor) string { | ||
|
Contributor
There was a problem hiding this comment. Let's use a different function name as I think this is a bit misleading alongside other utilities like This might be something like |
||
| if f := m.ParentFile(); f != nil { | ||
| if s := findResourceSingularInFile(pluralName, f); s != "" { | ||
| return s | ||
| } | ||
| // Also search directly imported files, since the resource message | ||
| // is often defined in a separate file (e.g. impression_metadata.proto) | ||
| // from the service file (impression_metadata_service.proto). | ||
| imports := f.Imports() | ||
| for i := 0; i < imports.Len(); i++ { | ||
| if s := findResourceSingularInFile(pluralName, imports.Get(i).FileDescriptor); s != "" { | ||
| return s | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. The current implementation only searches direct imports for the resource annotation. In complex proto structures, the resource might be defined in a file that is imported transitively. Consider implementing a recursive search to ensure the resource annotation is found even if it's not in a directly imported file.
Author
There was a problem hiding this comment. Good callout. Intentionally keeping this as direct imports only — in standard proto layouts the resource message is in a sibling file directly imported by the service file, so single-depth is sufficient. Recursive search would add complexity for a case we haven't seen in practice. Updated the comment to make this intentional, and added a multi-file test (TestResourceSingularImportedFile) that exercises the import search path. See 9c9e3a5.
Author
There was a problem hiding this comment. On reflection, agreed — implemented recursive search with cycle detection via a visited set in 027891b. Also added that exercises a 3-file chain (service.proto → common.proto → resource.proto) to verify the annotation is found through transitive imports. |
||
| } | ||
|
|
||
| return pluralizeClient.Singular(pluralName) | ||
| } | ||
|
|
||
| // findResourceSingularInFile searches all messages in a file for a resource | ||
| // whose singular annotation matches the given pluralName. | ||
| func findResourceSingularInFile(pluralName string, f protoreflect.FileDescriptor) string { | ||
| if f == nil { | ||
| return "" | ||
| } | ||
| for i := 0; i < f.Messages().Len(); i++ { | ||
| msg := f.Messages().Get(i) | ||
| res := GetResource(msg) | ||
| if res == nil { | ||
| continue | ||
| } | ||
| s := GetResourceSingular(res) | ||
| if s == "" { | ||
| continue | ||
| } | ||
| // The singular from the annotation is typically lowerCamelCase | ||
| // (e.g. "impressionMetadata"). Convert to UpperCamelCase for | ||
| // comparison with the message name. | ||
| upperSingular := strcase.UpperCamelCase(s) | ||
| // Check if the pluralName matches: either the singular itself | ||
| // (for uncountable nouns like "Metadata" where plural == singular) | ||
| // or the go-pluralize plural of the singular. | ||
| if pluralName == upperSingular || pluralName == pluralizeClient.Plural(upperSingular) { | ||
| return upperSingular | ||
| } | ||
| // Also check if the message name matches the plural portion. | ||
| if string(msg.Name()) == pluralName { | ||
| return upperSingular | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other rules in this PR (including the request rule in this package), consider using
TrimPrefixon the result ofTrimSuffix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9c9e3a5 — reordered to
TrimPrefix(TrimSuffix(...))to match all other rules.