From ee461c7cfc5929375321407990b6d392d417852e Mon Sep 17 00:00:00 2001 From: Alastair Mucklow Date: Mon, 18 Nov 2024 14:59:01 +0000 Subject: [PATCH] Merged PR 60009: Load products in bulk to reduce expensive queries ## What's being changed This PR further reduces SQL queries by loading products in bulk wherever possible. Products are cached by store id because, for brand values: - attributes can be global, website or store scoped - for store-scoped attributes, options can change between store scopes, as can the values of those options Example: - The Dotdigital Brand Attribute is set to 'Manufacturer' - Products can in theory have a different Manufacturer per store - The options for Manufacturer may be presented as 'Company A, 'Company B' and 'Company C', but in the attribute configuration the actual option text for each can vary (for example 'Le Company A' etc for a French store view). ## Why it's being changed Individual orders may have many associated products. All these need loading in order to arrive at a string of category ids for FIRST_CATEGORY_PUR and LAST_CATEGORY_PUR. Each time we replace e.g. 10 product entity loads with a single query, there is a reduction in resource utilisation. ## How to review / test this change - Compare the JSON exported from develop with the JSON exported on this branch - See no change - Further test steps specific to brand values in different scopes to follow in the next PR. ## Notes We can go further and restrict the attributes loaded in the collection query in the ProductLoader. This will follow in a separate PR. Related work items: #278457 --- Model/Connector/ContactData.php | 19 ++-- Model/Connector/ContactData/ProductLoader.php | 96 ++++++++++++++----- .../ContactData/ProductLoaderTest.php | 65 ++++++++----- 3 files changed, 121 insertions(+), 59 deletions(-) diff --git a/Model/Connector/ContactData.php b/Model/Connector/ContactData.php index 87a487e2..f483e5e8 100644 --- a/Model/Connector/ContactData.php +++ b/Model/Connector/ContactData.php @@ -503,8 +503,8 @@ public function getMostPurBrand() //if the id and attribute found if ($productId && $attributeCode) { - $product = $this->productLoader->getProduct((int) $productId, (int) $this->model->getStoreId()); - if ($product->getId()) { + $product = $this->productLoader->getCachedProductById((int) $productId, (int) $this->model->getStoreId()); + if ($product && $product->getId()) { $attribute = $this->productResource->getAttribute($attributeCode); $value = is_object($attribute) ? $attribute->getFrontend()->getValue($product) : null; if ($value) { @@ -552,9 +552,9 @@ public function getMostPurCategory() $productId = $this->model->getProductIdForMostSoldProduct(); //sales data found for customer with product id if ($productId) { - $product = $this->productLoader->getProduct((int) $productId, (int) $this->model->getStoreId()); + $product = $this->productLoader->getCachedProductById((int) $productId, (int) $this->model->getStoreId()); //product found - if ($product->getId()) { + if ($product && $product->getId()) { $categoryIds = $product->getCategoryIds(); if (count($categoryIds)) { $categories = $this->getCategoryNamesFromIds($categoryIds); @@ -658,10 +658,10 @@ private function getBrandAttributeValue($id, $attributeCode, $storeId) { //if the id and attribute found if ($id && $attributeCode) { - $product = $this->productLoader->getProduct((int) $id, (int) $storeId); + $product = $this->productLoader->getCachedProductById((int) $id, (int) $storeId); $attribute = $this->productResource->getAttribute($attributeCode); - if ($attribute && $product->getId()) { + if ($attribute && $product && $product->getId()) { $value = $attribute->setStoreId($storeId) ->getSource() ->getOptionText($product->getData($attributeCode)); @@ -751,9 +751,10 @@ public function getSubscriberStatusString($statusCode) private function getCategoriesFromProducts(array $productIds): array { $categoryIds = []; - foreach ($productIds as $productId) { - $product = $this->productLoader->getProduct((int) $productId, (int) $this->model->getStoreId()); - if ($product->getId()) { + $products = $this->productLoader->getCachedProducts($productIds, (int) $this->model->getStoreId()); + + foreach ($products as $product) { + if ($product && $product->getId()) { foreach ($product->getCategoryIds() as $categoryId) { if (!in_array($categoryId, $categoryIds)) { $categoryIds[] = $categoryId; diff --git a/Model/Connector/ContactData/ProductLoader.php b/Model/Connector/ContactData/ProductLoader.php index 75a583d9..a42b8d5d 100644 --- a/Model/Connector/ContactData/ProductLoader.php +++ b/Model/Connector/ContactData/ProductLoader.php @@ -4,21 +4,15 @@ namespace Dotdigitalgroup\Email\Model\Connector\ContactData; -use Magento\Catalog\Api\Data\ProductInterfaceFactory; use Magento\Catalog\Model\Product; -use Magento\Catalog\Model\ResourceModel\Product as ProductResource; +use Magento\Catalog\Model\ResourceModel\Product\CollectionFactory as CatalogCollectionFactory; class ProductLoader { /** - * @var ProductInterfaceFactory + * @var CatalogCollectionFactory */ - private $productFactory; - - /** - * @var ProductResource - */ - private $productResource; + private $catalogCollectionFactory; /** * @var array @@ -26,34 +20,86 @@ class ProductLoader private $products = []; /** - * @param ProductInterfaceFactory $productFactory - * @param ProductResource $productResource + * @param CatalogCollectionFactory $catalogCollectionFactory */ public function __construct( - ProductInterfaceFactory $productFactory, - ProductResource $productResource + CatalogCollectionFactory $catalogCollectionFactory ) { - $this->productFactory = $productFactory; - $this->productResource = $productResource; + $this->catalogCollectionFactory = $catalogCollectionFactory; } /** - * Load a product by id and store it to prevent repeat loading of the same entity. + * Get cached product by id. * * @param int $productId * @param int $storeId * - * @return Product + * @return Product|null + */ + public function getCachedProductById(int $productId, int $storeId) + { + if (!isset($this->products[$productId][$storeId])) { + $this->setProducts([$productId], $storeId); + } + + return $this->products[$productId][$storeId] ?? null; + } + + /** + * Get cached products. + * + * @param array $productIds + * @param int $storeId + * + * @return Product[] */ - public function getProduct(int $productId, int $storeId) + public function getCachedProducts(array $productIds, int $storeId) { - if (!isset($this->products[$productId])) { - /** @var Product $product */ - $product = $this->productFactory->create(); - $product->setStoreId($storeId); - $this->productResource->load($product, $productId); - $this->products[$productId] = $product; + $productIdsNotAlreadyCached = array_diff($productIds, array_keys($this->products)); + if (! empty($productIdsNotAlreadyCached)) { + $this->setProducts($productIdsNotAlreadyCached, $storeId); + } + + $productIdsNotAlreadyCachedForThisStore = array_filter($productIds, function ($productId) use ($storeId) { + return !isset($this->products[$productId][$storeId]); + }); + if (! empty($productIdsNotAlreadyCachedForThisStore)) { + $this->setProducts($productIdsNotAlreadyCachedForThisStore, $storeId); + } + + $productsToReturn = []; + foreach ($productIds as $productId) { + if (! isset($this->products[$productId][$storeId])) { + continue; + } + $productsToReturn[] = $this->products[$productId][$storeId]; + } + + return $productsToReturn; + } + + /** + * Set products. + * + * Load a product by id and store it to prevent repeat loading of the same entity. + * If we don't find a match for a product id, set it to null to prevent repeat attempts + * to load the missing product. + * + * @param array $productIds + * @param int $storeId + * + * @return void + */ + private function setProducts(array $productIds, int $storeId) + { + $productsCollection = $this->catalogCollectionFactory->create() + ->addStoreFilter($storeId) + ->addIdFilter($productIds) + ->addAttributeToSelect('*') + ->load(); + + foreach ($productIds as $productId) { + $this->products[$productId][$storeId] = $productsCollection->getItemById($productId) ?? null; } - return $this->products[$productId]; } } diff --git a/Test/Unit/Model/Connector/ContactData/ProductLoaderTest.php b/Test/Unit/Model/Connector/ContactData/ProductLoaderTest.php index 010d97a4..10a87cc6 100644 --- a/Test/Unit/Model/Connector/ContactData/ProductLoaderTest.php +++ b/Test/Unit/Model/Connector/ContactData/ProductLoaderTest.php @@ -4,9 +4,9 @@ namespace Dotdigitalgroup\Email\Test\Unit\Model\Connector\ContactData; -use Magento\Catalog\Api\Data\ProductInterfaceFactory; +use ArrayIterator; use Magento\Catalog\Model\Product; -use Magento\Catalog\Model\ResourceModel\Product as ProductResource; +use Magento\Catalog\Model\ResourceModel\Product\CollectionFactory as CatalogCollectionFactory; use Dotdigitalgroup\Email\Model\Connector\ContactData\ProductLoader; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -14,14 +14,9 @@ class ProductLoaderTest extends TestCase { /** - * @var ProductInterfaceFactory|MockObject + * @var CatalogCollectionFactory|MockObject */ - private $productFactoryMock; - - /** - * @var ProductResource|MockObject - */ - private $productResourceMock; + private $catalogCollectionFactoryMock; /** * @var ProductLoader @@ -30,20 +25,16 @@ class ProductLoaderTest extends TestCase protected function setUp(): void { - $this->productFactoryMock = $this->getMockBuilder(ProductInterfaceFactory::class) - ->disableOriginalConstructor() - ->getMock(); - $this->productResourceMock = $this->getMockBuilder(ProductResource::class) + $this->catalogCollectionFactoryMock = $this->getMockBuilder(CatalogCollectionFactory::class) ->disableOriginalConstructor() ->getMock(); $this->productLoader = new ProductLoader( - $this->productFactoryMock, - $this->productResourceMock + $this->catalogCollectionFactoryMock ); } - public function testGetProduct(): void + public function testGetCachedProductById(): void { $productId = 1; $storeId = 1; @@ -52,23 +43,47 @@ public function testGetProduct(): void ->disableOriginalConstructor() ->getMock(); - $this->productFactoryMock->expects($this->once()) + $collectionMock = $this->getMockBuilder(\Magento\Catalog\Model\ResourceModel\Product\Collection::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->catalogCollectionFactoryMock->expects($this->once()) ->method('create') - ->willReturn($productMock); + ->willReturn($collectionMock); + + $collectionMock->expects($this->once()) + ->method('addStoreFilter') + ->with($storeId) + ->willReturn($collectionMock); + + $collectionMock->expects($this->once()) + ->method('addIdFilter') + ->with([$productId]) + ->willReturn($collectionMock); - $productMock->expects($this->once()) - ->method('setStoreId') - ->with($storeId); + $collectionMock->expects($this->once()) + ->method('addAttributeToSelect') + ->with('*') + ->willReturn($collectionMock); - $this->productResourceMock->expects($this->once()) + $collectionMock->expects($this->once()) ->method('load') - ->with($productMock, $productId); + ->willReturn($collectionMock); + + $collectionMock->expects($this->any()) + ->method('getIterator') + ->willReturn(new ArrayIterator([$productMock])); + + $collectionMock->expects($this->once()) + ->method('getItemById') + ->with($productId) + ->willReturn($productMock); - $result = $this->productLoader->getProduct($productId, $storeId); + $result = $this->productLoader->getCachedProductById($productId, $storeId); $this->assertSame($productMock, $result); // Test caching of loaded product - $resultCached = $this->productLoader->getProduct($productId, $storeId); + $resultCached = $this->productLoader->getCachedProductById($productId, $storeId); $this->assertSame($productMock, $resultCached); } }