From 2b19ebefae4d08191a7852140f35602b24bd270f Mon Sep 17 00:00:00 2001 From: Nyholm Date: Sat, 29 Dec 2018 13:12:17 +0100 Subject: [PATCH 1/3] Dont add path to an url we already added a path to. --- CHANGELOG.md | 4 +- spec/Plugin/AddPathPluginSpec.php | 4 +- src/Plugin/AddPathPlugin.php | 44 ++++++++++++---- tests/Plugin/AddPathPluginTest.php | 85 ++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 tests/Plugin/AddPathPluginTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d028df3..787cab1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,8 +18,10 @@ ### Changed -- [RetryPlugin] Renamed the configuration options for the exception retry callback from `decider` to `exception_decider` +- RetryPlugin: Renamed the configuration options for the exception retry callback from `decider` to `exception_decider` and `delay` to `exception_delay`. The old names still work but are deprecated. +- AddPathPlugin: Do not add the prefix if the URL already has the same prefix. + ## 1.8.2 - 2018-12-14 diff --git a/spec/Plugin/AddPathPluginSpec.php b/spec/Plugin/AddPathPluginSpec.php index 6491069..1c6f09d 100644 --- a/spec/Plugin/AddPathPluginSpec.php +++ b/spec/Plugin/AddPathPluginSpec.php @@ -37,9 +37,9 @@ public function it_adds_path( $host->getPath()->shouldBeCalled()->willReturn('/api'); $request->getUri()->shouldBeCalled()->willReturn($uri); - $request->withUri($uri)->shouldBeCalled()->willReturn($request); + $request->withUri($uri)->shouldBeCalledTimes(1)->willReturn($request); - $uri->withPath('/api/users')->shouldBeCalled()->willReturn($uri); + $uri->withPath('/api/users')->shouldBeCalledTimes(1)->willReturn($uri); $uri->getPath()->shouldBeCalled()->willReturn('/users'); $this->beConstructedWith($host); diff --git a/src/Plugin/AddPathPlugin.php b/src/Plugin/AddPathPlugin.php index 8b92514..b127526 100644 --- a/src/Plugin/AddPathPlugin.php +++ b/src/Plugin/AddPathPlugin.php @@ -22,12 +22,8 @@ final class AddPathPlugin implements Plugin private $uri; /** - * Stores identifiers of the already altered requests. - * - * @var array + * @param UriInterface $uri */ - private $alteredRequests = []; - public function __construct(UriInterface $uri) { if ('' === $uri->getPath()) { @@ -42,17 +38,43 @@ public function __construct(UriInterface $uri) } /** + * Adds a prefix in the beginning of the URL's path. + * + * The prefix is not added if that prefix is already on the URL's path. This will fail on the edge + * case of the prefix being repeated, for example if `https://example.com/api/api/foo` is a valid + * URL on the server and the configured prefix is `/api`. + * + * We looked at other solutions, but they are all much more complicated, while still having edge + * cases: + * - Doing an spl_object_hash on `$first` will lead to collisions over time because over time the + * hash can collide. + * - Have the PluginClient provide a magic header to identify the request chain and only apply + * this plugin once. + * + * There are 2 reasons for the AddPathPlugin to be executed twice on the same request: + * - A plugin can restart the chain by calling `$first`, e.g. redirect + * - A plugin can call `$next` more than once, e.g. retry + * + * Depending on the scenario, the path should or should not be added. E.g. `$first` could + * be called after a redirect response from the server. The server likely already has the + * correct path. + * + * No solution fits all use cases. This implementation will work fine for the common use cases. + * If you have a specific situation where this is not the right thing, you can build a custom plugin + * that does exactly what you need. + * * {@inheritdoc} */ public function handleRequest(RequestInterface $request, callable $next, callable $first): Promise { - $identifier = spl_object_hash((object) $first); + $prepend = $this->uri->getPath(); + $path = $request->getUri()->getPath(); - if (!array_key_exists($identifier, $this->alteredRequests)) { - $prefixedUrl = $this->uri->getPath().$request->getUri()->getPath(); - $request = $request->withUri($request->getUri()->withPath($prefixedUrl)); - $this->alteredRequests[$identifier] = $identifier; - } + if (substr($path, 0, strlen($prepend)) !== $prepend) { + $request = $request->withUri($request->getUri() + ->withPath($prepend.$path) + ); + } return $next($request); } diff --git a/tests/Plugin/AddPathPluginTest.php b/tests/Plugin/AddPathPluginTest.php new file mode 100644 index 0000000..3980fa4 --- /dev/null +++ b/tests/Plugin/AddPathPluginTest.php @@ -0,0 +1,85 @@ +first = function () {}; + $this->plugin = new AddPathPlugin(new Uri('/api')); + } + + public function testRewriteSameUrl() + { + $verify = function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString()); + }; + + $request = new Request('GET', 'https://example.com/foo', ['Content-Type'=>'text/html']); + $this->plugin->handleRequest($request, $verify, $this->first); + + // Make a second call with the same $request object + $this->plugin->handleRequest($request, $verify, $this->first); + + // Make a new call with a new object but same URL + $request = new Request('GET', 'https://example.com/foo', ['Content-Type'=>'text/plain']); + $this->plugin->handleRequest($request, $verify, $this->first); + } + + public function testRewriteCallingThePluginTwice() + { + $request = new Request('GET', 'https://example.com/foo'); + $this->plugin->handleRequest($request, function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString()); + + // Run the plugin again with the modified request + $this->plugin->handleRequest($request, function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString()); + }, $this->first); + }, $this->first); + } + + public function testRewriteWithDifferentUrl() + { + $request = new Request('GET', 'https://example.com/foo'); + $this->plugin->handleRequest($request, function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString()); + }, $this->first); + + $request = new Request('GET', 'https://example.com/bar'); + $this->plugin->handleRequest($request, function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/bar', $request->getUri()->__toString()); + }, $this->first); + } + + public function testRewriteWhenPathIsIncluded() + { + $verify = function (RequestInterface $request) { + $this->assertEquals('https://example.com/api/foo', $request->getUri()->__toString()); + }; + + $request = new Request('GET', 'https://example.com/api/foo'); + $this->plugin->handleRequest($request, $verify, $this->first); + } +} From 4df813c2366827bc3a851c64a14376c57bbabdd6 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Thu, 3 Jan 2019 10:01:33 +0100 Subject: [PATCH 2/3] updated changelog --- CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 787cab1..f636a0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Added interfaces for BatchClient, HttpClientRouter and HttpMethodsClient. (These interfaces use the `Interface` suffix to avoid name collisions.) - Added an interface for HttpClientPool and moved the abstract class to the HttpClientPool sub namespace. +- AddPathPlugin: Do not add the prefix if the URL already has the same prefix. ### Removed - Deprecated option `debug_plugins` has been removed from `PluginClient` @@ -20,8 +21,6 @@ - RetryPlugin: Renamed the configuration options for the exception retry callback from `decider` to `exception_decider` and `delay` to `exception_delay`. The old names still work but are deprecated. -- AddPathPlugin: Do not add the prefix if the URL already has the same prefix. - ## 1.8.2 - 2018-12-14 From ffe4da26b3bd7223da2554ce695639d502decdc0 Mon Sep 17 00:00:00 2001 From: Tobias Nyholm Date: Thu, 3 Jan 2019 12:25:41 +0100 Subject: [PATCH 3/3] cs --- src/Plugin/AddPathPlugin.php | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Plugin/AddPathPlugin.php b/src/Plugin/AddPathPlugin.php index b127526..9d43104 100644 --- a/src/Plugin/AddPathPlugin.php +++ b/src/Plugin/AddPathPlugin.php @@ -21,9 +21,6 @@ final class AddPathPlugin implements Plugin */ private $uri; - /** - * @param UriInterface $uri - */ public function __construct(UriInterface $uri) { if ('' === $uri->getPath()) { @@ -67,14 +64,14 @@ public function __construct(UriInterface $uri) */ public function handleRequest(RequestInterface $request, callable $next, callable $first): Promise { - $prepend = $this->uri->getPath(); - $path = $request->getUri()->getPath(); + $prepend = $this->uri->getPath(); + $path = $request->getUri()->getPath(); - if (substr($path, 0, strlen($prepend)) !== $prepend) { - $request = $request->withUri($request->getUri() + if (substr($path, 0, strlen($prepend)) !== $prepend) { + $request = $request->withUri($request->getUri() ->withPath($prepend.$path) ); - } + } return $next($request); }