From 1829a7106368ef5f689fa5b09ab22fb76bb968d3 Mon Sep 17 00:00:00 2001 From: Matthew O'Loughlin Date: Mon, 17 Aug 2020 22:22:25 +0930 Subject: [PATCH 1/7] Separate operation_key into it's own getter/setter, restoring the increment ID as the key for collection building. This resolves an issue where performing a search that covers more than 1 bulk_uuid would throw an error since the collection builder attempts to re-use an existing array key when the same operation_key exists for both bulk_uuids --- .../Model/Operation.php | 17 ++++++++++++++++ .../Operation/OperationRepository.php | 2 +- .../Framework/Bulk/OperationInterface.php | 20 ++++++++++++++++++- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/AsynchronousOperations/Model/Operation.php b/app/code/Magento/AsynchronousOperations/Model/Operation.php index 00f33d10a1e1b..c9fc3dd5edf93 100644 --- a/app/code/Magento/AsynchronousOperations/Model/Operation.php +++ b/app/code/Magento/AsynchronousOperations/Model/Operation.php @@ -162,6 +162,23 @@ public function setErrorCode($errorCode) return $this->setData(self::ERROR_CODE, $errorCode); } + /** + * @inheritDoc + */ + public function getOperationKey() + { + return $this->getData(self::OPERATION_KEY); + } + + /** + * @inheritDoc + */ + public function setOperationKey(int $operationKey) + { + $this->setData(self::OPERATION_KEY, $operationKey); + return $this; + } + /** * Retrieve existing extension attributes object. * diff --git a/app/code/Magento/AsynchronousOperations/Model/ResourceModel/Operation/OperationRepository.php b/app/code/Magento/AsynchronousOperations/Model/ResourceModel/Operation/OperationRepository.php index 6757b0c8f0a5c..4151afa037502 100644 --- a/app/code/Magento/AsynchronousOperations/Model/ResourceModel/Operation/OperationRepository.php +++ b/app/code/Magento/AsynchronousOperations/Model/ResourceModel/Operation/OperationRepository.php @@ -91,7 +91,7 @@ public function createByTopic($topicName, $entityParams, $groupId, $operationId) ]; $data = [ 'data' => [ - OperationInterface::ID => $operationId, + OperationInterface::OPERATION_KEY => $operationId, OperationInterface::BULK_ID => $groupId, OperationInterface::TOPIC_NAME => $topicName, OperationInterface::SERIALIZED_DATA => $this->jsonSerializer->serialize($serializedData), diff --git a/lib/internal/Magento/Framework/Bulk/OperationInterface.php b/lib/internal/Magento/Framework/Bulk/OperationInterface.php index a621106be3806..2b55b488aa126 100644 --- a/lib/internal/Magento/Framework/Bulk/OperationInterface.php +++ b/lib/internal/Magento/Framework/Bulk/OperationInterface.php @@ -15,11 +15,12 @@ interface OperationInterface extends \Magento\Framework\Api\ExtensibleDataInterf /**#@+ * Constants for keys of data array. Identical to the name of the getter in snake case */ - const ID = 'operation_key'; + const ID = 'id'; const BULK_ID = 'bulk_uuid'; const TOPIC_NAME = 'topic_name'; const SERIALIZED_DATA = 'serialized_data'; const RESULT_SERIALIZED_DATA = 'result_serialized_data'; + const OPERATION_KEY = 'operation_key'; const STATUS = 'status'; const RESULT_MESSAGE = 'result_message'; const ERROR_CODE = 'error_code'; @@ -172,4 +173,21 @@ public function getErrorCode(); * @since 103.0.0 */ public function setErrorCode($errorCode); + + /** + * Get operation key + * + * @return int + * @since 103.0.1 + */ + public function getOperationKey(); + + /** + * Set operation key + * + * @param int $operationKey + * @return $this + * @since 103.0.1 + */ + public function setOperationKey(int $operationKey); } From 28ca7a477664fb0b06cb907723752193cdafe1b7 Mon Sep 17 00:00:00 2001 From: Matthew O'Loughlin Date: Mon, 17 Aug 2020 23:28:26 +0930 Subject: [PATCH 2/7] Adjustments to tests based on proposed changes, and a new test case to cover the bug being resolved by this fix. --- .../Model/BulkManagement.php | 2 +- .../Api/OperationRepositoryInterfaceTest.php | 48 +++++++++++++++++++ .../Model/OperationManagementTest.php | 2 +- .../_files/operation_searchable.php | 9 ++++ 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php b/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php index 6cf0611eb28ec..5437e25950695 100644 --- a/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php +++ b/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php @@ -168,7 +168,7 @@ public function retryBulk($bulkUuid, array $errorCodes) $currentBatchSize = 0; } $currentBatchSize++; - $operationIds[] = $operation->getId(); + $operationIds[] = $operation->getOperationKey(); } // remove operations from the last batch if (!empty($operationIds)) { diff --git a/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php b/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php index 0e8cdb6ef37ef..a5af0b08fbc1a 100644 --- a/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php +++ b/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php @@ -121,4 +121,52 @@ public function testGetList() $this->assertEquals('bulk-uuid-searchable-6', $item['bulk_uuid']); } } + + /** + * @magentoApiDataFixture Magento/AsynchronousOperations/_files/operation_searchable.php + */ + public function testGetMultipleBulkUuids() + { + $searchCriteria = [ + 'searchCriteria' => [ + 'filter_groups' => [ + [ + 'filters' => [ + [ + 'field' => 'status', + 'value' => OperationInterface::STATUS_TYPE_COMPLETE, + 'condition_type' => 'eq', + ], + ], + ], + ], + 'current_page' => 1, + ], + ]; + + $serviceInfo = [ + 'rest' => [ + 'resourcePath' => self::RESOURCE_PATH . '?' . http_build_query($searchCriteria), + 'httpMethod' => Request::HTTP_METHOD_GET, + ], + 'soap' => [ + 'service' => self::SERVICE_NAME, + 'operation' => self::SERVICE_NAME . 'GetList', + ], + ]; + + $response = $this->_webApiCall($serviceInfo, $searchCriteria); + + $this->assertArrayHasKey('search_criteria', $response); + $this->assertArrayHasKey('total_count', $response); + $this->assertArrayHasKey('items', $response); + + $this->assertEquals($searchCriteria['searchCriteria'], $response['search_criteria']); + $this->assertEquals(2, $response['total_count']); + $this->assertCount(2, $response['items']); + + foreach ($response['items'] as $item) { + $this->assertEquals('0', $item['operation_key']); + } + } } diff --git a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php index 4be795dd654cd..8d98ca7e5f8a8 100644 --- a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php +++ b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php @@ -61,7 +61,7 @@ public function testGetBulkStatus() } /** @var OperationInterface $operation */ $operation = array_shift($operations); - $operationId = $operation->getId(); + $operationId = $operation->getOperationKey(); $this->assertTrue($this->model->changeOperationStatus( 'bulk-uuid-5', diff --git a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php index 1e27df71d5709..46114673a8fd4 100644 --- a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php +++ b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php @@ -89,6 +89,15 @@ 'result_message' => '', 'operation_key' => 5 ], + [ + 'bulk_uuid' => 'bulk-uuid-searchable-6-1', + 'topic_name' => 'topic-5', + 'serialized_data' => json_encode(['entity_id' => 5]), + 'status' => OperationInterface::STATUS_TYPE_COMPLETE, + 'error_code' => null, + 'result_message' => '', + 'operation_key' => 0 + ], ]; $bulkQuery = "INSERT INTO {$bulkTable} (`uuid`, `user_id`, `description`, `operation_count`, `start_time`)" From ac37fdc24749e35729f1183627efecc6db9fcac5 Mon Sep 17 00:00:00 2001 From: Matthew O'Loughlin Date: Tue, 18 Aug 2020 15:57:45 +0930 Subject: [PATCH 3/7] Update BulkManagement unit tests to use getOperationKey instead of getId --- .../Test/Unit/Model/BulkManagementTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/AsynchronousOperations/Test/Unit/Model/BulkManagementTest.php b/app/code/Magento/AsynchronousOperations/Test/Unit/Model/BulkManagementTest.php index 14abb41c77fc4..bf0ac9e159b00 100644 --- a/app/code/Magento/AsynchronousOperations/Test/Unit/Model/BulkManagementTest.php +++ b/app/code/Magento/AsynchronousOperations/Test/Unit/Model/BulkManagementTest.php @@ -215,7 +215,7 @@ public function testRetryBulk() $operation = $this->getMockForAbstractClass(OperationInterface::class); $operationCollection->expects($this->once())->method('getItems')->willReturn([$operation]); $connection->expects($this->once())->method('beginTransaction')->willReturnSelf(); - $operation->expects($this->once())->method('getId')->willReturn($operationId); + $operation->expects($this->once())->method('getOperationKey')->willReturn($operationId); $this->resourceConnection->expects($this->once()) ->method('getTableName')->with($operationTable)->willReturn($operationTable); $connection->expects($this->at(1)) @@ -265,7 +265,7 @@ public function testRetryBulkWithException() $operation = $this->getMockForAbstractClass(OperationInterface::class); $operationCollection->expects($this->once())->method('getItems')->willReturn([$operation]); $connection->expects($this->once())->method('beginTransaction')->willReturnSelf(); - $operation->expects($this->once())->method('getId')->willReturn($operationId); + $operation->expects($this->once())->method('getOperationKey')->willReturn($operationId); $this->resourceConnection->expects($this->once()) ->method('getTableName')->with($operationTable)->willReturn($operationTable); $connection->expects($this->at(1)) From 52a316098bff1d4ad265d61dca27e065402611b3 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Wed, 30 Sep 2020 13:54:07 +0300 Subject: [PATCH 4/7] refactor bulk operation --- .../Magento/AsynchronousOperations/Model/Operation.php | 5 +++-- .../_files/operation_searchable.php | 9 ++++++++- .../Magento/Framework/Bulk/OperationInterface.php | 8 ++++---- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/code/Magento/AsynchronousOperations/Model/Operation.php b/app/code/Magento/AsynchronousOperations/Model/Operation.php index c9fc3dd5edf93..c15ed14c9575c 100644 --- a/app/code/Magento/AsynchronousOperations/Model/Operation.php +++ b/app/code/Magento/AsynchronousOperations/Model/Operation.php @@ -7,6 +7,7 @@ use Magento\AsynchronousOperations\Api\Data\OperationInterface; use Magento\AsynchronousOperations\Model\OperationStatusValidator; +use Magento\Framework\Bulk\OperationInterface as BulkOperationInterface; use Magento\Framework\DataObject; /** @@ -165,7 +166,7 @@ public function setErrorCode($errorCode) /** * @inheritDoc */ - public function getOperationKey() + public function getOperationKey(): ?int { return $this->getData(self::OPERATION_KEY); } @@ -173,7 +174,7 @@ public function getOperationKey() /** * @inheritDoc */ - public function setOperationKey(int $operationKey) + public function setOperationKey(?int $operationKey): BulkOperationInterface { $this->setData(self::OPERATION_KEY, $operationKey); return $this; diff --git a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php index 46114673a8fd4..eed6713dd5bcd 100644 --- a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php +++ b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/_files/operation_searchable.php @@ -25,6 +25,13 @@ 'operation_count' => 6, 'start_time' => '2009-10-10 00:00:00', ], + 'started_searchable_second' => [ + 'uuid' => 'bulk-uuid-searchable-8', + 'user_id' => 1, + 'description' => 'Bulk Description', + 'operation_count' => 1, + 'start_time' => '2012-10-10 00:00:00', + ], 'not_started' => [ 'uuid' => 'bulk-uuid-searchable-7', 'user_id' => 1, @@ -90,7 +97,7 @@ 'operation_key' => 5 ], [ - 'bulk_uuid' => 'bulk-uuid-searchable-6-1', + 'bulk_uuid' => 'bulk-uuid-searchable-8', 'topic_name' => 'topic-5', 'serialized_data' => json_encode(['entity_id' => 5]), 'status' => OperationInterface::STATUS_TYPE_COMPLETE, diff --git a/lib/internal/Magento/Framework/Bulk/OperationInterface.php b/lib/internal/Magento/Framework/Bulk/OperationInterface.php index 2b55b488aa126..45f684f1bb3cf 100644 --- a/lib/internal/Magento/Framework/Bulk/OperationInterface.php +++ b/lib/internal/Magento/Framework/Bulk/OperationInterface.php @@ -177,17 +177,17 @@ public function setErrorCode($errorCode); /** * Get operation key * - * @return int + * @return int|null * @since 103.0.1 */ - public function getOperationKey(); + public function getOperationKey(): ?int; /** * Set operation key * - * @param int $operationKey + * @param int|null $operationKey * @return $this * @since 103.0.1 */ - public function setOperationKey(int $operationKey); + public function setOperationKey(?int $operationKey): self; } From 8c39c0577874eb86e548fae921bd8be15a6e0c00 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Mon, 18 Jan 2021 13:24:48 +0200 Subject: [PATCH 5/7] fixed variable names --- .../AsynchronousOperations/Model/BulkManagement.php | 12 ++++++------ .../AsynchronousOperations/Model/Operation.php | 2 +- .../Model/OperationManagementTest.php | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php b/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php index 5437e25950695..4add11a41f515 100644 --- a/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php +++ b/app/code/Magento/AsynchronousOperations/Model/BulkManagement.php @@ -151,28 +151,28 @@ public function retryBulk($bulkUuid, array $errorCodes) // remove corresponding operations from database (i.e. move them to 'open' status) $connection->beginTransaction(); try { - $operationIds = []; + $operationKeys = []; $currentBatchSize = 0; $maxBatchSize = 10000; /** @var OperationInterface $operation */ foreach ($retriablyFailedOperations as $operation) { if ($currentBatchSize === $maxBatchSize) { - $whereCondition = $connection->quoteInto('operation_key IN (?)', $operationIds) + $whereCondition = $connection->quoteInto('operation_key IN (?)', $operationKeys) . " AND " . $connection->quoteInto('bulk_uuid = ?', $bulkUuid); $connection->delete( $this->resourceConnection->getTableName('magento_operation'), $whereCondition ); - $operationIds = []; + $operationKeys = []; $currentBatchSize = 0; } $currentBatchSize++; - $operationIds[] = $operation->getOperationKey(); + $operationKeys[] = $operation->getOperationKey(); } // remove operations from the last batch - if (!empty($operationIds)) { - $whereCondition = $connection->quoteInto('operation_key IN (?)', $operationIds) + if (!empty($operationKeys)) { + $whereCondition = $connection->quoteInto('operation_key IN (?)', $operationKeys) . " AND " . $connection->quoteInto('bulk_uuid = ?', $bulkUuid); $connection->delete( diff --git a/app/code/Magento/AsynchronousOperations/Model/Operation.php b/app/code/Magento/AsynchronousOperations/Model/Operation.php index c15ed14c9575c..cb7ac4b7b43cf 100644 --- a/app/code/Magento/AsynchronousOperations/Model/Operation.php +++ b/app/code/Magento/AsynchronousOperations/Model/Operation.php @@ -168,7 +168,7 @@ public function setErrorCode($errorCode) */ public function getOperationKey(): ?int { - return $this->getData(self::OPERATION_KEY); + return (int) $this->getData(self::OPERATION_KEY); } /** diff --git a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php index 8d98ca7e5f8a8..2db16af43c34e 100644 --- a/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php +++ b/dev/tests/integration/testsuite/Magento/AsynchronousOperations/Model/OperationManagementTest.php @@ -61,11 +61,11 @@ public function testGetBulkStatus() } /** @var OperationInterface $operation */ $operation = array_shift($operations); - $operationId = $operation->getOperationKey(); + $operationKey = $operation->getOperationKey(); $this->assertTrue($this->model->changeOperationStatus( 'bulk-uuid-5', - $operationId, + $operationKey, OperationInterface::STATUS_TYPE_OPEN )); @@ -74,7 +74,7 @@ public function testGetBulkStatus() $select = $connection->select() ->from($table) ->where("bulk_uuid = ?", 'bulk-uuid-5') - ->where("operation_key = ?", $operationId); + ->where("operation_key = ?", $operationKey); $updatedOperation = $connection->fetchRow($select); $this->assertEquals(OperationInterface::STATUS_TYPE_OPEN, $updatedOperation['status']); From 49614f49f7c787561bd6ed526a75e86b64749797 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Mon, 18 Jan 2021 14:13:17 +0200 Subject: [PATCH 6/7] fixed not convert all values to int --- app/code/Magento/AsynchronousOperations/Model/Operation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/Magento/AsynchronousOperations/Model/Operation.php b/app/code/Magento/AsynchronousOperations/Model/Operation.php index cb7ac4b7b43cf..79e7f2e10ff31 100644 --- a/app/code/Magento/AsynchronousOperations/Model/Operation.php +++ b/app/code/Magento/AsynchronousOperations/Model/Operation.php @@ -168,7 +168,7 @@ public function setErrorCode($errorCode) */ public function getOperationKey(): ?int { - return (int) $this->getData(self::OPERATION_KEY); + return $this->getData(self::OPERATION_KEY) ? (int) $this->getData(self::OPERATION_KEY) : null; } /** From e90bfa556226b3d6c766b04fe26831243f1495f3 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Wed, 20 Jan 2021 14:07:00 +0200 Subject: [PATCH 7/7] fixed WebApi test --- .../Api/OperationRepositoryInterfaceTest.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php b/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php index a5af0b08fbc1a..a0c96c4de4c1b 100644 --- a/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php +++ b/dev/tests/api-functional/testsuite/Magento/AsynchronousOperations/Api/OperationRepositoryInterfaceTest.php @@ -165,8 +165,9 @@ public function testGetMultipleBulkUuids() $this->assertEquals(2, $response['total_count']); $this->assertCount(2, $response['items']); - foreach ($response['items'] as $item) { - $this->assertEquals('0', $item['operation_key']); - } + $this->assertEquals( + ['bulk-uuid-searchable-6', 'bulk-uuid-searchable-8'], + array_column($response['items'], 'bulk_uuid') + ); } }