fluxera/Fluxera.Repository

[Bug - EfCore Repository] Changetracker error when updating entity from UI

Closed this issue · 2 comments

jeffward01 commented

Hello!

I was working on a feature for Fluxera.Repository, when I realized quickly that you solved it in a creative way 😄 . I added notes about it.

But when I was reviewing your code, I found a case where there is a bug.. The bug is on the UpdateAsync operation, if the entity is supplied from the UI and not in the change-tracker, an error is thrown.

I corrected the code, and have some test cases for it. I did not make a PR for it yet because I wanted you to review it and discuss it. You know your code better than I do, and perhaps I am doing something that I am unaware of.

Or maybe you can just take the refactored code and perform the PR yourself, whichever is easier for you. Sometimes copy / paste is easier than a PR.

Unit Test and Refactored code (bug solved)

  • Expected:

When a user updates an existing entity from the API, the entity should be updated in the database

In this case, the user is updating an existing entity, BUT Ef Core is not tracking the entity, because the
user has created it, not EfCore

  • Actual:

Exception is thrown:

The instance of entity type 'Person' cannot be tracked because another instance with the same key value for {'ID'} ...

System.InvalidOperationException : The instance of entity type 'Person' cannot be tracked because another instance with the same key value for {'ID'} is already being tracked. When attaching existing entities, ensure that only one entity instance with a given key value is attached. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the conflicting key values.

Here is the test cases with the refactored code:

Test Cases:

  • Entity Framework Core Repository: UpdateTests.cs
    • See the unit test on line 69 for the existing code that fails
    • See my code on line 128 for the specific case that uses the refactored code and passes

Refactored Code:

  • Source Code you should review: EntityFrameworkCoreRepository.cs

    • Line: 196:

      • I was implementing a way to manually update each property in the change-tracker, but then saw your code also did this. This is what prompted me to investigate the issue. My code has not been tested, and I don't remember if I finished it, I think i did - but I think your code might make my code on line 196 obsolete, im not sure tho. I added my notes there to explain more.

    • Line 491: your existing code: PerformUpdateAsync

    • Line 542: Refactored code with notes

      • Line: 568 and 593 ◀️ The bug fix is here

Sidenotes:

  • Sorry about the code style formatting 😄, my .editorconfig is configured to make all of the methods break with (1) argument per line,
  • I like how you are verbose (explicit / strongly typed) in your general style of how you write your code., and very modular in style of programming.
    • I also like how you don't use var and verbose type declarations instead. It makes it easier to check what type is returned. Its more verbose, but I now perfer it, it is also cleaner. imo.
mgernand commented

Hi Jeff,

I copy/pasted the unit tests you wrote and implemented the fix for it based on your comments. PrepareItem is nowe called after attaching the entity and setting the changed values.

The finally block is removed and the catch just ignores any exception so we cann proceed with the default behavior and setting of the values. I am not sure if the second PrepareItem is needed, but I left it in for now.

Thanks you! 👍

jeffward01 commented

Very cool! Thanks for getting that change in! I'll add more code and open up issues as I test and review.

I will be testing more of the Update, and 'Replace' (updating without querying the items / hitting the database ) today and in the next few days.

I will refactor the performUpdate method as I review and learn. I learned about this cool feature that could help with performance during updates, see these links:

I haven't tested it yet, but it should allow us to do some cool things like this:

(I have a soft delete field on my entity, this method sets that, without hitting the database... but I don't know what happens quite yet if the entity is not in the change-tracker, so i'll be testing this and other ways to update the entities)

    private async Task<UnitResult<DomainError>> PerformSoftDeleteStepsAsync(
        IQueryable<TAggregateRoot> queryable,
        PersistOptions persistOptions,
        CancellationToken cancellationToken = default)
    {
        var deactivationTime = this._dateTimeProvider.UtcNow;
        var actionOccurred = false;
        foreach (var aggregateRoot in queryable)
        {
           
             // This is the cool code here => 
            var proxy = this._dbSet.CreateProxy();
             // ^^^
            proxy.Id = aggregateRoot.Id;
  

            var result = this.PerformSoftDeleteEntity(proxy, deactivationTime);
            actionOccurred = SetActionOccurred(result, actionOccurred);
        }

        return await this.CommitIfHasChangesAsync(actionOccurred, persistOptions, cancellationToken);
    }

Unrelated-ish, but these are some interesting reads for EfCore also:

  • 👍1