KnpLabs / KnpLabs/php-github-api
ResultPager doesn't merge data correctly for all API's
- Vorherrschende Sprache
- PHP
- Sterne
- 2.2k
- Forks
- 587
- PR-Merge-Kennzahlen
- Keine gemergten PRs in 30 T.
Beschreibung
Hello!
## The Problem (Description)
This is technically a duplicate of #597 from 2017, but I wanted to add in details and present some possible solutions.
We're using this library on [ChipperCI](https://chipperci.com), some customers noted that we aren't correctly listing all of the repositories our GitHub App is installed into when they have a large number of repositories.
Digging in, we realized only the **last page** of results were being returned back when using the `ResultPager::fetchAll()` method on results that had more than one page.
This only happens on certain API calls (I've only seen it on the Apps API so far - **listed below**).
## The Issue
This is due to the use of `array_merge()` to merge resulting arrays back:
https://github.com/KnpLabs/php-github-api/blob/2.x/lib/Github/ResultPager.php#L92-L96:
```php
while ($this->hasNext()) {
$next = $this->fetchNext();
if ($isSearch) {
$result = array_merge($result, $next['items']);
} else {
$result = array_merge($result, $next); // <-- Specifically here
}
}
```
Here's an example call into the App's api that results in the issue:
```php
$github = new Client(new Builder, 'machine-man-preview');
$github->authenticate($userOAuthToken, Client::AUTH_HTTP_TOKEN);
$resultFetcher = new ResultPager($github);
$installations = $resultFetcher->fetchAll($github->currentUser(), 'installations');
```
A result object from one API call is (I set it to `1` per page to test this out, but it happens when there are more than 100 results for any API with results like this) - converted from JSON to PHP:
```php
// API call number one, 1 per page, when there are 2 results total:
$result = [
'total_count' => 2,
'installations' => [
['id' => 12345 /* and so on */],
]
]
// API call number two, again using 1 per page, when there are 2 results total:
$next = [
'total_count' => 2,
'installations' => [
['id' => 678910 /* and so on */],
]
]
```
If you `array_merge()` these, PHP will over-write the the first array with the second, as their associative keys match!
```php
$finalResult = array_merge($result, $next);
// since the arrays have matching keys, the $next array over-writes everything in the $result array
// and we effectively only get the last page of results for any set of results over the 100 results per page
// limit set by GitHub's API
$finalResult === $next; // true
array_diff($finalResult, $next); // yields an empty result [], they are the same
```
## Possible Solutions
**A first idea I had:**
The solution may be API class extending `AbstractAPI` to know if there is a key to be used there.
So far I can only find examples from the App's api, and from searching the source of that page, only these 3 endpoints appear to use `total_count` (and the other key for `repositories` / `installations`) in the returned results:
1. https://docs.github.com/en/rest/reference/apps#list-repositories-accessible-to-the-user-access-token
- e.g. `GET /user/installations/{installation_id}/repositories`
2. https://docs.github.com/en/rest/reference/apps#list-repositories-accessible-to-the-app-installation
- e.g. `GET /installation/repositories`
3. https://docs.github.com/en/rest/reference/apps#list-app-installations-accessible-to-the-user-access-token
- e.g. `GET /user/installations`
> However, I've checked some GitHub issues here that hint that this may be an issue elsewhere (and actually, this bug is open too: https://github.com/KnpLabs/php-github-api/issues/597).
The keys in the `$result` / `$next` array are `repositories` or `installations`.
So perhaps something like this:
```php
// File: AbstractAPI
protected $resultKey;
public function getResultKey()
{
return $this->resultKey;
}
```
And then in `ResultPager`:
```php
while ($this->hasNext()) {
$next = $this->fetchNext();
if ($isSearch) {
$result = array_merge($result, $next['items']);
} elseif (! $this->getResultKey()) {
$result = array_merge($result, $next[$this->getResultKey()]);
} else {
$result = array_merge($result, $next);
}
}
```
This would likely need to be used not just for certain API's (e.g. the App's api) but the specific endpoints that need it 😅
---
Hopefully these extra details can help pin-point the issue! In the meantime, I'll do a similar work-around as the others in [this (duplicate but less detailed) issue](https://github.com/KnpLabs/php-github-api/issues/597).
Let me know if I can clarify anything!
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Rechercherichtung
Start in lib/Github/ResultPager.php at the fetchAll pagination loop and reproduce the issue with the multi-page Apps API response described here. Compare the search and non-search merge paths, then verify that all repositories or installations from every page are retained without changing the existing result shape.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- github, php
- Bereich
- api
- Issue-Typ
- Bug
- Schwierigkeit
- 4/5
- Geschätzter Aufwand
- 3-5 Tage
- Aktivitätsstatus
- Veraltet
- Klarheit
- Größtenteils klar
- Anfängerfreundlichkeit
- 42/100