Read products by identifier in one query

src/Business/Grand.Business.Catalog/Services/Products/ProductService.cs

@@ -126,17 +126,32 @@ public virtual async Task<IList<Product>> GetProductsByIds(string[] productIds,
         if (productIds == null || productIds.Length == 0)
             return new List<Product>();
 
-        var products = new List<Product>();
-        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<Task<ILookup<string, Product>>>(() => 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;
+    /// <summary>
+    ///     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.
+    /// </summary>
+    private Task<ILookup<string, Product>> GetProductsFromDb(string[] productIds)
+    {
+        var products = _productRepository.Table.Where(product => productIds.Contains(product.Id)).ToList();
+        return Task.FromResult(products.ToLookup(product => product.Id));
     }
 
     /// <summary>

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;
+
+/// <summary>
+///     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.
+/// </summary>
+[TestClass]
+public class ProductServiceBatchTests
+{
+    private MemoryCacheBase _cacheBase;
+    private ProductService _productService;
+    private Mock<IRepository<Product>> _repository;
+    private int _tableReads;
+
+    [TestInitialize]
+    public void InitializeTests()
+    {
+        var products = new List<Product> {
+            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<IRepository<Product>>();
+        _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<IContextAccessor>();
+        contextAccessor.Setup(c => c.StoreContext.CurrentStore).Returns(() => new Store { Id = "store" });
+        contextAccessor.Setup(c => c.WorkContext.CurrentCustomer).Returns(() => customer);
+        var mediator = new Mock<IMediator>();
+        _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<string>()), 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);
+    }
+}