Separated snapshot reviews from SNapshots storage

Author: oskardudycz

src/jvm/src/main/java/io/eventdriven/strictland/ContractContext.java

@@ -4,10 +4,16 @@
  * The shared context between {@code given(...)} through to a {@code then} step: <br/>
  * - the serializer that turns a message into bytes, <br/>
  * - the storage that reads and writes snapshots, <br/>
- * - the type mapper that names a message type.
+ * - the type mapper that names a message type, <br/>
+ * - the reviewer that decides what a stored snapshot's outcome means.
  *
  * @param serializer turns a message into the bytes the contract compares, and back
  * @param storage reads and writes approved snapshots
  * @param typeMapper names a message type for locating its snapshot
+ * @param reviewer reacts to a store outcome: pass, re-baseline, or open a diff and fail
  */
-record ContractContext(MessageSerializer serializer, SnapshotStorage storage, MessageTypeMapper typeMapper) {}
+record ContractContext(
+        MessageSerializer serializer,
+        SnapshotStorage storage,
+        MessageTypeMapper typeMapper,
+        SnapshotReviewer reviewer) {}

src/jvm/src/main/java/io/eventdriven/strictland/FileSnapshotStorage.java

@@ -15,43 +15,28 @@
  * lays out (defaulting to {@code test/resources/contract-registry}).
  * It stores approved snapshots in relative {@link SnapshotLocation} path under the root.
  *
- * <p>The first run writes the approved file for you to review and commit (with {@code .snap.approved} marker). A later run compares the
- * payload against it: a match passes, a difference is reviewed according to the {@link SnapshotReview}
- * this store was built with. In {@link ReviewMode#APPROVE} a difference re-baselines the approved file;
- * otherwise it writes a sibling {@code .snap.received} file, opens a diff tool when the run is local and
- * interactive, and fails the check with a message carrying an inline diff of what moved.</p>
+ * <p>The first run writes the approved file for you to review and commit (with {@code .snap.approved}
+ * marker) and reports {@link SnapshotResult.Approved}. A later run compares the payload against it: a
+ * match reports {@link SnapshotResult.Unchanged}, a difference writes a sibling {@code .snap.received}
+ * file and reports {@link SnapshotResult.Drifted}. Storage only reads and writes; what a drift means is
+ * decided by the caller.</p>
  */
 class FileSnapshotStorage implements SnapshotStorage {
 
     private static final String APPROVED_MARKER = ".approved.";
     private static final String RECEIVED_MARKER = ".received.";
-    static final String SNAP_APPROVED_MARKER = ".snap.approved.";
 
     private final Path root;
-    private final SnapshotReview review;
-    private final DiffReview diffReview;
 
     FileSnapshotStorage(SnapshotLayout layout) {
-        this(layout, SnapshotReview.auto());
-    }
-
-    FileSnapshotStorage(SnapshotLayout layout, SnapshotReview review) {
-        this(layout, review, DiffReview.forReview(review));
-    }
-
-    FileSnapshotStorage(SnapshotLayout layout, SnapshotReview review, DiffReview diffReview) {
         this.root = Path.of(layout.rootPath(), layout.wrapperFolder());
-        this.review = review;
-        this.diffReview = diffReview;
     }
 
     @Override
-    public void store(SnapshotLocation location, SnapshotData data) {
+    public SnapshotResult store(SnapshotLocation location, SnapshotData data) {
         var approved = approvedPath(location);
-        var fileName = approved.getFileName().toString();
-        var received = approved.resolveSibling(fileName.replace(APPROVED_MARKER, RECEIVED_MARKER));
+        var received = receivedSibling(approved);
         var payload = data.bytes();
-        byte[] approvedBytes;
         try {
             var parent = approved.getParent();
             if (!Files.exists(approved)) {
@@ -60,36 +45,36 @@ public void store(SnapshotLocation location, SnapshotData data) {
                 }
                 Files.write(approved, payload);
                 Files.deleteIfExists(received);
-                return;
+                return new SnapshotResult.Approved(location);
             }
-            approvedBytes = Files.readAllBytes(approved);
+            var approvedBytes = Files.readAllBytes(approved);
             if (Arrays.equals(approvedBytes, payload)) {
                 Files.deleteIfExists(received);
-                return;
-            }
-            if (review.mode() == ReviewMode.APPROVE) {
-                SnapshotApproval.writeApproved(approved, received, payload);
-                return;
+                return new SnapshotResult.Unchanged(location);
             }
             var receivedParent = received.getParent();
             if (receivedParent != null) {
                 Files.createDirectories(receivedParent);
             }
             Files.write(received, payload);
+            return new SnapshotResult.Drifted(location, new SnapshotData(approvedBytes), data, approved, received);
         } catch (IOException e) {
             throw new UncheckedIOException("Failed to store snapshot at " + approved, e);
         }
-        diffReview.open(received, approved);
-        throw new AssertionError(driftMessage(approved, received, approvedBytes, payload));
     }
 
-    private static String driftMessage(Path approved, Path received, byte[] approvedBytes, byte[] payload) {
-        return "Snapshot drift: " + approved + " differs from the approved snapshot.\n\n"
-                + SnapshotDiff.render(approvedBytes, payload) + "\n"
-                + "received: " + received + "\n"
-                + "approved: " + approved + "\n\n"
-                + "To accept this change, re-run with -Dstrictland.review.mode=approve, "
-                + "or save the received payload over the approved file in the diff tool.";
+    @Override
+    public void approve(SnapshotLocation location, SnapshotData data) {
+        var approved = approvedPath(location);
+        try {
+            SnapshotApproval.writeApproved(approved, receivedSibling(approved), data.bytes());
+        } catch (IOException e) {
+            throw new UncheckedIOException("Failed to approve snapshot at " + approved, e);
+        }
+    }
+
+    private static Path receivedSibling(Path approved) {
+        return approved.resolveSibling(approved.getFileName().toString().replace(APPROVED_MARKER, RECEIVED_MARKER));
     }
 
     @Override

src/jvm/src/main/java/io/eventdriven/strictland/MessageContract.java

@@ -36,12 +36,11 @@ private MessageContract(SpecificationOptions options) {
         var configuredStorage = options.storage();
         var storage = configuredStorage != null
                 ? configuredStorage
-                : new FileSnapshotStorage(
-                        SnapshotLayoutResolver.resolve(options.snapshotLayout()),
-                        SnapshotReviewResolver.resolve(options.review()));
+                : new FileSnapshotStorage(SnapshotLayoutResolver.resolve(options.snapshotLayout()));
         var configuredTypeMapper = options.typeMapper();
         var typeMapper = configuredTypeMapper != null ? configuredTypeMapper : MessageTypeMapper.fullyQualifiedName();
-        this.context = new ContractContext(serializer, storage, typeMapper);
+        var reviewer = SnapshotReviewer.forReview(SnapshotReviewResolver.resolve(options.review()));
+        this.context = new ContractContext(serializer, storage, typeMapper, reviewer);
     }
 
     /**

src/jvm/src/main/java/io/eventdriven/strictland/SnapshotResult.java

@@ -0,0 +1,53 @@
+package io.eventdriven.strictland;
+
+import java.nio.file.Path;
+
+/**
+ * What {@link SnapshotStorage#store storing} a snapshot did: a first-run approval, an unchanged match,
+ * or a drift from the approved snapshot. Storage only reports the outcome; deciding what a drift means
+ * - open a diff tool, fail the check, or re-baseline - is the caller's job, so a custom store never has
+ * to know about diffs, assertions, or review tools.
+ */
+public sealed interface SnapshotResult {
+
+    /**
+     * The location this outcome is for.
+     *
+     * @return the snapshot location that was stored
+     */
+    SnapshotLocation location();
+
+    /**
+     * The first run for a location: no approved snapshot existed, so the payload was written as the
+     * approved snapshot for you to review and commit.
+     *
+     * @param location the location the approved snapshot was written at
+     */
+    record Approved(SnapshotLocation location) implements SnapshotResult {}
+
+    /**
+     * A later run whose payload matched the approved snapshot: the contract still holds.
+     *
+     * @param location the location that matched
+     */
+    record Unchanged(SnapshotLocation location) implements SnapshotResult {}
+
+    /**
+     * A later run whose payload differs from the approved snapshot. The received payload has been
+     * written beside the approved one so the two can be compared and, if the change is intended,
+     * accepted with {@link SnapshotStorage#approve}.
+     *
+     * @param location the location that drifted
+     * @param approved the approved payload the check compared against
+     * @param received the drifted payload this run produced
+     * @param approvedFile the approved snapshot file
+     * @param receivedFile the received snapshot file written for review
+     */
+    record Drifted(
+            SnapshotLocation location,
+            SnapshotData approved,
+            SnapshotData received,
+            Path approvedFile,
+            Path receivedFile)
+            implements SnapshotResult {}
+}

src/jvm/src/main/java/io/eventdriven/strictland/SnapshotReviewer.java

@@ -0,0 +1,42 @@
+package io.eventdriven.strictland;
+
+/**
+ * Decides what a {@link SnapshotResult} means. Storage only reports; this is the imperative shell that
+ * reacts: a match or a first-run approval passes, and a drift either re-baselines (in {@code approve}
+ * mode) or opens a diff tool and fails the check with an inline diff of what moved.
+ */
+final class SnapshotReviewer {
+
+    private final SnapshotReview review;
+    private final DiffReview diffReview;
+
+    SnapshotReviewer(SnapshotReview review, DiffReview diffReview) {
+        this.review = review;
+        this.diffReview = diffReview;
+    }
+
+    static SnapshotReviewer forReview(SnapshotReview review) {
+        return new SnapshotReviewer(review, DiffReview.forReview(review));
+    }
+
+    void review(SnapshotStorage storage, SnapshotResult result) {
+        if (!(result instanceof SnapshotResult.Drifted drift)) {
+            return;
+        }
+        if (review.mode() == ReviewMode.APPROVE) {
+            storage.approve(drift.location(), drift.received());
+            return;
+        }
+        diffReview.open(drift.receivedFile(), drift.approvedFile());
+        throw new AssertionError(driftMessage(drift));
+    }
+
+    private static String driftMessage(SnapshotResult.Drifted drift) {
+        return "Snapshot drift: " + drift.approvedFile() + " differs from the approved snapshot.\n\n"
+                + SnapshotDiff.render(drift.approved().bytes(), drift.received().bytes()) + "\n"
+                + "received: " + drift.receivedFile() + "\n"
+                + "approved: " + drift.approvedFile() + "\n\n"
+                + "To accept this change, re-run with -Dstrictland.review.mode=approve, "
+                + "or save the received payload over the approved file in the diff tool.";
+    }
+}

src/jvm/src/main/java/io/eventdriven/strictland/SnapshotStorage.java

@@ -29,15 +29,28 @@
 public interface SnapshotStorage {
 
     /**
-     * Approves and persists a snapshot at one exact location. The first run saves it for you to review
-     * and commit; on a later run a matching payload passes, and one that has drifted throws an {@link
-     * AssertionError} to fail the check.
+     * Stores a snapshot at one exact location and reports what happened, without deciding what a drift
+     * means. The first run writes the approved snapshot ({@link SnapshotResult.Approved}); a later run
+     * that matches passes ({@link SnapshotResult.Unchanged}); one that differs writes the received
+     * payload beside the approved one and reports the drift ({@link SnapshotResult.Drifted}) for the
+     * caller to open, fail, or accept. It never throws to fail a check - only wraps a genuine I/O
+     * failure.
      *
      * @param location the exact file the snapshot is stored at
-     * @param data the serialized message to approve and persist
-     * @throws AssertionError when the payload differs from the approved snapshot
+     * @param data the serialized message to store
+     * @return what storing did: approved, unchanged, or drifted
      */
-    void store(SnapshotLocation location, SnapshotData data) throws AssertionError;
+    SnapshotResult store(SnapshotLocation location, SnapshotData data);
+
+    /**
+     * Re-baselines a location by overwriting its approved snapshot with a payload you've accepted, and
+     * clearing any received file. This is how {@code approve} review mode accepts an intended change; it
+     * assumes the caller has already decided the drift is wanted.
+     *
+     * @param location the exact file to re-baseline
+     * @param data the accepted payload to store as the new approved snapshot
+     */
+    void approve(SnapshotLocation location, SnapshotData data);
 
     /**
      * Reads back the one snapshot approved at an exact location, for a compatibility check that replays a

src/jvm/src/main/java/io/eventdriven/strictland/ThenContractStep.java

@@ -41,7 +41,8 @@ public class ThenContractStep<S> {
      */
     public void thenContractIsUnchanged() {
         var data = new SnapshotData(context.serializer().serialize(instance));
-        context.storage().store(snapshotLocation(), data);
+        var result = context.storage().store(snapshotLocation(), data);
+        context.reviewer().review(context.storage(), result);
     }
 
     private SnapshotLocation snapshotLocation() {

src/jvm/src/test/java/io/eventdriven/strictland/Base64SnapshotStorage.java

@@ -17,6 +17,7 @@
 @NullMarked
 final class Base64SnapshotStorage implements SnapshotStorage {
     private static final String APPROVED_SUFFIX = ".approved.txt";
+    private static final String RECEIVED_SUFFIX = ".received.txt";
 
     private final Path dir;
 
@@ -25,7 +26,7 @@ final class Base64SnapshotStorage implements SnapshotStorage {
     }
 
     @Override
-    public void store(SnapshotLocation location, SnapshotData data) {
+    public SnapshotResult store(SnapshotLocation location, SnapshotData data) {
         var payload = data.bytes();
         var path = approvedPath(location.name());
         try {
@@ -34,19 +35,40 @@ public void store(SnapshotLocation location, SnapshotData data) {
                 if (parent != null) {
                     Files.createDirectories(parent);
                 }
-                Files.writeString(path, Base64.getEncoder().encodeToString(payload), UTF_8);
-                return;
+                Files.writeString(path, encode(payload), UTF_8);
+                return new SnapshotResult.Approved(location);
             }
             var approved = Base64.getDecoder().decode(Files.readString(path, UTF_8));
-            if (!Arrays.equals(approved, payload)) {
-                throw new AssertionError("Snapshot drift for '" + location.name()
-                        + "': stored payload differs from the approved snapshot");
+            if (Arrays.equals(approved, payload)) {
+                return new SnapshotResult.Unchanged(location);
             }
+            var received = receivedPath(location.name());
+            Files.writeString(received, encode(payload), UTF_8);
+            return new SnapshotResult.Drifted(location, new SnapshotData(approved), data, path, received);
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }
     }
 
+    @Override
+    public void approve(SnapshotLocation location, SnapshotData data) {
+        var path = approvedPath(location.name());
+        try {
+            var parent = path.getParent();
+            if (parent != null) {
+                Files.createDirectories(parent);
+            }
+            Files.writeString(path, encode(data.bytes()), UTF_8);
+            Files.deleteIfExists(receivedPath(location.name()));
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
+    }
+
+    private static String encode(byte[] payload) {
+        return Base64.getEncoder().encodeToString(payload);
+    }
+
     @Override
     public SnapshotData read(SnapshotLocation location) {
         var path = approvedPath(location.name());
@@ -82,4 +104,8 @@ public List<SnapshotData> readAll(SnapshotFilter filter) {
     private Path approvedPath(String name) {
         return dir.resolve(name + APPROVED_SUFFIX);
     }
+
+    private Path receivedPath(String name) {
+        return dir.resolve(name + RECEIVED_SUFFIX);
+    }
 }