Skip to content

Commit 4b5eadb

Browse files
blakeli0emmileaf
blakeli0
authored and
emmileaf
committed
fix: Get numeric value for Enum fields if it is configured as query param or path param (#1042)
Get numeric value for Enum fields if it is configured as query param or path param. Refactor HttpBinding to include Field info and use builder.
1 parent e58bf9d commit 4b5eadb

File tree

13 files changed

+674
-18
lines changed

13 files changed

+674
-18
lines changed

‎src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import com.google.api.generator.gapic.model.OperationResponse;
6060
import com.google.api.generator.gapic.model.Service;
6161
import com.google.api.generator.gapic.utils.JavaStyle;
62+
import com.google.common.annotations.VisibleForTesting;
6263
import com.google.common.collect.BiMap;
6364
import com.google.common.collect.ImmutableList;
6465
import com.google.protobuf.TypeRegistry;
@@ -74,6 +75,7 @@
7475
import java.util.stream.Collectors;
7576

7677
public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer {
78+
7779
private static final HttpJsonServiceStubClassComposer INSTANCE =
7880
new HttpJsonServiceStubClassComposer();
7981

@@ -940,9 +942,11 @@ private Expr createFieldsExtractorClassInstance(
940942
for (int i = 0; i < descendantFields.length; i++) {
941943
String currFieldName = descendantFields[i];
942944
String bindingFieldMethodName =
943-
(i < descendantFields.length - 1 || !httpBindingFieldName.isRepeated())
944-
? String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName))
945-
: String.format("get%sList", JavaStyle.toUpperCamelCase(currFieldName));
945+
getBindingFieldMethodName(
946+
httpBindingFieldName,
947+
descendantFields.length,
948+
i,
949+
JavaStyle.toUpperCamelCase(currFieldName));
946950
requestFieldGetterExprBuilder =
947951
requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName);
948952

@@ -997,6 +1001,7 @@ private Expr createFieldsExtractorClassInstance(
9971001
}
9981002
}
9991003

1004+
// Add a fixed query param for numeric enum, see b/232457244 for details
10001005
if (restNumericEnumsEnabled && serializerMethodName.equals("putQueryParam")) {
10011006
ImmutableList.Builder<Expr> paramsPutArgs = ImmutableList.builder();
10021007

@@ -1023,6 +1028,20 @@ private Expr createFieldsExtractorClassInstance(
10231028
.build();
10241029
}
10251030

1031+
@VisibleForTesting
1032+
String getBindingFieldMethodName(
1033+
HttpBinding httpBindingField, int descendantFieldsLengths, int index, String currFieldName) {
1034+
if (index == descendantFieldsLengths - 1) {
1035+
if (httpBindingField.isRepeated()) {
1036+
return String.format("get%sList", currFieldName);
1037+
}
1038+
if (httpBindingField.isEnum()) {
1039+
return String.format("get%sValue", currFieldName);
1040+
}
1041+
}
1042+
return String.format("get%s", currFieldName);
1043+
}
1044+
10261045
private List<Expr> getHttpMethodTypeExpr(Method protoMethod) {
10271046
return Collections.singletonList(
10281047
ValueExpr.withValue(

‎src/main/java/com/google/api/generator/gapic/model/HttpBindings.java‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,21 +36,54 @@ public enum HttpVerb {
3636

3737
@AutoValue
3838
public abstract static class HttpBinding implements Comparable<HttpBinding> {
39+
40+
// The fully qualified name of the field. e.g. request.complex_object.another_object.name
3941
public abstract String name();
4042

4143
abstract String lowerCamelName();
4244

43-
public abstract boolean isOptional();
45+
// An object that contains all info of the leaf level field
46+
@Nullable
47+
public abstract Field field();
4448

45-
public abstract boolean isRepeated();
49+
public boolean isOptional() {
50+
return field() != null && field().isProto3Optional();
51+
}
52+
53+
public boolean isRepeated() {
54+
return field() != null && field().isRepeated();
55+
}
56+
57+
public boolean isEnum() {
58+
return field() != null && field().isEnum();
59+
}
4660

4761
@Nullable
4862
public abstract String valuePattern();
4963

50-
public static HttpBinding create(
51-
String name, boolean isOptional, boolean isRepeated, String valuePattern) {
52-
return new AutoValue_HttpBindings_HttpBinding(
53-
name, JavaStyle.toLowerCamelCase(name), isOptional, isRepeated, valuePattern);
64+
public static HttpBindings.HttpBinding.Builder builder() {
65+
return new AutoValue_HttpBindings_HttpBinding.Builder();
66+
}
67+
68+
@AutoValue.Builder
69+
public abstract static class Builder {
70+
71+
public abstract HttpBindings.HttpBinding.Builder setName(String name);
72+
73+
public abstract HttpBindings.HttpBinding.Builder setField(Field field);
74+
75+
abstract HttpBindings.HttpBinding.Builder setLowerCamelName(String lowerCamelName);
76+
77+
public abstract HttpBindings.HttpBinding.Builder setValuePattern(String valuePattern);
78+
79+
abstract String name();
80+
81+
abstract HttpBindings.HttpBinding autoBuild();
82+
83+
public HttpBindings.HttpBinding build() {
84+
setLowerCamelName(JavaStyle.toLowerCamelCase(name()));
85+
return autoBuild();
86+
}
5487
}
5588

5689
// Do not forget to keep it in sync with equals() implementation.

‎src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,11 @@ private static HttpBindings parseHttpRuleHelper(
8282
Map<String, String> patternSampleValues = constructPathValuePatterns(pattern);
8383

8484
// TODO: support nested message fields bindings
85+
// Nested message fields bindings for query params are already supported as part of
86+
// https://github.com/googleapis/gax-java/pull/1784,
87+
// however we need to excludes fields that are already configured for path params and body, see
88+
// https://github.com/googleapis/googleapis/blob/532289228eaebe77c42438f74b8a5afa85fee1b6/google/api/http.proto#L208 for details,
89+
// the current logic does not exclude fields that are more than one level deep.
8590
String body = httpRule.getBody();
8691
Set<String> bodyParamNames;
8792
Set<String> queryParamNames;
@@ -133,8 +138,9 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
133138
String patternSampleValue =
134139
patternSampleValues != null ? patternSampleValues.get(paramName) : null;
135140
String[] subFields = paramName.split("\\.");
141+
HttpBinding.Builder httpBindingBuilder = HttpBinding.builder().setName(paramName);
136142
if (inputMessage == null) {
137-
httpBindings.add(HttpBinding.create(paramName, false, false, patternSampleValue));
143+
httpBindings.add(httpBindingBuilder.setValuePattern(patternSampleValue).build());
138144
continue;
139145
}
140146
Message nestedMessage = inputMessage;
@@ -156,8 +162,7 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
156162
}
157163
Field field = nestedMessage.fieldMap().get(subFieldName);
158164
httpBindings.add(
159-
HttpBinding.create(
160-
paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue));
165+
httpBindingBuilder.setValuePattern(patternSampleValue).setField(field).build());
161166
}
162167
}
163168
}

‎src/test/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposerTest.java‎

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,35 @@
1414

1515
package com.google.api.generator.gapic.composer.rest;
1616

17+
import com.google.api.generator.engine.ast.TypeNode;
1718
import com.google.api.generator.engine.writer.JavaWriterVisitor;
19+
import com.google.api.generator.gapic.model.Field;
1820
import com.google.api.generator.gapic.model.GapicClass;
1921
import com.google.api.generator.gapic.model.GapicContext;
22+
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
2023
import com.google.api.generator.gapic.model.Service;
2124
import com.google.api.generator.test.framework.Assert;
2225
import com.google.api.generator.test.framework.Utils;
26+
import com.google.common.truth.Truth;
2327
import java.nio.file.Path;
2428
import java.nio.file.Paths;
29+
import org.junit.Before;
2530
import org.junit.Test;
2631

2732
public class HttpJsonServiceStubClassComposerTest {
33+
34+
private HttpJsonServiceStubClassComposer composer;
35+
36+
@Before
37+
public void setUp() throws Exception {
38+
composer = HttpJsonServiceStubClassComposer.instance();
39+
}
40+
2841
@Test
2942
public void generateServiceClasses() {
3043
GapicContext context = RestTestProtoLoader.instance().parseCompliance();
3144
Service echoProtoService = context.services().get(0);
32-
GapicClass clazz =
33-
HttpJsonServiceStubClassComposer.instance().generate(context, echoProtoService);
45+
GapicClass clazz = composer.generate(context, echoProtoService);
3446

3547
JavaWriterVisitor visitor = new JavaWriterVisitor();
3648
clazz.classDefinition().accept(visitor);
@@ -39,4 +51,49 @@ public void generateServiceClasses() {
3951
Paths.get(Utils.getGoldenDir(this.getClass()), "HttpJsonComplianceStub.golden");
4052
Assert.assertCodeEquals(goldenFilePath, visitor.write());
4153
}
54+
55+
@Test
56+
public void
57+
getBindingFieldMethodName_shouldReturnGetFieldListIfTheFieldIsInLastPositionAndIsRepeated() {
58+
Field field =
59+
Field.builder()
60+
.setIsRepeated(true)
61+
.setName("doesNotMatter")
62+
.setType(TypeNode.OBJECT)
63+
.build();
64+
HttpBinding httpBinding =
65+
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
66+
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Values");
67+
Truth.assertThat(actual).isEqualTo("getValuesList");
68+
}
69+
70+
@Test
71+
public void
72+
getBindingFieldMethodName_shouldReturnGetFieldValueIfTheFieldIsInLastPositionAndIsEnum() {
73+
Field field =
74+
Field.builder().setIsEnum(true).setName("doesNotMatter").setType(TypeNode.OBJECT).build();
75+
HttpBinding httpBinding =
76+
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
77+
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Enums");
78+
Truth.assertThat(actual).isEqualTo("getEnumsValue");
79+
}
80+
81+
@Test
82+
public void
83+
getBindingFieldMethodName_shouldReturnGetFieldIfTheFieldIsInLastPositionAndNotRepeatedOrEnum() {
84+
Field field = Field.builder().setName("doesNotMatter").setType(TypeNode.OBJECT).build();
85+
HttpBinding httpBinding =
86+
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
87+
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Value");
88+
Truth.assertThat(actual).isEqualTo("getValue");
89+
}
90+
91+
@Test
92+
public void getBindingFieldMethodName_shouldReturnGetFieldIfTheFieldIsNotInLastPosition() {
93+
Field field = Field.builder().setName("doesNotMatter").setType(TypeNode.OBJECT).build();
94+
HttpBinding httpBinding =
95+
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
96+
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 1, "Value");
97+
Truth.assertThat(actual).isEqualTo("getValue");
98+
}
4299
}

‎src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceClientTest.golden‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,4 +560,102 @@ public class ComplianceClientTest {
560560
// Expected exception.
561561
}
562562
}
563+
564+
@Test
565+
public void getEnumTest() throws Exception {
566+
EnumResponse expectedResponse =
567+
EnumResponse.newBuilder()
568+
.setRequest(EnumRequest.newBuilder().build())
569+
.setContinent(Continent.forNumber(0))
570+
.build();
571+
mockService.addResponse(expectedResponse);
572+
573+
EnumRequest request = EnumRequest.newBuilder().setUnknownEnum(true).build();
574+
575+
EnumResponse actualResponse = client.getEnum(request);
576+
Assert.assertEquals(expectedResponse, actualResponse);
577+
578+
List<String> actualRequests = mockService.getRequestPaths();
579+
Assert.assertEquals(1, actualRequests.size());
580+
581+
String apiClientHeaderKey =
582+
mockService
583+
.getRequestHeaders()
584+
.get(ApiClientHeaderProvider.getDefaultApiClientHeaderKey())
585+
.iterator()
586+
.next();
587+
Assert.assertTrue(
588+
GaxHttpJsonProperties.getDefaultApiClientHeaderPattern()
589+
.matcher(apiClientHeaderKey)
590+
.matches());
591+
}
592+
593+
@Test
594+
public void getEnumExceptionTest() throws Exception {
595+
ApiException exception =
596+
ApiExceptionFactory.createException(
597+
new Exception(), FakeStatusCode.of(StatusCode.Code.INVALID_ARGUMENT), false);
598+
mockService.addException(exception);
599+
600+
try {
601+
EnumRequest request = EnumRequest.newBuilder().setUnknownEnum(true).build();
602+
client.getEnum(request);
603+
Assert.fail("No exception raised");
604+
} catch (InvalidArgumentException e) {
605+
// Expected exception.
606+
}
607+
}
608+
609+
@Test
610+
public void verifyEnumTest() throws Exception {
611+
EnumResponse expectedResponse =
612+
EnumResponse.newBuilder()
613+
.setRequest(EnumRequest.newBuilder().build())
614+
.setContinent(Continent.forNumber(0))
615+
.build();
616+
mockService.addResponse(expectedResponse);
617+
618+
EnumResponse request =
619+
EnumResponse.newBuilder()
620+
.setRequest(EnumRequest.newBuilder().build())
621+
.setContinent(Continent.forNumber(0))
622+
.build();
623+
624+
EnumResponse actualResponse = client.verifyEnum(request);
625+
Assert.assertEquals(expectedResponse, actualResponse);
626+
627+
List<String> actualRequests = mockService.getRequestPaths();
628+
Assert.assertEquals(1, actualRequests.size());
629+
630+
String apiClientHeaderKey =
631+
mockService
632+
.getRequestHeaders()
633+
.get(ApiClientHeaderProvider.getDefaultApiClientHeaderKey())
634+
.iterator()
635+
.next();
636+
Assert.assertTrue(
637+
GaxHttpJsonProperties.getDefaultApiClientHeaderPattern()
638+
.matcher(apiClientHeaderKey)
639+
.matches());
640+
}
641+
642+
@Test
643+
public void verifyEnumExceptionTest() throws Exception {
644+
ApiException exception =
645+
ApiExceptionFactory.createException(
646+
new Exception(), FakeStatusCode.of(StatusCode.Code.INVALID_ARGUMENT), false);
647+
mockService.addException(exception);
648+
649+
try {
650+
EnumResponse request =
651+
EnumResponse.newBuilder()
652+
.setRequest(EnumRequest.newBuilder().build())
653+
.setContinent(Continent.forNumber(0))
654+
.build();
655+
client.verifyEnum(request);
656+
Assert.fail("No exception raised");
657+
} catch (InvalidArgumentException e) {
658+
// Expected exception.
659+
}
660+
}
563661
}

‎src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceSettings.golden‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,16 @@ public class ComplianceSettings extends ClientSettings<ComplianceSettings> {
8686
return ((ComplianceStubSettings) getStubSettings()).repeatDataPathTrailingResourceSettings();
8787
}
8888

89+
/** Returns the object with the settings used for calls to getEnum. */
90+
public UnaryCallSettings<EnumRequest, EnumResponse> getEnumSettings() {
91+
return ((ComplianceStubSettings) getStubSettings()).getEnumSettings();
92+
}
93+
94+
/** Returns the object with the settings used for calls to verifyEnum. */
95+
public UnaryCallSettings<EnumResponse, EnumResponse> verifyEnumSettings() {
96+
return ((ComplianceStubSettings) getStubSettings()).verifyEnumSettings();
97+
}
98+
8999
public static final ComplianceSettings create(ComplianceStubSettings stub) throws IOException {
90100
return new ComplianceSettings.Builder(stub.toBuilder()).build();
91101
}
@@ -215,6 +225,16 @@ public class ComplianceSettings extends ClientSettings<ComplianceSettings> {
215225
return getStubSettingsBuilder().repeatDataPathTrailingResourceSettings();
216226
}
217227

228+
/** Returns the builder for the settings used for calls to getEnum. */
229+
public UnaryCallSettings.Builder<EnumRequest, EnumResponse> getEnumSettings() {
230+
return getStubSettingsBuilder().getEnumSettings();
231+
}
232+
233+
/** Returns the builder for the settings used for calls to verifyEnum. */
234+
public UnaryCallSettings.Builder<EnumResponse, EnumResponse> verifyEnumSettings() {
235+
return getStubSettingsBuilder().verifyEnumSettings();
236+
}
237+
218238
@Override
219239
public ComplianceSettings build() throws IOException {
220240
return new ComplianceSettings(this);

0 commit comments

Comments
 (0)