Skip to content

Revert "feat: include PostgreSQL error code in exceptions" #4238

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

Closed
wants to merge 1 commit into from
Closed

Revert "feat: include PostgreSQL error code in exceptions" #4238

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
2 changes: 1 addition & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/AsyncRunnerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private <R> R runTransaction(final AsyncWork<R> work) {
try {
return work.doWorkAsync(transaction).get();
} catch (ExecutionException e) {
throw SpannerExceptionFactory.asSpannerException(e.getCause());
throw SpannerExceptionFactory.newSpannerException(e.getCause());
} catch (InterruptedException e) {
throw SpannerExceptionFactory.propagateInterrupt(e);
}
Expand Down
2 changes: 1 addition & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/AsyncTransactionManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private ApiFuture<TransactionContext> internalBeginAsync(
@Override
public void onFailure(Throwable t) {
onError(t);
res.setException(SpannerExceptionFactory.asSpannerException(t));
res.setException(SpannerExceptionFactory.newSpannerException(t));
}

@Override
Expand Down
6 changes: 3 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/GrpcResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.google.cloud.spanner;

import static com.google.cloud.spanner.SpannerExceptionFactory.asSpannerException;
import static com.google.cloud.spanner.SpannerExceptionFactory.newSpannerException;
import static com.google.common.base.Preconditions.checkState;

import com.google.api.core.InternalApi;
Expand Down Expand Up @@ -76,7 +76,7 @@ protected GrpcStruct currRow() {
@Override
public boolean next() throws SpannerException {
if (error != null) {
throw asSpannerException(error);
throw newSpannerException(error);
}
try {
if (currRow == null) {
Expand Down Expand Up @@ -108,7 +108,7 @@ public boolean next() throws SpannerException {
return hasNext;
} catch (Throwable t) {
throw yieldError(
asSpannerException(t),
SpannerExceptionFactory.asSpannerException(t),
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

medium

This change seems inconsistent with the rest of the revert. Other occurrences of asSpannerException are being changed to newSpannerException, but this one is kept and fully qualified. For consistency, shouldn't this also be changed to newSpannerException(t)? The static import for newSpannerException is available.

Suggested change
SpannerExceptionFactory.asSpannerException(t),
newSpannerException(t),

iterator.isWithBeginTransaction() && currRow == null,
iterator.isLastStatement());
}
Expand Down
20 changes: 14 additions & 6 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/GrpcStreamIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ protected final PartialResultSet computeNext() {
call = null;

if (error != null) {
throw SpannerExceptionFactory.asSpannerException(error);
throw SpannerExceptionFactory.newSpannerException(error);
}

endOfData();
Expand Down Expand Up @@ -192,17 +192,25 @@ public void onCompleted() {
}

@Override
public void onError(SpannerException exception) {
public void onError(SpannerException e) {
if (statement != null) {
if (logger.isLoggable(Level.FINEST)) {
// Include parameter values if logging level is set to FINEST or higher.
exception.setStatement(statement.toString());
logger.log(Level.FINEST, "Error executing statement", exception);
e =
SpannerExceptionFactory.newSpannerExceptionPreformatted(
e.getErrorCode(),
String.format("%s - Statement: '%s'", e.getMessage(), statement.toString()),
e);
logger.log(Level.FINEST, "Error executing statement", e);
} else {
exception.setStatement(statement.getSql());
e =
SpannerExceptionFactory.newSpannerExceptionPreformatted(
e.getErrorCode(),
String.format("%s - Statement: '%s'", e.getMessage(), statement.getSql()),
e);
}
}
error = exception;
error = e;
addToStream(END_OF_STREAM);
}

Expand Down
2 changes: 1 addition & 1 deletion ...loud-spanner/src/main/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ private void verifyBeginTransactionWithRWOnMultiplexedSessionAsync(String sessio
}
readWriteBeginTransactionReferenceFuture.set(txn);
} catch (Exception e) {
SpannerException spannerException = SpannerExceptionFactory.asSpannerException(e);
SpannerException spannerException = SpannerExceptionFactory.newSpannerException(e);
// Mark multiplexed sessions for RW as unimplemented and fall back to regular sessions
// if UNIMPLEMENTED is returned.
maybeMarkUnimplementedForRW(spannerException);
Expand Down
2 changes: 1 addition & 1 deletion google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerApiFutures.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public static <T> T getOrNull(ApiFuture<T> future) throws SpannerException {
if (e.getCause() instanceof SpannerException) {
throw (SpannerException) e.getCause();
}
throw SpannerExceptionFactory.asSpannerException(e.getCause());
throw SpannerExceptionFactory.newSpannerException(e.getCause());
} catch (InterruptedException e) {
throw SpannerExceptionFactory.propagateInterrupt(e, null /*TODO: requestId*/);
} catch (CancellationException e) {
Expand Down
26 changes: 0 additions & 26 deletions 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 @@ -55,12 +55,10 @@ public String getResourceName() {
private static final long serialVersionUID = 20150916L;
private static final Metadata.Key<RetryInfo> KEY_RETRY_INFO =
ProtoUtils.keyForProto(RetryInfo.getDefaultInstance());
private static final String PG_ERR_CODE_KEY = "pg_sqlerrcode";

private final ErrorCode code;
private final ApiException apiException;
private XGoogSpannerRequestId requestId;
private String statement;

/** Private constructor. Use {@link SpannerExceptionFactory} to create instances. */
SpannerException(
Expand Down Expand Up @@ -101,31 +99,11 @@ public String getResourceName() {
this.requestId = requestId;
}

@Override
public String getMessage() {
if (this.statement == null) {
return super.getMessage();
}
return String.format("%s - Statement: '%s'", super.getMessage(), this.statement);
}

/** Returns the error code associated with this exception. */
public ErrorCode getErrorCode() {
return code;
}

/**
* Returns the PostgreSQL SQLState error code that is encoded in this exception, or null if this
* {@link SpannerException} does not include a PostgreSQL error code.
*/
public String getPostgreSQLErrorCode() {
ErrorDetails details = getErrorDetails();
if (details == null || details.getErrorInfo() == null) {
return null;
}
return details.getErrorInfo().getMetadataOrDefault(PG_ERR_CODE_KEY, null);
}

public String getRequestId() {
if (requestId == null) {
return "";
Expand Down Expand Up @@ -221,10 +199,6 @@ public ErrorDetails getErrorDetails() {
return null;
}

void setStatement(String statement) {
this.statement = statement;
}

/** Sets the requestId. */
@InternalApi
public void setRequestId(XGoogSpannerRequestId reqId) {
Expand Down
15 changes: 4 additions & 11 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 @@ -297,10 +297,7 @@ private static ResourceInfo extractResourceInfo(Throwable cause) {
return null;
}

private static ErrorInfo extractErrorInfo(Throwable cause, ApiException apiException) {
if (apiException != null && apiException.getErrorDetails() != null) {
return apiException.getErrorDetails().getErrorInfo();
}
private static ErrorInfo extractErrorInfo(Throwable cause) {
if (cause != null) {
Metadata trailers = Status.trailersFromThrowable(cause);
if (trailers != null) {
Expand All @@ -310,11 +307,7 @@ private static ErrorInfo extractErrorInfo(Throwable cause, ApiException apiExcep
return null;
}

static ErrorDetails extractErrorDetails(Throwable cause, ApiException apiException) {
if (apiException != null && apiException.getErrorDetails() != null) {
return apiException.getErrorDetails();
}

static ErrorDetails extractErrorDetails(Throwable cause) {
Throwable prevCause = null;
while (cause != null && cause != prevCause) {
if (cause instanceof ApiException) {
Expand Down Expand Up @@ -363,7 +356,7 @@ static SpannerException newSpannerExceptionPreformatted(
case ABORTED:
return new AbortedException(token, message, cause, apiException, reqId);
case RESOURCE_EXHAUSTED:
ErrorInfo info = extractErrorInfo(cause, apiException);
ErrorInfo info = extractErrorInfo(cause);
if (info != null
&& info.getMetadataMap()
.containsKey(AdminRequestsPerMinuteExceededException.ADMIN_REQUESTS_LIMIT_KEY)
Expand All @@ -389,7 +382,7 @@ static SpannerException newSpannerExceptionPreformatted(
}
}
case INVALID_ARGUMENT:
if (isTransactionMutationLimitException(cause, apiException)) {
if (isTransactionMutationLimitException(cause)) {
return new TransactionMutationLimitExceededException(
token, code, message, cause, apiException, reqId);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ner/src/main/java/com/google/cloud/spanner/TransactionMutationLimitExceededException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ static boolean isTransactionMutationLimitException(ErrorCode code, String messag
return code == ErrorCode.INVALID_ARGUMENT && message != null && message.contains(ERROR_MESSAGE);
}

static boolean isTransactionMutationLimitException(Throwable cause, ApiException apiException) {
static boolean isTransactionMutationLimitException(Throwable cause) {
if (cause == null
|| cause.getMessage() == null
|| !cause.getMessage().contains(ERROR_MESSAGE)) {
Expand All @@ -53,7 +53,7 @@ static boolean isTransactionMutationLimitException(Throwable cause, ApiException
// was that the transaction mutation limit was exceeded. We use that here to identify the error,
// as there is no other specific metadata in the error that identifies it (other than the error
// message).
ErrorDetails errorDetails = extractErrorDetails(cause, apiException);
ErrorDetails errorDetails = extractErrorDetails(cause);
if (errorDetails != null && errorDetails.getHelp() != null) {
return errorDetails.getHelp().getLinksCount() == 1
&& errorDetails
Expand Down
6 changes: 3 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ void ensureTxn() {
try {
ensureTxnAsync().get();
} catch (ExecutionException e) {
throw SpannerExceptionFactory.asSpannerException(e.getCause() == null ? e : e.getCause());
throw SpannerExceptionFactory.newSpannerException(e.getCause() == null ? e : e.getCause());
} catch (InterruptedException e) {
throw SpannerExceptionFactory.propagateInterrupt(e);
}
Expand Down Expand Up @@ -370,7 +370,7 @@ void commit() {
throw SpannerExceptionFactory.propagateTimeout((TimeoutException) e);
}
} catch (ExecutionException e) {
throw SpannerExceptionFactory.asSpannerException(e.getCause() == null ? e : e.getCause());
throw SpannerExceptionFactory.newSpannerException(e.getCause() == null ? e : e.getCause());
}
}

Expand Down Expand Up @@ -679,7 +679,7 @@ options, getPreviousTransactionId())))
aborted = true;
}
}
throw SpannerExceptionFactory.asSpannerException(e.getCause());
throw SpannerExceptionFactory.newSpannerException(e.getCause());
} catch (TimeoutException e) {
// Throw an ABORTED exception to force a retry of the transaction if no transaction
// has been returned by the first statement.
Expand Down
4 changes: 2 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/PartitionId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected Class<?> resolveClass(ObjectStreamClass desc)
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT, invalidClassException.getMessage(), invalidClassException);
} catch (Exception exception) {
throw SpannerExceptionFactory.asSpannerException(exception);
throw SpannerExceptionFactory.newSpannerException(exception);
}
}

Expand All @@ -90,7 +90,7 @@ public static String encodeToString(BatchTransactionId transactionId, Partition
new ObjectOutputStream(new GZIPOutputStream(byteArrayOutputStream))) {
objectOutputStream.writeObject(id);
} catch (Exception exception) {
throw SpannerExceptionFactory.asSpannerException(exception);
throw SpannerExceptionFactory.newSpannerException(exception);
}
return Base64.getUrlEncoder().encodeToString(byteArrayOutputStream.toByteArray());
}
Expand Down
15 changes: 5 additions & 10 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.google.cloud.spanner.spi.v1;

import static com.google.cloud.spanner.SpannerExceptionFactory.asSpannerException;
import static com.google.cloud.spanner.SpannerExceptionFactory.newSpannerException;
import static com.google.cloud.spanner.ThreadFactoryUtil.tryCreateVirtualThreadPerTaskExecutor;

Expand Down Expand Up @@ -539,7 +538,7 @@ public <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCalla
// is actually running.
checkEmulatorConnection(options, channelProvider, credentialsProvider, emulatorHost);
} catch (Exception e) {
throw asSpannerException(e);
throw newSpannerException(e);
}
} else {
this.databaseAdminStub = null;
Expand Down Expand Up @@ -727,7 +726,7 @@ private <T> T runWithRetryOnAdministrativeRequestsExceeded(Callable<T> callable)
new AdminRequestsLimitExceededRetryAlgorithm<>(),
NanoClock.getDefaultClock());
} catch (RetryHelperException e) {
throw asSpannerException(e.getCause());
throw SpannerExceptionFactory.asSpannerException(e.getCause());
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

medium

This change to fully qualify asSpannerException seems inconsistent with the overall goal of the revert, which appears to be replacing asSpannerException with newSpannerException. Other parts of this file and the PR make that replacement. For consistency, this should probably be newSpannerException(e.getCause()).

Suggested change
throw SpannerExceptionFactory.asSpannerException(e.getCause());
throw newSpannerException(e.getCause());

}
}

Expand Down Expand Up @@ -1318,7 +1317,7 @@ public OperationFuture<Empty, UpdateDatabaseDdlMetadata> updateDatabaseDdl(
throw newSpannerException(e);
} catch (ExecutionException e) {
Throwable t = e.getCause();
SpannerException se = asSpannerException(t);
SpannerException se = SpannerExceptionFactory.asSpannerException(t);
Copy link

@gemini-code-assist gemini-code-assist bot Nov 20, 2025

Choose a reason for hiding this comment

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

medium

Similar to other comments, this change to fully qualify asSpannerException is inconsistent with the revert. To maintain consistency with other changes in this PR, this should probably be newSpannerException(t).

Suggested change
SpannerException se = SpannerExceptionFactory.asSpannerException(t);
SpannerException se = newSpannerException(t);

if (se instanceof AdminRequestsPerMinuteExceededException) {
// Propagate this to trigger a retry.
throw se;
Expand Down Expand Up @@ -1984,12 +1983,8 @@ private static <T> T get(final Future<T> future) throws SpannerException {
// We are the sole consumer of the future, so cancel it.
future.cancel(true);
throw SpannerExceptionFactory.propagateInterrupt(e);
} catch (ExecutionException e) {
throw asSpannerException(e.getCause());
} catch (CancellationException e) {
} catch (Exception e) {
throw newSpannerException(context, e, null);
} catch (Exception exception) {
throw asSpannerException(exception);
}
}

Expand Down Expand Up @@ -2227,7 +2222,7 @@ public void onError(Throwable t) {
if (this.consumer.cancelQueryWhenClientIsClosed()) {
unregisterResponseObserver(this);
}
consumer.onError(asSpannerException(t));
consumer.onError(newSpannerException(t));
}

@Override
Expand Down
26 changes: 10 additions & 16 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import static java.util.Arrays.asList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeFalse;
Expand Down Expand Up @@ -132,21 +131,16 @@ public void simple() {

@Test
public void badQuery() {
SpannerException exception =
assertThrows(
SpannerException.class,
() -> execute(Statement.of("SELECT Apples AND Oranges"), Type.int64()));
assertEquals(ErrorCode.INVALID_ARGUMENT, exception.getErrorCode());
if (dialect.dialect == Dialect.POSTGRESQL) {
assertTrue(
exception.getMessage(),
exception.getMessage().contains("column \"apples\" does not exist"));
// See https://www.postgresql.org/docs/current/errcodes-appendix.html
// '42703' == undefined_column
assertEquals("42703", exception.getPostgreSQLErrorCode());
} else {
assertTrue(
exception.getMessage(), exception.getMessage().contains("Unrecognized name: Apples"));
try {
execute(Statement.of("SELECT Apples AND Oranges"), Type.int64());
fail("Expected exception");
} catch (SpannerException ex) {
assertThat(ex.getErrorCode()).isEqualTo(ErrorCode.INVALID_ARGUMENT);
if (dialect.dialect == Dialect.POSTGRESQL) {
assertThat(ex.getMessage()).contains("column \"apples\" does not exist");
} else {
assertThat(ex.getMessage()).contains("Unrecognized name: Apples");
}
}
}

Expand Down
Loading