[sakthivelmanii]

Problem

• The PgSpannerSample.java code catches an ExecutionException and attempts to cast its cause directly to a SpannerException.
• The cause is actually a com.google.api.gax.rpc.FailedPreconditionException, resulting in a ClassCastException.
• This causes the java8-samples test to fail.

Solution

• Use SpannerExceptionFactory.asSpannerException(e.getCause()) to correctly convert or wrap the GAX exception into a SpannerException instead of casting directly.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates several database administration methods in PgSpannerSample.java to safely handle ExecutionException. Instead of directly casting e.getCause() to SpannerException, which could lead to a ClassCastException if the cause is of a different type, the code now utilizes SpannerExceptionFactory.asSpannerException(e.getCause()) to perform a safe conversion. There are no review comments, so I have no additional feedback to provide.

[gemini-code-assist]

Code Review

This pull request updates PgSpannerSample.java to replace direct casting of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()) across multiple database administration methods. This prevents potential ClassCastException issues if the cause of the ExecutionException is not a SpannerException. There are no review comments to address, and I have no additional feedback to provide.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces direct casts of SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()) across multiple production and test files. The review feedback identifies a redundant if-condition in SpannerApiFutures.java that can be simplified, and points out redundant outer parentheses in LazySpannerInitializerTest.java and ITDatabaseAdminTest.java that should be cleaned up.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the codebase by replacing direct casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). The review feedback highlights two areas for improvement: in ClientSideStatementImpl.java, the factory call is redundant because of an enclosing instanceof check, so a direct cast should be retained; in ITBackupTest.java, the instanceof check should be removed entirely to allow GAX exceptions to be properly converted to SpannerException using the factory.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Google Cloud Spanner library, tests, and samples by replacing direct casts of exception causes to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). The reviewer feedback points out several locations where this replacement is redundant or contradictory, particularly where the code already explicitly checks or asserts that the cause is an instance of SpannerException. In these scenarios, keeping the direct cast is cleaner and more appropriate.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces direct casting of exceptions to SpannerException with SpannerExceptionFactory.asSpannerException across production, test, and sample files. Feedback highlights a critical compilation issue due to missing imports of SpannerExceptionFactory in the modified sample files. Additionally, the reviewer noted that using the factory method is redundant in locations where the exception type has already been verified via instanceof checks or test assertions, suggesting a revert to direct casting in those specific cases.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the java-spanner codebase by replacing direct casts of exception causes to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). While this standardizes exception conversion, it introduces numerous redundant and unused imports of SpannerExceptionFactory, particularly within the same package or where a class imports itself. Additionally, in ClientSideStatementImpl, using the factory is redundant because the cause is already verified to be a SpannerException, making a direct cast more appropriate.

[sakthivelmanii]

/gemini review

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the codebase by replacing direct casts of exception causes to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). This change is applied consistently across production code, tests, and sample snippets. The review feedback points out a redundancy in ClientSideStatementImpl.java, where SpannerExceptionFactory.asSpannerException is called inside an instanceof check that already guarantees the cause is a SpannerException, suggesting a revert to a direct cast in that specific instance.

[gemini-code-assist]

Code Review

This pull request standardizes exception handling across the codebase by replacing direct casts of exception causes to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). The review feedback highlights two key improvement opportunities: first, in DatabaseAddSplitPointsSample.java, catching a broad Exception and calling e.getCause() can lead to a NullPointerException if the exception lacks a cause, so ExecutionException and InterruptedException should be handled separately; second, in ClientSideStatementImpl.java, using the factory method is redundant because the cause is already verified with instanceof, so a direct cast should be retained to avoid unnecessary factory calls and an unused import.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the codebase by replacing direct casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). However, two issues were identified in the review: first, a missing .get() call on an asynchronous operation in DatabaseAddSplitPointsSample.java will cause a compilation error due to uncaught checked exceptions; second, a redundant factory call inside an instanceof guard in ClientSideStatementImpl.java can be simplified back to a direct cast.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Spanner codebase by replacing direct casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). It also updates DatabaseAddSplitPointsSample.java to block on the asynchronous addSplitPoints call. The review feedback points out a redundant factory call in ClientSideStatementImpl.java where the exception type is already guarded by an instanceof check, which also makes its import unnecessary if reverted. Additionally, the feedback suggests importing java.util.concurrent.ExecutionException in DatabaseAddSplitPointsSample.java to avoid using its fully qualified name in the catch block.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Spanner library, tests, and samples by replacing direct casts of exception causes with safe conversions using SpannerExceptionFactory.asSpannerException(). The reviewer identified a few issues with this refactoring: first, a redundant conversion inside an explicit instanceof check in ClientSideStatementImpl.java; second, logical contradictions in integration tests where assertions still verify that the cause is a SpannerException before converting it, which defeats the purpose of the safe conversion.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request simplifies exception handling across the codebase by replacing manual casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). This change is applied to various production files, tests, and sample snippets. Additionally, the addSplitPoints sample was updated to block on the future's result and handle exceptions properly. The review feedback correctly identifies two unused imports of SpannerExceptionFactory in ClientSideStatementImpl.java and ITTransactionTest.java that should be removed.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request standardizes exception handling across the Spanner library and its samples by replacing direct casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). Feedback on the changes highlights potential NullPointerException risks in several integration tests (ITDatabaseAdminTest, ITPitrUpdateDatabaseTest, and ITPitrCreateDatabaseTest) where broad exceptions or multi-catch blocks are caught before extracting the cause. The reviewer recommends catching ExecutionException specifically or using Assert.assertThrows to safely assert and handle the expected exceptions.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Google Cloud Spanner library, replacing manual casting of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). This change is consistently applied to production code, integration tests, and sample snippets to ensure robust and uniform exception translation. Additionally, in DatabaseAddSplitPointsSample.java, the asynchronous call to addSplitPoints is now properly blocked using .get() with appropriate exception handling. No review comments were provided, and the changes appear correct and complete.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Spanner codebase by replacing direct casts of exception causes to SpannerException with calls to SpannerExceptionFactory.asSpannerException(). This change is implemented in core classes, integration tests, and numerous sample snippets. Additionally, DatabaseAddSplitPointsSample.java has been updated to block on the future returned by addSplitPoints and handle exceptions appropriately. The review feedback correctly identifies that several modified sample snippet files are missing the import statement for SpannerExceptionFactory, which will result in compilation errors. These imports must be added to ensure the code compiles successfully.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Java Spanner client, tests, and samples by replacing direct casting of ExecutionException causes to SpannerException with the safer SpannerExceptionFactory.asSpannerException(e.getCause()) helper method. Additionally, it updates some tests to use Assert.assertThrows and ensures asynchronous operations in certain samples are properly awaited using .get(). I have no feedback to provide as there are no review comments.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors exception handling across the Google Cloud Spanner library, tests, and samples by replacing direct casts of e.getCause() to SpannerException with SpannerExceptionFactory.asSpannerException(e.getCause()). This ensures safer exception translation, particularly when dealing with GAX exceptions. The review feedback correctly points out that several tests still contain assertThat(e.getCause()).isInstanceOf(SpannerException.class) assertions, which will fail if the cause is a GAX exception. Additionally, a critical issue was identified in ITPitrCreateDatabaseTest.java where an incorrect @Test(expected = DatabaseNotFoundException.class) annotation is used on a method that internally catches the exception, which would lead to test failures.

[gemini-code-assist]

The @Test(expected = DatabaseNotFoundException.class) annotation is incorrect here because the ExecutionException is caught and handled inside the try-catch block, meaning no exception will propagate out of the method. Keeping this annotation will cause the test to fail with an AssertionError because the expected exception is not thrown.

Suggested change

	@Test(expected = DatabaseNotFoundException.class)
	public void returnsAnErrorWhenAnInvalidVersionRetentionPeriodIsGiven() {
	public void returnsAnErrorWhenAnInvalidVersionRetentionPeriodIsGiven() throws Exception {
	@Test
	public void returnsAnErrorWhenAnInvalidVersionRetentionPeriodIsGiven() throws Exception {

[gemini-code-assist]

Since e.getCause() can be a GAX exception (which is the primary reason for using SpannerExceptionFactory.asSpannerException), asserting that the cause is directly an instance of SpannerException will cause the test to fail. We should remove this assertion.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

Since e.getCause() can be a GAX exception, asserting that the cause is directly an instance of SpannerException will cause the test to fail. We should remove this assertion.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

Since e.getCause() can be a GAX exception, the assertion assertThat(e.getCause()).isInstanceOf(SpannerException.class) will fail. We should remove it and rely on SpannerExceptionFactory.asSpannerException to handle the conversion.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

The assertion assertThat(e.getCause()).isInstanceOf(SpannerException.class) should be removed because the cause can be a GAX exception, which would cause this assertion to fail.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

For consistency and to avoid redundant assertions, we can remove assertThat(e.getCause()).isInstanceOf(SpannerException.class) since SpannerExceptionFactory.asSpannerException safely handles any exception type.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

For consistency and to avoid redundant assertions, we can remove assertThat(e.getCause()).isInstanceOf(SpannerException.class) since SpannerExceptionFactory.asSpannerException safely handles any exception type.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

For consistency and to avoid redundant assertions, we can remove assertThat(e.getCause()).isInstanceOf(SpannerException.class) since SpannerExceptionFactory.asSpannerException safely handles any exception type.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

For consistency and to avoid redundant assertions, we can remove assertThat(e.getCause()).isInstanceOf(SpannerException.class) since SpannerExceptionFactory.asSpannerException safely handles any exception type.

Suggested change

	assertThat(e.getCause()).isInstanceOf(SpannerException.class);
	SpannerException se = (SpannerException) e.getCause();
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());
	SpannerException se = SpannerExceptionFactory.asSpannerException(e.getCause());

[gemini-code-assist]

For consistency and to avoid redundant assertions, we can remove assertThat(e.getCause()).isInstanceOf(SpannerException.class) since SpannerExceptionFactory.asSpannerException safely handles any exception type.

        assertThat(SpannerExceptionFactory.asSpannerException(e.getCause()).getErrorCode())