Fixed bug in EntityTrackerSubscriber when there are more than one bulk requests

Author: pmishev

src/Manager/ConnectionManager.php

@@ -216,16 +216,16 @@ public function commit(bool $forceRefresh = true)
 
         $this->bulkQueries = [];
 
+        if ($this->eventDispatcher) {
+            $this->eventDispatcher->dispatch(new PostCommitEvent($response, $this), Events::POST_COMMIT);
+        }
+
         if ($response['errors']) {
             $errorCount = $this->logBulkRequestErrors($response['items']);
             $e = new BulkRequestException(\sprintf('Bulk request failed with %s error(s)', $errorCount));
             $e->setBulkResponseItems($response['items'], $bulkRequest);
             throw $e;
         }
-
-        if ($this->eventDispatcher) {
-            $this->eventDispatcher->dispatch(new PostCommitEvent($response, $this), Events::POST_COMMIT);
-        }
     }
 
     /**

src/Subscriber/EntityTrackerSubscriber.php

@@ -67,13 +67,18 @@ public function onPostCommit(PostCommitEvent $postCommitEvent)
 
         // Update the ids of persisted entity objects
         foreach ($this->entitiesData[$postCommitEvent->getConnectionName()] as $bulkOperationIndex => $entityData) {
-            $idValue = \current($postCommitEvent->getBulkResponse()['items'][$bulkOperationIndex])['_id'];
-            $idPropertyMetadata = $entityData['metadata']['_id'];
-            $entity = $entityData['entity'];
-            if (DocumentMetadata::PROPERTY_ACCESS_PRIVATE === $idPropertyMetadata['propertyAccess']) {
-                $entity->{$idPropertyMetadata['methods']['setter']}($idValue);
-            } else {
-                $entity->{$idPropertyMetadata['propertyName']} = $idValue;
+            $bulkResponseItem = $postCommitEvent->getBulkResponse()['items'][$bulkOperationIndex];
+            $operation = \key($bulkResponseItem);
+            $bulkResponseItemValue = \current($bulkResponseItem);
+            if (in_array($operation, ['create', 'index']) && !isset($bulkResponseItemValue['error'])) {
+                $idValue = $bulkResponseItemValue['_id'];
+                $idPropertyMetadata = $entityData['metadata']['_id'];
+                $entity = $entityData['entity'];
+                if (DocumentMetadata::PROPERTY_ACCESS_PRIVATE === $idPropertyMetadata['propertyAccess']) {
+                    $entity->{$idPropertyMetadata['methods']['setter']}($idValue);
+                } else {
+                    $entity->{$idPropertyMetadata['propertyName']} = $idValue;
+                }
             }
         }
 

tests/Functional/Subscriber/EntityTrackerSubscriberTest.php

@@ -2,6 +2,7 @@
 
 namespace Sineflow\ElasticsearchBundle\Tests\Functional\Subscriber;
 
+use Sineflow\ElasticsearchBundle\Exception\BulkRequestException;
 use Sineflow\ElasticsearchBundle\Finder\Finder;
 use Sineflow\ElasticsearchBundle\Result\DocumentConverter;
 use Sineflow\ElasticsearchBundle\Tests\AbstractElasticsearchTestCase;
@@ -13,6 +14,50 @@
  */
 class EntityTrackerSubscriberTest extends AbstractElasticsearchTestCase
 {
+    /**
+     * Test executing 2 separate bulk requests, where there's an error in the first one,
+     * to make sure the subscriber properly keeps track of bulk request items.
+     */
+    public function testTwoBulkRequestWithErrorInFirstOne()
+    {
+        $imWithAliases = $this->getIndexManager('customer');
+        $customer1 = new Customer();
+        $customer1->name = 'batman';
+        $customer1->setActive('invalid value');
+        $customer2 = new Customer();
+        $customer2->name = 'robin';
+        $imWithAliases->persist($customer1);
+        $imWithAliases->persist($customer2);
+        try {
+            $imWithAliases->getConnection()->commit();
+        } catch (BulkRequestException $e) {
+            // ignore the exception
+        }
+
+        $customer3 = new Customer();
+        $customer3->name = 'superman';
+        $imWithAliases->persist($customer3);
+        $imWithAliases->getConnection()->commit();
+
+        $this->assertNull($customer1->id, 'id should not have been set');
+        $this->assertNotNull($customer2->id, 'id should have been set');
+        $this->assertNotNull($customer3->id, 'id should have been set');
+
+        // Make sure that the correct id was assigned to the object, not the id of another customer
+        // Get the customer from ES by name
+        $finder = $this->getContainer()->get(Finder::class);
+
+        $docs = $finder->find(['AcmeFooBundle:Customer'], ['query' => ['match' => ['name' => 'robin']]]);
+        $this->assertCount(1, $docs);
+        $retrievedCustomer2 = $docs->current();
+        $this->assertEquals($customer2->id, $retrievedCustomer2->id);
+
+        $docs = $finder->find(['AcmeFooBundle:Customer'], ['query' => ['match' => ['name' => 'superman']]]);
+        $this->assertCount(1, $docs);
+        $retrievedCustomer3 = $docs->current();
+        $this->assertEquals($customer3->id, $retrievedCustomer3->id);
+    }
+
     /**
      * Test populating persisted entity ids after a bulk operation with several operations
      */