Skip to content

Commit 343dc7d

Browse files
author
Oleksii Korshenko
committed
MAGETWO-55589: Wrong algorithm for calculation batch size on category indexing
 - covered changes with unit tests
1 parent 6e82b67 commit 343dc7d

File tree

3 files changed

+349
-5
lines changed

3 files changed

+349
-5
lines changed

‎lib/internal/Magento/Framework/DB/Query/BatchIterator.php

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ class BatchIterator implements \Iterator
5858
*/
5959
private $rangeFieldAlias;
6060

61+
/**
62+
* @var bool
63+
*/
64+
private $isValid = true;
65+
6166
/**
6267
* Initialize dependencies.
6368
*
@@ -87,6 +92,11 @@ public function __construct(
8792
*/
8893
public function current()
8994
{
95+
if (null == $this->currentSelect) {
96+
$this->currentSelect = $this->initSelectObject();
97+
$itemsCount = $this->calculateBatchSize($this->currentSelect);
98+
$this->isValid = $itemsCount > 0;
99+
}
90100
return $this->currentSelect;
91101
}
92102

@@ -95,7 +105,18 @@ public function current()
95105
*/
96106
public function next()
97107
{
98-
$this->iteration++;
108+
if (null == $this->currentSelect) {
109+
$this->current();
110+
}
111+
$select = $this->initSelectObject();
112+
$itemsCountInSelect = $this->calculateBatchSize($select);
113+
$this->isValid = $itemsCountInSelect > 0;
114+
if ($this->isValid) {
115+
$this->iteration++;
116+
$this->currentSelect = $select;
117+
} else {
118+
$this->currentSelect = null;
119+
}
99120
return $this->currentSelect;
100121
}
101122

@@ -112,9 +133,7 @@ public function key()
112133
*/
113134
public function valid()
114135
{
115-
$this->currentSelect = $this->initSelectObject();
116-
$batchSize = $this->calculateBatchSize($this->currentSelect);
117-
return $batchSize > 0;
136+
return $this->isValid;
118137
}
119138

120139
/**
@@ -123,7 +142,9 @@ public function valid()
123142
public function rewind()
124143
{
125144
$this->minValue = 0;
145+
$this->currentSelect = null;
126146
$this->iteration = 0;
147+
$this->isValid = true;
127148
}
128149

129150
/**
@@ -165,7 +186,7 @@ private function initSelectObject()
165186
/**
166187
* Reset sort order section from origin select object
167188
*/
168-
$object->order($this->correlationName . '.' . $this->rangeField . ' asc');
189+
$object->order($this->correlationName . '.' . $this->rangeField . ' ' . \Magento\Framework\DB\Select::SQL_ASC);
169190
return $object;
170191
}
171192
}

‎lib/internal/Magento/Framework/Test/DB/Query/BatchIteratorTest.php

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
<?php
2+
/**
3+
* Copyright © 2016 Magento. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
namespace Magento\Framework\Test\Unit\DB\Query;
7+
8+
use Magento\Framework\DB\Query\BatchIterator;
9+
use Magento\Framework\DB\Select;
10+
use Magento\Framework\DB\Adapter\AdapterInterface;
11+
12+
class BatchIteratorTest extends \PHPUnit_Framework_TestCase
13+
{
14+
/**
15+
* @var BatchIterator
16+
*/
17+
private $model;
18+
19+
/**
20+
* @var \PHPUnit_Framework_MockObject_MockObject
21+
*/
22+
private $selectMock;
23+
24+
/**
25+
* @var \PHPUnit_Framework_MockObject_MockObject
26+
*/
27+
private $wrapperSelectMock;
28+
29+
/**
30+
* @var \PHPUnit_Framework_MockObject_MockObject
31+
*/
32+
private $connectionMock;
33+
34+
private $batchSize;
35+
private $correlationName;
36+
private $rangeField;
37+
private $rangeFieldAlias;
38+
39+
protected function setUp()
40+
{
41+
$this->batchSize = 10;
42+
$this->correlationName = 'correlationName';
43+
$this->rangeField = 'rangeField';
44+
$this->rangeFieldAlias = 'rangeFieldAlias';
45+
46+
$this->selectMock = $this->getMock(Select::class, [], [], '', false, false);
47+
$this->wrapperSelectMock = $this->getMock(Select::class, [], [], '', false, false);
48+
$this->connectionMock = $this->getMock(AdapterInterface::class);
49+
$this->connectionMock->expects($this->any())->method('select')->willReturn($this->wrapperSelectMock);
50+
$this->selectMock->expects($this->once())->method('getConnection')->willReturn($this->connectionMock);
51+
$this->connectionMock->expects($this->any())->method('quoteIdentifier')->willReturnArgument(0);
52+
53+
$this->model = new BatchIterator(
54+
$this->selectMock,
55+
$this->batchSize,
56+
$this->correlationName,
57+
$this->rangeField,
58+
$this->rangeFieldAlias
59+
);
60+
}
61+
62+
/**
63+
* Test steps:
64+
* 1. $iterator->current();
65+
* 2. $iterator->current();
66+
* 3. $iterator->key();
67+
*/
68+
public function testCurrent()
69+
{
70+
$filed = $this->correlationName . '.' . $this->rangeField;
71+
72+
$this->selectMock->expects($this->once())->method('where')->with($filed . ' > ?', 0);
73+
$this->selectMock->expects($this->once())->method('limit')->with($this->batchSize);
74+
$this->selectMock->expects($this->once())->method('order')->with($filed . ' ASC');
75+
$this->assertEquals($this->selectMock, $this->model->current());
76+
$this->assertEquals($this->selectMock, $this->model->current());
77+
$this->assertEquals(0, $this->model->key());
78+
}
79+
80+
/**
81+
* SQL: select * from users
82+
* Batch size: 10
83+
* IDS: [1 - 25]
84+
* Items count: 25
85+
* Expected batches: [1-10, 11-20, 20-25]
86+
*
87+
* Test steps:
88+
* 1. $iterator->rewind();
89+
* 2. $iterator->valid();
90+
* 3. $iterator->current();
91+
* 4. $iterator->key();
92+
*
93+
* 1. $iterator->next()
94+
* 2. $iterator->valid();
95+
* 3. $iterator->current();
96+
* 4. $iterator->key();
97+
*
98+
* 1. $iterator->next()
99+
* 2. $iterator->valid();
100+
* 3. $iterator->current();
101+
* 4. $iterator->key();
102+
*
103+
*
104+
* 1. $iterator->next()
105+
* 2. $iterator->valid();
106+
*
107+
*/
108+
public function testIterations()
109+
{
110+
$startCallIndex = 3;
111+
$stepCall = 4;
112+
113+
$this->connectionMock->expects($this->at($startCallIndex))
114+
->method('fetchRow')
115+
->willReturn(['max' => 10, 'cnt' => 10]);
116+
117+
$this->connectionMock->expects($this->at($startCallIndex += $stepCall))
118+
->method('fetchRow')
119+
->willReturn(['max' => 20, 'cnt' => 10]);
120+
121+
$this->connectionMock->expects($this->at($startCallIndex += $stepCall))
122+
->method('fetchRow')
123+
->willReturn(['max' => 25, 'cnt' => 5]);
124+
125+
$this->connectionMock->expects($this->at($startCallIndex += $stepCall))
126+
->method('fetchRow')
127+
->willReturn(['max' => null, 'cnt' => 0]);
128+
129+
/**
130+
* Test 3 iterations
131+
* [1-10, 11-20, 20-25]
132+
*/
133+
$iteration = 0;
134+
$result = [];
135+
foreach ($this->model as $key => $select) {
136+
$result[] = $select;
137+
$this->assertEquals($iteration, $key);
138+
$iteration++;
139+
}
140+
$this->assertCount(3, $result);
141+
}
142+
143+
/**
144+
* Test steps:
145+
* 1. $iterator->next();
146+
* 2. $iterator->key()
147+
* 3. $iterator->next();
148+
* 4. $iterator->current()
149+
* 5. $iterator->key()
150+
*/
151+
public function testNext()
152+
{
153+
$filed = $this->correlationName . '.' . $this->rangeField;
154+
$this->selectMock->expects($this->at(0))->method('where')->with($filed . ' > ?', 0);
155+
$this->selectMock->expects($this->exactly(3))->method('limit')->with($this->batchSize);
156+
$this->selectMock->expects($this->exactly(3))->method('order')->with($filed . ' ASC');
157+
$this->selectMock->expects($this->at(3))->method('where')->with($filed . ' > ?', 25);
158+
159+
$this->wrapperSelectMock->expects($this->exactly(3))->method('from')->with(
160+
$this->selectMock,
161+
[
162+
new \Zend_Db_Expr('MAX(' . $this->rangeFieldAlias . ') as max'),
163+
new \Zend_Db_Expr('COUNT(*) as cnt')
164+
]
165+
);
166+
$this->connectionMock->expects($this->exactly(3))
167+
->method('fetchRow')
168+
->with($this->wrapperSelectMock)
169+
->willReturn([
170+
'max' => 25,
171+
'cnt' => 10
172+
]
173+
);
174+
175+
$this->assertEquals($this->selectMock, $this->model->next());
176+
$this->assertEquals(1, $this->model->key());
177+
178+
$this->assertEquals($this->selectMock, $this->model->next());
179+
$this->assertEquals($this->selectMock, $this->model->current());
180+
$this->assertEquals(2, $this->model->key());
181+
}
182+
}

‎lib/internal/Magento/Framework/Test/DB/Query/GeneratorTest.php

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
<?php
2+
/**
3+
* Copyright © 2016 Magento. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
namespace Magento\Framework\Test\Unit\DB\Query;
7+
8+
use Magento\Framework\DB\Query\Generator;
9+
use Magento\Framework\DB\Query\BatchIteratorFactory;
10+
use Magento\Framework\DB\Select;
11+
use Magento\Framework\DB\Query\BatchIterator;
12+
13+
class GeneratorTest extends \PHPUnit_Framework_TestCase
14+
{
15+
/**
16+
* @var Generator
17+
*/
18+
private $model;
19+
20+
/**
21+
* @var \PHPUnit_Framework_MockObject_MockObject
22+
*/
23+
private $selectMock;
24+
25+
/**
26+
* @var \PHPUnit_Framework_MockObject_MockObject
27+
*/
28+
private $factoryMock;
29+
30+
/**
31+
* @var \PHPUnit_Framework_MockObject_MockObject
32+
*/
33+
private $iteratorMock;
34+
35+
protected function setUp()
36+
{
37+
$this->factoryMock = $this->getMock(BatchIteratorFactory::class, [], [], '', false, false);
38+
$this->selectMock = $this->getMock(Select::class, [], [], '', false, false);
39+
$this->iteratorMock = $this->getMock(BatchIterator::class, [], [], '', false, false);
40+
$this->model = new Generator($this->factoryMock);
41+
}
42+
43+
public function testGenerate()
44+
{
45+
$map = [
46+
[
47+
Select::FROM,
48+
[
49+
'cp' => ['joinType' => Select::FROM]
50+
]
51+
],
52+
[
53+
Select::COLUMNS,
54+
[
55+
['cp', 'entity_id', 'product_id']
56+
]
57+
]
58+
];
59+
$this->selectMock->expects($this->exactly(2))->method('getPart')->willReturnMap($map);
60+
$this->factoryMock->expects($this->once())->method('create')->with(
61+
[
62+
'select' => $this->selectMock,
63+
'batchSize' => 100,
64+
'correlationName' => 'cp',
65+
'rangeField' => 'entity_id',
66+
'rangeFieldAlias' => 'product_id'
67+
]
68+
)->willReturn($this->iteratorMock);
69+
$this->assertEquals($this->iteratorMock, $this->model->generate('entity_id', $this->selectMock, 100));
70+
}
71+
72+
/**
73+
* @expectedException \Magento\Framework\Exception\LocalizedException
74+
* @expectedExceptionMessage Select object must have correct "FROM" part
75+
*/
76+
public function testGenerateWithoutFromPart()
77+
{
78+
$map = [
79+
[Select::FROM, []],
80+
[
81+
Select::COLUMNS,
82+
[
83+
['cp', 'entity_id', 'product_id']
84+
]
85+
]
86+
];
87+
$this->selectMock->expects($this->any())->method('getPart')->willReturnMap($map);
88+
$this->factoryMock->expects($this->never())->method('create');
89+
$this->model->generate('entity_id', $this->selectMock, 100);
90+
}
91+
92+
/**
93+
* @expectedException \Magento\Framework\Exception\LocalizedException
94+
* @expectedExceptionMessage Select object must have correct range field name "entity_id"
95+
*/
96+
public function testGenerateWithInvalidRangeField()
97+
{
98+
$map = [
99+
[
100+
Select::FROM,
101+
[
102+
'cp' => ['joinType' => Select::FROM]
103+
]
104+
],
105+
[
106+
Select::COLUMNS,
107+
[
108+
['cp', 'row_id', 'product_id']
109+
]
110+
]
111+
];
112+
$this->selectMock->expects($this->exactly(2))->method('getPart')->willReturnMap($map);
113+
$this->factoryMock->expects($this->never())->method('create');
114+
$this->model->generate('entity_id', $this->selectMock, 100);
115+
}
116+
117+
/**
118+
* @expectedException \Magento\Framework\Exception\LocalizedException
119+
* @expectedExceptionMessage Select object must have correct range field name "entity_id"
120+
*/
121+
public function testGenerateWithInvalidRangeFieldValue()
122+
{
123+
$map = [
124+
[
125+
Select::FROM,
126+
[
127+
'cp' => ['joinType' => Select::FROM]
128+
]
129+
],
130+
[
131+
Select::COLUMNS,
132+
[
133+
['cp', new \Zend_Db_Expr('MAX(entity_id) as max'), 'product_id']
134+
]
135+
]
136+
];
137+
$this->selectMock->expects($this->exactly(2))->method('getPart')->willReturnMap($map);
138+
$this->factoryMock->expects($this->never())->method('create');
139+
$this->model->generate('entity_id', $this->selectMock, 100);
140+
}
141+
}

0 commit comments

Comments
 (0)