N°8856 - Async retries of WebRequest not working properly

SendWebRequest.php

@@ -77,7 +77,7 @@ public static function Init()
 	 */
 	public static function AddToQueue(WebRequest $oWebRequest, $oLog = null)
 	{
-		$oNew = new static();
+		$oNew = new SendWebRequest();
 		if ($oLog)
 		{
 			$oNew->Set('event_id', $oLog->GetKey());
@@ -102,7 +102,8 @@ public function DoProcess()
 
 		$oSenderService = WebRequestSender::GetInstance();
 		$aResult = $oSenderService->Send($oWebRequest, $aIssues, $oLog, WebRequestSender::ENUM_SEND_MODE_SYNC);
-		switch ($aResult['sender_status'])
+		$iStatus = $aResult['sender_status'] ?? "";
+		switch ($iStatus)
 		{
 			case WebRequestSender::ENUM_SEND_STATE_OK:
 				return 'Sent';
@@ -111,7 +112,10 @@ public function DoProcess()
 				return 'Whoops! Seems like a bug occurred, the request should be sent in synchronous mode';
 
 			case WebRequestSender::ENUM_SEND_STATE_ERROR:
-				return 'Failed: '.implode(', ', $aIssues);
+				throw new Exception('Failed: '.implode(', ', $aIssues));
+
+			default:
+				return "Whoops! unexpected behavior (sender_status: $iStatus)";
 		}
 	}
-}
+}

module.combodo-webhook-integration.php

@@ -5,7 +5,7 @@
 //
 //
 SetupWebPage::AddModule(__FILE__, // Path to the current file, all other file names are relative to the directory containing this file
-	'combodo-webhook-integration/1.4.6',
+	'combodo-webhook-integration/1.4.7',
 	array(
 		//
 		'label' => 'Webhook integrations',

src/Service/WebRequestSender.php

@@ -62,7 +62,7 @@ class WebRequestSender
 
 	public static function SetMockDoPostRequest(bool $mockDoPostRequest): void
 	{
-		static::$mockDoPostRequest = $mockDoPostRequest;
+		self::$mockDoPostRequest = $mockDoPostRequest;
 	}
 	/**
 	 * Return the singleton instance for this class
@@ -71,12 +71,16 @@ public static function SetMockDoPostRequest(bool $mockDoPostRequest): void
 	 */
 	public static function GetInstance()
 	{
-		if(static::$oInstance === null)
+		if(self::$oInstance === null)
 		{
-			static::$oInstance = new static();
+			self::$oInstance = new WebRequestSender();
 		}
 
-		return static::$oInstance;
+		return self::$oInstance;
+	}
+
+	public static function SetInstance(?WebRequestSender $oInstance) : void {
+		self::$oInstance = $oInstance;
 	}
 
 	/**
@@ -93,22 +97,20 @@ public function Send(WebRequest $oRequest, &$aIssues, $oLog = null, $sForcedSend
 	{
 		if($sForcedSendMode === static::ENUM_SEND_MODE_SYNC)
 		{
-			$aResult = $this->SendSynchronously($oRequest, $aIssues, $oLog);
-		}
-		elseif($sForcedSendMode === static::ENUM_SEND_MODE_ASYNC)
-		{
-			$aResult = $this->SendAsynchronously($oRequest, $aIssues, $oLog);
+			return $this->SendSynchronously($oRequest, $aIssues, $oLog);
 		}
-		elseif(MetaModel::GetModuleSetting('combodo-webhook-integration', 'prefer_asynchronous', (static::DEFAULT_SEND_MODE === static::ENUM_SEND_MODE_ASYNC)))
+
+		if($sForcedSendMode === static::ENUM_SEND_MODE_ASYNC)
 		{
-			$aResult = $this->SendAsynchronously($oRequest, $aIssues, $oLog);
+			return $this->SendAsynchronously($oRequest, $aIssues, $oLog);
 		}
-		else
+
+		if(MetaModel::GetModuleSetting('combodo-webhook-integration', 'prefer_asynchronous', (static::DEFAULT_SEND_MODE === static::ENUM_SEND_MODE_ASYNC)))
 		{
-			$aResult = $this->SendSynchronously($oRequest, $aIssues, $oLog);
+			return $this->SendAsynchronously($oRequest, $aIssues, $oLog);
 		}
 
-		return $aResult;
+		return $this->SendSynchronously($oRequest, $aIssues, $oLog);
 	}
 
 	/**
@@ -205,4 +207,4 @@ private function DoPostRequest($sUrl, $aData, $sOptionnalHeaders = null, &$aResp
 			return utils::DoPostRequest($sUrl, $aData, $sOptionnalHeaders, $aResponseHeaders, $aCurlOptions);
 		}
 	}
-}
+}

src/Service/WebRequestService.php

@@ -14,11 +14,11 @@ protected function __construct()
 
 	final public static function GetInstance(): WebRequestService
 	{
-		if (!isset(static::$oInstance)) {
-			static::$oInstance = new static();
+		if (!isset(self::$oInstance)) {
+			self::$oInstance = new WebRequestService();
 		}
 
-		return static::$oInstance;
+		return self::$oInstance;
 	}
 
 	/**
@@ -34,7 +34,7 @@ final public static function SetInstance(WebRequestService $oInstance)
 	 */
 	final public static function ResetInstance()
 	{
-		self::$oInstance = new static();
+		self::$oInstance = new WebRequestService();
 	}
 
 	public function ObfuscateRawHeader(string $sRawHeaders) : string {

tests/php-unit-tests/SendWebRequestTest.php

@@ -0,0 +1,89 @@
+<?php
+
+namespace Combodo\iTop\Core\Test;
+
+namespace Combodo\iTop\Core\Test;
+
+use Combodo\iTop\Core\WebRequest;
+use Combodo\iTop\Service\WebRequestSender;
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use SendWebRequest;
+
+/**
+ * @runTestsInSeparateProcesses
+ * @preserveGlobalState disabled
+ * @backupGlobals disabled
+ */
+class SendWebRequestTest extends ItopDataTestCase
+{
+	private WebRequestSender $oWebRequestSender;
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+		$this->RequireOnceItopFile('env-production/combodo-webhook-integration/vendor/autoload.php');
+
+		$this->oWebRequestSender = $this->createMock(WebRequestSender::class);
+		WebRequestSender::SetInstance($this->oWebRequestSender);
+	}
+
+	protected function tearDown(): void
+	{
+		parent::tearDown();
+		WebRequestSender::SetInstance(null);
+	}
+
+	public static function DoProcessProvider()
+	{
+		return [
+			'cornercase error that should not happen' => [666, "Whoops! unexpected behavior (sender_status: 666)"],
+			'sent ENUM_SEND_STATE_OK' => [0, 'Sent'],
+			'sent ENUM_SEND_STATE_PENDING issue' => [1, 'Whoops! Seems like a bug occurred, the request should be sent in synchronous mode'],
+		];
+	}
+
+	/**
+	 * @dataProvider DoProcessProvider
+	 */
+	public function testDoProcessOK($iSenderStatus, $expected)
+	{
+		/** @var SendWebRequest $oSendWebRequest */
+		$oWebRequest = new WebRequest('http://127.0.0.1');
+		$aParams = [
+			'request' => serialize($oWebRequest),
+		];
+		$oSendWebRequest = $this->GivenObject(SendWebRequest::class, $aParams);
+
+		$aResult = [
+			'sender_status' => $iSenderStatus,
+		];
+
+		$this->oWebRequestSender->expects(self::once())
+			->method("Send")
+		->willReturn($aResult);
+
+		self::assertEquals($expected, $oSendWebRequest->DoProcess());
+	}
+
+	public function testDoProcessFailAndMakesureRetryWillBePossible()
+	{
+		/** @var SendWebRequest $oSendWebRequest */
+		$oWebRequest = new WebRequest('http://127.0.0.1');
+		$aParams = [
+			'request' => serialize($oWebRequest),
+		];
+		$oSendWebRequest = $this->GivenObject(SendWebRequest::class, $aParams);
+
+		$sExceptionThrownMsg = 'Error while sending request to webhook: BLABLA';
+		$this->oWebRequestSender->expects(self::once())
+			->method("Send")
+			->will(
+				$this->returnCallback(function ($oWebRequest, &$aIssues) use ($sExceptionThrownMsg) {
+					$aIssues []= $sExceptionThrownMsg;
+					return ['sender_status' => WebRequestSender::ENUM_SEND_STATE_ERROR ];
+				}));
+
+		$this->expectExceptionMessage("Failed: $sExceptionThrownMsg");
+		$oSendWebRequest->DoProcess();
+	}
+}