Skip to content

NGRequest: parse the body lazily (raw body + on-demand form values), not eagerly at the adaptor #66

Description

@hugithordarson

Summary

Make NGRequest lazy about interpreting the request body: stop draining the content stream and parsing form values eagerly at construction (in the adaptor), and instead expose the raw body, parsing form values on demand — and only when the content type actually says the body is a form.

This consolidates several existing FIXMEs in NGStandardRequest that are all facets of the same design smell, and removes a real footgun the dev /eval endpoint just had to work around.

The problem, concretely

Today the adaptor reads form values off the body first, then hands an already-drained stream to NGStandardRequest, which copies whatever's left into its content buffer:

  • NGAdaptorJetty.requestToNGRequestparametersFromRequest(...) runs, then Request.asInputStream(...) (which is now spent for a form-encoded POST). Same in the servlet adaptor.
  • NGStandardRequest constructor — contentStream.transferTo(_contentByteStream), with the FIXME already on it:

    // FIXME: We're consuming the entire content stream at construction time for now. Eventually, whether to do this should be the consumer's decision // Hugi 2026-05-12

Consequence: for an application/x-www-form-urlencoded POST, contentString() returns empty — the raw body is gone, shredded into the form map (split on =/&). A handler that wanted the raw body has already lost, before it ran. The /ng/dev/eval endpoint hit exactly this: real Java in a curl --data body (which defaults to form-encoding) is unrecoverable, so eval now has to detect the case and tell the caller to send text/plain instead.

This is not WO's behavior — WO keeps _contentData separate from parsed form values, so its contentString() survives form parsing. But WO does it by storing the body twice (raw copy + parsed values) on every request, which is a memory tax we don't want to copy. The goal here is neither "drain eagerly" (current) nor "keep two copies" (WO) but "decide lazily."

Proposed direction

Move the decision from the adaptor to NGRequest:

  1. The adaptor hands NGRequest the raw body (bytes/stream) and the headers — and does not pre-parse form values.
  2. formValues() / formValueForKey(...) parse the body on first call, and only if the content type is application/x-www-form-urlencoded (query-string params are always available; they don't touch the body).
  3. contentBytes() / contentString() return the raw body untouched.
  4. Parse once, cache the result.

A handler that wants the raw body reads it; a handler that wants form values triggers the parse. The two never fight, and neither pays for the other. This keeps ng's "a body is consume-once" philosophy — we just stop making the irreversible choice at the door.

FIXMEs this would retire / address

All already in NGStandardRequest, all the same underlying complaint ("the adaptor shouldn't be pre-populating / pre-deciding this; the request should model it"):

  • L76 — content stream consumed at construction; "should be the consumer's decision" ← the core of this issue
  • L136 — "The Map should be populated by the request object, not the adaptor" (form values)
  • L260 — "Cookie header deserialization should happen in NGRequest instead of in the adaptor" (cookies — same shape; worth folding in)
  • L154 / L171 — "NGRequest wants to be immutable" — the _setFormValues / _setCookieValues setters exist only because the adaptor pushes these in; lazy parsing lets them go, moving NGRequest toward the immutability it says it wants.

Notes / scope

  • Multipart (multipart/form-data, file uploads) is a separate content type and is already handled specially in the adaptor; keep that path as-is for now (it has its own FIXME about final design) or fold it into the same lazy model — decide during implementation, don't let it block the urlencoded case.
  • Thread-safety: lazy parse needs to be safe for concurrent first-callers (the file already flags formValueForKey/formValuesForKey as needing thread-safety, L305/L322) — do the parse under a lock / compute-once.
  • remoteAddress (added recently for the /eval loopback check, with its own FIXME) is the same category — an adaptor-populated setter on a request that wants to be immutable. Worth sweeping into the same redesign.
  • Both adaptors (ng-adaptor-jetty, ng-adaptor-jetty-servlet) construct requests; both change.

Context

Surfaced while building the /ng/dev/eval dev endpoint (part of the tool/agent dev loop driven by the parslips-dev-loop skill). The eval endpoint is correct as shipped — it rejects a form-encoded body with a clear "send text/plain" error — but that workaround exists only because of the eager-parse behavior above; this issue removes the need for it and cleans up the request model generally. Filed at the maintainer's request while the analysis was fresh; NGRequest is acknowledged to be in flux, so this is direction, not a rush.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions