Fix: URL appending tweaks (#206)

Unleash · Jun 25, 2024 · b149325 · b149325
1 parent 075d370
commit b149325
Show file tree

Hide file tree

Showing 11 changed files with 175 additions and 50 deletions.
diff --git a/src/Client/DefaultRegistrationService.php b/src/Client/DefaultRegistrationService.php
@@ -12,6 +12,7 @@
 use Unleash\Client\Configuration\UnleashConfiguration;
 use Unleash\Client\Enum\CacheKey;
 use Unleash\Client\Helper\StringStream;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Strategy\StrategyHandler;
 use Unleash\Client\Unleash;
 
@@ -47,7 +48,7 @@ public function register(iterable $strategyHandlers): bool
             $strategyHandlers = iterator_to_array($strategyHandlers);
         }
         $request = $this->requestFactory
-            ->createRequest('POST', $this->configuration->getUrl() . 'client/register')
+            ->createRequest('POST', (string) Url::appendPath($this->configuration->getUrl(), 'client/register'))
             ->withHeader('Content-Type', 'application/json')
             ->withBody(new StringStream(json_encode([
                 'appName' => $this->configuration->getAppName(),

diff --git a/src/Configuration/UnleashConfiguration.php b/src/Configuration/UnleashConfiguration.php
@@ -14,6 +14,7 @@
 use Unleash\Client\Bootstrap\EmptyBootstrapProvider;
 use Unleash\Client\ContextProvider\DefaultUnleashContextProvider;
 use Unleash\Client\ContextProvider\UnleashContextProvider;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Metrics\DefaultMetricsBucketSerializer;
 use Unleash\Client\Metrics\MetricsBucketSerializer;
 
@@ -67,12 +68,7 @@ public function getMetricsCache(): CacheInterface
 
     public function getUrl(): string
     {
-        $url = $this->url;
-        if (!str_ends_with($url, '/')) {
-            $url .= '/';
-        }
-
-        return (string) $url;
+        return $this->url;
     }
 
     public function getAppName(): string
@@ -105,8 +101,8 @@ public function setProxyKey(?string $proxyKey): self
     public function getMetricsUrl(): string
     {
         return $this->proxyKey !== null
-            ? $this->getUrl() . 'frontend/client/metrics'
-            : $this->getUrl() . 'client/metrics';
+            ? Url::appendPath($this->getUrl(), 'frontend/client/metrics')
+            : Url::appendPath($this->getUrl(), 'client/metrics');
     }
 
     public function setCache(CacheInterface $cache): self

diff --git a/src/Helper/Url.php b/src/Helper/Url.php
@@ -20,10 +20,12 @@ public function __construct(
     #[Override]
     public function __toString(): string
     {
+        $query = parse_url($this->url, PHP_URL_QUERY);
+
         $url = $this->url;
 
         if ($this->namePrefix !== null || $this->tags !== null) {
-            $url .= '?';
+            $url .= $query ? '&' : '?';
         }
 
         if ($this->namePrefix !== null && $this->namePrefix !== '') {
@@ -42,4 +44,58 @@ public function __toString(): string
 
         return $url;
     }
+
+    public static function appendPath(string $url, string $path): self
+    {
+        if (!$path) {
+            return new self($url);
+        }
+
+        $parts = parse_url($url);
+        assert(is_array($parts));
+
+        if (!str_starts_with($path, '/')) {
+            $path = "/{$path}";
+        }
+        $parts['path'] ??= '';
+        if (str_ends_with($parts['path'], '/')) {
+            $parts['path'] = substr($parts['path'], 0, -1);
+        }
+
+        $parts['path'] .= $path;
+
+        return self::buildUrl($parts);
+    }
+
+    /**
+     * @param array<string, mixed> $parts
+     */
+    public static function buildUrl(array $parts): self
+    {
+        $result = '';
+        if (isset($parts['scheme']) && is_string($parts['scheme'])) {
+            $result .= $parts['scheme'] . '://';
+        }
+        if (isset($parts['user']) && is_string($parts['user'])) {
+            $result .= $parts['user'];
+            if (isset($parts['pass']) && is_string($parts['pass'])) {
+                $result .= ':' . $parts['pass'];
+            }
+            $result .= '@';
+        }
+        if (isset($parts['host']) && is_string($parts['host'])) {
+            $result .= $parts['host'];
+        }
+        if (isset($parts['port']) && is_numeric($parts['port'])) {
+            $result .= ':' . $parts['port'];
+        }
+        if (isset($parts['path']) && is_string($parts['path'])) {
+            $result .= $parts['path'];
+        }
+        if (isset($parts['query']) && is_string($parts['query'])) {
+            $result .= '?' . $parts['query'];
+        }
+
+        return new self($result);
+    }
 }
diff --git a/src/Repository/DefaultUnleashProxyRepository.php b/src/Repository/DefaultUnleashProxyRepository.php
@@ -12,6 +12,7 @@
 use Unleash\Client\Configuration\UnleashContext;
 use Unleash\Client\DTO\DefaultProxyFeature;
 use Unleash\Client\DTO\ProxyFeature;
+use Unleash\Client\Helper\Url;
 
 final readonly class DefaultUnleashProxyRepository implements ProxyRepository
 {
@@ -46,7 +47,7 @@ public function findFeatureByContext(string $featureName, ?Context $context = nu
         }
 
         $context ??= new UnleashContext();
-        $featureUrl = $this->configuration->getUrl() . 'frontend/features/' . $featureName;
+        $featureUrl = (string) Url::appendPath($this->configuration->getUrl(), 'frontend/features/' . $featureName);
         $url = $this->addQuery($featureUrl, $this->contextToQueryString($context));
 
         $request = $this->requestFactory->createRequest('GET', $url)
@@ -135,7 +136,7 @@ private function addQuery(string $url, string $query): string
             $urlParts['query'] = $query;
         }
 
-        return $this->buildUrl($urlParts);
+        return Url::buildUrl($urlParts);
     }
 
     /**
@@ -175,38 +176,4 @@ private function validateResponse(array $response): ?array
 
         return $response;
     }
-
-    /**
-     * @param array<string, mixed> $parts
-     *
-     * @return string
-     */
-    private function buildUrl(array $parts): string
-    {
-        $result = '';
-        if (isset($parts['scheme']) && is_string($parts['scheme'])) {
-            $result .= $parts['scheme'] . '://';
-        }
-        if (isset($parts['user']) && is_string($parts['user'])) {
-            $result .= $parts['user'];
-            if (isset($parts['pass']) && is_string($parts['pass'])) {
-                $result .= ':' . $parts['pass'];
-            }
-            $result .= '@';
-        }
-        if (isset($parts['host']) && is_string($parts['host'])) {
-            $result .= $parts['host'];
-        }
-        if (isset($parts['port']) && is_numeric($parts['port'])) {
-            $result .= ':' . $parts['port'];
-        }
-        if (isset($parts['path']) && is_string($parts['path'])) {
-            $result .= $parts['path'];
-        }
-        if (isset($parts['query']) && is_string($parts['query'])) {
-            $result .= '?' . $parts['query'];
-        }
-
-        return $result;
-    }
 }
diff --git a/src/Repository/DefaultUnleashRepository.php b/src/Repository/DefaultUnleashRepository.php
@@ -33,6 +33,7 @@
 use Unleash\Client\Event\UnleashEvents;
 use Unleash\Client\Exception\HttpResponseException;
 use Unleash\Client\Exception\InvalidValueException;
+use Unleash\Client\Helper\Url;
 
 /**
  * @phpstan-type ConstraintArray array{
@@ -122,7 +123,7 @@ public function getFeatures(): iterable
                 }
             } else {
                 $request = $this->requestFactory
-                    ->createRequest('GET', $this->configuration->getUrl() . 'client/features')
+                    ->createRequest('GET', (string) Url::appendPath($this->configuration->getUrl(), 'client/features'))
                     ->withHeader('UNLEASH-APPNAME', $this->configuration->getAppName())
                     ->withHeader('UNLEASH-INSTANCEID', $this->configuration->getInstanceId())
                     ->withHeader('Unleash-Client-Spec', '4.3.2')

diff --git a/tests/Client/DefaultRegistrationServiceTest.php b/tests/Client/DefaultRegistrationServiceTest.php
@@ -3,10 +3,12 @@
 namespace Unleash\Client\Tests\Client;
 
 use ArrayIterator;
+use GuzzleHttp\Client;
 use GuzzleHttp\Psr7\HttpFactory;
 use RuntimeException;
 use Unleash\Client\Client\DefaultRegistrationService;
 use Unleash\Client\Configuration\UnleashConfiguration;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Strategy\DefaultStrategyHandler;
 use Unleash\Client\Tests\AbstractHttpClientTestCase;
 use Unleash\Client\Tests\Traits\RealCacheImplementationTrait;
@@ -110,4 +112,23 @@ public function testRegistrationException()
         $this->pushResponse(new RuntimeException("This exception shouldn't be propagated"), 1, 404);
         $instance->register([new DefaultStrategyHandler()]);
     }
+
+    public function testUrl()
+    {
+        $configuration = (new UnleashConfiguration(
+            new Url('https://localhost/api', 'somePrefix'),
+            '',
+            ''
+        ))->setCache($this->getCache());
+        $instance = new DefaultRegistrationService(
+            $this->httpClient,
+            new HttpFactory(),
+            $configuration
+        );
+        $this->pushResponse([]);
+
+        $instance->register([]);
+        self::assertCount(1, $this->requestHistory);
+        self::assertSame('https://localhost/api/client/register?namePrefix=somePrefix', (string) $this->requestHistory[0]['request']->getUri());
+    }
 }
diff --git a/tests/Configuration/UnleashConfigurationTest.php b/tests/Configuration/UnleashConfigurationTest.php
@@ -8,6 +8,7 @@
 use Unleash\Client\Configuration\UnleashConfiguration;
 use Unleash\Client\Configuration\UnleashContext;
 use Unleash\Client\ContextProvider\DefaultUnleashContextProvider;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Tests\Traits\FakeCacheImplementationTrait;
 
 final class UnleashConfigurationTest extends TestCase
@@ -17,7 +18,7 @@ final class UnleashConfigurationTest extends TestCase
     public function testConstructor()
     {
         $instance = new UnleashConfiguration('https://www.example.com/test', '', '');
-        self::assertEquals('https://www.example.com/test/', $instance->getUrl());
+        self::assertEquals('https://www.example.com/test', $instance->getUrl());
 
         $context = new UnleashContext('147');
         $instance = new UnleashConfiguration(
@@ -37,7 +38,7 @@ public function testSetUrl()
     {
         $instance = new UnleashConfiguration('', '', '');
         $instance->setUrl('https://www.example.com/test');
-        self::assertEquals('https://www.example.com/test/', $instance->getUrl());
+        self::assertEquals('https://www.example.com/test', $instance->getUrl());
     }
 
     public function testGetCache()
@@ -110,6 +111,15 @@ public function testGetMetricsUrl()
         $proxyInstance->setProxyKey('some-key');
         $resolvedMetricsUrl = $proxyInstance->getMetricsUrl();
         self::assertSame($resolvedMetricsUrl, 'http://localhost:3063/api/frontend/client/metrics');
+
+        $queriedInstance = new UnleashConfiguration(new Url('http://localhost:3063/api', 'somePrefix'), '', '');
+        $resolvedMetricsUrl = $queriedInstance->getMetricsUrl();
+        self::assertSame('http://localhost:3063/api/client/metrics?namePrefix=somePrefix', $resolvedMetricsUrl);
+
+        $queriedInstance = new UnleashConfiguration(new Url('http://localhost:3063/api', 'somePrefix'), '', '');
+        $queriedInstance->setProxyKey('some-key');
+        $resolvedMetricsUrl = $queriedInstance->getMetricsUrl();
+        self::assertSame('http://localhost:3063/api/frontend/client/metrics?namePrefix=somePrefix', $resolvedMetricsUrl);
     }
 
     public function testStringable()

diff --git a/tests/Helper/UrlTest.php b/tests/Helper/UrlTest.php
@@ -48,4 +48,38 @@ public function testCombined()
             (string) $instance
         );
     }
+
+    public function testPreexistingQueryString()
+    {
+        $instance = new Url('https://localhost?someQuery=someValue&someQuery2=someValue2', 'somePrefix', [
+            'someTag' => 'someValue',
+            'someTag2' => 'someValue2',
+        ]);
+        self::assertSame(
+            'https://localhost?someQuery=someValue&someQuery2=someValue2&namePrefix=somePrefix&tag=someTag%3AsomeValue&tag=someTag2%3AsomeValue2',
+            (string) $instance
+        );
+    }
+
+    /**
+     * @dataProvider appendPathData
+     */
+    public function testAppendPath(string $url, string $path, string $expected)
+    {
+        self::assertSame(
+            $expected,
+            (string) Url::appendPath($url, $path)
+        );
+    }
+
+    public function appendPathData(): iterable
+    {
+        yield ['http://localhost', 'test', 'http://localhost/test'];
+        yield ['http://localhost', '/test', 'http://localhost/test'];
+        yield ['http://localhost/', '/test', 'http://localhost/test'];
+        yield ['http://localhost/', '/test/', 'http://localhost/test/'];
+        yield ['http://localhost', '', 'http://localhost'];
+        yield ['http://localhost/test', '/test', 'http://localhost/test/test'];
+        yield ['http://localhost/test?someQuery=someParam', '/test', 'http://localhost/test/test?someQuery=someParam'];
+    }
 }
diff --git a/tests/Repository/DefaultUnleashProxyRepositoryTest.php b/tests/Repository/DefaultUnleashProxyRepositoryTest.php
@@ -12,6 +12,7 @@
 use Unleash\Client\DTO\DefaultVariant;
 use Unleash\Client\DTO\DefaultVariantPayload;
 use Unleash\Client\Enum\Stickiness;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Repository\DefaultUnleashProxyRepository;
 use Unleash\Client\Tests\AbstractHttpClientTestCase;
 use Unleash\Client\Tests\Traits\FakeCacheImplementationTrait;
@@ -150,4 +151,19 @@ public function testCacheTtlIsRespected()
 
         $this->assertCount(2, $container);
     }
+
+    public function testUrl()
+    {
+        $configuration = (new UnleashConfiguration(
+            new Url('https://localhost/api', 'somePrefix'),
+            '',
+            ''
+        ))->setCache($this->getCache())->setProxyKey('test');
+        $instance = new DefaultUnleashProxyRepository($configuration, $this->httpClient, new HttpFactory());
+        $this->pushResponse([]);
+
+        $instance->findFeatureByContext('testFeature');
+        self::assertCount(1, $this->requestHistory);
+        self::assertSame('https://localhost/api/frontend/features/testFeature?namePrefix=somePrefix', (string) $this->requestHistory[0]['request']->getUri());
+    }
 }
diff --git a/tests/Repository/DefaultUnleashRepositoryTest.php b/tests/Repository/DefaultUnleashRepositoryTest.php
@@ -21,6 +21,7 @@
 use Unleash\Client\Event\UnleashEvents;
 use Unleash\Client\Exception\HttpResponseException;
 use Unleash\Client\Exception\InvalidValueException;
+use Unleash\Client\Helper\Url;
 use Unleash\Client\Repository\DefaultUnleashRepository;
 use Unleash\Client\Tests\AbstractHttpClientTestCase;
 use Unleash\Client\Tests\Traits\FakeCacheImplementationTrait;
@@ -467,4 +468,26 @@ function (FetchingDataFailedEvent $event) use (&$failCount): void {
         self::assertNull($lastResponse);
         self::assertEquals(1, $failCount);
     }
+
+    public function testUrl()
+    {
+        $configuration = (new UnleashConfiguration(
+            new Url('https://localhost/api', 'somePrefix'),
+            '',
+            ''
+        ))->setCache($this->getCache());
+        $repository = new DefaultUnleashRepository(
+            new Client([
+                'handler' => $this->handlerStack,
+            ]),
+            new HttpFactory(),
+            $configuration
+        );
+
+        $this->pushResponse($this->response);
+
+        $repository->getFeatures();
+        self::assertCount(1, $this->requestHistory);
+        self::assertSame('https://localhost/api/client/features?namePrefix=somePrefix', (string) $this->requestHistory[0]['request']->getUri());
+    }
 }
diff --git a/tests/UnleashBuilderTest.php b/tests/UnleashBuilderTest.php
@@ -176,7 +176,7 @@ public function testBuild()
         $configuration = $configurationProperty->getValue($repository);
         assert($configuration instanceof UnleashConfiguration);
 
-        self::assertEquals('https://example.com/', $configuration->getUrl());
+        self::assertEquals('https://example.com', $configuration->getUrl());
         self::assertEquals('Test App', $configuration->getAppName());
         self::assertEquals('test', $configuration->getInstanceId());
         self::assertNotNull($configuration->getCache());