Skip to content

chore(spanner): extract ErrorDetails from grpc-status-details-bin for raw gRPC exceptions #13320

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

Open
sakthivelmanii wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore(spanner): extract ErrorDetails from grpc-status-details-bin for raw gRPC exceptions #13320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 13 additions & 2 deletions ...spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,21 @@ enum DoNotConstructDirectly {
* retry delay.
*/
public long getRetryDelayInMillis() {
return extractRetryDelay(this.getCause());
return extractRetryDelay(this, this.apiException);
}

static long extractRetryDelay(Throwable cause) {
return extractRetryDelay(cause, null);
}

static long extractRetryDelay(Throwable cause, @Nullable ApiException apiException) {
ErrorDetails details = SpannerExceptionFactory.extractErrorDetails(cause, apiException);
if (details != null && details.getRetryInfo() != null) {
RetryInfo retryInfo = details.getRetryInfo();
if (retryInfo.hasRetryDelay()) {
return Durations.toMillis(retryInfo.getRetryDelay());
}
}
if (cause != null) {
Metadata trailers = Status.trailersFromThrowable(cause);
if (trailers != null && trailers.containsKey(KEY_RETRY_INFO)) {
Expand Down Expand Up @@ -227,7 +238,7 @@ public ErrorDetails getErrorDetails() {
if (this.apiException != null) {
return this.apiException.getErrorDetails();
}
return null;
return SpannerExceptionFactory.extractErrorDetails(getCause(), null);
}

void setStatement(String statement) {
Expand Down
32 changes: 28 additions & 4 deletions .../google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.cloud.spanner.SpannerException.DoNotConstructDirectly;
import com.google.common.base.MoreObjects;
import com.google.common.base.Predicate;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.rpc.ErrorInfo;
import com.google.rpc.ResourceInfo;
import com.google.rpc.RetryInfo;
Expand Down Expand Up @@ -56,6 +57,8 @@ public final class SpannerExceptionFactory {
ProtoUtils.keyForProto(ResourceInfo.getDefaultInstance());
private static final Metadata.Key<ErrorInfo> KEY_ERROR_INFO =
ProtoUtils.keyForProto(ErrorInfo.getDefaultInstance());
private static final Metadata.Key<byte[]> KEY_ERROR_DETAILS =
Metadata.Key.of("grpc-status-details-bin", Metadata.BINARY_BYTE_MARSHALLER);

public static SpannerException newSpannerException(ErrorCode code, @Nullable String message) {
return newSpannerException(code, message, null);
Expand Down Expand Up @@ -247,6 +250,10 @@ private static String formatMessage(ErrorCode code, @Nullable String message) {
}

private static ResourceInfo extractResourceInfo(Throwable cause) {
ErrorDetails details = extractErrorDetails(cause, null);
if (details != null && details.getResourceInfo() != null) {
return details.getResourceInfo();
}
if (cause != null) {
Metadata trailers = Status.trailersFromThrowable(cause);
if (trailers != null) {
Expand All @@ -257,8 +264,9 @@ private static ResourceInfo extractResourceInfo(Throwable cause) {
}

private static ErrorInfo extractErrorInfo(Throwable cause, ApiException apiException) {
if (apiException != null && apiException.getErrorDetails() != null) {
return apiException.getErrorDetails().getErrorInfo();
ErrorDetails details = extractErrorDetails(cause, apiException);
if (details != null && details.getErrorInfo() != null) {
return details.getErrorInfo();
}
if (cause != null) {
Metadata trailers = Status.trailersFromThrowable(cause);
Expand All @@ -277,10 +285,26 @@ static ErrorDetails extractErrorDetails(Throwable cause, ApiException apiExcepti
Throwable prevCause = null;
while (cause != null && cause != prevCause) {
Comment on lines 285 to 286
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

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

high

The apiException parameter passed to extractErrorDetails is currently completely unused in the method body. This leads to a correctness bug where error details are not extracted from the apiException when cause is not a SpannerException (for example, in extractErrorInfo).

We should check apiException at the beginning of the method and return its error details if present.

    if (apiException != null && apiException.getErrorDetails() != null) {
      return apiException.getErrorDetails();
    }
    Throwable prevCause = null;
    while (cause != null && cause != prevCause) {

Copy link
Copy Markdown
Contributor Author

@sakthivelmanii sakthivelmanii Jun 2, 2026

Choose a reason for hiding this comment

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

apiException is used

if (cause instanceof ApiException) {
return ((ApiException) cause).getErrorDetails();
if (((ApiException) cause).getErrorDetails() != null) {
return ((ApiException) cause).getErrorDetails();
}
}
if (cause instanceof SpannerException) {
return ((SpannerException) cause).getErrorDetails();
if (((SpannerException) cause).getErrorDetails() != null) {
return ((SpannerException) cause).getErrorDetails();
}
}
Metadata trailers = Status.trailersFromThrowable(cause);
if (trailers != null) {
byte[] bytes = trailers.get(KEY_ERROR_DETAILS);
if (bytes != null) {
try {
com.google.rpc.Status status = com.google.rpc.Status.parseFrom(bytes);
return ErrorDetails.builder().setRawErrorMessages(status.getDetailsList()).build();
} catch (InvalidProtocolBufferException e) {
// ignore and continue
}
}
}
prevCause = cause;
cause = cause.getCause();
Expand Down
59 changes: 0 additions & 59 deletions ...r/google-cloud-spanner/src/test/java/com/google/cloud/spanner/ITTransactionRetryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@
import static com.google.cloud.spanner.testing.EmulatorSpannerHelper.isUsingEmulator;
import static com.google.cloud.spanner.testing.SpannerOmniHelper.isSpannerOmni;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeFalse;
import static org.junit.Assume.assumeTrue;

import org.junit.ClassRule;
import org.junit.Test;
Expand All @@ -39,12 +37,7 @@ public class ITTransactionRetryTest {
@Test
public void TestRetryInfo() {
assumeFalse("emulator does not support parallel transaction", isUsingEmulator());
// TODO(sakthivelmani) - Re-enable once b/422916293 is resolved
assumeFalse(
"Skipping the test due to a known bug b/422916293",
env.getTestHelper().getOptions().isEnableDirectAccess());
assumeFalse("Skipping the test due to a known bug b/422916293", isSpannerOmni());

// Creating a database with the table which contains INT64 columns
Database db =
env.getTestHelper()
Expand Down Expand Up @@ -88,56 +81,4 @@ public void TestRetryInfo() {

assertTrue("Transaction is not aborted with the trailers", isAbortedWithRetryInfo);
}

@Test
public void TestRetryInfoWithDirectPath() {
assumeFalse("emulator does not support parallel transaction", isUsingEmulator());
// TODO(sakthivelmani) - Re-enable once b/422916293 is resolved
assumeTrue(
"Enabling this test due to bug b/422916293",
env.getTestHelper().getOptions().isEnableDirectAccess());

// Creating a database with the table which contains INT64 columns
Database db =
env.getTestHelper()
.createTestDatabase("CREATE TABLE Test(ID INT64, " + "EMPID INT64) PRIMARY KEY (ID)");
DatabaseClient databaseClient = env.getTestHelper().getClient().getDatabaseClient(db.getId());

// Inserting one row
databaseClient
.readWriteTransaction()
.run(
transaction -> {
transaction.buffer(
Mutation.newInsertBuilder("Test").set("ID").to(1).set("EMPID").to(1).build());
return null;
});

int numRetries = 10;
boolean isAbortedWithRetryInfo = false;
while (numRetries-- > 0) {
try (TransactionManager transactionManager1 = databaseClient.transactionManager()) {
try (TransactionManager transactionManager2 = databaseClient.transactionManager()) {
try {
TransactionContext transaction1 = transactionManager1.begin();
TransactionContext transaction2 = transactionManager2.begin();
transaction1.executeUpdate(
Statement.of("UPDATE Test SET EMPID = EMPID + 1 WHERE ID = 1"));
transaction2.executeUpdate(
Statement.of("UPDATE Test SET EMPID = EMPID + 1 WHERE ID = 1"));
transactionManager1.commit();
transactionManager2.commit();
} catch (AbortedException abortedException) {
assertThat(abortedException.getErrorCode()).isEqualTo(ErrorCode.ABORTED);
if (abortedException.getRetryDelayInMillis() > 0) {
isAbortedWithRetryInfo = true;
break;
}
}
}
}
}

assertFalse("Transaction is aborted with the trailers", isAbortedWithRetryInfo);
}
}
25 changes: 25 additions & 0 deletions ...gle-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerExceptionFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,29 @@ private Metadata createResourceTypeMetadata(String resourceType, String resource

return trailers;
}

@Test
public void resourceExhaustedWithGrpcStatusDetails() {
Status status =
Status.fromCodeValue(Status.Code.RESOURCE_EXHAUSTED.value())
.withDescription("Memory pushback");
Metadata trailers = new Metadata();
Metadata.Key<byte[]> key =
Metadata.Key.of("grpc-status-details-bin", Metadata.BINARY_BYTE_MARSHALLER);
RetryInfo retryInfo =
RetryInfo.newBuilder()
.setRetryDelay(Duration.newBuilder().setNanos(1000000).setSeconds(1L))
.build();
com.google.rpc.Status rpcStatus =
com.google.rpc.Status.newBuilder()
.setCode(com.google.rpc.Code.RESOURCE_EXHAUSTED_VALUE)
.setMessage("Memory pushback")
.addDetails(com.google.protobuf.Any.pack(retryInfo))
.build();
trailers.put(key, rpcStatus.toByteArray());
SpannerException e =
SpannerExceptionFactory.newSpannerException(new StatusRuntimeException(status, trailers));
assertThat(e.isRetryable()).isTrue();
assertThat(e.getRetryDelayInMillis()).isEqualTo(1001);
}
}
12 changes: 0 additions & 12 deletions .../src/test/java/com/google/cloud/spanner/connection/it/ITRetryDmlAsPartitionedDmlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,6 @@ public static void setupTestData() {

@Test
public void testDmlFailsIfMutationLimitExceeded() {
// TODO(sakthivelmani) - Re-enable once b/422916293 is resolved
assumeFalse(
"Skipping the test due to a known bug b/422916293",
env.getTestHelper().getOptions().isEnableDirectAccess());
try (Connection connection = createConnection()) {
connection.setAutocommit(true);
assertThrows(
Expand All @@ -104,10 +100,6 @@ public void testDmlFailsIfMutationLimitExceeded() {

@Test
public void testRetryDmlAsPartitionedDml() throws Exception {
// TODO(sakthivelmani) - Re-enable once b/422916293 is resolved
assumeFalse(
"Skipping the test due to a known bug b/422916293",
env.getTestHelper().getOptions().isEnableDirectAccess());
try (Connection connection = createConnection()) {
connection.setAutocommit(true);
connection.setAutocommitDmlMode(
Expand Down Expand Up @@ -148,10 +140,6 @@ public void retryDmlAsPartitionedDmlFinished(

@Test
public void testRetryDmlAsPartitionedDml_failsForLargeInserts() throws Exception {
// TODO(sakthivelmani) - Re-enable once b/422916293 is resolved
assumeFalse(
"Skipping the test due to a known bug b/422916293",
env.getTestHelper().getOptions().isEnableDirectAccess());
try (Connection connection = createConnection()) {
connection.setAutocommit(true);
connection.setAutocommitDmlMode(
Expand Down
Loading