From 4161b466bcf2faebd2e6385bc250d1556863a58f Mon Sep 17 00:00:00 2001 From: Mohsen Zainalpour Date: Fri, 16 Oct 2026 16:55:35 -0400 Subject: [PATCH] fix: make Imposter.delete() idempotent on both transports rift 0.22.0 answers DELETE /imposters/:port for an absent port with 200 {} (rift#1371, Mountebank's teardown), while rift_delete_imposter still returns -1, so the same call succeeded remotely and threw embedded. Deleting an absent imposter is now a no-op everywhere: remote also accepts the 404 older engines send, and embedded probes rift_get_imposter after a -1, so a delete that leaves the imposter registered still throws. Closes #308 --- docs/design/sdk-api.md | 4 +-- .../rift/conformance/ImposterDeleteIT.java | 36 +++++++++++++++++++ .../io/github/achirdlabs/rift/Imposter.java | 9 +++++ .../rift/transport/RemoteTransport.java | 11 ++++-- .../rift/transport/RiftTransport.java | 4 +++ .../transport/RemoteTransportGateTest.java | 35 +++++++++++++++--- .../achirdlabs/rift/embedded/FfiCalls.java | 23 ++++++++++-- .../rift/embedded/EmbeddedTransportIT.java | 11 ++++++ 8 files changed, 122 insertions(+), 11 deletions(-) create mode 100644 rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/ImposterDeleteIT.java diff --git a/docs/design/sdk-api.md b/docs/design/sdk-api.md index 0bf9a6b..fcd98e2 100644 --- a/docs/design/sdk-api.md +++ b/docs/design/sdk-api.md @@ -255,7 +255,7 @@ public interface RiftTransport extends AutoCloseable { JsonValue createImposter(JsonValue definition); // returns created definition (with port) JsonValue getImposter(int port); // ImposterNotFound on 404 JsonValue getImposter(int port, boolean replayable, boolean removeProxies); - void deleteImposter(int port); + void deleteImposter(int port); // idempotent: an absent port is a no-op (rift#1371) void deleteAll(); JsonValue listImposters(boolean replayable, boolean removeProxies); void replaceAllImposters(JsonValue impostersDoc); @@ -384,7 +384,7 @@ public interface Imposter { // -- lifecycle -- void enable(); void disable(); boolean isEnabled(); // definition().enabled(): the engine's current view, one GET - void delete(); // DELETE /imposters/:port; handle unusable after + void delete(); // DELETE /imposters/:port; handle unusable after; idempotent (an absent imposter is a no-op, rift#1371) } public interface Recording extends AutoCloseable { diff --git a/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/ImposterDeleteIT.java b/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/ImposterDeleteIT.java new file mode 100644 index 0000000..f9ec71d --- /dev/null +++ b/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/ImposterDeleteIT.java @@ -0,0 +1,36 @@ +package io.github.achirdlabs.rift.conformance; + +import io.github.achirdlabs.rift.Imposter; +import io.github.achirdlabs.rift.Rift; +import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.TestFactory; + +import java.util.Optional; +import java.util.stream.Stream; + +import static io.github.achirdlabs.rift.conformance.LiveEngine.engine; +import static io.github.achirdlabs.rift.conformance.LiveEngine.gated; +import static io.github.achirdlabs.rift.dsl.RiftDsl.imposter; +import static io.github.achirdlabs.rift.dsl.RiftDsl.ok; +import static io.github.achirdlabs.rift.dsl.RiftDsl.onGet; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** {@code Imposter.delete()} is idempotent on every transport (#308, rift#1371). Needs {@code RIFT_IT=1}. */ +class ImposterDeleteIT { + + @TestFactory + Stream deleteIsIdempotent() { + return gated("deleting an imposter twice succeeds, and it is gone", () -> { + try (Rift rift = engine()) { + Imposter created = rift.create(imposter("gone").stub(onGet("/").willReturn(ok()))); + int port = created.port(); + assertTrue(rift.imposter(port).isPresent()); + + created.delete(); + created.delete(); + assertEquals(Optional.empty(), rift.imposter(port)); + } + }); + } +} diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/Imposter.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/Imposter.java index c4bccff..b4f30ad 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/Imposter.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/Imposter.java @@ -232,5 +232,14 @@ public interface Imposter { */ boolean isEnabled(); + /** + * Deletes this imposter, freeing its port. Idempotent, as in Mountebank (rift#1371): deleting + * an imposter that is already gone succeeds, on every transport and engine version. This handle + * is unusable afterwards. To tell whether an imposter still exists, use {@link Rift#imposter(int)}. + * + * @throws io.github.achirdlabs.rift.error.EngineError if the engine fails to delete it and it is + * still registered + * @throws io.github.achirdlabs.rift.error.EngineUnavailable if the engine cannot be reached + */ void delete(); } diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RemoteTransport.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RemoteTransport.java index a2a215b..bd67482 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RemoteTransport.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RemoteTransport.java @@ -40,8 +40,9 @@ * EngineUnavailable}; HTTP 400 becomes {@link InvalidDefinition}; HTTP 404 on a * {@code /imposters/{port}} path becomes {@link ImposterNotFound}; a 2xx response whose body * fails to parse becomes {@link CommunicationError}; any other non-2xx status becomes {@link - * EngineError}. {@link #flowStateGet} is the one exception: a 404 there means "no such key" and - * is reported as {@link Optional#empty()}, not an exception. + * EngineError}. Two 404s are not errors: {@link #flowStateGet}'s means "no such key" and is + * reported as {@link Optional#empty()}, and {@link #deleteImposter}'s means the imposter is already + * gone, which a delete does not refuse. */ public final class RemoteTransport implements RiftTransport { @@ -100,7 +101,11 @@ public JsonValue getImposter(int port, boolean replayable, boolean removeProxies @Override public void deleteImposter(int port) { - executeVoid("DELETE", "/imposters/" + port, null, OptionalInt.of(port)); + HttpResponse response = send("DELETE", "/imposters/" + port, null); + // An engine before 0.22.0 answers a missing port 404; from 0.22.0 it is 200 {} (rift#1371). + if (!isSuccess(response.statusCode()) && response.statusCode() != 404) { + throw mapError(response, OptionalInt.of(port)); + } } @Override diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RiftTransport.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RiftTransport.java index f67a873..99a7456 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RiftTransport.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/transport/RiftTransport.java @@ -37,6 +37,10 @@ public interface RiftTransport extends AutoCloseable { */ JsonValue getImposter(int port, boolean replayable, boolean removeProxies); + /** + * Deletes the imposter on {@code port}. Idempotent: a port with no imposter is not an error + * (rift#1371); only a delete that leaves the imposter in place, or an unreachable engine, throws. + */ void deleteImposter(int port); void deleteAll(); diff --git a/rift-java-core/src/test/java/io/github/achirdlabs/rift/transport/RemoteTransportGateTest.java b/rift-java-core/src/test/java/io/github/achirdlabs/rift/transport/RemoteTransportGateTest.java index 15c5a34..96cd14d 100644 --- a/rift-java-core/src/test/java/io/github/achirdlabs/rift/transport/RemoteTransportGateTest.java +++ b/rift-java-core/src/test/java/io/github/achirdlabs/rift/transport/RemoteTransportGateTest.java @@ -7,7 +7,6 @@ import io.github.achirdlabs.rift.error.CommunicationError; import io.github.achirdlabs.rift.error.EngineError; import io.github.achirdlabs.rift.error.EngineUnavailable; -import io.github.achirdlabs.rift.error.ImposterNotFound; import io.github.achirdlabs.rift.error.InvalidDefinition; import org.junit.jupiter.api.Test; @@ -77,14 +76,42 @@ void missingImposterOnGetReturnsEmpty() { } @Test - void deleteMissingImposterThrowsImposterNotFound() { + void deletingAnImposterAnOlderEngine404sIsANoOp() { + // engines before 0.22.0 answered a missing port with 404; delete is idempotent on every engine (#308) try (FakeAdminServer server = new FakeAdminServer()) { server.respond("POST /imposters", 201, IMPOSTER_JSON); server.respond("DELETE /imposters/4545", 404, "{\"errors\":[{\"code\":\"no such resource\",\"message\":\"missing\"}]}"); try (Rift rift = connectNoPreflight(server)) { Imposter imp = rift.create(imposter("x").port(4545)); - ImposterNotFound ex = assertThrows(ImposterNotFound.class, imp::delete); - assertEquals(4545, ex.port()); + imp.delete(); + assertEquals(1, server.received().stream().filter(r -> r.method().equals("DELETE") && r.path().equals("/imposters/4545")).count()); + } + } + } + + @Test + void deletingAnAbsentImposterOnA022EngineIsANoOp() { + // rift 0.22.0 answers it 200 {} (rift#1371), Mountebank's idempotent teardown + try (FakeAdminServer server = new FakeAdminServer()) { + server.respond("POST /imposters", 201, IMPOSTER_JSON); + server.respond("DELETE /imposters/4545", 200, "{}"); + try (Rift rift = connectNoPreflight(server)) { + rift.create(imposter("x").port(4545)).delete(); + assertEquals(1, server.received().stream().filter(r -> r.method().equals("DELETE") && r.path().equals("/imposters/4545")).count()); + } + } + } + + @Test + void aFailedDeleteStillThrows() { + try (FakeAdminServer server = new FakeAdminServer()) { + server.respond("POST /imposters", 201, IMPOSTER_JSON); + server.respond("DELETE /imposters/4545", 500, "{\"errors\":[{\"code\":\"internal\",\"message\":\"datadir unlink failed\"}]}"); + try (Rift rift = connectNoPreflight(server)) { + Imposter imp = rift.create(imposter("x").port(4545)); + EngineError ex = assertThrows(EngineError.class, imp::delete); + assertEquals(500, ex.code()); + assertEquals("datadir unlink failed", ex.getMessage()); } } } diff --git a/rift-java-embedded/src/main/java/io/github/achirdlabs/rift/embedded/FfiCalls.java b/rift-java-embedded/src/main/java/io/github/achirdlabs/rift/embedded/FfiCalls.java index 6bcc663..45d6993 100644 --- a/rift-java-embedded/src/main/java/io/github/achirdlabs/rift/embedded/FfiCalls.java +++ b/rift-java-embedded/src/main/java/io/github/achirdlabs/rift/embedded/FfiCalls.java @@ -66,8 +66,27 @@ void replaceStubs(int port, JsonValue stubs) { void deleteImposter(int port) { ensureLive(); - if (ffi.deleteImposter(handle, port) != 0) { - throw engineError(); + if (ffi.deleteImposter(handle, port) == 0) { + return; + } + // rift_delete_imposter answers -1 for a missing port too, but delete is idempotent over HTTP + // (rift#1371). Read the error first: the probe clears it. A failed delete leaves the imposter + // registered, so its presence tells a real failure from an absent one. + RuntimeException failure = engineError(); + if (imposterExists(port)) { + throw failure; + } + } + + private boolean imposterExists(int port) { + try (Arena args = Arena.ofConfined()) { + MemorySegment probe = ffi.getImposter(handle, port, + FfmCompat.allocateCString(args, listOptionsJson(false, false))); + if (RiftFfi.isNull(probe)) { + return false; + } + ffi.free(probe); + return true; } } diff --git a/rift-java-embedded/src/test/java/io/github/achirdlabs/rift/embedded/EmbeddedTransportIT.java b/rift-java-embedded/src/test/java/io/github/achirdlabs/rift/embedded/EmbeddedTransportIT.java index 3dd28bc..de87bc9 100644 --- a/rift-java-embedded/src/test/java/io/github/achirdlabs/rift/embedded/EmbeddedTransportIT.java +++ b/rift-java-embedded/src/test/java/io/github/achirdlabs/rift/embedded/EmbeddedTransportIT.java @@ -138,6 +138,17 @@ void flowStateClearEmptiesOneFlowAndIsIdempotent() { } } + @Test + void deletingAnImposterTwiceIsIdempotent() { + // rift_delete_imposter answers -1 for a missing port; the SDK's delete is idempotent like HTTP's (rift#1371) + try (EmbeddedTransport t = open()) { + int port = portOf(t.createImposter(flowStateImposter())); + t.deleteImposter(port); + t.deleteImposter(port); + t.deleteImposter(45999); + } + } + @Test void flowStateRoundTrips() { try (EmbeddedTransport t = open()) {