From 767d1c2ac93d732dd08fe5d76e71d17248e42203 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Mon, 7 Dec 2020 13:54:14 -0800 Subject: [PATCH] [ggj][codgen] fix: add HTTP's additional_bindings to GrpcServiceStub call settings (#591) * fix: fix dep ordering in Bazel dedupe rules * fix: use lowerCamelCase param names in ServiceClient JavaDocs * fix: add HTTP's additional_bindings to GrpcServiceStub call settings --- .../gapic/protoparser/HttpRuleParser.java | 22 +++++++++++----- .../composer/goldens/GrpcTestingStub.golden | 4 +++ .../composer/goldens/TestingClientTest.golden | 6 +++++ .../gapic/protoparser/HttpRuleParserTest.java | 25 ++++++++++++++++--- .../generator/gapic/testdata/testing.proto | 18 +++++++++++++ 5 files changed, 66 insertions(+), 9 deletions(-) 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 086eea4a2e..6998f3c26b 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 @@ -26,9 +26,11 @@ import com.google.protobuf.Descriptors.MethodDescriptor; import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; public class HttpRuleParser { private static final String ASTERISK = "*"; @@ -48,11 +50,20 @@ public static Optional> parseHttpBindings( } // Get pattern. - List bindings = getPatternBindings(httpRule); - if (bindings.isEmpty()) { + Set uniqueBindings = getPatternBindings(httpRule); + if (uniqueBindings.isEmpty()) { return Optional.empty(); } + if (httpRule.getAdditionalBindingsCount() > 0) { + for (HttpRule additionalRule : httpRule.getAdditionalBindingsList()) { + uniqueBindings.addAll(getPatternBindings(additionalRule)); + } + } + + List bindings = new ArrayList<>(uniqueBindings); + Collections.sort(bindings); + // Binding validation. for (String binding : bindings) { // Handle foo.bar cases by descending into the subfields. @@ -77,7 +88,7 @@ public static Optional> parseHttpBindings( return Optional.of(bindings); } - private static List getPatternBindings(HttpRule httpRule) { + private static Set getPatternBindings(HttpRule httpRule) { String pattern = null; // Assign a temp variable to prevent the formatter from removing the import. PatternCase patternCase = httpRule.getPatternCase(); @@ -100,12 +111,11 @@ private static List getPatternBindings(HttpRule httpRule) { case CUSTOM: // Invalid pattern. // Fall through. default: - return Collections.emptyList(); + return Collections.emptySet(); } PathTemplate template = PathTemplate.create(pattern); - List bindings = new ArrayList(template.vars()); - Collections.sort(bindings); + Set bindings = new HashSet(template.vars()); return bindings; } diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcTestingStub.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcTestingStub.golden index 61ea7d1fa9..a178c51a33 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcTestingStub.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/GrpcTestingStub.golden @@ -262,7 +262,11 @@ public class GrpcTestingStub extends TestingStub { @Override public Map extract(VerifyTestRequest request) { ImmutableMap.Builder params = ImmutableMap.builder(); + params.put("answer", String.valueOf(request.getAnswer())); + params.put("foo", String.valueOf(request.getFoo())); params.put("name", String.valueOf(request.getName())); + params.put( + "test_to_verify.name", String.valueOf(request.getTestToVerify().getName())); return params.build(); } }) diff --git a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden index bf955ecc31..f0dc05e18a 100644 --- a/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden +++ b/src/test/java/com/google/api/generator/gapic/composer/goldens/TestingClientTest.golden @@ -465,6 +465,8 @@ public class TestingClientTest { .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setAnswer(ByteString.EMPTY) .addAllAnswers(new ArrayList()) + .setFoo("foo101574") + .setTestToVerify(com.google.showcase.v1beta1.Test.newBuilder().build()) .build(); VerifyTestResponse actualResponse = client.verifyTest(request); @@ -477,6 +479,8 @@ public class TestingClientTest { Assert.assertEquals(request.getName(), actualRequest.getName()); Assert.assertEquals(request.getAnswer(), actualRequest.getAnswer()); Assert.assertEquals(request.getAnswersList(), actualRequest.getAnswersList()); + Assert.assertEquals(request.getFoo(), actualRequest.getFoo()); + Assert.assertEquals(request.getTestToVerify(), actualRequest.getTestToVerify()); Assert.assertTrue( channelProvider.isHeaderSent( ApiClientHeaderProvider.getDefaultApiClientHeaderKey(), @@ -494,6 +498,8 @@ public class TestingClientTest { .setName(TestName.of("[SESSION]", "[SHARD_ID]", "[TEST_ID]").toString()) .setAnswer(ByteString.EMPTY) .addAllAnswers(new ArrayList()) + .setFoo("foo101574") + .setTestToVerify(com.google.showcase.v1beta1.Test.newBuilder().build()) .build(); client.verifyTest(request); Assert.fail("No exception raised"); 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 69a482d610..1aed7b9f92 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 @@ -46,14 +46,33 @@ public void parseHttpAnnotation_basic() { HttpRuleParser.parseHttpBindings(rpcMethod, inputMessage, messages); assertFalse(httpBindingsOpt.isPresent()); - // VerityTest method. - rpcMethod = testingService.getMethods().get(testingService.getMethods().size() - 1); - inputMessage = messages.get("VerifyTestRequest"); + // GetSession method. + rpcMethod = testingService.getMethods().get(1); + inputMessage = messages.get("GetSessionRequest"); httpBindingsOpt = HttpRuleParser.parseHttpBindings(rpcMethod, inputMessage, messages); assertTrue(httpBindingsOpt.isPresent()); assertThat(httpBindingsOpt.get()).containsExactly("name"); } + @Test + public void parseHttpAnnotation_multipleBindings() { + FileDescriptor testingFileDescriptor = TestingOuterClass.getDescriptor(); + ServiceDescriptor testingService = testingFileDescriptor.getServices().get(0); + assertEquals("Testing", testingService.getName()); + + Map messages = Parser.parseMessages(testingFileDescriptor); + + // VerityTest method. + MethodDescriptor rpcMethod = + testingService.getMethods().get(testingService.getMethods().size() - 1); + Message inputMessage = messages.get("VerifyTestRequest"); + Optional> httpBindingsOpt = + HttpRuleParser.parseHttpBindings(rpcMethod, inputMessage, messages); + assertTrue(httpBindingsOpt.isPresent()); + assertThat(httpBindingsOpt.get()) + .containsExactly("answer", "foo", "name", "test_to_verify.name"); + } + @Test public void parseHttpAnnotation_missingFieldFromMessage() { FileDescriptor testingFileDescriptor = TestingOuterClass.getDescriptor(); diff --git a/src/test/java/com/google/api/generator/gapic/testdata/testing.proto b/src/test/java/com/google/api/generator/gapic/testdata/testing.proto index 2fe4ad9f66..28432bffaa 100644 --- a/src/test/java/com/google/api/generator/gapic/testdata/testing.proto +++ b/src/test/java/com/google/api/generator/gapic/testdata/testing.proto @@ -107,6 +107,19 @@ service Testing { rpc VerifyTest(VerifyTestRequest) returns (VerifyTestResponse) { option (google.api.http) = { post: "/v1beta1/{name=sessions/*/tests/*}:check" + body: "*" + additional_bindings { + post: "/v1beta1/{foo=sessions/*/tests/*}:check" + body: "*" + } + additional_bindings { + post: "/v1beta1/{answer=sessions/*/tests/*}:check" + body: "*" + } + additional_bindings { + post: "/v1beta1/{test_to_verify.name=sessions/*/tests/*}:check" + body: "*" + } }; } } @@ -392,6 +405,11 @@ message VerifyTestRequest { // The answers from the test if multiple are to be checked repeated bytes answers = 3; + + // Test fields for HTTP annotations. + string foo = 4; + + Test test_to_verify = 5; } message VerifyTestResponse {