Skip to content

Skip the body copy on a StreamOne 304, and close the revision-ETag test gaps - #5157

Merged
jeremydmiller merged 1 commit into
masterfrom
fix/5121-followup-streamone-revision-etag
Aug 3, 2026
Merged

Skip the body copy on a StreamOne 304, and close the revision-ETag test gaps#5157
jeremydmiller merged 1 commit into
masterfrom
fix/5121-followup-streamone-revision-etag

Conversation

@jeremydmiller

@jeremydmiller jeremydmiller commented Aug 3, 2026

Copy link
Copy Markdown
Member

Follow-up to #5121 (merged as 0e8daed), addressing the review points raised there. No behavior change to the ETag values themselves — same headers, same 304 semantics — plus one documentation correction where the shipped docs describe behavior the code does not have.

Efficiency

A 304 no longer buffers the document body. VersionSelectClause appends mt_version after data, but every reader accesses columns by GetOrdinal(name) and Marten never opens readers with CommandBehavior.SequentialAccess — so the row is fully buffered and read order is free. StreamOneWithVersion now reads the version first and consults a shouldWriteBody predicate before copying the payload. On a conditional-request hit WriteSingle declines the body, so the pooled MemoryStream is never grown and the document is never copied. The row still comes back from Postgres either way; this saves the copy, not the read. Benefits the Guid path identically.

No more boxing on the revision read. Convert.ToInt64(GetFieldValueAsync<object>(...)) is replaced with a GetFieldType branch picking GetFieldValueAsync<int> or <long> directly — same #4614 both-widths robustness, one less allocation per request on the projection read path.

The two duplicated reader methods collapse into one. StreamOneWithVersion and StreamOneWithRevision were near-verbatim copies differing only in the CLR type read back; they are now a single method taking a numericRevision flag, and MartenLinqQueryProvider has one branch instead of two. StreamOneJsonResult/StreamOneReadResult gain BodyWritten.

Test coverage

Six new tests, all Alba end-to-end through theHost.Scenario against real minimal-API endpoints except where noted:

  • stream_one_with_revision_etag_executes_a_single_db_command — the companion to the existing Guid-path acceptance test. The revision flavor is the projection read-model path, so pin that document + ETag resolve in ONE round trip. (Direct WriteSingle against a counting IMartenSessionLogger, matching the existing test's shape.)
  • stream_one_does_not_buffer_the_document_body_on_a_304 — pins the change above: a 20KB projected document with a matching If-None-Match leaves the response buffer at zero length.
  • stream_one_emits_revision_etag_for_long_versioned_document — new LongVersionedIssueNote : ILongVersioned keeps the bigint column where IRevisioned narrows to integer (UseTenantPartitionedEvents is incompatible with optimistic appends / FetchForWriting (aggregate handlers) #4614), so the pair now covers both widths on purpose rather than incidentally.
  • stream_one_returns_404_without_etag_for_a_revisioned_document — the 404-leaks-no-header pin existed only for the Guid path.
  • stream_one_suppresses_etag_on_a_revisioned_document_when_emit_etag_is_false — likewise for the EmitETag = false opt-out, and asserts the document is still served rather than only the header dropped.
  • interface_driven_revisions_plus_optimistic_concurrency_fails_fast (EventSourcingTests) — the likeliest real-world route into Derive the StreamOne ETag from the numeric revision for revisioned documents #5121's new guard and the one neither existing guard test covers: nothing in the configuration says "numeric revisions", VersionedPolicy turns them on because the type implements IRevisioned, and a fluent UseOptimisticConcurrency(true) layers the Guid version on top. Verified against master before the guard: this reached the database as MartenSchemaException: DDL Execution ... Failed from two mt_version columns.

Documentation correction

docs/documents/aspnetcore.md claimed EventProjection output documents "emit no ETag unless they opt into a versioning flavor themselves". They do emit one. ProjectionDocumentPolicy only forces numeric revisions onto aggregate projection targets, so an EventProjection's output keeps the plain-document default — Guid version metadata — and emits a quoted GUID ETag that changes on every projection write. That is a usable cache validator but is not a stream version and does not line up with StreamAggregate<T>. stream_one_emits_a_guid_etag_for_an_event_projection_output_document pins the real behavior against a new OrderTouchProjection in IssueService.

Also: #5121's fail-fast guard shipped undocumented. docs/documents/concurrency.md now carries a warning naming both routes into the invalid state, since neither reads as "I asked for both flavors".

Deliberately not included

Verified locally (net10.0)

Marten.AspNetCore.Testing 107/107 · EventSourcingTests guard file 3/3 · LinqTests, DocumentDbTests, EventSourcingTests full suites clean. markdownlint and cspell clean on the changed docs.

🤖 Generated with Claude Code

…st gaps

Follow-up to #5121. The ETag values and 304 semantics are unchanged.

Efficiency:

- Read mt_version off the row BEFORE the payload and consult a shouldWriteBody
  predicate, so a conditional-request hit never grows the pooled MemoryStream or
  copies the document. Column order is free here: readers access by GetOrdinal
  and Marten never opens them with CommandBehavior.SequentialAccess. The row
  still comes back from Postgres, so this saves the copy, not the read. Applies
  to the Guid path identically.
- Drop the Convert.ToInt64(GetFieldValueAsync<object>()) box on the revision
  read in favor of a GetFieldType branch, keeping the #4614 both-widths handling.
- Collapse StreamOneWithVersion/StreamOneWithRevision, which differed only in the
  CLR type read back, into one method behind a numericRevision flag.

Tests:

- Pin the single round trip on the revision path (the Guid path already had one).
- Pin that a 304 leaves the response buffer empty.
- Cover ILongVersioned (bigint) alongside IRevisioned (integer) on purpose.
- Cover 404-emits-no-ETag and EmitETag = false on a revisioned document.
- Cover the IRevisioned + UseOptimisticConcurrency route into #5121's guard,
  which is the likeliest real-world one and was untested. Verified against master
  that it previously failed as DDL with two mt_version columns.

Docs:

- Correct the claim that EventProjection outputs emit no ETag. They are not
  aggregate projection targets, so ProjectionDocumentPolicy leaves them on the
  plain-document default and they emit a Guid ETag that changes on every
  projection write -- a usable cache validator, but not a stream version.
- Document #5121's fail-fast guard, which shipped undocumented.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jeremydmiller
jeremydmiller marked this pull request as ready for review August 3, 2026 13:58
@jeremydmiller
jeremydmiller merged commit 77efabc into master Aug 3, 2026
10 checks passed
@jeremydmiller
jeremydmiller deleted the fix/5121-followup-streamone-revision-etag branch August 3, 2026 14:03
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.

1 participant