Ensure that we delist the trigger session on exception in the BeforeSave cycle

[koenbeuk]

Closes #168

[anthony-keller]

I've done some testing after applying these changes manually to the code I've been using and have encountered a different issue.

We have an integration test:

                var booking = _seeder.SuppSrcedBnplBooking;

                var effectiveAt = new DateTimeOffset(2021, 11, 11, 3, 0, 0, TimeSpan.Zero);

                var ex1 = await Should.ThrowAsync<NotImplementedException>(() =>
                    PaymentGenerator.InsertPaymentAsync(db, PaymentMethodEnum.Afterpay.ToString(), 100m, null, effectiveAt));
                ex1.Message.ShouldBe("Couldn't find any applicable payment method for given args. " +
                    $"EffectiveAt: {effectiveAt}, PaymentMethodId: {PaymentMethodEnum.Afterpay}, BookingTypeId: , SupplierId: , SupplierCommissionPercentage: ");

                var ex2 = await Should.ThrowAsync<NotImplementedException>(() =>
                    PaymentGenerator.InsertPaymentAsync(db, PaymentMethodEnum.Afterpay.ToString(), 100m, booking.BookingID, effectiveAt));
                ex2.Message.ShouldBe("Couldn't find any applicable payment method for given args. " +
                    $"EffectiveAt: {effectiveAt}, PaymentMethodId: {PaymentMethodEnum.Afterpay}, BookingTypeId: {booking.BookingTypeID}, SupplierId: {booking.SupplierID}, SupplierCommissionPercentage: 0.00000");

The save changes fires a before save trigger:

        public async Task BeforeSave(ITriggerContext<Payment> context, CancellationToken cancellationToken)
        {
            if (context.ChangeType != ChangeType.Added)
            {
                return;
            }

            var payment = context.Entity;

            var fees = await _paymentMerchantFeesService.CalculateFeesAsync(
                payment.PaymentMethodId,
                payment.AmountIncGst,
                payment.BookingID,
                payment.CreatedAt,
                cancellationToken);

            payment.MerchantFeeExGst = fees.MerchantFeeExGst;
        }

Which eventually thows a NotImplementedException:

                throw new NotImplementedException(
                    "Couldn't find any applicable payment method for given args. " +
                    $"EffectiveAt: {effectiveAt}, " +
                    $"PaymentMethodId: {paymentMethodId}, " +
                    $"BookingTypeId: {bookingTypeId}, " +
                    $"SupplierId: {supplierId}, " +
                    $"SupplierCommissionPercentage: {supplierCommissionPercentage}");

This delists the session using the code in the catch block.

When the second insert fires, the context.Entity of the payment does not have it's BookingID property set even though it is definitely passed to the InsertPaymentAsync method:

        public static async Task<Payment> InsertPaymentAsync(AutoGuruDbContext db, string paymentMethodId,
            decimal amountIncGst, int? bookingId, DateTimeOffset createdAt)
        {
            var payment = new Payment
            {
                PaymentMethodId = paymentMethodId,
                AmountIncGst = amountIncGst,
                TransactionId = Guid.NewGuid().ToString(),
                TransactionStatus = "authorise_pending",
                TransactionStatusDate = createdAt,
                CreatedAt = createdAt,
                BookingID = bookingId
            };

            await db.InsertAsync(payment);

            return payment;
        }

If I comment out the DelistTriggerSession the BookingID is populated.

[anthony-keller]

Becomes null in context.Entity

[anthony-keller]

Changed the test to use a different payment method and amount

                // Act / Assert
                var ex1 = await Should.ThrowAsync<NotImplementedException>(() =>
                    PaymentGenerator.InsertPaymentAsync(db, PaymentMethodEnum.Afterpay.ToString(), 100m, null, effectiveAt));
                ex1.Message.ShouldBe("Couldn't find any applicable payment method for given args. " +
                    $"EffectiveAt: {effectiveAt}, PaymentMethodId: {PaymentMethodEnum.Afterpay}, BookingTypeId: , SupplierId: , SupplierCommissionPercentage: ");

                // Act / Assert
                var ex2 = await Should.ThrowAsync<NotImplementedException>(() =>
                    PaymentGenerator.InsertPaymentAsync(db, PaymentMethodEnum.Openpay.ToString(), 120m, booking.BookingID, effectiveAt));
                ex2.Message.ShouldBe("Couldn't find any applicable payment method for given args. " +
                    $"EffectiveAt: {effectiveAt}, PaymentMethodId: {PaymentMethodEnum.Afterpay}, BookingTypeId: {booking.BookingTypeID}, SupplierId: {booking.SupplierID}, SupplierCommissionPercentage: 0.00000");

The second Insert still gets the old payment values in context.Entity in the trigger. Without the Delist added in this PR, the values are correct.

[koenbeuk]

I've added some additional integration tests to see if I could reproduce (which I was not), here: https://github.com/koenbeuk/EntityFrameworkCore.Triggered/blob/issue-168/test/EntityFrameworkCore.Triggered.IntegrationTests/TriggerExceptions/ExceptionTestScenario.cs

In your shared code, you do reuse the db instance between the 2 calls. Could the exception be due to the db instance still holding on on earlier data (generated during the first call)?

Keep in mind that throwing an exception in a BeforeSave trigger will not reset the state of the DbContext instance, e.g.

var entity = new Entity();
db.Add(entity);
try {
  db.SaveChanges(); // Assume that this throws in a BeforeSaveTrigger
}
catch (Exception ex) { ... }

Console.WriteLine(db.Entry(entity).State); // Prints: Added

Any subsequent call to SaveChanges will try and insert this entity again and rerun all the triggers

[anthony-keller]

Good point. Let me refactor it a bit and retest

[anthony-keller]

You're right - the object had the same hash code in the second call. I will run the tests today (and test #156) and let you know.

Appreciate your work 💯

[koenbeuk]

Keep me posted, I've hidden the 3.2.2 release on nuget for now. Just in case there is a regression here

[anthony-keller]

@koenbeuk I have tried to reproduce the issue using the new version and have not been able to. I've not noticed any other side effects so I believe the fix is good.