Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579

moloughlin · 2020-08-17T14:16:11Z

Description (*)

The 2.4 release broke our ability to query Bulk Operations by status or start_date. Any attempt to do so causes an exception response:

"Item (Magento\\AsynchronousOperations\\Model\\Operation) with the same ID \"0\" already exists.",

Steps to Recreate

POST two bulk loads into Magento REST - confirm that two magento_bulk records are created, and magento_operation records (depending on number of items in the bulk load). You'll see that for each bulk_uuid, the operation_key is sequential, e.g:

<first_bulk_uuid>, 0
<first_bulk_uuid>, 1
<first_bulk_uuid>, 2
...
<second_bulk_uuid>, 0
<second_bulk_uuid>, 1
<second_bulk_uuid>, 2
...

Query the operations via
/rest/V1/bulk/?searchCriteria[filter_groups][0][filters][0][field]=status&searchCriteria[filter_groups][0][filters][0][value]=1&searchCriteria[filter_groups][0][filters][0][condition_type]=eq" (change the status filter value to suit your message status, as long as there are two messages with the same operation_key and same status)

Receive the noted exception response.

Root Cause

\Magento\Framework\Data\Collection\AbstractDb::loadWithFilter
=> \Magento\Framework\Data\Collection::addItem
=> \Magento\Framework\Data\Collection::_getItemId
=> \Magento\AsynchronousOperations\Model\Operation::getId

In 2.4, the getId method of the Operation model was (incorrectly IMO) changed to return the operation_key value. But the operation_key is NOT a unique value - I believe that should have been added as a new field on the model, and that the existing ID field (mirroring the SQL autoincrement unique value) remains used for the array index when building the collection:

    public function addItem(DataObject $item)
    {
        $itemId = $this->_getItemId($item);

        if ($itemId !== null) {
            if (isset($this->_items[$itemId])) {
                //phpcs:ignore Magento2.Exceptions.DirectThrow
                throw new \Exception(
                    'Item (' . get_class($item) . ') with the same ID "' . $item->getId() . '" already exists.'
                );
            }
            $this->_items[$itemId] = $item;
        } else {
            $this->_addItem($item);
        }
        return $this;
    }

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
[TBC] All automated tests passed successfully (all builds are green)

Resolved issues:

resolves [Issue] Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29580: Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release)

…rement 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

…o cover the bug being resolved by this fix.

…collection_loading

m2-assistant · 2020-08-17T14:16:14Z

Hi @moloughlin. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

moloughlin · 2020-08-17T14:17:16Z

@magento run WebAPI

sidolov · 2020-08-17T14:46:33Z

@magento create issue

moloughlin · 2020-08-17T23:20:11Z

@magento run WebAPI Tests

moloughlin · 2020-08-18T05:38:44Z

@magento run all tests

BarnyShergold · 2020-08-21T15:39:28Z

@magento run all tests

BarnyShergold

Fix failing tests

BarnyShergold · 2020-08-21T15:35:44Z

app/code/Magento/AsynchronousOperations/Model/Operation.php

+    /**
+     * @inheritDoc
+     */
+    public function getOperationKey()


Missing return type on prototype

BarnyShergold · 2020-08-21T15:36:06Z

app/code/Magento/AsynchronousOperations/Model/Operation.php

+    /**
+     * @inheritDoc
+     */
+    public function setOperationKey(int $operationKey)


Missing return type on prototype

gabrieldagama · 2020-09-14T10:40:41Z

Hi @moloughlin, thanks for the contribution, can you address the review comments? I will re-run the tests, we will have to address the failing tests if we get them.

gabrieldagama · 2020-09-14T10:40:47Z

@magento run all tests

gabrieldagama · 2020-09-21T14:20:34Z

Hi @moloughlin can you review the failing tests? I think the proposed changes may causing some other problems. Thanks!

ihor-sviziev

✔ Approved.

Failing tests looks not related to changes form this PR.

BrentRobert · 2021-03-16T08:07:11Z

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,


Don't forget this class exists as well in the webapi async module, and needs to be changed there as well.

Magento/WebApiAsync/Model/OperationRepository.php:92

nuzil · 2021-03-23T16:07:55Z

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,


It should not work correctly.
OPERATION_KEY - its a DB key for "operation_key" column.

if you switch OPERATION_KEY to "id", it will stop working, cause $operationId here is a key of array, and in database "id" is an auto_increment value. And in case of multiple operations, will be duplicated for different bulk operations.

Implementing of OPERATION_KEY was done in purpose, for decouple operation keys from database increments and improve performance on saving big operations.

Another issue, it that not all Magento API functionality have a tests coverage, so we overslept broken another part of API

Matthew O'Loughlin added 3 commits August 17, 2020 22:22

Adjustments to tests based on proposed changes, and a new test case t…

28ca7a4

…o cover the bug being resolved by this fix.

Merge remote-tracking branch 'origin/2.4-develop' into fix/operation_…

a25bc9e

…collection_loading

magento-engcom-team added Component: AsynchronousOperations Component: Bulk Release Line: 2.4 labels Aug 17, 2020

ghost added the Progress: pending review label Aug 17, 2020

magento-engcom-team added Partner: Aligent Consulting partners-contribution Pull Request is created by Magento Partner labels Aug 17, 2020

m2-assistant bot mentioned this pull request Aug 17, 2020

[Issue] Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29580

Closed

3 tasks

sidolov added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Aug 17, 2020

Update BulkManagement unit tests to use getOperationKey instead of getId

ac37fdc

BarnyShergold self-assigned this Aug 21, 2020

BarnyShergold requested changes Aug 23, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: pending review labels Aug 23, 2020

Merge branch '2.4-develop' into fix/operation_collection_loading

8b6bba9

engcom-Charlie self-assigned this Sep 25, 2020

engcom-Charlie requested a review from ihor-sviziev January 20, 2021 15:20

ihor-sviziev approved these changes Jan 21, 2021

View reviewed changes

m2-community-project bot added Progress: ready for testing and removed Progress: needs update labels Jan 21, 2021

engcom-Alfa self-assigned this Jan 21, 2021

gabrieldagama added the up for grabs label Jan 22, 2021

gabrieldagama removed the up for grabs label Jan 22, 2021

gabrieldagama added Release Line: 2.5 and removed Release Line: 2.4 labels Mar 2, 2021

BrentRobert suggested changes Mar 16, 2021

View reviewed changes

m2-community-project bot assigned BrentRobert Mar 16, 2021

m2-community-project bot added Progress: needs update and removed Progress: ready for testing labels Mar 16, 2021

nuzil reviewed Mar 23, 2021

View reviewed changes

m2-community-project bot assigned nuzil Mar 23, 2021

m2-community-project bot added Progress: review and removed Progress: needs update labels Mar 23, 2021

ihor-sviziev removed their assignment Mar 23, 2021

nrkapoor assigned kandy Nov 27, 2022

m2-community-project bot added Progress: pending review and removed Progress: review labels Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579

Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579

moloughlin commented Aug 17, 2020 •

edited by m2-assistant bot

Loading

m2-assistant bot commented Aug 17, 2020

moloughlin commented Aug 17, 2020

sidolov commented Aug 17, 2020

moloughlin commented Aug 17, 2020

moloughlin commented Aug 18, 2020

BarnyShergold commented Aug 21, 2020

BarnyShergold left a comment

BarnyShergold Aug 21, 2020

BarnyShergold Aug 21, 2020

gabrieldagama commented Sep 14, 2020

gabrieldagama commented Sep 14, 2020

gabrieldagama commented Sep 21, 2020

ihor-sviziev left a comment

BrentRobert Mar 16, 2021

nuzil Mar 23, 2021

Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579

Are you sure you want to change the base?

Fix ability to query bulk operations across multiple bulk_uuid values (broken by 2.4 release) #29579

Conversation

moloughlin commented Aug 17, 2020 • edited by m2-assistant bot Loading

Description (*)

Steps to Recreate

Root Cause

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Aug 17, 2020

moloughlin commented Aug 17, 2020

sidolov commented Aug 17, 2020

moloughlin commented Aug 17, 2020

moloughlin commented Aug 18, 2020

BarnyShergold commented Aug 21, 2020

BarnyShergold left a comment

Choose a reason for hiding this comment

BarnyShergold Aug 21, 2020

Choose a reason for hiding this comment

BarnyShergold Aug 21, 2020

Choose a reason for hiding this comment

gabrieldagama commented Sep 14, 2020

gabrieldagama commented Sep 14, 2020

gabrieldagama commented Sep 21, 2020

ihor-sviziev left a comment

Choose a reason for hiding this comment

BrentRobert Mar 16, 2021

Choose a reason for hiding this comment

nuzil Mar 23, 2021

Choose a reason for hiding this comment

moloughlin commented Aug 17, 2020 •

edited by m2-assistant bot

Loading