From a433a0ef6ffe5f7d91eb5478975dd09733fa34f8 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 13:06:18 +0200 Subject: [PATCH] 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 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); + } +}