From a433a0ef6ffe5f7d91eb5478975dd09733fa34f8 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 13:06:18 +0200 Subject: [PATCH 1/2] perf(catalog): read products by identifier in one query GetProductsByIds looped GetProductById, so the batch method was itself the N+1: a cart, a related-products block or a recently-viewed list cost one round trip per identifier. It is called from personalised, recommended and suggested products, recently viewed, blog post products and two discount rules. The naive fix - one $in query - would have thrown away the per-identifier cache that the loop was at least benefiting from, and ICacheBase offers no way to ask whether a key is present without also supplying a value to store. A lazy shared task gets both: identifiers already cached are served from memory and never reach it, while the first identifier that misses starts a single query covering the request, and every other miss awaits that same task. Warm calls therefore cost nothing and cold calls cost one round trip instead of N. Behaviour is unchanged in the parts callers depend on, and the tests pin them: the order of the identifiers given is the order returned - recently viewed products rely on it - an identifier matching nothing is skipped, and a repeated identifier still yields the product twice. The tests count reads at the repository rather than asserting on the products, because the number of round trips is the point; on the previous implementation the count is zero, since it went through GetByIdAsync per identifier instead. Co-Authored-By: Claude Opus 5 --- .../Services/Products/ProductService.cs | 35 ++++-- .../Products/ProductServiceBatchTests.cs | 111 ++++++++++++++++++ 2 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs diff --git a/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs b/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs index a72285354f..f3cf4da311 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 0000000000..32679c5331 --- /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); + } +} From 97961f64eca4991ea7d8aba03dd5cad62b7f092d Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 13 Sep 2026 17:41:58 +0200 Subject: [PATCH 2/2] perf(catalog): query only uncached products in GetProductsByIds Replace the Lazy/Task.WhenAll construction with an explicit cache check: add ICacheBase.TryGetValue, read the misses in one async query and cache them (including not-found identifiers), then return in the given order. --- .../Services/Products/ProductService.cs | 57 ++++++------ .../Caching/ICacheBase.cs | 1 + .../Caching/MemoryCacheBase.cs | 5 ++ .../Products/ProductServiceBatchTests.cs | 87 ++++++++++++------- 4 files changed, 95 insertions(+), 55 deletions(-) diff --git a/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs b/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs index 57b0ca1b51..952c2a5fba 100644 --- a/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs +++ b/src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs @@ -126,32 +126,39 @@ public virtual async Task> GetProductsByIds(string[] productIds, if (productIds == null || productIds.Length == 0) return new List(); - //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(); - } + var products = new Dictionary(); + var missing = new List(); - /// - /// 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)); + foreach (var id in productIds.Distinct()) + { + if (_cacheBase.TryGetValue(string.Format(CacheKey.PRODUCTS_BY_ID_KEY, id), out Product cached)) + products[id] = cached; + else + missing.Add(id); + } + + if (missing.Count > 0) + { + var query = _productRepository.Table.Where(product => missing.Contains(product.Id)); + var fromDb = (await _productRepository.ToListAsync(query)).ToDictionary(product => product.Id); + + foreach (var id in missing) + { + //cache the miss as well, same as GetProductById does + fromDb.TryGetValue(id, out var product); + await _cacheBase.SetAsync(string.Format(CacheKey.PRODUCTS_BY_ID_KEY, id), () => Task.FromResult(product)); + products[id] = product; + } + } + + //keep the order of the identifiers given - recently viewed products rely on it + return productIds + .Select(id => products[id]) + .Where(product => product != null && (showHidden || + (_aclService.Authorize(product, _contextAccessor.WorkContext.CurrentCustomer) && + _aclService.Authorize(product, _contextAccessor.StoreContext.CurrentStore.Id) && + product.IsAvailable()))) + .ToList(); } /// diff --git a/src/Core/Grand.Infrastructure/Caching/ICacheBase.cs b/src/Core/Grand.Infrastructure/Caching/ICacheBase.cs index 068520fcea..35d819979c 100644 --- a/src/Core/Grand.Infrastructure/Caching/ICacheBase.cs +++ b/src/Core/Grand.Infrastructure/Caching/ICacheBase.cs @@ -5,6 +5,7 @@ namespace Grand.Infrastructure.Caching; /// public interface ICacheBase { + bool TryGetValue(string key, out T value); T Get(string key, Func acquire); T Get(string key, Func acquire, int cacheTime); Task GetAsync(string key, Func> acquire); diff --git a/src/Core/Grand.Infrastructure/Caching/MemoryCacheBase.cs b/src/Core/Grand.Infrastructure/Caching/MemoryCacheBase.cs index cf4e54c615..74b073a307 100644 --- a/src/Core/Grand.Infrastructure/Caching/MemoryCacheBase.cs +++ b/src/Core/Grand.Infrastructure/Caching/MemoryCacheBase.cs @@ -37,6 +37,11 @@ public MemoryCacheBase(IMemoryCache cache, IMediator mediator, CacheConfig cache #region Methods + public virtual bool TryGetValue(string key, out T value) + { + return _cache.TryGetValue(key, out value); + } + public virtual T Get(string key, Func acquire) { return Get(key, acquire, _cacheConfig.DefaultCacheTimeMinutes); diff --git a/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs b/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs index 32679c5331..f4fd7d4650 100644 --- a/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs +++ b/src/Tests/Grand.Business.Catalog.Tests/Services/Products/ProductServiceBatchTests.cs @@ -14,37 +14,33 @@ 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 readonly List _products = [ + new() { Id = "1", Published = true, VisibleIndividually = true }, + new() { Id = "2", Published = true, VisibleIndividually = true }, + new() { Id = "3", Published = true, VisibleIndividually = true } + ]; + private MemoryCacheBase _cacheBase; private ProductService _productService; - private Mock> _repository; - private int _tableReads; + private List _queries; [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 + _queries = []; + var repository = new Mock>(); + repository.Setup(x => x.Table).Returns(() => _products.AsQueryable()); + repository.Setup(x => x.ToListAsync(It.IsAny>(), It.IsAny())) + .ReturnsAsync((IQueryable query, CancellationToken _) => + { + var result = query.ToList(); + _queries.Add(result.Select(x => x.Id).ToArray()); + return result; + }); + var customer = new Customer { Id = "customer" }; var contextAccessor = new Mock(); contextAccessor.Setup(c => c.StoreContext.CurrentStore).Returns(() => new Store { Id = "store" }); @@ -52,30 +48,60 @@ public void InitializeTests() var mediator = new Mock(); _cacheBase = new MemoryCacheBase(MemoryCacheTest.Get(), mediator.Object, new CacheConfig { DefaultCacheTimeMinutes = 1 }); - _productService = new ProductService(_cacheBase, _repository.Object, contextAccessor.Object, + _productService = new ProductService(_cacheBase, repository.Object, contextAccessor.Object, mediator.Object, new AclService(new AccessControlConfig())); } [TestMethod] - public async Task ColdCache_ReadsEveryProductInOneGo() + public async Task ColdCache_ReadsEveryProductInOneQuery() { 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); + Assert.HasCount(1, _queries); } [TestMethod] - public async Task WarmCache_DoesNotReadAtAll() + public async Task WarmCache_DoesNotQuery() { 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"); + Assert.HasCount(1, _queries); + } + + [TestMethod] + public async Task PartlyWarmCache_QueriesOnlyTheMissingProducts() + { + await _productService.GetProductsByIds(["1"], true); + + var result = await _productService.GetProductsByIds(["1", "2", "3"], true); + + Assert.HasCount(3, result); + Assert.HasCount(2, _queries); + CollectionAssert.AreEquivalent(new[] { "2", "3" }, _queries[1]); + } + + [TestMethod] + public async Task MissingProduct_IsCachedAndNotQueriedAgain() + { + await _productService.GetProductsByIds(["missing"], true); + + var result = await _productService.GetProductsByIds(["missing"], true); + + Assert.IsEmpty(result); + Assert.HasCount(1, _queries); + } + + [TestMethod] + public async Task SharesCacheEntriesWithGetProductById() + { + await _productService.GetProductsByIds(["1"], true); + _products.Clear(); + + Assert.IsNotNull(await _productService.GetProductById("1")); } [TestMethod] @@ -100,12 +126,13 @@ public async Task RepeatsAProductWhoseIdentifierRepeats() var result = await _productService.GetProductsByIds(["1", "1"], true); CollectionAssert.AreEqual(new[] { "1", "1" }, result.Select(x => x.Id).ToArray()); + Assert.HasCount(1, _queries); } [TestMethod] public async Task ReturnsNothingForAnEmptyRequest() { Assert.IsEmpty(await _productService.GetProductsByIds([], true)); - Assert.AreEqual(0, _tableReads); + Assert.IsEmpty(_queries); } }