Skip to content

Commit 7e6adac

Browse files
normanrzclaude
andauthored
Support open-ended ranges in getInputStream, fail loudly on unreadable zip (#103)
HttpStore.getInputStream formatted "bytes=%d-%d" unconditionally, so the default Store.getInputStream(keys) — which delegates to (keys, 0, -1) — sent "bytes=0--1" and returned null. Emit "bytes=<start>-" when end is negative, mirroring get(keys, start). S3Store had the identical bug. ReadOnlyZipStore.ensureCache treated a null stream from the underlying store as an empty archive, turning a read failure into valid-but-empty data. Throw StoreException.readFailed instead; a genuinely empty archive still parses to an empty index through the normal path. Fixes #100 Fixes #101 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent fc418e8 commit 7e6adac

6 files changed

Lines changed: 78 additions & 6 deletions

File tree

src/main/java/dev/zarr/zarrjava/store/HttpStore.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,12 @@ public InputStream getInputStream(String[] keys, long start, long end) {
128128
if (start < 0) {
129129
throw new IllegalArgumentException("Argument 'start' needs to be non-negative.");
130130
}
131-
Request request = new Request.Builder().url(resolveKeys(keys)).header(
132-
"Range", String.format("bytes=%d-%d", start, end - 1)).build();
131+
// A negative end means "until the end of the object", which HTTP expresses as an
132+
// open-ended range.
133+
String range = end < 0
134+
? String.format("bytes=%d-", start)
135+
: String.format("bytes=%d-%d", start, end - 1);
136+
Request request = new Request.Builder().url(resolveKeys(keys)).header("Range", range).build();
133137

134138
try {
135139
// We do NOT use try-with-resources here because the stream must remain open

src/main/java/dev/zarr/zarrjava/store/ReadOnlyZipStore.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,10 @@ private synchronized void ensureCache() {
5050

5151
InputStream inputStream = underlyingStore.getInputStream();
5252
if (inputStream == null) {
53-
isCached = true;
54-
return;
53+
throw StoreException.readFailed(
54+
underlyingStore.toString(),
55+
new String[]{},
56+
new IOException("Could not open the ZIP archive from the underlying store"));
5557
}
5658

5759
try (ZipArchiveInputStream zis = new ZipArchiveInputStream(inputStream)) {

src/main/java/dev/zarr/zarrjava/store/S3Store.java

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,17 @@ public ByteBuffer get(String[] keys, long start) {
103103
@Nullable
104104
@Override
105105
public ByteBuffer get(String[] keys, long start, long end) {
106+
if (start < 0) {
107+
throw new IllegalArgumentException("Argument 'start' needs to be non-negative.");
108+
}
109+
// S3 ranges are inclusive; a negative end means "until the end of the object".
110+
String range = end < 0
111+
? String.format("bytes=%d-", start)
112+
: String.format("bytes=%d-%d", start, end - 1);
106113
GetObjectRequest req = GetObjectRequest.builder()
107114
.bucket(bucketName)
108115
.key(resolveKeys(keys))
109-
.range(String.format("bytes=%d-%d", start, end - 1)) // S3 range is inclusive
116+
.range(range)
110117
.build();
111118
return get(req);
112119
}
@@ -221,10 +228,17 @@ public StoreHandle resolve(String... keys) {
221228

222229
@Override
223230
public InputStream getInputStream(String[] keys, long start, long end) {
231+
if (start < 0) {
232+
throw new IllegalArgumentException("Argument 'start' needs to be non-negative.");
233+
}
234+
// S3 ranges are inclusive; a negative end means "until the end of the object".
235+
String range = end < 0
236+
? String.format("bytes=%d-", start)
237+
: String.format("bytes=%d-%d", start, end - 1);
224238
GetObjectRequest req = GetObjectRequest.builder()
225239
.bucket(bucketName)
226240
.key(resolveKeys(keys))
227-
.range(String.format("bytes=%d-%d", start, end - 1)) // S3 range is inclusive
241+
.range(range)
228242
.build();
229243
return s3client.getObject(req);
230244
}

src/test/java/dev/zarr/zarrjava/store/HttpStoreTest.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,28 @@ public void testNoRetryOn404() throws IOException {
8787
}
8888
}
8989

90+
@Test
91+
public void testOpenEndedRangeHeader() throws IOException, InterruptedException {
92+
try (MockWebServer server = new MockWebServer()) {
93+
server.enqueue(new MockResponse().setBody("data").setResponseCode(206));
94+
server.start();
95+
HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10);
96+
Assertions.assertNotNull(httpStore.getInputStream(new String[]{"path"}, 0, -1));
97+
Assertions.assertEquals("bytes=0-", server.takeRequest().getHeader("Range"));
98+
}
99+
}
100+
101+
@Test
102+
public void testBoundedRangeHeader() throws IOException, InterruptedException {
103+
try (MockWebServer server = new MockWebServer()) {
104+
server.enqueue(new MockResponse().setBody("dat").setResponseCode(206));
105+
server.start();
106+
HttpStore httpStore = new HttpStore(server.url("/").toString(), 1, 3, 10);
107+
Assertions.assertNotNull(httpStore.getInputStream(new String[]{"path"}, 1, 4));
108+
Assertions.assertEquals("bytes=1-3", server.takeRequest().getHeader("Range"));
109+
}
110+
}
111+
90112
@Override
91113
@Test
92114
@Disabled("List is not supported in HttpStore")

src/test/java/dev/zarr/zarrjava/store/ReadOnlyZipStoreTest.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,13 @@ Store storeWithArrays() {
4242
}
4343

4444

45+
@Test
46+
public void testUnreadableArchiveThrows() {
47+
ReadOnlyZipStore zipStore = new ReadOnlyZipStore(TESTOUTPUT.resolve("does_not_exist.zip"));
48+
Assertions.assertThrows(StoreException.class, () -> zipStore.resolve().listChildren().count());
49+
Assertions.assertThrows(StoreException.class, () -> zipStore.resolve("array", "0.0.0").exists());
50+
}
51+
4552
@Override
4653
@Test
4754
public void testListChildren() {

src/test/java/dev/zarr/zarrjava/store/StoreTest.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,29 @@ public void testInputStream() throws IOException {
4646
Assertions.assertArrayEquals(expectedBuffer, buffer);
4747
}
4848

49+
@Test
50+
public void testInputStreamOpenEnded() throws IOException {
51+
StoreHandle storeHandle = storeHandleWithData();
52+
ByteBuffer expected = storeHandle.read();
53+
try (InputStream is = storeHandle.getInputStream()) {
54+
Assertions.assertNotNull(is, "Open-ended getInputStream() returned null");
55+
byte[] actual = readFully(is);
56+
byte[] expectedBytes = new byte[expected.remaining()];
57+
expected.get(expectedBytes);
58+
Assertions.assertArrayEquals(expectedBytes, actual);
59+
}
60+
}
61+
62+
private static byte[] readFully(InputStream is) throws IOException {
63+
java.io.ByteArrayOutputStream baos = new java.io.ByteArrayOutputStream();
64+
byte[] buffer = new byte[8192];
65+
int len;
66+
while ((len = is.read(buffer)) != -1) {
67+
baos.write(buffer, 0, len);
68+
}
69+
return baos.toByteArray();
70+
}
71+
4972
@Test
5073
public void testExists() throws ZarrException, IOException {
5174
Assertions.assertTrue(storeHandleWithData().exists());

0 commit comments

Comments
 (0)