From 3c26ecbf879028d077ee59f6f72d181bddf7e550 Mon Sep 17 00:00:00 2001 From: JaredScar Date: Fri, 30 Jan 2026 12:05:11 -0500 Subject: [PATCH] Rid of bulk delete error --- .../Services/Implementations/EventService.cs | 32 +++++++++- .../DeleteCollectionCommand.cs | 27 +++++++- .../DeleteCollectionCommandTests.cs | 62 +++++++++++++++++++ 3 files changed, 116 insertions(+), 5 deletions(-) diff --git a/src/Core/Dirt/Services/Implementations/EventService.cs b/src/Core/Dirt/Services/Implementations/EventService.cs index 77d481890e..155e3eb828 100644 --- a/src/Core/Dirt/Services/Implementations/EventService.cs +++ b/src/Core/Dirt/Services/Implementations/EventService.cs @@ -152,6 +152,20 @@ public class EventService : IEventService public async Task LogCollectionEventsAsync(IEnumerable<(Collection collection, EventType type, DateTime? date)> events) { var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync(); + + // Batch lookup provider IDs for all unique organization IDs upfront + var uniqueOrgIds = events + .Select(e => e.collection.OrganizationId) + .Distinct() + .Where(orgId => CanUseEvents(orgAbilities, orgId)) + .ToList(); + + var providerIds = new Dictionary(); + foreach (var orgId in uniqueOrgIds) + { + providerIds[orgId] = await GetProviderIdAsync(orgId); + } + var eventMessages = new List(); foreach (var (collection, type, date) in events) { @@ -166,7 +180,7 @@ public class EventService : IEventService CollectionId = collection.Id, Type = type, ActingUserId = _currentContext?.UserId, - ProviderId = await GetProviderIdAsync(collection.OrganizationId), + ProviderId = providerIds.GetValueOrDefault(collection.OrganizationId), Date = date.GetValueOrDefault(DateTime.UtcNow) }); } @@ -183,6 +197,20 @@ public class EventService : IEventService public async Task LogGroupEventsAsync(IEnumerable<(Group group, EventType type, EventSystemUser? systemUser, DateTime? date)> events) { var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync(); + + // Batch lookup provider IDs for all unique organization IDs upfront + var uniqueOrgIds = events + .Select(e => e.group.OrganizationId) + .Distinct() + .Where(orgId => CanUseEvents(orgAbilities, orgId)) + .ToList(); + + var providerIds = new Dictionary(); + foreach (var orgId in uniqueOrgIds) + { + providerIds[orgId] = await GetProviderIdAsync(orgId); + } + var eventMessages = new List(); foreach (var (group, type, systemUser, date) in events) { @@ -197,7 +225,7 @@ public class EventService : IEventService GroupId = group.Id, Type = type, ActingUserId = _currentContext?.UserId, - ProviderId = await GetProviderIdAsync(group.OrganizationId), + ProviderId = providerIds.GetValueOrDefault(group.OrganizationId), SystemUser = systemUser, Date = date.GetValueOrDefault(DateTime.UtcNow) }; diff --git a/src/Core/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommand.cs b/src/Core/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommand.cs index 4f678633a9..be65c35c1b 100644 --- a/src/Core/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommand.cs +++ b/src/Core/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommand.cs @@ -4,6 +4,7 @@ using Bit.Core.Exceptions; using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces; using Bit.Core.Repositories; using Bit.Core.Services; +using Microsoft.Extensions.Logging; namespace Bit.Core.OrganizationFeatures.OrganizationCollections; @@ -11,13 +12,16 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand { private readonly ICollectionRepository _collectionRepository; private readonly IEventService _eventService; + private readonly ILogger _logger; public DeleteCollectionCommand( ICollectionRepository collectionRepository, - IEventService eventService) + IEventService eventService, + ILogger logger) { _collectionRepository = collectionRepository; _eventService = eventService; + _logger = logger; } public async Task DeleteAsync(Collection collection) @@ -28,7 +32,15 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand } await _collectionRepository.DeleteAsync(collection); - await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Deleted, DateTime.UtcNow); + + try + { + await _eventService.LogCollectionEventAsync(collection, Enums.EventType.Collection_Deleted, DateTime.UtcNow); + } + catch (Exception ex) + { + _logger.LogError(ex, "Failed to log collection deletion event for collection {CollectionId}", collection.Id); + } } public async Task DeleteManyAsync(IEnumerable collectionIds) @@ -46,6 +58,15 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand } await _collectionRepository.DeleteManyAsync(collections.Select(c => c.Id)); - await _eventService.LogCollectionEventsAsync(collections.Select(c => (c, Enums.EventType.Collection_Deleted, (DateTime?)DateTime.UtcNow))); + + try + { + await _eventService.LogCollectionEventsAsync(collections.Select(c => (c, Enums.EventType.Collection_Deleted, (DateTime?)DateTime.UtcNow))); + } + catch (Exception ex) + { + var collectionIds = string.Join(", ", collections.Select(c => c.Id)); + _logger.LogError(ex, "Failed to log collection deletion events for collections: {CollectionIds}", collectionIds); + } } } diff --git a/test/Core.Test/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommandTests.cs b/test/Core.Test/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommandTests.cs index efe9223f1b..123a21f4ce 100644 --- a/test/Core.Test/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommandTests.cs +++ b/test/Core.Test/OrganizationFeatures/OrganizationCollections/DeleteCollectionCommandTests.cs @@ -90,4 +90,66 @@ public class DeleteCollectionCommandTests .LogCollectionEventsAsync(default); } + [Theory, BitAutoData] + [OrganizationCustomize] + public async Task DeleteManyAsync_WithManyCollections_DeletesAllCollections(SutProvider sutProvider) + { + // Arrange - Create 100 collections to test bulk delete performance + var collections = new List(); + var collectionIds = new List(); + + for (int i = 0; i < 100; i++) + { + var collection = new Collection + { + Id = Guid.NewGuid(), + OrganizationId = Guid.NewGuid(), + Type = CollectionType.SharedCollection, + Name = $"Collection {i}" + }; + collections.Add(collection); + collectionIds.Add(collection.Id); + } + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(collectionIds) + .Returns(collections); + + // Act + await sutProvider.Sut.DeleteManyAsync(collectionIds); + + // Assert + await sutProvider.GetDependency().Received() + .DeleteManyAsync(Arg.Is>(ids => ids.SequenceEqual(collectionIds))); + + await sutProvider.GetDependency().Received().LogCollectionEventsAsync( + Arg.Is>(a => + a.Count() == 100 && + a.All(c => collectionIds.Contains(c.Item1.Id) && c.Item2 == EventType.Collection_Deleted))); + } + + [Theory, BitAutoData] + [OrganizationCustomize] + public async Task DeleteManyAsync_WhenEventLoggingFails_StillDeletesCollections(Collection collection, SutProvider sutProvider) + { + // Arrange + var collectionIds = new[] { collection.Id }; + collection.Type = CollectionType.SharedCollection; + + sutProvider.GetDependency() + .GetManyByManyIdsAsync(collectionIds) + .Returns(new List { collection }); + + sutProvider.GetDependency() + .LogCollectionEventsAsync(Arg.Any>()) + .Returns(_ => throw new Exception("Event logging failed")); + + // Act - Should not throw exception even though event logging fails + await sutProvider.Sut.DeleteManyAsync(collectionIds); + + // Assert - Collections should still be deleted + await sutProvider.GetDependency().Received() + .DeleteManyAsync(Arg.Is>(ids => ids.SequenceEqual(collectionIds))); + } + }