From 1741eeebf87bd1ffeac9ba6f6b5af826b789ccf2 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sun, 2 Apr 2017 18:23:30 +0200 Subject: [PATCH 1/8] Make sure we vary on the Authorization header --- composer.json | 2 +- lib/Github/HttpClient/Builder.php | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 5b226cc70bb..acd07063f46 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,7 @@ "php-http/discovery": "^1.0", "php-http/client-implementation": "^1.0", "php-http/client-common": "^1.3", - "php-http/cache-plugin": "^1.3" + "php-http/cache-plugin": "^1.4" }, "require-dev": { "phpunit/phpunit": "^4.0 || ^5.5", diff --git a/lib/Github/HttpClient/Builder.php b/lib/Github/HttpClient/Builder.php index e42a5efe22e..86ed6fbd5ed 100644 --- a/lib/Github/HttpClient/Builder.php +++ b/lib/Github/HttpClient/Builder.php @@ -13,6 +13,7 @@ use Http\Message\RequestFactory; use Http\Message\StreamFactory; use Psr\Cache\CacheItemPoolInterface; +use Http\Client\Common\Plugin\Cache\Generator\VaryGenerator; /** * A builder that builds the API client. @@ -181,6 +182,9 @@ public function addHeaderValue($header, $headerValue) */ public function addCache(CacheItemPoolInterface $cachePool, array $config = []) { + if (!isset($config['cache_key_generator'])) { + $config['cache_key_generator'] = new VaryGenerator(['Authorization']); + } $this->cachePlugin = Plugin\CachePlugin::clientCache($cachePool, $this->streamFactory, $config); $this->httpClientModified = true; } From 379135c42ae7c4a262e3fcec97e4e1894c997568 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Wed, 5 Apr 2017 13:50:28 +0200 Subject: [PATCH 2/8] Updated name --- lib/Github/HttpClient/Builder.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Github/HttpClient/Builder.php b/lib/Github/HttpClient/Builder.php index 86ed6fbd5ed..5f27603150b 100644 --- a/lib/Github/HttpClient/Builder.php +++ b/lib/Github/HttpClient/Builder.php @@ -13,7 +13,7 @@ use Http\Message\RequestFactory; use Http\Message\StreamFactory; use Psr\Cache\CacheItemPoolInterface; -use Http\Client\Common\Plugin\Cache\Generator\VaryGenerator; +use Http\Client\Common\Plugin\Cache\Generator\HeaderCacheKeyGenerator; /** * A builder that builds the API client. @@ -183,7 +183,7 @@ public function addHeaderValue($header, $headerValue) public function addCache(CacheItemPoolInterface $cachePool, array $config = []) { if (!isset($config['cache_key_generator'])) { - $config['cache_key_generator'] = new VaryGenerator(['Authorization']); + $config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept']); } $this->cachePlugin = Plugin\CachePlugin::clientCache($cachePool, $this->streamFactory, $config); $this->httpClientModified = true; From 857ae23dc480aa2ed41a011a02d5f382a5c2df88 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Wed, 5 Apr 2017 13:58:05 +0200 Subject: [PATCH 3/8] Added Content-type --- lib/Github/HttpClient/Builder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Github/HttpClient/Builder.php b/lib/Github/HttpClient/Builder.php index 5f27603150b..d87a021c076 100644 --- a/lib/Github/HttpClient/Builder.php +++ b/lib/Github/HttpClient/Builder.php @@ -183,7 +183,7 @@ public function addHeaderValue($header, $headerValue) public function addCache(CacheItemPoolInterface $cachePool, array $config = []) { if (!isset($config['cache_key_generator'])) { - $config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept']); + $config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept', 'Content-type']); } $this->cachePlugin = Plugin\CachePlugin::clientCache($cachePool, $this->streamFactory, $config); $this->httpClientModified = true; From bcc093ed6c903467009e8424f144e1c117f04293 Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Wed, 5 Apr 2017 14:28:57 +0100 Subject: [PATCH 4/8] Allow us to test unreleased versions --- composer.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/composer.json b/composer.json index acd07063f46..3e02f70d23d 100644 --- a/composer.json +++ b/composer.json @@ -35,6 +35,8 @@ "autoload": { "psr-4": { "Github\\": "lib/Github/" } }, + "minimum-stability": "dev", + "prefer-stable": true, "extra": { "branch-alias": { "dev-master": "2.3.x-dev" From 2511479d1378d1a8574e73b8318d4f282e128672 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sat, 8 Apr 2017 17:49:55 +0200 Subject: [PATCH 5/8] Added functional tests --- composer.json | 3 +- test/Github/Tests/Functional/CacheTest.php | 110 +++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 test/Github/Tests/Functional/CacheTest.php diff --git a/composer.json b/composer.json index 3e02f70d23d..2e00f94fa8b 100644 --- a/composer.json +++ b/composer.json @@ -30,7 +30,8 @@ "phpunit/phpunit": "^4.0 || ^5.5", "php-http/guzzle6-adapter": "^1.0", "guzzlehttp/psr7": "^1.2", - "sllh/php-cs-fixer-styleci-bridge": "^1.3" + "sllh/php-cs-fixer-styleci-bridge": "^1.3", + "cache/array-adapter": "^0.5.0" }, "autoload": { "psr-4": { "Github\\": "lib/Github/" } diff --git a/test/Github/Tests/Functional/CacheTest.php b/test/Github/Tests/Functional/CacheTest.php new file mode 100644 index 00000000000..bc27ab94494 --- /dev/null +++ b/test/Github/Tests/Functional/CacheTest.php @@ -0,0 +1,110 @@ + + */ +class CacheTest extends \PHPUnit_Framework_TestCase +{ + /** + * @test + */ + public function shouldServeCachedResponse() + { + $mockClient = new \Http\Mock\Client(); + $mockClient->addResponse($this->getCurrentUserResponse('nyholm')); + $mockClient->addResponse($this->getCurrentUserResponse('octocat')); + + $github = Client::createWithHttpClient($mockClient); + $github->addCache(new ArrayCachePool(), ['default_ttl'=>60]); + + $github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN); + $userA = $github->currentUser()->show(); + $this->assertEquals('nyholm', $userA['login']); + + $userB = $github->currentUser()->show(); + $this->assertEquals('nyholm', $userB['login']); + } + /** + * @test + */ + public function shouldVaryOnAuthorization() + { + $mockClient = new \Http\Mock\Client(); + $mockClient->addResponse($this->getCurrentUserResponse('nyholm')); + $mockClient->addResponse($this->getCurrentUserResponse('octocat')); + + $github = Client::createWithHttpClient($mockClient); + $github->addCache(new ArrayCachePool(), ['default_ttl'=>60]); + + $github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN); + $userA = $github->currentUser()->show(); + $this->assertEquals('nyholm', $userA['login']); + + $github->authenticate('fake_token_bbb', Client::AUTH_HTTP_TOKEN); + $userB = $github->currentUser()->show(); + $this->assertEquals('octocat', $userB['login']); + } + + private function getCurrentUserResponse($username) + { + $headers = [ + 'Content-Type' => 'application/json', + ]; + + $body = stream_for(json_encode([ + 'login' => $username, + 'id' => 1, + 'avatar_url' => 'https://github.com/images/error/'.$username.'_happy.gif', + 'gravatar_id' => '', + 'url' => 'https://api.github.com/users/'.$username, + 'html_url' => 'https://github.com/'.$username, + 'followers_url' => 'https://api.github.com/users/'.$username.'/followers', + 'following_url' => 'https://api.github.com/users/'.$username.'/following{/other_user}', + 'gists_url' => 'https://api.github.com/users/'.$username.'/gists{/gist_id}', + 'starred_url' => 'https://api.github.com/users/'.$username.'/starred{/owner}{/repo}', + 'subscriptions_url' => 'https://api.github.com/users/'.$username.'/subscriptions', + 'organizations_url' => 'https://api.github.com/users/'.$username.'/orgs', + 'repos_url' => 'https://api.github.com/users/'.$username.'/repos', + 'events_url' => 'https://api.github.com/users/'.$username.'/events{/privacy}', + 'received_events_url' => 'https://api.github.com/users/'.$username.'/received_events', + 'type' => 'User', + 'site_admin' => false, + 'name' => 'monalisa '.$username, + 'company' => 'GitHub', + 'blog' => 'https://github.com/blog', + 'location' => 'San Francisco', + 'email' => $username.'@github.com', + 'hireable' => false, + 'bio' => 'There once was...', + 'public_repos' => 2, + 'public_gists' => 1, + 'followers' => 20, + 'following' => 0, + 'created_at' => '2008-01-14T04:33:35Z', + 'updated_at' => '2008-01-14T04:33:35Z', + 'total_private_repos' => 100, + 'owned_private_repos' => 100, + 'private_gists' => 81, + 'disk_usage' => 10000, + 'collaborators' => 8, + 'two_factor_authentication' => true, + 'plan' => [ + 'name' => 'Medium', + 'space' => 400, + 'private_repos' => 20, + 'collaborators' => 0, + ], + ])); + + return new Response(200, $headers, $body); + } +} From 87f68955154d2559a7cacadffe3b5ac74609f40c Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sat, 8 Apr 2017 17:52:48 +0200 Subject: [PATCH 6/8] Reduced unneeded code --- test/Github/Tests/Functional/CacheTest.php | 49 ++-------------------- 1 file changed, 4 insertions(+), 45 deletions(-) diff --git a/test/Github/Tests/Functional/CacheTest.php b/test/Github/Tests/Functional/CacheTest.php index bc27ab94494..7df503a2b9b 100644 --- a/test/Github/Tests/Functional/CacheTest.php +++ b/test/Github/Tests/Functional/CacheTest.php @@ -24,14 +24,14 @@ public function shouldServeCachedResponse() $mockClient->addResponse($this->getCurrentUserResponse('octocat')); $github = Client::createWithHttpClient($mockClient); - $github->addCache(new ArrayCachePool(), ['default_ttl'=>60]); + $github->addCache(new ArrayCachePool(), ['default_ttl'=>600]); $github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN); $userA = $github->currentUser()->show(); $this->assertEquals('nyholm', $userA['login']); $userB = $github->currentUser()->show(); - $this->assertEquals('nyholm', $userB['login']); + $this->assertEquals('nyholm', $userB['login'], 'Two request following each other should be cached.'); } /** * @test @@ -43,7 +43,7 @@ public function shouldVaryOnAuthorization() $mockClient->addResponse($this->getCurrentUserResponse('octocat')); $github = Client::createWithHttpClient($mockClient); - $github->addCache(new ArrayCachePool(), ['default_ttl'=>60]); + $github->addCache(new ArrayCachePool(), ['default_ttl'=>600]); $github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN); $userA = $github->currentUser()->show(); @@ -51,7 +51,7 @@ public function shouldVaryOnAuthorization() $github->authenticate('fake_token_bbb', Client::AUTH_HTTP_TOKEN); $userB = $github->currentUser()->show(); - $this->assertEquals('octocat', $userB['login']); + $this->assertEquals('octocat', $userB['login'], 'We must vary on the Authorization header.'); } private function getCurrentUserResponse($username) @@ -62,47 +62,6 @@ private function getCurrentUserResponse($username) $body = stream_for(json_encode([ 'login' => $username, - 'id' => 1, - 'avatar_url' => 'https://github.com/images/error/'.$username.'_happy.gif', - 'gravatar_id' => '', - 'url' => 'https://api.github.com/users/'.$username, - 'html_url' => 'https://github.com/'.$username, - 'followers_url' => 'https://api.github.com/users/'.$username.'/followers', - 'following_url' => 'https://api.github.com/users/'.$username.'/following{/other_user}', - 'gists_url' => 'https://api.github.com/users/'.$username.'/gists{/gist_id}', - 'starred_url' => 'https://api.github.com/users/'.$username.'/starred{/owner}{/repo}', - 'subscriptions_url' => 'https://api.github.com/users/'.$username.'/subscriptions', - 'organizations_url' => 'https://api.github.com/users/'.$username.'/orgs', - 'repos_url' => 'https://api.github.com/users/'.$username.'/repos', - 'events_url' => 'https://api.github.com/users/'.$username.'/events{/privacy}', - 'received_events_url' => 'https://api.github.com/users/'.$username.'/received_events', - 'type' => 'User', - 'site_admin' => false, - 'name' => 'monalisa '.$username, - 'company' => 'GitHub', - 'blog' => 'https://github.com/blog', - 'location' => 'San Francisco', - 'email' => $username.'@github.com', - 'hireable' => false, - 'bio' => 'There once was...', - 'public_repos' => 2, - 'public_gists' => 1, - 'followers' => 20, - 'following' => 0, - 'created_at' => '2008-01-14T04:33:35Z', - 'updated_at' => '2008-01-14T04:33:35Z', - 'total_private_repos' => 100, - 'owned_private_repos' => 100, - 'private_gists' => 81, - 'disk_usage' => 10000, - 'collaborators' => 8, - 'two_factor_authentication' => true, - 'plan' => [ - 'name' => 'Medium', - 'space' => 400, - 'private_repos' => 20, - 'collaborators' => 0, - ], ])); return new Response(200, $headers, $body); From b4a116ebb45d9b0103a2731ba01f000cb38cfdad Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sat, 8 Apr 2017 18:31:20 +0200 Subject: [PATCH 7/8] updateing deps --- composer.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 2e00f94fa8b..e5a0e106c83 100644 --- a/composer.json +++ b/composer.json @@ -29,9 +29,10 @@ "require-dev": { "phpunit/phpunit": "^4.0 || ^5.5", "php-http/guzzle6-adapter": "^1.0", + "php-http/mock-client": "^1.0", "guzzlehttp/psr7": "^1.2", "sllh/php-cs-fixer-styleci-bridge": "^1.3", - "cache/array-adapter": "^0.5.0" + "cache/array-adapter": "^0.4" }, "autoload": { "psr-4": { "Github\\": "lib/Github/" } From db55a26c59a0a56958162d208660ca39219dce82 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Sat, 8 Apr 2017 18:35:55 +0200 Subject: [PATCH 8/8] Do not use any php5.5+ code --- test/Github/Tests/Functional/CacheTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/Github/Tests/Functional/CacheTest.php b/test/Github/Tests/Functional/CacheTest.php index 7df503a2b9b..31c6f5a4f5d 100644 --- a/test/Github/Tests/Functional/CacheTest.php +++ b/test/Github/Tests/Functional/CacheTest.php @@ -5,7 +5,6 @@ use Cache\Adapter\PHPArray\ArrayCachePool; use Github\Client; use GuzzleHttp\Psr7\Response; -use function GuzzleHttp\Psr7\stream_for; /** * @group functional @@ -60,7 +59,7 @@ private function getCurrentUserResponse($username) 'Content-Type' => 'application/json', ]; - $body = stream_for(json_encode([ + $body = \GuzzleHttp\Psr7\stream_for(json_encode([ 'login' => $username, ]));