fullstackhero / blazor-starter-kit

Clean Architecture Template for Blazor WebAssembly Built with MudBlazor Components.
MIT License
3.48k stars 730 forks source link

SaveChangesAsync impropper override #276

Open PizzaConsole opened 3 years ago

PizzaConsole commented 3 years ago

BlazorHeroContext.SaveChangesAsync does not actually override AuditableDbContext.SaveChangesAsync.

AuditableContext

        public virtual async Task<int> SaveChangesAsync(string userId = null, CancellationToken cancellationToken = new())
        {
            var auditEntries = OnBeforeSaveChanges(userId);
            var result = await base.SaveChangesAsync(cancellationToken);
            await OnAfterSaveChanges(auditEntries, cancellationToken);
            return result;
        }

BlazorHeroContext

This does NOT override the AuditableContext it overrides the DbContext is it missing string userId = null,

        public override async Task<int> SaveChangesAsync(CancellationToken cancellationToken = new())
        {
            foreach (var entry in ChangeTracker.Entries<IAuditableEntity>().ToList())
            {
                switch (entry.State)
                {
                    case EntityState.Added:
                        entry.Entity.CreatedOn = _dateTimeService.NowUtc;
                        entry.Entity.CreatedBy = _currentUserService.UserId;
                        break;

                    case EntityState.Modified:
                        entry.Entity.LastModifiedOn = _dateTimeService.NowUtc;
                        entry.Entity.LastModifiedBy = _currentUserService.UserId;
                        break;
                }
            }
            if (_currentUserService.UserId == null)
            {
                return await base.SaveChangesAsync(cancellationToken);
            }
            else
            {
                return await base.SaveChangesAsync(_currentUserService.UserId, cancellationToken);
            }
        }

Example in ChatService: When it saves the changes here it is calling the AuditConext NOT the HeroContext

        public async Task<IResult> SaveMessageAsync(ChatHistory<IChatUser> message)
        {
            message.ToUser = await _context.Users.Where(user => user.Id == message.ToUserId).FirstOrDefaultAsync();
            await _context.ChatHistories.AddAsync(_mapper.Map<ChatHistory<BlazorHeroUser>>(message));
            await _context.SaveChangesAsync();
            return await Result.SuccessAsync();
        }
274188A commented 3 years ago

Not sure what you mean by overriding - can you explain a bit more what the problem you're seeing? Did you turn-off ChangeTracking by any chance?

PizzaConsole commented 3 years ago

@274188A See above

274188A commented 3 years ago

So the job of the AuditableContext is to keep a history of changes made. It only takes care of entities that have implemented the IAuditable interface. So yes it will save to the AuditContext as well as the BlazorHeroContext.

You can check for yourself by inspecting the Tables ChatHistory and AuditTrails

So the idea basically is that the Save call for any entity that implements the IAuditable interface will have it call intercepted if you like to do some entries into the AuditTrails table. for example if you were able to edit a chat item then those edit changes would be recorded in the AuditTrails table. The ChatHistory is the chat itself - not the 'edited changes'. I can see how it would be easy to get this mixed up due to the naming...

PizzaConsole commented 3 years ago

Ah, I see that OnAfterSaveChanges calls SaveChangesAsync which calls it from BlazorHeroContext , yeah that is a bit confusing.

PizzaConsole commented 3 years ago

I know I just closed this, but I just walked through the code in a debugging session and I understand what is happening now. It is inefficient. If you follow the save when it is called in the chat service you can see that it calls AuditableContext.SaveChangesAsync first then it calls BlazorHeroContext.SaveChangesAsync in AuditableContext.OnAfterSaveChanges which then calls AuditableContext.SaveChangesAsync again... this can definitely be optimized.

274188A commented 3 years ago

might be best to raise a fresh ticket with a good Title to help out the author

PizzaConsole commented 3 years ago

Or you could just suggest a "good" title to rename this ticket

274188A commented 3 years ago

wow - good luck champ

PizzaConsole commented 3 years ago

Haha that was my reaction. I will just let the author review this when they get time.