diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d63116..38abf75e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/Redmine/Api/AbstractApi.php b/src/Redmine/Api/AbstractApi.php index 57068976..9eb7c497 100644 --- a/src/Redmine/Api/AbstractApi.php +++ b/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; } } diff --git a/tests/Unit/Api/AbstractApiTest.php b/tests/Unit/Api/AbstractApiTest.php index cc0fc68c..d886960c 100644 --- a/tests/Unit/Api/AbstractApiTest.php +++ b/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 */