Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure events with both Create and Apply are only ever handled once #2559

Merged

Conversation

elexisvenator
Copy link
Contributor

Fixes #2528

Expected outcome: Each event passed into a projection is handled a maximum of once, either with a Create or an Apply method.

Old behaviour: If the first event in a stream has matching Create and Apply methods, both would be called.

New behaviour: If a Create method matches, that event is then skipped when iterating over apply event methods.

Example of issue:

    public record IncrementEvent{}

    public record CounterProjection(Guid Id, int Counter)
    {
        public static CounterProjection Create(IncrementEvent @event, IEvent metadata)
        {
            return new CounterProjection(metadata.StreamId, 1);
        }

        public CounterProjection Apply(IncrementEvent @event, CounterProjection current)
        {
            return current with {Counter = current.Counter + 1};
        }
    }

    [Fact]
    public async Task count_of_events_should_match_event_count()
    {
        var streamId = Guid.NewGuid();
        theSession.Events.Append(streamId, new IncrementEvent(), new IncrementEvent(), new IncrementEvent());
        await theSession.SaveChangesAsync();

        var aggregate = await theSession.Events.AggregateStreamAsync<CounterProjection>(streamId);

        Assert.NotNull(aggregate);
        Assert.Equal(3, aggregate.Counter); // Returned 4 before fix
    }

This required a bit of fiddling around with the codegeneration which I am definitly not super confident in. Pretty sure my changes are solid thanks to the tests covering them, but my approach may be off.

@elexisvenator elexisvenator changed the title Fix events with create and apply Ensure events with both Create and Apply are only ever handled once Apr 17, 2023
Copy link
Collaborator

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me in general, but I'm unsure if when we have #2560 then if that'll be needed. Having static Apply method capabilities could be enough, as it will do upsert. The logic in this PR seems okay, but it'll add more magic to the usage pattern. @elexisvenator, @jeremydmiller thoughts?

@jeremydmiller
Copy link
Member

I'm asking #2560 to be changed, and I see this as orthogonal anyway. Good stuff, I'm pulling this in now.

@jeremydmiller jeremydmiller merged commit 44109d3 into JasperFx:master Apr 27, 2023
14 checks passed
@agross
Copy link
Contributor

agross commented May 4, 2023

Is this included in Marten 6.0?

@mysticmind
Copy link
Member

Is this included in Marten 6.0?

yes @agross!

@mysticmind mysticmind added this to the 6.0.0 milestone May 4, 2023
@agross
Copy link
Contributor

agross commented May 4, 2023

I was asking because after upgrading to 6.0 I tested this.

The second transaction affecting the projection still calls Create first and Apply second (like it did before 6.0 was released).

Since the test above models a single transaction, could this be the reason why it does not work in my use case? 🤔

@mysticmind
Copy link
Member

@agross unsure, please do share a repro code for us to check.

@agross
Copy link
Contributor

agross commented May 4, 2023

@mysticmind See #2528 (comment)

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

Successfully merging this pull request may close these issues.

Allow projections to be started and modified by the same event type
5 participants