Skip to content

Commit

Permalink
#869. Add null checks for field values before using them to construct…
Browse files Browse the repository at this point in the history 
… routing header params map.

- Add test cases for all the examples provided in routing.proto. Move all testing protos for explicit routing headers to its own package and bazel rule.
- Switch to inline String format for error messages.
  • Loading branch information
@blakeli0
blakeli0 committed Jan 18, 2022
1 parent 7b3503e commit b824c94
Show file tree
Hide file tree
Showing 17 changed files with 1,135 additions and 37 deletions.
78 changes: 64 additions & 14 deletions src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@
import com.google.api.generator.engine.ast.EnumRefExpr;
import com.google.api.generator.engine.ast.Expr;
import com.google.api.generator.engine.ast.ExprStatement;
import com.google.api.generator.engine.ast.IfStatement;
import com.google.api.generator.engine.ast.LambdaExpr;
import com.google.api.generator.engine.ast.LogicalOperationExpr;
import com.google.api.generator.engine.ast.MethodInvocationExpr;
import com.google.api.generator.engine.ast.NewObjectExpr;
import com.google.api.generator.engine.ast.RelationalOperationExpr;
import com.google.api.generator.engine.ast.ScopeNode;
import com.google.api.generator.engine.ast.Statement;
import com.google.api.generator.engine.ast.StringObjectValue;
Expand Down Expand Up @@ -54,7 +57,6 @@
import java.util.Set;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;

public class GrpcServiceStubClassComposer extends AbstractTransportServiceStubClassComposer {

Expand Down Expand Up @@ -255,7 +257,7 @@ protected String getProtoRpcFullMethodName(Service protoService, Method protoMet

private LambdaExpr createRequestParamsExtractorClassInstance(
Method method, List<Statement> classStatements) {
List<Expr> bodyExprs = new ArrayList<>();
List<Statement> bodyStatements = new ArrayList<>();
VariableExpr requestVarExpr =
VariableExpr.withVariable(
Variable.builder().setType(method.inputType()).setName("request").build());
Expand All @@ -269,27 +271,27 @@ private LambdaExpr createRequestParamsExtractorClassInstance(
// If the google.api.routing annotation is present(even with empty routing parameters),
// the implicit routing headers specified in the google.api.http annotation should not be sent
if (method.routingHeaders() == null) {
returnExpr = addRequestParamsForHttpBindings(method, bodyExprs, requestVarExpr, returnType);
returnExpr =
addRequestParamsForHttpBindings(method, bodyStatements, requestVarExpr, returnType);
} else {
returnExpr =
addRequestParamsForRoutingHeaders(
method, classStatements, bodyExprs, requestVarExpr, returnType);
method, classStatements, bodyStatements, requestVarExpr, returnType);
}

// Overrides extract().
// https://github.com/googleapis/gax-java/blob/8d45d186e36ae97b789a6f89d80ae5213a773b65/gax/src/main/java/com/google/api/gax/rpc/RequestParamsExtractor.java#L55
return LambdaExpr.builder()
.setArguments(requestVarExpr.toBuilder().setIsDecl(true).build())
.setBody(
bodyExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()))
.setBody(bodyStatements)
.setReturnExpr(returnExpr)
.build();
}

private Expr addRequestParamsForRoutingHeaders(
Method method,
List<Statement> classStatements,
List<Expr> bodyExprs,
List<Statement> bodyStatements,
VariableExpr requestVarExpr,
TypeNode returnType) {
TypeNode routingHeadersBuilderType =
Expand All @@ -307,8 +309,8 @@ private Expr addRequestParamsForRoutingHeaders(
.setVariableExpr(routingHeadersBuilderVarExpr)
.setValueExpr(newBuilderExpr)
.build();
bodyExprs.add(newRoutingHeadersAssignExpr);
ImmutableList<RoutingHeader> routingHeaders = method.routingHeaders().routingHeadersList();
bodyStatements.add(ExprStatement.withExpr(newRoutingHeadersAssignExpr));
List<RoutingHeader> routingHeaders = method.routingHeaders().routingHeadersList();
VariableExpr routingHeadersBuilderVarNonDeclExpr =
VariableExpr.builder()
.setVariable(
Expand Down Expand Up @@ -351,14 +353,27 @@ private Expr addRequestParamsForRoutingHeaders(
.build();
Statement pathTemplateClassVar = ExprStatement.withExpr(pathTemplateExpr);
classStatements.add(pathTemplateClassVar);
Expr valueOfExpr =
MethodInvocationExpr.builder()
.setStaticReferenceType(TypeNode.STRING)
.setMethodName("valueOf")
.setArguments(requestFieldGetterExpr)
.build();
MethodInvocationExpr addParamsMethodExpr =
MethodInvocationExpr.builder()
.setExprReferenceExpr(routingHeadersBuilderVarNonDeclExpr)
.setMethodName("add")
.setArguments(requestFieldGetterExpr, routingHeaderKeyExpr, routingHeaderPatternExpr)
.setArguments(valueOfExpr, routingHeaderKeyExpr, routingHeaderPatternExpr)
.build();

bodyExprs.add(addParamsMethodExpr);
IfStatement ifStatement =
IfStatement.builder()
.setConditionExpr(
fieldValuesNotNullConditionExpr(
requestVarExpr, routingHeader.getDescendantFieldNames()))
.setBody(ImmutableList.of(ExprStatement.withExpr(addParamsMethodExpr)))
.build();
bodyStatements.add(ifStatement);
}

return MethodInvocationExpr.builder()
Expand All @@ -368,8 +383,43 @@ private Expr addRequestParamsForRoutingHeaders(
.build();
}

private Expr fieldValuesNotNullConditionExpr(
VariableExpr requestVarExpr, List<String> fieldNames) {
MethodInvocationExpr.Builder requestFieldGetterExprBuilder =
MethodInvocationExpr.builder().setExprReferenceExpr(requestVarExpr);
Expr fieldValuesNotNullExpr = null;
for (int i = 0; i < fieldNames.size(); i++) {
String currFieldName = fieldNames.get(i);
String bindingFieldMethodName =
String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName));
requestFieldGetterExprBuilder =
requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName);
// set return type of each method invocation to String just to pass the validation for
// RelationalOperationExpr that both side of relational operation needs to be a valid equality
// type
MethodInvocationExpr requestGetterExpr =
requestFieldGetterExprBuilder.setReturnType(TypeNode.STRING).build();
Expr currentValueNotNullExpr =
RelationalOperationExpr.notEqualToWithExprs(
requestGetterExpr, ValueExpr.createNullExpr());
if (fieldValuesNotNullExpr == null) {
fieldValuesNotNullExpr = currentValueNotNullExpr;
} else {
fieldValuesNotNullExpr =
LogicalOperationExpr.logicalAndWithExprs(
fieldValuesNotNullExpr, currentValueNotNullExpr);
}
requestFieldGetterExprBuilder =
MethodInvocationExpr.builder().setExprReferenceExpr(requestGetterExpr);
}
return fieldValuesNotNullExpr;
}

private Expr addRequestParamsForHttpBindings(
Method method, List<Expr> bodyExprs, VariableExpr requestVarExpr, TypeNode returnType) {
Method method,
List<Statement> bodyStatements,
VariableExpr requestVarExpr,
TypeNode returnType) {
TypeNode paramsVarType =
TypeNode.withReference(
ConcreteReference.builder()
Expand All @@ -390,7 +440,7 @@ private Expr addRequestParamsForHttpBindings(
.setReturnType(paramsVarType)
.build())
.build();
bodyExprs.add(paramsAssignExpr);
bodyStatements.add(ExprStatement.withExpr(paramsAssignExpr));

for (HttpBinding httpBindingFieldBinding : method.httpBindings().pathParameters()) {
MethodInvocationExpr requestBuilderExpr =
Expand All @@ -410,7 +460,7 @@ private Expr addRequestParamsForHttpBindings(
ValueExpr.withValue(StringObjectValue.withValue(httpBindingFieldBinding.name())),
valueOfExpr)
.build();
bodyExprs.add(paramsPutExpr);
bodyStatements.add(ExprStatement.withExpr(paramsPutExpr));
}

return MethodInvocationExpr.builder()
Expand Down
30 changes: 18 additions & 12 deletions src/main/java/com/google/api/generator/gapic/model/Message.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.api.generator.engine.ast.TypeNode;
import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
Expand Down Expand Up @@ -84,28 +85,33 @@ public boolean hasResource() {
}

public void validateField(String fieldName, Map<String, Message> messageTypes) {
String[] subFields = fieldName.split("\\.");
List<String> subFields = Splitter.on(".").splitToList(fieldName);
Message nestedMessage = this;
for (int i = 0; i < subFields.length; i++) {
String subFieldName = subFields[i];
if (i < subFields.length - 1) {
Field field = nestedMessage.fieldMap().get(subFieldName);
for (int i = 0; i < subFields.size(); i++) {
String subFieldName = subFields.get(i);
Preconditions.checkState(
!Strings.isNullOrEmpty(subFieldName),
String.format("Null or empty field name found for message %s", nestedMessage.name()));
Field field = nestedMessage.fieldMap().get(subFieldName);
Preconditions.checkNotNull(
field,
String.format(
"Expected message %s to contain field %s but none found",
nestedMessage.name(), subFieldName));
if (i < subFields.size() - 1) {
nestedMessage = messageTypes.get(field.type().reference().fullName());
Preconditions.checkNotNull(
nestedMessage,
String.format(
"No containing message found for field %s with type %s",
field.name(), field.type().reference().simpleName()));
} else {
// TODO: Type check for String or primitive?
Preconditions.checkState(
!Strings.isNullOrEmpty(subFieldName),
String.format("Null or empty field name found for message %s", nestedMessage.name()));
Preconditions.checkState(
nestedMessage.fieldMap().containsKey(subFieldName),
!field.isRepeated() && field.type().isProtoPrimitiveType(),
String.format(
"Expected message %s to contain field %s but none found",
nestedMessage.name(), subFieldName));
// TODO: Add type check for String only?
"The type of field %s must be primitive and not repeated.",
field.name()));
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/google/api/generator/gapic/model/RoutingHeaders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
package com.google.api.generator.gapic.model;

import com.google.auto.value.AutoValue;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import java.util.List;

@AutoValue
public abstract class RoutingHeaders {
Expand All @@ -38,6 +40,10 @@ public abstract static class RoutingHeader {
public static RoutingHeaders.RoutingHeader create(String field, String name, String pattern) {
return new AutoValue_RoutingHeaders_RoutingHeader(field, name, pattern);
}

public List<String> getDescendantFieldNames() {
return Splitter.on(".").splitToList(fieldName());
}
}

@AutoValue.Builder
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/api/generator/gapic/composer/common/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ TEST_DEPS = [
"//src/test/java/com/google/api/generator/gapic/testdata:deprecated_service_java_proto",
"//src/test/java/com/google/api/generator/gapic/testdata:bookshop_java_proto",
"//src/test/java/com/google/api/generator/gapic/testdata:showcase_java_proto",
"//src/test/java/com/google/api/generator/gapic/testdata:explicit_dynamic_routing_headers_testing_java_proto",
"//src/test/java/com/google/api/generator/gapic/testdata:testgapic_java_proto",
"@com_google_api_api_common//jar",
"@com_google_api_gax_java//gax",
Expand Down
26 changes: 26 additions & 0 deletions src/test/java/com/google/api/generator/gapic/composer/common/TestProtoLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.google.api.generator.gapic.protoparser.Parser;
import com.google.api.generator.gapic.protoparser.ServiceConfigParser;
import com.google.bookshop.v1beta1.BookshopProto;
import com.google.explicit.dynamic.routing.header.ExplicitDynamicRoutingHeaderTestingOuterClass;
import com.google.logging.v2.LogEntryProto;
import com.google.logging.v2.LoggingConfigProto;
import com.google.logging.v2.LoggingMetricsProto;
Expand Down Expand Up @@ -218,6 +219,31 @@ public GapicContext parseShowcaseTesting() {
.build();
}

public GapicContext parseExplicitDynamicRoutingHeaderTesting() {
FileDescriptor testingFileDescriptor = ExplicitDynamicRoutingHeaderTestingOuterClass.getDescriptor();
ServiceDescriptor testingService = testingFileDescriptor.getServices().get(0);
assertEquals(testingService.getName(), "ExplicitDynamicRoutingHeaderTesting");

Map<String, Message> messageTypes = Parser.parseMessages(testingFileDescriptor);
Map<String, ResourceName> resourceNames = Parser.parseResourceNames(testingFileDescriptor);
Set<ResourceName> outputResourceNames = new HashSet<>();
List<Service> services =
Parser.parseService(
testingFileDescriptor,
messageTypes,
resourceNames,
Optional.empty(),
outputResourceNames);

return GapicContext.builder()
.setMessages(messageTypes)
.setResourceNames(resourceNames)
.setServices(services)
.setHelperResourceNames(outputResourceNames)
.setTransport(transport)
.build();
}

public GapicContext parsePubSubPublisher() {
FileDescriptor serviceFileDescriptor = PubsubProto.getDescriptor();
FileDescriptor commonResourcesFileDescriptor = CommonResources.getDescriptor();
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/api/generator/gapic/composer/grpc/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ TEST_DEPS = [
"//src/main/java/com/google/api/generator/gapic/protoparser",
"//src/main/java/com/google/api/generator/gapic/composer/defaultvalue",
"//src/test/java/com/google/api/generator/gapic/testdata:showcase_java_proto",
"//src/test/java/com/google/api/generator/gapic/testdata:explicit_dynamic_routing_headers_testing_java_proto",
"@com_google_api_api_common//jar",
"@com_google_api_gax_java//gax",
"@com_google_api_api_common",
Expand Down
15 changes: 15 additions & 0 deletions ...t/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,21 @@ public void generateGrpcServiceStubClass_httpBindings() {
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void generateGrpcServiceStubClass_routingHeaders() {
GapicContext context =
GrpcTestProtoLoader.instance().parseExplicitDynamicRoutingHeaderTesting();
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Utils.saveCodegenToFile(this.getClass(), "GrpcRoutingHeadersStub.golden", visitor.write());
Path goldenFilePath =
Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcRoutingHeadersStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void generateGrpcServiceStubClass_httpBindingsWithSubMessageFields() {
GapicContext context = GrpcTestProtoLoader.instance().parsePubSubPublisher();
Expand Down
Loading

0 comments on commit b824c94

Please sign in to comment.