From acecbfe3bd029e76128701c67107f289b21ace19 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Wed, 17 Mar 2021 21:46:54 +1100 Subject: [PATCH] fix: addresses PR comments --- .../java/com/google/cloud/spanner/Backup.java | 4 --- .../com/google/cloud/spanner/BackupInfo.java | 3 +- .../spanner/DatabaseAdminClientImpl.java | 5 +++ .../encryption/BackupEncryptionConfig.java | 2 +- .../spanner/encryption/EncryptionConfig.java | 26 --------------- .../com/google/cloud/spanner/BackupTest.java | 31 ----------------- .../spanner/DatabaseAdminClientImplTest.java | 33 ++++++++++++++----- .../EncryptionConfigProtoMapperTest.java | 3 -- .../encryption/EncryptionConfigsTest.java | 2 -- 9 files changed, 32 insertions(+), 77 deletions(-) delete mode 100644 google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/EncryptionConfig.java diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java index 23362e8e15..27308c0400 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java @@ -62,10 +62,6 @@ public Backup build() { /** Creates a backup on the server based on the source of this {@link Backup} instance. */ public OperationFuture create() { - Preconditions.checkState( - getExpireTime() != null, "Cannot create a backup without an expire time"); - Preconditions.checkState( - getDatabase() != null, "Cannot create a backup without a source database"); return dbClient.createBackup(this); } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java index adb40fed0c..0657ff2b7b 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java @@ -219,8 +219,7 @@ public long getSize() { /** * Returns the {@link BackupEncryptionConfig} to encrypt the backup during its creation. Returns - * - * null if no customer-managed encryption key should be used. + * null if no customer-managed encryption key should be used. */ public BackupEncryptionConfig getEncryptionConfig() { return encryptionConfig; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClientImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClientImpl.java index c29129f1f9..6129e0fa2d 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClientImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseAdminClientImpl.java @@ -143,6 +143,11 @@ public OperationFuture createBackup( @Override public OperationFuture createBackup(Backup backupInfo) throws SpannerException { + Preconditions.checkArgument( + backupInfo.getExpireTime() != null, "Cannot create a backup without an expire time"); + Preconditions.checkArgument( + backupInfo.getDatabase() != null, "Cannot create a backup without a source database"); + final OperationFuture rawOperationFuture = rpc.createBackup(backupInfo); diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/BackupEncryptionConfig.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/BackupEncryptionConfig.java index f8d3fcad48..a6e9fc1356 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/BackupEncryptionConfig.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/BackupEncryptionConfig.java @@ -20,4 +20,4 @@ /** Marker interface for encryption configurations that can be applied on backups. */ @InternalApi -public interface BackupEncryptionConfig extends EncryptionConfig {} +public interface BackupEncryptionConfig {} diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/EncryptionConfig.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/EncryptionConfig.java deleted file mode 100644 index d2df76f5a2..0000000000 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/EncryptionConfig.java +++ /dev/null @@ -1,26 +0,0 @@ -/* - * Copyright 2021 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.cloud.spanner.encryption; - -import com.google.api.core.InternalApi; - -/** - * Marker interface for encryption configurations that can be applied on databases, backups and - * restores. - */ -@InternalApi -public interface EncryptionConfig {} diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java index 2110794d64..f2b479b666 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/BackupTest.java @@ -113,37 +113,6 @@ public void create() { verify(dbClient).createBackup(backup); } - @Test - public void createWithoutSource() { - Timestamp expireTime = Timestamp.now(); - Backup backup = - dbClient - .newBackupBuilder(BackupId.of("test-project", "dest-instance", "backup-id")) - .setExpireTime(expireTime) - .build(); - try { - backup.create(); - fail("Expected exception"); - } catch (IllegalStateException e) { - assertNotNull(e.getMessage()); - } - } - - @Test - public void createWithoutExpireTime() { - Backup backup = - dbClient - .newBackupBuilder(BackupId.of("test-project", "instance-id", "backup-id")) - .setDatabase(DatabaseId.of("test-project", "instance-id", "src-database")) - .build(); - try { - backup.create(); - fail("Expected exception"); - } catch (IllegalStateException e) { - assertNotNull(e.getMessage()); - } - } - @Test public void createWithoutVersionTimeShouldSucceed() { final Timestamp expireTime = Timestamp.now(); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseAdminClientImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseAdminClientImplTest.java index e8e5f91dc4..9079729ff7 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseAdminClientImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseAdminClientImplTest.java @@ -17,6 +17,7 @@ package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; @@ -261,7 +262,7 @@ public void listDatabasesError() { SpannerExceptionFactory.newSpannerException(ErrorCode.INVALID_ARGUMENT, "Test error")); try { client.listDatabases(INSTANCE_ID, Options.pageSize(1)); - Assert.fail("Missing expected exception"); + fail("Missing expected exception"); } catch (SpannerException e) { assertThat(e.getMessage()).contains(INSTANCE_NAME); // Assert that the call was done without a page token. @@ -279,7 +280,7 @@ public void listDatabaseErrorWithToken() { SpannerExceptionFactory.newSpannerException(ErrorCode.INVALID_ARGUMENT, "Test error")); try { Lists.newArrayList(client.listDatabases(INSTANCE_ID, Options.pageSize(1)).iterateAll()); - Assert.fail("Missing expected exception"); + fail("Missing expected exception"); } catch (SpannerException e) { assertThat(e.getMessage()).contains(INSTANCE_NAME); // Assert that the call was done without a page token. @@ -388,12 +389,6 @@ public void createBackupWithBackupObject() throws ExecutionException, Interrupte final Timestamp versionTime = Timestamp.ofTimeMicroseconds( TimeUnit.MILLISECONDS.toMicros(System.currentTimeMillis()) - TimeUnit.DAYS.toMicros(2)); - final Backup expectedCallBackup = - Backup.newBuilder() - .setDatabase(DB_NAME) - .setExpireTime(expireTime.toProto()) - .setVersionTime(versionTime.toProto()) - .build(); final com.google.cloud.spanner.Backup requestBackup = client .newBackupBuilder(BackupId.of(PROJECT_ID, INSTANCE_ID, BK_ID)) @@ -410,6 +405,28 @@ public void createBackupWithBackupObject() throws ExecutionException, Interrupte assertThat(op.get().getId().getName()).isEqualTo(BK_NAME); } + @Test(expected = IllegalArgumentException.class) + public void testCreateBackupNoExpireTime() { + final com.google.cloud.spanner.Backup requestBackup = + client + .newBackupBuilder(BackupId.of(PROJECT_ID, INSTANCE_ID, BK_ID)) + .setDatabase(DatabaseId.of(PROJECT_ID, INSTANCE_ID, DB_ID)) + .build(); + + client.createBackup(requestBackup); + } + + @Test(expected = IllegalArgumentException.class) + public void testCreateBackupNoDatabase() { + final com.google.cloud.spanner.Backup requestBackup = + client + .newBackupBuilder(BackupId.of(PROJECT_ID, INSTANCE_ID, BK_ID)) + .setExpireTime(Timestamp.now()) + .build(); + + client.createBackup(requestBackup); + } + @Test public void createEncryptedBackup() throws ExecutionException, InterruptedException { final OperationFuture rawOperationFuture = diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java index fd64fc590c..4ce300c2f6 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java @@ -16,7 +16,6 @@ package com.google.cloud.spanner.encryption; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.fail; import com.google.spanner.admin.database.v1.CreateBackupEncryptionConfig; import com.google.spanner.admin.database.v1.EncryptionConfig; @@ -85,7 +84,6 @@ public void testCreateBackupConfigUseDatabaseEncryption() { @Test(expected = IllegalArgumentException.class) public void testCreateBackupInvalidEncryption() { EncryptionConfigProtoMapper.createBackupEncryptionConfig(null); - fail("Create backup encryption config with invalid encryption should fail"); } @Test @@ -137,6 +135,5 @@ public void testRestoreDatabaseConfigUseBackupEncryption() { @Test(expected = IllegalArgumentException.class) public void testRestoreDatabaseConfigInvalidEncryption() { EncryptionConfigProtoMapper.restoreDatabaseEncryptionConfig(null); - fail("Restore database encryption config with invalid encryption should fail"); } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigsTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigsTest.java index a95ca876eb..82f997a1c4 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigsTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigsTest.java @@ -17,7 +17,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; -import static org.junit.Assert.fail; import org.junit.Test; @@ -37,7 +36,6 @@ public void testCustomerManagedEncryption() { @Test(expected = IllegalArgumentException.class) public void testCustomerManagedEncryptionNullKeyName() { EncryptionConfigs.customerManagedEncryption(null); - fail("Customer managed encryption with null key should fail"); } @Test