Skip to content

92 - adding stream support for lists - #94

Merged
tomsontom merged 56 commits into
mainfrom
92-add-streaming-support-for-lists
Sep 1, 2026
Merged

tomsontom merged 56 commits into
mainfrom
92-add-streaming-support-for-lists

Conversation

@tomsontom

Copy link
Copy Markdown
Member
  • extended language to support @stream in the return-types
  • added streaming-property to model

@tomsontom tomsontom linked an issue Aug 5, 2026 that may be closed by this pull request
@tomsontom

Copy link
Copy Markdown
Member Author

Automated review (Claude)

Reviewed the diff across grammar, generators, and generated Java/TypeScript clients. 22 findings, grouped by severity.

Correctness — likely bugs

  1. msgpack streaming is silently corrupted. response-builder.ts:80encodeAsJsonArray(!$contentType.equals("application/json")) is inverted: it wraps msgpack streams in JSON array framing ([, ,, ]) and leaves JSON streams unwrapped — backwards from the newline-delimited-JSON / raw-concatenated-msgpack framing this PR establishes elsewhere. Those bracket/comma bytes are valid msgpack fixints, so the client's unpacker.hasNext() loop won't throw — it silently misinterprets them as extra bogus values interleaved with real records.

  2. Compile error: void method returns a void-typed call. json-utils.ts:437 — for projects with a single content-type encoding, generated _JsonUtils.decodeStream(...) (declared void) contains return decodeJsonStream(stream, consumer); where decodeJsonStream is itself void. Illegal in Java. Not caught by this PR's tests because both test specs configure two encodings and take a different branch.

  3. Compile error: missing return/throw for streaming ops with explicit REST result codes. service.ts:861generateResponseDispatchStream's per-status-code if blocks have no trailing return/throw after the loop, unlike the non-streaming dispatch. Not exercised because no streaming op in the test spec declares explicit REST results.

  4. Generated TS client fails tsc --strict. fetch-type-utils.ts:299-347 — confirmed by running tsc --strict on the generated fixture: TS2504 (ReadableStream<Uint8Array> isn't declared AsyncIterable under DOM lib) and TS2345 (re-declared stream re-reads response.body instead of the already-narrowed outer binding, losing the null-check). Invisible in this repo's own CI since the CLI package's build step is a no-op and Vitest only transpiles.

  5. Transport errors reported to the client as success. response-utils.ts:348-360StreamSubscriber.onError(Throwable t) discards t and just closes the pipe; the decode thread's finally block then always signals a clean completion regardless of whether the stream ended normally or via a dropped connection. Callers can't detect a truncated stream.

  6. @Produces dropped for the whole resource class if any operation streams. resource.ts:81 — the guard is keyed on s.operations.some(...) (per-service), not per-operation, so one streaming operation silently disables JAX-RS content-negotiation/406 handling for every non-streaming sibling operation in the same resource. Flagged independently by 4 separate review passes.

  7. Streaming onSuccess called with null responseAdapter. service.ts:826 — contradicts the LifecycleHook.onSuccess JavaDoc, which documents the adapter as always available to read headers/status. Any hook that calls .adapt(...) there will NPE for streaming operations.

  8. Streaming onSuccess bypasses the safeExecute error guard (TS). service.ts:910 — non-streaming calls wrap the onSuccess hook in safeExecute (catches/logs exceptions); the streaming branch calls it unguarded, turning a throwing hook into an unhandled promise rejection.

  9. (Lower confidence) onNext write failure never closes the pipe. response-utils.ts:335 — if the reader side is still alive when a write fails, the decode thread can block forever on in.read(), hanging the client call.

  10. Minor: stray § character leaked into the streaming error message at service.ts:901 ("...operation §${o.name}").

Efficiency

  • Per-element encoder/packer allocation on the server. _JsonUtils.java:1080encodeJsonValue/encodeMsgPackValue allocate a fresh StringWriter+JsonGenerator or MsgpackJson+MessageBufferPacker per streamed element via Multi.map(), instead of reusing one encoder for the whole stream.
  • Per-element parser allocation on the client. _JsonUtils.java:2294decodeJsonStream creates a new StringReader+JsonReader per element instead of one reusable streaming parser.
  • Per-chunk byte[] copy in the HTTP subscriber. response-utils.ts:313 — every ByteBuffer is copied into a fresh byte[] instead of a reusable scratch buffer.

Reuse / duplication

  • New mapLiteral helper not used by its own siblings. json-utils.ts:1194 — six new mapXxx(JsonValue) methods hand-roll the same instanceof/exception logic that the newly-added generic mapLiteral helper already provides.
  • handleOkResult duplicates its dispatch chain for streaming vs. array results. service.ts:819 — flagged independently by three review passes.
  • toResultType/toAPIResultType patched identically in two files. java-client-api/service.ts:134 — already-duplicate functions both got the same new logic hand-applied instead of being factored into java-gen-utils.ts.
  • Response-builder duplicates its encode chain per variant. response-builder.ts:67 — the sibling client-side generator in the same PR already shows the correct single-template pattern this file didn't follow.
  • Grammar duplicates the typeRef/inlineEnum match for streaming. remote-service-definition.langium:103streaming is added as a third top-level alternative instead of an optional flag on the existing rule.

Minor / cleanup

  • Dead TypeInfo<?> typeInfo parameter threaded through streamSubscriber/decodeStream but always null and never forwarded (response-utils.ts:298).
  • Unused Duration import + commented-out delay code in StreamRecordHandlerImpl.java:5.
  • Doubled "Handler" in generated class name StreamShortHandlerHandlerImpl.
  • Unused mutiny dependency added to the plain-JDK java-client module's pom.xml (likely meant for java-quarkus).

Generated by an automated review pass (Claude Code); please verify before acting on any finding.

@tomsontom

Copy link
Copy Markdown
Member Author

Automated review, round 2 (Claude)

Re-reviewed after the round-1 fixes landed (msgpack framing, compile errors, silent-error-swallowing, @Produces regression, encoder/buffer reuse, handleOkResult/toResultType consolidation, etc.). This pass found new issues, mostly at the edges of what round 1 touched — some directly in code fixed during round 1.

Correctness — new issues

  1. Double terminal-callback firing on stream connection abort. response-utils.ts wires the response body to streamSubscriber(...), and service.ts:901 separately attaches $responseFuture.whenComplete((...) => { if ($e != null) {...} }) on the same future. Per JDK semantics, sendAsync's future only completes once the body subscriber's stage completes — so when onError fires (e.g. connection dropped mid-stream), both paths fire: the decode thread's finisher.accept(error)$onComplete, and whenComplete's $e != null branch. Each independently calls $consumer.accept(...), onCatch, and onFinally — a single connection abort delivers the terminal signal to the consumer twice.

  2. onNext's IOException catch doesn't set the error field. response-utils.ts — it closes out and cancels the subscription, but unlike onError, never assigns e to error. If the write fails for a reason other than onError firing, the decode thread's finally sees error == null, and if decodeStream reaches a clean EOF without throwing, finisher.accept(null) reports success despite a truncated stream.

  3. decodeMsgPackStream's "skip every second value" hack is fragile and costs 2x. Flagged independently by three review angles. The server relies on the transport inserting a raw \n between msgpack chunks (it happens to also be a valid 1-byte msgpack fixint), and both the Java (unpacker.skipValue() in json-utils.ts) and TS (count % 2 === 0 in fetch-type-utils.ts) decoders "skip" the phantom value positionally rather than via real framing. Undocumented, was added/removed/re-added across this PR's own commits, and doubles CPU/allocation per element since every real value requires decoding and discarding an adjacent phantom one.

  4. Streaming decode silently drops malformed elements instead of erroring. Non-streaming decodeJsonBody/decodeMsgPackBody throw on a guard failure; the streaming decoders in fetch-type-utils.ts just console.error and continue — one malformed record mid-stream vanishes with no signal to the consumer.

  5. Java JDK streaming client doesn't propagate typed throws-errors. The non-streaming path and the TS streaming path both convert a declared error status into a typed RSDError subtype; service.ts's streaming path (generateResponseDispatchStream) just throws IllegalStateException("Error results not yet supported") for any declared error code, degrading to a generic error. Zero test coverage today since no streaming op in the sample spec declares throws.

  6. OpenAPI generator never checks resultType.streaming. open-api/service.ts — streaming endpoints render in the generated openapi.json as ordinary "type":"array" responses, indistinguishable from synchronous list endpoints. Any third-party codegen consuming that spec would get the wrong client behavior.

  7. StringWriter.toString().getBytes() uses the platform-default charset, not UTF-8, in createJsonStreamEncoder (json-utils.ts) — inconsistent with every decode path in the same class, which is explicit about StandardCharsets.UTF_8.

Duplication left over from centralizing toResultType/toAPIResultType

There are two more near-duplicate toResultType functions beyond the two already centralized into computeAPIResultType in java-gen-utils.ts:

Both independently re-implement the same variant dispatch (differing only in how inline-enum resolves), and this PR extended each separately to add the streaming/Multi<T> branch instead of consolidating. Also: RSDError.$GenericError(...) construction is hand-written 4 times across java-rest-client-jdk/service.ts, and one copy already drifted — it hardcodes RSDError instead of routing it through fqn(...) like the other three.

Lower priority

  • generateOperationMethod/appendMethodSignature interleave 5-6 separate if (streaming) checks across one function (flagged by two review angles) — candidate for splitting into sibling functions the way the response-dispatch generators already were.
  • Streaming vs. non-streaming parameter/return-type branch duplicated between java-rest-client-jdk/service.ts and java-client-api/service.ts, with naming drift ($consumer vs consumer) as evidence it's already desyncing.
  • 12 near-identical hand-authored sample handler files (Stream*HandlerImpl.java) — test fixtures, not generator output, could collapse via a shared helper.
  • GreetingResource.java demo endpoint has commented-out debug code and a swallowed InterruptedException — sample code, low stakes.

Confirmed non-issues

Conventions clean (no CLAUDE.md violations), the @Produces per-method refactor verified correct, client/server _JsonUtils.java byte-identical is unavoidable (shared template, different packages), and the Multi.repeatUntil boundary-exclusion in StreamRecordHandlerImpl is correct per Mutiny's own semantics (verified against source + test).


Generated by an automated review pass (Claude Code); please verify before acting on any finding.

@tomsontom
tomsontom merged commit 1ee7615 into main Sep 1, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add streaming support for lists

1 participant