Skip to content

Commit db69ce4

Browse files
emmileafemmileaf
authored
Revert "fix: Get numeric value for Enum fields if it is configured as query param or path param (#1042)"
This reverts commit 4b5eadb.
1 parent a35ec81 commit db69ce4

File tree

13 files changed

+18
-674
lines changed

13 files changed

+18
-674
lines changed

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

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@
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;
6362
import com.google.common.collect.BiMap;
6463
import com.google.common.collect.ImmutableList;
6564
import com.google.protobuf.TypeRegistry;
@@ -75,7 +74,6 @@
7574
import java.util.stream.Collectors;
7675

7776
public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer {
78-
7977
private static final HttpJsonServiceStubClassComposer INSTANCE =
8078
new HttpJsonServiceStubClassComposer();
8179

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

@@ -1001,7 +997,6 @@ private Expr createFieldsExtractorClassInstance(
1001997
}
1002998
}
1003999

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

@@ -1028,20 +1023,6 @@ private Expr createFieldsExtractorClassInstance(
10281023
.build();
10291024
}
10301025

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-
10451026
private List<Expr> getHttpMethodTypeExpr(Method protoMethod) {
10461027
return Collections.singletonList(
10471028
ValueExpr.withValue(

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

Lines changed: 6 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -36,54 +36,21 @@ 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
4139
public abstract String name();
4240

4341
abstract String lowerCamelName();
4442

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

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-
}
45+
public abstract boolean isRepeated();
6046

6147
@Nullable
6248
public abstract String valuePattern();
6349

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-
}
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);
8754
}
8855

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

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

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,6 @@ 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.
9085
String body = httpRule.getBody();
9186
Set<String> bodyParamNames;
9287
Set<String> queryParamNames;
@@ -138,9 +133,8 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
138133
String patternSampleValue =
139134
patternSampleValues != null ? patternSampleValues.get(paramName) : null;
140135
String[] subFields = paramName.split("\\.");
141-
HttpBinding.Builder httpBindingBuilder = HttpBinding.builder().setName(paramName);
142136
if (inputMessage == null) {
143-
httpBindings.add(httpBindingBuilder.setValuePattern(patternSampleValue).build());
137+
httpBindings.add(HttpBinding.create(paramName, false, false, patternSampleValue));
144138
continue;
145139
}
146140
Message nestedMessage = inputMessage;
@@ -162,7 +156,8 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
162156
}
163157
Field field = nestedMessage.fieldMap().get(subFieldName);
164158
httpBindings.add(
165-
httpBindingBuilder.setValuePattern(patternSampleValue).setField(field).build());
159+
HttpBinding.create(
160+
paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue));
166161
}
167162
}
168163
}

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

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

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

17-
import com.google.api.generator.engine.ast.TypeNode;
1817
import com.google.api.generator.engine.writer.JavaWriterVisitor;
19-
import com.google.api.generator.gapic.model.Field;
2018
import com.google.api.generator.gapic.model.GapicClass;
2119
import com.google.api.generator.gapic.model.GapicContext;
22-
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
2320
import com.google.api.generator.gapic.model.Service;
2421
import com.google.api.generator.test.framework.Assert;
2522
import com.google.api.generator.test.framework.Utils;
26-
import com.google.common.truth.Truth;
2723
import java.nio.file.Path;
2824
import java.nio.file.Paths;
29-
import org.junit.Before;
3025
import org.junit.Test;
3126

3227
public class HttpJsonServiceStubClassComposerTest {
33-
34-
private HttpJsonServiceStubClassComposer composer;
35-
36-
@Before
37-
public void setUp() throws Exception {
38-
composer = HttpJsonServiceStubClassComposer.instance();
39-
}
40-
4128
@Test
4229
public void generateServiceClasses() {
4330
GapicContext context = RestTestProtoLoader.instance().parseCompliance();
4431
Service echoProtoService = context.services().get(0);
45-
GapicClass clazz = composer.generate(context, echoProtoService);
32+
GapicClass clazz =
33+
HttpJsonServiceStubClassComposer.instance().generate(context, echoProtoService);
4634

4735
JavaWriterVisitor visitor = new JavaWriterVisitor();
4836
clazz.classDefinition().accept(visitor);
@@ -51,49 +39,4 @@ public void generateServiceClasses() {
5139
Paths.get(Utils.getGoldenDir(this.getClass()), "HttpJsonComplianceStub.golden");
5240
Assert.assertCodeEquals(goldenFilePath, visitor.write());
5341
}
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-
}
9942
}

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

Lines changed: 0 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -560,102 +560,4 @@ 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-
}
661563
}

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

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -86,16 +86,6 @@ 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-
9989
public static final ComplianceSettings create(ComplianceStubSettings stub) throws IOException {
10090
return new ComplianceSettings.Builder(stub.toBuilder()).build();
10191
}
@@ -225,16 +215,6 @@ public class ComplianceSettings extends ClientSettings<ComplianceSettings> {
225215
return getStubSettingsBuilder().repeatDataPathTrailingResourceSettings();
226216
}
227217

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-
238218
@Override
239219
public ComplianceSettings build() throws IOException {
240220
return new ComplianceSettings(this);

0 commit comments

Comments
 (0)