fix(storage): make every ObjectStore answer a ranged read the same way

docs/TODO.md

@@ -5,24 +5,16 @@
 The module has its auto-configuration and its `openelements.storage.*` namespace. What is still open
 is everything below the wiring:
 
-- **Tests for the implementations.** Only the auto-configuration is covered. `FileObjectStore` alone
-  justifies several: the traversal guard on keys, the scratch-then-move visibility guarantee, ranged
-  reads past the end, and the incomplete-upload sweep. `S3ObjectStore`'s multipart boundary (the
-  switch from a single `PutObject` to a multipart upload at exactly `PART_SIZE_BYTES`) is the other
-  untested edge that matters.
-- **Ranged reads diverge between the implementations, against the interface's own contract.**
-  `ObjectStore#get(String, long, long)` documents that "a range extending past the end returns the
-  available bytes rather than failing". `FileObjectStore` implements that — `offset >= size` yields
-  an empty stream. `S3ObjectStore` does not: it sends `Range: bytes=offset-last` and catches only
-  `NoSuchKeyException`, so a first-byte position at or past the object's length comes back as a raw
-  `S3Exception` (HTTP 416), not even wrapped as `ObjectStoreException`. A second divergence sits next
-  to it: for `length == 0` the file store throws `ObjectNotFoundException` when the key does not
-  exist ("a zero-length read of a missing key is a wrong key, not 'no bytes'"), while the S3 store
-  returns an empty stream without checking. Two backends behind one interface must not answer the
-  same call differently. **Fix before 1.5.0 ships** — the storage module is new in it, so this is
-  still cheap to correct. Found by reading the code while writing `docs/releases/upgrade-to-1.5.md`;
-  the 416 behaviour is the standard range semantics, not something that was run against a live
-  endpoint.
+- **Two untested edges remain.** The contract suite covers the interface's promises against all three
+  implementations, and the S3 store runs against a real server (`adobe/s3mock`) including its
+  multipart path. Not covered: `abortIncompleteUploadsOlderThan` against an actually interrupted
+  multipart upload (the S3 test only asserts that nothing pending means nothing reclaimed), and
+  `S3ObjectStore`'s abort-on-failure path when the source stream dies mid-upload.
+- **`S3ObjectStore` lets raw SDK exceptions escape.** `get`, `size`, `delete` and `list` catch only
+  `NoSuchKeyException` (and now 416), so any other `S3Exception` — a refused credential, a missing
+  bucket — reaches the caller as an AWS type rather than the `ObjectStoreException` the package
+  documents as "thrown when an object-store operation fails". The file store wraps consistently.
+  Left alone while fixing the range contract, to keep that change surgical.
 - **`InMemoryObjectStore` is public API here, not a test fixture.** Its `failDeletes`, `failPuts`,
   `lastGetOffset` and `lastGetLength` are public mutable fields — fine inside one application, not as
   a published surface. `openelements.storage.type=memory` now makes it selectable in configuration,

docs/releases/upgrade-to-1.5.md

@@ -130,6 +130,9 @@ Both are unchecked.
 - A write that never becomes visible half-finished. The S3 store aborts its multipart upload on
   failure; the file store streams into a scratch file and moves it into place, so a reader sees
   either the previous object or the complete new one, never a truncated one.
+- The same answers from every backend. A shared contract suite runs the interface's promises against
+  all three implementations, so a ranged read past the end, a zero-length read of a missing key or a
+  negative offset behaves identically whether you configured `s3`, `file` or `memory`.
 - Declaring your own `ObjectStore` bean makes the library back off, whatever `type` says.
 
 ### Guard rails / Don't do this
@@ -162,11 +165,10 @@ Both are unchecked.
   surrounding database transaction fails. If you need the two to agree, write the object first and
   record it second, then reconcile orphans with `list` — which is what
   `abortIncompleteUploadsOlderThan` complements rather than replaces.
-- **Maturity: the implementations themselves are not yet covered by tests.** The auto-configuration
-  is (activation, selection, misconfiguration), but the S3, file and in-memory stores arrived from an
-  application that exercised them in its own suite and the tests did not come along. Treat the file
-  and S3 stores as proven in one deployment rather than in this library, and weigh that before
-  adopting them for a new one.
+- **Maturity: the module is new in 1.5.0.** The implementations arrived from an application that ran
+  them in production, and they are covered here by a shared contract suite — the S3 store against a
+  real S3 server, including its multipart path. Two edges are still untested: aborting an actually
+  interrupted multipart upload, and the S3 store's own abort-on-failure path during `put`.
 - **The AWS SDK comes along even if you only use `type=file`.** `software.amazon.awssdk:s3` is a
   hard dependency of the module today. Making it optional is planned; until then, budget for it in
   your dependency footprint.

spring-services-storage/pom.xml

@@ -32,6 +32,18 @@
             <artifactId>spring-boot-starter-test</artifactId>
             <scope>test</scope>
         </dependency>
+        <!-- The S3 store is tested against a real S3 server: the 416 answer to an out-of-range
+             read is precisely what a stub would have to assume, and that assumption was the bug. -->
+        <dependency>
+            <groupId>org.testcontainers</groupId>
+            <artifactId>testcontainers</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.testcontainers</groupId>
+            <artifactId>testcontainers-junit-jupiter</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
 </project>

spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStore.java

@@ -76,17 +76,23 @@ public InputStream get(String key) {
 
     @Override
     public InputStream get(String key, long offset, long length) {
+        if (offset < 0) {
+            throw new IllegalArgumentException("offset must not be negative");
+        }
+        if (length < 0) {
+            throw new IllegalArgumentException("length must not be negative");
+        }
         lastGetOffset = offset;
         lastGetLength = length;
         byte[] bytes = objects.get(key);
         if (bytes == null) {
             throw new ObjectNotFoundException(key);
         }
+        // Clamped in long arithmetic: offset + length overflows for a caller asking "everything from
+        // here" with Long.MAX_VALUE, and the wrapped value used to produce a negative slice size.
         int from = (int) Math.min(offset, bytes.length);
-        int to = (int) Math.min(offset + length, bytes.length);
-        byte[] slice = new byte[to - from];
-        System.arraycopy(bytes, from, slice, 0, to - from);
-        return new ByteArrayInputStream(slice);
+        int count = (int) Math.min(length, bytes.length - (long) from);
+        return new ByteArrayInputStream(bytes, from, count);
     }
 
     @Override

spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/s3/S3ObjectStore.java

@@ -20,6 +20,7 @@
 import software.amazon.awssdk.services.s3.model.CompletedPart;
 import software.amazon.awssdk.services.s3.model.CreateMultipartUploadResponse;
 import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
+import software.amazon.awssdk.services.s3.model.S3Exception;
 import software.amazon.awssdk.services.s3.model.UploadPartResponse;
 
 /**
@@ -35,6 +36,9 @@ public class S3ObjectStore implements ObjectStore {
     /** Part size for multipart uploads (8 MiB, above S3's 5 MiB minimum for non-final parts). */
     static final int PART_SIZE_BYTES = 8 * 1024 * 1024;
 
+    /** HTTP 416, the answer to a range whose first byte lies at or past the object's end. */
+    private static final int RANGE_NOT_SATISFIABLE = 416;
+
     private final S3Client s3;
     private final String bucket;
 
@@ -138,18 +142,44 @@ public InputStream get(final String key, final long offset, final long length) {
         if (offset < 0) {
             throw new IllegalArgumentException("offset must not be negative");
         }
-        if(length < 0) {
+        if (length < 0) {
             throw new IllegalArgumentException("length must not be negative");
         }
         if (length == 0) {
-            return new ByteArrayInputStream(new byte[0]);
+            // Empty, but only for an object that exists: a zero-length read of a missing key is a
+            // wrong key, not "no bytes". The extra HEAD is paid by this rare case alone.
+            if (size(key).isEmpty()) {
+                throw new ObjectNotFoundException(key);
+            }
+            return InputStream.nullInputStream();
         }
-        long last = offset + length - 1;
         try {
-            return s3.getObject(b -> b.bucket(bucket).key(key).range("bytes=" + offset + "-" + last));
+            return s3.getObject(b -> b.bucket(bucket).key(key).range(range(offset, length)));
         } catch (final NoSuchKeyException e) {
             throw new ObjectNotFoundException(key, e);
+        } catch (final S3Exception e) {
+            if (e.statusCode() == RANGE_NOT_SATISFIABLE) {
+                // A range starting past the end yields the available bytes — none — as the interface
+                // specifies. HTTP answers 416 to that, which is where this store used to diverge from
+                // the file one: it let the SDK's exception out instead of returning nothing.
+                return InputStream.nullInputStream();
+            }
+            throw e;
+        }
+    }
+
+    /**
+     * The {@code Range} header for a slice, left open-ended when the last byte would overflow.
+     *
+     * <p>{@code length} is a maximum, so {@link Long#MAX_VALUE} is a legitimate way to ask for
+     * "everything from here". Computing {@code offset + length - 1} for that wraps negative and would
+     * make the store ask for a range no server can answer.
+     */
+    private static String range(final long offset, final long length) {
+        if (length > Long.MAX_VALUE - offset) {
+            return "bytes=" + offset + "-";
         }
+        return "bytes=" + offset + "-" + (offset + length - 1);
     }
 
     @Override

spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/ObjectStoreContractTest.java

@@ -0,0 +1,224 @@
+package com.openelements.spring.base.services.storage;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.List;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+
+/**
+ * The behaviour every {@link ObjectStore} must show, run against each implementation.
+ *
+ * <p>It exists because the implementations disagreed. {@code ObjectStore} promises that a range past
+ * the end returns the available bytes rather than failing; the file store did that and the S3 store
+ * answered with the SDK's own exception, so the same call gave two different answers depending on
+ * configuration. A shared suite is the only thing that keeps three backends honest about one
+ * interface.
+ *
+ * <p>Only portable keys are used here. A key is an opaque string on S3 and a path in the file store,
+ * so {@code a/../b} means different things to them — a deliberate divergence, covered in
+ * {@code FileObjectStoreTest} rather than smuggled into the shared contract.
+ */
+public abstract class ObjectStoreContractTest {
+
+    /** The store under test, fresh per test method. */
+    protected abstract ObjectStore store();
+
+    @Nested
+    @DisplayName("whole-object reads and writes")
+    class WholeObject {
+
+        @Test
+        @DisplayName("An object reads back exactly as written")
+        void roundTrip() throws IOException {
+            put("round/trip.txt", "hello");
+
+            assertThat(readAll(store().get("round/trip.txt"))).isEqualTo("hello");
+        }
+
+        @Test
+        @DisplayName("Writing the same key again replaces the object")
+        void overwrite() throws IOException {
+            put("overwrite.txt", "first");
+            put("overwrite.txt", "second");
+
+            assertThat(readAll(store().get("overwrite.txt"))).isEqualTo("second");
+        }
+
+        @Test
+        @DisplayName("An empty payload is a valid object, not a missing one")
+        void emptyObject() throws IOException {
+            put("empty.txt", "");
+
+            assertThat(readAll(store().get("empty.txt"))).isEmpty();
+            assertThat(store().size("empty.txt")).hasValue(0L);
+        }
+
+        @Test
+        @DisplayName("Reading a key that has no object throws ObjectNotFoundException")
+        void missingKey() {
+            assertThatExceptionOfType(ObjectNotFoundException.class)
+                    .isThrownBy(() -> store().get("absent.txt"));
+        }
+    }
+
+    @Nested
+    @DisplayName("ranged reads")
+    class RangedReads {
+
+        @Test
+        @DisplayName("A range inside the object returns exactly that slice")
+        void sliceInsideTheObject() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 2, 4))).isEqualTo("2345");
+        }
+
+        @Test
+        @DisplayName("A range extending past the end returns the available bytes")
+        void rangeExtendingPastTheEnd() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 7, 100))).isEqualTo("789");
+        }
+
+        @Test
+        @DisplayName("A range starting exactly at the end returns nothing")
+        void rangeStartingAtTheEnd() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 10, 5))).isEmpty();
+        }
+
+        @Test
+        @DisplayName("A range starting past the end returns nothing rather than failing")
+        void rangeStartingPastTheEnd() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 50, 5))).isEmpty();
+        }
+
+        @Test
+        @DisplayName("A length of Long.MAX_VALUE reads to the end instead of overflowing")
+        void readToTheEnd() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 4, Long.MAX_VALUE))).isEqualTo("456789");
+        }
+
+        @Test
+        @DisplayName("A zero-length range of an existing object returns nothing")
+        void zeroLengthOfAnExistingObject() throws IOException {
+            put("ranged.txt", "0123456789");
+
+            assertThat(readAll(store().get("ranged.txt", 0, 0))).isEmpty();
+        }
+
+        @Test
+        @DisplayName("A zero-length range of a missing key is a wrong key, not 'no bytes'")
+        void zeroLengthOfAMissingKey() {
+            assertThatExceptionOfType(ObjectNotFoundException.class)
+                    .isThrownBy(() -> store().get("absent.txt", 0, 0));
+        }
+
+        @Test
+        @DisplayName("A ranged read of a missing key throws ObjectNotFoundException")
+        void rangedReadOfAMissingKey() {
+            assertThatExceptionOfType(ObjectNotFoundException.class)
+                    .isThrownBy(() -> store().get("absent.txt", 0, 10));
+        }
+
+        @Test
+        @DisplayName("A negative offset is rejected")
+        void negativeOffset() {
+            put("ranged.txt", "0123456789");
+
+            assertThatExceptionOfType(IllegalArgumentException.class)
+                    .isThrownBy(() -> store().get("ranged.txt", -1, 5));
+        }
+
+        @Test
+        @DisplayName("A negative length is rejected")
+        void negativeLength() {
+            put("ranged.txt", "0123456789");
+
+            assertThatExceptionOfType(IllegalArgumentException.class)
+                    .isThrownBy(() -> store().get("ranged.txt", 0, -1));
+        }
+    }
+
+    @Nested
+    @DisplayName("size, delete and list")
+    class Metadata {
+
+        @Test
+        @DisplayName("size is empty for a key that has no object")
+        void sizeOfAMissingKey() {
+            assertThat(store().size("absent.txt")).isEmpty();
+        }
+
+        @Test
+        @DisplayName("size reports the byte count of a stored object")
+        void sizeOfAStoredObject() {
+            put("sized.txt", "0123456789");
+
+            assertThat(store().size("sized.txt")).hasValue(10L);
+        }
+
+        @Test
+        @DisplayName("delete removes the object and reports that it did")
+        void deleteRemoves() {
+            put("doomed.txt", "bye");
+
+            assertThat(store().delete("doomed.txt")).isTrue();
+            assertThat(store().size("doomed.txt")).isEmpty();
+        }
+
+        @Test
+        @DisplayName("Deleting a key that has no object succeeds and reports nothing removed")
+        void deleteIsIdempotent() {
+            assertThat(store().delete("absent.txt")).isFalse();
+        }
+
+        @Test
+        @DisplayName("list returns the objects under a prefix and nothing else")
+        void listByPrefix() {
+            put("wanted/a.txt", "aa");
+            put("wanted/b.txt", "bbbb");
+            put("other/c.txt", "cccccc");
+
+            final List<StoredObject> found = store().list("wanted/").toList();
+
+            assertThat(found).extracting(StoredObject::key)
+                    .containsExactlyInAnyOrder("wanted/a.txt", "wanted/b.txt");
+            assertThat(found).extracting(StoredObject::size)
+                    .containsExactlyInAnyOrder(2L, 4L);
+        }
+
+        @Test
+        @DisplayName("list returns nothing for a prefix no object matches")
+        void listWithoutMatches() {
+            put("wanted/a.txt", "aa");
+
+            assertThat(store().list("nothing/").toList()).isEmpty();
+        }
+    }
+
+    /** Writes a UTF-8 payload under a key. */
+    protected void put(final String key, final String content) {
+        store().put(key, new ByteArrayInputStream(content.getBytes(UTF_8)), "text/plain");
+    }
+
+    /** Reads a stream to the end and closes it — the contract hands ownership to the caller. */
+    protected static String readAll(final InputStream in) throws IOException {
+        try (in) {
+            return new String(in.readAllBytes(), UTF_8);
+        }
+    }
+}