KnpLabs / KnpLabs/php-github-api

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

未关闭
#912 9 条评论 3 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
PHP
星标
2.2k
派生
587
PR 合并指标
30 天内没有已合并 PR

描述

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!

贡献指南

这个仓库没有索引到贡献指南

调研方向

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.

由索引模型根据 Issue 内容生成。

评估

技术栈
github, php
领域
api
Issue 类型
缺陷
难度
4/5
预计耗时
3-5 天
活跃度
停滞
描述清晰度
基本清楚
新手友好度
42/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。