diff --git a/src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java b/src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java index 3092ce1165..4930e52d2e 100644 --- a/src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java +++ b/src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java @@ -159,7 +159,12 @@ protected MethodDefinition createStartStaticServerMethod( List mockServiceVarExprs = new ArrayList<>(); varInitExprs.add(serviceToVarInitExprFn.apply(service)); mockServiceVarExprs.add(serviceToVarExprFn.apply(service)); - for (Service mixinService : context.mixinServices()) { + // Careful: Java 8 and 11 make different ordering choices if this set is not explicitly sorted. + // Context: https://github.com/googleapis/gapic-generator-java/pull/750 + for (Service mixinService : + context.mixinServices().stream() + .sorted((s1, s2) -> s2.name().compareTo(s1.name())) + .collect(Collectors.toList())) { varInitExprs.add(serviceToVarInitExprFn.apply(mixinService)); mockServiceVarExprs.add(serviceToVarExprFn.apply(mixinService)); } @@ -201,12 +206,15 @@ protected MethodDefinition createStartStaticServerMethod( varInitExprs.add(initServiceHelperExpr); varInitExprs.add(startServiceHelperExpr); + List body = new ArrayList<>(); + return MethodDefinition.builder() .setAnnotations(Arrays.asList(AnnotationNode.withType(FIXED_TYPESTORE.get("BeforeClass")))) .setScope(ScopeNode.PUBLIC) .setIsStatic(true) .setReturnType(TypeNode.VOID) .setName("startStaticServer") + .setBody(body) .setBody( varInitExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList())) .build(); diff --git a/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java b/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java index a088e8ae46..051aff1b83 100644 --- a/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java +++ b/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java @@ -270,9 +270,15 @@ public static List parseServices( } } + // Sort potential mixin services alphabetically. + List orderedBlockedCodegenMixinApis = + blockedCodegenMixinApis.stream() + .sorted((s1, s2) -> s2.name().compareTo(s1.name())) + .collect(Collectors.toList()); + Set apiDefinedRpcs = new HashSet<>(); for (Service service : services) { - if (blockedCodegenMixinApis.contains(service)) { + if (orderedBlockedCodegenMixinApis.contains(service)) { continue; } apiDefinedRpcs.addAll( @@ -285,7 +291,7 @@ public static List parseServices( Service originalService = services.get(i); List updatedOriginalServiceMethods = new ArrayList<>(originalService.methods()); // If mixin APIs are present, add the methods to all other services. - for (Service mixinService : blockedCodegenMixinApis) { + for (Service mixinService : orderedBlockedCodegenMixinApis) { final String mixinServiceFullName = serviceFullNameFn.apply(mixinService); if (!mixedInApis.contains(mixinServiceFullName)) { continue; @@ -327,6 +333,11 @@ public static List parseServices( m.toBuilder() .setMixedInApiName(serviceFullNameFn.apply(mixinService)) .build())); + // Sort by method name, to ensure a deterministic method ordering (for tests). + updatedMixinMethods = + updatedMixinMethods.stream() + .sorted((m1, m2) -> m2.name().compareTo(m1.name())) + .collect(Collectors.toList()); outputMixinServiceSet.add( mixinService.toBuilder().setMethods(updatedMixinMethods).build()); } @@ -343,7 +354,10 @@ public static List parseServices( } // Use a list to ensure ordering for deterministic tests. - outputMixinServices.addAll(outputMixinServiceSet); + outputMixinServices.addAll( + outputMixinServiceSet.stream() + .sorted((s1, s2) -> s2.name().compareTo(s1.name())) + .collect(Collectors.toSet())); return services; } diff --git a/test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml b/test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml index 2828ca13f0..7ce51316c7 100644 --- a/test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml +++ b/test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml @@ -16,12 +16,14 @@ documentation: Manages keys and performs cryptographic operations in a central cloud service, for direct use by other cloud resources and applications. rules: + # This RPC shouldn't appear in the proto, since it's been clobered by KMS's definition in the proto. - selector: google.iam.v1.IAMPolicy.GetIamPolicy description: |- Gets the access control policy for a resource. Returns an empty policy if the resource exists and does not have a policy set. - # This RPC shouldn't appear in the proto even though the documentation field is set. + # This RPC shouldn't appear in the proto, since it's not in the HTTP rules list below, + # even though the documentation field is set. - selector: google.iam.v1.IAMPolicy.SetIamPolicy description: |- Sets the access control policy on the specified resource. Replaces diff --git a/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java b/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java index 5702c6dfec..0b603417ae 100644 --- a/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java +++ b/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java @@ -76,12 +76,12 @@ public class KeyManagementServiceClientTest { @BeforeClass public static void startStaticServer() { mockKeyManagementService = new MockKeyManagementService(); - mockIAMPolicy = new MockIAMPolicy(); mockLocations = new MockLocations(); + mockIAMPolicy = new MockIAMPolicy(); mockServiceHelper = new MockServiceHelper( UUID.randomUUID().toString(), - Arrays.asList(mockKeyManagementService, mockIAMPolicy, mockLocations)); + Arrays.asList(mockKeyManagementService, mockLocations, mockIAMPolicy)); mockServiceHelper.start(); } diff --git a/test/integration/goldens/kms/com/google/iam/v1/MockIAMPolicyImpl.java b/test/integration/goldens/kms/com/google/iam/v1/MockIAMPolicyImpl.java index 6245f271c9..d1c753da0d 100644 --- a/test/integration/goldens/kms/com/google/iam/v1/MockIAMPolicyImpl.java +++ b/test/integration/goldens/kms/com/google/iam/v1/MockIAMPolicyImpl.java @@ -59,11 +59,13 @@ public void reset() { } @Override - public void getIamPolicy(GetIamPolicyRequest request, StreamObserver responseObserver) { + public void testIamPermissions( + TestIamPermissionsRequest request, + StreamObserver responseObserver) { Object response = responses.poll(); - if (response instanceof Policy) { + if (response instanceof TestIamPermissionsResponse) { requests.add(request); - responseObserver.onNext(((Policy) response)); + responseObserver.onNext(((TestIamPermissionsResponse) response)); responseObserver.onCompleted(); } else if (response instanceof Exception) { responseObserver.onError(((Exception) response)); @@ -71,21 +73,19 @@ public void getIamPolicy(GetIamPolicyRequest request, StreamObserver res responseObserver.onError( new IllegalArgumentException( String.format( - "Unrecognized response type %s for method GetIamPolicy, expected %s or %s", + "Unrecognized response type %s for method TestIamPermissions, expected %s or %s", response == null ? "null" : response.getClass().getName(), - Policy.class.getName(), + TestIamPermissionsResponse.class.getName(), Exception.class.getName()))); } } @Override - public void testIamPermissions( - TestIamPermissionsRequest request, - StreamObserver responseObserver) { + public void getIamPolicy(GetIamPolicyRequest request, StreamObserver responseObserver) { Object response = responses.poll(); - if (response instanceof TestIamPermissionsResponse) { + if (response instanceof Policy) { requests.add(request); - responseObserver.onNext(((TestIamPermissionsResponse) response)); + responseObserver.onNext(((Policy) response)); responseObserver.onCompleted(); } else if (response instanceof Exception) { responseObserver.onError(((Exception) response)); @@ -93,9 +93,9 @@ public void testIamPermissions( responseObserver.onError( new IllegalArgumentException( String.format( - "Unrecognized response type %s for method TestIamPermissions, expected %s or %s", + "Unrecognized response type %s for method GetIamPolicy, expected %s or %s", response == null ? "null" : response.getClass().getName(), - TestIamPermissionsResponse.class.getName(), + Policy.class.getName(), Exception.class.getName()))); } } diff --git a/test/integration/goldens/pubsub/com/google/cloud/pubsub/v1/MockIAMPolicyImpl.java b/test/integration/goldens/pubsub/com/google/cloud/pubsub/v1/MockIAMPolicyImpl.java index 836144b7ee..e4528ae534 100644 --- a/test/integration/goldens/pubsub/com/google/cloud/pubsub/v1/MockIAMPolicyImpl.java +++ b/test/integration/goldens/pubsub/com/google/cloud/pubsub/v1/MockIAMPolicyImpl.java @@ -64,11 +64,13 @@ public void reset() { } @Override - public void setIamPolicy(SetIamPolicyRequest request, StreamObserver responseObserver) { + public void testIamPermissions( + TestIamPermissionsRequest request, + StreamObserver responseObserver) { Object response = responses.poll(); - if (response instanceof Policy) { + if (response instanceof TestIamPermissionsResponse) { requests.add(request); - responseObserver.onNext(((Policy) response)); + responseObserver.onNext(((TestIamPermissionsResponse) response)); responseObserver.onCompleted(); } else if (response instanceof Exception) { responseObserver.onError(((Exception) response)); @@ -76,15 +78,15 @@ public void setIamPolicy(SetIamPolicyRequest request, StreamObserver res responseObserver.onError( new IllegalArgumentException( String.format( - "Unrecognized response type %s for method SetIamPolicy, expected %s or %s", + "Unrecognized response type %s for method TestIamPermissions, expected %s or %s", response == null ? "null" : response.getClass().getName(), - Policy.class.getName(), + TestIamPermissionsResponse.class.getName(), Exception.class.getName()))); } } @Override - public void getIamPolicy(GetIamPolicyRequest request, StreamObserver responseObserver) { + public void setIamPolicy(SetIamPolicyRequest request, StreamObserver responseObserver) { Object response = responses.poll(); if (response instanceof Policy) { requests.add(request); @@ -96,7 +98,7 @@ public void getIamPolicy(GetIamPolicyRequest request, StreamObserver res responseObserver.onError( new IllegalArgumentException( String.format( - "Unrecognized response type %s for method GetIamPolicy, expected %s or %s", + "Unrecognized response type %s for method SetIamPolicy, expected %s or %s", response == null ? "null" : response.getClass().getName(), Policy.class.getName(), Exception.class.getName()))); @@ -104,13 +106,11 @@ public void getIamPolicy(GetIamPolicyRequest request, StreamObserver res } @Override - public void testIamPermissions( - TestIamPermissionsRequest request, - StreamObserver responseObserver) { + public void getIamPolicy(GetIamPolicyRequest request, StreamObserver responseObserver) { Object response = responses.poll(); - if (response instanceof TestIamPermissionsResponse) { + if (response instanceof Policy) { requests.add(request); - responseObserver.onNext(((TestIamPermissionsResponse) response)); + responseObserver.onNext(((Policy) response)); responseObserver.onCompleted(); } else if (response instanceof Exception) { responseObserver.onError(((Exception) response)); @@ -118,9 +118,9 @@ public void testIamPermissions( responseObserver.onError( new IllegalArgumentException( String.format( - "Unrecognized response type %s for method TestIamPermissions, expected %s or %s", + "Unrecognized response type %s for method GetIamPolicy, expected %s or %s", response == null ? "null" : response.getClass().getName(), - TestIamPermissionsResponse.class.getName(), + Policy.class.getName(), Exception.class.getName()))); } }