From 76a82cc0c955f28dfd590d64004be05ec33f2485 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 27 May 2021 16:00:54 -0400 Subject: [PATCH 1/4] fix: update BucketInfo translation code to properly handle lifecycle rules Fixes #850 --- .../src/main/java/com/google/cloud/storage/BucketInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 5b69814ec7..fa7a6867cd 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -1862,7 +1862,7 @@ public Rule apply(LifecycleRule lifecycleRule) { })); } - if (rules != null) { + if (!rules.isEmpty()) { Lifecycle lifecycle = new Lifecycle(); lifecycle.setRule(ImmutableList.copyOf(rules)); bucketPb.setLifecycle(lifecycle); From 13f79a5aaed01e55225849a787f590db90799808 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 27 May 2021 17:42:00 -0400 Subject: [PATCH 2/4] update handling of rules to ensure removal of rules is correctly accounted for Added unit tests to exercise every branch of rules mapping --- .../com/google/cloud/storage/BucketInfo.java | 64 ++++++++------ .../google/cloud/storage/BucketInfoTest.java | 86 +++++++++++++++++++ 2 files changed, 125 insertions(+), 25 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index fa7a6867cd..991a8aff16 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -43,6 +43,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -1838,33 +1839,46 @@ public ObjectAccessControl apply(Acl acl) { website.setNotFoundPage(notFoundPage); bucketPb.setWebsite(website); } - Set rules = new HashSet<>(); - if (deleteRules != null) { - rules.addAll( - transform( - deleteRules, - new Function() { - @Override - public Rule apply(DeleteRule deleteRule) { - return deleteRule.toPb(); - } - })); - } - if (lifecycleRules != null) { - rules.addAll( - transform( - lifecycleRules, - new Function() { - @Override - public Rule apply(LifecycleRule lifecycleRule) { - return lifecycleRule.toPb(); - } - })); - } - if (!rules.isEmpty()) { + if (deleteRules != null || lifecycleRules != null) { Lifecycle lifecycle = new Lifecycle(); - lifecycle.setRule(ImmutableList.copyOf(rules)); + + if ( + (deleteRules == null && lifecycleRules.isEmpty()) + || (lifecycleRules == null && deleteRules.isEmpty()) + || (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty()) + ) { + lifecycle.setRule(Collections.emptyList()); + } else { + Set rules = new HashSet<>(); + if (deleteRules != null) { + rules.addAll( + transform( + deleteRules, + new Function() { + @Override + public Rule apply(DeleteRule deleteRule) { + return deleteRule.toPb(); + } + })); + } + if (lifecycleRules != null) { + rules.addAll( + transform( + lifecycleRules, + new Function() { + @Override + public Rule apply(LifecycleRule lifecycleRule) { + return lifecycleRule.toPb(); + } + })); + } + + if (!rules.isEmpty()) { + lifecycle.setRule(ImmutableList.copyOf(rules)); + } + } + bucketPb.setLifecycle(lifecycle); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 8def7c306d..08d1c04956 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -19,10 +19,12 @@ import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import com.google.api.client.util.DateTime; import com.google.api.services.storage.model.Bucket; +import com.google.api.services.storage.model.Bucket.Lifecycle; import com.google.api.services.storage.model.Bucket.Lifecycle.Rule; import com.google.cloud.storage.Acl.Project; import com.google.cloud.storage.Acl.Role; @@ -39,6 +41,7 @@ import com.google.cloud.storage.BucketInfo.RawDeleteRule; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -172,6 +175,8 @@ public class BucketInfoTest { .setLogging(LOGGING) .build(); + private static final Lifecycle EMPTY_LIFECYCLE = lifecycle(Collections.emptyList()); + @Test public void testToBuilder() { compareBuckets(BUCKET_INFO, BUCKET_INFO.toBuilder().build()); @@ -376,4 +381,85 @@ public void testLogging() { assertEquals("test-bucket", logging.getLogBucket()); assertEquals("test-", logging.getLogObjectPrefix()); } + + @Test + public void testRuleMappingIsCorrect_noMutations() { + Bucket bucket = bi().build().toPb(); + assertNull(bucket.getLifecycle()); + } + + @Test + public void testRuleMappingIsCorrect_deleteLifecycleRules() { + Bucket bucket = bi() + .deleteLifecycleRules() + .build().toPb(); + assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); + } + + @Test + @SuppressWarnings({"deprecation"}) + public void testRuleMappingIsCorrect_setDeleteRules_null() { + Bucket bucket = bi() + .setDeleteRules(null) + .build().toPb(); + assertNull(bucket.getLifecycle()); + } + + @Test + @SuppressWarnings({"deprecation"}) + public void testRuleMappingIsCorrect_setDeleteRules_empty() { + Bucket bucket = bi() + .setDeleteRules(Collections.emptyList()) + .build().toPb(); + assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); + } + + @Test + public void testRuleMappingIsCorrect_setLifecycleRules_empty() { + Bucket bucket = bi() + .setLifecycleRules(Collections.emptyList()) + .build().toPb(); + assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); + } + + + @Test + public void testRuleMappingIsCorrect_setLifeCycleRules_nonEmpty() { + LifecycleRule lifecycleRule = new LifecycleRule( + LifecycleAction.newDeleteAction(), + LifecycleCondition.newBuilder().setAge(10).build()); + Rule lifecycleDeleteAfter10 = + lifecycleRule.toPb(); + Bucket bucket = bi() + .setLifecycleRules(ImmutableList.of(lifecycleRule)) + .build().toPb(); + assertEquals(lifecycle(lifecycleDeleteAfter10), bucket.getLifecycle()); + } + + @Test + @SuppressWarnings({"deprecation"}) + public void testRuleMappingIsCorrect_setDeleteRules_nonEmpty() { + DeleteRule deleteRule = DELETE_RULES.get(0); + Rule deleteRuleAge5 = deleteRule.toPb(); + Bucket bucket = bi() + .setDeleteRules(ImmutableList.of(deleteRule)) + .build().toPb(); + assertEquals(lifecycle(deleteRuleAge5), bucket.getLifecycle()); + } + + private static Lifecycle lifecycle(Rule... rules) { + return lifecycle(Arrays.asList(rules)); + } + + private static Lifecycle lifecycle(List rules) { + Lifecycle emptyLifecycle = new Lifecycle(); + emptyLifecycle.setRule(rules); + return emptyLifecycle; + } + + private static BucketInfo.Builder bi() { + String bucketId = "bucketId"; + return BucketInfo.newBuilder(bucketId); + } + } From c39b1889de3ff9fe9f7d9ec42d1442c83abe9a12 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 27 May 2021 17:53:57 -0400 Subject: [PATCH 3/4] bow to the all mighty formatter *bow* --- .../com/google/cloud/storage/BucketInfo.java | 8 ++--- .../google/cloud/storage/BucketInfoTest.java | 35 ++++++------------- 2 files changed, 13 insertions(+), 30 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 991a8aff16..b3664cbc46 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -1843,11 +1843,9 @@ public ObjectAccessControl apply(Acl acl) { if (deleteRules != null || lifecycleRules != null) { Lifecycle lifecycle = new Lifecycle(); - if ( - (deleteRules == null && lifecycleRules.isEmpty()) + if ((deleteRules == null && lifecycleRules.isEmpty()) || (lifecycleRules == null && deleteRules.isEmpty()) - || (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty()) - ) { + || (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty())) { lifecycle.setRule(Collections.emptyList()); } else { Set rules = new HashSet<>(); @@ -1878,7 +1876,7 @@ public Rule apply(LifecycleRule lifecycleRule) { lifecycle.setRule(ImmutableList.copyOf(rules)); } } - + bucketPb.setLifecycle(lifecycle); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 08d1c04956..d72901c17e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -390,49 +390,37 @@ public void testRuleMappingIsCorrect_noMutations() { @Test public void testRuleMappingIsCorrect_deleteLifecycleRules() { - Bucket bucket = bi() - .deleteLifecycleRules() - .build().toPb(); + Bucket bucket = bi().deleteLifecycleRules().build().toPb(); assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); } @Test @SuppressWarnings({"deprecation"}) public void testRuleMappingIsCorrect_setDeleteRules_null() { - Bucket bucket = bi() - .setDeleteRules(null) - .build().toPb(); + Bucket bucket = bi().setDeleteRules(null).build().toPb(); assertNull(bucket.getLifecycle()); } @Test @SuppressWarnings({"deprecation"}) public void testRuleMappingIsCorrect_setDeleteRules_empty() { - Bucket bucket = bi() - .setDeleteRules(Collections.emptyList()) - .build().toPb(); + Bucket bucket = bi().setDeleteRules(Collections.emptyList()).build().toPb(); assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); } @Test public void testRuleMappingIsCorrect_setLifecycleRules_empty() { - Bucket bucket = bi() - .setLifecycleRules(Collections.emptyList()) - .build().toPb(); + Bucket bucket = bi().setLifecycleRules(Collections.emptyList()).build().toPb(); assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle()); } - @Test public void testRuleMappingIsCorrect_setLifeCycleRules_nonEmpty() { - LifecycleRule lifecycleRule = new LifecycleRule( - LifecycleAction.newDeleteAction(), - LifecycleCondition.newBuilder().setAge(10).build()); - Rule lifecycleDeleteAfter10 = - lifecycleRule.toPb(); - Bucket bucket = bi() - .setLifecycleRules(ImmutableList.of(lifecycleRule)) - .build().toPb(); + LifecycleRule lifecycleRule = + new LifecycleRule( + LifecycleAction.newDeleteAction(), LifecycleCondition.newBuilder().setAge(10).build()); + Rule lifecycleDeleteAfter10 = lifecycleRule.toPb(); + Bucket bucket = bi().setLifecycleRules(ImmutableList.of(lifecycleRule)).build().toPb(); assertEquals(lifecycle(lifecycleDeleteAfter10), bucket.getLifecycle()); } @@ -441,9 +429,7 @@ public void testRuleMappingIsCorrect_setLifeCycleRules_nonEmpty() { public void testRuleMappingIsCorrect_setDeleteRules_nonEmpty() { DeleteRule deleteRule = DELETE_RULES.get(0); Rule deleteRuleAge5 = deleteRule.toPb(); - Bucket bucket = bi() - .setDeleteRules(ImmutableList.of(deleteRule)) - .build().toPb(); + Bucket bucket = bi().setDeleteRules(ImmutableList.of(deleteRule)).build().toPb(); assertEquals(lifecycle(deleteRuleAge5), bucket.getLifecycle()); } @@ -461,5 +447,4 @@ private static BucketInfo.Builder bi() { String bucketId = "bucketId"; return BucketInfo.newBuilder(bucketId); } - } From 38b2abcaec3fdc3f014a37e6d0ad609ef1dd1c80 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Tue, 1 Jun 2021 15:07:11 -0400 Subject: [PATCH 4/4] add comment --- .../main/java/com/google/cloud/storage/BucketInfo.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index b3664cbc46..c13395238b 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -1843,6 +1843,14 @@ public ObjectAccessControl apply(Acl acl) { if (deleteRules != null || lifecycleRules != null) { Lifecycle lifecycle = new Lifecycle(); + // Here we determine if we need to "clear" any defined Lifecycle rules by explicitly setting + // the Rule list of lifecycle to the empty list. + // In order for us to clear the rules, one of the three following must be true: + // 1. deleteRules is null while lifecycleRules is non-null and empty + // 2. lifecycleRules is null while deleteRules is non-null and empty + // 3. lifecycleRules is non-null and empty while deleteRules is non-null and empty + // If none of the above three is true, we will interpret as the Lifecycle rules being + // updated to the defined set of DeleteRule and LifecycleRule. if ((deleteRules == null && lifecycleRules.isEmpty()) || (lifecycleRules == null && deleteRules.isEmpty()) || (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty())) {