Repository navigation
feat(generator): wire transport stub delegation to resumable upload stubs - #14304
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for resumable uploads in the GAPIC generator for Java by adding delegation to HttpJsonServiceResumableUploadStub in both gRPC and HTTP/REST transport stub composers, along with corresponding unit tests. The review feedback focuses on refactoring opportunities to simplify the generated code and method signatures. Specifically, the reviewer suggests removing the unused typeStore parameter from createResumableUploadCallableGetterMethods and passing the resolved resumableUploadStubVarExpr directly to createStubOverrideMethods and the close() method generation to avoid redundant variable recreation and condition checks.
| createStubOverrideMethods( | ||
| classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service)); | ||
| classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service, typeStore)); |
There was a problem hiding this comment.
Simplify the call to createStubOverrideMethods by passing the already resolved resumableUploadStubVarExpr from classMemberVarExprs instead of typeStore. This avoids recreating the variable expression and re-evaluating the resumable upload condition inside the method.
| createStubOverrideMethods( | |
| classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service)); | |
| classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), service, typeStore)); | |
| createStubOverrideMethods( | |
| classMemberVarExprs.get(BACKGROUND_RESOURCES_MEMBER_NAME), | |
| service, | |
| classMemberVarExprs.get(RESUMABLE_UPLOAD_STUB_MEMBER_NAME))); |
| javaMethods.addAll(createCallableGetterMethods(callableClassMemberVarExprs)); | ||
| if (service.methods().stream().anyMatch(Method::isResumableUpload)) { | ||
| javaMethods.addAll( | ||
| createResumableUploadCallableGetterMethods(service, typeStore, classMemberVarExprs)); |
There was a problem hiding this comment.
Remove the unused typeStore parameter from the call to createResumableUploadCallableGetterMethods.
| createResumableUploadCallableGetterMethods(service, typeStore, classMemberVarExprs)); | |
| createResumableUploadCallableGetterMethods(service, classMemberVarExprs)); |
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | ||
| Service service, TypeStore typeStore, Map<String, VariableExpr> classMemberVarExprs) { |
There was a problem hiding this comment.
Remove the unused TypeStore typeStore parameter from the createResumableUploadCallableGetterMethods method signature.
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | |
| Service service, TypeStore typeStore, Map<String, VariableExpr> classMemberVarExprs) { | |
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | |
| Service service, Map<String, VariableExpr> classMemberVarExprs) { |
| private List<MethodDefinition> createStubOverrideMethods( | ||
| VariableExpr backgroundResourcesVarExpr, Service service) { | ||
| VariableExpr backgroundResourcesVarExpr, Service service, TypeStore typeStore) { |
There was a problem hiding this comment.
Update the signature of createStubOverrideMethods to accept the resumableUploadStubVarExpr directly instead of TypeStore typeStore.
| private List<MethodDefinition> createStubOverrideMethods( | |
| VariableExpr backgroundResourcesVarExpr, Service service) { | |
| VariableExpr backgroundResourcesVarExpr, Service service, TypeStore typeStore) { | |
| private List<MethodDefinition> createStubOverrideMethods( | |
| VariableExpr backgroundResourcesVarExpr, | |
| Service service, | |
| VariableExpr resumableUploadStubVarExpr) { |
| if (service.methods().stream().anyMatch(Method::isResumableUpload)) { | ||
| VariableExpr resumableUploadStubVarExpr = | ||
| VariableExpr.withVariable( | ||
| Variable.builder() | ||
| .setName(RESUMABLE_UPLOAD_STUB_MEMBER_NAME) | ||
| .setType( | ||
| typeStore.get(String.format("HttpJson%sResumableUploadStub", service.name()))) | ||
| .build()); | ||
| Statement closeResumableUploadStubStmt = | ||
| ExprStatement.withExpr( | ||
| MethodInvocationExpr.builder() | ||
| .setExprReferenceExpr(resumableUploadStubVarExpr) | ||
| .setMethodName("close") | ||
| .build()); | ||
| if (isResumableUploadStubNullable()) { | ||
| tryBodyStatements.add( | ||
| IfStatement.builder() | ||
| .setConditionExpr( | ||
| RelationalOperationExpr.notEqualToWithExprs( | ||
| resumableUploadStubVarExpr, ValueExpr.createNullExpr())) | ||
| .setBody(Arrays.asList(closeResumableUploadStubStmt)) | ||
| .build()); | ||
| } else { | ||
| tryBodyStatements.add(closeResumableUploadStubStmt); | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify the close() method generation by using the passed resumableUploadStubVarExpr directly and checking if it is non-null, rather than re-evaluating the service methods and recreating the variable expression.
if (resumableUploadStubVarExpr != null) {
Statement closeResumableUploadStubStmt =
ExprStatement.withExpr(
MethodInvocationExpr.builder()
.setExprReferenceExpr(resumableUploadStubVarExpr)
.setMethodName("close")
.build());
if (isResumableUploadStubNullable()) {
tryBodyStatements.add(
IfStatement.builder()
.setConditionExpr(
RelationalOperationExpr.notEqualToWithExprs(
resumableUploadStubVarExpr, ValueExpr.createNullExpr()))
.setBody(Arrays.asList(closeResumableUploadStubStmt))
.build());
} else {
tryBodyStatements.add(closeResumableUploadStubStmt);
}
}References
- When using lazily initialized resources (such as ExecutorService), ensure that teardown or close methods perform explicit null checks before invoking methods on them to prevent NullPointerException.
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | ||
| Service service, TypeStore typeStore, Map<String, VariableExpr> classMemberVarExprs) { |
There was a problem hiding this comment.
Remove the unused TypeStore typeStore parameter from the overridden createResumableUploadCallableGetterMethods method signature to match the base class.
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | |
| Service service, TypeStore typeStore, Map<String, VariableExpr> classMemberVarExprs) { | |
| @Override | |
| protected List<MethodDefinition> createResumableUploadCallableGetterMethods( | |
| Service service, Map<String, VariableExpr> classMemberVarExprs) { |
8cb5081 to
e8fec50
Compare
1169a63 to
daf1b64
Compare
daf1b64
into
whowes/showcase-baseline-resumable-upload
|
|
❌ The last analysis has failed. |
1 participant
Work in progress, not ready for review