Open fideloper opened 4 years ago
@GrahamCampbell This could effect https://github.com/KnpLabs/php-github-api/pull/907 as well. Let me know what you think!
I think we should report this as a bug to GitHub actually. It looks like the developer who implemented the installations API was not aware of their conventions.
Inline hack that I used to get fetchAll
working for the listRepositories
command.
Mileage may vary.
$pager = new class($client) extends ResultPager {
protected function callApi(ApiInterface $api, $method, array $parameters)
{
return parent::callApi($api, $method, $parameters)['repositories'];
}
public function fetchNext()
{
return parent::fetchNext()['repositories'];
}
};
Right on. I did the following since I have one call that needs repositories
and one that needs installations
:
<?php
namespace App\Api\GitHub;
use Github\Api\ApiInterface;
use Github\Api\Search;
use Github\ResultPager as BaseResultPager;
class ResultPager extends BaseResultPager
{
protected $resultKey;
/**
* @param $key
* @return $this
*/
public function setResultKey($key)
{
$this->resultKey = $key;
return $this;
}
/**
* {@inheritdoc}
*/
public function fetchAll(ApiInterface $api, $method, array $parameters = [])
{
$isSearch = $api instanceof Search;
// get the perPage from the api
$perPage = $api->getPerPage();
// set parameters per_page to GitHub max to minimize number of requests
$api->setPerPage(100);
try {
$result = $this->callApi($api, $method, $parameters);
$this->postFetch();
if ($this->resultKey) {
$result = $result[$this->resultKey];
}
if ($isSearch) {
$result = isset($result['items']) ? $result['items'] : $result;
}
while ($this->hasNext()) {
$next = $this->fetchNext();
if ($isSearch) {
$result = array_merge($result, $next['items']);
} elseif ($this->resultKey) {
$result = array_merge($result, $next[$this->resultKey]);
} else {
$result = array_merge($result, $next);
}
}
} finally {
// restore the perPage
$api->setPerPage($perPage);
}
return $result;
}
}
Example call to this result fetcher:
$github = new Client(new Builder, 'machine-man-preview');
$github->authenticate($oAuthToken, Client::AUTH_HTTP_TOKEN);
$resultFetcher = new GitHubResultPager($github);
$resultFetcher
->setResultKey('installations')
->fetchAll($github->currentUser(), 'installations');
@GrahamCampbell I opened a ticket with GitHub but haven't heard a thing back :D
From GitHub:
Hi there Chris,
Thanks for writing in to ask about this - and I'm sorry for the time we've kept you waiting for a response.
Returning those results in a nested array with a total_count is the intended behaviour for these endpoints. This isn't the only endpoint to behave this way, you'll notice that our Search API also returns a nested array of results.
We've used this format on some endpoints where it's particuarly useful to obtain a total count. It would be a breaking change to roll this out to more endpoints which does mean things can be a bit inconsistent.
I can understand how this could cause trouble with libraries that attempt to treat all REST API responses the same but it won't be possible to change the format of these responses. I hope this answers your question and helps move the discussion forward in the issue you linked.
Best regards, Steve
Just updated this library and found that the previous solution posted by @fideloper doesn't work anymore.
Here is an updated class for version 3.2.0 in case somebody will need it too:
<?php
namespace App\Api\Github;
use Github\Api\AbstractApi;
use Github\ResultPager as BaseResultPager;
class ResultPager extends BaseResultPager
{
private $resultKey;
public function setResultKey($key)
{
$this->resultKey = $key;
return $this;
}
public function fetch(AbstractApi $api, string $method, array $parameters = []): array
{
return $this->prepareResult(parent::fetch($api, $method, $parameters));
}
protected function get(string $key): array
{
return $this->prepareResult(parent::get($key));
}
private function prepareResult(array $result): array
{
if ($this->resultKey && $result) {
$result['items'] = $result[$this->resultKey] ?? [];
}
return $result;
}
}
Usage:
$repositories = (new ResultPager($client))
->setResultKey('repositories')
->fetchAll($client->apps(), 'listRepositories');
Just updated this library and found that the previous solution posted by @fideloper doesn't work anymore.
Here is an updated class for version 3.2.0 in case somebody will need it too:
<?php namespace App\Api\Github; use Github\Api\AbstractApi; use Github\ResultPager as BaseResultPager; class ResultPager extends BaseResultPager { private $resultKey; public function setResultKey($key) { $this->resultKey = $key; return $this; } public function fetch(AbstractApi $api, string $method, array $parameters = []): array { return $this->prepareResult(parent::fetch($api, $method, $parameters)); } protected function get(string $key): array { return $this->prepareResult(parent::get($key)); } private function prepareResult(array $result): array { if ($this->resultKey && $result) { $result['items'] = $result[$this->resultKey] ?? []; } return $result; } }
Usage:
$repositories = (new ResultPager($client)) ->setResultKey('repositories') ->fetchAll($client->apps(), 'listRepositories');
Thank you! it works like a charm.
3.5.1
The API results (when I get a list of installations and repositories) are:
$github = new Client(new Builder, 'machine-man-preview');
$github->authenticate('<user oauth token>', AuthMethod::ACCESS_TOKEN);
$resultFetcher = new ResultPager($github);
$installations = $resultFetcher->fetchAll($github->currentUser(), 'installations');
// RESULTS LOOK LIKE THIS:
//[
// 'total_count' => 100,
// 'installations' => [...],
//]
// Get GitHub App token for JWT auth
$token = AppsFacade::newInstallationToken($installation['id']);
$github = new Client(new Builder, 'machine-man-preview');
$github->authenticate($token['token'], AuthMethod::JWT);
$resultFetcher = new ResultPager($github);
$response = $resultFetcher->fetchAll($github->apps(), 'listRepositories');
// RESULTS LOOK LIKE THIS:
//[
// 'total_count' => 100,
// 'repositories' => [...],
//]
➡️ The way this iterates over results only returns the last page!
Essentially code return iterator_to_array($this->fetchAllLazy($api, $method, $parameters));
yields those results. Each iteration yielding the results over-write the previous ones, as its return an array with keys total_count
and repositories
each iteration.
It doesn't seem to merge results!
So, I've extended the class once again, here it is!
🔴 This iteration assumes the method setResultKey() is always used
<?php
namespace App\Api\GitHub;
use Generator;
use Github\Api\AbstractApi;
use Github\ResultPager as BasePager;
class ResultPager extends BasePager
{
private $resultKey;
/**
* @param $key
* @return $this
*/
public function setResultKey($key)
{
$this->resultKey = $key;
return $this;
}
/**
* {@inheritdoc}
*/
public function fetchAllLazy(AbstractApi $api, string $method, array $parameters = []): Generator
{
$result = $this->fetch($api, $method, $parameters);
foreach ($result[$this->resultKey] as $key => $item) {
if (is_string($key)) {
yield $key => $item;
} else {
yield $item;
}
}
while ($this->hasNext()) {
$result = $this->fetchNext();
foreach ($result[$this->resultKey] as $key => $item) {
if (is_string($key)) {
yield $key => $item;
} else {
yield $item;
}
}
}
}
}
Another Laravel-centric solution that does not involve over-riding the ResultPager
class is to use a LazyCollection
.
use Illuminate\Support\LazyCollection;
$github = new Client(new Builder, 'machine-man-preview');
$github->authenticate('<oauth token here>', AuthMethod::ACCESS_TOKEN);
$resultFetcher = new ResultPager($github);
$allInstallations = LazyCollection::make(function () use ($github, $resultFetcher) {
yield $resultFetcher->fetch($github->currentUser(), 'installations');
while ($resultFetcher->hasNext()) {
yield $resultFetcher->fetchNext();
}
})->reduce(function ($results, $result) {
$results['installations'] = array_merge($results['installations'] ?? [], $result['installations']);
return $results;
}, []);
foreach($allInstallations['installations'] as $installation) {...}
h/t to @tlaverdure 🍻
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, 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:
Here's an example call into the App's api that results in the issue:
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:If you
array_merge()
these, PHP will over-write the the first array with the second, as their associative keys match!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 forrepositories
/installations
) in the returned results:GET /user/installations/{installation_id}/repositories
GET /installation/repositories
GET /user/installations
The keys in the
$result
/$next
array arerepositories
orinstallations
.So perhaps something like this:
And then in
ResultPager
: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.
Let me know if I can clarify anything!