Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IBasicRepository.DeleteManyAsync in RepositoryExtensions.HardDeleteAsync #7453

Merged
merged 5 commits into from Sep 27, 2021

Conversation

olicooper
Copy link
Contributor

@olicooper olicooper commented Jan 25, 2021

This relates to the recently implemented bulk operations. This PR should make RepositoryExtensions.HardDeleteAsync more performant when deleting collections of entities with a predicate, whilst not being a breaking change.

I considered creating a new HardDeleteManyAsync(Expression...) method, but it seems redundant given that IRepository providers who don't support bulk delete will just cycle the IEnumerable anyway and we would have to duplicate the hard delete code for no obvious benefit.

This PR is currently missing a HardDeleteManyAsync(IEnumerable<TEntity>...) method to delete collections of already fetched entities. I wasn't sure where to place it so I was thinking about creating a new issue instead.

@olicooper
Copy link
Contributor Author

olicooper commented Jan 26, 2021

I think the build is failing on the MongoDB tests because the DeleteManyAsync method is not checking IsHardDeleted unlike the DeleteAsync method.

if (typeof(ISoftDelete).IsAssignableFrom(typeof(TEntity)))

if (entity is ISoftDelete softDeleteEntity && !IsHardDeleted(entity))

Replace MongoDbRepository.DeleteManyAsync with the test code below (t is really bad, I know 😆) which will allow the tests to pass:

See the code...
        public override async Task DeleteManyAsync(
            IEnumerable<TEntity> entities,
            bool autoSave = false,
            CancellationToken cancellationToken = default)
        {
            var entityArray = entities.ToArray();

            foreach (var entity in entityArray)
            {
                await ApplyAbpConceptsForDeletedEntityAsync(entity);
                SetNewConcurrencyStamp(entity);
            }

            var dbContext = await GetDbContextAsync(GetCancellationToken(cancellationToken));
            var collection = dbContext.Collection<TEntity>();

            if (BulkOperationProvider != null)
            {
                await BulkOperationProvider.DeleteManyAsync(this, entityArray, dbContext.SessionHandle, autoSave, cancellationToken);
                return;
            }

            var entitiesCount = entityArray.Count();

            if (typeof(ISoftDelete).IsAssignableFrom(typeof(TEntity)))
            {
                var softEntityArray = entities.Where(entity => !IsHardDeleted(entity)).ToArray();
                var softEntitiesCount = softEntityArray.Count();

                if (softEntitiesCount > 0)
                {
                    UpdateResult updateResult;
                    if (dbContext.SessionHandle != null)
                    {
                        updateResult = await collection.UpdateManyAsync(
                            dbContext.SessionHandle,
                            CreateEntitiesFilter(softEntityArray),
                            Builders<TEntity>.Update.Set(x => ((ISoftDelete)x).IsDeleted, true)
                            );
                    }
                    else
                    {
                        updateResult = await collection.UpdateManyAsync(
                            CreateEntitiesFilter(softEntityArray),
                            Builders<TEntity>.Update.Set(x => ((ISoftDelete)x).IsDeleted, true)
                            );
                    }

                    if (updateResult.MatchedCount < softEntitiesCount)
                    {
                        ThrowOptimisticConcurrencyException();
                    }
                }

                var hardEntityArray = entities.Except(softEntityArray).ToArray();
                var hardEntitiesCount = hardEntityArray.Count();

                if (hardEntitiesCount > 0)
                {
                    DeleteResult deleteResult;
                    if (dbContext.SessionHandle != null)
                    {
                        deleteResult = await collection.DeleteManyAsync(
                            dbContext.SessionHandle,
                            CreateEntitiesFilter(hardEntityArray)
                            );
                    }
                    else
                    {
                        deleteResult = await collection.DeleteManyAsync(
                            CreateEntitiesFilter(hardEntityArray)
                            );
                    }

                    if (deleteResult.DeletedCount < hardEntitiesCount)
                    {
                        ThrowOptimisticConcurrencyException();
                    }
                }
            }
            else
            {
                DeleteResult deleteResult;
                if (dbContext.SessionHandle != null)
                {
                    deleteResult = await collection.DeleteManyAsync(
                        dbContext.SessionHandle,
                        CreateEntitiesFilter(entityArray)
                        );
                }
                else
                {
                    deleteResult = await collection.DeleteManyAsync(
                        CreateEntitiesFilter(entityArray)
                        );
                }

                if (deleteResult.DeletedCount < entitiesCount)
                {
                    ThrowOptimisticConcurrencyException();
                }
            }
        }

@maliming maliming self-requested a review January 26, 2021 01:00
@maliming maliming added this to the 4.3-preview milestone Jan 26, 2021
@maliming maliming marked this pull request as draft January 26, 2021 03:39
@maliming maliming removed this from the 4.3-preview milestone Jan 26, 2021
@maliming maliming removed their request for review January 26, 2021 03:39
@olicooper olicooper mentioned this pull request Jan 26, 2021
@enisn
Copy link
Member

enisn commented Jan 27, 2021

Thanks @olicooper
I'm working on it

@olicooper olicooper changed the base branch from rel-4.2 to dev January 27, 2021 17:08
@olicooper
Copy link
Contributor Author

olicooper commented Jan 27, 2021

Are we happy with this implementation? Should I add a HardDeleteAsync(IEnumerable<TEntity>...) method?
I can add the following:

public static async Task HardDeleteAsync<TEntity>(
    this IBasicRepository<TEntity> repository,
    IEnumerable<TEntity> entities,
    bool autoSave = false,
    CancellationToken cancellationToken = default
)
    where TEntity : class, IEntity, ISoftDelete
{
    if (!(ProxyHelper.UnProxy(repository) is IUnitOfWorkManagerAccessor unitOfWorkManagerAccessor))
    {
        throw new AbpException($"The given repository (of type {repository.GetType().AssemblyQualifiedName}) should implement the {typeof(IUnitOfWorkManagerAccessor).AssemblyQualifiedName} interface in order to invoke the {nameof(HardDeleteAsync)} method!");
    }

    var uowManager = unitOfWorkManagerAccessor.UnitOfWorkManager;
    if (uowManager == null)
    {
        throw new AbpException($"{nameof(unitOfWorkManagerAccessor.UnitOfWorkManager)} property of the given {nameof(repository)} object is null!");
    }

    if (uowManager.Current == null)
    {
        using (var uow = uowManager.Begin())
        {
            await HardDeleteWithUnitOfWorkAsync(repository, entities, autoSave, cancellationToken, uowManager.Current);
            await uow.CompleteAsync(cancellationToken);
        }
    }
    else
    {
        await HardDeleteWithUnitOfWorkAsync(repository, entities, autoSave, cancellationToken, uowManager.Current);
    }
}

I haven't done this so far because my concern is that you can write the following without compile errors: myRepo.HardDeleteAsync(myRepo.Where(...)) (because IQueryable can be casted to IEnumerable) when it should be written myRepo.HardDeleteAsync(await AsyncExecuter.ToListAsync(myRepo.Where(...)))

@maliming
Copy link
Member

hi

I think developers need to pay attention to this, The same thing exists in our Repository.

You can add HardDeleteAsync(IEnumerable<TEntity>...) method.

@olicooper olicooper marked this pull request as ready for review January 28, 2021 11:51
@stale stale bot added the inactive label Mar 29, 2021
@abpframework abpframework deleted a comment from stale bot Mar 30, 2021
@stale stale bot removed the inactive label Mar 30, 2021
@maliming maliming added this to the 4.4-preview milestone Mar 30, 2021
@maliming maliming self-assigned this Mar 30, 2021
@hikalkan hikalkan modified the milestones: 4.4-preview, 5.0-preview Jun 14, 2021
@maliming maliming merged commit db041f5 into abpframework:dev Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants