From 740b11755b5a44bddc0b1c5c0912de879d97ece4 Mon Sep 17 00:00:00 2001 From: Emily Wang Date: Thu, 3 Nov 2022 12:56:58 -0400 Subject: [PATCH] Revert "fix: Get numeric value for Enum fields if it is configured as query param or path param (#1042)" This reverts commit 4b5eadb343c90c988e4bcec24e17dc17a6ee5964. --- .../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, 18 insertions(+), 674 deletions(-) delete mode 100644 src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java delete 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 67e003f50f..fa4d71498c 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,7 +59,6 @@ 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; @@ -75,7 +74,6 @@ import java.util.stream.Collectors; public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer { - private static final HttpJsonServiceStubClassComposer INSTANCE = new HttpJsonServiceStubClassComposer(); @@ -942,11 +940,9 @@ private Expr createFieldsExtractorClassInstance( for (int i = 0; i < descendantFields.length; i++) { String currFieldName = descendantFields[i]; String bindingFieldMethodName = - getBindingFieldMethodName( - httpBindingFieldName, - descendantFields.length, - i, - JavaStyle.toUpperCamelCase(currFieldName)); + (i < descendantFields.length - 1 || !httpBindingFieldName.isRepeated()) + ? String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName)) + : String.format("get%sList", JavaStyle.toUpperCamelCase(currFieldName)); requestFieldGetterExprBuilder = requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName); @@ -1001,7 +997,6 @@ 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(); @@ -1028,20 +1023,6 @@ 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 696683cefe..e6ecea5c94 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,54 +36,21 @@ 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(); - // An object that contains all info of the leaf level field - @Nullable - public abstract Field field(); + public abstract boolean isOptional(); - public boolean isOptional() { - return field() != null && field().isProto3Optional(); - } - - public boolean isRepeated() { - return field() != null && field().isRepeated(); - } - - public boolean isEnum() { - return field() != null && field().isEnum(); - } + public abstract boolean isRepeated(); @Nullable public abstract String 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(); - } + public static HttpBinding create( + String name, boolean isOptional, boolean isRepeated, String valuePattern) { + return new AutoValue_HttpBindings_HttpBinding( + name, JavaStyle.toLowerCamelCase(name), isOptional, isRepeated, valuePattern); } // 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 87bcd76abb..bead11e082 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,11 +82,6 @@ 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; @@ -138,9 +133,8 @@ 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(httpBindingBuilder.setValuePattern(patternSampleValue).build()); + httpBindings.add(HttpBinding.create(paramName, false, false, patternSampleValue)); continue; } Message nestedMessage = inputMessage; @@ -162,7 +156,8 @@ private static Set validateAndConstructHttpBindings( } Field field = nestedMessage.fieldMap().get(subFieldName); httpBindings.add( - httpBindingBuilder.setValuePattern(patternSampleValue).setField(field).build()); + HttpBinding.create( + paramName, field.isProto3Optional(), field.isRepeated(), patternSampleValue)); } } } 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 243ef54ce2..e8eccd9eec 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,35 +14,23 @@ 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 = composer.generate(context, echoProtoService); + GapicClass clazz = + HttpJsonServiceStubClassComposer.instance().generate(context, echoProtoService); JavaWriterVisitor visitor = new JavaWriterVisitor(); clazz.classDefinition().accept(visitor); @@ -51,49 +39,4 @@ 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 07e6b442e2..9f72651e9c 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,102 +560,4 @@ 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 3730206623..afa6402beb 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,16 +86,6 @@ 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(); } @@ -225,16 +215,6 @@ 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 acd321473f..f124efd10d 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,8 +19,6 @@ 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; @@ -77,8 +75,6 @@ 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() { @@ -110,16 +106,6 @@ 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() @@ -203,8 +189,6 @@ public class ComplianceStubSettings extends StubSettings repeatDataPathResourceSettings = settingsBuilder.repeatDataPathResourceSettings().build(); repeatDataPathTrailingResourceSettings = settingsBuilder.repeatDataPathTrailingResourceSettings().build(); - getEnumSettings = settingsBuilder.getEnumSettings().build(); - verifyEnumSettings = settingsBuilder.verifyEnumSettings().build(); } /** Builder for ComplianceStubSettings. */ @@ -220,8 +204,6 @@ 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; @@ -255,8 +237,6 @@ public class ComplianceStubSettings extends StubSettings repeatDataSimplePathSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); repeatDataPathResourceSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); repeatDataPathTrailingResourceSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); - getEnumSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); - verifyEnumSettings = UnaryCallSettings.newUnaryCallSettingsBuilder(); unaryMethodSettingsBuilders = ImmutableList.>of( @@ -265,9 +245,7 @@ public class ComplianceStubSettings extends StubSettings repeatDataQuerySettings, repeatDataSimplePathSettings, repeatDataPathResourceSettings, - repeatDataPathTrailingResourceSettings, - getEnumSettings, - verifyEnumSettings); + repeatDataPathTrailingResourceSettings); initDefaults(this); } @@ -281,8 +259,6 @@ public class ComplianceStubSettings extends StubSettings repeatDataPathResourceSettings = settings.repeatDataPathResourceSettings.toBuilder(); repeatDataPathTrailingResourceSettings = settings.repeatDataPathTrailingResourceSettings.toBuilder(); - getEnumSettings = settings.getEnumSettings.toBuilder(); - verifyEnumSettings = settings.verifyEnumSettings.toBuilder(); unaryMethodSettingsBuilders = ImmutableList.>of( @@ -291,9 +267,7 @@ public class ComplianceStubSettings extends StubSettings repeatDataQuerySettings, repeatDataSimplePathSettings, repeatDataPathResourceSettings, - repeatDataPathTrailingResourceSettings, - getEnumSettings, - verifyEnumSettings); + repeatDataPathTrailingResourceSettings); } private static Builder createDefault() { @@ -340,16 +314,6 @@ 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; } @@ -400,16 +364,6 @@ 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 bf282ba988..f2836caf50 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,8 +13,6 @@ 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; @@ -170,7 +168,7 @@ public class HttpJsonComplianceStub extends ComplianceStub { serializer.putPathParam( fields, "info.fInt32", request.getInfo().getFInt32()); serializer.putPathParam( - fields, "info.fKingdom", request.getInfo().getFKingdomValue()); + fields, "info.fKingdom", request.getInfo().getFKingdom()); serializer.putPathParam( fields, "info.fString", request.getInfo().getFString()); return fields; @@ -287,77 +285,12 @@ 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; @@ -431,16 +364,6 @@ 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( @@ -468,12 +391,6 @@ 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()); @@ -488,8 +405,6 @@ public class HttpJsonComplianceStub extends ComplianceStub { methodDescriptors.add(repeatDataSimplePathMethodDescriptor); methodDescriptors.add(repeatDataPathResourceMethodDescriptor); methodDescriptors.add(repeatDataPathTrailingResourceMethodDescriptor); - methodDescriptors.add(getEnumMethodDescriptor); - methodDescriptors.add(verifyEnumMethodDescriptor); return methodDescriptors; } @@ -523,16 +438,6 @@ 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 deleted file mode 100644 index f8713e0564..0000000000 --- a/src/test/java/com/google/api/generator/gapic/model/HttpBindingsTest.java +++ /dev/null @@ -1,99 +0,0 @@ -// 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 cecee806f9..93fe4b8135 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.builder().setName("table").build())) + .setPathParameters(ImmutableSet.of(HttpBinding.create("table", true, false, ""))) .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 5269e1d982..31fc531555 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,16 +22,12 @@ 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 { @@ -95,83 +91,4 @@ 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 00f71a9c05..4467a387ab 100644 --- a/src/test/proto/compliance.proto +++ b/src/test/proto/compliance.proto @@ -80,44 +80,6 @@ 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 deleted file mode 100644 index d9946e2134..0000000000 --- a/src/test/proto/http_rule_parser_testing.proto +++ /dev/null @@ -1,63 +0,0 @@ -// 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; -} -