win7user10 / Laraue.EfCoreTriggers

Library to write triggers in C# with EF.Core
MIT License
112 stars 20 forks source link

EfCoreDbSchemaRetriever does not manage it's cache with dependency injection #58

Closed JustusGreiberORGADATA closed 1 year ago

JustusGreiberORGADATA commented 2 years ago

Hi,

im using this library and just parallelised the test execution of all of my EF Core tests. But while doing this, I hit the following error:

 Message: 
  Initialization method Ofcas.Odid.IntegrationTests.CoreTest.ServicesTest.AddressServiceTest.TestInitialize threw exception. System.ArgumentException: An item with the same key has already been added. Key: Ofcas.Odid.Core.Entities.AddressEntity.

  Stack Trace: 
    Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
    Dictionary`2.Add(TKey key, TValue value)
    EfCoreDbSchemaRetriever.GetTableName(Type entity)
    TriggerMethodCallConverter.Visit(MethodCallExpression expression, ArgumentTypes argumentTypes, VisitedMembers visitedMembers) line 27
    MethodCallExpressionVisitor.Visit(MethodCallExpression expression, ArgumentTypes argumentTypes, VisitedMembers visitedMembers)
    ExpressionVisitorFactory.Visit[TExpression](TExpression expression, ArgumentTypes argumentTypes, VisitedMembers visitedMembers)
    ExpressionVisitorFactory.Visit(Expression expression, ArgumentTypes argumentTypes, VisitedMembers visitedMembers)
    TriggerRawActionVisitor.Visit(TriggerRawAction triggerAction, VisitedMembers visitedMembers)
    TriggerActionVisitorFactory.Visit[T](T triggerAction, VisitedMembers visitedMembers)
    TriggerActionVisitorFactory.Visit(ITriggerAction triggerAction, VisitedMembers visitedMembers)
    SqliteTriggerVisitor.<GenerateCreateTriggerSql>b__3_0(ITriggerAction action)
    SelectListIterator`2.ToArray()
    SqliteTriggerVisitor.GenerateCreateTriggerSql(ITrigger trigger)
    MigrationsExtensions.ConvertTriggerAnnotationsToSql(ITriggerVisitor triggerVisitor, IModel model)
    MigrationsModelDiffer.GetDifferences(IRelationalModel source, IRelationalModel target)
    RelationalDatabaseCreator.GetCreateTablesCommands(MigrationsSqlGenerationOptions options)
    RelationalDatabaseCreator.CreateTables()
    SQLiteConnectionTestHelper.TestInitialize() line 53

I am using a custom MethodCallConverter called TriggerMethodCallConverter, which injects IDbSchemaRetriever.

public class TriggerMethodCallConverter : BaseMethodCallVisitor
{
    private readonly IDbSchemaRetriever _dbSchemaRetriever;

    protected override Type ReflectedType => typeof(TriggerFunctions);

    protected override string MethodName => nameof(TriggerFunctions.GetDbTableName);

    public TriggerMethodCallConverter(IDbSchemaRetriever dbSchemaRetriever, IExpressionVisitorFactory visitorFactory) : base(visitorFactory)
    {
        _dbSchemaRetriever = dbSchemaRetriever;
    }

    public override SqlBuilder Visit(MethodCallExpression expression, ArgumentTypes argumentTypes, VisitedMembers visitedMembers)
    {
        var genericArguments = expression.Method.GetGenericArguments();
        return SqlBuilder.FromString(_dbSchemaRetriever.GetTableName(genericArguments[0]));
    }
}

Because IDbSchemaRetriever.GetTableName caches the table names in a static variable, I assume i run into a race condition here:

https://github.com/win7user10/Laraue.EfCoreTriggers/blob/e30ecb03d5bb1ec4ea7d0057c331eb1a623c3a5f/src/Laraue.EfCoreTriggers.Common/Services/Impl/EfCoreDbSchemaRetriever.cs#L68-L74

I think instead of a static variable, the cache should be managed by the dependency injection container of EFCore. Then each of my DbContexts will have a different cache.

The other option would be to use a concurrent dictionary, but I feel like that is not 100% clean.

Furthermore you might still need a concurrent dictionary using the first option, I am not 100% sure how the concurrency guarantees are, if two DbContext instances share the same dependency container.

win7user10 commented 1 year ago

Hi, fixed in 6.4.1/5.4.1