Skip to content

MC-23881: When category URL page param is larger than the pagination, the page will break with elastic search 2 #31594

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 13, 2021
Merged

MC-23881: When category URL page param is larger than the pagination, the page will break with elastic search 2 #31594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions app/code/Magento/Elasticsearch/Elasticsearch5/SearchAdapter/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
*/
class Builder
{
private const ELASTIC_INT_MAX = 2147483647;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const ELASTIC_INT_MAX = 2147483647;
private const ELASTIC_MAX_INT = 2147483647;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that max_int sounds better but there is defined PHP_INT_MAX constant. I think it is better to keep the same format here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that make sense


/**
* @var Config
* @since 100.2.2
Expand Down Expand Up @@ -83,18 +85,18 @@ public function initQuery(RequestInterface $request)
{
$dimension = current($request->getDimensions());
$storeId = $this->scopeResolver->getScope($dimension->getValue())->getId();

$searchQuery = [
'index' => $this->searchIndexNameResolver->getIndexName($storeId, $request->getIndex()),
'type' => $this->clientConfig->getEntityType(),
'body' => [
'from' => $request->getFrom(),
'from' => min(self::ELASTIC_INT_MAX, $request->getFrom()),
'size' => $request->getSize(),
'stored_fields' => ['_id', '_score'],
'sort' => $this->sortBuilder->getSort($request),
'query' => [],
],
];

return $searchQuery;
}

Expand Down
4 changes: 3 additions & 1 deletion app/code/Magento/Elasticsearch/SearchAdapter/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
*/
class Builder extends Elasticsearch5Builder
{
private const ELASTIC_INT_MAX = 2147483647;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const ELASTIC_INT_MAX = 2147483647;
private const ELASTIC_MAX_INT = 2147483647;


/**
* @var Sort
*/
Expand Down Expand Up @@ -61,7 +63,7 @@ public function initQuery(RequestInterface $request)
'index' => $this->searchIndexNameResolver->getIndexName($storeId, $request->getIndex()),
'type' => $this->clientConfig->getEntityType(),
'body' => [
'from' => $request->getFrom(),
'from' => min(self::ELASTIC_INT_MAX, $request->getFrom()),
'size' => $request->getSize(),
'fields' => ['_id', '_score'],
'sort' => $this->sortBuilder->getSort($request),
Expand Down
189 changes: 189 additions & 0 deletions app/code/Magento/Elasticsearch/Test/Unit/Elasticsearch5/SearchAdapter/Query/BuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
<?php

/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Elasticsearch\Test\Unit\Elasticsearch5\SearchAdapter\Query;

use Magento\Elasticsearch\Elasticsearch5\SearchAdapter\Query\Builder;
use Magento\Elasticsearch\Model\Config;
use Magento\Elasticsearch\SearchAdapter\Query\Builder\Aggregation as AggregationBuilder;
use Magento\Elasticsearch\SearchAdapter\SearchIndexNameResolver;
use Magento\Framework\App\ScopeInterface;
use Magento\Framework\App\ScopeResolverInterface;
use Magento\Framework\Search\Request\Dimension;
use Magento\Framework\Search\RequestInterface;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

/**
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class BuilderTest extends TestCase
{
/**
* @var Builder
*/
protected $model;

/**
* @var Config|MockObject
*/
protected $clientConfig;

/**
* @var SearchIndexNameResolver|MockObject
*/
protected $searchIndexNameResolver;

/**
* @var AggregationBuilder|MockObject
*/
protected $aggregationBuilder;

/**
* @var RequestInterface|MockObject
*/
protected $request;

/**
* @var ScopeResolverInterface|MockObject
*/
protected $scopeResolver;

/**
* @var ScopeInterface|MockObject
*/
protected $scopeInterface;

/**
* Setup method
*
* @return void
*/
protected function setUp(): void
{
$this->clientConfig = $this->getMockBuilder(Config::class)
->onlyMethods(['getEntityType'])
->disableOriginalConstructor()
->getMock();
$this->searchIndexNameResolver = $this
->getMockBuilder(SearchIndexNameResolver::class)
->onlyMethods(['getIndexName'])
->disableOriginalConstructor()
->getMock();
$this->aggregationBuilder = $this
->getMockBuilder(\Magento\Elasticsearch\SearchAdapter\Query\Builder\Aggregation::class)
->onlyMethods(['build'])
->disableOriginalConstructor()
->getMock();
$this->request = $this->getMockBuilder(RequestInterface::class)
->disableOriginalConstructor()
->getMockForAbstractClass();
$this->scopeResolver = $this->getMockForAbstractClass(
ScopeResolverInterface::class,
[],
'',
false
);
$this->scopeInterface = $this->getMockForAbstractClass(
ScopeInterface::class,
[],
'',
false
);

$this->model = new Builder(
$this->clientConfig,
$this->searchIndexNameResolver,
$this->aggregationBuilder,
$this->scopeResolver,
);
}

/**
* Test initQuery() method
*/
public function testInitQuery()
{
$dimensionValue = 1;
$dimension = $this->getMockBuilder(Dimension::class)
->onlyMethods(['getValue'])
->disableOriginalConstructor()
->getMock();

$this->request->expects($this->once())
->method('getDimensions')
->willReturn([$dimension]);
$dimension->expects($this->once())
->method('getValue')
->willReturn($dimensionValue);
$this->scopeResolver->expects($this->once())
->method('getScope')
->willReturn($this->scopeInterface);
$this->scopeInterface->expects($this->once())
->method('getId')
->willReturn($dimensionValue);
$this->request->expects($this->once())
->method('getFrom')
->willReturn(0);
$this->request->expects($this->once())
->method('getSize')
->willReturn(10);
$this->request->expects($this->once())
->method('getIndex')
->willReturn('catalogsearch_fulltext');
$this->searchIndexNameResolver->expects($this->once())
->method('getIndexName')
->willReturn('indexName');
$this->clientConfig->expects($this->once())
->method('getEntityType')
->willReturn('document');
$this->model->initQuery($this->request);
}

/**
* Test initQuery() method with update from value
*/
public function testInitQueryLimitFrom()
{
$dimensionValue = 1;
$dimension = $this->getMockBuilder(Dimension::class)
->onlyMethods(['getValue'])
->disableOriginalConstructor()
->getMock();

$this->request->expects($this->once())
->method('getDimensions')
->willReturn([$dimension]);
$dimension->expects($this->once())
->method('getValue')
->willReturn($dimensionValue);
$this->scopeResolver->expects($this->once())
->method('getScope')
->willReturn($this->scopeInterface);
$this->scopeInterface->expects($this->once())
->method('getId')
->willReturn($dimensionValue);
$this->request->expects($this->once())
->method('getFrom')
->willReturn(PHP_INT_MAX);
$this->request->expects($this->once())
->method('getSize')
->willReturn(10);
$this->request->expects($this->once())
->method('getIndex')
->willReturn('catalogsearch_fulltext');
$this->searchIndexNameResolver->expects($this->once())
->method('getIndexName')
->willReturn('indexName');
$this->clientConfig->expects($this->once())
->method('getEntityType')
->willReturn('document');
$query = $this->model->initQuery($this->request);
$this->assertLessThanOrEqual(2147483647, $query['body']['from']);
}
}
72 changes: 59 additions & 13 deletions app/code/Magento/Elasticsearch/Test/Unit/SearchAdapter/Query/BuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Magento\Elasticsearch\Model\Config;
use Magento\Elasticsearch\SearchAdapter\Query\Builder;
use Magento\Elasticsearch\SearchAdapter\Query\Builder\Aggregation as AggregationBuilder;
use Magento\Elasticsearch\SearchAdapter\Query\Builder\Sort;
use Magento\Elasticsearch\SearchAdapter\SearchIndexNameResolver;
use Magento\Framework\App\ScopeInterface;
use Magento\Framework\App\ScopeResolverInterface;
Expand Down Expand Up @@ -66,17 +67,17 @@ class BuilderTest extends TestCase
protected function setUp(): void
{
$this->clientConfig = $this->getMockBuilder(Config::class)
->setMethods(['getEntityType'])
->onlyMethods(['getEntityType'])
->disableOriginalConstructor()
->getMock();
$this->searchIndexNameResolver = $this
->getMockBuilder(SearchIndexNameResolver::class)
->setMethods(['getIndexName'])
->onlyMethods(['getIndexName'])
->disableOriginalConstructor()
->getMock();
$this->aggregationBuilder = $this
->getMockBuilder(\Magento\Elasticsearch\SearchAdapter\Query\Builder\Aggregation::class)
->setMethods(['build'])
->onlyMethods(['build'])
->disableOriginalConstructor()
->getMock();
$this->request = $this->getMockBuilder(RequestInterface::class)
Expand All @@ -94,16 +95,19 @@ protected function setUp(): void
'',
false
);
$sortBuilder = $this->getMockForAbstractClass(
Sort::class,
[],
'',
false
);

$objectManagerHelper = new ObjectManagerHelper($this);
$this->model = $objectManagerHelper->getObject(
Builder::class,
[
'clientConfig' => $this->clientConfig,
'searchIndexNameResolver' => $this->searchIndexNameResolver,
'aggregationBuilder' => $this->aggregationBuilder,
'scopeResolver' => $this->scopeResolver
]
$this->model = new Builder(
$this->clientConfig,
$this->searchIndexNameResolver,
$this->aggregationBuilder,
$this->scopeResolver,
$sortBuilder,
);
}

Expand All @@ -114,7 +118,7 @@ public function testInitQuery()
{
$dimensionValue = 1;
$dimension = $this->getMockBuilder(Dimension::class)
->setMethods(['getValue'])
->onlyMethods(['getValue'])
->disableOriginalConstructor()
->getMock();

Expand Down Expand Up @@ -148,6 +152,48 @@ public function testInitQuery()
$this->model->initQuery($this->request);
}

/**
* Test initQuery() method with update from value
*/
public function testInitQueryLimitFrom()
{
$dimensionValue = 1;
$dimension = $this->getMockBuilder(Dimension::class)
->onlyMethods(['getValue'])
->disableOriginalConstructor()
->getMock();

$this->request->expects($this->once())
->method('getDimensions')
->willReturn([$dimension]);
$dimension->expects($this->once())
->method('getValue')
->willReturn($dimensionValue);
$this->scopeResolver->expects($this->once())
->method('getScope')
->willReturn($this->scopeInterface);
$this->scopeInterface->expects($this->once())
->method('getId')
->willReturn($dimensionValue);
$this->request->expects($this->once())
->method('getFrom')
->willReturn(PHP_INT_MAX);
$this->request->expects($this->once())
->method('getSize')
->willReturn(10);
$this->request->expects($this->once())
->method('getIndex')
->willReturn('catalogsearch_fulltext');
$this->searchIndexNameResolver->expects($this->once())
->method('getIndexName')
->willReturn('indexName');
$this->clientConfig->expects($this->once())
->method('getEntityType')
->willReturn('document');
$query = $this->model->initQuery($this->request);
$this->assertLessThanOrEqual(2147483647, $query['body']['from']);
}

/**
* Test initQuery() method
*/
Expand Down