Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(resnames): ensure determinstic code generation #865

Merged
merged 1 commit into from
Oct 28, 2021
+28 −30
Merged

fix(resnames): ensure determinstic code generation #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 1 addition & 21 deletions ...a/com/google/api/generator/gapic/composer/samplecode/ServiceClientSampleCodeComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
Expand All @@ -71,7 +70,6 @@ public static String composeClassHeaderMethodSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
// Use the first pure unary RPC method's sample code as showcase, if no such method exists, use
// the first method in the service's methods list.
Method method =
Expand Down Expand Up @@ -234,7 +232,6 @@ public static String composeRpcMethodHeaderSampleCode(
List<MethodArgument> arguments,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -281,7 +278,6 @@ public static String composeRpcDefaultMethodHeaderSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -340,7 +336,6 @@ public static String composeLroCallableMethodHeaderSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -453,7 +448,6 @@ public static String composePagedCallableMethodHeaderSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -573,7 +567,6 @@ public static String composeRegularCallableMethodHeaderSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -623,7 +616,6 @@ public static String composeStreamCallableMethodHeaderSampleCode(
TypeNode clientType,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes) {
resourceNames = sortAlphabetically(resourceNames);
VariableExpr clientVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -847,7 +839,7 @@ private static List<Statement> composeStreamServerBodyStatements(
List<Statement> bodyStatements =
bodyExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList());

// For-loop on server stream variable expresion.
// For-loop on server stream variable expression.
// e.g. for (EchoResponse response : stream) {
// // Do something when a response is received.
// }
Expand Down Expand Up @@ -1357,16 +1349,4 @@ private static boolean isProtoEmptyType(TypeNode type) {
return type.reference().pakkage().equals("com.google.protobuf")
&& type.reference().name().equals("Empty");
}

/**
* Returns the same "resource type name" to "resource name" key-value map as given, but sorted in
* alphabetical order. This prevents resource name flip-flopping between executions on the various
* machines used for client library publication.
*/
private static TreeMap<String, ResourceName> sortAlphabetically(
Map<String, ResourceName> unsorted) {
// This simple wrapper is not redundant because it hides implementation details from the
// callers.
return new TreeMap<>(unsorted);
}
}
14 changes: 12 additions & 2 deletions src/main/java/com/google/api/generator/gapic/model/GapicContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -96,7 +97,6 @@ public abstract static class Builder {
public Builder setHelperResourceNames(Set<ResourceName> helperResourceNames) {
return setHelperResourceNames(
helperResourceNames.stream()
.map(r -> r)
.collect(Collectors.toMap(r -> r.resourceTypeString(), r -> r)));
}

Expand All @@ -110,6 +110,16 @@ public Builder setHelperResourceNames(Set<ResourceName> helperResourceNames) {

public abstract Builder setTransport(Transport transport);

public abstract GapicContext build();
abstract ImmutableMap<String, ResourceName> resourceNames();

abstract ImmutableMap<String, ResourceName> helperResourceNames();

abstract GapicContext autoBuild();

public GapicContext build() {
setResourceNames(new TreeMap<>(resourceNames()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with Autovalue, but can you just do this in the GapicContext.resourceNames() getter with @Memoized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally new to AutoValue, and you may know better than I do. I tried to look up a way to achieve what I wanted to do, and this was basically the only way I could think of. I looked into @Memoized, but I think conceptually it's more appropriate to use for caching to solve a problem with an expensive operation. And the doc seems to suggest that I need another "derived method" to use @Memoized (though I am not sure). build() is called only once and new TreeMap<> is a simple thing to do, so I think storing the converted value at the beginning works and is more robust without any magic.

setHelperResourceNames(new TreeMap<>(helperResourceNames()));
return autoBuild();
}
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/protoparser/MethodSignatureParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static List<List<MethodArgument>> parseMethodSignatures(
for (String argumentName : stringSig.split(METHOD_SIGNATURE_DELIMITER)) {
// For resource names, this will be empty.
List<Field> argumentFieldPathAcc = new ArrayList<>();
// There should be more than one type returned only when we encounter a reousrce name.
// There should be more than one type returned only when we encounter a resource name.
Map<TypeNode, Field> argumentTypes =
parseTypeFromArgumentName(
argumentName,
Expand Down
20 changes: 14 additions & 6 deletions test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1533,7 +1533,7 @@ public void encryptTest() throws Exception {
.build();
mockKeyManagementService.addResponse(expectedResponse);

ResourceName name = KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]");
ResourceName name = CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]");
ByteString plaintext = ByteString.EMPTY;

EncryptResponse actualResponse = client.encrypt(name, plaintext);
Expand All @@ -1557,7 +1557,7 @@ public void encryptExceptionTest() throws Exception {
mockKeyManagementService.addException(exception);

try {
ResourceName name = KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]");
ResourceName name = CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]");
ByteString plaintext = ByteString.EMPTY;
client.encrypt(name, plaintext);
Assert.fail("No exception raised");
Expand Down Expand Up @@ -2221,7 +2221,9 @@ public void getIamPolicyTest() throws Exception {

GetIamPolicyRequest request =
GetIamPolicyRequest.newBuilder()
.setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
.setResource(
CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]")
.toString())
.setOptions(GetPolicyOptions.newBuilder().build())
.build();

Expand All @@ -2248,7 +2250,9 @@ public void getIamPolicyExceptionTest() throws Exception {
try {
GetIamPolicyRequest request =
GetIamPolicyRequest.newBuilder()
.setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
.setResource(
CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]")
.toString())
.setOptions(GetPolicyOptions.newBuilder().build())
.build();
client.getIamPolicy(request);
Expand Down Expand Up @@ -2367,7 +2371,9 @@ public void testIamPermissionsTest() throws Exception {

TestIamPermissionsRequest request =
TestIamPermissionsRequest.newBuilder()
.setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
.setResource(
CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]")
.toString())
.addAllPermissions(new ArrayList<String>())
.build();

Expand All @@ -2394,7 +2400,9 @@ public void testIamPermissionsExceptionTest() throws Exception {
try {
TestIamPermissionsRequest request =
TestIamPermissionsRequest.newBuilder()
.setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
.setResource(
CryptoKeyName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]", "[CRYPTO_KEY]")
.toString())
.addAllPermissions(new ArrayList<String>())
.build();
client.testIamPermissions(request);
Expand Down