diff --git a/docs/TODO.md b/docs/TODO.md index d203ac3..f851c04 100644 --- a/docs/TODO.md +++ b/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, diff --git a/docs/releases/upgrade-to-1.5.md b/docs/releases/upgrade-to-1.5.md index 7f23d69..a76a318 100644 --- a/docs/releases/upgrade-to-1.5.md +++ b/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. diff --git a/spring-services-storage/pom.xml b/spring-services-storage/pom.xml index 627ad5d..f55728a 100644 --- a/spring-services-storage/pom.xml +++ b/spring-services-storage/pom.xml @@ -32,6 +32,18 @@ spring-boot-starter-test test + + + org.testcontainers + testcontainers + test + + + org.testcontainers + testcontainers-junit-jupiter + test + diff --git a/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStore.java b/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStore.java index 7307d12..77c9946 100644 --- a/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStore.java +++ b/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 diff --git a/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/s3/S3ObjectStore.java b/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/s3/S3ObjectStore.java index 6b6cb77..39c417c 100644 --- a/spring-services-storage/src/main/java/com/openelements/spring/base/services/storage/s3/S3ObjectStore.java +++ b/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. + * + *

{@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 diff --git a/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/ObjectStoreContractTest.java b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/ObjectStoreContractTest.java new file mode 100644 index 0000000..cc5dc94 --- /dev/null +++ b/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. + * + *

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. + * + *

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 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); + } + } +} diff --git a/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/file/FileObjectStoreTest.java b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/file/FileObjectStoreTest.java new file mode 100644 index 0000000..f6fcb08 --- /dev/null +++ b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/file/FileObjectStoreTest.java @@ -0,0 +1,147 @@ +package com.openelements.spring.base.services.storage.file; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + +import com.openelements.spring.base.services.storage.ObjectStore; +import com.openelements.spring.base.services.storage.ObjectStoreContractTest; +import com.openelements.spring.base.services.storage.ObjectStoreException; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.FileTime; +import java.time.Instant; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * The {@link ObjectStore} contract against the file-system store, plus the three promises this + * implementation makes on its own: a key cannot escape the root, a failed write leaves the previous + * object in place, and an interrupted write is reclaimable. + */ +@DisplayName("FileObjectStore") +class FileObjectStoreTest extends ObjectStoreContractTest { + + @TempDir + Path root; + + private FileObjectStore store; + + @BeforeEach + void setUp() { + store = new FileObjectStore(root); + } + + @Override + protected ObjectStore store() { + return store; + } + + @ParameterizedTest(name = "\"{0}\"") + @ValueSource(strings = {"../escape.txt", "a/../../escape.txt", "/absolute.txt", "trailing/", + "a//b.txt", "a/./b.txt", "a\\b.txt", ""}) + @DisplayName("A key that would leave the store is rejected") + void keysCannotEscapeTheRoot(final String key) { + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> store.get(key, 0, 1)); + } + + @Test + @DisplayName("Nothing is written outside the store's own directory") + void rejectedKeysWriteNothing() { + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> put("../escape.txt", "payload")); + + assertThat(root.getParent().resolve("escape.txt")).doesNotExist(); + } + + @Test + @DisplayName("A write that fails mid-stream leaves the previous object untouched") + void failedWriteKeepsThePreviousObject() throws IOException { + put("fragile.txt", "original"); + + assertThatExceptionOfType(ObjectStoreException.class) + .isThrownBy(() -> store.put("fragile.txt", failingStream(), "text/plain")); + + assertThat(readAll(store.get("fragile.txt"))).isEqualTo("original"); + } + + @Test + @DisplayName("A write that fails leaves no scratch file behind") + void failedWriteLeavesNoScratchFile() throws IOException { + assertThatExceptionOfType(ObjectStoreException.class) + .isThrownBy(() -> store.put("fragile.txt", failingStream(), "text/plain")); + + try (Stream uploads = Files.list(root.resolve("uploads"))) { + assertThat(uploads).isEmpty(); + } + } + + @Test + @DisplayName("An interrupted upload is invisible to list but reclaimed by the sweep") + void interruptedUploadsAreReclaimed() throws IOException { + final Path scratch = Files.writeString(root.resolve("uploads").resolve("stale.part"), "x"); + Files.setLastModifiedTime(scratch, FileTime.from(Instant.now().minusSeconds(3600))); + + assertThat(store.list("").toList()).isEmpty(); + assertThat(store.abortIncompleteUploadsOlderThan(Instant.now().minusSeconds(60))).isOne(); + assertThat(scratch).doesNotExist(); + } + + @Test + @DisplayName("A recent upload is left alone by the sweep") + void recentUploadsSurviveTheSweep() throws IOException { + final Path scratch = Files.writeString(root.resolve("uploads").resolve("fresh.part"), "x"); + + assertThat(store.abortIncompleteUploadsOlderThan(Instant.now().minusSeconds(3600))).isZero(); + assertThat(scratch).exists(); + } + + @Test + @DisplayName("Deleting the last object under a prefix removes the directory it created") + void deletePrunesEmptyDirectories() { + put("nested/deep/object.txt", "payload"); + + assertThat(store.delete("nested/deep/object.txt")).isTrue(); + assertThat(root.resolve("objects").resolve("nested")).doesNotExist(); + assertThat(root.resolve("objects")).exists(); + } + + /** A source that fails after the first byte, standing in for a connection that drops. */ + private static InputStream failingStream() { + return new InputStream() { + private boolean served; + + @Override + public int read() throws IOException { + if (served) { + throw new IOException("source failed mid-stream"); + } + served = true; + return 'x'; + } + }; + } + + @Test + @DisplayName("The content type is accepted and deliberately not stored — no sidecar file appears") + void contentTypeIsNotPersisted() throws IOException { + store.put("typed.txt", new ByteArrayInputStream("x".getBytes(StandardCharsets.UTF_8)), + "image/png"); + + try (Stream stored = Files.walk(root.resolve("objects"))) { + assertThat(stored.filter(Files::isRegularFile)) + .as("one object means one file; a remembered content type would need a second") + .singleElement() + .satisfies(file -> assertThat(file.getFileName()).hasToString("typed.txt")); + } + } +} diff --git a/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStoreTest.java b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStoreTest.java new file mode 100644 index 0000000..0204a41 --- /dev/null +++ b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/memory/InMemoryObjectStoreTest.java @@ -0,0 +1,46 @@ +package com.openelements.spring.base.services.storage.memory; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.openelements.spring.base.services.storage.ObjectStore; +import com.openelements.spring.base.services.storage.ObjectStoreContractTest; +import java.time.Instant; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** The {@link ObjectStore} contract, against the in-memory fake. */ +@DisplayName("InMemoryObjectStore") +class InMemoryObjectStoreTest extends ObjectStoreContractTest { + + private InMemoryObjectStore store; + + @BeforeEach + void setUp() { + store = new InMemoryObjectStore(); + } + + @Override + protected ObjectStore store() { + return store; + } + + @Test + @DisplayName("list reports when an object was written, not when it was listed") + void listReportsTheWriteTime() { + final Instant before = Instant.now(); + put("aged.txt", "content"); + + assertThat(store.list("").toList()) + .singleElement() + .satisfies(object -> assertThat(object.lastModified()) + .isAfterOrEqualTo(before.minusSeconds(1)) + .isBeforeOrEqualTo(Instant.now().plusSeconds(1))); + } + + @Test + @DisplayName("The fake has no multipart uploads to reclaim") + void nothingToAbort() { + assertThat(store.abortIncompleteUploadsOlderThan(Instant.now())).isZero(); + } +} diff --git a/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/s3/S3ObjectStoreTest.java b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/s3/S3ObjectStoreTest.java new file mode 100644 index 0000000..841cfe4 --- /dev/null +++ b/spring-services-storage/src/test/java/com/openelements/spring/base/services/storage/s3/S3ObjectStoreTest.java @@ -0,0 +1,151 @@ +package com.openelements.spring.base.services.storage.s3; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +import com.openelements.spring.base.services.storage.ObjectStore; +import com.openelements.spring.base.services.storage.ObjectStoreContractTest; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.net.URI; +import java.time.Instant; +import java.util.UUID; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.junit.jupiter.Container; +import org.testcontainers.junit.jupiter.Testcontainers; +import org.testcontainers.utility.DockerImageName; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3Client; + +/** + * The {@link ObjectStore} contract against a real S3 server, plus the multipart boundary. + * + *

A stub would not do here. The behaviour that made this suite necessary — what an S3 server + * answers to a range whose first byte lies at or past the object's end — is exactly the thing a stub + * would have to assume, and the assumption is what was wrong. The server answers {@code 416}, and + * {@link S3ObjectStore} has to turn that into the empty result the interface promises. + * + *

The client is built through {@link S3Clients#withoutChunkedEncoding}, the same helper the + * auto-configuration uses, so this never passes against a client configured more leniently than the + * one an application gets. + */ +@Testcontainers +@DisplayName("S3ObjectStore") +class S3ObjectStoreTest extends ObjectStoreContractTest { + + private static final int S3MOCK_PORT = 9090; + + @Container + @SuppressWarnings("resource") + private static final GenericContainer S3 = + new GenericContainer<>(DockerImageName.parse("adobe/s3mock:5.2.3")) + .withExposedPorts(S3MOCK_PORT) + .waitingFor(Wait.forHttp("/").forStatusCode(200)); + + private S3Client client; + + private String bucket; + + private S3ObjectStore store; + + @BeforeEach + void setUp() { + client = S3Clients.withoutChunkedEncoding(S3Client.builder() + .endpointOverride(URI.create( + "http://" + S3.getHost() + ":" + S3.getMappedPort(S3MOCK_PORT))) + .region(Region.US_EAST_1) + .credentialsProvider(StaticCredentialsProvider.create( + AwsBasicCredentials.create("test", "test")))) + .forcePathStyle(true) + .build(); + // A bucket per test, so list(prefix) and delete see only what their own test wrote. + bucket = "contract-" + UUID.randomUUID(); + client.createBucket(b -> b.bucket(bucket)); + store = new S3ObjectStore(client, bucket); + } + + @AfterEach + void tearDown() { + client.close(); + } + + @Override + protected ObjectStore store() { + return store; + } + + @Test + @DisplayName("A payload larger than one part uploads as multipart and reads back whole") + void multipartUpload() throws IOException { + final byte[] payload = payloadOf(S3ObjectStore.PART_SIZE_BYTES + 1024); + + store.put("large.bin", new ByteArrayInputStream(payload), "application/octet-stream"); + + assertThat(store.size("large.bin")).hasValue((long) payload.length); + try (var stream = store.get("large.bin")) { + assertThat(stream.readAllBytes()).isEqualTo(payload); + } + } + + @Test + @DisplayName("A payload of exactly one part still takes the multipart path and reads back whole") + void payloadOfExactlyOnePart() throws IOException { + final byte[] payload = payloadOf(S3ObjectStore.PART_SIZE_BYTES); + + store.put("exact.bin", new ByteArrayInputStream(payload), "application/octet-stream"); + + assertThat(store.size("exact.bin")).hasValue((long) payload.length); + try (var stream = store.get("exact.bin")) { + assertThat(stream.readAllBytes()).isEqualTo(payload); + } + } + + @Test + @DisplayName("A ranged read reaches into a multipart object without reading it whole") + void rangedReadOfAMultipartObject() throws IOException { + final byte[] payload = payloadOf(S3ObjectStore.PART_SIZE_BYTES + 1024); + + store.put("large.bin", new ByteArrayInputStream(payload), "application/octet-stream"); + + try (var stream = store.get("large.bin", S3ObjectStore.PART_SIZE_BYTES, 4)) { + assertThat(stream.readAllBytes()).isEqualTo(new byte[]{ + payload[S3ObjectStore.PART_SIZE_BYTES], + payload[S3ObjectStore.PART_SIZE_BYTES + 1], + payload[S3ObjectStore.PART_SIZE_BYTES + 2], + payload[S3ObjectStore.PART_SIZE_BYTES + 3]}); + } + } + + @Test + @DisplayName("There is nothing to reclaim while no upload has been interrupted") + void nothingToAbort() { + put("settled.txt", "content"); + + assertThat(store.abortIncompleteUploadsOlderThan(Instant.now())).isZero(); + } + + @Test + @DisplayName("The content type survives the round trip, unlike in the file store") + void contentTypeIsStored() { + store.put("typed.png", new ByteArrayInputStream("x".getBytes(UTF_8)), "image/png"); + + assertThat(client.headObject(b -> b.bucket(bucket).key("typed.png")).contentType()) + .isEqualTo("image/png"); + } + + /** Bytes with a recognisable pattern, so a misplaced part shows up as a mismatch. */ + private static byte[] payloadOf(final int size) { + final byte[] payload = new byte[size]; + for (int i = 0; i < size; i++) { + payload[i] = (byte) (i % 251); + } + return payload; + } +}