From 4b5eadb343c90c988e4bcec24e17dc17a6ee5964 Mon Sep 17 00:00:00 2001 From: Blake Li Date: Mon, 3 Oct 2022 16:10:19 +0000 Subject: [PATCH] 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. --- .../HttpJsonServiceStubClassComposer.java | 25 ++++- .../generator/gapic/model/HttpBindings.java | 45 +++++++-- .../gapic/protoparser/HttpRuleParser.java | 11 ++- .../HttpJsonServiceStubClassComposerTest.java | 61 +++++++++++- .../rest/goldens/ComplianceClientTest.golden | 98 ++++++++++++++++++ .../rest/goldens/ComplianceSettings.golden | 20 ++++ .../goldens/ComplianceStubSettings.golden | 50 +++++++++- .../goldens/HttpJsonComplianceStub.golden | 97 +++++++++++++++++- .../gapic/model/HttpBindingsTest.java | 99 +++++++++++++++++++ .../api/generator/gapic/model/MethodTest.java | 2 +- .../gapic/protoparser/HttpRuleParserTest.java | 83 ++++++++++++++++ src/test/proto/compliance.proto | 38 +++++++ src/test/proto/http_rule_parser_testing.proto | 63 ++++++++++++ 13 files changed, 674 insertions(+), 18 deletions(-) create mode 100644 src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java create mode 100644 src/test/proto/http_rule_parser_testing.proto diff --git a/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java index fa4d71498c..67e003f50f 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java @@ -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; @@ -74,6 +75,7 @@ import java.util.stream.Collectors; public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer { + private static final HttpJsonServiceStubClassComposer INSTANCE = new HttpJsonServiceStubClassComposer(); @@ -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); @@ -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 paramsPutArgs = ImmutableList.builder(); @@ -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 getHttpMethodTypeExpr(Method protoMethod) { return Collections.singletonList( ValueExpr.withValue( diff --git a/src/main/java/com/google/api/generator/gapic/model/HttpBindings.java b/src/main/java/com/google/api/generator/gapic/model/HttpBindings.java index e6ecea5c94..696683cefe 100644 --- a/src/main/java/com/google/api/generator/gapic/model/HttpBindings.java +++ b/src/main/java/com/google/api/generator/gapic/model/HttpBindings.java @@ -36,21 +36,54 @@ public enum HttpVerb { @AutoValue public abstract static class HttpBinding implements Comparable { + + // 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. diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java b/src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java index bead11e082..87bcd76abb 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java @@ -82,6 +82,11 @@ private static HttpBindings parseHttpRuleHelper( Map 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 bodyParamNames; Set queryParamNames; @@ -133,8 +138,9 @@ private static Set 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; @@ -156,8 +162,7 @@ private static Set validateAndConstructHttpBindings( } Field field = nestedMessage.fieldMap().get(subFieldName); httpBindings.add( - HttpBinding.create( - paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue)); + httpBindingBuilder.setValuePattern(patternSampleValue).setField(field).build()); } } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposerTest.java b/src/test/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposerTest.java index e8eccd9eec..243ef54ce2 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposerTest.java +++ b/src/test/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposerTest.java @@ -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); @@ -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"); + } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceClientTest.golden index 9f72651e9c..07e6b442e2 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceClientTest.golden @@ -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 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 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. + } + } } diff --git a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceSettings.golden b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceSettings.golden index afa6402beb..3730206623 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceSettings.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceSettings.golden @@ -86,6 +86,16 @@ public class ComplianceSettings extends ClientSettings { return ((ComplianceStubSettings) getStubSettings()).repeatDataPathTrailingResourceSettings(); } + /** Returns the object with the settings used for calls to getEnum. */ + public UnaryCallSettings getEnumSettings() { + return ((ComplianceStubSettings) getStubSettings()).getEnumSettings(); + } + + /** Returns the object with the settings used for calls to verifyEnum. */ + public UnaryCallSettings verifyEnumSettings() { + return ((ComplianceStubSettings) getStubSettings()).verifyEnumSettings(); + } + public static final ComplianceSettings create(ComplianceStubSettings stub) throws IOException { return new ComplianceSettings.Builder(stub.toBuilder()).build(); } @@ -215,6 +225,16 @@ public class ComplianceSettings extends ClientSettings { return getStubSettingsBuilder().repeatDataPathTrailingResourceSettings(); } + /** Returns the builder for the settings used for calls to getEnum. */ + public UnaryCallSettings.Builder getEnumSettings() { + return getStubSettingsBuilder().getEnumSettings(); + } + + /** Returns the builder for the settings used for calls to verifyEnum. */ + public UnaryCallSettings.Builder verifyEnumSettings() { + return getStubSettingsBuilder().verifyEnumSettings(); + } + @Override public ComplianceSettings build() throws IOException { return new ComplianceSettings(this); diff --git a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceStubSettings.golden b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceStubSettings.golden index f124efd10d..acd321473f 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceStubSettings.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/ComplianceStubSettings.golden @@ -19,6 +19,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; +import com.google.showcase.v1beta1.EnumRequest; +import com.google.showcase.v1beta1.EnumResponse; import com.google.showcase.v1beta1.RepeatRequest; import com.google.showcase.v1beta1.RepeatResponse; import java.io.IOException; @@ -75,6 +77,8 @@ public class ComplianceStubSettings extends StubSettings private final UnaryCallSettings repeatDataPathResourceSettings; private final UnaryCallSettings repeatDataPathTrailingResourceSettings; + private final UnaryCallSettings getEnumSettings; + private final UnaryCallSettings verifyEnumSettings; /** Returns the object with the settings used for calls to repeatDataBody. */ public UnaryCallSettings repeatDataBodySettings() { @@ -106,6 +110,16 @@ public class ComplianceStubSettings extends StubSettings return repeatDataPathTrailingResourceSettings; } + /** Returns the object with the settings used for calls to getEnum. */ + public UnaryCallSettings getEnumSettings() { + return getEnumSettings; + } + + /** Returns the object with the settings used for calls to verifyEnum. */ + public UnaryCallSettings verifyEnumSettings() { + return verifyEnumSettings; + } + public ComplianceStub createStub() throws IOException { if (getTransportChannelProvider() .getTransportName() @@ -189,6 +203,8 @@ public class ComplianceStubSettings extends StubSettings repeatDataPathResourceSettings = settingsBuilder.repeatDataPathResourceSettings().build(); repeatDataPathTrailingResourceSettings = settingsBuilder.repeatDataPathTrailingResourceSettings().build(); + getEnumSettings = settingsBuilder.getEnumSettings().build(); + verifyEnumSettings = settingsBuilder.verifyEnumSettings().build(); } /** Builder for ComplianceStubSettings. */ @@ -204,6 +220,8 @@ public class ComplianceStubSettings extends StubSettings repeatDataPathResourceSettings; private final UnaryCallSettings.Builder repeatDataPathTrailingResourceSettings; + private final UnaryCallSettings.Builder getEnumSettings; + private final UnaryCallSettings.Builder verifyEnumSettings; private static final ImmutableMap> RETRYABLE_CODE_DEFINITIONS; @@ -237,6 +255,8 @@ public class ComplianceStubSettings extends StubSettings repeatDataSimplePathSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); repeatDataPathResourceSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); repeatDataPathTrailingResourceSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); + getEnumSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); + verifyEnumSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); unaryMethodSettingsBuilders = ImmutableList.>of( @@ -245,7 +265,9 @@ public class ComplianceStubSettings extends StubSettings repeatDataQuerySettings, repeatDataSimplePathSettings, repeatDataPathResourceSettings, - repeatDataPathTrailingResourceSettings); + repeatDataPathTrailingResourceSettings, + getEnumSettings, + verifyEnumSettings); initDefaults(this); } @@ -259,6 +281,8 @@ public class ComplianceStubSettings extends StubSettings repeatDataPathResourceSettings = settings.repeatDataPathResourceSettings.toBuilder(); repeatDataPathTrailingResourceSettings = settings.repeatDataPathTrailingResourceSettings.toBuilder(); + getEnumSettings = settings.getEnumSettings.toBuilder(); + verifyEnumSettings = settings.verifyEnumSettings.toBuilder(); unaryMethodSettingsBuilders = ImmutableList.>of( @@ -267,7 +291,9 @@ public class ComplianceStubSettings extends StubSettings repeatDataQuerySettings, repeatDataSimplePathSettings, repeatDataPathResourceSettings, - repeatDataPathTrailingResourceSettings); + repeatDataPathTrailingResourceSettings, + getEnumSettings, + verifyEnumSettings); } private static Builder createDefault() { @@ -314,6 +340,16 @@ public class ComplianceStubSettings extends StubSettings .setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("no_retry_codes")) .setRetrySettings(RETRY_PARAM_DEFINITIONS.get("no_retry_params")); + builder + .getEnumSettings() + .setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("no_retry_codes")) + .setRetrySettings(RETRY_PARAM_DEFINITIONS.get("no_retry_params")); + + builder + .verifyEnumSettings() + .setRetryableCodes(RETRYABLE_CODE_DEFINITIONS.get("no_retry_codes")) + .setRetrySettings(RETRY_PARAM_DEFINITIONS.get("no_retry_params")); + return builder; } @@ -364,6 +400,16 @@ public class ComplianceStubSettings extends StubSettings return repeatDataPathTrailingResourceSettings; } + /** Returns the builder for the settings used for calls to getEnum. */ + public UnaryCallSettings.Builder getEnumSettings() { + return getEnumSettings; + } + + /** Returns the builder for the settings used for calls to verifyEnum. */ + public UnaryCallSettings.Builder verifyEnumSettings() { + return verifyEnumSettings; + } + @Override public ComplianceStubSettings build() throws IOException { return new ComplianceStubSettings(this); diff --git a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonComplianceStub.golden b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonComplianceStub.golden index f2836caf50..bf282ba988 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonComplianceStub.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/rest/goldens/HttpJsonComplianceStub.golden @@ -13,6 +13,8 @@ import com.google.api.gax.httpjson.ProtoRestSerializer; import com.google.api.gax.rpc.ClientContext; import com.google.api.gax.rpc.UnaryCallable; import com.google.protobuf.TypeRegistry; +import com.google.showcase.v1beta1.EnumRequest; +import com.google.showcase.v1beta1.EnumResponse; import com.google.showcase.v1beta1.RepeatRequest; import com.google.showcase.v1beta1.RepeatResponse; import java.io.IOException; @@ -168,7 +170,7 @@ public class HttpJsonComplianceStub extends ComplianceStub { serializer.putPathParam( fields, "info.fInt32", request.getInfo().getFInt32()); serializer.putPathParam( - fields, "info.fKingdom", request.getInfo().getFKingdom()); + fields, "info.fKingdom", request.getInfo().getFKingdomValue()); serializer.putPathParam( fields, "info.fString", request.getInfo().getFString()); return fields; @@ -285,12 +287,77 @@ public class HttpJsonComplianceStub extends ComplianceStub { .build()) .build(); + private static final ApiMethodDescriptor getEnumMethodDescriptor = + ApiMethodDescriptor.newBuilder() + .setFullMethodName("google.showcase.v1beta1.Compliance/GetEnum") + .setHttpMethod("GET") + .setType(ApiMethodDescriptor.MethodType.UNARY) + .setRequestFormatter( + ProtoMessageRequestFormatter.newBuilder() + .setPath( + "/v1beta1/compliance/enum", + request -> { + Map fields = new HashMap<>(); + ProtoRestSerializer serializer = ProtoRestSerializer.create(); + return fields; + }) + .setQueryParamsExtractor( + request -> { + Map> fields = new HashMap<>(); + ProtoRestSerializer serializer = ProtoRestSerializer.create(); + serializer.putQueryParam(fields, "unknownEnum", request.getUnknownEnum()); + serializer.putQueryParam(fields, "$alt", "json;enum-encoding=int"); + return fields; + }) + .setRequestBodyExtractor(request -> null) + .build()) + .setResponseParser( + ProtoMessageResponseParser.newBuilder() + .setDefaultInstance(EnumResponse.getDefaultInstance()) + .setDefaultTypeRegistry(typeRegistry) + .build()) + .build(); + + private static final ApiMethodDescriptor verifyEnumMethodDescriptor = + ApiMethodDescriptor.newBuilder() + .setFullMethodName("google.showcase.v1beta1.Compliance/VerifyEnum") + .setHttpMethod("POST") + .setType(ApiMethodDescriptor.MethodType.UNARY) + .setRequestFormatter( + ProtoMessageRequestFormatter.newBuilder() + .setPath( + "/v1beta1/compliance/enum", + request -> { + Map fields = new HashMap<>(); + ProtoRestSerializer serializer = ProtoRestSerializer.create(); + return fields; + }) + .setQueryParamsExtractor( + request -> { + Map> fields = new HashMap<>(); + ProtoRestSerializer serializer = ProtoRestSerializer.create(); + serializer.putQueryParam(fields, "continent", request.getContinentValue()); + serializer.putQueryParam(fields, "request", request.getRequest()); + serializer.putQueryParam(fields, "$alt", "json;enum-encoding=int"); + return fields; + }) + .setRequestBodyExtractor(request -> null) + .build()) + .setResponseParser( + ProtoMessageResponseParser.newBuilder() + .setDefaultInstance(EnumResponse.getDefaultInstance()) + .setDefaultTypeRegistry(typeRegistry) + .build()) + .build(); + private final UnaryCallable repeatDataBodyCallable; private final UnaryCallable repeatDataBodyInfoCallable; private final UnaryCallable repeatDataQueryCallable; private final UnaryCallable repeatDataSimplePathCallable; private final UnaryCallable repeatDataPathResourceCallable; private final UnaryCallable repeatDataPathTrailingResourceCallable; + private final UnaryCallable getEnumCallable; + private final UnaryCallable verifyEnumCallable; private final BackgroundResource backgroundResources; private final HttpJsonStubCallableFactory callableFactory; @@ -364,6 +431,16 @@ public class HttpJsonComplianceStub extends ComplianceStub { .setMethodDescriptor(repeatDataPathTrailingResourceMethodDescriptor) .setTypeRegistry(typeRegistry) .build(); + HttpJsonCallSettings getEnumTransportSettings = + HttpJsonCallSettings.newBuilder() + .setMethodDescriptor(getEnumMethodDescriptor) + .setTypeRegistry(typeRegistry) + .build(); + HttpJsonCallSettings verifyEnumTransportSettings = + HttpJsonCallSettings.newBuilder() + .setMethodDescriptor(verifyEnumMethodDescriptor) + .setTypeRegistry(typeRegistry) + .build(); this.repeatDataBodyCallable = callableFactory.createUnaryCallable( @@ -391,6 +468,12 @@ public class HttpJsonComplianceStub extends ComplianceStub { repeatDataPathTrailingResourceTransportSettings, settings.repeatDataPathTrailingResourceSettings(), clientContext); + this.getEnumCallable = + callableFactory.createUnaryCallable( + getEnumTransportSettings, settings.getEnumSettings(), clientContext); + this.verifyEnumCallable = + callableFactory.createUnaryCallable( + verifyEnumTransportSettings, settings.verifyEnumSettings(), clientContext); this.backgroundResources = new BackgroundResourceAggregation(clientContext.getBackgroundResources()); @@ -405,6 +488,8 @@ public class HttpJsonComplianceStub extends ComplianceStub { methodDescriptors.add(repeatDataSimplePathMethodDescriptor); methodDescriptors.add(repeatDataPathResourceMethodDescriptor); methodDescriptors.add(repeatDataPathTrailingResourceMethodDescriptor); + methodDescriptors.add(getEnumMethodDescriptor); + methodDescriptors.add(verifyEnumMethodDescriptor); return methodDescriptors; } @@ -438,6 +523,16 @@ public class HttpJsonComplianceStub extends ComplianceStub { return repeatDataPathTrailingResourceCallable; } + @Override + public UnaryCallable getEnumCallable() { + return getEnumCallable; + } + + @Override + public UnaryCallable verifyEnumCallable() { + return verifyEnumCallable; + } + @Override public final void close() { try { diff --git a/src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java b/src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java new file mode 100644 index 0000000000..f8713e0564 --- /dev/null +++ b/src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java @@ -0,0 +1,99 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.api.generator.gapic.model; + +import com.google.api.generator.engine.ast.TypeNode; +import com.google.api.generator.gapic.model.HttpBindings.HttpBinding; +import com.google.common.truth.Truth; +import org.junit.Before; +import org.junit.Test; + +public class HttpBindingsTest { + + public Field.Builder fieldBuilder; + public HttpBinding.Builder httpBindingBuilder; + + @Before + public void setUp() throws Exception { + fieldBuilder = Field.builder().setName("doesNotMatter").setType(TypeNode.OBJECT); + httpBindingBuilder = HttpBinding.builder().setName("doesNotMatter"); + } + + @Test + public void isOptional_shouldReturnFalseIfFieldIsNull() { + HttpBinding httpBinding = httpBindingBuilder.build(); + Truth.assertThat(httpBinding.isOptional()).isFalse(); + } + + @Test + public void isOptional_shouldReturnFalseIfFieldExistsAndIsOptionalIsFalse() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsProto3Optional(false).build()).build(); + + Truth.assertThat(httpBinding.isOptional()).isFalse(); + } + + @Test + public void isOptional_shouldReturnTrueIfFieldExistsAndIsOptionalIsTue() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsProto3Optional(true).build()).build(); + + Truth.assertThat(httpBinding.isOptional()).isTrue(); + } + + @Test + public void isRepeated_shouldReturnFalseIfFieldIsNull() { + HttpBinding httpBinding = httpBindingBuilder.build(); + Truth.assertThat(httpBinding.isRepeated()).isFalse(); + } + + @Test + public void isRepeated_shouldReturnFalseIfFieldExistsAndIsRepeatedIsFalse() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsRepeated(false).build()).build(); + + Truth.assertThat(httpBinding.isRepeated()).isFalse(); + } + + @Test + public void isRepeated_shouldReturnTrueIfFieldExistsAndIsRepeatedIsTue() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsRepeated(true).build()).build(); + + Truth.assertThat(httpBinding.isRepeated()).isTrue(); + } + + @Test + public void isEnum_shouldReturnFalseIfFieldIsNull() { + HttpBinding httpBinding = httpBindingBuilder.build(); + Truth.assertThat(httpBinding.isEnum()).isFalse(); + } + + @Test + public void isEnum_shouldReturnFalseIfFieldExistsAndIsEnumIsFalse() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsEnum(false).build()).build(); + + Truth.assertThat(httpBinding.isEnum()).isFalse(); + } + + @Test + public void isEnum_shouldReturnTrueIfFieldExistsAndIsEnumIsTue() { + HttpBinding httpBinding = + httpBindingBuilder.setField(fieldBuilder.setIsEnum(true).build()).build(); + + Truth.assertThat(httpBinding.isEnum()).isTrue(); + } +} diff --git a/src/test/java/com/google/api/generator/gapic/model/MethodTest.java b/src/test/java/com/google/api/generator/gapic/model/MethodTest.java index 93fe4b8135..cecee806f9 100644 --- a/src/test/java/com/google/api/generator/gapic/model/MethodTest.java +++ b/src/test/java/com/google/api/generator/gapic/model/MethodTest.java @@ -34,7 +34,7 @@ public class MethodTest { .build(); private static final HttpBindings HTTP_BINDINGS = HttpBindings.builder() - .setPathParameters(ImmutableSet.of(HttpBinding.create("table", true, false, ""))) + .setPathParameters(ImmutableSet.of(HttpBinding.builder().setName("table").build())) .setPattern("/pattern/test") .setAdditionalPatterns(Arrays.asList("/extra_pattern/test", "/extra_pattern/hey")) .setIsAsteriskBody(false) diff --git a/src/test/java/com/google/api/generator/gapic/protoparser/HttpRuleParserTest.java b/src/test/java/com/google/api/generator/gapic/protoparser/HttpRuleParserTest.java index 31fc531555..5269e1d982 100644 --- a/src/test/java/com/google/api/generator/gapic/protoparser/HttpRuleParserTest.java +++ b/src/test/java/com/google/api/generator/gapic/protoparser/HttpRuleParserTest.java @@ -22,12 +22,16 @@ import com.google.api.generator.gapic.model.HttpBindings; import com.google.api.generator.gapic.model.HttpBindings.HttpBinding; import com.google.api.generator.gapic.model.Message; +import com.google.common.truth.Truth; +import com.google.http.rule.parser.HttpRuleParserTestingOuterClass; import com.google.protobuf.Descriptors.FileDescriptor; import com.google.protobuf.Descriptors.MethodDescriptor; import com.google.protobuf.Descriptors.ServiceDescriptor; import com.google.showcase.v1beta1.TestingOuterClass; +import java.util.HashSet; import java.util.Map; import java.util.stream.Collectors; +import org.junit.Ignore; import org.junit.Test; public class HttpRuleParserTest { @@ -91,4 +95,83 @@ public void parseHttpAnnotation_missingFieldFromMessage() { assertThrows( IllegalStateException.class, () -> HttpRuleParser.parse(rpcMethod, inputMessage, messages)); } + + @Test + public void + parseHttpAnnotation_shouldPutAllFieldsIntoQueryParamsIfPathParamAndBodyAreNotConfigured() { + FileDescriptor fileDescriptor = HttpRuleParserTestingOuterClass.getDescriptor(); + ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0); + assertEquals("HttpRuleParserTesting", serviceDescriptor.getName()); + + Map messages = Parser.parseMessages(fileDescriptor); + + MethodDescriptor rpcMethod = + serviceDescriptor.getMethods().stream() + .filter( + methodDescriptor -> methodDescriptor.getName().equals("QueryParamHappyPathTest")) + .findAny() + .get(); + Message inputMessage = messages.get("com.google.http.rule.parser.QueryParamRequest"); + HttpBindings actual = HttpRuleParser.parse(rpcMethod, inputMessage, messages); + + HttpBinding expected1 = + HttpBinding.builder().setName("name").setField(inputMessage.fieldMap().get("name")).build(); + HttpBinding expected2 = + HttpBinding.builder() + .setName("nested_object") + .setField(inputMessage.fieldMap().get("nested_object")) + .build(); + Truth.assertThat(new HashSet<>(actual.queryParameters())).containsExactly(expected1, expected2); + } + + @Ignore + @Test + // request + // / \ + // nested_object name + // / \ + // name continent + // Given a request above, if nested_object.name is configured as path param, we should only + // include nested_object.continent and name as query param. + // However, the current logic put the name and the whole nested_object as query params, gax-java + // will then serialize all sub fields to query params. + // We need to either traverse all the leaf level fields and exclude field in the generator or pass + // the excluded fields to gax-java. Re-enable this test once + // https://github.com/googleapis/gapic-generator-java/issues/1041 is fixed + public void parseHttpAnnotation_shouldExcludeFieldsFromQueryParamsIfPathParamsAreConfigured() { + FileDescriptor fileDescriptor = HttpRuleParserTestingOuterClass.getDescriptor(); + ServiceDescriptor serviceDescriptor = fileDescriptor.getServices().get(0); + assertEquals("HttpRuleParserTesting", serviceDescriptor.getName()); + + Map messages = Parser.parseMessages(fileDescriptor); + + MethodDescriptor rpcMethod = + serviceDescriptor.getMethods().stream() + .filter( + methodDescriptor -> + methodDescriptor.getName().equals("ExcludePathParamsQueryParamTest")) + .findAny() + .get(); + Message inputMessage = messages.get("com.google.http.rule.parser.QueryParamRequest"); + + HttpBindings actual = HttpRuleParser.parse(rpcMethod, inputMessage, messages); + + Message nestedObjectMessage = messages.get("com.google.http.rule.parser.NestedObject"); + + HttpBinding expected1 = + HttpBinding.builder().setName("name").setField(inputMessage.fieldMap().get("name")).build(); + HttpBinding expected2 = + HttpBinding.builder() + .setName("nested_object.continent") + .setField(nestedObjectMessage.fieldMap().get("continent")) + .build(); + HttpBinding expectedPathParam = + HttpBinding.builder() + .setName("nested_object.name") + .setField(nestedObjectMessage.fieldMap().get("name")) + .build(); + + Truth.assertThat(new HashSet<>(actual.queryParameters())).containsExactly(expected1, expected2); + Truth.assertThat(new HashSet<>(actual.pathParameters())).containsExactly(expectedPathParam); + } } diff --git a/src/test/proto/compliance.proto b/src/test/proto/compliance.proto index 4467a387ab..00f71a9c05 100644 --- a/src/test/proto/compliance.proto +++ b/src/test/proto/compliance.proto @@ -80,6 +80,44 @@ service Compliance { get: "/v1beta1/repeat/{info.f_string=first/*}/{info.f_child.f_string=second/**}:pathtrailingresource" }; } + + // This method requests an enum value from the server. Depending on the contents of EnumRequest, the enum value returned will be a known enum declared in the + // .proto file, or a made-up enum value the is unknown to the client. To verify that clients can round-trip unknown enum vaues they receive, use the + // response from this RPC as the request to VerifyEnum() + // + // The values of enums sent by the server when a known or unknown value is requested will be the same within a single Showcase server run (this is needed for + // VerifyEnum() to work) but are not guaranteed to be the same across separate Showcase server runs. + rpc GetEnum(EnumRequest) returns (EnumResponse) { + option (google.api.http) = { + get: "/v1beta1/compliance/enum" + }; + } + + // This method is used to verify that clients can round-trip enum values, which is particularly important for unknown enum values over REST. VerifyEnum() + // verifies that its request, which is presumably the response that the client previously got to a GetEnum(), contains the correct data. If so, it responds + // with the same EnumResponse; otherwise, the RPC errors. + // + // This works because the values of enums sent by the server when a known or unknown value is requested will be the same within a single Showcase server run, + // although they are not guaranteed to be the same across separate Showcase server runs. + rpc VerifyEnum(EnumResponse) returns (EnumResponse) { + option (google.api.http) = { + post: "/v1beta1/compliance/enum" + }; + } + +} + +message EnumRequest { + // Whether the client is requesting a new, unknown enum value or a known enum value already declard in this proto file. + bool unknown_enum = 1; +} + +message EnumResponse { + // The original request for a known or unknown enum from the server. + EnumRequest request = 1; + + // The actual enum the server provided. + Continent continent = 2; } message RepeatRequest { diff --git a/src/test/proto/http_rule_parser_testing.proto b/src/test/proto/http_rule_parser_testing.proto new file mode 100644 index 0000000000..d9946e2134 --- /dev/null +++ b/src/test/proto/http_rule_parser_testing.proto @@ -0,0 +1,63 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +syntax = "proto3"; + +package google.http.rule.parser; + +import "google/api/annotations.proto"; +import "google/protobuf/empty.proto"; + +option java_package = "com.google.http.rule.parser"; +option java_multiple_files = true; + +// This service is only meant for unit testing HttpRuleParser +service HttpRuleParserTesting { + + // Test case for putting all fields to query params + rpc QueryParamHappyPathTest(QueryParamRequest) returns (google.protobuf.Empty) { + option (google.api.http) = { + post: "/v1/test" + }; + } + + // Test case for excluding path params from query params + rpc ExcludePathParamsQueryParamTest(QueryParamRequest) returns (google.protobuf.Empty) { + option (google.api.http) = { + post: "/v1/test/{nested_object.name}" + }; + } + + //TODO: Add more test cases once https://github.com/googleapis/gapic-generator-java/issues/1041 is fixed +} + +enum Continent { + CONTINENT_UNSPECIFIED = 0; + AFRICA = 1; + AMERICA = 2; + ANTARCTICA = 3; + AUSTRALIA = 4; + EUROPE = 5; +} + +message QueryParamRequest { + string name = 1; + NestedObject nested_object = 2; +} + +message NestedObject { + string name = 1; + Continent continent = 2; +} +