fix: Respect google.api.resource singular annotation in batch method rules#1629
fix: Respect google.api.resource singular annotation in batch method rules#1629stevenwarejones wants to merge 4 commits into
Conversation
Batch method rules (AIP-0231, 0233, 0234, 0235) for request-requests-field, request-names-field, and response-resource-field used go-pluralize to singularize the resource name extracted from batch message names. This produced incorrect results for words like "Metadata", which go-pluralize singularizes to "Metadatum" using Latin grammar rules. Add ResourceSingular() helper that first checks for a google.api.resource annotation with a singular field on sibling or imported resource messages before falling back to go-pluralize. This is analogous to the fix in googleapis#1573 for the plural-method-name rule. Issue: googleapis#1566
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request replaces the go-pluralize library with a custom ResourceSingular utility across AIP rules 0231, 0233, 0234, and 0235 to correctly handle resource singularization by checking google.api.resource annotations. This change prevents incorrect singularization of words like "Metadata" and includes new test cases for verification. Feedback includes a suggestion to use consistent string trimming methods across the packages and a recommendation to implement recursive import searching to ensure resource annotations are found in complex proto hierarchies.
| // The singular form the resource message name; the first letter capitalized. | ||
| plural := strings.TrimSuffix(strings.TrimPrefix(string(m.Name()), "BatchGet"), "Response") | ||
| resourceMsgName := pluralize.NewClient().Singular(plural) | ||
| pluralName := strings.TrimSuffix(strings.TrimPrefix(string(m.Name()), "BatchGet"), "Response") |
There was a problem hiding this comment.
For consistency with other rules in this PR (including the request rule in this package), consider using TrimPrefix on the result of TrimSuffix.
| pluralName := strings.TrimSuffix(strings.TrimPrefix(string(m.Name()), "BatchGet"), "Response") | |
| pluralName := strings.TrimPrefix(strings.TrimSuffix(string(m.Name()), "Response"), "BatchGet") |
There was a problem hiding this comment.
Fixed in 9c9e3a5 — reordered to TrimPrefix(TrimSuffix(...)) to match all other rules.
| imports := f.Imports() | ||
| for i := 0; i < imports.Len(); i++ { | ||
| if s := findResourceSingularInFile(pluralName, imports.Get(i).FileDescriptor); s != "" { | ||
| return s | ||
| } | ||
| } |
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.
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.
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.
… lookup Add test cases across all 8 modified batch rules (AIP-0231, 0233, 0234, 0235) and the ResourceSingular utility to verify that google.api.resource singular annotations are respected. Uses ImpressionMetadata as the test resource since go-pluralize incorrectly singularizes it to Metadatum. Also fix copyright year in string_pluralize.go to 2026 for new code. Issue: googleapis#1628
… docs - Make TrimPrefix/TrimSuffix ordering consistent in aip0231 response_resource_field.go (was TrimSuffix(TrimPrefix(...)), now TrimPrefix(TrimSuffix(...)) like all other rules) - Clarify that single-depth import search is intentional and sufficient for standard proto layouts - Add TestResourceSingularImportedFile exercising the cross-file import search path using ParseProtoStrings with separate resource.proto and service.proto files Issue: googleapis#1628
noahdietz
left a comment
There was a problem hiding this comment.
Hi there, thanks for the extensive improvement. Couple of comments.
| // | ||
| // 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 { |
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 GetResourceSingular (which arguably should've been named ResourceSingular).
This might be something like DeriveResourceSingular as it tries to find a resource from a derived name and then get the singular.
| m := serviceFile.Messages().Get(0) | ||
| got := ResourceSingular("ImpressionMetadata", m) | ||
| if got != "ImpressionMetadata" { | ||
| t.Errorf("ResourceSingular(\"ImpressionMetadata\") = %q, want \"ImpressionMetadata\"", got) | ||
| } |
There was a problem hiding this comment.
Please make "ImpressionMetadata" a variable want, then use that throughout and change the failure message from using \"ImpressionMetadata\" to %q
Here and everywhere similar
Summary
Batch method rules (AIP-0231, 0233, 0234, 0235) for
request-requests-field,request-names-field, andresponse-resource-fieldusedgo-pluralizeto singularize the resource name extracted from batch message names. This produced incorrect results for words like "Metadata", whichgo-pluralizesingularizes to "Metadatum" using Latin grammar rules.This PR adds a
ResourceSingular()helper that first checks for agoogle.api.resourceannotation with asingularfield on sibling or imported resource messages before falling back togo-pluralize. This is analogous to the fix in #1573 for theplural-method-namerule.When no
google.api.resourceannotation is found, the function falls back togo-pluralize, preserving existing behavior for all other resources.Affected Rules
core::0231::request-names-fieldcore::0231::response-resource-fieldcore::0233::request-requests-fieldcore::0233::response-resource-fieldcore::0234::request-requests-fieldcore::0234::response-resource-fieldcore::0235::request-names-fieldcore::0235::response-resource-fieldChanges
rules/internal/utils/string_pluralize.go: AddResourceSingular()function that searches messages in the current file and directly imported files for agoogle.api.resourceannotation with a matchingsingularvalue, converting it to UpperCamelCase. Falls back togo-pluralizeif no annotation is found.pluralize.NewClient().Singular()calls withutils.ResourceSingular().ImpressionMetadatawithgoogle.api.resourceannotation verifying bothrequest-requests-fieldandresponse-resource-field.Test plan
ResourceSingular()covering: annotation in same file, annotation in imported file, fallback to go-pluralize, uncountable nouns, message name matching, and no-annotation Latin wordsTestResourceSingularImportedFile) exercising the cross-file import search pathgo test ./...passes (56 packages, 0 failures)golangci-lint run ./...reports 0 issuesMetadatumdisable comments produces zero false positives with the fixed linterIssue: #1628