Skip to content

Promote a store-agnostic Events return type to core — the IEnumerable<object> fallback is positional and can silently pick the wrong collection #3941

Description

@jeremydmiller

Wolverine.Marten.Events and Wolverine.Polecat.Events are identical (class Events : List<object>, IWolverineReturnType) and store-named, so a handler that wants to be store-agnostic cannot use either. The store-agnostic path exists — it just has no name.

Where this lands today

IEventSourcingFrameProvider.TryApplyEventsFromReturnValues resolves the events return in two steps:

var eventsVariable =
    firstCall.Creates.FirstOrDefault(x => x.VariableType == provider.EventsCollectionType)
 ?? firstCall.Creates.FirstOrDefault(x =>
        x.VariableType.CanBeCastTo<IEnumerable<object>>() &&
        !x.VariableType.CanBeCastTo<IWolverineReturnType>());

So a store-agnostic handler returns a bare IReadOnlyList<object> / List<object> and hits the second clause. That works, on every provider, and is what we do in CritterWatch — e.g. a handler under [WriteModel] returning (IReadOnlyList<object>, SomeMessage?). Nothing is broken.

The problem with relying on the fallback

It is positional and implicit. FirstOrDefault takes the first match in Creates, and because IEnumerable<T> is covariant, every reference-typed collection in the return is castable to IEnumerable<object>. So:

public static (IReadOnlyList<object>, IReadOnlyList<string>) Handle(SomeCommand cmd, [WriteModel] Thing thing)

has two candidates. Whichever appears first in Creates silently becomes the events appended to the stream. Nothing fails at codegen and nothing fails at runtime — you simply get the wrong collection in the event stream.

OutgoingMessages is safe today only because it is an IWolverineReturnType, which the fallback explicitly excludes. That is a happy accident of an unrelated marker interface, not a designed guarantee, and it does not extend to a user's own collection type.

The [WriteModel] / [DeciderFunction] attributes are what make store-agnostic handlers attractive in the first place (GH-3907), so the number of handlers relying on this fallback is going up, not down.

Proposal

Promote a core Events to Wolverine.Persistence.EventSourcing, mirroring the two store-specific ones including the operator + accumulation sugar:

namespace Wolverine.Persistence.EventSourcing;

public class Events : List<object>, IWolverineReturnType
{
    public Events() { }
    public Events(IEnumerable<object> collection) : base(collection) { }
    public static Events operator +(Events events, object @event) { events.Add(@event); return events; }
}

Then (Events, OutgoingMessages) becomes a declared, unambiguous, store-agnostic signature.

⚠️ The design detail that matters: it cannot simply be added and left to the fallback. IWolverineReturnType is exactly what the fallback excludes, so a core Events would be skipped. The resolution needs a check for the core type ahead of the fallback, alongside provider.EventsCollectionType — roughly:

var eventsVariable =
    firstCall.Creates.FirstOrDefault(x => x.VariableType == provider.EventsCollectionType)
 ?? firstCall.Creates.FirstOrDefault(x => x.VariableType == typeof(Events))   // new
 ?? firstCall.Creates.FirstOrDefault(x => /* existing IEnumerable<object> fallback */);

The same addition is needed in the sibling paths that repeat this shape — DcbModelAttribute has the identical IEnumerable<object> check, and RegisterEventsFrame / BoundaryEventCaptureFrames both branch on CanBeCastTo<IEnumerable<object>>().

The store-specific Wolverine.Marten.Events / Wolverine.Polecat.Events should stay as-is for existing code, exactly as WriteAggregateAttribute was kept alongside WriteModelAttribute in GH-3907.

Explicitly not proposed

This is the single-stream case only, where [WriteModel] pins the aggregate and every event goes to its stream. It does nothing for "append to a variable number of different streams", which is a separate gap — SideEffectPolicy.lookForSingularSideEffects walks individual return variables via chain.ReturnVariablesOfType<ISideEffect>(), so a List<AppendEvents> is not seen as a side effect at all. Worth keeping the two apart.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions