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.

Sign up for GitHub

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

fix: [REGAPIC] Fix repeated fields handling for query parameters #989

Merged
merged 1 commit into from
May 12, 2022
Merged

fix: [REGAPIC] Fix repeated fields handling for query parameters #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion ...n/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,9 @@ private Expr createFieldsExtractorClassInstance(
for (int i = 0; i < descendantFields.length; i++) {
String currFieldName = descendantFields[i];
String bindingFieldMethodName =
String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName));
(i < descendantFields.length - 1 || !httpBindingFieldName.isRepeated())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's not a valid case, can we simplify it to if (!httpBindingFieldName.isRepeated())?

? String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName))
: String.format("get%sList", JavaStyle.toUpperCamelCase(currFieldName));
requestFieldGetterExprBuilder =
requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName);

Expand Down
7 changes: 5 additions & 2 deletions src/main/java/com/google/api/generator/gapic/model/HttpBindings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,15 @@ public abstract static class HttpBinding implements Comparable<HttpBinding> {

public abstract boolean isOptional();

public abstract boolean isRepeated();

@Nullable
public abstract String valuePattern();

public static HttpBinding create(String name, boolean isOptional, String valuePattern) {
public static HttpBinding create(
String name, boolean isOptional, boolean isRepeated, String valuePattern) {
return new AutoValue_HttpBindings_HttpBinding(
name, JavaStyle.toLowerCamelCase(name), isOptional, valuePattern);
name, JavaStyle.toLowerCamelCase(name), isOptional, isRepeated, valuePattern);
}

// Do not forget to keep it in sync with equals() implementation.
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
patternSampleValues != null ? patternSampleValues.get(paramName) : null;
String[] subFields = paramName.split("\\.");
if (inputMessage == null) {
httpBindings.add(HttpBinding.create(paramName, false, patternSampleValue));
httpBindings.add(HttpBinding.create(paramName, false, false, patternSampleValue));
continue;
}
Message nestedMessage = inputMessage;
Expand All @@ -150,7 +150,8 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
}
Field field = nestedMessage.fieldMap().get(subFieldName);
httpBindings.add(
HttpBinding.create(paramName, field.isProto3Optional(), patternSampleValue));
HttpBinding.create(
paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue));
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClient.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,10 @@ public class EchoClient implements BackgroundResource {
* // It may require modifications to work in your environment.
* try (EchoClient echoClient = EchoClient.create()) {
* ExpandRequest request =
* ExpandRequest.newBuilder().setContent("content951530617").setInfo("info3237038").build();
* ExpandRequest.newBuilder()
* .setContent("content951530617")
* .addAllInfo(new ArrayList<String>())
* .build();
* ServerStream<EchoResponse> stream = echoClient.expandCallable().call(request);
* for (EchoResponse response : stream) {
* // Do something when a response is received.
Expand Down
11 changes: 9 additions & 2 deletions src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoClientTest.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.google.protobuf.Timestamp;
import com.google.rpc.Status;
import io.grpc.StatusRuntimeException;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.UUID;
Expand Down Expand Up @@ -400,7 +401,10 @@ public class EchoClientTest {
.build();
mockEcho.addResponse(expectedResponse);
ExpandRequest request =
ExpandRequest.newBuilder().setContent("content951530617").setInfo("info3237038").build();
ExpandRequest.newBuilder()
.setContent("content951530617")
.addAllInfo(new ArrayList<String>())
.build();

MockStreamObserver<EchoResponse> responseObserver = new MockStreamObserver<>();

Expand All @@ -417,7 +421,10 @@ public class EchoClientTest {
StatusRuntimeException exception = new StatusRuntimeException(io.grpc.Status.INVALID_ARGUMENT);
mockEcho.addException(exception);
ExpandRequest request =
ExpandRequest.newBuilder().setContent("content951530617").setInfo("info3237038").build();
ExpandRequest.newBuilder()
.setContent("content951530617")
.addAllInfo(new ArrayList<String>())
.build();

MockStreamObserver<EchoResponse> responseObserver = new MockStreamObserver<>();

Expand Down
5 changes: 3 additions & 2 deletions ...est/java/com/google/api/generator/gapic/composer/grpcrest/goldens/HttpJsonEchoStub.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,12 @@ public class HttpJsonEchoStub extends EchoStub {
Map<String, List<String>> fields = new HashMap<>();
ProtoRestSerializer<ExpandRequest> serializer =
ProtoRestSerializer.create();
serializer.putQueryParam(fields, "content", request.getContent());
serializer.putQueryParam(fields, "info", request.getInfoList());
return fields;
})
.setRequestBodyExtractor(
request ->
ProtoRestSerializer.create().toBody("*", request.toBuilder().build()))
request -> ProtoRestSerializer.create().toBody("error", request.getError()))
.build())
.setResponseParser(
ProtoMessageResponseParser.<EchoResponse>newBuilder()
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/api/generator/gapic/model/MethodTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class MethodTest {
.build();
private static final HttpBindings HTTP_BINDINGS =
HttpBindings.builder()
.setPathParameters(ImmutableSet.of(HttpBinding.create("table", true, "")))
.setPathParameters(ImmutableSet.of(HttpBinding.create("table", true, false, "")))
.setPattern("/pattern/test")
.setIsAsteriskBody(false)
.setHttpVerb(HttpVerb.GET)
Expand Down
4 changes: 2 additions & 2 deletions src/test/proto/echo_grpcrest.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ service Echo {
rpc Expand(ExpandRequest) returns (stream EchoResponse) {
option (google.api.http) = {
post: "/v1beta1/echo:expand"
body: "*"
body: "error"
};
option (google.api.method_signature) = "content,error";
}
Expand Down Expand Up @@ -202,7 +202,7 @@ message ExpandRequest {
// The error that is thrown after all words are sent on the stream.
google.rpc.Status error = 2;

string info = 3;
repeated string info = 3;
}

// The request for the PagedExpand method.
Expand Down