Fix StreamOne streaming the document id as the body on an identity-tracking session - #5166
Conversation
…sion The ETag support added in JasperFx#5015 selects mt_version alongside the payload and aliases the payload to "data", because the streaming reader looks it up by that name. It aliases by POSITION, on the strength of a comment in DocumentTable.SelectColumns claiming "the order of the selection is data, id, everything else". That order is not what the method does. It puts the id first whenever the id is selected at all, and IdColumn.ShouldSelect is storageStyle != QueryOnly. So the select list starts d.data only for a QueryOnly session; through any identity-tracking session it starts d.id, d.data. Aliasing field 0 then put the alias on the id column, the reader looked up "data", found the id, and streamed the document's id as the entire response body: GET /minimal/issue/{id} 200 OK Content-Length: 38 ETag: "1" "422c81ae-d73c-48ac-be1f-4eb65eefb606" A 200 whose payload does not deserialize into the document type. Reported against 9.22.3 from an application whose endpoints receive a session from Wolverine's Marten integration rather than a QueryOnly IQuerySession. Every existing endpoint in IssueService takes IQuerySession, which is why the whole ETag test suite passed over it. This adds the missing shape — one endpoint streaming through an IDocumentSession — and matches the payload column by name instead of by position. The positional fallback stays for an inner clause that projects rather than selects the column (SelectDataSelectClause's jsonb_build_object under a Select(), JasperFx#5158), where there is no column name to match and the projection is the only candidate anyway. The new test fails on main with "the body must be the document, not its id" and passes with the fix. Marten.AspNetCore.Testing is 114 green on both TFMs.
|
Sharpening the scope, because "identity-tracking session" undersells it: every Wolverine.HTTP endpoint gets one, including the ones that ask for Our endpoint is declared exactly the way the docs suggest: [WolverineGet("/api/v1/institution-invoice/{id}")]
public static StreamOne<InstitutionInvoice> Get(string id, IQuerySession session)
=> new(session.Query<InstitutionInvoice>().Where(x => x.Id == id));and Wolverine's Marten integration generates this: await using var documentSession = _outboxedSessionFactory.OpenSession(messageContext, tenantId);
...
InstitutionInvoiceEndpoint.Get(id, ((Marten.IQuerySession)documentSession));It opens a document session for the outbox and casts it to satisfy the parameter. So the storage style is never Which makes the blast radius: on 9.22.3, every Two things follow that I would not have written into the original description:
Workaround for anyone hitting this before a release: I would suggest this warrants a patch rather than riding the next feature release — the failure mode is a wrong 200 on a default-on code path. |
StreamOne<T>returns 200 with the document's id as the whole body when the query runs on an identity-tracking session. Found on 9.22.3 in an application whose HTTP endpoints receive their session from Wolverine's Marten integration rather than aQueryOnlyIQuerySession.which surfaces at the caller as
38 bytes is a quoted GUID, and that was the clue: the body is the id column.
Cause
VersionSelectClause<T>.innerFields()aliases the payload todata— the name the streaming reader looks it up by — by position:on the strength of its own comment: "
DocumentTable.SelectColumnsguarantees the payload is the first selected field ('the order of the selection is data, id, everything else')".SelectColumnsdoes not do that. It adds the id first whenever the id is selected:and
IdColumn.ShouldSelectisstorageStyle != StorageStyle.QueryOnly. So the select list startsd.dataonly forQueryOnly; through a lightweight, identity-map or dirty-tracking session it startsd.id, d.data. The alias landed on the id,GetOrdinal("data")found the id column, andWriteJsonValueAsyncstreamed the id.The comment in
SelectColumnsis describing what older code assumes, not what the method guarantees — worth reading as a warning rather than a contract.Why the tests did not catch it
Every streaming endpoint in
IssueServicetakesIQuerySession, which resolves to aQueryOnlysession — the one storage style where the positional assumption happens to hold. So the entire ETag suite (#5010, #5015, #5027, #5120, #5157) exercised only the case that works.This PR adds the missing shape: one endpoint streaming through an
IDocumentSession, and a test that asserts the body is the document rather than its id. It fails onmainwith "the body must be the document, not its id".Fix
Match the payload column by name instead of by position. The positional fallback stays for an inner clause that projects rather than selects the column —
SelectDataSelectClause'sjsonb_build_object(...)under aSelect()(#5158) — where there is no column name to match and the projection is the only candidate anyway. So both shapes that motivated the alias keep working, and the ETag assertion is in the new test to prove the fix did not cost the header this code path exists for.Scope worth a second opinion
Only
StreamOnegoes throughVersionSelectClause, soStreamMany/StreamPaged/StreamPagedByCursorare unaffected — they readdatafrom an unaliased select list.StreamAggregatestreams a live aggregation, not a document row. ButEmitETagdefaults totrue, so on 9.22.3 everyStreamOneendpoint that receives a tracking session returns the id instead of the document; ours was one endpoint of several and the others simply were not covered by a test that deserializes the body. That may be worth a patch release rather than waiting for the next feature drop.