ardalis / Specification

Base class with tests for adding specifications to a DDD model
MIT License
1.95k stars 245 forks source link

How to use ExecuteUpdateAsync? #393

Closed aminsharifi closed 7 months ago

aminsharifi commented 7 months ago

I want to implement the below expression =>

       _ = await _db.L_UserConditions
           .Where(x => _FilterdRecords.Contains(x.ERPCode))
           .ExecuteUpdateAsync(setters => setters.SetProperty(field => field.EnableRecord, false));

But I was forced to implement like this =>

GetEntityLinkByERPCodeSpec<L_UserCondition> _getEntityLinkByERPCodeSpec = new(_FilterdRecords);

  var _AllUserConditions = await userConditionRepository.ListAsync(_getEntityLinkByERPCodeSpec);

  Parallel.ForEach(_AllUserConditions, _AllUserCondition =>
  {
      _AllUserCondition.EnableRecord = false;
  });

  await userConditionRepository.UpdateRangeAsync(_AllUserConditions);

Is it a better approach to implement it?

fiseni commented 7 months ago

Hi @aminsharifi,

If you want to utilize the ExecuteUpdateAsync, then you might do the following

await _db.L_UserConditions
    .WithSpecification(_getEntityLinkByERPCodeSpec )
    .ExecuteUpdateAsync(setters => setters.SetProperty(field => field.EnableRecord, false));

The specifications are not bound exclusively to repositories, you may also use them with DbContext directly. If you want to use repositories, then I'd expose ExecuteUpdateAsync as a repository method. It seems you're implementing a sort of soft delete. Perhaps you can have more specific repo methods, e.g. ApplySoftDelete or whatever.

PS. In any case, do not use Parellel.ForEach. In your example, you already have the list in memory, simply iterate and set the flag. The Parallel.ForEach is an overkill here, it does more harm than good in this case.

aminsharifi commented 7 months ago

Hi @aminsharifi,

If you want to utilize the ExecuteUpdateAsync, then you might do the following

await _db.L_UserConditions
    .WithSpecification(_getEntityLinkByERPCodeSpec )
    .ExecuteUpdateAsync(setters => setters.SetProperty(field => field.EnableRecord, false));

The specifications are not bound exclusively to repositories, you may also use them with DbContext directly. If you want to use repositories, then I'd expose ExecuteUpdateAsync as a repository method. It seems you're implementing a sort of soft delete. Perhaps you can have more specific repo methods, e.g. ApplySoftDelete or whatever.

PS. In any case, do not use Parellel.ForEach. In your example, you already have the list in memory, simply iterate and set the flag. The Parallel.ForEach is an overkill here, it does more harm than good in this case.

Hi @fiseni Thanks for your reply. I know it is possible to write ExecuteUpdateAsync in the Infra layer but I want to use it in the use case layer via @ardalis spec Is there any plan to implement this method to the @ardalis spec methods?

fiseni commented 7 months ago

No, we do not plan to do that. The ExecuteUpdateAsync is a termination method, the same as ToListAsync, FirstOrDefaultAsync, etc. These are the methods where EF evaluates the IQueryable and translates/generates SQL statements. That's why I suggested adding it as a repo method.

If your intention is somehow to keep the expression in the spec, that is possible. We've added an arbitrary dictionary called Items in the spec. You can use that one to store any arbitrary state, and then utilize it from your repositories (or any other infrastructure you may have). You can find an example of that here in our sample projects.

aminsharifi commented 7 months ago

No, we do not plan to do that. The ExecuteUpdateAsync is a termination method, the same as ToListAsync, FirstOrDefaultAsync, etc. These are the methods where EF evaluates the IQueryable and translates/generates SQL statements. That's why I suggested adding it as a repo method.

If your intention is somehow to keep the expression in the spec, that is possible. We've added an arbitrary dictionary called Items in the spec. You can use that one to store any arbitrary state, and then utilize it from your repositories (or any other infrastructure you may have). You can find an example of that here in our sample projects.

Thanks again for your response I don't expect to implement it in the spec! I request to implement it in the package's IRepository. Currently we have other termination method like FirstOrDefaultAsync in this package's interface https://github.com/aminsharifi/Cheetah/blob/master/src%2FCheetah.Application.Business%2FCase%2FCase%2FGet%2FCopyCaseHandler.cs

fiseni commented 7 months ago

Oh OK, I do understand. We're trying to keep the repositories slim. The focus of this library is not the repository at all. We're providing the base repositories only as a sample or a starting point. Then, consumers can add their desired behavior on top of it.

Also, the ExecuteUpdate and ExecuteDelete are way too specific methods, heavily depending on the EF infrastructure. For instance, it accepts an argument that depends on SetPropertyCalls. There is no good way to abstract that (and you shouldn't). So, in order to add that to IRepository, we should reference EF from our base package, and that defies the whole purpose.

Expression<Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>>> setPropertyCalls

The specification library is designed to work with your domain model (preferably aggregates). It's not a replacement for your ORM. The whole point of ExecuteUpdate is to bypass your domain model. I hope you get my point.

fiseni commented 7 months ago

Hey @aminsharifi, Do you have any other comments on this issue? I'm not sure if we can include this feature in the library. What would be your desired API on this? Can you post a hypothetical sample code?

ardalis commented 7 months ago

Just to add to this:

We're trying to keep the repositories slim.

We've mostly failed to do this so far, I think, which is totally my fault. Because we have, in the past, accepted "just one more" feature/method for the repository. Honestly we probably need to make a breaking change and reset things to something more opinionated that would work properly. That, and/or I need to separately ship an Ardalis.Repository package. Because the current "sample" one in this package is very bloated, doesn't follow ISP, and has silly things like exposing SaveChanges and automatically performing SaveChanges within certain methods, which is redundant and not what most developers would likely expect.

aminsharifi commented 7 months ago

I would like to express my gratitude to @fiseni and @ardalis for their valuable contribution. I agree that it would be beneficial to separate Ardalis.Repository from Ardalis.Specification package. For the issues regarding automatic SaveChanges and Bulk operations, it would be great if features like EntityFramework-Plus or similar ones could be added to the new Ardalis.Repository package. Thank you both for your efforts.