feat: allow specifying a negative offset to ReadChannel (#1916)

BenWhitehead · web-flow · commit 6df546945211 · 2023-03-01T13:07:50.000-05:00
GCS Supports negative offsets in ranged reads and is documented in https://cloud.google.com/storage/docs/json_api/v1/parameters#range Update the ReadChannel returned from Storage.reader to allow providing a negative value to `seek`. NOTE: `seek` to a negative offset will be ignored by GCS if `limit` is also specified
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BaseStorageReadChannel.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BaseStorageReadChannel.java
@@ -88,7 +88,9 @@ public final synchronized int read(ByteBuffer dst) throws IOException {
       throw new ClosedChannelException();
     }
     long diff = byteRangeSpec.length();
-    if (diff <= 0) {
+    // the check on beginOffset >= 0 used to be a precondition on seek(long)
+    // move it here to preserve existing behavior while allowing new negative offsets
+    if (diff <= 0 && byteRangeSpec.beginOffset() >= 0) {
       return -1;
     }
     try {
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/ByteRangeSpec.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/ByteRangeSpec.java
@@ -472,6 +472,8 @@ public ReadObjectRequest.Builder seekReadObjectRequest(ReadObjectRequest.Builder
     protected String fmtAsHttpRangeHeader() throws ArithmeticException {
       if (beginOffset > 0) {
         return String.format("bytes=%d-", beginOffset);
+      } else if (beginOffset < 0) {
+        return String.format("bytes=%d", beginOffset);
       } else {
         return null;
       }
@@ -510,7 +512,7 @@ long length() throws ArithmeticException {
 
     @Override
     ByteRangeSpec withNewBeginOffset(long beginOffset) {
-      if (beginOffset > 0) {
+      if (beginOffset != 0) {
         return new LeftClosedByteRangeSpec(beginOffset);
       } else {
         return this;
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GapicUnbufferedReadableByteChannel.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GapicUnbufferedReadableByteChannel.java
@@ -173,7 +173,6 @@ private static final class ReadCursor {
     private final long limit;
 
     private ReadCursor(long beginning, long limit) {
-      checkArgument(0 <= beginning && beginning <= limit, "0 <= %d <= %d", beginning, limit);
       this.limit = limit;
       this.beginning = beginning;
       this.offset = beginning;
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageReadChannel.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageReadChannel.java
@@ -41,7 +41,6 @@ default ByteRangeSpec getByteRangeSpec() {
   @SuppressWarnings("resource")
   @Override
   default void seek(long position) throws IOException {
-    checkArgument(position >= 0, "position must be >= 0");
     try {
       setByteRangeSpec(getByteRangeSpec().withNewBeginOffset(position));
     } catch (StorageException e) {
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ByteRangeSpecTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ByteRangeSpecTest.java
@@ -45,6 +45,20 @@ public final class ByteRangeSpecTest {
 
   public static final class Behavior {
 
+    @Test
+    public void negativeBeginOffset() throws Exception {
+      ByteRangeSpec rel = ByteRangeSpec.relativeLength(-5L, null);
+      ByteRangeSpec exO = ByteRangeSpec.explicit(-5L, null);
+      ByteRangeSpec exC = ByteRangeSpec.explicitClosed(-5L, null);
+      threeWayEqual(exO, exC, rel);
+    }
+
+    @Test
+    public void negativeBeginOffset_fromNull() {
+      ByteRangeSpec spec = ByteRangeSpec.nullRange().withNewBeginOffset(-5L);
+      assertThat(spec.getHttpRangeHeader()).isEqualTo("bytes=-5");
+    }
+
     @Test
     public void beginNonNullZero_endNonNullNonInfinity() throws Exception {
       ByteRangeSpec rel = ByteRangeSpec.relativeLength(0L, 52L);
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBlobReadChannelTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBlobReadChannelTest.java
@@ -459,6 +459,27 @@ public void readingLastByteReturnsOneByte_seekOnly() throws IOException {
     }
   }
 
+  @Test
+  public void readingLastByteReturnsOneByte_seekOnly_negativeOffset() throws IOException {
+    int length = 10;
+    byte[] bytes = DataGenerator.base64Characters().genBytes(length);
+
+    BlobInfo info1 = BlobInfo.newBuilder(bucket, generator.randomObjectName()).build();
+    Blob gen1 = storage.create(info1, bytes, BlobTargetOption.doesNotExist());
+
+    byte[] expected1 = Arrays.copyOfRange(bytes, 9, 10);
+    String xxdExpected1 = xxd(expected1);
+    try (ReadChannel reader = storage.reader(gen1.getBlobId());
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        WritableByteChannel writer = Channels.newChannel(baos)) {
+      reader.seek(-1);
+      ByteStreams.copy(reader, writer);
+      byte[] bytes1 = baos.toByteArray();
+      String xxd1 = xxd(bytes1);
+      assertThat(xxd1).isEqualTo(xxdExpected1);
+    }
+  }
+
   @Test
   public void readingLastByteReturnsOneByte_seekAndLimit() throws IOException {
     int length = 10;

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,9 @@ public final synchronized int read(ByteBuffer dst) throws IOException {`
`88`	`88`	`throw new ClosedChannelException();`
`89`	`89`	`}`
`90`	`90`	`long diff = byteRangeSpec.length();`
`91`		`- if (diff <= 0) {`
	`91`	`+ // the check on beginOffset >= 0 used to be a precondition on seek(long)`
	`92`	`+ // move it here to preserve existing behavior while allowing new negative offsets`
	`93`	`+ if (diff <= 0 && byteRangeSpec.beginOffset() >= 0) {`
`92`	`94`	`return -1;`
`93`	`95`	`}`
`94`	`96`	`try {`