KnpLabs / KnpLabs/php-github-api

ResultPager doesn't merge data correctly for all API's

Abierto
#912 9 comentarios 3 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
PHP
Estrellas
2.2k
Forks
587
Métricas de merge de PR
Sin PR fusionados en 30 d

Descripción

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!

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Línea de trabajo

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.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
github, php
Área
api
Tipo de issue
Error
Dificultad
4/5
Tiempo estimado
3-5 días
Estado de actividad
Estancado
Claridad
Bastante claro
Aptitud para principiantes
42/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.