From 525805a79d5da651949120def70413ee66e0371c Mon Sep 17 00:00:00 2001 From: Mohsen Zainalpour Date: Fri, 16 Oct 2026 23:33:06 -0400 Subject: [PATCH] fix(flow-state): warn about a trigger-less imposter only on an engine before 0.23.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rift 0.23.0 gives every imposter an in-memory flow store (rift#1504), so the advisory that flowState() logs for an imposter with no store trigger — reads return empty, the engine uses a no-op store — is false on it. The warning now fires only on an engine known to predate that release, and the version alone settles it, so a newer engine costs no definition fetch. The trigger check stays for older engines. Closes #307 --- docs/design/sdk-api.md | 4 ++- .../rift/conformance/FlowStateIT.java | 14 ++++++++ .../io/github/achirdlabs/rift/Imposter.java | 10 +++--- .../github/achirdlabs/rift/ImposterImpl.java | 29 ++++++++++++++-- .../io/github/achirdlabs/rift/RiftImpl.java | 12 ++++++- .../rift/model/FlowStateSupport.java | 9 ++--- .../rift/ImposterImplFlowStateWarnTest.java | 33 ++++++++++++++++++- 7 files changed, 96 insertions(+), 15 deletions(-) diff --git a/docs/design/sdk-api.md b/docs/design/sdk-api.md index f6d48b3..eb24f29 100644 --- a/docs/design/sdk-api.md +++ b/docs/design/sdk-api.md @@ -452,7 +452,9 @@ caching, every call hits the engine (test code wants truth, not staleness). and per-space APIs with a real store only when the def declares one — an explicit `_rift.flowState`, a scenario stub (`scenarioName`/`requiredScenarioState`/`newScenarioState`), a `_rift.script` stub, or an `is` response with `_rift.stateOps` (§7.3; the engine checks this when the imposter is -created); otherwise it uses a silent no-op store (reads return empty). Separately, a per-space stub only +created); otherwise, before rift 0.23.0, it uses a silent no-op store (reads return empty). From +rift 0.23.0 every imposter gets an in-memory store (300 s TTL) by default (rift#1504), and the SDK +warns about a trigger-less def only on an older engine. Separately, a per-space stub only matches when a header-form `flowIdSource` is configured (`flowIdFromHeader(...)`), because the engine's flow-id source defaults to the imposter port. The SDK does **not** auto-inject a flow store (a transport is a faithful wire mapping — it never rewrites user input); instead it fails fast and diff --git a/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/FlowStateIT.java b/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/FlowStateIT.java index be51769..5fdd63d 100644 --- a/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/FlowStateIT.java +++ b/rift-java-conformance/src/test/java/io/github/achirdlabs/rift/conformance/FlowStateIT.java @@ -3,6 +3,7 @@ import io.github.achirdlabs.rift.FlowState; import io.github.achirdlabs.rift.Imposter; import io.github.achirdlabs.rift.Rift; +import io.github.achirdlabs.rift.json.JsonString; import io.github.achirdlabs.rift.json.JsonValue; import org.junit.jupiter.api.DynamicTest; import org.junit.jupiter.api.TestFactory; @@ -21,6 +22,19 @@ /** {@code FlowState.get} returns the stored value on every transport (#236). Needs {@code RIFT_IT=1}. */ class FlowStateIT { + @TestFactory + Stream anImposterWithNoStoreTriggerStillKeepsFlowState() { + // rift 0.23.0 gives every imposter an in-memory store (rift#1504); before it, this read empty (#307) + return gated("a trigger-less imposter's flow state reads back", () -> { + try (Rift rift = engine()) { + Imposter imposter = rift.create(imposter("no-trigger").stub(onGet("/").willReturn(ok()))); + imposter.flowState("f").put("k", "v"); + assertEquals(Optional.of(new JsonString("v")), + imposter.flowState("f").get("k")); + } + }); + } + @TestFactory Stream getReturnsTheStoredValue() { return gated("a value put into flow state reads back as itself", () -> { 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 b4f30ad..dcc5a0c 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 @@ -176,11 +176,11 @@ public interface Imposter { Space space(String flowId); /** - * The runtime flow-state store for a flow id. The engine backs this with a real store only when - * the def declares one — an explicit {@code _rift.flowState}, a scenario stub, a {@code - * _rift.script} stub, or an {@code is} response with {@code _rift.stateOps}; otherwise reads - * return empty (a no-op store). This accessor logs one advisory warning if no such trigger is - * present. + * The runtime flow-state store for a flow id. From rift 0.23.0 every imposter has an in-memory + * store (300 s TTL) unless it declares another (rift#1504). An older engine backs it with a real + * store only when the def declares one — an explicit {@code _rift.flowState}, a scenario stub, a + * {@code _rift.script} stub, or an {@code is} response with {@code _rift.stateOps}; otherwise + * reads return empty (a no-op store), and this accessor logs one advisory warning. * * @throws IllegalArgumentException if {@code flowId} is blank — a blank id is never the default * flow but a distinct, silently-wrong partition, so it is rejected rather than sent verbatim diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/ImposterImpl.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/ImposterImpl.java index fc31944..6765e1f 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/ImposterImpl.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/ImposterImpl.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.function.Supplier; import java.util.OptionalLong; final class ImposterImpl implements Imposter { @@ -48,12 +49,24 @@ final class ImposterImpl implements Imposter { /** The imposter's engine protocol, {@code http} or {@code https}: fixed for its lifetime on this port. */ private final String protocol; + /** The first rift release that gives every imposter an in-memory flow store (rift#1504). */ + static final String DEFAULT_FLOW_STORE_SINCE = "0.23.0"; + + /** The engine's version for advisories: never throws, empty when it cannot be read. */ + private final Supplier> engineVersion; + ImposterImpl(int port, RiftTransport transport, ConnectOptions options) { this(port, transport, options, Optional.empty(), ImposterDefinition.DEFAULT_PROTOCOL); } ImposterImpl(int port, RiftTransport transport, ConnectOptions options, Optional boundHost, String protocol) { + this(port, transport, options, boundHost, protocol, Optional::empty); + } + + ImposterImpl(int port, RiftTransport transport, ConnectOptions options, Optional boundHost, + String protocol, Supplier> engineVersion) { + this.engineVersion = Objects.requireNonNull(engineVersion, "engineVersion"); this.port = port; this.transport = transport; this.options = options; @@ -301,19 +314,29 @@ private synchronized void warnIfSpacesUnusable() { /** * Warn (once) if the flow-state API is used on an imposter with no store trigger (an explicit * {@code _rift.flowState}, a scenario stub, a {@code _rift.script} stub, or {@code - * _rift.stateOps}): the engine uses a no-op store, so reads return empty (#40). Advisory, same - * fetch-once/swallow policy as above. + * _rift.stateOps}) on an engine before {@value #DEFAULT_FLOW_STORE_SINCE}, which uses a no-op + * store, so reads return empty (#40, #307). Advisory, same fetch-once/swallow policy as above. */ private synchronized void warnIfFlowStateUnusable() { if (flowStateConfigChecked) { return; } flowStateConfigChecked = true; + // Only an engine known to predate rift#1504 gives a trigger-less imposter a no-op store. A newer + // engine, an unknown version, or a dev build below the floor ("0.1.0") is not warned about. + boolean noOpStoreEngine = engineVersion.get() + .filter(v -> EngineVersion.atLeast(v, RiftImpl.MIN_ENGINE_VERSION)) + .filter(v -> !EngineVersion.atLeast(v, DEFAULT_FLOW_STORE_SINCE)) + .isPresent(); + if (!noOpStoreEngine) { + return; + } try { if (!FlowStateSupport.hasStoreTrigger(definition())) { LOG.log(Level.WARNING, "imposter on port " + port + " uses the flow-state API but declares no " + "store trigger (_rift.flowState, a scenario stub, a _rift.script stub, or _rift.stateOps); " - + "reads return empty (engine uses a no-op store). Declare flowState(inMemoryFlowState())."); + + "reads return empty (rift < " + DEFAULT_FLOW_STORE_SINCE + " uses a no-op store). Declare " + + "flowState(inMemoryFlowState()), or upgrade the engine."); } } catch (RiftException e) { // Advisory only: the SPI's declared def-fetch failures (engine unavailable, not found, diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/RiftImpl.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/RiftImpl.java index 9d06dea..9dcb18b 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/RiftImpl.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/RiftImpl.java @@ -333,6 +333,16 @@ private Optional engineVersion() { return Optional.of(cached); } + /** {@link #engineVersion()} for an advisory: an unreadable version is empty, never a failure. */ + private Optional engineVersionForAdvisory() { + try { + return engineVersion(); + } catch (RiftException e) { + LOG.log(Level.DEBUG, "could not read the rift engine version for an advisory", e); + return Optional.empty(); + } + } + @Override public Imposter create(ImposterSpec spec) { return create(spec.build()); @@ -401,7 +411,7 @@ private ImposterImpl imposterAt(int port, JsonValue definition) { ? Optional.of(h.value()) : Optional.empty(); String protocol = obj.get("protocol") instanceof JsonString p ? p.value() : ImposterDefinition.DEFAULT_PROTOCOL; - return new ImposterImpl(port, transport, options, host, protocol); + return new ImposterImpl(port, transport, options, host, protocol, this::engineVersionForAdvisory); } @Override diff --git a/rift-java-core/src/main/java/io/github/achirdlabs/rift/model/FlowStateSupport.java b/rift-java-core/src/main/java/io/github/achirdlabs/rift/model/FlowStateSupport.java index ebf9251..422c899 100644 --- a/rift-java-core/src/main/java/io/github/achirdlabs/rift/model/FlowStateSupport.java +++ b/rift-java-core/src/main/java/io/github/achirdlabs/rift/model/FlowStateSupport.java @@ -3,9 +3,10 @@ /** * Predicates over an {@link ImposterDefinition} for flow-state / spaces configuration (#40). * - *

The rift engine backs the flow-state and per-space APIs with a real store only when the def - * declares one — an explicit {@code _rift.flowState}, a scenario-naming stub (rift#514), or a {@code - * _rift.script} stub (rift#358); otherwise reads silently return {@code found:false}. Separately, a + *

Before rift 0.23.0 the engine backs the flow-state and per-space APIs with a real store only + * when the def declares one — an explicit {@code _rift.flowState}, a scenario-naming stub (rift#514), + * or a {@code _rift.script} stub (rift#358); otherwise reads silently return {@code found:false}. + * From 0.23.0 every imposter has an in-memory store by default (rift#1504). Separately, a * space stub can only match when a header-form {@code flowIdSource} is configured, because the * engine's flow-id source defaults to the imposter port. These predicates let the DSL fail fast, and * the runtime accessors warn, on those otherwise-silent misconfigurations. @@ -29,7 +30,7 @@ public static boolean hasSpaceStub(ImposterDefinition def) { } /** - * The engine will provision a real (non-NoOp) flow store: the def has an explicit {@code + * An engine before rift 0.23.0 will provision a real (non-NoOp) flow store: the def has an explicit {@code * _rift.flowState}, a scenario-naming stub, a {@code _rift.script} stub, or an {@code is} * response with {@code _rift.stateOps}. */ diff --git a/rift-java-core/src/test/java/io/github/achirdlabs/rift/ImposterImplFlowStateWarnTest.java b/rift-java-core/src/test/java/io/github/achirdlabs/rift/ImposterImplFlowStateWarnTest.java index e8be19a..3445792 100644 --- a/rift-java-core/src/test/java/io/github/achirdlabs/rift/ImposterImplFlowStateWarnTest.java +++ b/rift-java-core/src/test/java/io/github/achirdlabs/rift/ImposterImplFlowStateWarnTest.java @@ -27,8 +27,14 @@ class ImposterImplFlowStateWarnTest { private static final int PORT = 4545; + /** An engine before rift#1504, whose trigger-less imposters get a no-op flow store. */ private static ImposterImpl imposter(FakeTransport t) { - return new ImposterImpl(PORT, t, ConnectOptions.builder(URI.create("http://127.0.0.1:2525")).build()); + return imposter(t, Optional.of("0.22.0")); + } + + private static ImposterImpl imposter(FakeTransport t, Optional engineVersion) { + return new ImposterImpl(PORT, t, ConnectOptions.builder(URI.create("http://127.0.0.1:2525")).build(), + Optional.empty(), "http", () -> engineVersion); } private static final String TRIGGERLESS = @@ -100,6 +106,31 @@ void warnsOnMisconfiguredDefAndIsSilentOnConfigured() { } } + @Test + void aFlowStoreForEveryImposterNeedsNoTriggerWarningOrDefFetch() { + // rift 0.23.0 (rift#1504) gives every imposter an in-memory flow store; an unknown version, or a + // below-floor dev build ("0.1.0"), is not known to lack one either + for (Optional version : List.of(Optional.of("0.23.0"), Optional.of("1.0.0"), Optional.empty(), + Optional.of("0.1.0"))) { + Logger jul = Logger.getLogger(ImposterImpl.class.getName()); + List records = new CopyOnWriteArrayList<>(); + Handler handler = new Handler() { + @Override public void publish(LogRecord r) { records.add(r); } + @Override public void flush() {} + @Override public void close() {} + }; + jul.addHandler(handler); + try { + FakeTransport t = new FakeTransport(TRIGGERLESS); + assertNotNull(imposter(t, version).flowState("alice")); + assertTrue(records.stream().noneMatch(r -> r.getLevel() == Level.WARNING), version + ": " + records); + assertEquals(0, t.getImposterCalls, version + ": the version alone settles it"); + } finally { + jul.removeHandler(handler); + } + } + } + @Test void defFetchFailureNeverBreaksTheAccessor() { // The advisory check must not propagate a transport failure — the accessor still works.