Merge pull request #440 from Art4/436-redundant-request

Fix redundant requests `AbstractApi::retrieveData()`

Author: Art4

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Improvement in `Redmine\Client\AbstractApi::retrieveData()` by using `total_count` from redmine response to avoid unnecessary http requests.
 - Behaviour-driven tests are run against Redmine 6.0.2, 5.1.4, 5.0.10.
 
 ### Deprecated

src/Redmine/Api/AbstractApi.php

@@ -320,18 +320,21 @@ protected function retrieveData(string $endpoint, array $params = []): array
 
         $returnData = [];
 
-        $limit = $params['limit'];
+        // Redmine max limit is 100,
+        // @see https://www.redmine.org/projects/redmine/wiki/Rest_api#Collection-resources-and-pagination
+        $redmineLimit = 100;
+        $requestedLimit = $remaininglimit = $params['limit'];
         $offset = $params['offset'];
 
-        while ($limit > 0) {
-            if ($limit > 100) {
-                $_limit = 100;
-                $limit -= 100;
+        while ($remaininglimit > 0) {
+            if ($remaininglimit > $redmineLimit) {
+                $realLimit = $redmineLimit;
+                $remaininglimit -= $redmineLimit;
             } else {
-                $_limit = $limit;
-                $limit = 0;
+                $realLimit = $remaininglimit;
+                $remaininglimit = 0;
             }
-            $params['limit'] = $_limit;
+            $params['limit'] = $realLimit;
             $params['offset'] = $offset;
 
             $this->lastResponse = $this->getHttpClient()->request(HttpFactory::makeRequest(
@@ -344,16 +347,34 @@ protected function retrieveData(string $endpoint, array $params = []): array
 
             $returnData = array_merge_recursive($returnData, $newDataSet);
 
-            $offset += $_limit;
+            // After the first request we know the total_count for this endpoint
+            // so lets use the total_count to correct $requestedLimit to save us
+            // from making unnecessary requests
+            // e.g. total_count = 5 and $requestedLimit = 500 will make only 1 request instead of 2
+            if (isset($newDataSet['total_count']) && $newDataSet['total_count'] < $requestedLimit) {
+                $requestedLimit = $remaininglimit = (int) $newDataSet['total_count'];
+
+                if ($remaininglimit > $redmineLimit) {
+                    $realLimit = $redmineLimit;
+                    $remaininglimit -= $redmineLimit;
+                } else {
+                    $realLimit = $remaininglimit;
+                    $remaininglimit = 0;
+                }
+            }
+
+            $offset += $realLimit;
 
             if (
-                empty($newDataSet) || !isset($newDataSet['limit']) || (
-                    isset($newDataSet['offset']) &&
-                    isset($newDataSet['total_count']) &&
-                    $newDataSet['offset'] >= $newDataSet['total_count']
+                empty($newDataSet)
+                || !isset($newDataSet['limit'])
+                || (
+                    isset($newDataSet['offset'])
+                    && isset($newDataSet['total_count'])
+                    && $newDataSet['offset'] >= $newDataSet['total_count']
                 )
             ) {
-                $limit = 0;
+                $remaininglimit = 0;
             }
         }
 

tests/Unit/Api/AbstractApiTest.php

@@ -398,6 +398,48 @@ public static function retrieveDataData(): array
         ];
     }
 
+    public function testRetrieveDataWith100ResultsMakes1Request(): void
+    {
+        $response1 = $this->createStub(Response::class);
+        $response1->method('getContentType')->willReturn('application/json');
+        $response1->method('getContent')->willReturn('{"total_count":100,"offset":0,"limit":100,"data":[]}');
+
+        $client = $this->createMock(HttpClient::class);
+        $client->expects($this->once())->method('request')->willReturn($response1);
+
+        $api = new class ($client) extends AbstractApi {};
+
+        $method = new ReflectionMethod($api, 'retrieveData');
+        $method->setAccessible(true);
+
+        $method->invoke($api, '/data.json', ['limit' => 101]);
+    }
+
+    public function testRetrieveDataWith250ResultsMakes3Requests(): void
+    {
+        $response1 = $this->createStub(Response::class);
+        $response1->method('getContentType')->willReturn('application/json');
+        $response1->method('getContent')->willReturn('{"total_count":250,"offset":0,"limit":100,"data":[]}');
+
+        $response2 = $this->createStub(Response::class);
+        $response2->method('getContentType')->willReturn('application/json');
+        $response2->method('getContent')->willReturn('{"total_count":250,"offset":100,"limit":100,"data":[]}');
+
+        $response3 = $this->createStub(Response::class);
+        $response3->method('getContentType')->willReturn('application/json');
+        $response3->method('getContent')->willReturn('{"total_count":250,"offset":200,"limit":100,"data":[]}');
+
+        $client = $this->createMock(HttpClient::class);
+        $client->expects($this->exactly(3))->method('request')->willReturnOnConsecutiveCalls($response1, $response2, $response3);
+
+        $api = new class ($client) extends AbstractApi {};
+
+        $method = new ReflectionMethod($api, 'retrieveData');
+        $method->setAccessible(true);
+
+        $method->invoke($api, '/data.json', ['limit' => 301]);
+    }
+
     /**
      * @dataProvider getRetrieveDataToExceptionData
      */