ardalis / Specification

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

Multiple repository calls to ToListAsync with the same spec causes caching on included entities #302

Closed chrislomax83 closed 1 year ago

chrislomax83 commented 1 year ago

I have a specification, which is below

public GiftCardServiceActiveProductWithOptionsByProduct(Guid productId, decimal denomination, bool activeOnly = true)
        {
            var costRelativeFilter = new[]
                { CostRelativeSale.Below, CostRelativeSale.Equal };

            Query
                .Where(e => e.ProductId.Equals(productId))
                .Include(e => e.GiftCardServiceAccount)
                .Include(e => e.Options.Where(
                    s => s.IsActive == activeOnly && s.CostRelativeToSalePrice != null
                        && costRelativeFilter.Contains(s.CostRelativeToSalePrice.Value)
                        && ((s.ValueType == ProductOptionType.Fixed && s.Denomination == denomination) ||
                                (s.ValueType == ProductOptionType.Range && s.MinRange <= denomination && s.MaxRange >= denomination))

                    ));

        }

I have a service which is injected with the repository and a method which is called which gets all available denominations for a product.

I had an issue which was returning the incorrect denomination option for a product as it was returning 2 options.

All my tests were passing for this method when I ran individual calls to the service and instantiating it every time.

The problem was, when this order service was injected into the service I was using and I re-use the service then it was returning all the options for the product if the previous iteration had already made a call.

The call to the repository is

var availableProductOptions =
            await _giftCardServiceProductRepository.ListAsync(
                new GiftCardServiceActiveProductWithOptionsByProduct(productId, denomination));

After investigation, this call works

[Fact]
        public async Task Get_Active_Options_For_Product_And_Denomination()
        {
            using var scope = _factory.Services.CreateScope();
            var service = scope.ServiceProvider.GetService<GiftCardProductService>();

            var productId = new Guid("12bff6e0-7574-11ed-9f09-31b61f7a6390");
            var denomination = 10.000000000000000000000000000M;

            var options = await service.GetOptionsForProductAndDenomination(productId, denomination);

            Assert.True(options.Count == 1);
        }

This call does not

[Fact]
        public async Task Get_Active_Options_For_Product_And_Denomination()
        {
            using var scope = _factory.Services.CreateScope();
            var service = scope.ServiceProvider.GetService<GiftCardProductService>();

            var productId = new Guid("12bff6e0-7574-11ed-9f09-31b61f7a6390");
            var denomination = 10.000000000000000000000000000M;

            var options = await service.GetOptionsForProductAndDenomination(productId, denomination);

            Assert.True(options.Count == 1);

            denomination = 25.000000000000000000000000000M;
            options = await service.GetOptionsForProductAndDenomination(productId, denomination);

            Assert.True(options.Count == 1);
        }

Calling the method without making the previous call works.

I've tried replicating this with another service where I don't use an Include Where clause and it works fine so I can only pinpoint it to that call.

I've debugged the actual SQL call it's making and it's only making one call to the db with the correct parameters passed in but then the SpecificationEvaluator for that spec always returns both options.

My ListAsync call

public async Task<IReadOnlyList<T>> ListAsync(ISpecification<T> spec, CancellationToken cancellationToken = default)
        {
            var specificationResult = ApplySpecification(spec);
            return await specificationResult.ToListAsync(cancellationToken);
        }

My ApplySpecification call

private IQueryable<T> ApplySpecification(ISpecification<T> spec)
        {
            return SpecificationEvaluator.Default.GetQuery(_dbContext.Set<T>().AsQueryable(), spec);
        }

Is this an issue in my misunderstanding? I've tried to look through the source for the code but I don't fully understand what it's doing so I don't know if it's an issue in my lack of understanding of how it works.

Packages: Ardalis.Specification 6.1.0 Ardalis.Specification.EntityFrameworkCore 6.1.0

If it makes a difference, I'm using Pomelo.EntityFrameworkCore.MySql 6.0.2 as the db driver

MWA-NET commented 1 year ago

I have the same problem

ardalis commented 1 year ago

Sounds like a bug; I assume if you were to do all the same logic using EF and LINQ directly it would work as expected. @fiseni and I will try to track it down.

fiseni commented 1 year ago

Hi @chrislomax83, @MWA-NET, @ardalis

First, that's quite a complex filtered include :) This smells like a closure problem. There is a lot of caching going on EF's side, and filtered includes are quite nasty to implement. So, I suspect the parameter is captured somewhere. Let's be systematic and troubleshoot one problem at a time. First, try to implement the same thing without specs. Work directly with dbContext and check whether you face the same issue.

Few suggestions that have nothing to do with this issue per se.

fiseni commented 1 year ago

I'm re-reading your post once again more carefully and trying to figure out your snippets. I think I have an idea of what's going on. Your GiftCardProductService is registered as scoped, right? You make a call to the database, EF loads the entities in the tracker. Then you make another call, EF loads them again in the tracker. But, it's the same DbContext instance. Everything that you have fetched up to that point is in the tracker. This is a common pitfall, and I have seen many bugs across teams related to this. Let me make it clear with this sample

// Let's say customer with Id = 1 has two addresses, London and New York
var customer1 = await dbContext.Customers
    .Where(x=>x.Id ==1)
    .Include(x => x.Addresses.Where(x => x.City == "London"))
    .FirstOrDefaultAsync();

var customer2 = await dbContext.Customers
    .Where(x => x.Id == 1)
    .Include(x => x.Addresses.Where(x => x.City == "New York"))
    .FirstOrDefaultAsync();

// You expect this to be 1, but will be 2
var count1 = customer1.Addresses.Count;

// Same here, you expect this to be 1, but will be 2
var count2 = customer2.Addresses.Count;

This is not a bug, neither in EF nor in this library. It's by design. If you want to have a clean state on each call, either you work with a new DbContext each time, or you can add AsNoTracking(). That will force EF not to add the entities in the tracker. But, once again, I assume you're just trying to read, and you don't intent to mutate and save them back.

chrislomax83 commented 1 year ago

Hi @fiseni

Thanks for your insight, this makes sense in this context. I can't say I've ever noticed this before, mainly because I don't call methods like this normally iteratively with the same parent item. It makes perfect sense now.

I did end up rewriting the method to project into a ViewModel while waiting for a response on this as it made more sense to be a read-only model as I was populating another table with the results.

Sorry for wasting your time and thanks for the lesson!