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

fix: Multiple REST transport related fixes #997

Merged
merged 5 commits into from
Jun 3, 2022
Merged

fix: Multiple REST transport related fixes #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9937cd
fix: Multiple resource-related fixes
vam-google May 25, 2022
86e10bb
4) Fix multipattern `of%Name` method resoluiton to make sure that the…
vam-google May 25, 2022
5cd3b45
5) Fix anonymous resource name class construction (add proper toStrin…
vam-google May 26, 2022
84672b7
address PR feedback
vam-google May 26, 2022
3b58122
add more tests, treat methods with no bindings as not supported for …
vam-google Jun 2, 2022
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
4 changes: 2 additions & 2 deletions scripts/diff_gen_and_golden.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ find src -type f -name 'PlaceholderFile.java' -delete
find src -type d -empty -delete
# This will not print diff_output to the console unless `--test_output=all` option
# is enabled, it only emits the comparison results to the test.log.
diff -ru src test/integration/goldens/${API_NAME}/src
diff -ru samples test/integration/goldens/${API_NAME}/samples
diff -ru test/integration/goldens/${API_NAME}/src src
diff -ru test/integration/goldens/${API_NAME}/samples samples
23 changes: 18 additions & 5 deletions ...om/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ protected GapicClass generate(String className, GapicContext context, Service se
return GapicClass.create(kind, classDef);
}

protected boolean isSupportedMethod(Method method) {
return true;
}

private List<AnnotationNode> createClassAnnotations() {
return Arrays.asList(
AnnotationNode.builder()
Expand Down Expand Up @@ -225,6 +229,9 @@ private List<MethodDefinition> createTestMethods(
Map<String, Message> messageTypes = context.messages();
List<MethodDefinition> javaMethods = new ArrayList<>();
for (Method method : service.methods()) {
if (!isSupportedMethod(method)) {
continue;
}
Service matchingService = service;
if (method.isMixin()) {
int dotIndex = method.mixedInApiName().lastIndexOf(".");
Expand Down Expand Up @@ -388,7 +395,8 @@ private MethodDefinition createRpcTestMethod(
DefaultValueComposer.createSimpleMessageBuilderValue(
messageTypes.get(methodOutputType.reference().fullName()),
resourceNames,
messageTypes);
messageTypes,
method.httpBindings());
} else {
// Wrap this in a field so we don't have to split the helper into lots of different methods,
// or duplicate it for VariableExpr.
Expand Down Expand Up @@ -461,9 +469,14 @@ private MethodDefinition createRpcTestMethod(
if (getTransportContext().useValuePatterns() && method.hasHttpBindings()) {
pathParamValuePatterns = method.httpBindings().getPathParametersValuePatterns();
}

Expr valExpr =
DefaultValueComposer.createSimpleMessageBuilderValue(
requestMessage, resourceNames, messageTypes, pathParamValuePatterns);
requestMessage,
resourceNames,
messageTypes,
pathParamValuePatterns,
method.httpBindings());
methodExprs.add(
AssignmentExpr.builder()
.setVariableExpr(requestVarExpr.toBuilder().setIsDecl(true).build())
Expand All @@ -482,7 +495,7 @@ private MethodDefinition createRpcTestMethod(
argExprs.add(varExpr);
Expr valExpr =
DefaultValueComposer.createMethodArgValue(
methodArg, resourceNames, messageTypes, valuePatterns);
methodArg, resourceNames, messageTypes, valuePatterns, method.httpBindings());
methodExprs.add(
AssignmentExpr.builder()
.setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
Expand Down Expand Up @@ -770,7 +783,7 @@ protected List<Statement> createRpcExceptionTestStatements(
}
Expr valExpr =
DefaultValueComposer.createSimpleMessageBuilderValue(
requestMessage, resourceNames, messageTypes, valuePatterns);
requestMessage, resourceNames, messageTypes, valuePatterns, method.httpBindings());
tryBodyExprs.add(
AssignmentExpr.builder()
.setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
Expand All @@ -789,7 +802,7 @@ protected List<Statement> createRpcExceptionTestStatements(
argVarExprs.add(varExpr);
Expr valExpr =
DefaultValueComposer.createMethodArgValue(
methodArg, resourceNames, messageTypes, valuePatterns);
methodArg, resourceNames, messageTypes, valuePatterns, method.httpBindings());
tryBodyExprs.add(
AssignmentExpr.builder()
.setVariableExpr(varExpr.toBuilder().setIsDecl(true).build())
Expand Down
16 changes: 15 additions & 1 deletion ...google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ public GapicClass generate(GapicContext context, Service service) {
return GapicClass.create(kind, classDef);
}

protected boolean isSupportedMethod(Method method) {
return true;
}

protected abstract Statement createMethodDescriptorVariableDecl(
Service service,
Method protoMethod,
Expand Down Expand Up @@ -299,6 +303,7 @@ protected List<Statement> createMethodDescriptorVariableDecls(
Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs,
Map<String, Message> messageTypes) {
return service.methods().stream()
.filter(this::isSupportedMethod)
.map(
m ->
createMethodDescriptorVariableDecl(
Expand All @@ -323,6 +328,7 @@ private static List<Statement> createClassMemberFieldDeclarations(
protected Map<String, VariableExpr> createProtoMethodNameToDescriptorClassMembers(
Service service, Class<?> descriptorClass) {
return service.methods().stream()
.filter(this::isSupportedMethod)
.collect(
Collectors.toMap(
Method::name,
Expand Down Expand Up @@ -354,6 +360,9 @@ private Map<String, VariableExpr> createCallableClassMembers(
Map<String, VariableExpr> callableClassMembers = new LinkedHashMap<>();
// Using a for-loop because the output cardinality is not a 1:1 mapping to the input set.
for (Method protoMethod : service.methods()) {
if (!isSupportedMethod(protoMethod)) {
continue;
}
String javaStyleProtoMethodName = JavaStyle.toLowerCamelCase(protoMethod.name());
String callableName = String.format(CALLABLE_CLASS_MEMBER_PATTERN, javaStyleProtoMethodName);
callableClassMembers.put(
Expand Down Expand Up @@ -643,6 +652,7 @@ protected List<MethodDefinition> createConstructorMethods(
// Transport settings local variables.
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs =
service.methods().stream()
.filter(this::isSupportedMethod)
.collect(
Collectors.toMap(
m -> JavaStyle.toLowerCamelCase(m.name()),
Expand All @@ -666,6 +676,7 @@ protected List<MethodDefinition> createConstructorMethods(

secondCtorExprs.addAll(
service.methods().stream()
.filter(this::isSupportedMethod)
.map(
m ->
createTransportSettingsInitExpr(
Expand Down Expand Up @@ -1035,7 +1046,9 @@ private List<MethodDefinition> createStubOverrideMethods(
}

private boolean checkOperationPollingMethod(Service service) {
return service.methods().stream().anyMatch(Method::isOperationPollingMethod);
return service.methods().stream()
.filter(this::isSupportedMethod)
.anyMatch(Method::isOperationPollingMethod);
}

protected List<MethodDefinition> createLongRunningClientGetters() {
Expand Down Expand Up @@ -1073,6 +1086,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
typeStore.putAll(
service.pakkage(),
service.methods().stream()
.filter(this::isSupportedMethod)
.filter(Method::isPaged)
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
.collect(Collectors.toList()),
Expand Down
88 changes: 62 additions & 26 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 @@ -32,6 +32,7 @@
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.resourcename.ResourceNameTokenizer;
import com.google.api.generator.gapic.model.Field;
import com.google.api.generator.gapic.model.HttpBindings;
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.MethodArgument;
import com.google.api.generator.gapic.model.ResourceName;
Expand Down Expand Up @@ -65,7 +66,8 @@ public static Expr createMethodArgValue(
MethodArgument methodArg,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes,
Map<String, String> valuePatterns) {
Map<String, String> valuePatterns,
HttpBindings bindings) {
if (methodArg.isResourceNameHelper()) {
Preconditions.checkState(
methodArg.field().hasResourceReference(),
Expand All @@ -84,7 +86,8 @@ public static Expr createMethodArgValue(
resourceName,
methodArg.field().resourceReference().isChildType(),
resourceNames.values().stream().collect(Collectors.toList()),
methodArg.field().name());
methodArg.field().name(),
bindings);

if (!methodArg.isResourceNameHelper() && methodArg.field().hasResourceReference()) {
defValue =
Expand Down Expand Up @@ -156,7 +159,7 @@ public static Expr createValue(
Message nestedMessage = messageTypes.get(field.type().reference().fullName());
if (nestedMessage != null) {
return createSimpleMessageBuilderValue(
nestedMessage, resourceNames, messageTypes, nestedValuePatterns);
nestedMessage, resourceNames, messageTypes, nestedValuePatterns, null);
}
}

Expand Down Expand Up @@ -209,8 +212,10 @@ public static Expr createResourceHelperValue(
ResourceName resourceName,
boolean isChildType,
List<ResourceName> resnames,
String fieldOrMessageName) {
return createResourceHelperValue(resourceName, isChildType, resnames, fieldOrMessageName, true);
String fieldOrMessageName,
HttpBindings bindings) {
return createResourceHelperValue(
resourceName, isChildType, resnames, fieldOrMessageName, true, bindings);
}

private static Optional<ResourceName> findParentResource(
Expand Down Expand Up @@ -239,7 +244,8 @@ static Expr createResourceHelperValue(
boolean isChildType,
List<ResourceName> resnames,
String fieldOrMessageName,
boolean allowAnonResourceNameClass) {
boolean allowAnonResourceNameClass,
HttpBindings bindings) {

if (isChildType) {
resourceName = findParentResource(resourceName, resnames).orElse(resourceName);
Expand All @@ -249,13 +255,15 @@ static Expr createResourceHelperValue(
List<ResourceName> unexaminedResnames = new ArrayList<>(resnames);
for (ResourceName resname : resnames) {
unexaminedResnames.remove(resname);
if (!resname.isOnlyWildcard()) {
return createResourceHelperValue(resname, false, unexaminedResnames, fieldOrMessageName);
if (!resname.isOnlyWildcard()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a couple of new logics in this method, and I understand it's in composers so unit testing it might get complicated, can we add some test cases in the protos if it's not too hard?

Copy link
Contributor Author

@vam-google vam-google May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My general feeling about this codebase is that it is beyond comprehension unfortunately and this affects the testing and developing strategy.
This new logic is being tested by the current set of tests already, though it does not have a dedicated unit test item (but most of the code in this huge codebase doesn't)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the sentiment and I totally feel you when I was developing dynamic routing headers, but do we want to make it worse? e.g. I added this proto to test different combinations for dynamic routing headers, it may still not cover everything but I think it helps.

Copy link
Contributor Author

@vam-google vam-google May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I really believe that those extra tests are not making it better, but making it worse. Basically we add more unmaintainable code. This codebase has 10+ times more code than it should for the task it performs. We are just adding unmaintainable tests to unmaintainable code. The gist is covered - this logic affects the existing tests. Testing multiple times the same code does not seem right

&& (bindings == null || resname.getMatchingPattern(bindings) != null)) {
return createResourceHelperValue(
resname, false, unexaminedResnames, fieldOrMessageName, null);
}
}

return allowAnonResourceNameClass
? createAnonymousResourceNameClassValue(fieldOrMessageName)
? createAnonymousResourceNameClassValue(fieldOrMessageName, bindings)
: ValueExpr.withValue(
StringObjectValue.withValue(
String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode())));
Expand All @@ -272,15 +280,22 @@ static Expr createResourceHelperValue(
if (containsOnlyDeletedTopic) {
ofMethodName = "ofDeletedTopic";
} else {
for (String pattern : resourceName.patterns()) {
if (pattern.equals(ResourceNameConstants.WILDCARD_PATTERN)
|| pattern.equals(ResourceNameConstants.DELETED_TOPIC_LITERAL)) {
continue;
String matchingPattern = null;
if (bindings != null) {
matchingPattern = resourceName.getMatchingPattern(bindings);
}
if (matchingPattern == null) {
for (String pattern : resourceName.patterns()) {
if (pattern.equals(ResourceNameConstants.WILDCARD_PATTERN)
|| pattern.equals(ResourceNameConstants.DELETED_TOPIC_LITERAL)) {
continue;
}
matchingPattern = pattern;
break;
}
patternPlaceholderTokens.addAll(
ResourceNameTokenizer.parseTokenHierarchy(Arrays.asList(pattern)).get(0));
break;
}
patternPlaceholderTokens.addAll(
ResourceNameTokenizer.parseTokenHierarchy(Arrays.asList(matchingPattern)).get(0));
}

boolean hasOnePattern = resourceName.patterns().size() == 1;
Expand Down Expand Up @@ -312,16 +327,20 @@ static Expr createResourceHelperValue(
}

public static Expr createSimpleMessageBuilderValue(
Message message, Map<String, ResourceName> resourceNames, Map<String, Message> messageTypes) {
Message message,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes,
HttpBindings bindings) {
return createSimpleMessageBuilderValue(
message, resourceNames, messageTypes, Collections.emptyMap());
message, resourceNames, messageTypes, Collections.emptyMap(), bindings);
}

public static Expr createSimpleMessageBuilderValue(
Message message,
Map<String, ResourceName> resourceNames,
Map<String, Message> messageTypes,
Map<String, String> valuePatterns) {
Map<String, String> valuePatterns,
HttpBindings bindings) {
MethodInvocationExpr builderExpr =
MethodInvocationExpr.builder()
.setStaticReferenceType(message.type())
Expand Down Expand Up @@ -350,7 +369,8 @@ public static Expr createSimpleMessageBuilderValue(
field.resourceReference().isChildType(),
resourceNames.values().stream().collect(Collectors.toList()),
message.name(),
/* allowAnonResourceNameClass = */ false);
/* allowAnonResourceNameClass = */ false,
bindings);
defaultExpr =
MethodInvocationExpr.builder()
.setExprReferenceExpr(defaultExpr)
Expand Down Expand Up @@ -507,7 +527,8 @@ public static Expr createSimplePagedResponseValue(
}

@VisibleForTesting
static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMessageName) {
static AnonymousClassExpr createAnonymousResourceNameClassValue(
String fieldOrMessageName, HttpBindings matchedBindings) {
TypeNode stringMapType =
TypeNode.withReference(
ConcreteReference.builder()
Expand All @@ -525,12 +546,18 @@ static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMe
// fieldValuesMap.put("resource", "resource-12345");
// return fieldValuesMap;
// }

String toStringValue = String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode());
if (matchedBindings != null) {
Map<String, String> valuePatterns = matchedBindings.getPathParametersValuePatterns();
toStringValue =
constructValueMatchingPattern(fieldOrMessageName, valuePatterns.get(fieldOrMessageName));
}

VariableExpr fieldValuesMapVarExpr =
VariableExpr.withVariable(
Variable.builder().setType(stringMapType).setName("fieldValuesMap").build());
StringObjectValue fieldOrMessageStringValue =
StringObjectValue.withValue(
String.format("%s%s", fieldOrMessageName, fieldOrMessageName.hashCode()));
StringObjectValue fieldOrMessageStringValue = StringObjectValue.withValue(toStringValue);

List<Expr> bodyExprs =
Arrays.asList(
Expand Down Expand Up @@ -586,15 +613,24 @@ static AnonymousClassExpr createAnonymousResourceNameClassValue(String fieldOrMe
.build())
.build();

MethodDefinition toStringMethod =
MethodDefinition.builder()
.setIsOverride(true)
.setScope(ScopeNode.PUBLIC)
.setReturnType(TypeNode.STRING)
.setName("toString")
.setReturnExpr(ValueExpr.withValue(StringObjectValue.withValue(toStringValue)))
.build();

return AnonymousClassExpr.builder()
.setType(
TypeNode.withReference(
ConcreteReference.withClazz(com.google.api.resourcenames.ResourceName.class)))
.setMethods(Arrays.asList(getFieldValuesMapMethod, getFieldValueMethod))
.setMethods(Arrays.asList(getFieldValuesMapMethod, getFieldValueMethod, toStringMethod))
.build();
}

private static String constructValueMatchingPattern(String fieldName, String pattern) {
public static String constructValueMatchingPattern(String fieldName, String pattern) {
if (pattern == null || pattern.isEmpty()) {
return fieldName + fieldName.hashCode();
}
Expand Down
7 changes: 4 additions & 3 deletions ...ain/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,8 @@ protected MethodDefinition createStreamingRpcTestMethod(
DefaultValueComposer.createSimpleMessageBuilderValue(
messageTypes.get(methodOutputType.reference().fullName()),
resourceNames,
messageTypes);
messageTypes,
method.httpBindings());
} else {
// Wrap this in a field so we don't have to split the helper into lots of different methods,
// or duplicate it for VariableExpr.
Expand Down Expand Up @@ -608,7 +609,7 @@ protected MethodDefinition createStreamingRpcTestMethod(
Preconditions.checkNotNull(requestMessage);
Expr valExpr =
DefaultValueComposer.createSimpleMessageBuilderValue(
requestMessage, resourceNames, messageTypes);
requestMessage, resourceNames, messageTypes, method.httpBindings());
methodExprs.add(
AssignmentExpr.builder()
.setVariableExpr(requestVarExpr.toBuilder().setIsDecl(true).build())
Expand Down Expand Up @@ -881,7 +882,7 @@ protected List<Statement> createStreamingRpcExceptionTestStatements(
Preconditions.checkNotNull(requestMessage);
Expr valExpr =
DefaultValueComposer.createSimpleMessageBuilderValue(
requestMessage, resourceNames, messageTypes);
requestMessage, resourceNames, messageTypes, method.httpBindings());

List<Statement> statements = new ArrayList<>();
statements.add(
Expand Down
Loading