diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Crc32cValue.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Crc32cValue.java index 245999dd76..5da5a20376 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Crc32cValue.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Crc32cValue.java @@ -16,6 +16,7 @@ package com.google.cloud.storage; +import java.nio.ByteBuffer; import java.util.Locale; import java.util.Objects; @@ -56,6 +57,10 @@ public boolean eqValue(Crc32cValue other) { return this.getValue() == other.getValue(); } + static Crc32cLengthKnown zero() { + return Crc32cLengthKnown.ZERO; + } + static Crc32cLengthUnknown of(int value) { return new Crc32cLengthUnknown(value); } @@ -81,6 +86,9 @@ public int getValue() { @Override public Crc32cLengthUnknown concat(Crc32cLengthKnown other) { + if (other == Crc32cLengthKnown.ZERO) { + return this; + } int combined = Crc32cUtility.concatCrc32c(value, other.value, other.length); return new Crc32cLengthUnknown(combined); } @@ -118,6 +126,7 @@ public int hashCode() { } static final class Crc32cLengthKnown extends Crc32cValue { + private static final Crc32cLengthKnown ZERO = Hasher.enabled().hash(ByteBuffer.allocate(0)); private final int value; private final long length; @@ -137,6 +146,11 @@ public long getLength() { @Override public Crc32cLengthKnown concat(Crc32cLengthKnown other) { + if (other == ZERO) { + return this; + } else if (this == ZERO) { + return other; + } int combined = Crc32cUtility.concatCrc32c(value, other.value, other.length); return new Crc32cLengthKnown(combined, length + other.length); } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Hasher.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Hasher.java index 3342fd8c3b..7ae9a756eb 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Hasher.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Hasher.java @@ -27,10 +27,13 @@ import java.util.List; import java.util.Locale; import java.util.function.Supplier; +import javax.annotation.ParametersAreNonnullByDefault; import javax.annotation.concurrent.Immutable; import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; +@SuppressWarnings("ClassEscapesDefinedScope") +@ParametersAreNonnullByDefault interface Hasher { @Nullable @@ -49,13 +52,14 @@ default Crc32cLengthKnown hash(Supplier b) { void validateUnchecked(Crc32cValue expected, ByteString byteString) throws UncheckedChecksumMismatchException; - @Nullable Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2); + @Nullable Crc32cLengthKnown nullSafeConcat( + @Nullable Crc32cLengthKnown r1, @NonNull Crc32cLengthKnown r2); - static Hasher noop() { + static NoOpHasher noop() { return NoOpHasher.INSTANCE; } - static Hasher enabled() { + static GuavaHasher enabled() { return GuavaHasher.INSTANCE; } @@ -85,7 +89,8 @@ public void validate(Crc32cValue expected, ByteString b) {} public void validateUnchecked(Crc32cValue expected, ByteString byteString) {} @Override - public @Nullable Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2) { + public @Nullable Crc32cLengthKnown nullSafeConcat( + @Nullable Crc32cLengthKnown r1, @NonNull Crc32cLengthKnown r2) { return null; } } @@ -107,7 +112,7 @@ private GuavaHasher() {} return Crc32cValue.of(Hashing.crc32c().hashBytes(b).asInt(), remaining); } - @SuppressWarnings({"ConstantConditions", "UnstableApiUsage"}) + @SuppressWarnings({"UnstableApiUsage"}) @Override public @NonNull Crc32cLengthKnown hash(ByteString byteString) { List buffers = byteString.asReadOnlyByteBufferList(); @@ -118,7 +123,6 @@ private GuavaHasher() {} return Crc32cValue.of(crc32c.hash().asInt(), byteString.size()); } - @SuppressWarnings({"ConstantConditions"}) @Override public void validate(Crc32cValue expected, ByteString byteString) throws ChecksumMismatchException { @@ -137,7 +141,6 @@ public void validate(Crc32cValue expected, Supplier b) } } - @SuppressWarnings({"ConstantConditions"}) @Override public void validateUnchecked(Crc32cValue expected, ByteString byteString) throws UncheckedChecksumMismatchException { @@ -149,9 +152,10 @@ public void validateUnchecked(Crc32cValue expected, ByteString byteString) @Override @Nullable - public Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2) { + public Crc32cLengthKnown nullSafeConcat( + @Nullable Crc32cLengthKnown r1, @NonNull Crc32cLengthKnown r2) { if (r1 == null) { - return r2; + return null; } else { return r1.concat(r2); } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/WriteCtx.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/WriteCtx.java index 9816d4dc0e..21eaa59a29 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/WriteCtx.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/WriteCtx.java @@ -31,13 +31,22 @@ final class WriteCtx { private final AtomicLong totalSentBytes; private final AtomicLong confirmedBytes; - private final AtomicReference cumulativeCrc32c; + private final AtomicReference<@Nullable Crc32cLengthKnown> cumulativeCrc32c; WriteCtx(RequestFactoryT requestFactory) { + this(requestFactory, null); + } + + /** + * TODO: Remove initialValue and replace with Crc32cValue.zero() once all uploads have been + * updated to do e2e checksumming by default. + */ + @Deprecated + WriteCtx(RequestFactoryT requestFactory, @Nullable Crc32cLengthKnown initialValue) { this.requestFactory = requestFactory; this.totalSentBytes = new AtomicLong(0); this.confirmedBytes = new AtomicLong(0); - this.cumulativeCrc32c = new AtomicReference<>(); + this.cumulativeCrc32c = new AtomicReference<>(initialValue); } public RequestFactoryT getRequestFactory() { @@ -56,7 +65,7 @@ public AtomicLong getConfirmedBytes() { return confirmedBytes; } - public AtomicReference getCumulativeCrc32c() { + public AtomicReference<@Nullable Crc32cLengthKnown> getCumulativeCrc32c() { return cumulativeCrc32c; } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/Crc32cValueTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/Crc32cValueTest.java index 600b1337b9..1565e926c6 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/Crc32cValueTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/Crc32cValueTest.java @@ -20,9 +20,11 @@ import com.google.cloud.storage.Crc32cValue.Crc32cLengthKnown; import com.google.cloud.storage.Crc32cValue.Crc32cLengthUnknown; +import com.google.cloud.storage.it.ChecksummedTestContent; import com.google.common.hash.HashFunction; import com.google.common.hash.Hashing; import net.jqwik.api.Example; +import org.checkerframework.checker.nullness.qual.NonNull; final class Crc32cValueTest { @@ -67,4 +69,30 @@ public void ensureConcatSatisfiesTheLeftDistributedProperty() { assertThat(nesting.getValue()).isEqualTo(expected); assertThat(mixed.getValue()).isEqualTo(expected); } + + @Example + void zeroDoesNotTransform() { + Crc32cLengthKnown base = + Hasher.enabled().hash(DataGenerator.base64Characters().genByteBuffer(64)); + + assertThat(base.concat(Crc32cValue.zero())).isSameInstanceAs(base); + assertThat(Crc32cValue.zero().concat(base)).isSameInstanceAs(base); + } + + @Example + void nullSafeConcat_isAlwaysNull() { + ChecksummedTestContent testContent = + ChecksummedTestContent.of(DataGenerator.base64Characters().genBytes(2 * 1024 * 1024)); + + Crc32cLengthKnown actual = + testContent.chunkup(373).stream() + .map(Crc32cValueTest::toCrc32cValue) + .reduce(null, Hasher.enabled()::nullSafeConcat); + + assertThat(actual).isNull(); + } + + private static @NonNull Crc32cLengthKnown toCrc32cValue(ChecksummedTestContent testContent) { + return Crc32cValue.of(testContent.getCrc32c(), testContent.getBytes().length); + } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGapicUnbufferedWritableByteChannelTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGapicUnbufferedWritableByteChannelTest.java index a31fc0c337..daf4554cf7 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGapicUnbufferedWritableByteChannelTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITGapicUnbufferedWritableByteChannelTest.java @@ -159,7 +159,10 @@ public void directUpload() throws IOException, InterruptedException, ExecutionEx SettableApiFuture result = SettableApiFuture.create(); try (GapicUnbufferedDirectWritableByteChannel c = new GapicUnbufferedDirectWritableByteChannel( - result, segmenter, sc.writeObjectCallable(), new WriteCtx<>(reqFactory))) { + result, + segmenter, + sc.writeObjectCallable(), + new WriteCtx<>(reqFactory, Crc32cValue.zero()))) { c.write(ByteBuffer.wrap(bytes)); } assertThat(result.get()).isEqualTo(resp); @@ -185,7 +188,7 @@ public void resumableUpload() throws IOException, InterruptedException, Executio result, segmenter, sc.writeObjectCallable(), - new WriteCtx<>(reqFactory), + new WriteCtx<>(reqFactory, Crc32cValue.zero()), RetrierWithAlg.attemptOnce(), Retrying::newCallContext); ArrayList debugMessages = new ArrayList<>(); @@ -267,7 +270,7 @@ public void resumableUpload_chunkAutomaticRetry() result, segmenter, sc.writeObjectCallable(), - new WriteCtx<>(reqFactory), + new WriteCtx<>(reqFactory, Crc32cValue.zero()), TestUtils.retrierFromStorageOptions(fake.getGrpcStorageOptions()) .withAlg(Retrying.alwaysRetry()), Retrying::newCallContext)) { @@ -319,7 +322,7 @@ public void resumableUpload_finalizeWhenWriteAndCloseCalledEvenWhenQuantumAligne result, segmenter, sc.writeObjectCallable(), - new WriteCtx<>(reqFactory), + new WriteCtx<>(reqFactory, Crc32cValue.zero()), RetrierWithAlg.attemptOnce(), Retrying::newCallContext); try { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java index a4902111ff..14e9ef1ec7 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java @@ -892,7 +892,8 @@ public BlobInfo compose(ComposeRequest composeRequest) { .map(Data::getCrc32c) .collect(ImmutableList.toImmutableList()); - Crc32cLengthKnown reduce = crc32cs.stream().reduce(null, HASHER::nullSafeConcat); + Crc32cLengthKnown reduce = + crc32cs.stream().reduce(Crc32cValue.zero(), Crc32cLengthKnown::concat); Preconditions.checkState(reduce != null, "unable to compute crc32c for compose request"); b.setCrc32c(Utils.crc32cCodec.encode(reduce.getValue())); BlobInfo gen1 = b.build(); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ChecksummedTestContent.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ChecksummedTestContent.java index aa87e64780..dfba635768 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ChecksummedTestContent.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ChecksummedTestContent.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkPositionIndexes; import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableList; import com.google.common.hash.Hashing; import com.google.common.io.BaseEncoding; import com.google.common.primitives.Ints; @@ -28,8 +29,10 @@ import com.google.storage.v2.ChecksummedData; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; +import java.util.List; public final class ChecksummedTestContent { @@ -100,6 +103,14 @@ public ChecksummedTestContent slice(int begin, int length) { return of(bytes, begin, Math.min(length, bytes.length - begin)); } + public List chunkup(int chunkSize) { + List elements = new ArrayList<>(); + for (int i = 0; i < bytes.length; i += chunkSize) { + elements.add(slice(i, chunkSize)); + } + return ImmutableList.copyOf(elements); + } + @Override public String toString() { return MoreObjects.toStringHelper(this)