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

feat: Implement field presence support for DIREGAPIC #774

Merged
merged 2 commits into from
Jun 19, 2021
Merged

feat: Implement field presence support for DIREGAPIC #774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc3a66b
feat: Implement field presence support for DIREGAPIC
vam-google Jun 18, 2021
84b542e
address PR feedback
vam-google Jun 19, 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
7 changes: 4 additions & 3 deletions src/main/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.common.AbstractServiceStubClassComposer;
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.utils.JavaStyle;
Expand Down Expand Up @@ -288,11 +289,11 @@ private AnonymousClassExpr createRequestParamsExtractorAnonClass(Method method)
VariableExpr.withVariable(
Variable.builder().setType(method.inputType()).setName("request").build());

for (String httpBindingFieldName : method.httpBindings().pathParameters()) {
for (HttpBinding httpBindingFieldBinding : method.httpBindings().pathParameters()) {
// Handle foo.bar cases by descending into the subfields.
MethodInvocationExpr.Builder requestFieldGetterExprBuilder =
MethodInvocationExpr.builder().setExprReferenceExpr(requestVarExpr);
String[] descendantFields = httpBindingFieldName.split("\\.");
String[] descendantFields = httpBindingFieldBinding.name().split("\\.");
for (int i = 0; i < descendantFields.length; i++) {
String currFieldName = descendantFields[i];
String bindingFieldMethodName =
Expand All @@ -319,7 +320,7 @@ private AnonymousClassExpr createRequestParamsExtractorAnonClass(Method method)
.setExprReferenceExpr(paramsVarExpr)
.setMethodName("put")
.setArguments(
ValueExpr.withValue(StringObjectValue.withValue(httpBindingFieldName)),
ValueExpr.withValue(StringObjectValue.withValue(httpBindingFieldBinding.name())),
valueOfExpr)
.build();
bodyExprs.add(paramsPutExpr);
Expand Down
45 changes: 36 additions & 9 deletions ...n/java/com/google/api/generator/gapic/composer/rest/HttpJsonServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
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.MethodDefinition;
import com.google.api.generator.engine.ast.MethodInvocationExpr;
import com.google.api.generator.engine.ast.NewObjectExpr;
Expand All @@ -42,6 +43,7 @@
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.common.AbstractServiceStubClassComposer;
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.utils.JavaStyle;
Expand Down Expand Up @@ -357,9 +359,9 @@ private List<Expr> setResponseParserExpr(Method protoMethod) {
private Expr createFieldsExtractorAnonClass(
Method method,
TypeNode extractorReturnType,
Set<String> httpBindingFieldNames,
Set<HttpBinding> httpBindingFieldNames,
String serializerMethodName) {
List<Expr> bodyExprs = new ArrayList<>();
List<Statement> bodyStatements = new ArrayList<>();

Expr returnExpr = null;
VariableExpr fieldsVarExpr = null;
Expand Down Expand Up @@ -389,7 +391,7 @@ private Expr createFieldsExtractorAnonClass(
.build())
.build();

bodyExprs.add(fieldsAssignExpr);
bodyStatements.add(ExprStatement.withExpr(fieldsAssignExpr));
returnExpr = fieldsVarExpr;

TypeNode serializerVarType =
Expand Down Expand Up @@ -417,40 +419,57 @@ private Expr createFieldsExtractorAnonClass(

serializerExpr = serializerVarExpr;

bodyExprs.add(serializerAssignExpr);
bodyStatements.add(ExprStatement.withExpr(serializerAssignExpr));
}

VariableExpr requestVarExpr =
VariableExpr.withVariable(
Variable.builder().setType(method.inputType()).setName("request").build());

for (String httpBindingFieldName : httpBindingFieldNames) {
for (HttpBinding httpBindingFieldName : httpBindingFieldNames) {
// Handle foo.bar cases by descending into the subfields.
MethodInvocationExpr.Builder requestFieldGetterExprBuilder =
MethodInvocationExpr.builder().setExprReferenceExpr(requestVarExpr);
String[] descendantFields = httpBindingFieldName.split("\\.");
MethodInvocationExpr.Builder requestFieldHasExprBuilder =
MethodInvocationExpr.builder().setExprReferenceExpr(requestVarExpr);
String[] descendantFields = httpBindingFieldName.name().split("\\.");
for (int i = 0; i < descendantFields.length; i++) {
String currFieldName = descendantFields[i];
String bindingFieldMethodName =
String.format("get%s", JavaStyle.toUpperCamelCase(currFieldName));
requestFieldGetterExprBuilder =
requestFieldGetterExprBuilder.setMethodName(bindingFieldMethodName);

String bindingFieldHasMethodName =
(i < descendantFields.length - 1)
? bindingFieldMethodName
: String.format("has%s", JavaStyle.toUpperCamelCase(currFieldName));
requestFieldHasExprBuilder =
requestFieldHasExprBuilder
.setMethodName(bindingFieldHasMethodName)
.setReturnType(TypeNode.BOOLEAN);

if (i < descendantFields.length - 1) {
requestFieldGetterExprBuilder =
MethodInvocationExpr.builder()
.setExprReferenceExpr(requestFieldGetterExprBuilder.build());
requestFieldHasExprBuilder =
MethodInvocationExpr.builder()
.setExprReferenceExpr(requestFieldHasExprBuilder.build());
}
}

MethodInvocationExpr requestBuilderExpr = requestFieldGetterExprBuilder.build();
MethodInvocationExpr requestHasExpr = requestFieldHasExprBuilder.build();

ImmutableList.Builder<Expr> paramsPutArgs = ImmutableList.builder();
if (fieldsVarExpr != null) {
paramsPutArgs.add(fieldsVarExpr);
}
paramsPutArgs.add(
ValueExpr.withValue(
StringObjectValue.withValue(JavaStyle.toLowerCamelCase(httpBindingFieldName))));
StringObjectValue.withValue(
JavaStyle.toLowerCamelCase(httpBindingFieldName.name()))));
paramsPutArgs.add(requestBuilderExpr);

Expr paramsPutExpr =
Expand All @@ -464,7 +483,15 @@ private Expr createFieldsExtractorAnonClass(
if (fieldsVarExpr == null) {
returnExpr = paramsPutExpr;
} else {
bodyExprs.add(paramsPutExpr);
if (httpBindingFieldName.isOptional()) {
bodyStatements.add(
IfStatement.builder()
.setConditionExpr(requestHasExpr)
.setBody(Arrays.asList(ExprStatement.withExpr(paramsPutExpr)))
.build());
} else {
bodyStatements.add(ExprStatement.withExpr(paramsPutExpr));
}
}
}

Expand All @@ -475,7 +502,7 @@ private Expr createFieldsExtractorAnonClass(
.setReturnType(extractorReturnType)
.setName("extract")
.setArguments(requestVarExpr.toBuilder().setIsDecl(true).build())
.setBody(bodyExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()))
.setBody(bodyStatements)
.setReturnExpr(returnExpr)
.build();

Expand Down
9 changes: 8 additions & 1 deletion src/main/java/com/google/api/generator/gapic/model/Field.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ public abstract class Field {

public abstract boolean isContainedInOneof();

public abstract boolean isProto3Optional();

@Nullable
public abstract ResourceReference resourceReference();

Expand Down Expand Up @@ -63,6 +65,7 @@ && isEnum() == other.isEnum()
&& isRepeated() == other.isRepeated()
&& isMap() == other.isMap()
&& isContainedInOneof() == other.isContainedInOneof()
&& isProto3Optional() == other.isProto3Optional()
&& Objects.equals(resourceReference(), other.resourceReference())
&& Objects.equals(description(), other.description());
}
Expand All @@ -76,6 +79,7 @@ public int hashCode() {
+ (isRepeated() ? 1 : 0) * 31
+ (isMap() ? 1 : 0) * 37
+ (isContainedInOneof() ? 1 : 0) * 41
+ (isProto3Optional() ? 1 : 0) * 43
+ (resourceReference() == null ? 0 : resourceReference().hashCode())
+ (description() == null ? 0 : description().hashCode());
}
Expand All @@ -88,7 +92,8 @@ public static Builder builder() {
.setIsEnum(false)
.setIsRepeated(false)
.setIsMap(false)
.setIsContainedInOneof(false);
.setIsContainedInOneof(false)
.setIsProto3Optional(false);
}

@AutoValue.Builder
Expand All @@ -107,6 +112,8 @@ public abstract static class Builder {

public abstract Builder setIsContainedInOneof(boolean isContainedInOneof);

public abstract Builder setIsProto3Optional(boolean isProto3Optional);

public abstract Builder setResourceReference(ResourceReference resourceReference);

public abstract Builder setDescription(String description);
Expand Down
32 changes: 24 additions & 8 deletions src/main/java/com/google/api/generator/gapic/model/HttpBindings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,31 @@ public enum HttpVerb {
PATCH,
}

@AutoValue
public abstract static class HttpBinding implements Comparable<HttpBinding> {
public abstract String name();

public abstract boolean isOptional();

public static HttpBinding create(String name, boolean isOptional) {
return new AutoValue_HttpBindings_HttpBinding(name, isOptional);
}

@Override
public int compareTo(HttpBinding o) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to account for other fields? If not, can we add an explanatory comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! It does not matter for our use case (since protobuf guarantees that there can't be optional and non-optional fields in the same message with the same name), but by omitting isOptional() in compareTo() I made compareTo() inconsistent with equals(), which is plain wrong and may lead to nondeterministic behavior of sorted collections. Fixed.

return name().compareTo(o.name());
}
}

public abstract HttpVerb httpVerb();

public abstract String pattern();

public abstract Set<String> pathParameters();
public abstract Set<HttpBinding> pathParameters();

public abstract Set<String> queryParameters();
public abstract Set<HttpBinding> queryParameters();

public abstract Set<String> bodyParameters();
public abstract Set<HttpBinding> bodyParameters();

public static HttpBindings.Builder builder() {
return new AutoValue_HttpBindings.Builder()
Expand All @@ -53,10 +69,10 @@ public static HttpBindings.Builder builder() {
// in .java file: "/global/instanceTemplates/{instanceTemplate=*}"
public String patternLowerCamel() {
String lowerCamelPattern = pattern();
for (String pathParam : pathParameters()) {
for (HttpBinding pathParam : pathParameters()) {
lowerCamelPattern =
lowerCamelPattern.replaceAll(
"\\{" + pathParam, "{" + JavaStyle.toLowerCamelCase(pathParam));
"\\{" + pathParam.name(), "{" + JavaStyle.toLowerCamelCase(pathParam.name()));
}
return lowerCamelPattern;
}
Expand All @@ -69,11 +85,11 @@ public abstract static class Builder {

abstract String pattern();

public abstract HttpBindings.Builder setPathParameters(Set<String> pathParameters);
public abstract HttpBindings.Builder setPathParameters(Set<HttpBinding> pathParameters);

public abstract HttpBindings.Builder setQueryParameters(Set<String> queryParameters);
public abstract HttpBindings.Builder setQueryParameters(Set<HttpBinding> queryParameters);

public abstract HttpBindings.Builder setBodyParameters(Set<String> bodyParameters);
public abstract HttpBindings.Builder setBodyParameters(Set<HttpBinding> bodyParameters);

public abstract HttpBindings autoBuild();

Expand Down
97 changes: 57 additions & 40 deletions src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.google.api.HttpRule.PatternCase;
import com.google.api.generator.gapic.model.Field;
import com.google.api.generator.gapic.model.HttpBindings;
import com.google.api.generator.gapic.model.HttpBindings.HttpBinding;
import com.google.api.generator.gapic.model.Message;
import com.google.api.pathtemplate.PathTemplate;
import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -70,63 +71,79 @@ private static HttpBindings parseHttpRuleHelper(
}
}

Set<String> bindings = bindingsBuilder.build();

// Binding validation.
for (String binding : bindings) {
// Handle foo.bar cases by descending into the subfields.
String[] descendantBindings = binding.split("\\.");
Optional<Message> containingMessageOpt = inputMessageOpt;
for (int i = 0; i < descendantBindings.length; i++) {
String subField = descendantBindings[i];
if (!containingMessageOpt.isPresent()) {
continue;
}

if (i < descendantBindings.length - 1) {
Field field = containingMessageOpt.get().fieldMap().get(subField);
containingMessageOpt = Optional.of(messageTypes.get(field.type().reference().fullName()));
Preconditions.checkNotNull(
containingMessageOpt.get(),
String.format(
"No containing message found for field %s with type %s",
field.name(), field.type().reference().simpleName()));
} else {
checkHttpFieldIsValid(subField, containingMessageOpt.get(), false);
}
}
}
Set<String> pathParamNames = bindingsBuilder.build();

// TODO: support nested message fields bindings
String body = httpRule.getBody();
Set<String> bodyParameters;
Set<String> queryParameters;
Set<String> bodyParamNames;
Set<String> queryParamNames;
if (!inputMessageOpt.isPresent()) {
// Must be a mixin, do not support full HttpRuleBindings for now
bodyParameters = ImmutableSet.of();
queryParameters = ImmutableSet.of();
bodyParamNames = ImmutableSet.of();
queryParamNames = ImmutableSet.of();
} else if (Strings.isNullOrEmpty(body)) {
bodyParameters = ImmutableSet.of();
queryParameters = Sets.difference(inputMessageOpt.get().fieldMap().keySet(), bindings);
bodyParamNames = ImmutableSet.of();
queryParamNames = Sets.difference(inputMessageOpt.get().fieldMap().keySet(), pathParamNames);
} else if (body.equals(ASTERISK)) {
bodyParameters = Sets.difference(inputMessageOpt.get().fieldMap().keySet(), bindings);
queryParameters = ImmutableSet.of();
bodyParamNames = Sets.difference(inputMessageOpt.get().fieldMap().keySet(), pathParamNames);
queryParamNames = ImmutableSet.of();
} else {
bodyParameters = ImmutableSet.of(body);
Set<String> bodyBinidngsUnion = Sets.union(bodyParameters, bindings);
queryParameters =
bodyParamNames = ImmutableSet.of(body);
Set<String> bodyBinidngsUnion = Sets.union(bodyParamNames, pathParamNames);
queryParamNames =
Sets.difference(inputMessageOpt.get().fieldMap().keySet(), bodyBinidngsUnion);
}

Message message = inputMessageOpt.orElse(null);
return HttpBindings.builder()
.setHttpVerb(HttpBindings.HttpVerb.valueOf(httpRule.getPatternCase().toString()))
.setPattern(pattern)
.setPathParameters(ImmutableSortedSet.copyOf(bindings))
.setQueryParameters(ImmutableSortedSet.copyOf(queryParameters))
.setBodyParameters(ImmutableSortedSet.copyOf(bodyParameters))
.setPathParameters(
validateAndConstructHttpBindings(pathParamNames, message, messageTypes, true))
.setQueryParameters(
validateAndConstructHttpBindings(queryParamNames, message, messageTypes, false))
.setBodyParameters(
validateAndConstructHttpBindings(bodyParamNames, message, messageTypes, false))
.build();
}

private static Set<HttpBinding> validateAndConstructHttpBindings(
Set<String> paramNames,
Message inputMessage,
Map<String, Message> messageTypes,
boolean isPath) {
ImmutableSortedSet.Builder<HttpBinding> httpBindings = ImmutableSortedSet.naturalOrder();
for (String paramName : paramNames) {
// Handle foo.bar cases by descending into the subfields.
String[] subFields = paramName.split("\\.");
if (inputMessage == null) {
httpBindings.add(HttpBinding.create(paramName, false));
continue;
}
Message nestedMessage = inputMessage;
for (int i = 0; i < subFields.length; i++) {
String subFieldName = subFields[i];
if (i < subFields.length - 1) {
Field field = nestedMessage.fieldMap().get(subFieldName);
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 {
if (isPath) {
checkHttpFieldIsValid(subFieldName, nestedMessage, !isPath);
}
Field field = nestedMessage.fieldMap().get(subFieldName);
httpBindings.add(HttpBinding.create(paramName, field.isProto3Optional()));
}
}
}
return httpBindings.build();
}

private static String getHttpVerbPattern(HttpRule httpRule) {
PatternCase patternCase = httpRule.getPatternCase();
switch (patternCase) {
Expand Down
Loading