Dont add path to an url we already added a path to. (#141)

Author: Nyholm

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`
@@ -18,7 +19,7 @@
 
 ### 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.
 
 ## 1.8.2 - 2018-12-14

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);

src/Plugin/AddPathPlugin.php

@@ -21,13 +21,6 @@ final class AddPathPlugin implements Plugin
      */
     private $uri;
 
-    /**
-     * Stores identifiers of the already altered requests.
-     *
-     * @var array
-     */
-    private $alteredRequests = [];
-
     public function __construct(UriInterface $uri)
     {
         if ('' === $uri->getPath()) {
@@ -42,16 +35,42 @@ 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);

tests/Plugin/AddPathPluginTest.php

@@ -0,0 +1,85 @@
+<?php
+
+namespace tests\Http\Client\Common\Plugin;
+
+use Http\Client\Common\Plugin;
+use Http\Client\Common\Plugin\AddPathPlugin;
+use Http\Client\Common\PluginClient;
+use Http\Client\HttpClient;
+use Nyholm\Psr7\Request;
+use Nyholm\Psr7\Response;
+use Nyholm\Psr7\Uri;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\RequestInterface;
+
+class AddPathPluginTest extends TestCase
+{
+    /**
+     * @var Plugin
+     */
+    private $plugin;
+
+    /**
+     * @var callable empty
+     */
+    private $first;
+
+    protected function setUp()
+    {
+        $this->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);
+    }
+}