Repository navigation
Read only the named stream for Guid.Empty, an empty key and a null key, and refuse them in CompactStreamAsync - #5695
Merged
Conversation
…y, and refuse them in CompactStreamAsync EventStatement emitted its stream predicate only if (StreamId != Guid.Empty), and only for a non-empty StreamKey. Every caller sets exactly one of the two, so "empty means no filter" was never a contract -- it fell out of the property defaults. For Guid.Empty, "" and a null key alike: - FetchStreamAsync and the batched FetchStream returned every non-archived event of the tenant (of the store, when single-tenanted); - AggregateStreamAsync<T>, and the live plan's FetchLatest<T> from a session or a batch, folded them all into one aggregate; - CompactStreamAsync<T> rewrote the newest event in the store -- another stream's -- as Compacted<T> and deleted every other event, in one transaction. A set value is now a stream. EventStatement.StreamId is Guid?, and a string key is tracked as assigned by its setter rather than read as non-empty. Guid.Empty and "" read that stream: what BulkInsertEventsAsync (or legacy data, or SQL) wrote under it, and nothing otherwise, as a read of any unknown id does. A null key names no stream -- stream_id is never null -- so it reads nothing. Both CompactStreamAsync overloads refuse Guid.Empty and a null or empty key with the ArgumentOutOfRangeException Append and StartStream already throw, so the one API here that deletes events never depends on a read predicate for its reach. FetchForWriting needed no change on either path: it already ends in the start-stream guard for all three, and the tests pin that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 20, 2026
This was referenced Oct 27, 2026
This was referenced Oct 29, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CompactStreamAsync<T>(Guid.Empty)compacts the whole store. Take a store with two ordinary streams and noGuid.Emptystream at all. CompactingGuid.Emptyrewrites the newest event in the store asCompacted<T>, although that event belongs to another stream, and deletes every other event, all in one transaction. Onmasterthe other stream's three events are simply gone:CompactStreamAsync<T>("")andCompactStreamAsync<T>(null)do the same on a string-identified store.The reads underneath are wrong in the same way, for
Guid.Empty,""and a null key alike:FetchStreamAsyncand the batchedFetchStreamreturn every non-archived event of the tenant, or of the whole store when it is single-tenanted.AggregateStreamAsync<T>, and the live plan'sFetchLatest<T>from a session or a batch, fold all of those events into one aggregate.Root cause
EventStatement.filters()emits the stream predicate onlyif (StreamId != Guid.Empty), and only forStreamKey.IsNotEmpty(). All eight places that construct anEventStatementset exactly one of the two, so nothing relies on "empty means no filter". It only falls out of the property defaults: the string overloads leaveStreamIdatGuid.Empty.Marten refuses these values on
AppendandStartStream. They still reach these reads in two ways:Guid.Empty.BulkInsertEventsAsyncwrites one:StreamAction.Startdoes not guard the id, and neither does the bulk path. The test uses that path. By readingEventBoundaryandBulkEventAppender, a DCB boundary routing on an empty tag value and legacy or hand-written SQL can leave one behind too. I have not tested those two.Fix
The rule is a set value is a stream.
EventStatement.StreamIdbecomesGuid?. A string key is tracked as assigned by its setter rather than read as non-empty.Guid.Emptyand""read that stream: its events if one exists, and nothing otherwise. That matches what every read already does for an id nobody wrote, and whatFetchStreamStateAsync(Guid.Empty)already does (where id = ?, unconditionally).stream_idis never null, so it reads nothing. It emitsfalse, the predicate Marten's LINQ already uses for a constantfalse.Both
CompactStreamAsyncoverloads refuseGuid.Emptyand a null or empty key withArgumentOutOfRangeException, using the messagesAppendandStartStreamalready use. With the read fixed, compacting a realGuid.Emptystream would only touch that stream. But compaction is the one API here that deletes events, so it should not depend on a read predicate for its reach. Every other write already refuses these ids.Why reads filter rather than throw. A read of an id nobody wrote already returns an empty list. Throwing would turn
GET /streams/{id}with a defaulted id into a 500 instead of an empty result, and the behaviour that existed was a data leak rather than a contract anyone could rely on. For a null key specifically, nothing in Marten refuses one withArgumentNullExceptiontoday, so a read that returns nothing matches existing behaviour best. As measured on this branch:FetchStreamStateAsync((string)null)fails incidentally, in Npgsql:InvalidOperationException: Parameter '' cannot be null, DBNull.Value should be used instead.FetchForWriting<T>andFetchForExclusiveWriting<T>with a null key throw the start-stream guard'sArgumentOutOfRangeException. With an expected version they throwConcurrencyException.Append(null)throwsArgumentOutOfRangeException.FetchForWriting<T>needs no change for any of these ids, on either path: the session's (BuildEventQueryHandler(bool, string, IEventStorage, …)) or the batch's (BuildEventQueryHandler(bool, string, ISqlFragment?)). Once it has read, it goes throughStartStream/Append, which throw. Before this change it read the whole tenant first. A test pins the refusal on both paths.Only
EventStatementand the two compaction entry points change. The store-levelIEventStore.CompactStreamAsyncreaches the session-level overloads, so it is covered as well.Tests
src/EventSourcingTests/empty_stream_identity_is_not_a_wildcard.cshas 22 cases:FetchStreamAsyncwith and without a stream stored underGuid.Empty, the batchedFetchStream,AggregateStreamAsyncandFetchLatest.""andnull:FetchStreamAsync, the batchedFetchStream,AggregateStreamAsync,FetchLatestfrom the session and from a batch, andFetchForWritingfrom the session and from a batch.Guid.Empty(with and without such a stream),""andnull.The data-loss assertions run before the refusal assertion, so a regression reads as the lost events rather than as a missing exception.
Red on
master(2ece3a9)20 of 22 fail. The 2 that pass are the
FetchForWritingpins, which hold before and after.Green with the fix
Neighbouring tests that go through
EventStatement, all on the fix, net10.0, againstpostgres:17-alpine:🤖 Generated with Claude Code