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

Talpers/delete range by spec #369

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

thorstenalpers
Copy link
Contributor

New feature DeleteRange by specification

@fiseni
Copy link
Collaborator

fiseni commented Sep 5, 2023

Hi @thorstenalpers,

Thank you for your work. It looks great.
There is a slight issue in the integration tests (e.g. RepositoryOfT_DeleteRangeAsync). You correctly created a new collection WriteCollection, but all methods within that class will run against the same fixture and same database. So, if the first method removed that entity successfully, the rest of the methods actually won't do anything (there is no such record anyway), hence we have a false sense of security.

I'd suggest, deleting a different entity in each test (passing a different id). I think we're seeding many products, so just use 1,2,3, and so on.

Let me know if you face any issues.

@thorstenalpers
Copy link
Contributor Author

thorstenalpers commented Sep 7, 2023

Hi @fiseni, thanks for the help. Yes, I have an issue with EF6 and deleting entities using specs that use .AsNoTracking() to query entities. With EfCore the test with deletion was successful.

Failed Test: DeletesProductWithIdOne_GivenProductByIdAsUntrackedSpec()

 await repo.DeleteRangeAsync(new ProductByIdAsUntrackedSpec(4));

 //   System.InvalidOperationException : The object cannot be deleted because it was not found in the ObjectStateManager.
public class ProductByIdAsUntrackedSpec : Specification<Product>, ISingleResultSpecification
{
    public ProductByIdAsUntrackedSpec(int id)
    {
        Query.Where(product => product.Id == id).AsNoTracking();
    }
}

For me a deletion by a query as untracked makes no sense either. Should I remove these tests?

I have temporary removed them, what do you think about it?

@fiseni
Copy link
Collaborator

fiseni commented Sep 8, 2023

Hey @thorstenalpers,

It looks fine, thanks. It's a little bit dubious that we're not explicitly loading the entities (e.g. use ToListAsync, or LoadAsync), but we're passing an IQueryable to RemoveRange. So, we're relying completely on the internal mechanism of EF to load the entities (which may change). But, I think it's ok. Let's merge this.

var query = ApplySpecification(specification);
_dbContext.Set<T>().RemoveRange(query);

@fiseni fiseni merged commit a2698eb into ardalis:main Sep 8, 2023
1 check passed
@hgedik
Copy link

hgedik commented Oct 10, 2023

Could you please update Nuget as well? :)

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

3 participants