feat: lro compute poc - #14051
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for propagating custom Long-Running Operation (LRO) error details across both gRPC and HTTP/JSON transports, including the generation of a custom LRO error parser for the Google Cloud Compute service. Feedback on these changes suggests falling back to the snapshot's error details if the custom parser returns null to prevent losing transport-level errors. Additionally, the parenthesis-wrapping logic in JavaWriterVisitor should be expanded to include cast and arithmetic expressions to avoid operator precedence bugs, and the hardcoded package checks for the Compute service in the composer classes should be refactored to use a more decoupled, metadata-driven approach.
| if (errorParser != null && operationSnapshot.getResponse() != null) { | ||
| details = errorParser.parse(operationSnapshot.getResponse()); | ||
| String parsedMsg = errorParser.parseErrorMessage(operationSnapshot.getResponse()); | ||
| if (parsedMsg != null && !parsedMsg.isEmpty()) { | ||
| errorMessage = parsedMsg; | ||
| } | ||
| } else { | ||
| details = operationSnapshot.getErrorDetails(); | ||
| } |
There was a problem hiding this comment.
If errorParser is configured but returns null (for example, if the response is not of the expected type or the error is a transport-level error rather than an application-level LRO error), the current implementation will result in details being null. This causes the exception to lose any transport-level ErrorDetails that might have been populated in operationSnapshot.getErrorDetails().
To prevent losing error details on fallback/transport errors, we should fall back to operationSnapshot.getErrorDetails() if details is still null after attempting to use the custom parser.
| if (errorParser != null && operationSnapshot.getResponse() != null) { | |
| details = errorParser.parse(operationSnapshot.getResponse()); | |
| String parsedMsg = errorParser.parseErrorMessage(operationSnapshot.getResponse()); | |
| if (parsedMsg != null && !parsedMsg.isEmpty()) { | |
| errorMessage = parsedMsg; | |
| } | |
| } else { | |
| details = operationSnapshot.getErrorDetails(); | |
| } | |
| if (errorParser != null && operationSnapshot.getResponse() != null) { | |
| details = errorParser.parse(operationSnapshot.getResponse()); | |
| String parsedMsg = errorParser.parseErrorMessage(operationSnapshot.getResponse()); | |
| if (parsedMsg != null && !parsedMsg.isEmpty()) { | |
| errorMessage = parsedMsg; | |
| } | |
| } | |
| if (details == null) { | |
| details = operationSnapshot.getErrorDetails(); | |
| } |
| boolean needsParen = unaryOperationExpr.expr() instanceof InstanceofExpr | ||
| || unaryOperationExpr.expr() instanceof RelationalOperationExpr | ||
| || unaryOperationExpr.expr() instanceof LogicalOperationExpr; |
There was a problem hiding this comment.
The current logic for needsParen only covers InstanceofExpr, RelationalOperationExpr, and LogicalOperationExpr. However, unary operators (such as logical negation ! or unary minus -) have higher precedence than most other expressions in Java.
To prevent operator precedence bugs or compilation errors, needsParen should also include other expression types that have lower precedence than unary operators, such as CastExpr (e.g., !((Boolean) x) vs !(Boolean) x which is a syntax error) and ArithmeticOperationExpr (e.g., -(a + b) vs -a + b which changes semantics).
boolean needsParen = unaryOperationExpr.expr() instanceof InstanceofExpr
|| unaryOperationExpr.expr() instanceof RelationalOperationExpr
|| unaryOperationExpr.expr() instanceof LogicalOperationExpr
|| unaryOperationExpr.expr() instanceof CastExpr
|| unaryOperationExpr.expr() instanceof ArithmeticOperationExpr;| if (s.pakkage().startsWith("com.google.cloud.compute.v1")) { | ||
| clazzes.add( | ||
| ComputeLroErrorParserClassComposer.instance().generate(context, s)); | ||
| } |
There was a problem hiding this comment.
Hardcoding the package prefix "com.google.cloud.compute.v1" directly in the core Composer class introduces tight coupling to a specific service (Compute) and violates the Open-Closed Principle.
If other services in the future require custom LRO error parsing, this class will have to be modified again. Consider making this behavior configurable or metadata-driven (e.g., via a flag in GapicContext or service configuration) rather than hardcoding package names.
| if (service.pakkage().startsWith("com.google.cloud.compute.v1") && operationResponseTransformer.reference().pakkage().equals("com.google.api.gax.httpjson")) { | ||
| createArgs.add( | ||
| NewObjectExpr.builder() | ||
| .setType( | ||
| TypeNode.withReference( | ||
| VaporReference.builder() | ||
| .setName("ComputeLroErrorParser") | ||
| .setPakkage(service.pakkage() + ".stub") | ||
| .build())) | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
Similar to the change in Composer.java, hardcoding the package prefix "com.google.cloud.compute.v1" here introduces tight coupling to the Compute service.
Consider abstracting this check by introducing a property on the Service or GapicContext model (e.g., service.hasCustomLroErrorParser() or similar) to determine if a custom LRO error parser should be instantiated and passed to the response transformer.
|
|
Overview
This draft PR is a Proof of Concept (POC) implementing Option C (Interface-driven Delegation) for Compute-specific REST LRO error propagation.
It defines a generic error parsing interface in
gax-httpjsonand updates the code generator (gapic-generator-java) to generate and register a type-safe parser implementation in the Compute client stub settings. This avoids runtime reflection overhead while keeping the generator footprint clean.Changes Included
1. GAX Core Changes (
gax-httpjson)HttpJsonLroErrorParser.java: A new interface defining methods for custom LRO error parsing (parseandparseErrorMessage).ProtoOperationTransformers.java: UpdatedResponseTransformerto accept a nullableHttpJsonLroErrorParserinstance. If present, it delegates the unpacking of custom REST LRO error metadata (such as Compute'sErrorslist) into standardErrorDetails.2. Generator Changes (
gapic-generator-java)ComputeLroErrorParserClassComposer.java: A new class composer that writes out the concreteComputeLroErrorParserclass in Compute client stub packages.Composer.java: Triggers the generation ofComputeLroErrorParserspecifically for packages starting withcom.google.cloud.compute.v1.RetrySettingsComposer.java: Modifies response transformer generation to instantiate and pass the newComputeLroErrorParserwhen compiling stub settings.JavaWriterVisitor.java: Adds parenthesis wrapping for unary negation expressions (e.g.!(response instanceof Operation)) to prevent syntax compilation errors in generated output.