fix: Permit top-level resources to have batch methods. #619
Conversation
Also, I rewrote the tests for these rules from scratch, as they were in a weird state. Fixes #599.
google-cla
bot
added
the
cla: yes
| plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchGet") | ||
| if resp := m.GetFile().FindMessage(fmt.Sprintf("BatchGet%sResponse", plural)); resp != nil { | ||
| if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil { | ||
| if resource := utils.GetResource(paged.GetMessageType()); resource != nil { | ||
| for _, pattern := range resource.GetPattern() { | ||
| if strings.Count(pattern, "{") == 1 { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this logic (which is used for BatchGet, BatchCreate, and BatchUpdate) be pulled out into a function that takes the "batching method type" as param i.e. "BatchGet", as well as the necessary proto input i.e. m *desc.MessageDescriptor? This code is non-trivial and used in three places, feels ripe for a func. WDYT? If there is a nuance in the code for each that prevents this that I missed, lmk.
There was a problem hiding this comment.
It can be, although the code is slightly different each time, and I am not sure there is a good place to put it. I do not know that it is common enough to justify going in utils. This is right in the sour spot where the function can not be in the AIP modules themselves, but are not generic enough for me to be convinced that they belong in utils.
There was a problem hiding this comment.
Ok, I understand. Bummer!
Also, I rewrote the tests for these rules from scratch, as they
were in a weird state.
Fixes #615.