From 35167b3fbb94901747af5c0e8683f97053b22821 Mon Sep 17 00:00:00 2001
From: adunn49 <andrew.dunn@silverstripe.com>
Date: Thu, 31 Oct 2024 11:17:44 +1300
Subject: [PATCH] Add unit test for checking handling of indexing errors

---
 .../EnterpriseSearchService.php               |  2 +-
 tests/DataObject/DataObjectDocumentTest.php   |  5 ++
 .../EnterpriseSearchServiceTest.php           | 64 +++++++++++++++----
 3 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/src/Service/EnterpriseSearch/EnterpriseSearchService.php b/src/Service/EnterpriseSearch/EnterpriseSearchService.php
index a1faddc..2a94d4a 100644
--- a/src/Service/EnterpriseSearch/EnterpriseSearchService.php
+++ b/src/Service/EnterpriseSearch/EnterpriseSearchService.php
@@ -532,7 +532,7 @@ protected function handleErrorAndContinue(?array $responseBody, string $indexNam
         }
 
         foreach ($responseBody as $key => $response) {
-            if (!$response['errors']) {
+            if (!isset($response['errors']) || sizeof($response['errors']) === 0) {
                 continue;
             }
 
diff --git a/tests/DataObject/DataObjectDocumentTest.php b/tests/DataObject/DataObjectDocumentTest.php
index 8f9b5ed..66f375c 100644
--- a/tests/DataObject/DataObjectDocumentTest.php
+++ b/tests/DataObject/DataObjectDocumentTest.php
@@ -598,6 +598,11 @@ public function testEvents(): void
         $mock->expects($this->exactly(2))
             ->method('markIndexed');
 
+        $dataObject = DataObjectFakeVersioned::create(['Title' => 'Document']);
+        $dataObject->publishSingle();
+
+        $mock->setDataObject($dataObject);
+
         $mock->onAddToSearchIndexes(DocumentAddHandler::BEFORE_ADD);
         $mock->onAddToSearchIndexes(DocumentAddHandler::AFTER_ADD);
         $mock->onRemoveFromSearchIndexes(DocumentRemoveHandler::BEFORE_REMOVE);
diff --git a/tests/Service/EnterpriseSearch/EnterpriseSearchServiceTest.php b/tests/Service/EnterpriseSearch/EnterpriseSearchServiceTest.php
index 4ae7d90..180d882 100644
--- a/tests/Service/EnterpriseSearch/EnterpriseSearchServiceTest.php
+++ b/tests/Service/EnterpriseSearch/EnterpriseSearchServiceTest.php
@@ -8,7 +8,10 @@
 use GuzzleHttp\Handler\MockHandler;
 use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Psr7\Response;
+use Monolog\Logger;
 use Page;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 use ReflectionMethod;
 use SilverStripe\Core\Environment;
 use SilverStripe\Core\Injector\Injector;
@@ -44,6 +47,8 @@ class EnterpriseSearchServiceTest extends SearchServiceTest
 
     protected ?MockHandler $mock;
 
+    protected ?MockObject $mockLogger;
+
     protected EnterpriseSearchService $searchService;
 
     public function testMaxDocumentSize(): void
@@ -1243,36 +1248,60 @@ public function testAddDocumentsEmpty(): void
         $this->assertEqualsCanonicalizing([], $resultIds);
     }
 
-    public function testAddDocumentsError(): void
+
+    /**
+     * Test that when an error occurs, the remaining documents are processed and errors are logged.
+     */
+    public function testAddDocumentsWithErrorAndContinue(): void
     {
         $documentOne = $this->objFromFixture(DataObjectFake::class, 'one');
-        $documents = [DataObjectDocument::create($documentOne)];
+        $documentTwo = $this->objFromFixture(DataObjectFake::class, 'two');
+        $documentThree = $this->objFromFixture(DataObjectFake::class, 'three');
 
-        $this->expectExceptionMessage('Testing failure');
+        $documents = [
+            DataObjectDocument::create($documentOne),
+            DataObjectDocument::create($documentTwo),
+            DataObjectDocument::create($documentThree)
+        ];
 
         // Valid headers
         $headers = [
             'Content-Type' => 'application/json;charset=utf-8',
         ];
-        // Body content containing errors
-        $body = json_encode([
+
+        $response = json_encode([
             [
-                'id' => 'doc-123',
+                'id' => 'silverstripe_cms_model_sitetree_1',
                 'errors' => [
-                    'Testing failure',
+                    'Invalid field value=> Value \'Test\' cannot be parsed as a float',
                 ],
             ],
+            [
+                'id'=> 'silverstripe_cms_model_sitetree_2',
+                'errors'=> [],
+            ],
+            [
+                'id'=> 'silverstripe_cms_model_sitetree_3',
+                'errors'=> [],
+            ]
         ]);
 
         // Append this mock response to our stack
-        $this->mock->append(new Response(200, $headers, $body));
+        $this->mock->append(new Response(200, $headers, $response));
 
-        // We expect this to throw an Exception
-        $this->searchService->addDocuments($documents);
+        $error = '[SEARCH SERVICE ERROR]: {"Message":"Unable to index a document to content"'
+            . ',"Title":"Dataobject one","URL":"","ClassName":"SilverStripe\\\\SearchService\\\\'
+            . 'Tests\\\\Fake\\\\DataObjectFake","ID":1,"Error":"Invalid field value=> Value \'Test\' cannot'
+            . ' be parsed as a float"}';
 
-        // And make sure nothing is left in our Response Stack. This would indicate that every Request we expect to make
-        // has been made
-        $this->assertEquals(0, $this->mock->count());
+        $this->mockLogger->expects($this->once())
+            ->method('error')
+            ->with($error);
+
+        $processedIds = $this->searchService->addDocuments($documents);
+
+        // check that all docs were processed, indicating no exceptions were thrown
+        $this->assertCount(3, $processedIds);
     }
 
     public function testAddDocument(): void
@@ -1651,6 +1680,15 @@ protected function setUp(): void
         $documentBuilder = Injector::inst()->get(DocumentBuilder::class);
 
         $this->searchService = EnterpriseSearchService::create($elasticClient, $indexConfiguration, $documentBuilder);
+
+
+        // mock logger so we can check errors were logged
+        $this->mockLogger = $this->getMockBuilder(Logger::class)
+            ->setConstructorArgs(['error-log'])
+            ->onlyMethods(['error'])
+            ->getMock();
+
+        Injector::inst()->registerService($this->mockLogger, LoggerInterface::class);
     }
 
 }