Skip to content

Commit 82fb014

Browse files
BenWhiteheadBenWhitehead
authored
fix: update GrpcConversions to use Bucket.RetentionPolicy.retention_duration (#1798)
Also update Apiary Conversions to carry through all fields, while updating HttpStorageRpc#update(Bucket) to take rpc specific action to filter down fields in the case of a patch.
1 parent 3414515 commit 82fb014

File tree

5 files changed

+152
-52
lines changed

5 files changed

+152
-52
lines changed

‎google-cloud-storage/src/main/java/com/google/cloud/storage/ApiaryConversions.java

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@
7373
import com.google.common.collect.ImmutableList;
7474
import com.google.common.collect.Maps;
7575
import java.math.BigInteger;
76-
import java.time.Duration;
7776
import java.time.OffsetDateTime;
7877
import java.util.ArrayList;
7978
import java.util.Collections;
@@ -375,19 +374,7 @@ private Bucket bucketInfoEncode(BucketInfo from) {
375374
k -> new Encryption().setDefaultKmsKeyName(k),
376375
to::setEncryption);
377376
ifNonNull(from.getLabels(), to::setLabels);
378-
Duration retentionPeriod = from.getRetentionPeriodDuration();
379-
if (retentionPeriod == null) {
380-
to.setRetentionPolicy(Data.nullOf(Bucket.RetentionPolicy.class));
381-
} else {
382-
Bucket.RetentionPolicy retentionPolicy = new Bucket.RetentionPolicy();
383-
retentionPolicy.setRetentionPeriod(durationSecondsCodec.encode(retentionPeriod));
384-
ifNonNull(
385-
from.getRetentionEffectiveTimeOffsetDateTime(),
386-
dateTimeCodec::encode,
387-
retentionPolicy::setEffectiveTime);
388-
ifNonNull(from.retentionPolicyIsLocked(), retentionPolicy::setIsLocked);
389-
to.setRetentionPolicy(retentionPolicy);
390-
}
377+
maybeEncodeRetentionPolicy(from, to);
391378
ifNonNull(from.getIamConfiguration(), this::iamConfigEncode, to::setIamConfiguration);
392379
ifNonNull(from.getAutoclass(), this::autoclassEncode, to::setAutoclass);
393380
ifNonNull(from.getLogging(), this::loggingEncode, to::setLogging);
@@ -434,13 +421,7 @@ private BucketInfo bucketInfoDecode(com.google.api.services.storage.model.Bucket
434421
to.setDefaultKmsKeyName(encryption.getDefaultKmsKeyName());
435422
}
436423

437-
RetentionPolicy retentionPolicy = from.getRetentionPolicy();
438-
if (retentionPolicy != null && retentionPolicy.getEffectiveTime() != null) {
439-
to.setRetentionEffectiveTimeOffsetDateTime(
440-
dateTimeCodec.decode(retentionPolicy.getEffectiveTime()));
441-
}
442-
ifNonNull(retentionPolicy, RetentionPolicy::getIsLocked, to::setRetentionPolicyIsLocked);
443-
ifNonNull(retentionPolicy, RetentionPolicy::getRetentionPeriod, to::setRetentionPeriod);
424+
maybeDecodeRetentionPolicy(from, to);
444425
ifNonNull(from.getIamConfiguration(), this::iamConfigDecode, to::setIamConfiguration);
445426
ifNonNull(from.getAutoclass(), this::autoclassDecode, to::setAutoclass);
446427
ifNonNull(from.getLogging(), this::loggingDecode, to::setLogging);
@@ -901,4 +882,39 @@ private Bucket.CustomPlacementConfig customPlacementConfigEncode(CustomPlacement
901882
private CustomPlacementConfig customPlacementConfigDecode(Bucket.CustomPlacementConfig from) {
902883
return CustomPlacementConfig.newBuilder().setDataLocations(from.getDataLocations()).build();
903884
}
885+
886+
private static void maybeEncodeRetentionPolicy(BucketInfo from, Bucket to) {
887+
if (from.getRetentionPeriodDuration() != null
888+
|| from.retentionPolicyIsLocked() != null
889+
|| from.getRetentionEffectiveTimeOffsetDateTime() != null) {
890+
RetentionPolicy retentionPolicy = new RetentionPolicy();
891+
ifNonNull(
892+
from.getRetentionPeriodDuration(),
893+
durationSecondsCodec::encode,
894+
retentionPolicy::setRetentionPeriod);
895+
ifNonNull(from.retentionPolicyIsLocked(), retentionPolicy::setIsLocked);
896+
ifNonNull(
897+
from.getRetentionEffectiveTimeOffsetDateTime(),
898+
dateTimeCodec::encode,
899+
retentionPolicy::setEffectiveTime);
900+
to.setRetentionPolicy(retentionPolicy);
901+
} else {
902+
to.setRetentionPolicy(Data.nullOf(Bucket.RetentionPolicy.class));
903+
}
904+
}
905+
906+
private static void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder to) {
907+
RetentionPolicy retentionPolicy = from.getRetentionPolicy();
908+
if (retentionPolicy != null && retentionPolicy.getEffectiveTime() != null) {
909+
to.setRetentionEffectiveTimeOffsetDateTime(
910+
dateTimeCodec.decode(retentionPolicy.getEffectiveTime()));
911+
}
912+
if (retentionPolicy != null) {
913+
ifNonNull(retentionPolicy.getIsLocked(), to::setRetentionPolicyIsLocked);
914+
ifNonNull(
915+
retentionPolicy.getRetentionPeriod(),
916+
durationSecondsCodec::decode,
917+
to::setRetentionPeriodDuration);
918+
}
919+
}
904920
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java

Lines changed: 46 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package com.google.cloud.storage;
1818

1919
import static com.google.cloud.storage.Utils.bucketNameCodec;
20-
import static com.google.cloud.storage.Utils.durationSecondsCodec;
2120
import static com.google.cloud.storage.Utils.ifNonNull;
2221
import static com.google.cloud.storage.Utils.lift;
2322
import static com.google.cloud.storage.Utils.projectNameCodec;
@@ -56,6 +55,7 @@
5655
import com.google.storage.v2.Owner;
5756
import com.google.type.Date;
5857
import com.google.type.Expr;
58+
import java.time.Duration;
5959
import java.time.Instant;
6060
import java.time.LocalDate;
6161
import java.time.OffsetDateTime;
@@ -118,6 +118,17 @@ final class GrpcConversions {
118118
.plusNanos(t.getNanos())
119119
.atOffset(ZoneOffset.UTC));
120120

121+
@VisibleForTesting
122+
final Codec<Duration, com.google.protobuf.Duration> durationCodec =
123+
Codec.of(
124+
javaDuration ->
125+
com.google.protobuf.Duration.newBuilder()
126+
.setSeconds(javaDuration.getSeconds())
127+
.setNanos(javaDuration.getNano())
128+
.build(),
129+
protoDuration ->
130+
Duration.ofSeconds(protoDuration.getSeconds()).plusNanos(protoDuration.getNanos()));
131+
121132
@VisibleForTesting
122133
final Codec<OffsetDateTime, Date> odtDateCodec =
123134
Codec.of(
@@ -200,18 +211,7 @@ private BucketInfo bucketInfoDecode(Bucket from) {
200211
BucketInfo.Builder to = new BucketInfo.BuilderImpl(bucketNameCodec.decode(from.getName()));
201212
to.setProject(from.getProject());
202213
to.setGeneratedId(from.getBucketId());
203-
if (from.hasRetentionPolicy()) {
204-
Bucket.RetentionPolicy retentionPolicy = from.getRetentionPolicy();
205-
to.setRetentionPolicyIsLocked(retentionPolicy.getIsLocked());
206-
if (retentionPolicy.hasRetentionPeriod()) {
207-
to.setRetentionPeriodDuration(
208-
durationSecondsCodec.decode(retentionPolicy.getRetentionPeriod()));
209-
}
210-
if (retentionPolicy.hasEffectiveTime()) {
211-
to.setRetentionEffectiveTimeOffsetDateTime(
212-
timestampCodec.decode(retentionPolicy.getEffectiveTime()));
213-
}
214-
}
214+
maybeDecodeRetentionPolicy(from, to);
215215
ifNonNull(from.getLocation(), to::setLocation);
216216
ifNonNull(from.getLocationType(), to::setLocationType);
217217
ifNonNull(from.getMetageneration(), to::setMetageneration);
@@ -303,21 +303,7 @@ private Bucket bucketInfoEncode(BucketInfo from) {
303303
Bucket.Builder to = Bucket.newBuilder();
304304
to.setName(bucketNameCodec.encode(from.getName()));
305305
ifNonNull(from.getGeneratedId(), to::setBucketId);
306-
if (from.getRetentionPeriodDuration() != null) {
307-
Bucket.RetentionPolicy.Builder retentionPolicyBuilder = to.getRetentionPolicyBuilder();
308-
ifNonNull(
309-
from.getRetentionPeriodDuration(),
310-
durationSecondsCodec::encode,
311-
retentionPolicyBuilder::setRetentionPeriod);
312-
ifNonNull(from.retentionPolicyIsLocked(), retentionPolicyBuilder::setIsLocked);
313-
if (from.retentionPolicyIsLocked() == Boolean.TRUE) {
314-
ifNonNull(
315-
from.getRetentionEffectiveTimeOffsetDateTime(),
316-
timestampCodec::encode,
317-
retentionPolicyBuilder::setEffectiveTime);
318-
}
319-
to.setRetentionPolicy(retentionPolicyBuilder.build());
320-
}
306+
maybeEncodeRetentionPolicy(from, to);
321307
ifNonNull(from.getLocation(), to::setLocation);
322308
ifNonNull(from.getLocationType(), to::setLocationType);
323309
ifNonNull(from.getMetageneration(), to::setMetageneration);
@@ -395,6 +381,38 @@ private Bucket bucketInfoEncode(BucketInfo from) {
395381
return to.build();
396382
}
397383

384+
private void maybeEncodeRetentionPolicy(BucketInfo from, Bucket.Builder to) {
385+
if (from.getRetentionPeriodDuration() != null
386+
|| from.retentionPolicyIsLocked() != null
387+
|| from.getRetentionEffectiveTimeOffsetDateTime() != null) {
388+
Bucket.RetentionPolicy.Builder retentionPolicyBuilder = to.getRetentionPolicyBuilder();
389+
ifNonNull(
390+
from.getRetentionPeriodDuration(),
391+
durationCodec::encode,
392+
retentionPolicyBuilder::setRetentionDuration);
393+
ifNonNull(from.retentionPolicyIsLocked(), retentionPolicyBuilder::setIsLocked);
394+
ifNonNull(
395+
from.getRetentionEffectiveTimeOffsetDateTime(),
396+
timestampCodec::encode,
397+
retentionPolicyBuilder::setEffectiveTime);
398+
to.setRetentionPolicy(retentionPolicyBuilder.build());
399+
}
400+
}
401+
402+
private void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder to) {
403+
if (from.hasRetentionPolicy()) {
404+
Bucket.RetentionPolicy retentionPolicy = from.getRetentionPolicy();
405+
to.setRetentionPolicyIsLocked(retentionPolicy.getIsLocked());
406+
if (retentionPolicy.hasRetentionDuration()) {
407+
to.setRetentionPeriodDuration(durationCodec.decode(retentionPolicy.getRetentionDuration()));
408+
}
409+
if (retentionPolicy.hasEffectiveTime()) {
410+
to.setRetentionEffectiveTimeOffsetDateTime(
411+
timestampCodec.decode(retentionPolicy.getEffectiveTime()));
412+
}
413+
}
414+
}
415+
398416
private Bucket.Logging loggingEncode(BucketInfo.Logging from) {
399417
Bucket.Logging.Builder to = Bucket.Logging.newBuilder();
400418
if (from.getLogObjectPrefix() != null && !from.getLogObjectPrefix().isEmpty()) {

‎google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,12 @@
4141
import com.google.api.client.http.json.JsonHttpContent;
4242
import com.google.api.client.json.JsonFactory;
4343
import com.google.api.client.json.jackson2.JacksonFactory;
44+
import com.google.api.client.util.Data;
4445
import com.google.api.services.storage.Storage;
4546
import com.google.api.services.storage.Storage.Objects.Get;
4647
import com.google.api.services.storage.Storage.Objects.Insert;
4748
import com.google.api.services.storage.model.Bucket;
49+
import com.google.api.services.storage.model.Bucket.RetentionPolicy;
4850
import com.google.api.services.storage.model.BucketAccessControl;
4951
import com.google.api.services.storage.model.Buckets;
5052
import com.google.api.services.storage.model.ComposeRequest;
@@ -534,6 +536,18 @@ public Bucket patch(Bucket bucket, Map<Option, ?> options) {
534536
Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_PATCH_BUCKET);
535537
Scope scope = tracer.withSpan(span);
536538
try {
539+
RetentionPolicy retentionPolicy = bucket.getRetentionPolicy();
540+
if (retentionPolicy != null) {
541+
// according to https://cloud.google.com/storage/docs/json_api/v1/buckets both effectiveTime
542+
// and isLocked are output_only. If retentionPeriod is null, null out the whole
543+
// RetentionPolicy.
544+
if (retentionPolicy.getRetentionPeriod() == null) {
545+
// Using Data.nullOf here is important here so the null value is written into the request
546+
// json. The explicit null values tells the backend to remove the policy.
547+
bucket.setRetentionPolicy(Data.nullOf(RetentionPolicy.class));
548+
}
549+
}
550+
537551
String projection = Option.PROJECTION.getString(options);
538552
if (bucket.getIamConfiguration() != null
539553
&& bucket.getIamConfiguration().getBucketPolicyOnly() != null

‎google-cloud-storage/src/test/java/com/google/cloud/storage/DurationCodecPropertyTest.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/*
2+
* Copyright 2022 Google LLC
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.cloud.storage;
18+
19+
import static com.google.common.truth.Truth.assertThat;
20+
21+
import com.google.cloud.storage.Conversions.Codec;
22+
import com.google.cloud.storage.jqwik.StorageArbitraries;
23+
import com.google.protobuf.Duration;
24+
import net.jqwik.api.Arbitrary;
25+
import net.jqwik.api.ArbitrarySupplier;
26+
import net.jqwik.api.ForAll;
27+
import net.jqwik.api.Property;
28+
29+
final class DurationCodecPropertyTest {
30+
31+
@Property
32+
void timestampCodecShouldRoundTrip(@ForAll(supplier = Supp.class) Duration ts) {
33+
Codec<java.time.Duration, Duration> codec = GrpcConversions.INSTANCE.durationCodec;
34+
java.time.Duration decode = codec.decode(ts);
35+
Duration encode = codec.encode(decode);
36+
37+
assertThat(encode).isEqualTo(ts);
38+
}
39+
40+
private static final class Supp implements ArbitrarySupplier<Duration> {
41+
@Override
42+
public Arbitrary<Duration> get() {
43+
return StorageArbitraries.duration();
44+
}
45+
}
46+
}

‎google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,12 @@ public static Arbitrary<Timestamp> timestamp() {
7878
Timestamp.newBuilder().setSeconds(odt.toEpochSecond()).setNanos(nanos).build());
7979
}
8080

81+
public static Arbitrary<com.google.protobuf.Duration> duration() {
82+
return Arbitraries.longs()
83+
.between(0, 315_576_000_000L)
84+
.map(seconds -> com.google.protobuf.Duration.newBuilder().setSeconds(seconds).build());
85+
}
86+
8187
public static Arbitrary<Date> date() {
8288
return DateTimes.offsetDateTimes()
8389
.offsetBetween(ZoneOffset.UTC, ZoneOffset.UTC)
@@ -384,11 +390,11 @@ public Arbitrary<Bucket.Encryption> encryption() {
384390
}
385391

386392
public Arbitrary<Bucket.RetentionPolicy> retentionPolicy() {
387-
return Combinators.combine(bool(), Arbitraries.longs().greaterOrEqual(0), timestamp())
393+
return Combinators.combine(bool(), duration().injectNull(0.25), timestamp())
388394
.as(
389-
(locked, period, effectiveTime) -> {
395+
(locked, duration, effectiveTime) -> {
390396
RetentionPolicy.Builder retentionBuilder = RetentionPolicy.newBuilder();
391-
retentionBuilder.setRetentionPeriod(period);
397+
ifNonNull(duration, retentionBuilder::setRetentionDuration);
392398
retentionBuilder.setIsLocked(locked);
393399
if (locked) {
394400
retentionBuilder.setEffectiveTime(effectiveTime);

0 commit comments

Comments
 (0)