Skip to content

Commit 04d3465

Browse files
JacobBrownAustinJacobBrownAustin
committed
ACPT-757
updating unit tests fixing use-case where request_path is NULL (why do we need this?) Fixing static tests Other small changes
1 parent 39778b5 commit 04d3465

File tree

2 files changed

+178
-93
lines changed

2 files changed

+178
-93
lines changed

‎app/code/Magento/UrlRewrite/Model/Storage/DbStorage.php

Lines changed: 102 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,9 @@ class DbStorage extends AbstractStorage
5555
private $maxRetryCount;
5656

5757
/**
58-
* @param UrlRewriteFactory $urlRewriteFactory
59-
* @param DataObjectHelper $dataObjectHelper
60-
* @param ResourceConnection $resource
58+
* @param UrlRewriteFactory $urlRewriteFactory
59+
* @param DataObjectHelper $dataObjectHelper
60+
* @param ResourceConnection $resource
6161
* @param LoggerInterface|null $logger
6262
* @param int $maxRetryCount
6363
*/
@@ -66,7 +66,7 @@ public function __construct(
6666
DataObjectHelper $dataObjectHelper,
6767
ResourceConnection $resource,
6868
LoggerInterface $logger = null,
69-
int $maxRetryCount = 3
69+
int $maxRetryCount = 5
7070
) {
7171
$this->connection = $resource->getConnection();
7272
$this->resource = $resource;
@@ -111,7 +111,6 @@ protected function doFindOneByData(array $data)
111111
&& is_string($data[UrlRewrite::REQUEST_PATH])
112112
) {
113113
$result = null;
114-
115114
$requestPath = $data[UrlRewrite::REQUEST_PATH];
116115
$decodedRequestPath = urldecode($requestPath);
117116
$data[UrlRewrite::REQUEST_PATH] = array_unique(
@@ -122,16 +121,13 @@ protected function doFindOneByData(array $data)
122121
rtrim($decodedRequestPath, '/') . '/',
123122
]
124123
);
125-
126124
$resultsFromDb = $this->connection->fetchAll($this->prepareSelect($data));
127125
if ($resultsFromDb) {
128126
$urlRewrite = $this->extractMostRelevantUrlRewrite($requestPath, $resultsFromDb);
129127
$result = $this->prepareUrlRewrite($requestPath, $urlRewrite);
130128
}
131-
132129
return $result;
133130
}
134-
135131
return $this->connection->fetchRow($this->prepareSelect($data));
136132
}
137133

@@ -224,23 +220,30 @@ private function deleteOldUrls(array $uniqueEntities): void
224220
);
225221
foreach ($uniqueEntities as $storeId => $entityTypes) {
226222
foreach ($entityTypes as $entityType => $entities) {
223+
// phpcs:ignore Magento2.Performance.ForeachArrayMerge
224+
$requestPaths = array_merge(...$entities);
225+
$requestPathFilter = '';
226+
if (!empty($requestPaths)) {
227+
$requestPathFilter = ' AND ' . $this->connection->quoteIdentifier(UrlRewrite::REQUEST_PATH)
228+
. ' NOT IN (' . $this->connection->quote($requestPaths) . ')';
229+
}
227230
$oldUrlsSelect->orWhere(
228231
$this->connection->quoteIdentifier(UrlRewrite::STORE_ID)
229-
. ' = ' . $this->connection->quote($storeId, 'INTEGER') .
230-
' AND ' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_ID)
231-
. ' IN (' . $this->connection->quote(array_keys($entities), 'INTEGER') . ')' .
232-
' AND ' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_TYPE)
233-
. ' = ' . $this->connection->quote($entityType) .
234-
' AND ' . $this->connection->quoteIdentifier(UrlRewrite::REQUEST_PATH)
235-
. ' NOT IN (' . $this->connection->quote(array_merge(...$entities)) . ')'
232+
. ' = ' . $this->connection->quote($storeId, 'INTEGER')
233+
. ' AND ' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_ID)
234+
. ' IN (' . $this->connection->quote(array_keys($entities), 'INTEGER') . ')'
235+
. ' AND ' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_TYPE)
236+
. ' = ' . $this->connection->quote($entityType)
237+
. $requestPathFilter
236238
);
237239
}
238240
}
239241
// prevent query locking in a case when nothing to delete
240242
$checkOldUrlsSelect = clone $oldUrlsSelect;
241243
$checkOldUrlsSelect->reset(Select::COLUMNS);
242-
$checkOldUrlsSelect->columns('count(*)');
243-
$hasOldUrls = (bool)$this->connection->fetchOne($checkOldUrlsSelect);
244+
$checkOldUrlsSelect->columns([new \Zend_Db_Expr('1')]);
245+
$checkOldUrlsSelect->limit(1);
246+
$hasOldUrls = false !== $this->connection->fetchOne($checkOldUrlsSelect);
244247
if ($hasOldUrls) {
245248
$this->connection->query(
246249
$oldUrlsSelect->deleteFromSelect(
@@ -250,6 +253,59 @@ private function deleteOldUrls(array $uniqueEntities): void
250253
}
251254
}
252255

256+
/**
257+
* Checks for duplicates both inside the new urls, and outside.
258+
* Because we are using INSERT ON DUPLICATE UPDATE, the insert won't give us an error.
259+
* So, we have to check for existing requestPaths in database with different entity_id.
260+
* And also, we need to check to make sure we don't have same requestPath more than once in our new rewrites.
261+
*
262+
* @param array $uniqueEntities
263+
* @return void
264+
*/
265+
private function checkDuplicates(array $uniqueEntities): void
266+
{
267+
$oldUrlsSelect = $this->connection->select();
268+
$oldUrlsSelect->from(
269+
$this->resource->getTableName(self::TABLE_NAME),
270+
[new \Zend_Db_Expr('1')]
271+
);
272+
$allEmpty = true;
273+
foreach ($uniqueEntities as $storeId => $entityTypes) {
274+
$newRequestPaths = [];
275+
foreach ($entityTypes as $entityType => $entities) {
276+
$requestPaths = array_merge(...$entities);
277+
$requestPathFilter = '';
278+
if (empty($requestPaths)) {
279+
continue;
280+
}
281+
$allEmpty = false;
282+
$oldUrlsSelect->orWhere(
283+
$this->connection->quoteIdentifier(UrlRewrite::STORE_ID)
284+
. ' = ' . $this->connection->quote($storeId, 'INTEGER')
285+
. ' AND (' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_ID)
286+
. ' NOT IN (' . $this->connection->quote(array_keys($entities), 'INTEGER') . ')'
287+
. ' OR ' . $this->connection->quoteIdentifier(UrlRewrite::ENTITY_TYPE)
288+
. ' != ' . $this->connection->quote($entityType)
289+
. ') AND ' . $this->connection->quoteIdentifier(UrlRewrite::REQUEST_PATH)
290+
. ' IN (' . $this->connection->quote($requestPaths) . ')'
291+
);
292+
foreach($requestPaths as $requestPath) {
293+
if (isset($newRequestPaths[$requestPath])) {
294+
throw new \Magento\Framework\Exception\AlreadyExistsException();
295+
}
296+
$newRequestPaths[$requestPath] = true;
297+
}
298+
}
299+
}
300+
if ($allEmpty) {
301+
return;
302+
}
303+
$oldUrlsSelect->limit(1);
304+
if (false !== $this->connection->fetchOne($oldUrlsSelect)) {
305+
throw new \Magento\Framework\Exception\AlreadyExistsException();
306+
}
307+
}
308+
253309
/**
254310
* Prepare array with unique entities
255311
*
@@ -260,7 +316,16 @@ private function prepareUniqueEntities(array $urls): array
260316
{
261317
$uniqueEntities = [];
262318
foreach ($urls as $url) {
263-
$uniqueEntities[$url->getStoreId()][$url->getEntityType()][$url->getEntityId()][] = $url->getRequestPath();
319+
$storeId = $url->getStoreId();
320+
$entityType = $url->getEntityType();
321+
$entityId = $url->getEntityId();
322+
$requestPath = $url->getRequestPath();
323+
if (null === $requestPath) { // Note: because SQL unique keys allow multiple nulls, we skip it.
324+
if (!isset($uniqueEntities[$storeId][$entityType][$entityId])) {
325+
$uniqueEntities[$storeId][$entityType][$entityId] = [];
326+
}
327+
}
328+
$uniqueEntities[$storeId][$entityType][$entityId][] = $requestPath;
264329
}
265330
return $uniqueEntities;
266331
}
@@ -275,13 +340,18 @@ protected function doReplace(array $urls): array
275340
foreach ($urls as $url) {
276341
$data[] = $url->toArray();
277342
}
278-
for ($tries = 0; $tries < $this->maxRetryCount; $tries++) {
343+
for ($tries = 0; ; $tries++) {
279344
$this->connection->beginTransaction();
280345
try {
281346
$this->deleteOldUrls($uniqueEntities);
347+
$this->checkDuplicates($uniqueEntities);
282348
$this->upsertMultiple($data);
283349
$this->connection->commit();
284350
} catch (\Magento\Framework\DB\Adapter\DeadlockException $deadlockException) {
351+
$this->connection->rollBack();
352+
if ($tries >= $this->maxRetryCount) {
353+
throw $deadlockException;
354+
}
285355
continue;
286356
} catch (\Magento\Framework\Exception\AlreadyExistsException $e) {
287357
$this->connection->rollBack();
@@ -295,6 +365,13 @@ protected function doReplace(array $urls): array
295365
]
296366
);
297367
if (isset($urlFound[UrlRewrite::URL_REWRITE_ID])) {
368+
if (isset($uniqueEntities
369+
[$urlFound[UrlRewrite::STORE_ID]]
370+
[$urlFound[UrlRewrite::ENTITY_TYPE]]
371+
[$urlFound[UrlRewrite::ENTITY_ID]
372+
])) {
373+
continue; // Note: If it's one of the entities we are updating, then it is okay.
374+
}
298375
$urlConflicted[$urlFound[UrlRewrite::URL_REWRITE_ID]] = $url->toArray();
299376
}
300377
}
@@ -312,15 +389,14 @@ protected function doReplace(array $urls): array
312389
$this->connection->rollBack();
313390
throw $e;
314391
}
315-
break;
392+
return $urls;
316393
}
317-
return $urls;
318394
}
319395

320396
/**
321397
* Insert multiple
322398
*
323-
* @param array $data
399+
* @param array $data
324400
* @return void
325401
* @throws \Magento\Framework\Exception\AlreadyExistsException|\Exception
326402
* @throws \Exception
@@ -352,15 +428,17 @@ protected function insertMultiple($data): void
352428
*/
353429
private function upsertMultiple(array $data): void
354430
{
431+
355432
$this->connection->insertOnDuplicate($this->resource->getTableName(self::TABLE_NAME), $data);
356433
}
357434

358435
/**
359436
* Get filter for url rows deletion due to provided urls
360437
*
361-
* @param UrlRewrite[] $urls
362-
* @return array
438+
* @param UrlRewrite[] $urls
439+
* @return array
363440
* @deprecated 101.0.3 Not used anymore.
441+
* @see nothing
364442
*/
365443
protected function createFilterDataBasedOnUrls($urls): array
366444
{

0 commit comments

Comments
 (0)