From ed5d6dccd55e66e2bcf0ada78ca4bff018402f4e Mon Sep 17 00:00:00 2001 From: ankiaga Date: Mon, 9 Oct 2023 13:49:41 +0530 Subject: [PATCH 1/5] Fix: Noop in case there is no change in autocommit value for setAutocommit() method --- .../cloud/spanner/connection/ConnectionImpl.java | 3 +++ .../spanner/connection/ConnectionImplTest.java | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index 566a93e76bc..4841f633fff 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -401,6 +401,9 @@ public boolean isClosed() { @Override public void setAutocommit(boolean autocommit) { + if (isAutocommit() == autocommit) { + return; + } ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG); ConnectionPreconditions.checkState(!isBatchActive(), "Cannot set autocommit while in a batch"); ConnectionPreconditions.checkState( diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java index 7c5ba508e8f..9b4bc3cdefb 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java @@ -412,6 +412,22 @@ public void testExecuteSetAutocommitOff() { } } + @Test + public void testExecuteAutocommitNoop() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI) + .build())) { + assertThat(subject.isAutocommit(), is(true)); + + StatementResult res = subject.execute(Statement.of("set autocommit = true")); + assertThat(res.getResultType(), is(equalTo(ResultType.NO_RESULT))); + assertThat(subject.isAutocommit(), is(true)); + } + } + @Test public void testExecuteGetAutocommit() { try (ConnectionImpl subject = From e95a142477e429b4568de817ca5b2ba8b66edf8b Mon Sep 17 00:00:00 2001 From: ankiaga Date: Mon, 9 Oct 2023 18:21:11 +0530 Subject: [PATCH 2/5] Resolved comments and Added tests --- .../spanner/connection/ConnectionImpl.java | 2 +- .../connection/ConnectionImplTest.java | 124 +++++++++++++++++- 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java index 4841f633fff..9135fc44a54 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java @@ -401,10 +401,10 @@ public boolean isClosed() { @Override public void setAutocommit(boolean autocommit) { + ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG); if (isAutocommit() == autocommit) { return; } - ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG); ConnectionPreconditions.checkState(!isBatchActive(), "Cannot set autocommit while in a batch"); ConnectionPreconditions.checkState( !isTransactionStarted(), "Cannot set autocommit while a transaction is active"); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java index 9b4bc3cdefb..8b5104e1426 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java @@ -413,7 +413,7 @@ public void testExecuteSetAutocommitOff() { } @Test - public void testExecuteAutocommitNoop() { + public void testSetAutocommitToTrue_inAutoCommitAndNotInTransaction_noop() { try (ConnectionImpl subject = createConnection( ConnectionOptions.newBuilder() @@ -422,9 +422,127 @@ public void testExecuteAutocommitNoop() { .build())) { assertThat(subject.isAutocommit(), is(true)); - StatementResult res = subject.execute(Statement.of("set autocommit = true")); - assertThat(res.getResultType(), is(equalTo(ResultType.NO_RESULT))); + subject.setAutocommit(true); + + assertThat(subject.isAutocommit(), is(true)); + } + } + + @Test + public void testSetAutocommitToTrue_inAutoCommitAndInTransaction_noop() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI) + .build())) { + assertThat(subject.isAutocommit(), is(true)); + subject.execute(Statement.of("begin transaction")); + + subject.setAutocommit(true); + + assertThat(subject.isAutocommit(), is(true)); + } + } + + @Test + public void testSetAutocommitToFalse_inAutoCommitAndNotInTransaction_autocommitModeChanged() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI) + .build())) { assertThat(subject.isAutocommit(), is(true)); + + subject.setAutocommit(false); + + assertThat(subject.isAutocommit(), is(false)); + } + } + + @Test + public void testSetAutocommitToFalse_inAutoCommitAndInTransaction_throwsException() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI) + .build())) { + assertThat(subject.isAutocommit(), is(true)); + subject.execute(Statement.of("begin transaction")); + + subject.setAutocommit(false); + fail("Cannot set autocommit while in a temporary transaction"); + } catch (SpannerException e) { + assertThat(e.getErrorCode(), is(equalTo(ErrorCode.FAILED_PRECONDITION))); + } + } + + @Test + public void testSetAutocommitToFalse_notInAutoCommitAndTransactionNotStarted_noop() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI + ";autocommit=false") + .build())) { + assertThat(subject.isAutocommit(), is(false)); + + subject.setAutocommit(false); + + assertThat(subject.isAutocommit(), is(false)); + } + } + + @Test + public void testSetAutocommitToFalse_notInAutoCommitAndTransactionStarted_noop() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI + ";autocommit=false") + .build())) { + assertThat(subject.isAutocommit(), is(false)); + subject.executeQuery(Statement.of(SELECT)); + + subject.setAutocommit(false); + + assertThat(subject.isAutocommit(), is(false)); + } + } + + @Test + public void testSetAutocommitToTrue_notInAutoCommitAndTransactionNotStarted_autocommitModeChanged() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI + ";autocommit=false") + .build())) { + assertThat(subject.isAutocommit(), is(false)); + + subject.setAutocommit(true); + + assertThat(subject.isAutocommit(), is(true)); + } + } + + @Test + public void testSetAutocommitToTrue_notInAutoCommitAndTransactionStarted_throwsException() { + try (ConnectionImpl subject = + createConnection( + ConnectionOptions.newBuilder() + .setCredentials(NoCredentials.getInstance()) + .setUri(URI + ";autocommit=false") + .build())) { + assertThat(subject.isAutocommit(), is(false)); + subject.executeQuery(Statement.of(SELECT)); + + subject.setAutocommit(true); + fail("Cannot set autocommit while in a temporary transaction"); + } catch (SpannerException e) { + assertThat(e.getErrorCode(), is(equalTo(ErrorCode.FAILED_PRECONDITION))); } } From 41915e03828d7a5f1f3a0d663ad38813ea62e543 Mon Sep 17 00:00:00 2001 From: ankiaga Date: Mon, 9 Oct 2023 19:19:59 +0530 Subject: [PATCH 3/5] Resolved comments --- .../connection/ConnectionImplTest.java | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java index 8b5104e1426..e632feccccd 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java @@ -424,7 +424,7 @@ public void testSetAutocommitToTrue_inAutoCommitAndNotInTransaction_noop() { subject.setAutocommit(true); - assertThat(subject.isAutocommit(), is(true)); + assertTrue(subject.isAutocommit()); } } @@ -441,7 +441,7 @@ public void testSetAutocommitToTrue_inAutoCommitAndInTransaction_noop() { subject.setAutocommit(true); - assertThat(subject.isAutocommit(), is(true)); + assertTrue(subject.isAutocommit()); } } @@ -457,7 +457,7 @@ public void testSetAutocommitToFalse_inAutoCommitAndNotInTransaction_autocommitM subject.setAutocommit(false); - assertThat(subject.isAutocommit(), is(false)); + assertFalse(subject.isAutocommit()); } } @@ -472,10 +472,9 @@ public void testSetAutocommitToFalse_inAutoCommitAndInTransaction_throwsExceptio assertThat(subject.isAutocommit(), is(true)); subject.execute(Statement.of("begin transaction")); - subject.setAutocommit(false); - fail("Cannot set autocommit while in a temporary transaction"); - } catch (SpannerException e) { - assertThat(e.getErrorCode(), is(equalTo(ErrorCode.FAILED_PRECONDITION))); + SpannerException exception = assertThrows(SpannerException.class, () -> subject.setAutocommit(false)); + assertEquals(ErrorCode.FAILED_PRECONDITION, exception.getErrorCode()); + assertTrue(exception.getMessage().contains("Cannot set autocommit while in a temporary transaction")); } } @@ -491,7 +490,7 @@ public void testSetAutocommitToFalse_notInAutoCommitAndTransactionNotStarted_noo subject.setAutocommit(false); - assertThat(subject.isAutocommit(), is(false)); + assertFalse(subject.isAutocommit()); } } @@ -508,7 +507,7 @@ public void testSetAutocommitToFalse_notInAutoCommitAndTransactionStarted_noop() subject.setAutocommit(false); - assertThat(subject.isAutocommit(), is(false)); + assertFalse(subject.isAutocommit()); } } @@ -524,7 +523,7 @@ public void testSetAutocommitToTrue_notInAutoCommitAndTransactionNotStarted_auto subject.setAutocommit(true); - assertThat(subject.isAutocommit(), is(true)); + assertTrue(subject.isAutocommit()); } } @@ -539,10 +538,9 @@ public void testSetAutocommitToTrue_notInAutoCommitAndTransactionStarted_throwsE assertThat(subject.isAutocommit(), is(false)); subject.executeQuery(Statement.of(SELECT)); - subject.setAutocommit(true); - fail("Cannot set autocommit while in a temporary transaction"); - } catch (SpannerException e) { - assertThat(e.getErrorCode(), is(equalTo(ErrorCode.FAILED_PRECONDITION))); + SpannerException exception = assertThrows(SpannerException.class, () -> subject.setAutocommit(true)); + assertEquals(ErrorCode.FAILED_PRECONDITION, exception.getErrorCode()); + assertTrue(exception.getMessage().contains("Cannot set autocommit while a transaction is active")); } } From ee83c7976a76d147081497091b52cf635a9477c3 Mon Sep 17 00:00:00 2001 From: ankiaga Date: Tue, 10 Oct 2023 13:08:15 +0530 Subject: [PATCH 4/5] Fix formatting errors --- .../spanner/connection/ConnectionImplTest.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java index e632feccccd..0b54b352253 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/ConnectionImplTest.java @@ -472,9 +472,13 @@ public void testSetAutocommitToFalse_inAutoCommitAndInTransaction_throwsExceptio assertThat(subject.isAutocommit(), is(true)); subject.execute(Statement.of("begin transaction")); - SpannerException exception = assertThrows(SpannerException.class, () -> subject.setAutocommit(false)); + SpannerException exception = + assertThrows(SpannerException.class, () -> subject.setAutocommit(false)); assertEquals(ErrorCode.FAILED_PRECONDITION, exception.getErrorCode()); - assertTrue(exception.getMessage().contains("Cannot set autocommit while in a temporary transaction")); + assertTrue( + exception + .getMessage() + .contains("Cannot set autocommit while in a temporary transaction")); } } @@ -512,7 +516,8 @@ public void testSetAutocommitToFalse_notInAutoCommitAndTransactionStarted_noop() } @Test - public void testSetAutocommitToTrue_notInAutoCommitAndTransactionNotStarted_autocommitModeChanged() { + public void + testSetAutocommitToTrue_notInAutoCommitAndTransactionNotStarted_autocommitModeChanged() { try (ConnectionImpl subject = createConnection( ConnectionOptions.newBuilder() @@ -538,9 +543,11 @@ public void testSetAutocommitToTrue_notInAutoCommitAndTransactionStarted_throwsE assertThat(subject.isAutocommit(), is(false)); subject.executeQuery(Statement.of(SELECT)); - SpannerException exception = assertThrows(SpannerException.class, () -> subject.setAutocommit(true)); + SpannerException exception = + assertThrows(SpannerException.class, () -> subject.setAutocommit(true)); assertEquals(ErrorCode.FAILED_PRECONDITION, exception.getErrorCode()); - assertTrue(exception.getMessage().contains("Cannot set autocommit while a transaction is active")); + assertTrue( + exception.getMessage().contains("Cannot set autocommit while a transaction is active")); } } From f161539723e0e673314492fd233aeba71bf57424 Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Tue, 10 Oct 2023 10:17:21 +0000 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 169541eff05..2c5415c6780 100644 --- a/README.md +++ b/README.md @@ -57,13 +57,13 @@ implementation 'com.google.cloud:google-cloud-spanner' If you are using Gradle without BOM, add this to your dependencies: ```Groovy -implementation 'com.google.cloud:google-cloud-spanner:6.49.0' +implementation 'com.google.cloud:google-cloud-spanner:6.50.0' ``` If you are using SBT, add this to your dependencies: ```Scala -libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.49.0" +libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.50.0" ``` @@ -432,7 +432,7 @@ Java is a registered trademark of Oracle and/or its affiliates. [kokoro-badge-link-5]: http://storage.googleapis.com/cloud-devrel-public/java/badges/java-spanner/java11.html [stability-image]: https://img.shields.io/badge/stability-stable-green [maven-version-image]: https://img.shields.io/maven-central/v/com.google.cloud/google-cloud-spanner.svg -[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-spanner/6.49.0 +[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-spanner/6.50.0 [authentication]: https://github.com/googleapis/google-cloud-java#authentication [auth-scopes]: https://developers.google.com/identity/protocols/oauth2/scopes [predefined-iam-roles]: https://cloud.google.com/iam/docs/understanding-roles#predefined_roles