Skip to content

Commit c3e5809

Browse files
aplapanaaplapana
committed
ACP2E-4094: Add index check for queries when loggging mode is enabled.
1 parent 2a39aa6 commit c3e5809

File tree

4 files changed

+143
-70
lines changed

4 files changed

+143
-70
lines changed

‎app/code/Magento/Developer/Console/Command/QueryLogEnableCommand.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ protected function configure()
9191
self::INPUT_ARG_LOG_INDEX_CHECK,
9292
null,
9393
InputOption::VALUE_OPTIONAL,
94-
'Include index check. [true|false]',
94+
'Include index check. Warning: may cause performance degradation. [true|false]',
9595
"false"
9696
)
9797
]

‎lib/internal/Magento/Framework/DB/Logger/QueryIndexAnalyzer.php‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,11 @@ public function process(string $sql, array $bindings): array
5858
$explainOutput = $this->analyzerCache[$cacheKey];
5959
} else {
6060
$connection = $this->resource->getConnection();
61-
$explainOutput = $connection->query('EXPLAIN ' . $sql, $bindings)->fetchAll();
61+
try {
62+
$explainOutput = $connection->query('EXPLAIN ' . $sql, $bindings)->fetchAll();
63+
} catch (\Zend_Db_Adapter_Exception) {
64+
$explainOutput = [];
65+
}
6266
$this->analyzerCache[$cacheKey] = $explainOutput;
6367
}
6468

@@ -142,7 +146,7 @@ private function getQueryIssues(array $selectDetails): ?array
142146
$issues[] = self::FULL_TABLE_SCAN;
143147
}
144148

145-
if ($this->isUsingIndex($selectDetails)) {
149+
if (false === $this->isUsingIndex($selectDetails)) {
146150
$issues[] = self::NO_INDEX;
147151
}
148152

@@ -198,7 +202,7 @@ private function isUsingIndex(array $selectDetails): bool
198202
$extra = strtolower($selectDetails['extra'] ?? '');
199203
$key = $selectDetails['key'] ?? null;
200204

201-
return empty($key) && !str_contains($extra, 'no matching row in const table');
205+
return !(empty($key) && !str_contains($extra, 'no matching row in const table'));
202206
}
203207

204208
/**
@@ -259,11 +263,11 @@ private function isPartialIndexUsage(array $row): bool
259263
*/
260264
private function checkForCoveringIndex(string $extra, string $type): bool
261265
{
262-
return str_contains($extra, 'using index')
266+
return (str_contains($extra, 'using index')
263267
&& !str_contains($extra, 'using where')
264268
|| str_contains($extra, 'using index')
265269
&& str_contains($extra, 'using where')
266-
&& in_array($type, ['range', 'ref']);
270+
&& in_array($type, ['range', 'ref']));
267271
}
268272

269273
/**

‎lib/internal/Magento/Framework/DB/Test/Unit/Logger/FileTest.php‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -83,20 +83,31 @@ public function testLog()
8383
}
8484

8585
/**
86-
* @param $type
87-
*
86+
* @return void
87+
*/
88+
public function testNothingToLog(): void
89+
{
90+
$this->stream->expects($this->never())->method('write');
91+
$this->object->logStats(
92+
LoggerInterface::TYPE_QUERY,
93+
'EXPLAIN SELECT something',
94+
['data']
95+
);
96+
}
97+
98+
/**
99+
* @param string $type
88100
* @param string $q
89101
* @param array $bind
90-
* @param \Zend_Db_Statement_Pdo|null $result
91102
* @param string $expected
92103
* @dataProvider logStatsDataProvider
93104
*/
94-
public function testLogStats($type, $q, array $bind, $result, $expected)
105+
public function testLogStats(string $type, string $q, array $bind, string $expected)
95106
{
96-
$this->stream->expects($expected ? $this->once() : $this->never())
107+
$this->stream->expects($this->once())
97108
->method('write')
98109
->with($this->matches($expected));
99-
$this->object->logStats($type, $q, $bind, $result);
110+
$this->object->logStats($type, $q, $bind);
100111
}
101112

102113
/**
@@ -105,35 +116,25 @@ public function testLogStats($type, $q, array $bind, $result, $expected)
105116
public static function logStatsDataProvider()
106117
{
107118
return [
108-
[LoggerInterface::TYPE_CONNECT, '', [], null, '%aCONNECT%a'],
119+
[LoggerInterface::TYPE_CONNECT, '', [], '%aCONNECT%a'],
109120
[
110121
LoggerInterface::TYPE_TRANSACTION,
111122
'SELECT something',
112123
[],
113-
null,
114124
'%aTRANSACTION SELECT something%a'
115125
],
116126
[
117127
LoggerInterface::TYPE_QUERY,
118128
'SELECT something',
119129
[],
120-
null,
121130
'%aSQL: SELECT something%a'
122131
],
123132
[
124133
LoggerInterface::TYPE_QUERY,
125134
'SELECT something',
126135
['data'],
127-
null,
128136
"%aQUERY%aSQL: SELECT something%aBIND: array (%a0 => 'data',%a)%a"
129-
],
130-
[
131-
LoggerInterface::TYPE_QUERY,
132-
'EXPLAIN SELECT something',
133-
['data'],
134-
null,
135-
''
136-
],
137+
]
137138
];
138139
}
139140

‎lib/internal/Magento/Framework/DB/Test/Unit/Logger/QueryIndexAnalyzerTest.php‎

Lines changed: 114 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,9 @@
99

1010
use Magento\Framework\App\ResourceConnection;
1111
use Magento\Framework\DB\Adapter\AdapterInterface;
12-
use Magento\Framework\DB\Logger\File;
1312
use Magento\Framework\DB\Logger\QueryAnalyzerException;
1413
use Magento\Framework\DB\Logger\QueryAnalyzerInterface;
1514
use Magento\Framework\DB\Logger\QueryIndexAnalyzer;
16-
use Magento\Framework\DB\LoggerInterface;
17-
use Magento\Framework\Exception\FileSystemException;
1815
use Magento\Framework\Serialize\Serializer\Json;
1916
use PHPUnit\Framework\MockObject\Exception;
2017
use PHPUnit\Framework\MockObject\MockObject;
@@ -50,53 +47,87 @@ protected function setUp(): void
5047
}
5148

5249
/**
53-
* @param string $sql
54-
* @param array $bind
55-
* @param int $serializeCall
56-
* @param string $explainResult
57-
* @param mixed $expectedResult
5850
* @return void
5951
* @throws Exception
6052
* @throws QueryAnalyzerException
6153
* @throws \Zend_Db_Statement_Exception
62-
* @testdox $sql with bindings $bind to get $expectedResult
63-
* @dataProvider statsDataProvider
6454
*/
65-
public function testProcess(
66-
string $sql,
67-
array $bind,
68-
int $serializeCall,
69-
string $explainResult,
70-
mixed $expectedResult
71-
): void {
72-
$this->serializer->expects($this->exactly($serializeCall))
55+
public function testQueryException(): void
56+
{
57+
$sql = "SELECT * FROM `admin_system_messages`";
58+
$bind = [];
59+
$this->serializer->expects($this->once())
60+
->method('serialize')
61+
->with($bind)
62+
->willReturn(json_encode($bind));
63+
$connection = $this->createMock(AdapterInterface::class);
64+
$connection->expects($this->once())
65+
->method('query')
66+
->with('EXPLAIN ' . $sql)
67+
->willThrowException(new \Zend_Db_Adapter_Exception("Query error"));
68+
$this->resource->expects($this->once())->method('getConnection')->willReturn($connection);
69+
70+
$this->expectException(QueryAnalyzerException::class);
71+
$this->expectExceptionMessage("No 'explain' output available");
72+
$this->queryAnalyzer->process($sql, $bind);
73+
}
74+
75+
/**
76+
* @return void
77+
* @throws Exception
78+
* @throws QueryAnalyzerException
79+
* @throws \Zend_Db_Statement_Exception
80+
*/
81+
public function testProcessSmallTable(): void
82+
{
83+
$sql = "SELECT `main_table`.* FROM `admin_system_messages` AS `main_table`
84+
ORDER BY severity ASC, created_at DESC";
85+
$bind = [];
86+
$explainResult = '[{"id":"1","select_type":"SIMPLE","table":"admin_system_messages","partitions":null,"type":"ALL",
87+
"possible_keys":null,"key":null,"key_len":null,"ref":null,"rows":"1","filtered":"100.00",
88+
"Extra":"Using filesort"}]';
89+
90+
$this->serializer->expects($this->once())
7391
->method('serialize')
7492
->with($bind)
7593
->willReturn(json_encode($bind));
7694
$statement = $this->createMock(\Zend_Db_Statement_Interface::class);
77-
$statement->expects($this->any())->method('fetchAll')->willReturn(json_decode($explainResult, true));
95+
$statement->expects($this->once())
96+
->method('fetchAll')
97+
->willReturn(json_decode($explainResult, true));
7898
$connection = $this->createMock(AdapterInterface::class);
79-
$connection->expects($this->any())
99+
$connection->expects($this->once())
80100
->method('query')
81101
->with('EXPLAIN ' . $sql)
82102
->willReturn($statement);
83-
$this->resource->expects($this->any())->method('getConnection')->willReturn($connection);
103+
$this->resource->expects($this->once())->method('getConnection')->willReturn($connection);
84104

85-
if ($expectedResult instanceof \Exception) {
86-
$this->expectException(\Exception::class);
87-
$this->expectExceptionMessage($expectedResult->getMessage());
88-
$this->queryAnalyzer->process($sql, $bind);
89-
} else {
90-
$result = $this->queryAnalyzer->process($sql, $bind);
91-
$this->assertSame($expectedResult, $result);
92-
}
105+
$this->expectException(QueryAnalyzerException::class);
106+
$this->expectExceptionMessage("Small table");
107+
$this->queryAnalyzer->process($sql, $bind);
93108
}
94109

95110
/**
96-
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
97-
* @return array
111+
* @param string $sql
112+
* @param array $bind
113+
* @return void
114+
* @throws QueryAnalyzerException
115+
* @throws \Zend_Db_Statement_Exception
116+
* @dataProvider statsNonSelectDataProvider
117+
* @testdox $sql with bindings $bind to get $expectedResult
98118
*/
99-
public static function statsDataProvider(): array
119+
public function testProcessThrowsExceptionForNonSelectQuery(string $sql, array $bind): void {
120+
$this->expectException(QueryAnalyzerException::class);
121+
$this->expectExceptionMessage("Can't process query type");
122+
$this->serializer->expects($this->never())->method('serialize');
123+
124+
$this->queryAnalyzer->process($sql, $bind);
125+
}
126+
127+
/**
128+
* @return array[]
129+
*/
130+
public static function statsNonSelectDataProvider(): array
100131
{
101132
return [
102133
'no-stats-for-update-query' => [
@@ -123,17 +154,57 @@ public static function statsDataProvider(): array
123154
0,
124155
'{}',
125156
new QueryAnalyzerException("Can't process query type")
126-
],
127-
'small-table-query' => [
128-
"SELECT `main_table`.* FROM `admin_system_messages` AS `main_table`
129-
ORDER BY severity ASC, created_at DESC",
130-
[],
131-
1,
132-
'[{"id":"1","select_type":"SIMPLE","table":"admin_system_messages","partitions":null,"type":"ALL",
133-
"possible_keys":null,"key":null,"key_len":null,"ref":null,"rows":"1","filtered":"100.00",
134-
"Extra":"Using filesort"}]',
135-
new QueryAnalyzerException("Small table")
136-
],
157+
]
158+
];
159+
}
160+
161+
/**
162+
* @param string $sql
163+
* @param array $bind
164+
* @param string $explainResult
165+
* @param mixed $expectedResult
166+
* @return void
167+
* @throws Exception
168+
* @throws QueryAnalyzerException
169+
* @throws \Zend_Db_Statement_Exception
170+
* @testdox $sql with bindings $bind to get $expectedResult
171+
* @dataProvider statsDataProvider
172+
*/
173+
public function testProcess(
174+
string $sql,
175+
array $bind,
176+
string $explainResult,
177+
mixed $expectedResult
178+
): void {
179+
$this->serializer->expects($this->once())
180+
->method('serialize')
181+
->with($bind)
182+
->willReturn(json_encode($bind));
183+
$statement = $this->createMock(\Zend_Db_Statement_Interface::class);
184+
$statement->expects($this->any())->method('fetchAll')->willReturn(json_decode($explainResult, true));
185+
$connection = $this->createMock(AdapterInterface::class);
186+
$connection->expects($this->once())
187+
->method('query')
188+
->with('EXPLAIN ' . $sql)
189+
->willReturn($statement);
190+
$this->resource->expects($this->once())->method('getConnection')->willReturn($connection);
191+
192+
if ($expectedResult instanceof \Exception) {
193+
$this->expectException(\Exception::class);
194+
$this->expectExceptionMessage($expectedResult->getMessage());
195+
$this->queryAnalyzer->process($sql, $bind);
196+
} else {
197+
$result = $this->queryAnalyzer->process($sql, $bind);
198+
$this->assertSame($expectedResult, $result);
199+
}
200+
}
201+
202+
/**
203+
* @return array
204+
*/
205+
public static function statsDataProvider(): array
206+
{
207+
return [
137208
'subselect-with-dependent-query' => [
138209
"SELECT `main_table`.*, (IF(
139210
(SELECT count(*)
@@ -145,7 +216,6 @@ public static function statsDataProvider(): array
145216
)) AS `status` FROM `magento_bulk` AS `main_table` WHERE (`user_id` = '1')
146217
ORDER BY FIELD(status, 2,3,0,4,1), start_time DESC",
147218
[],
148-
1,
149219
'[{"id":"1","select_type":"PRIMARY","table":"main_table","partitions":null,"type":"ref",
150220
"possible_keys":"MAGENTO_BULK_USER_ID","key":"MAGENTO_BULK_USER_ID","key_len":"5","ref":"const",
151221
"rows":"1","filtered":"100.00","Extra":"Using filesort"},{"id":"3","select_type":"DEPENDENT SUBQUERY",
@@ -168,7 +238,6 @@ public static function statsDataProvider(): array
168238
('simple', 'virtual', 'bundle', 'downloadable', 'configurable', 'grouped')))
169239
GROUP BY `o`.`product_type`",
170240
[],
171-
1,
172241
'[{"id":1,"select_type":"SIMPLE","table":"o","partitions":null,"type":"ref","possible_keys":
173242
"SALES_ORDER_ITEM_ORDER_ID","key":"SALES_ORDER_ITEM_ORDER_ID","key_len":"4","ref":"const",
174243
"rows":2,"filtered":45,"Extra":"Using where; Using temporary"}]',
@@ -186,7 +255,6 @@ public static function statsDataProvider(): array
186255
OR (`is_visible_in_advanced_search` = '1') OR (((`is_filterable` = '1') OR (`is_filterable` = '2')))
187256
OR (`is_filterable_in_search` = '1'))",
188257
[],
189-
1,
190258
'[{"id":1,"select_type":"SIMPLE","table":"additional_table","partitions":null,"type":"ALL",
191259
"possible_keys":"PRIMARY","key":null,"key_len":null,"ref":null,"rows":170,"filtered":40.95,"Extra":
192260
"Using where"},{"id":1,"select_type":"SIMPLE","table":"main_table","partitions":null,"type":"eq_ref",

0 commit comments

Comments
 (0)