mirror of
https://github.com/bitwarden/server.git
synced 2026-01-31 06:03:12 +08:00
Rid of bulk delete error
This commit is contained in:
@@ -152,6 +152,20 @@ public class EventService : IEventService
|
|||||||
public async Task LogCollectionEventsAsync(IEnumerable<(Collection collection, EventType type, DateTime? date)> events)
|
public async Task LogCollectionEventsAsync(IEnumerable<(Collection collection, EventType type, DateTime? date)> events)
|
||||||
{
|
{
|
||||||
var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync();
|
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<Guid, Guid?>();
|
||||||
|
foreach (var orgId in uniqueOrgIds)
|
||||||
|
{
|
||||||
|
providerIds[orgId] = await GetProviderIdAsync(orgId);
|
||||||
|
}
|
||||||
|
|
||||||
var eventMessages = new List<IEvent>();
|
var eventMessages = new List<IEvent>();
|
||||||
foreach (var (collection, type, date) in events)
|
foreach (var (collection, type, date) in events)
|
||||||
{
|
{
|
||||||
@@ -166,7 +180,7 @@ public class EventService : IEventService
|
|||||||
CollectionId = collection.Id,
|
CollectionId = collection.Id,
|
||||||
Type = type,
|
Type = type,
|
||||||
ActingUserId = _currentContext?.UserId,
|
ActingUserId = _currentContext?.UserId,
|
||||||
ProviderId = await GetProviderIdAsync(collection.OrganizationId),
|
ProviderId = providerIds.GetValueOrDefault(collection.OrganizationId),
|
||||||
Date = date.GetValueOrDefault(DateTime.UtcNow)
|
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)
|
public async Task LogGroupEventsAsync(IEnumerable<(Group group, EventType type, EventSystemUser? systemUser, DateTime? date)> events)
|
||||||
{
|
{
|
||||||
var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync();
|
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<Guid, Guid?>();
|
||||||
|
foreach (var orgId in uniqueOrgIds)
|
||||||
|
{
|
||||||
|
providerIds[orgId] = await GetProviderIdAsync(orgId);
|
||||||
|
}
|
||||||
|
|
||||||
var eventMessages = new List<IEvent>();
|
var eventMessages = new List<IEvent>();
|
||||||
foreach (var (group, type, systemUser, date) in events)
|
foreach (var (group, type, systemUser, date) in events)
|
||||||
{
|
{
|
||||||
@@ -197,7 +225,7 @@ public class EventService : IEventService
|
|||||||
GroupId = group.Id,
|
GroupId = group.Id,
|
||||||
Type = type,
|
Type = type,
|
||||||
ActingUserId = _currentContext?.UserId,
|
ActingUserId = _currentContext?.UserId,
|
||||||
ProviderId = await GetProviderIdAsync(group.OrganizationId),
|
ProviderId = providerIds.GetValueOrDefault(group.OrganizationId),
|
||||||
SystemUser = systemUser,
|
SystemUser = systemUser,
|
||||||
Date = date.GetValueOrDefault(DateTime.UtcNow)
|
Date = date.GetValueOrDefault(DateTime.UtcNow)
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ using Bit.Core.Exceptions;
|
|||||||
using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
|
using Bit.Core.OrganizationFeatures.OrganizationCollections.Interfaces;
|
||||||
using Bit.Core.Repositories;
|
using Bit.Core.Repositories;
|
||||||
using Bit.Core.Services;
|
using Bit.Core.Services;
|
||||||
|
using Microsoft.Extensions.Logging;
|
||||||
|
|
||||||
namespace Bit.Core.OrganizationFeatures.OrganizationCollections;
|
namespace Bit.Core.OrganizationFeatures.OrganizationCollections;
|
||||||
|
|
||||||
@@ -11,13 +12,16 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand
|
|||||||
{
|
{
|
||||||
private readonly ICollectionRepository _collectionRepository;
|
private readonly ICollectionRepository _collectionRepository;
|
||||||
private readonly IEventService _eventService;
|
private readonly IEventService _eventService;
|
||||||
|
private readonly ILogger<DeleteCollectionCommand> _logger;
|
||||||
|
|
||||||
public DeleteCollectionCommand(
|
public DeleteCollectionCommand(
|
||||||
ICollectionRepository collectionRepository,
|
ICollectionRepository collectionRepository,
|
||||||
IEventService eventService)
|
IEventService eventService,
|
||||||
|
ILogger<DeleteCollectionCommand> logger)
|
||||||
{
|
{
|
||||||
_collectionRepository = collectionRepository;
|
_collectionRepository = collectionRepository;
|
||||||
_eventService = eventService;
|
_eventService = eventService;
|
||||||
|
_logger = logger;
|
||||||
}
|
}
|
||||||
|
|
||||||
public async Task DeleteAsync(Collection collection)
|
public async Task DeleteAsync(Collection collection)
|
||||||
@@ -28,7 +32,15 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand
|
|||||||
}
|
}
|
||||||
|
|
||||||
await _collectionRepository.DeleteAsync(collection);
|
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<Guid> collectionIds)
|
public async Task DeleteManyAsync(IEnumerable<Guid> collectionIds)
|
||||||
@@ -46,6 +58,15 @@ public class DeleteCollectionCommand : IDeleteCollectionCommand
|
|||||||
}
|
}
|
||||||
|
|
||||||
await _collectionRepository.DeleteManyAsync(collections.Select(c => c.Id));
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -90,4 +90,66 @@ public class DeleteCollectionCommandTests
|
|||||||
.LogCollectionEventsAsync(default);
|
.LogCollectionEventsAsync(default);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Theory, BitAutoData]
|
||||||
|
[OrganizationCustomize]
|
||||||
|
public async Task DeleteManyAsync_WithManyCollections_DeletesAllCollections(SutProvider<DeleteCollectionCommand> sutProvider)
|
||||||
|
{
|
||||||
|
// Arrange - Create 100 collections to test bulk delete performance
|
||||||
|
var collections = new List<Collection>();
|
||||||
|
var collectionIds = new List<Guid>();
|
||||||
|
|
||||||
|
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<ICollectionRepository>()
|
||||||
|
.GetManyByManyIdsAsync(collectionIds)
|
||||||
|
.Returns(collections);
|
||||||
|
|
||||||
|
// Act
|
||||||
|
await sutProvider.Sut.DeleteManyAsync(collectionIds);
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
await sutProvider.GetDependency<ICollectionRepository>().Received()
|
||||||
|
.DeleteManyAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.SequenceEqual(collectionIds)));
|
||||||
|
|
||||||
|
await sutProvider.GetDependency<IEventService>().Received().LogCollectionEventsAsync(
|
||||||
|
Arg.Is<IEnumerable<(Collection, EventType, DateTime?)>>(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<DeleteCollectionCommand> sutProvider)
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var collectionIds = new[] { collection.Id };
|
||||||
|
collection.Type = CollectionType.SharedCollection;
|
||||||
|
|
||||||
|
sutProvider.GetDependency<ICollectionRepository>()
|
||||||
|
.GetManyByManyIdsAsync(collectionIds)
|
||||||
|
.Returns(new List<Collection> { collection });
|
||||||
|
|
||||||
|
sutProvider.GetDependency<IEventService>()
|
||||||
|
.LogCollectionEventsAsync(Arg.Any<IEnumerable<(Collection, EventType, DateTime?)>>())
|
||||||
|
.Returns<Task>(_ => 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<ICollectionRepository>().Received()
|
||||||
|
.DeleteManyAsync(Arg.Is<IEnumerable<Guid>>(ids => ids.SequenceEqual(collectionIds)));
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user