Dont add path to an url we already added a path to.

Author: Nyholm

.travis.yml

@@ -30,6 +30,8 @@ matrix:
         - php: 7.1
         - php: 7.2
           env: COVERAGE=true TEST_COMMAND="composer test-ci"  DEPENDENCIES="henrikbjorn/phpspec-code-coverage:^1.0"
+        - php: 7.2
+          env: TEST_COMMAND="./vendor/bin/phpunit" DEPENDENCIES="phpunit/phpunit:^7.5 nyholm/psr7:^1.0"
 
           # Test LTS versions
         - php: 7.1

CHANGELOG.md

@@ -4,8 +4,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
 

phpunit.xml.dist

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<phpunit bootstrap="./vendor/autoload.php"
+         colors="true"
+         convertErrorsToExceptions="true"
+         convertNoticesToExceptions="true"
+         convertWarningsToExceptions="true">
+    <php>
+        <env name="SHELL_VERBOSITY" value="-1" />
+    </php>
+
+    <testsuites>
+        <testsuite name="HTTPlug unit tests">
+            <directory suffix="Test.php">./tests</directory>
+        </testsuite>
+    </testsuites>
+</phpunit>

spec/Plugin/AddPathPluginSpec.php

@@ -37,9 +37,9 @@ 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

@@ -18,13 +18,6 @@ final class AddPathPlugin implements Plugin
      */
     private $uri;
 
-    /**
-     * Stores identifiers of the already altered requests.
-     *
-     * @var array
-     */
-    private $alteredRequests = [];
-
     /**
      * @param UriInterface $uri
      */
@@ -42,18 +35,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)
     {
-        $identifier = spl_object_hash((object) $first);
+         $prepend = $this->uri->getPath();
+         $path = $request->getUri()->getPath();
 
-        if (!array_key_exists($identifier, $this->alteredRequests)) {
-            $request = $request->withUri($request->getUri()
-                ->withPath($this->uri->getPath().$request->getUri()->getPath())
-            );
-            $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);
+    }
+}