Skip to content

fix(resnames): Use anon resname classes when only wildcards are present #761

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

Merged
merged 2 commits into from
Jun 10, 2021
Merged

fix(resnames): Use anon resname classes when only wildcards are present #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2403572
fix(resnames): Use anon resname classes when no non-only ds are present
miraleung Jun 10, 2021
a9d9d95
Merge remote-tracking branch 'origin/fix/proto_name_conflicts' into f…
miraleung Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/main/java/com/google/api/generator/engine/ast/AssignmentExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,10 @@ public AssignmentExpr build() {
if (rhsType != TypeNode.NULL && !lhsType.isSupertypeOrEquals(rhsType)) {
throw new TypeMismatchException(
String.format(
"LHS type %s must be a supertype of the RHS type %s",
lhsType.reference().name(), rhsType.reference().name()));
"LHS type %s of variable %s must be a supertype of the RHS type %s",
lhsType.reference().name(),
assignmentExpr.variableExpr().variable().identifier(),
rhsType.reference().name()));
}
}

Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/gapic/composer/defaultvalue/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ java_library(
"//src/main/java/com/google/api/generator/gapic/composer/resourcename",
"//src/main/java/com/google/api/generator/gapic/model",
"//src/main/java/com/google/api/generator/gapic/utils",
"@com_google_api_api_common",
"@com_google_googleapis//google/longrunning:longrunning_java_proto",
"@com_google_guava_guava//jar",
"@com_google_protobuf//java/core",
Expand Down
117 changes: 112 additions & 5 deletions src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,16 @@

package com.google.api.generator.gapic.composer.defaultvalue;

import com.google.api.generator.engine.ast.AnonymousClassExpr;
import com.google.api.generator.engine.ast.AssignmentExpr;
import com.google.api.generator.engine.ast.ConcreteReference;
import com.google.api.generator.engine.ast.Expr;
import com.google.api.generator.engine.ast.ExprStatement;
import com.google.api.generator.engine.ast.MethodDefinition;
import com.google.api.generator.engine.ast.MethodInvocationExpr;
import com.google.api.generator.engine.ast.NewObjectExpr;
import com.google.api.generator.engine.ast.PrimitiveValue;
import com.google.api.generator.engine.ast.ScopeNode;
import com.google.api.generator.engine.ast.StringObjectValue;
import com.google.api.generator.engine.ast.TypeNode;
import com.google.api.generator.engine.ast.ValueExpr;
Expand All @@ -31,6 +36,7 @@
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.utils.JavaStyle;
import com.google.api.generator.gapic.utils.ResourceNameConstants;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.longrunning.Operation;
import com.google.protobuf.Any;
Expand Down Expand Up @@ -157,6 +163,16 @@ static Expr createDefaultValue(Field f, boolean useExplicitInitTypeInGenerics) {

public static Expr createDefaultValue(
ResourceName resourceName, List<ResourceName> resnames, String fieldOrMessageName) {
return createDefaultValueResourceHelper(resourceName, resnames, fieldOrMessageName, true);
}

@VisibleForTesting
static Expr createDefaultValueResourceHelper(
ResourceName resourceName,
List<ResourceName> resnames,
String fieldOrMessageName,
boolean allowAnonResourceNameClass) {

boolean hasOnePattern = resourceName.patterns().size() == 1;
if (resourceName.isOnlyWildcard()) {
List<ResourceName> unexaminedResnames = new ArrayList<>(resnames);
Expand All @@ -170,9 +186,11 @@ public static Expr createDefaultValue(
}

if (unexaminedResnames.isEmpty()) {
return ValueExpr.withValue(
StringObjectValue.withValue(
String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode())));
return allowAnonResourceNameClass
? createAnonymousResourceNameClass(fieldOrMessageName)
: ValueExpr.withValue(
StringObjectValue.withValue(
String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode())));
}
}

Expand Down Expand Up @@ -247,10 +265,11 @@ public static Expr createSimpleMessageBuilderExpr(
if (field.hasResourceReference()
&& resourceNames.get(field.resourceReference().resourceTypeString()) != null) {
defaultExpr =
createDefaultValue(
createDefaultValueResourceHelper(
resourceNames.get(field.resourceReference().resourceTypeString()),
resourceNames.values().stream().collect(Collectors.toList()),
message.name());
message.name(),
/* allowAnonResourceNameClass = */ false);
defaultExpr =
MethodInvocationExpr.builder()
.setExprReferenceExpr(defaultExpr)
Expand Down Expand Up @@ -345,4 +364,92 @@ public static Expr createSimplePagedResponse(
.setReturnType(responseType)
.build();
}

@VisibleForTesting
static AnonymousClassExpr createAnonymousResourceNameClass(String fieldOrMessageName) {
TypeNode stringMapType =
TypeNode.withReference(
ConcreteReference.builder()
.setClazz(Map.class)
.setGenerics(
Arrays.asList(
ConcreteReference.withClazz(String.class),
ConcreteReference.withClazz(String.class)))
.build());

// Method code:
// @Override
// public Map<String, String> getFieldValuesMap() {
// Map<String, String> fieldValuesMap = new HashMap<>();
// fieldValuesMap.put("resource", "resource-12345");
// return fieldValuesMap;
// }
VariableExpr fieldValuesMapVarExpr =
VariableExpr.withVariable(
Variable.builder().setType(stringMapType).setName("fieldValuesMap").build());
StringObjectValue fieldOrMessageStringValue =
StringObjectValue.withValue(
String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode()));

List<Expr> bodyExprs =
Arrays.asList(
AssignmentExpr.builder()
.setVariableExpr(fieldValuesMapVarExpr.toBuilder().setIsDecl(true).build())
.setValueExpr(
NewObjectExpr.builder()
.setType(TypeNode.withReference(ConcreteReference.withClazz(HashMap.class)))
.setIsGeneric(true)
.build())
.build(),
MethodInvocationExpr.builder()
.setExprReferenceExpr(fieldValuesMapVarExpr)
.setMethodName("put")
.setArguments(
ValueExpr.withValue(StringObjectValue.withValue(fieldOrMessageName)),
ValueExpr.withValue(fieldOrMessageStringValue))
.build());

MethodDefinition getFieldValuesMapMethod =
MethodDefinition.builder()
.setIsOverride(true)
.setScope(ScopeNode.PUBLIC)
.setReturnType(stringMapType)
.setName("getFieldValuesMap")
.setBody(
bodyExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()))
.setReturnExpr(fieldValuesMapVarExpr)
.build();

// Method code:
// @Override
// public String getFieldValue(String fieldName) {
// return getFieldValuesMap().get(fieldName);
// }
VariableExpr fieldNameVarExpr =
VariableExpr.withVariable(
Variable.builder().setType(TypeNode.STRING).setName("fieldName").build());
MethodDefinition getFieldValueMethod =
MethodDefinition.builder()
.setIsOverride(true)
.setScope(ScopeNode.PUBLIC)
.setReturnType(TypeNode.STRING)
.setName("getFieldValue")
.setArguments(fieldNameVarExpr.toBuilder().setIsDecl(true).build())
.setReturnExpr(
MethodInvocationExpr.builder()
.setExprReferenceExpr(
MethodInvocationExpr.builder().setMethodName("getFieldValuesMap").build())
.setMethodName("get")
.setArguments(fieldNameVarExpr)
.setReturnType(TypeNode.STRING)
.build())
.build();

return AnonymousClassExpr.builder()
.setType(
TypeNode.withReference(
ConcreteReference.withClazz(com.google.api.resourcenames.ResourceName.class)))
.setMethods(Arrays.asList(getFieldValuesMapMethod, getFieldValueMethod))
.build();
}
}
70 changes: 66 additions & 4 deletions ...t/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.protoparser.Parser;
import com.google.api.generator.testutils.LineFormatter;
import com.google.protobuf.ByteString;
import com.google.protobuf.Descriptors.FileDescriptor;
import com.google.showcase.v1beta1.EchoOuterClass;
Expand Down Expand Up @@ -222,8 +223,8 @@ public void defaultValue_wildcardResourceNameWithOnlyDeletedTopic() {
}

@Test
public void defaultValue_resourceNameWithOnlyWildcards() {
// Edge case that should never happen in practice.
public void defaultValue_resourceNameWithOnlyWildcards_valueOnly() {
// Edge case that occurs in GCS.
// Wildcard, but the resource names map has only other names that contain only the deleted-topic
// constant.
FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor();
Expand All @@ -233,13 +234,50 @@ public void defaultValue_resourceNameWithOnlyWildcards() {
typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything");
String fallbackField = "foobar";
Expr expr =
DefaultValueComposer.createDefaultValue(
resourceName, Collections.emptyList(), fallbackField);
DefaultValueComposer.createDefaultValueResourceHelper(
resourceName,
Collections.emptyList(),
fallbackField,
/* allowAnonResourceNameClass = */ false);
expr.accept(writerVisitor);
assertEquals(
String.format("\"%s%s\"", fallbackField, fallbackField.hashCode()), writerVisitor.write());
}

@Test
public void defaultValue_resourceNameWithOnlyWildcards_allowAnonResourceNameClass() {
// Edge case that occurs in GCS.
// Wildcard, but the resource names map has only other names that contain only the deleted-topic
// constant.
FileDescriptor lockerServiceFileDescriptor = LockerProto.getDescriptor();
Map<String, ResourceName> typeStringsToResourceNames =
Parser.parseResourceNames(lockerServiceFileDescriptor);
ResourceName resourceName =
typeStringsToResourceNames.get("cloudresourcemanager.googleapis.com/Anything");
String fallbackField = "foobar";
Expr expr =
DefaultValueComposer.createDefaultValue(
resourceName, Collections.emptyList(), fallbackField);
expr.accept(writerVisitor);
String expected =
LineFormatter.lines(
"new ResourceName() {\n",
"@Override\n",
"public Map<String, String> getFieldValuesMap() {\n",
"Map<String, String> fieldValuesMap = new HashMap<>();\n",
"fieldValuesMap.put(\"foobar\", \"foobar-1268878963\");\n",
"return fieldValuesMap;\n",
"}\n",
"\n",
"@Override\n",
"public String getFieldValue(String fieldName) {\n",
"return getFieldValuesMap().get(fieldName);\n",
"}\n",
"\n",
"}");
assertEquals(expected, writerVisitor.write());
}

@Test
public void createSimpleMessage_basicPrimitivesOnly() {
FileDescriptor echoFileDescriptor = EchoOuterClass.getDescriptor();
Expand Down Expand Up @@ -307,4 +345,28 @@ public void createSimpleMessage_onlyOneofs() {
expr.accept(writerVisitor);
assertEquals("WaitRequest.newBuilder().build()", writerVisitor.write());
}

@Test
public void createAnonymousResourceNameClass() {
Expr expr = DefaultValueComposer.createAnonymousResourceNameClass("resource");
expr.accept(writerVisitor);
String expected =
LineFormatter.lines(
"new ResourceName() {\n",
"@Override\n",
"public Map<String, String> getFieldValuesMap() {\n",
"Map<String, String> fieldValuesMap = new HashMap<>();\n",
"fieldValuesMap.put(\"resource\", \"resource-341064690\");\n",
"return fieldValuesMap;\n",
"}\n",
"\n",
"@Override\n",
"public String getFieldValue(String fieldName) {\n",
"return getFieldValuesMap().get(fieldName);\n",
"}\n",
"\n",
"}");

assertEquals(expected, writerVisitor.write());
}
}