JasperFx / marten

.NET Transactional Document DB and Event Store on PostgreSQL
https://martendb.io
MIT License
2.82k stars 445 forks source link

TeardownDataOnRebuild = false prevents deleting existing projections #3213

Closed AlexZeitler closed 4 months ago

AlexZeitler commented 4 months ago

It's a repro for this discussion on Discord.

AlexZeitler commented 4 months ago

When commenting out this line, the test succeeds.

AlexZeitler commented 4 months ago

Would it make sense to add TeardownDataOnRebuild to AsyncOptions and make the call to teardownExistingProjectionProgress (here) conditional based on the new property?

I've added it locally and set the option when adding the projection and my test then succeeded.

But it also broke some other tests.

AlexZeitler commented 4 months ago

Digging into the history, I noticed that TeardownDataOnRebuild started to be available starting with Commit 3a9ba4e435d38dd366e3a93f54d9d0c6f50f2e72, but it already broke with the next one f7642cdfe5d0fd1399ac9ff7a3245e809e7b8ef8.

The test I ran is similar to the one in this PR but it's using MultiStreamAggregation (before it has been refactored to MultiStreamProjection.

At first, Commit 682c9e1ef2cf1726f2ec0ef6df2c8af77d4f40b7 looked suspicious to my because of the refactoring, but aside from completely removing the check completely it didn't make the result worse.

using System;
using System.Threading;
using System.Threading.Tasks;
using Marten.Events.Projections;
using Marten.Testing.Harness;
using Xunit;

namespace Marten.AsyncDaemon.Testing;

public class Base
{
    public string Id { get; set; }
}

public class ImplementationA: Base
{
}

public class ImplementationB: Base
{
}

public class SomethingHappened
{
    public Guid Id { get; set; }
}

public class ImplementationAProjection: MultiStreamAggregation<ImplementationA, string>
{
    public ImplementationAProjection()
    {
        Identity<SomethingHappened>(e => $"a-{e.Id}");
        TeardownDataOnRebuild = false;
    }

    public static ImplementationA Create(
        SomethingHappened e
    ) => new() { Id = $"a-{e.Id}" };
}

public class ImplementationBProjection: MultiStreamAggregation<ImplementationB, string>
{
    public ImplementationBProjection()
    {
        Identity<SomethingHappened>(e => $"b-{e.Id}");
        TeardownDataOnRebuild = false;
    }

    public static ImplementationB Create(
        SomethingHappened e
    ) => new() { Id = $"b-{e.Id}" };
}

public class MultiStreamDataTeardownOnRebuildTests: OneOffConfigurationsContext
{
    [Fact]
    public async Task Adheres_to_disabled_teardown_on_rebuild()
    {
        StoreOptions(
            opts =>
            {
                opts.Schema.For<Base>()
                    .AddSubClass<ImplementationA>()
                    .AddSubClass<ImplementationB>();

                opts.Projections.Add<ImplementationAProjection>(ProjectionLifecycle.Inline);

                opts.Projections.Add<ImplementationBProjection>(ProjectionLifecycle.Inline);
            }
        );

        var commonId = Guid.NewGuid();

        using var daemon = await theStore.BuildProjectionDaemonAsync();
        await using var session = theStore.LightweightSession();

        session.Events.StartStream(
            commonId,
            new SomethingHappened() { Id = commonId }
        );
        await session.SaveChangesAsync();

        var implementationA = await session.LoadAsync<ImplementationA>($"a-{commonId}");
        var implementationB = await session.LoadAsync<ImplementationB>($"b-{commonId}");

        implementationA.ShouldNotBeNull();
        implementationB.ShouldNotBeNull();

        await daemon.RebuildProjection<ImplementationAProjection>(CancellationToken.None);

        var implementationBAfterRebuildOfA = await session.LoadAsync<ImplementationB>($"b-{commonId}");

        implementationBAfterRebuildOfA.ShouldNotBeNull();
    }
}
AlexZeitler commented 4 months ago

After a few random checks in the history up to the current status, I am tempted to say that it has never worked again apart from the first commit.

AlexZeitler commented 4 months ago

Turns out, that the failing tests also failed without my changes.

Before applying my changes: image

After applying my changes: image

AlexZeitler commented 4 months ago

As can be seen in the tests, the fix doesn't use the existing TeardownDataOnRebuild property from ProjectionBase but Options.TeardownDataOnRebuild from AsyncOptions on IProjectionSource.

Having TeardownDataOnRebuild in AsyncOptions it can be set either in the Projection implementation or in the registration.