-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed Issue #17295 : Search REST API returns wrong total_count #20253
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,16 @@ class Adapter implements AdapterInterface | |
*/ | ||
private $temporaryStorageFactory; | ||
|
||
/** | ||
* Query Select Parts to be skipped when prepare query for count | ||
* | ||
* @var array | ||
*/ | ||
protected $countSqlSkipParts = [ | ||
\Magento\Framework\DB\Select::LIMIT_COUNT => true, | ||
\Magento\Framework\DB\Select::LIMIT_OFFSET => true, | ||
]; | ||
|
||
/** | ||
* @param Mapper $mapper | ||
* @param ResponseFactory $responseFactory | ||
|
@@ -86,6 +96,7 @@ public function query(RequestInterface $request) | |
$response = [ | ||
'documents' => $documents, | ||
'aggregations' => $aggregations, | ||
'size' => $this->getSize($query) | ||
]; | ||
return $this->responseFactory->create($response); | ||
} | ||
|
@@ -114,4 +125,35 @@ private function getConnection() | |
{ | ||
return $this->resource->getConnection(); | ||
} | ||
|
||
/** | ||
* Get rows size | ||
* | ||
* @return int | ||
*/ | ||
public function getSize($query) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change method access modifier to private |
||
{ | ||
$sql = $this->getSelectCountSql($query); | ||
$parentSelect = $this->getConnection()->select(); | ||
$parentSelect->from(['core_select' => $sql]); | ||
$parentSelect->reset(\Magento\Framework\DB\Select::COLUMNS); | ||
$parentSelect->columns('COUNT(*)'); | ||
$totalRecords = $this->getConnection()->fetchOne($parentSelect); | ||
return (int)$totalRecords; | ||
} | ||
|
||
/** | ||
* Reset limit and offset | ||
* | ||
* @return Select | ||
*/ | ||
protected function getSelectCountSql($query) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change method access modifier to private |
||
{ | ||
foreach ($this->countSqlSkipParts as $part => $toSkip) { | ||
if ($toSkip) { | ||
$query->reset($part); | ||
} | ||
} | ||
return $query; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,14 +29,23 @@ class QueryResponse implements ResponseInterface | |
*/ | ||
protected $aggregations; | ||
|
||
/** | ||
* Total count of collection | ||
* | ||
* @var int | ||
*/ | ||
protected $totalCount; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change property access modifier to private |
||
|
||
/** | ||
* @param Document[] $documents | ||
* @param AggregationInterface $aggregations | ||
* @param Total size int $size | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please correct the phpdoc: |
||
*/ | ||
public function __construct(array $documents, AggregationInterface $aggregations) | ||
public function __construct(array $documents, AggregationInterface $aggregations, int $size = 0) | ||
{ | ||
$this->documents = $documents; | ||
$this->aggregations = $aggregations; | ||
$this->totalCount = $size; | ||
} | ||
|
||
/** | ||
|
@@ -65,4 +74,12 @@ public function getAggregations() | |
{ | ||
return $this->aggregations; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getTotalCount() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @buskamuza @melnikovi can you please take a look if we can allow adding this method to the framework. It appears there is no other way to return the total count of the search result. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class has count method already, that seem to have similar purpose. Are there cases when count that it uses is not efficient? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @melnikovi Count method returns a total count of the search result items of the current page. |
||
{ | ||
return $this->totalCount; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the access modifier to private