Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -211,20 +211,21 @@ public function getList(array $params): array
$total = $this->countList($params);

$listQuery = $this->buildListQuery($params);
$this->applyListSelect($listQuery, $includeContent);
$this->applySort($listQuery, $params);
$listQuery->limit($limit, $offset);

/** @var list<msProduct> $products */
/** @var array<int|string, msProduct> $products */
$products = $this->modx->getCollection(msProduct::class, $listQuery) ?: [];
$productList = array_values($products);
$ids = $this->prefetchAndAttachProductData($productList);

$optionsByProduct = [];
if ($includeOptions && $products !== []) {
$ids = array_map(static fn (msProduct $p) => (int) $p->get('id'), array_values($products));
$optionsByProduct = $this->loadOptionsForProducts($ids);
}
$optionsByProduct = ($includeOptions && $ids !== [])
? $this->loadOptionsForProducts($ids)
: [];

$items = [];
foreach ($products as $product) {
foreach ($productList as $product) {
$productId = (int) $product->get('id');
$options = $includeOptions ? ($optionsByProduct[$productId] ?? []) : null;
$items[] = $this->formatProduct($product, $includeContent, $options);
Expand Down Expand Up @@ -269,7 +270,8 @@ private function resolveContext(array $params): string
private function countList(array $params): int
{
$countQuery = $this->buildListQuery($params);
$countQuery->select('COUNT(DISTINCT msProduct.id)');
// 1:1 join on Data — DISTINCT is unnecessary until many-joins are added.
$countQuery->select('COUNT(msProduct.id)');
if (!$countQuery->prepare() || !$countQuery->stmt->execute()) {
return 0;
}
Expand Down Expand Up @@ -307,6 +309,60 @@ private function buildListQuery(array $params): xPDOQuery
return $c;
}

/**
* Limit selected resource columns; skip content blob on PLP when not requested.
* Same pattern as ms3_products snippet.
*/
private function applyListSelect(xPDOQuery $query, bool $includeContent): void
{
$query->select(
$includeContent
? $this->modx->getSelectColumns(msProduct::class, 'msProduct')
: $this->modx->getSelectColumns(msProduct::class, 'msProduct', '', ['content'], true)
);
}

/**
* Batch-load msProductData and attach via addOne so loadData() skips getOne N+1.
*
* List query already JOINs Data for WHERE/ORDER only (no related hydrate from that JOIN).
* One IN-query here is O(1) vs L× getOne; total SQL ≈ count + list + data (+ options).
*
* @param list<msProduct> $products
* @return list<int>
*/
private function prefetchAndAttachProductData(array $products): array
{
$byId = [];
foreach ($products as $product) {
$id = (int) $product->get('id');
if ($id > 0) {
$byId[$id] = $product;
}
}

if ($byId === []) {
return [];
}

$ids = array_keys($byId);
$c = $this->modx->newQuery(msProductData::class);
$c->where(['id:IN' => $ids]);

/** @var msProductData $data */
foreach ($this->modx->getCollection(msProductData::class, $c) ?: [] as $data) {
$id = (int) $data->get('id');
if (!isset($byId[$id])) {
continue;
}
// addOne requires a by-ref argument (xPDO signature).
$attached = $data;
$byId[$id]->addOne($attached, 'Data');
}

return $ids;
}

/**
* @param array<string, mixed> $params
*/
Expand Down
57 changes: 57 additions & 0 deletions core/components/minishop3/tests/ProductCatalogPerfTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

/**
* #575: ProductCatalogService list avoids N+1 Data and useless COUNT DISTINCT.
*
* Run: php tests/ProductCatalogPerfTest.php
*/

declare(strict_types=1);

$fail = static function (string $message): never {
fwrite(STDERR, "FAIL: {$message}\n");
exit(1);
};

$src = file_get_contents(__DIR__ . '/../src/Services/Product/ProductCatalogService.php');
$modelSrc = file_get_contents(__DIR__ . '/../src/Model/msProduct.php');
if ($src === false || $modelSrc === false) {
$fail('unable to read source files');
}

if (str_contains($modelSrc, 'function attachData')) {
$fail('do not add attachData on msProduct — use addOne($data, \'Data\')');
}

if (!str_contains($src, "addOne(\$attached, 'Data')") && !str_contains($src, 'addOne($attached, "Data")')) {
$fail('prefetch must attach Data via addOne(..., Data)');
}

if (!str_contains($src, "where(['id:IN'")) {
$fail('Data prefetch must use id:IN batch query');
}

if (str_contains($src, 'COUNT(DISTINCT msProduct.id)')) {
$fail('countList must not use COUNT(DISTINCT) for 1:1 Data join');
}

if (!str_contains($src, "select('COUNT(msProduct.id)')")) {
$fail('countList must COUNT(msProduct.id)');
}

if (!preg_match(
"/getSelectColumns\(\s*msProduct::class\s*,\s*'msProduct'\s*,\s*''\s*,\s*\['content'\]\s*,\s*true\s*\)/",
$src
)) {
$fail('include_content=0 must exclude content via getSelectColumns(..., [content], true)');
}

// Prefetch must run on getList path before format (batch, not per-row getOne).
if (!preg_match('/function getList[\s\S]*?id:IN[\s\S]*?formatProduct/m', $src)
&& !preg_match('/function getList[\s\S]*?prefetchAndAttachProductData[\s\S]*?formatProduct/m', $src)
) {
$fail('getList must batch-load Data before formatProduct');
}

fwrite(STDOUT, "OK ProductCatalogPerfTest\n");
exit(0);