From 2cbceeb16cdd966a0f0ec921253427309a684666 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Mon, 19 Jun 2017 23:19:26 +0200 Subject: [PATCH 1/6] add failing profiling test when using cache plugin --- Tests/Functional/ProfilingTest.php | 80 ++++++++++++++++++++++++++++++ composer.json | 2 + 2 files changed, 82 insertions(+) create mode 100644 Tests/Functional/ProfilingTest.php diff --git a/Tests/Functional/ProfilingTest.php b/Tests/Functional/ProfilingTest.php new file mode 100644 index 00000000..dd68a5b5 --- /dev/null +++ b/Tests/Functional/ProfilingTest.php @@ -0,0 +1,80 @@ +collector = new Collector([]); + $this->formatter = new Formatter(new FullHttpMessageFormatter(), new CurlCommandFormatter()); + $this->stopwatch = new Stopwatch(); + } + + public function testCachePluginProfiling() + { + $pool = new ArrayAdapter(); + + $client = $this->createClient([ + new Plugin\CachePlugin($pool, StreamFactoryDiscovery::find(), [ + 'respect_response_cache_directives' => [], + 'default_ttl' => 86400, + ]), + ]); + + $client->sendRequest(new Request('GET', 'https://example.com')); + $client->sendRequest(new Request('GET', 'https://example.com')); + + $this->assertCount(2, $this->collector->getStacks()); + $stack = $this->collector->getStacks()[1]; + $this->assertEquals('GET', $stack->getRequestMethod()); + $this->assertEquals('https', $stack->getRequestScheme()); + $this->assertEquals('/', $stack->getRequestTarget()); + $this->assertEquals('example.com', $stack->getRequestHost()); + } + + private function createClient(array $plugins, $clientName = 'Acme', array $clientOptions = []) + { + $plugins = array_map(function (Plugin $plugin) { + return new ProfilePlugin($plugin, $this->collector, $this->formatter, get_class($plugin)); + }, $plugins); + + array_unshift($plugins, new StackPlugin($this->collector, $this->formatter, $clientName)); + + $client = new Client(); + $client = new ProfileClient($client, $this->collector, $this->formatter, $this->stopwatch); + $client = new PluginClient($client, $plugins, $clientOptions); + + return $client; + } +} diff --git a/composer.json b/composer.json index 3376db0c..755129c3 100644 --- a/composer.json +++ b/composer.json @@ -37,11 +37,13 @@ "php-http/guzzle6-adapter": "^1.1.1", "php-http/react-adapter": "^0.2.1", "php-http/buzz-adapter": "^0.3", + "php-http/mock-client": "^1.0", "symfony/phpunit-bridge": "^3.2", "symfony/twig-bundle": "^2.8 || ^3.0", "symfony/twig-bridge": "^2.8 || ^3.0", "symfony/web-profiler-bundle": "^2.8 || ^3.0", "symfony/finder": "^2.7 || ^3.0", + "symfony/cache": "^3.1", "polishsymfonycommunity/symfony-mocker-container": "^1.0", "matthiasnoback/symfony-dependency-injection-test": "^1.0" }, From 2f4b1bd640d12fc9ca8df2b3d28d7cf7bdc496e7 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Wed, 28 Jun 2017 09:47:18 +0200 Subject: [PATCH 2/6] add failing test when plugin throw an exception --- Tests/Functional/ProfilingTest.php | 38 ++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/Tests/Functional/ProfilingTest.php b/Tests/Functional/ProfilingTest.php index dd68a5b5..ce39a85d 100644 --- a/Tests/Functional/ProfilingTest.php +++ b/Tests/Functional/ProfilingTest.php @@ -14,6 +14,7 @@ use Http\Message\Formatter\CurlCommandFormatter; use Http\Message\Formatter\FullHttpMessageFormatter; use Http\Mock\Client; +use Psr\Http\Message\RequestInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Stopwatch\Stopwatch; @@ -41,12 +42,10 @@ public function setUp() $this->stopwatch = new Stopwatch(); } - public function testCachePluginProfiling() + public function testProfilingWithCachePlugin() { - $pool = new ArrayAdapter(); - $client = $this->createClient([ - new Plugin\CachePlugin($pool, StreamFactoryDiscovery::find(), [ + new Plugin\CachePlugin(new ArrayAdapter(), StreamFactoryDiscovery::find(), [ 'respect_response_cache_directives' => [], 'default_ttl' => 86400, ]), @@ -63,6 +62,26 @@ public function testCachePluginProfiling() $this->assertEquals('example.com', $stack->getRequestHost()); } + public function testProfilingWhenPluginThrowException() + { + $client = $this->createClient([ + new ExceptionThrowerPlugin(), + ]); + + $this->setExpectedException(\Exception::class); + + try { + $client->sendRequest(new Request('GET', 'https://example.com')); + } finally { + $this->assertCount(1, $this->collector->getStacks()); + $stack = $this->collector->getStacks()[0]; + $this->assertEquals('GET', $stack->getRequestMethod()); + $this->assertEquals('https', $stack->getRequestScheme()); + $this->assertEquals('/', $stack->getRequestTarget()); + $this->assertEquals('example.com', $stack->getRequestHost()); + } + } + private function createClient(array $plugins, $clientName = 'Acme', array $clientOptions = []) { $plugins = array_map(function (Plugin $plugin) { @@ -78,3 +97,14 @@ private function createClient(array $plugins, $clientName = 'Acme', array $clien return $client; } } + +class ExceptionThrowerPlugin implements Plugin +{ + /** + * {@inheritdoc} + */ + public function handleRequest(RequestInterface $request, callable $next, callable $first) + { + throw new \Exception(); + } +} From bdce23729542653b698cf16c137d267379039e23 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Tue, 20 Jun 2017 14:22:54 +0200 Subject: [PATCH 3/6] add working test for normal profiling --- Tests/Functional/ProfilingTest.php | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Tests/Functional/ProfilingTest.php b/Tests/Functional/ProfilingTest.php index ce39a85d..1a06e535 100644 --- a/Tests/Functional/ProfilingTest.php +++ b/Tests/Functional/ProfilingTest.php @@ -6,6 +6,7 @@ use Http\Client\Common\Plugin; use Http\Client\Common\PluginClient; use Http\Discovery\StreamFactoryDiscovery; +use Http\Discovery\UriFactoryDiscovery; use Http\HttplugBundle\Collector\Collector; use Http\HttplugBundle\Collector\Formatter; use Http\HttplugBundle\Collector\ProfileClient; @@ -82,6 +83,25 @@ public function testProfilingWhenPluginThrowException() } } + public function testProfiling() + { + $client = $this->createClient([ + new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri('https://example.com')), + new Plugin\RedirectPlugin(), + new Plugin\RetryPlugin(), + ]); + + $client->sendRequest(new Request('GET', '/')); + + $this->assertCount(1, $this->collector->getStacks()); + $stack = $this->collector->getStacks()[0]; + $this->assertCount(3, $stack->getProfiles()); + $this->assertEquals('GET', $stack->getRequestMethod()); + $this->assertEquals('https', $stack->getRequestScheme()); + $this->assertEquals('/', $stack->getRequestTarget()); + $this->assertEquals('example.com', $stack->getRequestHost()); + } + private function createClient(array $plugins, $clientName = 'Acme', array $clientOptions = []) { $plugins = array_map(function (Plugin $plugin) { From 8fa554d39bb1c6325055e1c92dfb93211a03aae7 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Tue, 27 Jun 2017 14:50:35 +0200 Subject: [PATCH 4/6] fix the cache plugin case --- Collector/ProfilePlugin.php | 39 +++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/Collector/ProfilePlugin.php b/Collector/ProfilePlugin.php index 96d5b817..82bed2ba 100644 --- a/Collector/ProfilePlugin.php +++ b/Collector/ProfilePlugin.php @@ -77,15 +77,50 @@ public function handleRequest(RequestInterface $request, callable $next, callabl return $first($request); }; - return $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst)->then(function (ResponseInterface $response) use ($profile) { + return $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst)->then(function (ResponseInterface $response) use ($profile, $request, $stack) { $profile->setResponse($this->formatter->formatResponse($response)); + $this->collectRequestInformation($request, $stack); return $response; - }, function (Exception $exception) use ($profile) { + }, function (Exception $exception) use ($profile, $request, $stack) { $profile->setFailed(true); $profile->setResponse($this->formatter->formatException($exception)); + $this->collectRequestInformation($request, $stack); throw $exception; }); } + + /** + * Collect request information when not already done by the HTTP client. This happens when using the CachePlugin + * and the cache is hit without re-validation. + * + * @param RequestInterface $request + * @param Stack|null $stack + */ + private function collectRequestInformation(RequestInterface $request, Stack $stack = null) + { + if (null === $stack) { + return; + } + + if (empty($stack->getRequestTarget())) { + $stack->setRequestTarget($request->getRequestTarget()); + } + if (empty($stack->getRequestMethod())) { + $stack->setRequestMethod($request->getMethod()); + } + if (empty($stack->getRequestScheme())) { + $stack->setRequestScheme($request->getUri()->getScheme()); + } + if (empty($stack->getRequestHost())) { + $stack->setRequestHost($request->getUri()->getHost()); + } + if (empty($stack->getClientRequest())) { + $stack->setClientRequest($this->formatter->formatRequest($request)); + } + if (empty($stack->getCurlCommand())) { + $stack->setCurlCommand($this->formatter->formatAsCurlCommand($request)); + } + } } From fb18f836dde74d54a25a38643d6725b33828d638 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Tue, 27 Jun 2017 14:59:30 +0200 Subject: [PATCH 5/6] fix exception case --- Collector/ProfilePlugin.php | 14 +++++++++++-- Tests/Unit/Collector/ProfilePluginTest.php | 24 ++++++++++++++++++---- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Collector/ProfilePlugin.php b/Collector/ProfilePlugin.php index 82bed2ba..d9d34601 100644 --- a/Collector/ProfilePlugin.php +++ b/Collector/ProfilePlugin.php @@ -77,7 +77,17 @@ public function handleRequest(RequestInterface $request, callable $next, callabl return $first($request); }; - return $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst)->then(function (ResponseInterface $response) use ($profile, $request, $stack) { + try { + $promise = $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst); + } catch (Exception $e) { + $profile->setFailed(true); + $profile->setResponse($this->formatter->formatException($e)); + $this->collectRequestInformation($request, $stack); + + throw $e; + } + + return $promise->then(function (ResponseInterface $response) use ($profile, $request, $stack) { $profile->setResponse($this->formatter->formatResponse($response)); $this->collectRequestInformation($request, $stack); @@ -96,7 +106,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl * and the cache is hit without re-validation. * * @param RequestInterface $request - * @param Stack|null $stack + * @param Stack|null $stack */ private function collectRequestInformation(RequestInterface $request, Stack $stack = null) { diff --git a/Tests/Unit/Collector/ProfilePluginTest.php b/Tests/Unit/Collector/ProfilePluginTest.php index 2e971a28..dbf7eac4 100644 --- a/Tests/Unit/Collector/ProfilePluginTest.php +++ b/Tests/Unit/Collector/ProfilePluginTest.php @@ -11,6 +11,8 @@ use Http\HttplugBundle\Collector\ProfilePlugin; use Http\HttplugBundle\Collector\Stack; use Http\Promise\FulfilledPromise; +use Http\Promise\Promise; +use Http\Promise\RejectedPromise; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; @@ -36,6 +38,11 @@ class ProfilePluginTest extends \PHPUnit_Framework_TestCase */ private $response; + /** + * @var Promise + */ + private $fulfilledPromise; + /** * @var Stack */ @@ -46,6 +53,11 @@ class ProfilePluginTest extends \PHPUnit_Framework_TestCase */ private $exception; + /** + * @var Promise + */ + private $rejectedPromise; + /** * @var Formatter */ @@ -62,8 +74,10 @@ public function setUp() $this->collector = $this->getMockBuilder(Collector::class)->disableOriginalConstructor()->getMock(); $this->request = new Request('GET', '/'); $this->response = new Response(); + $this->fulfilledPromise = new FulfilledPromise($this->response); $this->currentStack = new Stack('default', 'FormattedRequest'); $this->exception = new TransferException(); + $this->rejectedPromise = new RejectedPromise($this->exception); $this->formatter = $this->getMockBuilder(Formatter::class)->disableOriginalConstructor()->getMock(); $this->collector @@ -74,9 +88,7 @@ public function setUp() $this->plugin ->method('handleRequest') ->willReturnCallback(function ($request, $next, $first) { - $next($request); - - return new FulfilledPromise($this->response); + return $next($request); }) ; @@ -115,6 +127,7 @@ public function testCallDecoratedPlugin() ; $this->subject->handleRequest($this->request, function () { + return $this->fulfilledPromise; }, function () { }); } @@ -122,6 +135,7 @@ public function testCallDecoratedPlugin() public function testProfileIsInitialized() { $this->subject->handleRequest($this->request, function () { + return $this->fulfilledPromise; }, function () { }); @@ -133,6 +147,7 @@ public function testProfileIsInitialized() public function testCollectRequestInformations() { $this->subject->handleRequest($this->request, function () { + return $this->fulfilledPromise; }, function () { }); @@ -143,6 +158,7 @@ public function testCollectRequestInformations() public function testOnFulfilled() { $promise = $this->subject->handleRequest($this->request, function () { + return $this->fulfilledPromise; }, function () { }); @@ -156,7 +172,7 @@ public function testOnRejected() $this->setExpectedException(TransferException::class); $promise = $this->subject->handleRequest($this->request, function () { - throw new TransferException(); + return $this->rejectedPromise; }, function () { }); From 20663767301c4e00c35e56df502fd3f9f7bd6938 Mon Sep 17 00:00:00 2001 From: Fabien Bourigault Date: Wed, 28 Jun 2017 15:29:56 +0200 Subject: [PATCH 6/6] improve ProfilePlugin readability --- Collector/ProfilePlugin.php | 53 ++++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/Collector/ProfilePlugin.php b/Collector/ProfilePlugin.php index d9d34601..bf5d54e8 100644 --- a/Collector/ProfilePlugin.php +++ b/Collector/ProfilePlugin.php @@ -65,14 +65,14 @@ public function handleRequest(RequestInterface $request, callable $next, callabl // wrap the next callback to profile the plugin request changes $wrappedNext = function (RequestInterface $request) use ($next, $profile) { - $profile->setRequest($this->formatter->formatRequest($request)); + $this->onOutgoingRequest($request, $profile); return $next($request); }; // wrap the first callback to profile the plugin request changes $wrappedFirst = function (RequestInterface $request) use ($first, $profile) { - $profile->setRequest($this->formatter->formatRequest($request)); + $this->onOutgoingRequest($request, $profile); return $first($request); }; @@ -80,27 +80,60 @@ public function handleRequest(RequestInterface $request, callable $next, callabl try { $promise = $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst); } catch (Exception $e) { - $profile->setFailed(true); - $profile->setResponse($this->formatter->formatException($e)); - $this->collectRequestInformation($request, $stack); + $this->onException($request, $profile, $e, $stack); throw $e; } return $promise->then(function (ResponseInterface $response) use ($profile, $request, $stack) { - $profile->setResponse($this->formatter->formatResponse($response)); - $this->collectRequestInformation($request, $stack); + $this->onOutgoingResponse($response, $profile, $request, $stack); return $response; }, function (Exception $exception) use ($profile, $request, $stack) { - $profile->setFailed(true); - $profile->setResponse($this->formatter->formatException($exception)); - $this->collectRequestInformation($request, $stack); + $this->onException($request, $profile, $exception, $stack); throw $exception; }); } + /** + * @param RequestInterface $request + * @param Profile $profile + * @param Exception $exception + * @param Stack $stack + */ + private function onException( + RequestInterface $request, + Profile $profile, + Exception $exception, + Stack $stack = null + ) { + $profile->setFailed(true); + $profile->setResponse($this->formatter->formatException($exception)); + $this->collectRequestInformation($request, $stack); + } + + /** + * @param RequestInterface $request + * @param Profile $profile + */ + private function onOutgoingRequest(RequestInterface $request, Profile $profile) + { + $profile->setRequest($this->formatter->formatRequest($request)); + } + + /** + * @param ResponseInterface $response + * @param Profile $profile + * @param RequestInterface $request + * @param Stack $stack + */ + private function onOutgoingResponse(ResponseInterface $response, Profile $profile, RequestInterface $request, Stack $stack = null) + { + $profile->setResponse($this->formatter->formatResponse($response)); + $this->collectRequestInformation($request, $stack); + } + /** * Collect request information when not already done by the HTTP client. This happens when using the CachePlugin * and the cache is hit without re-validation.