SQLite: RETURNING clause doesn't work with AFTER triggers

[bridgesquared]

The change in v7.0.0 to use the SQLite RETURNING clause during updates, as opposed to the extra SELECT statement in v6.x, means that @bricelam's concurrency strategy for SQLite https://www.bricelam.net/2020/08/07/sqlite-and-efcore-concurrency-tokens.html no longer works when making multiple saves from an existing context.

Though the theory of using RETURNING to avoid an extra round-trip is great, the current SQLite implementation: https://www.sqlite.org/lang_returning.html#limitations_and_caveats means that RETURNING does not return any values generated by an AFTER trigger and as a result concurrency checks fail when making subsequent SaveChangesAsync calls from an existing context.

This issue only really arises if you are making multiple calls to SaveChangesAsync for the same context, and hence wouldn't arise if you use a new context each time but we are hitting it when doing batch imports where we re-use the context for performance optimisation.

To be honest, this is not a bug in EF Core, it's merely that a side-effect of the change in EF Core behaviour causes an edge-case to fail because of a current limitation in the SQLite implementation of RETURNING.

I think the best strategy at the moment would be if there was the ability to opt out of the new SQLite update behaviour as suggested by #29512 - hence the feature request.

We have found a workaround for this particular issue by using a SaveChangesInterceptor whereby we update the version counter in the context before persistence, but I would prefer to use a trigger if possible.

[roji]

As this is a second case of RETURNING not being supported on SQLite, we should consider a general metadata flag for not using RETURNING (UseSqlReturningClause(false)?), and set that by convention when e.g. a table is configured as virtual. We could do the same for SQL Server.

[ChaosEngine]

Or You could use same pattern as SqlServer is doing for notifying EF core there are triggers:

entity.ToTable( t => t.HasTrigger("MyTrigger") );

I've tested across various providers and and only Sqlite in ef-7 is suffering from this.

[ChaosEngine]

I was able to overcome this issue by changing the triggers from AFTER to BEFORE on the Sqlite side. But this could very misleading sometimes and could lead to hidden errors.

[bridgesquared]

@ChaosEngine - I agree that BEFORE triggers would probably lead to hidden errors, my brain is having serious trouble thinking through the edge cases.

FYI the work around we've adopted is to use a purely EF side solution:

We define a concurrency interface:

public interface IVersionedConcurrency
{
    public int Version { get; set; }
}

And declare appropriate entities as implementing this interface

public class Entity : IVersionedConcurrency {
    public int Id { get; set; }
    ...
    public int Version { get; set; }
}

And we configure these entities as using the version field for concurrency

public class EntityConfiguration : IEntityTypeConfiguration<Entity> {

    public void Configure(EntityTypeBuilder<Entity> builder) {
        ...
        builder.Property(e => e.Version).IsConcurrencyToken();
        ...
    }
}

To take care of incrementing the version on save, we created an interceptor:

public class SaveChangesInterceptor : Microsoft.EntityFrameworkCore.Diagnostics.SaveChangesInterceptor
{
    public override ValueTask<InterceptionResult<int>> SavingChangesAsync(DbContextEventData eventData, InterceptionResult<int> result, CancellationToken cancellationToken = default)
    {
        foreach (var entry in eventData.Context!.ChangeTracker.Entries().Where(e => e.State == EntityState.Modified))
        {
            if (entry.Entity is IVersionedConcurrency v)
            {
                v.Version += 1;
            }
        }
        return new(result);
    }
}

And then add the interceptor into the context:

public class DataContext : DbContext {

    private static SaveChangesInterceptor _saveChangesInterceptor = new SaveChangesInterceptor();
    ...
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        base.OnConfiguring(optionsBuilder);
        optionsBuilder.AddInterceptors(_saveChangesInterceptor);
    }
    ...
}

And this all works quite happily, it means that an entity instance that is "live" in the context will have the version field updated every time we save it.

I'm sure there are some improvements to be made:

• Look at using a shadow property to store the version field data - this would reduce the "contamination" of the domain model.
• Look to using a convention or similar to avoid the need for an interface - again to avoid implementation details in the domain model.

Overall though, I would still prefer to use of triggers as that way the versioning works regardless of whether we access the data through EF or some other framework. So in the first instance a switch on EF Core to revert to the old non-RETURNING strategy would be ideal, and then secondly I'm going to ask the SQLite team to see if there is a time line for adding the side-effects from AFTER triggers to the data returned by the RETURNING clause.

[ChaosEngine]

Nice but still, this is edge case for Sqlite only. In my project I want to support many DB providers (MySql, Sqlite, SqlServer, Oracle, Postgres) and doing that whole shebang for single one is .... a "code smell". That's why I went with changing trigger for sqlite ONLY. But that's just me :-)

[roji]

Providing a standalone opt-out from RETURNING is now tracked by #29916.

We do have a relational API for defining triggers (which currently does nothing), but the issue with RETURNING in Sqlite is only with a certain kind of triggers (AFTER triggers); so it doesn't seem right to have a convention the disables RETURNING for any trigger. If/when we one day provide actual trigger support for Sqlite, we can have a convention that kicks in only for AFTER triggers.

Should we close this as a dup of #29916?

[roji]

Note: for anyone blocked by this, you can revert to the previous SQLite behavior (without the RETURNING clause) by calling ReplaceService as follows:

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    => optionsBuilder
        .UseSqlite("Filename=/tmp/foo.sqlite")
        .ReplaceService<IUpdateSqlGenerator, SqliteLegacyUpdateSqlGenerator>();

Note that SqliteLegacyUpdateSqlGenerator is considered an internal service (in the Internal namespace), and will very likely go away in EF 8.0. But this is a good workaround in the meantime.

[bridgesquared]

Hey all, closing this issue as an opt-out is being worked on under #29916 and we now have a high-quality workaround identified by @roji. Many thanks to you all.

[roji]

Duplicate of #29916