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.

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: REST LRO implementation #859

Merged
merged 11 commits into from
Oct 9, 2021
3 changes: 2 additions & 1 deletion WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ jvm_maven_import_external(
# gapic-generator-java dependencies to match the order in googleapis repository,
# which in its turn, prioritizes actual generated clients runtime dependencies
# over the generator dependencies.
_gax_java_version = "2.3.0"

_gax_java_version = "2.4.0"

http_archive(
name = "com_google_api_gax_java",
Expand Down
10 changes: 6 additions & 4 deletions repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,20 @@ def gapic_generator_java_repositories():
_maybe(
http_archive,
name = "com_google_googleapis",
strip_prefix = "googleapis-efecdbf96311bb705d619459280ffc651b10844a",
strip_prefix = "googleapis-ba30d8097582039ac4cc4e21b4e4baa426423075",
urls = [
"https://github.com/googleapis/googleapis/archive/efecdbf96311bb705d619459280ffc651b10844a.zip",
"https://github.com/googleapis/googleapis/archive/ba30d8097582039ac4cc4e21b4e4baa426423075.zip",
],
)

# TODO: replace with upstream googleapis-discovery link once
# https://github.com/googleapis/googleapis-discovery/pull/59 is merged
_maybe(
http_archive,
name = "com_google_googleapis_discovery",
strip_prefix = "googleapis-discovery-abf4cec1ce9e02e4d7d650bf66137c347cdd0d44",
strip_prefix = "googleapis-discovery-bb8a053b93ef8698297c41634aa9201f7e075277",
urls = [
"https://github.com/googleapis/googleapis-discovery/archive/abf4cec1ce9e02e4d7d650bf66137c347cdd0d44.zip",
"https://github.com/vam-google/googleapis-discovery/archive/bb8a053b93ef8698297c41634aa9201f7e075277.zip",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"https://github.com/vam-google/googleapis-discovery/archive/bb8a053b93ef8698297c41634aa9201f7e075277.zip",
"https://github.com/googleapis/googleapis-discovery/archive/....zip",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to point to upstream

],
)

Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/api/generator/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ java_library(
"//src/main/java/com/google/api/generator/gapic/model",
"//src/main/java/com/google/api/generator/util",
"@com_google_googleapis//google/api:api_java_proto",
"@com_google_googleapis//google/cloud:extended_operations_java_proto",
"@com_google_googleapis//google/longrunning:longrunning_java_proto",
"@com_google_guava_guava//jar",
"@com_google_protobuf//:protobuf_java",
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/api/generator/ProtoRegistry.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.api.ClientProto;
import com.google.api.FieldBehaviorProto;
import com.google.api.ResourceProto;
import com.google.cloud.ExtendedOperationsProto;
import com.google.longrunning.OperationsProto;
import com.google.protobuf.ExtensionRegistry;

Expand All @@ -29,5 +30,6 @@ public static void registerAllExtensions(ExtensionRegistry extensionRegistry) {
ClientProto.registerAllExtensions(extensionRegistry);
ResourceProto.registerAllExtensions(extensionRegistry);
FieldBehaviorProto.registerAllExtensions(extensionRegistry);
ExtendedOperationsProto.registerAllExtensions(extensionRegistry);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ protected TransportContext getTransportContext() {

@Override
public GapicClass generate(GapicContext context, Service service) {
TypeStore typeStore = createTypes();
TypeStore typeStore = createTypes(service);

String className =
getTransportContext().classNames().getTransportServiceCallableFactoryClassName(service);
GapicClass.Kind kind = Kind.STUB;
Expand All @@ -77,9 +78,9 @@ public GapicClass generate(GapicContext context, Service service) {
commentComposer.createTransportServiceCallableFactoryClassHeaderComments(
service.name(), service.isDeprecated()))
.setAnnotations(createClassAnnotations(service, typeStore))
.setImplementsTypes(createClassImplements(typeStore))
.setImplementsTypes(createClassImplements(typeStore, service))
.setName(className)
.setMethods(createClassMethods(typeStore))
.setMethods(createClassMethods(service, typeStore))
.setScope(ScopeNode.PUBLIC)
.build();
return GapicClass.create(kind, classDef);
Expand Down Expand Up @@ -110,22 +111,23 @@ protected List<AnnotationNode> createClassAnnotations(Service service, TypeStore
* @return {@code TypeNode} containing the interface to be implemented by the generated callable
* factory class.
*/
protected abstract List<TypeNode> createClassImplements(TypeStore typeStore);
protected abstract List<TypeNode> createClassImplements(TypeStore typeStore, Service service);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth having the consistency in the argument order with others?

Suggested change
protected abstract List<TypeNode> createClassImplements(TypeStore typeStore, Service service);
protected abstract List<TypeNode> createClassImplements(Service service, TypeStore typeStore);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is better to have ordering consistent, though there are so much code in gapic-generator in general, so things like arguments ordering is not really a consistent thing accross the generator codebase (meaning other similar methods, not this specific one necessarily). As result it is hard to define what is the "right" ordering. for this specific one it is clearly service should go first, so reordered the parsms, thanks!


protected List<MethodDefinition> createClassMethods(TypeStore typeStore) {
protected List<MethodDefinition> createClassMethods(Service service, TypeStore typeStore) {
return Arrays.asList(
createUnaryCallableMethod(typeStore),
createPagedCallableMethod(typeStore),
createBatchingCallableMethod(typeStore),
createOperationCallableMethod(typeStore));
createUnaryCallableMethod(service, typeStore),
createPagedCallableMethod(service, typeStore),
createBatchingCallableMethod(service, typeStore),
createOperationCallableMethod(service, typeStore));
}

protected MethodDefinition createUnaryCallableMethod(TypeStore typeStore) {
protected MethodDefinition createUnaryCallableMethod(Service service, TypeStore typeStore) {
String methodVariantName = "Unary";
String requestTemplateName = "RequestT";
String responseTemplateName = "ResponseT";
List<String> methodTemplateNames = Arrays.asList(requestTemplateName, responseTemplateName);
return createGenericCallableMethod(
service,
typeStore,
/*methodTemplateNames=*/ methodTemplateNames,
/*returnCallableKindName=*/ methodVariantName,
Expand All @@ -140,14 +142,15 @@ protected MethodDefinition createUnaryCallableMethod(TypeStore typeStore) {
.collect(Collectors.toList()));
}

protected MethodDefinition createPagedCallableMethod(TypeStore typeStore) {
protected MethodDefinition createPagedCallableMethod(Service service, TypeStore typeStore) {
String methodVariantName = "Paged";
String requestTemplateName = "RequestT";
String pagedResponseTemplateName = "PagedListResponseT";
String responseTemplateName = "ResponseT";
List<String> methodTemplateNames =
Arrays.asList(requestTemplateName, responseTemplateName, pagedResponseTemplateName);
return createGenericCallableMethod(
service,
typeStore,
/*methodTemplateNames=*/ methodTemplateNames,
/*returnCallableKindName=*/ "Unary",
Expand All @@ -162,12 +165,13 @@ protected MethodDefinition createPagedCallableMethod(TypeStore typeStore) {
.collect(Collectors.toList()));
}

protected MethodDefinition createBatchingCallableMethod(TypeStore typeStore) {
protected MethodDefinition createBatchingCallableMethod(Service service, TypeStore typeStore) {
String methodVariantName = "Batching";
String requestTemplateName = "RequestT";
String responseTemplateName = "ResponseT";
List<String> methodTemplateNames = Arrays.asList(requestTemplateName, responseTemplateName);
return createGenericCallableMethod(
service,
typeStore,
/*methodTemplateNames=*/ methodTemplateNames,
/*returnCallableKindName=*/ "Unary",
Expand All @@ -182,9 +186,11 @@ protected MethodDefinition createBatchingCallableMethod(TypeStore typeStore) {
.collect(Collectors.toList()));
}

protected abstract MethodDefinition createOperationCallableMethod(TypeStore typeStore);
protected abstract MethodDefinition createOperationCallableMethod(
Service service, TypeStore typeStore);

protected MethodDefinition createGenericCallableMethod(
Service service,
TypeStore typeStore,
List<String> methodTemplateNames,
String returnCallableKindName,
Expand All @@ -194,6 +200,7 @@ protected MethodDefinition createGenericCallableMethod(
String callSettingsVariantName,
List<Object> callSettingsTemplateObjects) {
return createGenericCallableMethod(
service,
typeStore,
methodTemplateNames,
returnCallableKindName,
Expand All @@ -206,6 +213,7 @@ protected MethodDefinition createGenericCallableMethod(
}

protected MethodDefinition createGenericCallableMethod(
Service service,
TypeStore typeStore,
List<String> methodTemplateNames,
String returnCallableKindName,
Expand Down Expand Up @@ -257,7 +265,7 @@ protected MethodDefinition createGenericCallableMethod(
.setVariable(
Variable.builder()
.setName("operationsStub")
.setType(getTransportContext().operationsStubTypes().get(0))
.setType(getOperationsStubType(service))
.build())
.setIsDecl(true)
.build());
Expand Down Expand Up @@ -288,7 +296,16 @@ protected MethodDefinition createGenericCallableMethod(
.build();
}

private static TypeStore createTypes() {
protected TypeNode getOperationsStubType(Service service) {
TypeNode opeationsStubType = service.operationServiceStubType();
if (opeationsStubType == null) {
opeationsStubType = getTransportContext().operationsStubTypes().get(0);
}
return opeationsStubType;
}


private TypeStore createTypes(Service service) {
List<Class<?>> concreteClazzes =
Arrays.asList(
// Gax-java classes.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public GapicClass generate(GapicContext context, Service service) {
String className = ClassNames.getServiceClientClassName(service);
GapicClass.Kind kind = Kind.MAIN;
String pakkage = service.pakkage();
boolean hasLroClient = service.hasLroMethods();
boolean hasLroClient = service.hasStandardLroMethods();

Map<String, List<String>> grpcRpcsToJavaMethodNames = new HashMap<>();

Expand Down Expand Up @@ -741,6 +741,7 @@ private static MethodDefinition createMethodDefaultMethod(
method.isPaged()
? typeStore.get(String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, method.name()))
: method.outputType();
List<AnnotationNode> annotations = new ArrayList<>();
if (method.hasLro()) {
LongrunningOperation lro = method.lro();
methodOutputType =
Expand All @@ -751,6 +752,13 @@ private static MethodDefinition createMethodDefaultMethod(
.copyAndSetGenerics(
Arrays.asList(
lro.responseType().reference(), lro.metadataType().reference())));
if (method.hasLro() && method.lro().operationServiceStubType() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already checked method.hasLro().

Suggested change
if (method.hasLro() && method.lro().operationServiceStubType() != null) {
if (lro.operationServiceStubType() != null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that was redundant. Removed

annotations.add(
AnnotationNode.withTypeAndDescription(
typeStore.get("BetaApi"),
"The surface for long-running operations is not stable yet and may change in the"
+ " future."));
}
}

// Construct the method that accepts a request proto.
Expand Down Expand Up @@ -792,8 +800,7 @@ private static MethodDefinition createMethodDefaultMethod(
.setArguments(Arrays.asList(requestArgVarExpr));

if (method.isDeprecated()) {
methodBuilder =
methodBuilder.setAnnotations(Arrays.asList(AnnotationNode.withType(TypeNode.DEPRECATED)));
annotations.add(AnnotationNode.withType(TypeNode.DEPRECATED));
}

if (isProtoEmptyType(methodOutputType)) {
Expand All @@ -805,6 +812,9 @@ private static MethodDefinition createMethodDefaultMethod(
methodBuilder =
methodBuilder.setReturnExpr(callableMethodExpr).setReturnType(methodOutputType);
}

methodBuilder.setAnnotations(annotations);

return methodBuilder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@
import com.google.api.gax.rpc.BidiStreamingCallable;
import com.google.api.gax.rpc.ClientStreamingCallable;
import com.google.api.gax.rpc.InvalidArgumentException;
import com.google.api.gax.rpc.PagedCallSettings;
import com.google.api.gax.rpc.ServerStreamingCallSettings;
import com.google.api.gax.rpc.ServerStreamingCallable;
import com.google.api.gax.rpc.StatusCode;
import com.google.api.gax.rpc.StreamingCallSettings;
import com.google.api.gax.rpc.UnaryCallSettings;
import com.google.api.generator.engine.ast.AnnotationNode;
import com.google.api.generator.engine.ast.AssignmentExpr;
import com.google.api.generator.engine.ast.ClassDefinition;
Expand Down Expand Up @@ -54,6 +58,7 @@
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.MethodArgument;
import com.google.api.generator.gapic.model.OperationResponse;
import com.google.api.generator.gapic.model.ResourceName;
import com.google.api.generator.gapic.model.Service;
import com.google.api.generator.gapic.utils.JavaStyle;
Expand Down Expand Up @@ -408,7 +413,10 @@ private MethodDefinition createRpcTestMethod(
.setValueExpr(expectedResponseValExpr)
.build());

if (method.hasLro()) {
if (method.hasLro()
&& (method.lro().operationServiceStubType() == null
|| !method.lro().responseType().equals(method.outputType()))) {

VariableExpr resultOperationVarExpr =
VariableExpr.withVariable(
Variable.builder()
Expand Down Expand Up @@ -905,6 +913,46 @@ private void addDynamicTypes(GapicContext context, Service service, TypeStore ty
}
}

private static TypeNode getCallSettingsTypeHelper(
Method protoMethod, TypeStore typeStore, boolean isBuilder) {
Class callSettingsClazz = isBuilder ? UnaryCallSettings.Builder.class : UnaryCallSettings.class;
if (protoMethod.isPaged()) {
callSettingsClazz = isBuilder ? PagedCallSettings.Builder.class : PagedCallSettings.class;
} else {
switch (protoMethod.stream()) {
case CLIENT:
// Fall through.
case BIDI:
callSettingsClazz =
isBuilder ? StreamingCallSettings.Builder.class : StreamingCallSettings.class;
break;
case SERVER:
callSettingsClazz =
isBuilder
? ServerStreamingCallSettings.Builder.class
: ServerStreamingCallSettings.class;
break;
case NONE:
// Fall through
default:
// Fall through
}
}

List<Reference> generics = new ArrayList<>();
generics.add(protoMethod.inputType().reference());
generics.add(protoMethod.outputType().reference());
if (protoMethod.isPaged()) {
generics.add(
typeStore
.get(String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, protoMethod.name()))
.reference());
}

return TypeNode.withReference(
ConcreteReference.builder().setClazz(callSettingsClazz).setGenerics(generics).build());
}

protected static TypeNode getCallableType(Method protoMethod) {
Preconditions.checkState(
!protoMethod.stream().equals(Method.Stream.NONE),
Expand Down