-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(gax): propagate structured LRO error details to ApiException #14022
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
base: main
Are you sure you want to change the base?
Changes from all commits
97672a9
5329849
f6913df
5607dae
0fbf1ba
7e2c12b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,19 @@ | |
|
|
||
| import com.google.api.gax.longrunning.OperationFuture; | ||
| import com.google.api.gax.retrying.RetrySettings; | ||
| import com.google.api.gax.rpc.ApiException; | ||
| import com.google.protobuf.Any; | ||
| import com.google.protobuf.Timestamp; | ||
| import com.google.rpc.Code; | ||
| import com.google.rpc.Status; | ||
| import com.google.showcase.v1beta1.EchoClient; | ||
| import com.google.showcase.v1beta1.PoetryError; | ||
| import com.google.showcase.v1beta1.WaitMetadata; | ||
| import com.google.showcase.v1beta1.WaitRequest; | ||
| import com.google.showcase.v1beta1.WaitResponse; | ||
| import com.google.showcase.v1beta1.it.util.TestClientInitializer; | ||
| import java.util.concurrent.CancellationException; | ||
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.threeten.bp.Duration; | ||
|
|
@@ -193,4 +199,35 @@ void testHttpJson_LROUnsuccessfulResponse_exceedsTotalTimeout_throwsDeadlineExce | |
| TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void testGRPC_LROErrorResponse_propagatesErrorDetails() throws Exception { | ||
| EchoClient grpcClient = TestClientInitializer.createGrpcEchoClient(); | ||
| try { | ||
| PoetryError poetryError = | ||
| PoetryError.newBuilder().setPoem("Roses are red, violets are blue").build(); | ||
| Status status = | ||
| Status.newBuilder() | ||
| .setCode(Code.ALREADY_EXISTS_VALUE) | ||
| .setMessage("The resource already exists") | ||
| .addDetails(Any.pack(poetryError)) | ||
| .build(); | ||
|
Comment on lines
+204
to
+214
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I know we're blocking on the ErrorDetails Showcase test. I think these tests may be better suited in that file. Let's have this live here for now and then we can move it over there in the future. |
||
| WaitRequest waitRequest = WaitRequest.newBuilder().setError(status).build(); | ||
| OperationFuture<WaitResponse, WaitMetadata> operationFuture = | ||
| grpcClient.waitOperationCallable().futureCall(waitRequest); | ||
| ExecutionException exception = assertThrows(ExecutionException.class, operationFuture::get); | ||
|
nnicolee marked this conversation as resolved.
|
||
| assertThat(exception.getCause()).isInstanceOf(ApiException.class); | ||
| ApiException apiException = (ApiException) exception.getCause(); | ||
|
|
||
| // Verify that error details are successfully propagated | ||
| assertThat(apiException.getErrorDetails()).isNotNull(); | ||
| PoetryError unpackedError = apiException.getErrorDetails().getMessage(PoetryError.class); | ||
| assertThat(unpackedError).isNotNull(); | ||
| assertThat(unpackedError.getPoem()).isEqualTo("Roses are red, violets are blue"); | ||
| } finally { | ||
| grpcClient.close(); | ||
| grpcClient.awaitTermination( | ||
| TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,11 @@ | |
| package com.google.api.gax.grpc; | ||
|
|
||
| import com.google.api.gax.longrunning.OperationSnapshot; | ||
| import com.google.api.gax.rpc.ErrorDetails; | ||
| import com.google.api.gax.rpc.StatusCode; | ||
| import com.google.longrunning.Operation; | ||
| import io.grpc.Status; | ||
| import java.util.Collections; | ||
| import org.jspecify.annotations.NullMarked; | ||
|
|
||
| /** | ||
|
|
@@ -79,6 +81,16 @@ public String getErrorMessage() { | |
| return operation.getError().getMessage(); | ||
| } | ||
|
|
||
| @Override | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also here mark the javadoc with |
||
| public ErrorDetails getErrorDetails() { | ||
| if (operation.hasError() && operation.getError().getDetailsCount() > 0) { | ||
| return ErrorDetails.builder() | ||
| .setRawErrorMessages(operation.getError().getDetailsList()) | ||
| .build(); | ||
| } | ||
| return ErrorDetails.builder().setRawErrorMessages(Collections.emptyList()).build(); | ||
|
Comment on lines
+86
to
+91
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the if block here? Can we just do something like
|
||
| } | ||
|
|
||
| public static GrpcOperationSnapshot create(Operation operation) { | ||
| return new GrpcOperationSnapshot(operation); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,10 +31,13 @@ | |
|
|
||
| import com.google.api.core.InternalApi; | ||
| import com.google.api.gax.longrunning.OperationSnapshot; | ||
| import com.google.api.gax.rpc.ErrorDetails; | ||
| import com.google.api.gax.rpc.StatusCode; | ||
| import com.google.api.gax.rpc.StatusCode.Code; | ||
| import com.google.longrunning.Operation; | ||
| import java.util.Collections; | ||
| import org.jspecify.annotations.NullMarked; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** | ||
| * Implementation of OperationSnapshot based on REST transport. | ||
|
|
@@ -50,20 +53,23 @@ public class HttpJsonOperationSnapshot implements OperationSnapshot { | |
| private final Object response; | ||
| private final StatusCode errorCode; | ||
| private final String errorMessage; | ||
| private final ErrorDetails errorDetails; | ||
|
|
||
| private HttpJsonOperationSnapshot( | ||
| String name, | ||
| Object metadata, | ||
| boolean done, | ||
| Object response, | ||
| StatusCode errorCode, | ||
| String errorMessage) { | ||
| String errorMessage, | ||
| ErrorDetails errorDetails) { | ||
| this.name = name; | ||
| this.metadata = metadata; | ||
| this.done = done; | ||
| this.response = response; | ||
| this.errorCode = errorCode; | ||
| this.errorMessage = errorMessage; | ||
| this.errorDetails = errorDetails; | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
|
|
@@ -102,6 +108,11 @@ public String getErrorMessage() { | |
| return this.errorMessage; | ||
| } | ||
|
|
||
| @Override | ||
| public ErrorDetails getErrorDetails() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you mark this javadoc with |
||
| return this.errorDetails; | ||
| } | ||
|
|
||
| public static HttpJsonOperationSnapshot create(Operation operation) { | ||
| return newBuilder().setOperation(operation).build(); | ||
| } | ||
|
|
@@ -117,6 +128,22 @@ public static class Builder { | |
| private Object response; | ||
| private StatusCode errorCode; | ||
| private String errorMessage; | ||
| private ErrorDetails errorDetails = | ||
| ErrorDetails.builder().setRawErrorMessages(Collections.emptyList()).build(); | ||
|
|
||
| /** | ||
| * Sets the LRO error details. | ||
| * | ||
| * @param errorDetails the LRO error details | ||
| * @return the builder instance | ||
| */ | ||
| public Builder setErrorDetails(final @Nullable ErrorDetails errorDetails) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this is only for testing right? If so, can we make this package-private and enforce that ErrorDetails cannot be nullable?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other question with this: I don't think I see a gRPC variant for this. Do we need to expose this setter? |
||
| this.errorDetails = | ||
| errorDetails != null | ||
| ? errorDetails | ||
| : ErrorDetails.builder().setRawErrorMessages(Collections.emptyList()).build(); | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setName(String name) { | ||
| this.name = name; | ||
|
|
@@ -153,11 +180,21 @@ private Builder setOperation(Operation operation) { | |
| this.errorCode = | ||
| HttpJsonStatusCode.of(com.google.rpc.Code.forNumber(operation.getError().getCode())); | ||
| this.errorMessage = operation.getError().getMessage(); | ||
| if (operation.hasError() && operation.getError().getDetailsCount() > 0) { | ||
| this.errorDetails = | ||
| ErrorDetails.builder() | ||
| .setRawErrorMessages(operation.getError().getDetailsList()) | ||
| .build(); | ||
| } else { | ||
| this.errorDetails = | ||
| ErrorDetails.builder().setRawErrorMessages(Collections.emptyList()).build(); | ||
| } | ||
|
nnicolee marked this conversation as resolved.
|
||
| return this; | ||
| } | ||
|
|
||
| public HttpJsonOperationSnapshot build() { | ||
| return new HttpJsonOperationSnapshot(name, metadata, done, response, errorCode, errorMessage); | ||
| return new HttpJsonOperationSnapshot( | ||
| name, metadata, done, response, errorCode, errorMessage, errorDetails); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know parsing HttpJson is a bit more involved/ difficult since we need to manually unpack the Any proto. Would it be possible to also add a HttpJson variant as well?