fix: [REGAPIC] Fix repeated fields handling for query parameters#989
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
vam-google
requested review from
Neenu1995 and
eaball35
and removed request for
Neenu1995
Why do we want to change this? Looks cosmetic to me. If it is indeed required change, do we want to port this over to GRPC client as well, like here? Also in the same class, there is another similar logic as well. |
| String currFieldName = descendantFields[i]; | ||
| String bindingFieldMethodName = | ||
| String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName)); | ||
| (i < descendantFields.length - 1 || !httpBindingFieldName.isRepeated()) |
There was a problem hiding this comment.
Is it easier to read if we change it to httpBindingFieldName.isRepeated() && i == descendantFields.length - 1? Also is it a valid case that a repeated field appear in positions other than the last one? Or you are just trying to be defensive here?
If it is indeed a valid case, can you please add a test case for it as well?
There was a problem hiding this comment.
I just keep i < descendantFields.length - 1 construct in the if statement to be consistent with the rest of similar statements in the same method (there are several of them in this method).
No, repeated field can't have "nested" fields because repeated field is a List. But most importantly httpBindingFieldName.isRepeated() always represents the characteristic of the last field in the . chain only. I.e. if we have field_one.field_two.field_three there will be only one binding object with name = "field_one.field_two.field_three" and isRepeated = false if field_three is a regular field and isRepeated = true if it is repeated. field_one and field_two are guaranteed to be non-primitive, non-repeated message fields (when the type of field_one has a field named field_two), otherwise it would be a broken API definition.
There was a problem hiding this comment.
If it's not a valid case, can we simplify it to if (!httpBindingFieldName.isRepeated())?
|
@blakeli0 This is not cosmetic, this is required for the generated code to compile, as As for porting it to grpc code - maybe, but it is not in scope of this PR as I don't want to risk breaking grpc code while working on regapic part (it is going to be a big rollout and I would like keep regapic changes separate to simplify debugging and minimize risk of breaking what is already working). |
That makes sense and I think it's OK not porting over too. Because on gRPC side, the request extractor is only used for routing headers and the field can not be repeated AFAIK. However, this is another example that we have long similar logics in multiple places which we could benefit a lot if extracted to common utility methods. |
if a field is declared as repeated in proto then its getter's name is `get<FieldName>List` not `get<FieldName>` instead.
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
3 participants
if a field is declared as repeated in proto then its getter's name is
get<FieldName>Listnotget<FieldName>instead.