diff --git a/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs b/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs index a72285354..f3cf4da31 100644 --- a/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs +++ b/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs @@ -126,17 +126,32 @@ public virtual async Task> GetProductsByIds(string[] productIds, if (productIds == null || productIds.Length == 0) return new List(); - var products = new List(); - foreach (var id in productIds) - { - var product = await GetProductById(id); - if (product != null && (showHidden || (_aclService.Authorize(product, _contextAccessor.WorkContext.CurrentCustomer) && - _aclService.Authorize(product, _contextAccessor.StoreContext.CurrentStore.Id) && - product.IsAvailable()))) - products.Add(product); - } + //One query serves every identifier that is not cached yet, and the identifiers that are cached + //never reach it - so a warm call still costs nothing, and a cold one costs a single round trip + //instead of one per identifier. The lazy is what ties the misses together: the first of them + //starts the query, the rest await the same task. + var batch = new Lazy>>(() => GetProductsFromDb(productIds)); + + var found = await Task.WhenAll(productIds.Select(id => + _cacheBase.GetAsync(string.Format(CacheKey.PRODUCTS_BY_ID_KEY, id), + async () => (await batch.Value)[id].FirstOrDefault()))); + + return found.Where(product => + product != null && (showHidden || + (_aclService.Authorize(product, _contextAccessor.WorkContext.CurrentCustomer) && + _aclService.Authorize(product, _contextAccessor.StoreContext.CurrentStore.Id) && + product.IsAvailable()))) + .ToList(); + } - return products; + /// + /// Reads the given products in one go. A lookup rather than a dictionary because the caller may + /// repeat an identifier and because an identifier may match nothing. + /// + private Task> GetProductsFromDb(string[] productIds) + { + var products = _productRepository.Table.Where(product => productIds.Contains(product.Id)).ToList(); + return Task.FromResult(products.ToLookup(product => product.Id)); } /// diff --git a/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs b/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs new file mode 100644 index 000000000..32679c533 --- /dev/null +++ b/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs @@ -0,0 +1,111 @@ +using Grand.Business.Catalog.Services.Products; +using Grand.Business.Common.Services.Security; +using Grand.Data; +using Grand.Domain.Catalog; +using Grand.Domain.Customers; +using Grand.Domain.Stores; +using Grand.Infrastructure; +using Grand.Infrastructure.Caching; +using Grand.Infrastructure.Configuration; +using Grand.Infrastructure.Tests.Caching; +using Grand.Mediator; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Grand.Business.Catalog.Tests.Services.Products; + +/// +/// GetProductsByIds used to loop GetProductById, so a "batch" read cost one query per identifier. +/// These tests count reads at the repository, because the number of round trips is the whole point +/// of the change - asserting only on the returned products would pass either way. +/// +[TestClass] +public class ProductServiceBatchTests +{ + private MemoryCacheBase _cacheBase; + private ProductService _productService; + private Mock> _repository; + private int _tableReads; + + [TestInitialize] + public void InitializeTests() + { + var products = new List { + new() { Id = "1", Published = true, VisibleIndividually = true }, + new() { Id = "2", Published = true, VisibleIndividually = true }, + new() { Id = "3", Published = true, VisibleIndividually = true } + }; + + _tableReads = 0; + _repository = new Mock>(); + _repository.Setup(x => x.Table).Returns(() => + { + _tableReads++; + return products.AsQueryable(); + }); + + //a single customer and store: a fresh instance per access would give each call its own cache key + var customer = new Customer { Id = "customer" }; + var contextAccessor = new Mock(); + contextAccessor.Setup(c => c.StoreContext.CurrentStore).Returns(() => new Store { Id = "store" }); + contextAccessor.Setup(c => c.WorkContext.CurrentCustomer).Returns(() => customer); + var mediator = new Mock(); + _cacheBase = new MemoryCacheBase(MemoryCacheTest.Get(), mediator.Object, + new CacheConfig { DefaultCacheTimeMinutes = 1 }); + _productService = new ProductService(_cacheBase, _repository.Object, contextAccessor.Object, + mediator.Object, new AclService(new AccessControlConfig())); + } + + [TestMethod] + public async Task ColdCache_ReadsEveryProductInOneGo() + { + var result = await _productService.GetProductsByIds(["1", "2", "3"], true); + + Assert.HasCount(3, result); + Assert.AreEqual(1, _tableReads, "three identifiers must not cost three reads"); + _repository.Verify(x => x.GetByIdAsync(It.IsAny()), Times.Never); + } + + [TestMethod] + public async Task WarmCache_DoesNotReadAtAll() + { + await _productService.GetProductsByIds(["1", "2", "3"], true); + var reads = _tableReads; + + var result = await _productService.GetProductsByIds(["1", "2", "3"], true); + + Assert.HasCount(3, result); + Assert.AreEqual(reads, _tableReads, "everything was already cached"); + } + + [TestMethod] + public async Task KeepsTheOrderOfTheIdentifiersGiven() + { + var result = await _productService.GetProductsByIds(["3", "1", "2"], true); + + CollectionAssert.AreEqual(new[] { "3", "1", "2" }, result.Select(x => x.Id).ToArray()); + } + + [TestMethod] + public async Task SkipsAnIdentifierThatMatchesNothing() + { + var result = await _productService.GetProductsByIds(["1", "missing", "2"], true); + + CollectionAssert.AreEqual(new[] { "1", "2" }, result.Select(x => x.Id).ToArray()); + } + + [TestMethod] + public async Task RepeatsAProductWhoseIdentifierRepeats() + { + var result = await _productService.GetProductsByIds(["1", "1"], true); + + CollectionAssert.AreEqual(new[] { "1", "1" }, result.Select(x => x.Id).ToArray()); + } + + [TestMethod] + public async Task ReturnsNothingForAnEmptyRequest() + { + Assert.IsEmpty(await _productService.GetProductsByIds([], true)); + Assert.AreEqual(0, _tableReads); + } +}