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.

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: Get numeric value for Enum fields if it is configured as query param or path param #1042

Merged
merged 5 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import com.google.api.generator.gapic.model.OperationResponse;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.utils.JavaStyle;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.BiMap;
import com.google.common.collect.ImmutableList;
import com.google.protobuf.TypeRegistry;
Expand All @@ -74,6 +75,7 @@
import java.util.stream.Collectors;

public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer {

private static final HttpJsonServiceStubClassComposer INSTANCE =
new HttpJsonServiceStubClassComposer();

Expand Down Expand Up @@ -940,9 +942,11 @@ private Expr createFieldsExtractorClassInstance(
for (int i = 0; i < descendantFields.length; i++) {
String currFieldName = descendantFields[i];
String bindingFieldMethodName =
(i < descendantFields.length - 1 || !httpBindingFieldName.isRepeated())
? String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName))
: String.format("get%sList", JavaStyle.toUpperCamelCase(currFieldName));
getBindingFieldMethodName(
httpBindingFieldName,
descendantFields.length,
i,
JavaStyle.toUpperCamelCase(currFieldName));
requestFieldGetterExprBuilder =
requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName);

Expand Down Expand Up @@ -997,6 +1001,7 @@ private Expr createFieldsExtractorClassInstance(
}
}

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

Expand All @@ -1023,6 +1028,20 @@ private Expr createFieldsExtractorClassInstance(
.build();
}

@VisibleForTesting
String getBindingFieldMethodName(
HttpBinding httpBindingField, int descendantFieldsLengths, int index, String currFieldName) {
if (index == descendantFieldsLengths - 1) {
if (httpBindingField.isRepeated()) {
return String.format("get%sList", currFieldName);
}
if (httpBindingField.isEnum()) {
return String.format("get%sValue", currFieldName);
}
}
return String.format("get%s", currFieldName);
}

private List<Expr> getHttpMethodTypeExpr(Method protoMethod) {
return Collections.singletonList(
ValueExpr.withValue(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,54 @@ public enum HttpVerb {

@AutoValue
public abstract static class HttpBinding implements Comparable<HttpBinding> {

// The fully qualified name of the field. e.g. request.complex_object.another_object.name
public abstract String name();

abstract String lowerCamelName();

public abstract boolean isOptional();
// An object that contains all info of the leaf level field
@Nullable
public abstract Field field();

public abstract boolean isRepeated();
public boolean isOptional() {
return field() != null && field().isProto3Optional();
}

public boolean isRepeated() {
return field() != null && field().isRepeated();
}

public boolean isEnum() {
return field() != null && field().isEnum();
}

@Nullable
public abstract String valuePattern();

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

@AutoValue.Builder
public abstract static class Builder {

public abstract HttpBindings.HttpBinding.Builder setName(String name);

public abstract HttpBindings.HttpBinding.Builder setField(Field field);

abstract HttpBindings.HttpBinding.Builder setLowerCamelName(String lowerCamelName);

public abstract HttpBindings.HttpBinding.Builder setValuePattern(String valuePattern);

abstract String name();

abstract HttpBindings.HttpBinding autoBuild();

public HttpBindings.HttpBinding build() {
setLowerCamelName(JavaStyle.toLowerCamelCase(name()));
return autoBuild();
}
}

// Do not forget to keep it in sync with equals() implementation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ private static HttpBindings parseHttpRuleHelper(
Map<String, String> patternSampleValues = constructPathValuePatterns(pattern);

// TODO: support nested message fields bindings
// Nested message fields bindings for query params are already supported as part of
// https://github.com/googleapis/gax-java/pull/1784,
// however we need to excludes fields that are already configured for path params and body, see
// https://github.com/googleapis/googleapis/blob/532289228eaebe77c42438f74b8a5afa85fee1b6/google/api/http.proto#L208 for details,
// the current logic does not exclude fields that are more than one level deep.
String body = httpRule.getBody();
Set<String> bodyParamNames;
Set<String> queryParamNames;
Expand Down Expand Up @@ -133,8 +138,9 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
String patternSampleValue =
patternSampleValues != null ? patternSampleValues.get(paramName) : null;
String[] subFields = paramName.split("\\.");
HttpBinding.Builder httpBindingBuilder = HttpBinding.builder().setName(paramName);
if (inputMessage == null) {
httpBindings.add(HttpBinding.create(paramName, false, false, patternSampleValue));
httpBindings.add(httpBindingBuilder.setValuePattern(patternSampleValue).build());
continue;
}
Message nestedMessage = inputMessage;
Expand All @@ -156,8 +162,7 @@ private static Set<HttpBinding> validateAndConstructHttpBindings(
}
Field field = nestedMessage.fieldMap().get(subFieldName);
httpBindings.add(
HttpBinding.create(
paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue));
httpBindingBuilder.setValuePattern(patternSampleValue).setField(field).build());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,35 @@

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

import com.google.api.generator.engine.ast.TypeNode;
import com.google.api.generator.engine.writer.JavaWriterVisitor;
import com.google.api.generator.gapic.model.Field;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.test.framework.Assert;
import com.google.api.generator.test.framework.Utils;
import com.google.common.truth.Truth;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Before;
import org.junit.Test;

public class HttpJsonServiceStubClassComposerTest {

private HttpJsonServiceStubClassComposer composer;

@Before
public void setUp() throws Exception {
composer = HttpJsonServiceStubClassComposer.instance();
}

@Test
public void generateServiceClasses() {
GapicContext context = RestTestProtoLoader.instance().parseCompliance();
Service echoProtoService = context.services().get(0);
GapicClass clazz =
HttpJsonServiceStubClassComposer.instance().generate(context, echoProtoService);
GapicClass clazz = composer.generate(context, echoProtoService);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Expand All @@ -39,4 +51,49 @@ public void generateServiceClasses() {
Paths.get(Utils.getGoldenDir(this.getClass()), "HttpJsonComplianceStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void
getBindingFieldMethodName_shouldReturnGetFieldListIfTheFieldIsInLastPositionAndIsRepeated() {
Field field =
Field.builder()
.setIsRepeated(true)
.setName("doesNotMatter")
.setType(TypeNode.OBJECT)
.build();
HttpBinding httpBinding =
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Values");
Truth.assertThat(actual).isEqualTo("getValuesList");
}

@Test
public void
getBindingFieldMethodName_shouldReturnGetFieldValueIfTheFieldIsInLastPositionAndIsEnum() {
Field field =
Field.builder().setIsEnum(true).setName("doesNotMatter").setType(TypeNode.OBJECT).build();
HttpBinding httpBinding =
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Enums");
Truth.assertThat(actual).isEqualTo("getEnumsValue");
}

@Test
public void
getBindingFieldMethodName_shouldReturnGetFieldIfTheFieldIsInLastPositionAndNotRepeatedOrEnum() {
Field field = Field.builder().setName("doesNotMatter").setType(TypeNode.OBJECT).build();
HttpBinding httpBinding =
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 3, "Value");
Truth.assertThat(actual).isEqualTo("getValue");
}

@Test
public void getBindingFieldMethodName_shouldReturnGetFieldIfTheFieldIsNotInLastPosition() {
Field field = Field.builder().setName("doesNotMatter").setType(TypeNode.OBJECT).build();
HttpBinding httpBinding =
HttpBinding.builder().setField(field).setName("doesNotMatter").build();
String actual = composer.getBindingFieldMethodName(httpBinding, 4, 1, "Value");
Truth.assertThat(actual).isEqualTo("getValue");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -560,4 +560,102 @@ public class ComplianceClientTest {
// Expected exception.
}
}

@Test
public void getEnumTest() throws Exception {
EnumResponse expectedResponse =
EnumResponse.newBuilder()
.setRequest(EnumRequest.newBuilder().build())
.setContinent(Continent.forNumber(0))
.build();
mockService.addResponse(expectedResponse);

EnumRequest request = EnumRequest.newBuilder().setUnknownEnum(true).build();

EnumResponse actualResponse = client.getEnum(request);
Assert.assertEquals(expectedResponse, actualResponse);

List<String> actualRequests = mockService.getRequestPaths();
Assert.assertEquals(1, actualRequests.size());

String apiClientHeaderKey =
mockService
.getRequestHeaders()
.get(ApiClientHeaderProvider.getDefaultApiClientHeaderKey())
.iterator()
.next();
Assert.assertTrue(
GaxHttpJsonProperties.getDefaultApiClientHeaderPattern()
.matcher(apiClientHeaderKey)
.matches());
}

@Test
public void getEnumExceptionTest() throws Exception {
ApiException exception =
ApiExceptionFactory.createException(
new Exception(), FakeStatusCode.of(StatusCode.Code.INVALID_ARGUMENT), false);
mockService.addException(exception);

try {
EnumRequest request = EnumRequest.newBuilder().setUnknownEnum(true).build();
client.getEnum(request);
Assert.fail("No exception raised");
} catch (InvalidArgumentException e) {
// Expected exception.
}
}

@Test
public void verifyEnumTest() throws Exception {
EnumResponse expectedResponse =
EnumResponse.newBuilder()
.setRequest(EnumRequest.newBuilder().build())
.setContinent(Continent.forNumber(0))
.build();
mockService.addResponse(expectedResponse);

EnumResponse request =
EnumResponse.newBuilder()
.setRequest(EnumRequest.newBuilder().build())
.setContinent(Continent.forNumber(0))
.build();

EnumResponse actualResponse = client.verifyEnum(request);
Assert.assertEquals(expectedResponse, actualResponse);

List<String> actualRequests = mockService.getRequestPaths();
Assert.assertEquals(1, actualRequests.size());

String apiClientHeaderKey =
mockService
.getRequestHeaders()
.get(ApiClientHeaderProvider.getDefaultApiClientHeaderKey())
.iterator()
.next();
Assert.assertTrue(
GaxHttpJsonProperties.getDefaultApiClientHeaderPattern()
.matcher(apiClientHeaderKey)
.matches());
}

@Test
public void verifyEnumExceptionTest() throws Exception {
ApiException exception =
ApiExceptionFactory.createException(
new Exception(), FakeStatusCode.of(StatusCode.Code.INVALID_ARGUMENT), false);
mockService.addException(exception);

try {
EnumResponse request =
EnumResponse.newBuilder()
.setRequest(EnumRequest.newBuilder().build())
.setContinent(Continent.forNumber(0))
.build();
client.verifyEnum(request);
Assert.fail("No exception raised");
} catch (InvalidArgumentException e) {
// Expected exception.
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ public class ComplianceSettings extends ClientSettings<ComplianceSettings> {
return ((ComplianceStubSettings) getStubSettings()).repeatDataPathTrailingResourceSettings();
}

/** Returns the object with the settings used for calls to getEnum. */
public UnaryCallSettings<EnumRequest, EnumResponse> getEnumSettings() {
return ((ComplianceStubSettings) getStubSettings()).getEnumSettings();
}

/** Returns the object with the settings used for calls to verifyEnum. */
public UnaryCallSettings<EnumResponse, EnumResponse> verifyEnumSettings() {
return ((ComplianceStubSettings) getStubSettings()).verifyEnumSettings();
}

public static final ComplianceSettings create(ComplianceStubSettings stub) throws IOException {
return new ComplianceSettings.Builder(stub.toBuilder()).build();
}
Expand Down Expand Up @@ -215,6 +225,16 @@ public class ComplianceSettings extends ClientSettings<ComplianceSettings> {
return getStubSettingsBuilder().repeatDataPathTrailingResourceSettings();
}

/** Returns the builder for the settings used for calls to getEnum. */
public UnaryCallSettings.Builder<EnumRequest, EnumResponse> getEnumSettings() {
return getStubSettingsBuilder().getEnumSettings();
}

/** Returns the builder for the settings used for calls to verifyEnum. */
public UnaryCallSettings.Builder<EnumResponse, EnumResponse> verifyEnumSettings() {
return getStubSettingsBuilder().verifyEnumSettings();
}

@Override
public ComplianceSettings build() throws IOException {
return new ComplianceSettings(this);
Expand Down
Loading