Implement events for before and after SaveChanges

[ajcvickers]

Splitting this out as a separate issue as required on #626 so we can track it independently of other life-cycle hooks.

• SavingChanges: An event fired when the entity object is just about to be written to the database.
• SavedChanges: An event fired after the SavingChanges operation has completed (successfully or otherwise)

[nphmuller]

For my main use case: A query level cache, it looks like I'd need an event after SaveChanges.
Specifically, I need to know if the transaction has been committed (succeeded) or has been rolled back.

The query level cache we used for EF6 was based on this project: https://github.com/moozzyk/EFCache
We're basically thinking of porting this to EFCore if the necessary events are supported. Of course things maybe can be simplified.

Basically EFCache is using a custom DBCommand to figure out which entity sets are affected during Save/Rollback. It only needs to invalidate every cache entry of the specific entity set, so that's all the information it needs. The invalidation is done via IDbTransactionInterceptor's RolledBack and Committed hooks.

If the new AfterSaveChanges event of EFCore supported the following this logic described above could be simplified:

• The event needs to expose if the transaction got committed or if it got rolled back.
• If it could expose which entity sets are affected by the transaction, much of the logic of EFCache could be simplified.

See also this comment by the author of EFCache: #5858 (comment)

[ajcvickers]

@nphmuller Thanks for the info. I've also been doing some experiments with database interceptors, similar to EF6, which might be a better fit for this.

[douglasg14b]

My use case for this would be logging and audit trails.

[SidShetye]

@ajcvickers Could we please have this for 3.1 (along with it's counterpart in #15911)

[ajcvickers]

@SidShetye We haven't decided what will be in 3.1 yet.

[SARAVANA1501]

@ajcvickers I would like to contribute this, it would be great if you provide more implementation details.

[ajcvickers]

Removing to discuss in triage: would an interceptor or normal events be better here?

[SidShetye]

For us, the normal event is what is required.

[ajcvickers]

@SARAVANA1501 We discussed this in triage and agreed that the interceptor is most appropriate. This will allow SaveChanges to have other functionality than is normal. See the patterns for existing interceptors (e.g. IDbCommandInterceptor) to get started.

Writing events to fire when the interceptor fires should be trivial. If not, we could revisit simple events as well.

[SARAVANA1501]

@ajcvickers , Will take a look IDbCommandInterceptor

[SARAVANA1501]

public interface IDbContextLifeCycleHook { void OnSaveChangesStarting(); void OnSaveChangesCompleted(int entitiesSaved); }

OnSaveChangesStarting invoked before save changes in DbContext.
OnSaveChangesCompleted invoked after save changes in DbContext.

@ajcvickers will it serve the purpose?

[ajcvickers]

@SARAVANA1501 It should more closely follow the pattern for other interceptors, including appropriate use of event data classes, sync/async overloads, interception results, etc. Off the top of my head something like this:

public class SaveChangesEventData : DbContextEventData
{
    public SaveChangesEventData(
        EventDefinitionBase eventDefinition, 
        Func<EventDefinitionBase, EventData, string> messageGenerator, 
        DbContext context,
        IList<IUpdateEntry> entriesToSave,
        bool acceptChangesEnabled) 
        : base(eventDefinition, messageGenerator, context)
    {
        // TODO
    }
}

public interface ISaveChangesInterceptor : IInterceptor
{
    InterceptionResult<int> SavingChanges(
        SaveChangesEventData eventData,
        InterceptionResult<int> result); 

    Task<InterceptionResult<int>> SavingChangesAsync(
        SaveChangesEventData eventData,
        InterceptionResult<int> result,
        CancellationToken cancellationToken = default); 

    int SavedChanges(
        SaveChangesCompletedEventData eventData,
        int result); 

    Task<int> SavedChangesAsync(
        SaveChangesCompletedEventData eventData,
        int result,
        CancellationToken cancellationToken = default); 

    void SavingChangesFiled(
        SaveChangesFailedEventData eventData); 
}

[SidShetye]

@ajcvickers Taking one step back, based off this,

would an interceptor or normal events be better here?

what's the difference between the interceptors and events? Just a class vs a function or something more conceptually different? We've historically registered on the events, not interceptors, so that's our familiar go-to.

The proposal above at face value seems like it may cover what we're looking for by wrapping our events into an interceptor class. (BTW SavingChangesFiled should be SavingChangesFailed right?).

How would one register the interceptor with EF? I guess it'll be very clear once this is implemented and there is a code sample demonstrating it's usage in documentation.

@SARAVANA1501 Thanks for picking this up. Do you think it'll make it to the EF Core 3.1 release?

[nphmuller]

@SidShetye

Thanks for picking this up. Do you think it'll make it to the EF Core 3.1 release?

Pretty sure it’s been mentioned that 3.1 will basically be only bug fixes.

[SARAVANA1501]

@ajcvickers Did you get chance to look at the above code? I am waiting for your comments to continue with other test cases.

[ajcvickers]

@SARAVANA1501 Sorry for being slow here. I think it might help if I provide a details on how an interceptor is intended to work:

• Each conceptual event will have a "before" and an "after" interceptor method.
• One purpose of the before method is to allow the interceptor to stop execution of this event--so in this case this would prevent EF from actually saving changes to the database.
• When this happens EF still needs the result to continue execution. So in this case the number of entities saved needs to be returned from the interceptor as the InterceptionResult.

The best way to get this all right is to thoroughly understand some of the existing interceptors and repeat the pattern here. For example, IDbCommandInterceptor has some good examples which do similar things. All the testing should also follow the patterns already used for existing interceptors.

[dncnkrs]

With no recent motion here would anyone object to me picking this up?

@SARAVANA1501?