Improvements for checking RateLimit during synchronization

ConductionNL · Nov 29, 2024 · 927e629 · 927e629
1 parent 6977d12
commit 927e629
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 67 deletions.
diff --git a/lib/Controller/SynchronizationsController.php b/lib/Controller/SynchronizationsController.php
@@ -2,6 +2,7 @@
 
 namespace OCA\OpenConnector\Controller;
 
+use GuzzleHttp\Exception\GuzzleException;
 use OCA\OpenConnector\Service\ObjectService;
 use OCA\OpenConnector\Service\SearchService;
 use OCA\OpenConnector\Service\SynchronizationService;
@@ -15,6 +16,8 @@
 use OCP\IRequest;
 use Exception;
 use OCP\AppFramework\Db\DoesNotExistException;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
 
 class SynchronizationsController extends Controller
 {
@@ -213,33 +216,36 @@ public function logs(int $id): JSONResponse
         }
     }
 
-    /**
-     * Tests a synchronization
-     *
-     * This method tests a synchronization without persisting anything to the database.
-     *
-     * @NoAdminRequired
-     * @NoCSRFRequired
-     *
-     * @param int $id The ID of the synchronization
-     *
-     * @return JSONResponse A JSON response containing the test results
-     *
-     * @example
-     * Request:
-     * empty POST
-     *
-     * Response:
-     * {
-     *     "resultObject": {
-     *         "fullName": "John Doe",
-     *         "userAge": 30,
-     *         "contactEmail": "[email protected]"
-     *     },
-     *     "isValid": true,
-     *     "validationErrors": []
-     * }
-     */
+	/**
+	 * Tests a synchronization
+	 *
+	 * This method tests a synchronization without persisting anything to the database.
+	 *
+	 * @NoAdminRequired
+	 * @NoCSRFRequired
+	 *
+	 * @param int $id The ID of the synchronization
+	 *
+	 * @return JSONResponse A JSON response containing the test results
+	 * @throws GuzzleException
+	 * @throws ContainerExceptionInterface
+	 * @throws NotFoundExceptionInterface
+	 *
+	 * @example
+	 * Request:
+	 * empty POST
+	 *
+	 * Response:
+	 * {
+	 *     "resultObject": {
+	 *         "fullName": "John Doe",
+	 *         "userAge": 30,
+	 *         "contactEmail": "[email protected]"
+	 *     },
+	 *     "isValid": true,
+	 *     "validationErrors": []
+	 * }
+	 */
     public function test(int $id): JSONResponse
     {
         try {
@@ -250,17 +256,26 @@ public function test(int $id): JSONResponse
 
         // Try to synchronize
         try {
-            $logAndContractArray = $this->synchronizationService->synchronize(synchronization: $synchronization, isTest: true);
+            $logAndContractArray = $this->synchronizationService->synchronize(
+				synchronization: $synchronization,
+				isTest: true
+			);
+
             // Return the result as a JSON response
             return new JSONResponse(data: $logAndContractArray, statusCode: 200);
         } catch (Exception $e) {
-            // If synchronizaiton fails, return an error response
-            return new JSONResponse([
-                'error' => 'Synchronization error',
-                'message' => $e->getMessage()
-            ], 400);
-        }
+			// Check if getHeaders method exists and use it if available
+			$headers = method_exists($e, 'getHeaders') ? $e->getHeaders() : null;
 
-        return new JSONResponse($resultFromTest, 200);
+            // If synchronization fails, return an error response
+            return new JSONResponse(
+				data: [
+					'error' => 'Synchronization error',
+					'message' => $e->getMessage()
+				],
+				statusCode: $e->getCode() ?? 400,
+				headers: $headers
+			);
+        }
     }
-}
+}
diff --git a/lib/Cron/ActionTask.php b/lib/Cron/ActionTask.php
@@ -8,7 +8,10 @@
 use OCP\BackgroundJob\TimedJob;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
+use OCP\DB\Exception;
+use Psr\Container\ContainerExceptionInterface;
 use Psr\Container\ContainerInterface;
+use Psr\Container\NotFoundExceptionInterface;
 use Symfony\Component\Uid\Uuid;
 use DateInterval;
 use DateTime;
@@ -55,6 +58,9 @@ public function __construct(
 	 * @param $argument
 	 *
 	 * @return JobLog|void
+	 * @throws Exception
+	 * @throws ContainerExceptionInterface
+	 * @throws NotFoundExceptionInterface
 	 */
     public function run($argument)
     {
@@ -101,7 +107,7 @@ public function run($argument)
         // Update the job
 		$nextRun = new DateTime();
 		if (isset($result['nextRun']) === true) {
-			$nextRun = $result['nextRun'];
+			$nextRun = DateTime::createFromFormat('U', $result['nextRun']);
 		}
         $job->setLastRun(new DateTime());
         $job->setNextRun($nextRun);

diff --git a/lib/Service/SynchronizationService.php b/lib/Service/SynchronizationService.php
@@ -146,7 +146,6 @@ public function synchronize(Synchronization $synchronization, ?bool $isTest = fa
 		}
 
 		if (isset($rateLimitException) === true) {
-			var_dump('test');
 			throw new TooManyRequestsHttpException(
 				message: $rateLimitException->getMessage(),
 				code: 429,
@@ -445,17 +444,13 @@ public function getAllObjectsFromApi(Synchronization $synchronization, ?bool $is
         $objects = [];
         $source = $this->sourceMapper->find(id: $synchronization->getSourceId());
 
-		if ($source->getRateLimitRemaining() !== null && $source->getRateLimitRemaining() <= 0) {
+		if ($source->getRateLimitRemaining() !== null && $source->getRateLimitReset() !== null
+			&& $source->getRateLimitRemaining() <= 0 && $source->getRateLimitReset() > time()
+		) {
 			throw new TooManyRequestsHttpException(
 				message: "Rate Limit on Source has been exceeded. Canceling synchronization...",
 				code: 429,
-				headers: [
-					'X-RateLimit-Limit' => [(string) $source->getRateLimitLimit()],
-					'X-RateLimit-Remaining' => [(string) $source->getRateLimitRemaining()],
-					'X-RateLimit-Reset' => [(string) $source->getRateLimitReset()],
-					'X-RateLimit-Used' => ["0"],
-					'X-RateLimit-Window' => [(string) $source->getRateLimitWindow()],
-				]
+				headers: $this->getRateLimitHeaders($source)
 			);
 		}
 
@@ -486,13 +481,7 @@ public function getAllObjectsFromApi(Synchronization $synchronization, ?bool $is
 				throw new TooManyRequestsHttpException(
 					message: "Stopped sync because rate limit on Source has been exceeded.",
 					code: 429,
-					headers: [
-						'X-RateLimit-Limit' => [(string) $source->getRateLimitLimit()],
-						'X-RateLimit-Remaining' => [(string) $source->getRateLimitRemaining()],
-						'X-RateLimit-Reset' => [(string) $source->getRateLimitReset()],
-						'X-RateLimit-Used' => ["1"],
-						'X-RateLimit-Window' => [(string) $source->getRateLimitWindow()],
-					]
+					headers: $this->getRateLimitHeaders($source)
 				);
 			}
 		}
@@ -532,13 +521,7 @@ public function getAllObjectsFromApi(Synchronization $synchronization, ?bool $is
 					throw new TooManyRequestsHttpException(
 						message: "Stopped sync because rate limit on Source has been exceeded.",
 						code: 429,
-						headers: [
-							'X-RateLimit-Limit' => [(string) $source->getRateLimitLimit()],
-							'X-RateLimit-Remaining' => [(string) $source->getRateLimitRemaining()],
-							'X-RateLimit-Reset' => [(string) $source->getRateLimitReset()],
-							'X-RateLimit-Used' => ["1"],
-							'X-RateLimit-Window' => [(string) $source->getRateLimitWindow()],
-						]
+						headers: $this->getRateLimitHeaders($source)
 					);
 				}
 			}
@@ -563,13 +546,7 @@ public function getAllObjectsFromApi(Synchronization $synchronization, ?bool $is
 						throw new TooManyRequestsHttpException(
 							message: "Stopped sync because rate limit on Source has been exceeded.",
 							code: 429,
-							headers: [
-								'X-RateLimit-Limit' => [(string) $source->getRateLimitLimit()],
-								'X-RateLimit-Remaining' => [(string) $source->getRateLimitRemaining()],
-								'X-RateLimit-Reset' => [(string) $source->getRateLimitReset()],
-								'X-RateLimit-Used' => ["1"],
-								'X-RateLimit-Window' => [(string) $source->getRateLimitWindow()],
-							]
+							headers: $this->getRateLimitHeaders($source)
 						);
 					}
 				}
@@ -604,6 +581,17 @@ public function getAllObjectsFromApi(Synchronization $synchronization, ?bool $is
         return $objects;
     }
 
+	private function getRateLimitHeaders($source): array
+	{
+		return [
+			'X-RateLimit-Limit' => $source->getRateLimitLimit(),
+			'X-RateLimit-Remaining' => $source->getRateLimitRemaining(),
+			'X-RateLimit-Reset' => $source->getRateLimitReset(),
+			'X-RateLimit-Used' => 0,
+			'X-RateLimit-Window' => $source->getRateLimitWindow(),
+		];
+	}
+
 	/**
 	 * Determines the next API endpoint based on a provided next.
 	 *