FetchForWriting/FetchLatest accept a strong-typed identifier (#5144) - #5193
Merged
Conversation
FetchForWriting<T, TId>, FetchForExclusiveWriting<T, TId> and FetchLatest<T, TId> died with a bare NullReferenceException when handed a strong-typed id wrapping the stream identity -- PaymentId(Guid), InvoiceId(string) and friends. Root cause is a planning gap, and it is wider than strong-typed ids. determineFetchPlan routes any TId that is neither Guid nor string down the natural-key branch, which passes `null!` as the identity strategy because natural-key planners do not use one. But the three lifecycle planners (Inline, Async, Live) match on the projection alone and never look at the identity, so they happily returned a plan holding that null -- and LiveFetchPlanner always matches, so nothing ever fell through to the "unable to determine a fetch plan" error. Any non-Guid/string TId that was not a natural key hit this. Two changes: - The three lifecycle planners now decline to match when the identity strategy is null. NaturalKeyFetchPlanner is ordered ahead of them and gates on NaturalKeyDefinition.OuterType, so genuine natural keys are unaffected; what changes is that a non-natural-key TId now falls through instead of producing a plan that cannot work. - Falling through, planning then asks whether TId is a registered value type. A strong-typed id IS the stream identity, just wrapped, so the plan for the underlying Guid/string is reused via UnwrappedIdentityFetchPlan, a pure forwarder that unwraps on the way in. IAggregateFetchPlan<TDoc, in TId> is contravariant and every member takes the identity as input, which is what makes forwarding sufficient. A wrapper over anything other than Guid or string cannot address a stream and now says so instead of failing obscurely. Found by the cross-store StrongTypedIdentityCompliance suite (marten#5144), which had to fall back to passing raw identities to get green. Both products' existing tests pass the raw value and Marten's own doc sample explicitly unwraps, so this overload had never been exercised with a wrapper. Regression tests verified to fail on master first (5 x NullReferenceException). EventSourcingTests 1694/0/7 on net9.0.
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.
FetchForWriting<T, TId>,FetchForExclusiveWriting<T, TId>andFetchLatest<T, TId>died with a bareNullReferenceExceptionwhen handed a strong-typed id wrapping the stream identity —PaymentId(Guid),InvoiceId(string)and friends.Root cause — a planning gap, wider than strong-typed ids
determineFetchPlanroutes anyTIdthat is neitherGuidnorstringdown the natural-key branch, which passesnull!as the identity strategy because natural-key planners do not use one:But the three lifecycle planners match on the projection alone and never look at the identity:
…so they returned a plan holding that null, and it blew up on first use.
LiveFetchPlanneralways matches, so nothing ever fell through to the existing "unable to determine a fetch plan" error. Any non-Guid/stringTIdthat was not a natural key hit this — strong-typed ids are just the case someone finally tried.The fix
1. The three lifecycle planners decline to match when the identity strategy is null.
NaturalKeyFetchPlanneris ordered ahead of them (_builtInPlanners= NaturalKey, Inline, Async, Live) and gates onNaturalKeyDefinition.OuterType == typeof(TId), so genuine natural keys are unaffected. What changes is that a non-natural-keyTIdnow falls through instead of producing a plan that cannot work.2. Falling through, planning asks whether
TIdis a registered value type. A strong-typed id is the stream identity, just wrapped — so the plan for the underlyingGuid/stringis reused via a newUnwrappedIdentityFetchPlan, a pure forwarder that unwraps on the way in.IAggregateFetchPlan<TDoc, in TId>is contravariant and every member takes the identity as input, which is what makes forwarding sufficient rather than requiring a parallel plan.A wrapper over anything other than
Guidorstringcannot address a stream, and now says so explicitly instead of failing obscurely.How it was found
The cross-store
StrongTypedIdentityCompliancesuite (#636, marten#5144), which had to fall back to passing raw identities to get green. Worth noting why this survived so long: every strong-typed-id test in both Marten and Polecat passes the raw value, and Marten's own doc sample explicitly unwraps —— so the generic overload had simply never been called with a wrapper, despite being generic over the identity type and being the obvious thing to reach for.
Verification
Bug_5144_strong_typed_id_fetch_overloads— 6 tests covering Guid-backed and string-backed wrappers across Inline/Async lifecycles, exclusive writing,FetchLatest, and an unknown id yielding an empty handle. Verified to fail on master first (5 ×NullReferenceException).Full
EventSourcingTests: 1694 passed / 0 failed / 7 skipped on net9.0.Polecat parity for the same overload is being checked separately — this shape carries forward to the Sqlite consumer (#5156), which is the reason for fixing it rather than documenting the raw-value workaround.