Skip to content

Commit 7b597af

Browse files
authored
chore: remove cumulative crc32c check on GapicUnbufferedReadableByteChannel (#1903)
Cumulative crc32c check is only applicable to an initial offset of 0 because the crc32c value return for a resource is for the entire resource. Instead, we continue to perform the crc32c integrity check for each ChecksummedData which is always for the bytes it is packed with.
1 parent bec36b7 commit 7b597af

File tree

2 files changed

+0
-71
lines changed

2 files changed

+0
-71
lines changed

google-cloud-storage/src/main/java/com/google/cloud/storage/GapicUnbufferedReadableByteChannel.java

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import com.google.cloud.storage.UnbufferedReadableByteChannelSession.UnbufferedReadableByteChannel;
2828
import com.google.storage.v2.ChecksummedData;
2929
import com.google.storage.v2.Object;
30-
import com.google.storage.v2.ObjectChecksums;
3130
import com.google.storage.v2.ReadObjectRequest;
3231
import com.google.storage.v2.ReadObjectResponse;
3332
import java.io.Closeable;
@@ -50,12 +49,9 @@ final class GapicUnbufferedReadableByteChannel
5049

5150
private boolean open = true;
5251
private boolean complete = false;
53-
private boolean ioExceptionAlreadyThrown = false;
5452
private long blobOffset;
55-
private Crc32cLengthKnown cumulativeCrc32c;
5653

5754
private Object metadata;
58-
private ObjectChecksums objectChecksums;
5955

6056
private ByteBuffer leftovers;
6157

@@ -110,30 +106,16 @@ public long read(ByteBuffer[] dsts, int offset, int length) throws IOException {
110106
result.set(metadata);
111107
}
112108
}
113-
if (resp.hasObjectChecksums()) {
114-
ObjectChecksums checksums = resp.getObjectChecksums();
115-
if (this.objectChecksums == null) {
116-
this.objectChecksums = checksums;
117-
} else if (!objectChecksums.equals(checksums)) {
118-
throw closeWithError(
119-
String.format(
120-
"Mismatch checksums between subsequent reads. Expected %s but received %s",
121-
Crc32cValue.fmtCrc32cValue(objectChecksums.getCrc32C()),
122-
Crc32cValue.fmtCrc32cValue(checksums.getCrc32C())));
123-
}
124-
}
125109
ChecksummedData checksummedData = resp.getChecksummedData();
126110
ByteBuffer content = checksummedData.getContent().asReadOnlyByteBuffer();
127111
// very important to know whether a crc32c value is set. Without checking, protobuf will
128112
// happily return 0, which is a valid crc32c value.
129113
if (checksummedData.hasCrc32C()) {
130114
Crc32cLengthKnown expected =
131115
Crc32cValue.of(checksummedData.getCrc32C(), checksummedData.getContent().size());
132-
cumulativeCrc32c = hasher.nullSafeConcat(cumulativeCrc32c, expected);
133116
try {
134117
hasher.validate(expected, content::duplicate);
135118
} catch (IOException e) {
136-
ioExceptionAlreadyThrown = true;
137119
close();
138120
throw e;
139121
}
@@ -161,18 +143,6 @@ public boolean isOpen() {
161143

162144
@Override
163145
public void close() throws IOException {
164-
if (!ioExceptionAlreadyThrown
165-
&& cumulativeCrc32c != null
166-
&& objectChecksums != null
167-
&& objectChecksums.hasCrc32C()) {
168-
Crc32cLengthKnown expected = Crc32cValue.of(objectChecksums.getCrc32C(), metadata.getSize());
169-
if (!expected.eqValue(cumulativeCrc32c)) {
170-
throw new IOException(
171-
String.format(
172-
"Mismatch checksum value. Expected %s actual %s",
173-
expected.debugString(), cumulativeCrc32c.debugString()));
174-
}
175-
}
176146
open = false;
177147
iter.close();
178148
}
@@ -187,7 +157,6 @@ private void copy(ReadCursor c, ByteBuffer content, ByteBuffer[] dsts, int offse
187157
}
188158

189159
private IOException closeWithError(String message) throws IOException {
190-
ioExceptionAlreadyThrown = true;
191160
close();
192161
StorageException cause =
193162
new StorageException(HttpStatusCodes.STATUS_CODE_PRECONDITION_FAILED, message);

google-cloud-storage/src/test/java/com/google/cloud/storage/GapicUnbufferedReadableByteChannelTest.java

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -271,46 +271,6 @@ public void readObject(
271271
}
272272
}
273273

274-
@Test
275-
public void ioException_if_crc32c_mismatch_cumulative() throws IOException, InterruptedException {
276-
StorageGrpc.StorageImplBase fakeStorage =
277-
new StorageGrpc.StorageImplBase() {
278-
@Override
279-
public void readObject(
280-
ReadObjectRequest request, StreamObserver<ReadObjectResponse> responseObserver) {
281-
if (request.equals(req1)) {
282-
responseObserver.onNext(resp1);
283-
responseObserver.onCompleted();
284-
} else {
285-
responseObserver.onError(apiException(Code.PERMISSION_DENIED));
286-
}
287-
}
288-
};
289-
try (FakeServer server = FakeServer.of(fakeStorage);
290-
StorageClient storageClient = StorageClient.create(server.storageSettings())) {
291-
292-
UnbufferedReadableByteChannelSession<Object> session =
293-
new UnbufferedReadSession<>(
294-
ApiFutures.immediateFuture(req1),
295-
(start, resultFuture) ->
296-
new GapicUnbufferedReadableByteChannel(
297-
resultFuture,
298-
storageClient
299-
.readObjectCallable()
300-
.withDefaultCallContext(
301-
contextWithRetryForCodes(StatusCode.Code.DATA_LOSS)),
302-
start,
303-
Hasher.enabled()));
304-
byte[] actualBytes = new byte[40];
305-
//noinspection resource
306-
UnbufferedReadableByteChannel c = session.open();
307-
c.read(ByteBuffer.wrap(actualBytes));
308-
309-
IOException ioException = assertThrows(IOException.class, c::close);
310-
assertThat(ioException).hasMessageThat().contains("Mismatch checksum");
311-
}
312-
}
313-
314274
@Test
315275
public void overRead_handledProperly() throws IOException, InterruptedException {
316276
StorageGrpc.StorageImplBase fakeStorage =

0 commit comments

Comments
 (0)