Add a 3-second timeout to title resolution

2026-03-11 09:43:13 +08:00 · 2024-02-17 12:13:05 +01:00
parent e5713df008 e3de403c6c
commit d3bfd99210
29 changed files with 218 additions and 624 deletions
--- a/module/Core/test/Exception/InvalidUrlExceptionTest.php
+++ b/module/Core/test/Exception/InvalidUrlExceptionTest.php
@@ -1,41 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace ShlinkioTest\Shlink\Core\Exception;
-
-use Exception;
-use Fig\Http\Message\StatusCodeInterface;
-use PHPUnit\Framework\Attributes\DataProvider;
-use PHPUnit\Framework\Attributes\Test;
-use PHPUnit\Framework\TestCase;
-use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
-use Throwable;
-
-use function sprintf;
-
-class InvalidUrlExceptionTest extends TestCase
-{
-    #[Test, DataProvider('providePrevious')]
-    public function properlyCreatesExceptionFromUrl(?Throwable $prev): void
-    {
-        $url = 'http://the_url.com';
-        $expectedMessage = sprintf('Provided URL %s is invalid. Try with a different one.', $url);
-        $e = InvalidUrlException::fromUrl($url, $prev);
-
-        self::assertEquals($expectedMessage, $e->getMessage());
-        self::assertEquals($expectedMessage, $e->getDetail());
-        self::assertEquals('Invalid URL', $e->getTitle());
-        self::assertEquals('https://shlink.io/api/error/invalid-url', $e->getType());
-        self::assertEquals(['url' => $url], $e->getAdditionalData());
-        self::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $e->getCode());
-        self::assertEquals(StatusCodeInterface::STATUS_BAD_REQUEST, $e->getStatus());
-        self::assertEquals($prev, $e->getPrevious());
-    }
-
-    public static function providePrevious(): iterable
-    {
-        yield 'null previous' => [null];
-        yield 'instance previous' => [new Exception('Previous error', 10)];
-    }
-}
--- a/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
+++ b/module/Core/test/ShortUrl/Helper/ShortUrlTitleResolutionHelperTest.php
@@ -4,46 +4,144 @@ declare(strict_types=1);

 namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper;

-use PHPUnit\Framework\Attributes\DataProvider;
+use Exception;
+use Fig\Http\Message\RequestMethodInterface;
+use GuzzleHttp\ClientInterface;
+use GuzzleHttp\RequestOptions;
+use Laminas\Diactoros\Response;
+use Laminas\Diactoros\Response\JsonResponse;
+use Laminas\Diactoros\Stream;
 use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\MockObject\Builder\InvocationMocker;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
 use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelper;
 use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation;
-use Shlinkio\Shlink\Core\Util\UrlValidatorInterface;

 class ShortUrlTitleResolutionHelperTest extends TestCase
 {
-    private ShortUrlTitleResolutionHelper $helper;
-    private MockObject & UrlValidatorInterface $urlValidator;
+    private const LONG_URL = 'http://foobar.com/12345/hello?foo=bar';
+
+    private MockObject & ClientInterface $httpClient;

    protected function setUp(): void
    {
-        $this->urlValidator = $this->createMock(UrlValidatorInterface::class);
-        $this->helper = new ShortUrlTitleResolutionHelper($this->urlValidator);
+        $this->httpClient = $this->createMock(ClientInterface::class);
    }

-    #[Test, DataProvider('provideTitles')]
-    public function urlIsProperlyShortened(?string $title, int $validateWithTitleCallsNum, int $validateCallsNum): void
+    #[Test]
+    public function dataIsReturnedAsIsWhenResolvingTitlesIsDisabled(): void
    {
-        $longUrl = 'http://foobar.com/12345/hello?foo=bar';
-        $this->urlValidator->expects($this->exactly($validateWithTitleCallsNum))->method('validateUrlWithTitle')->with(
-            $longUrl,
-            $this->isFalse(),
-        );
-        $this->urlValidator->expects($this->exactly($validateCallsNum))->method('validateUrl')->with(
-            $longUrl,
-            $this->isFalse(),
-        );
+        $data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
+        $this->httpClient->expects($this->never())->method('request');

-        $this->helper->processTitleAndValidateUrl(
-            ShortUrlCreation::fromRawData(['longUrl' => $longUrl, 'title' => $title]),
+        $result = $this->helper()->processTitle($data);
+
+        self::assertSame($data, $result);
+    }
+
+    #[Test]
+    public function dataIsReturnedAsIsWhenItAlreadyHasTitle(): void
+    {
+        $data = ShortUrlCreation::fromRawData([
+            'longUrl' => self::LONG_URL,
+            'title' => 'foo',
+        ]);
+        $this->httpClient->expects($this->never())->method('request');
+
+        $result = $this->helper(autoResolveTitles: true)->processTitle($data);
+
+        self::assertSame($data, $result);
+    }
+
+    #[Test]
+    public function dataIsReturnedAsIsWhenFetchingFails(): void
+    {
+        $data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
+        $this->expectRequestToBeCalled()->willThrowException(new Exception('Error'));
+
+        $result = $this->helper(autoResolveTitles: true)->processTitle($data);
+
+        self::assertSame($data, $result);
+    }
+
+    #[Test]
+    public function dataIsReturnedAsIsWhenResponseIsNotHtml(): void
+    {
+        $data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
+        $this->expectRequestToBeCalled()->willReturn(new JsonResponse(['foo' => 'bar']));
+
+        $result = $this->helper(autoResolveTitles: true)->processTitle($data);
+
+        self::assertSame($data, $result);
+    }
+
+    #[Test]
+    public function dataIsReturnedAsIsWhenTitleCannotBeResolvedFromResponse(): void
+    {
+        $data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
+        $this->expectRequestToBeCalled()->willReturn($this->respWithoutTitle());
+
+        $result = $this->helper(autoResolveTitles: true)->processTitle($data);
+
+        self::assertSame($data, $result);
+    }
+
+    #[Test]
+    public function titleIsUpdatedWhenItCanBeResolvedFromResponse(): void
+    {
+        $data = ShortUrlCreation::fromRawData(['longUrl' => self::LONG_URL]);
+        $this->expectRequestToBeCalled()->willReturn($this->respWithTitle());
+
+        $result = $this->helper(autoResolveTitles: true)->processTitle($data);
+
+        self::assertNotSame($data, $result);
+        self::assertEquals('Resolved "title"', $result->title);
+    }
+
+    private function expectRequestToBeCalled(): InvocationMocker
+    {
+        return $this->httpClient->expects($this->once())->method('request')->with(
+            RequestMethodInterface::METHOD_GET,
+            self::LONG_URL,
+            [
+                RequestOptions::TIMEOUT => 3,
+                RequestOptions::CONNECT_TIMEOUT => 3,
+                RequestOptions::ALLOW_REDIRECTS => ['max' => ShortUrlTitleResolutionHelper::MAX_REDIRECTS],
+                RequestOptions::IDN_CONVERSION => true,
+                RequestOptions::HEADERS => ['User-Agent' => ShortUrlTitleResolutionHelper::CHROME_USER_AGENT],
+                RequestOptions::STREAM => true,
+            ],
        );
    }

-    public static function provideTitles(): iterable
+    private function respWithoutTitle(): Response
    {
-        yield 'no title' => [null, 1, 0];
-        yield 'title' => ['link title', 0, 1];
+        $body = $this->createStreamWithContent('<body>No title</body>');
+        return new Response($body, 200, ['Content-Type' => 'text/html']);
+    }
+
+    private function respWithTitle(): Response
+    {
+        $body = $this->createStreamWithContent('<title data-foo="bar">  Resolved &quot;title&quot; </title>');
+        return new Response($body, 200, ['Content-Type' => 'TEXT/html; charset=utf-8']);
+    }
+
+    private function createStreamWithContent(string $content): Stream
+    {
+        $body = new Stream('php://temp', 'wr');
+        $body->write($content);
+        $body->rewind();
+
+        return $body;
+    }
+
+    private function helper(bool $autoResolveTitles = false): ShortUrlTitleResolutionHelper
+    {
+        return new ShortUrlTitleResolutionHelper(
+            $this->httpClient,
+            new UrlShortenerOptions(autoResolveTitles: $autoResolveTitles),
+        );
    }
 }
--- a/module/Core/test/ShortUrl/ShortUrlServiceTest.php
+++ b/module/Core/test/ShortUrl/ShortUrlServiceTest.php
@@ -63,7 +63,7 @@ class ShortUrlServiceTest extends TestCase
        )->willReturn($shortUrl);

        $this->titleResolutionHelper->expects($expectedValidateCalls)
-                                    ->method('processTitleAndValidateUrl')
+                                    ->method('processTitle')
                                    ->with($shortUrlEdit)
                                    ->willReturn($shortUrlEdit);

@@ -102,10 +102,6 @@ class ShortUrlServiceTest extends TestCase
            'maxVisits' => 10,
            'longUrl' => 'https://modifiedLongUrl',
        ]), ApiKey::create()];
-        yield 'long URL with validation' => [new InvokedCount(1), ShortUrlEdition::fromRawData([
-            'longUrl' => 'https://modifiedLongUrl',
-            'validateUrl' => true,
-        ]), null];
        yield 'device redirects' => [new InvokedCount(0), ShortUrlEdition::fromRawData([
            'deviceLongUrls' => [
                DeviceType::IOS->value => 'https://iosLongUrl',
--- a/module/Core/test/ShortUrl/UrlShortenerTest.php
+++ b/module/Core/test/ShortUrl/UrlShortenerTest.php
@@ -57,7 +57,7 @@ class UrlShortenerTest extends TestCase
    {
        $longUrl = 'http://foobar.com/12345/hello?foo=bar';
        $meta = ShortUrlCreation::fromRawData(['longUrl' => $longUrl]);
-        $this->titleResolutionHelper->expects($this->once())->method('processTitleAndValidateUrl')->with(
+        $this->titleResolutionHelper->expects($this->once())->method('processTitle')->with(
            $meta,
        )->willReturnArgument(0);
        $this->shortCodeHelper->method('ensureShortCodeUniqueness')->willReturn(true);
@@ -90,7 +90,7 @@ class UrlShortenerTest extends TestCase
        );

        $this->shortCodeHelper->expects($this->once())->method('ensureShortCodeUniqueness')->willReturn(false);
-        $this->titleResolutionHelper->expects($this->once())->method('processTitleAndValidateUrl')->with(
+        $this->titleResolutionHelper->expects($this->once())->method('processTitle')->with(
            $meta,
        )->willReturnArgument(0);

@@ -105,7 +105,7 @@ class UrlShortenerTest extends TestCase
        $repo = $this->createMock(ShortUrlRepository::class);
        $repo->expects($this->once())->method('findOneMatching')->willReturn($expected);
        $this->em->expects($this->once())->method('getRepository')->with(ShortUrl::class)->willReturn($repo);
-        $this->titleResolutionHelper->expects($this->never())->method('processTitleAndValidateUrl');
+        $this->titleResolutionHelper->expects($this->never())->method('processTitle');
        $this->shortCodeHelper->method('ensureShortCodeUniqueness')->willReturn(true);

        $result = $this->urlShortener->shorten($meta);
--- a/module/Core/test/Util/UrlValidatorTest.php
+++ b/module/Core/test/Util/UrlValidatorTest.php
@@ -1,176 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace ShlinkioTest\Shlink\Core\Util;
-
-use Fig\Http\Message\RequestMethodInterface;
-use GuzzleHttp\ClientInterface;
-use GuzzleHttp\Exception\ClientException;
-use GuzzleHttp\Psr7\Request;
-use GuzzleHttp\RequestOptions;
-use Laminas\Diactoros\Response;
-use Laminas\Diactoros\Stream;
-use PHPUnit\Framework\Assert;
-use PHPUnit\Framework\Attributes\Test;
-use PHPUnit\Framework\MockObject\MockObject;
-use PHPUnit\Framework\TestCase;
-use Shlinkio\Shlink\Core\Exception\InvalidUrlException;
-use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
-use Shlinkio\Shlink\Core\Util\UrlValidator;
-
-class UrlValidatorTest extends TestCase
-{
-    private MockObject & ClientInterface $httpClient;
-
-    protected function setUp(): void
-    {
-        $this->httpClient = $this->createMock(ClientInterface::class);
-    }
-
-    #[Test]
-    public function exceptionIsThrownWhenUrlIsInvalid(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->willThrowException($this->clientException());
-        $this->expectException(InvalidUrlException::class);
-
-        $this->urlValidator()->validateUrl('http://foobar.com/12345/hello?foo=bar', true);
-    }
-
-    #[Test]
-    public function expectedUrlIsCalledWhenTryingToVerify(): void
-    {
-        $expectedUrl = 'http://foobar.com';
-
-        $this->httpClient->expects($this->once())->method('request')->with(
-            RequestMethodInterface::METHOD_GET,
-            $expectedUrl,
-            $this->callback(function (array $options) {
-                Assert::assertArrayHasKey(RequestOptions::ALLOW_REDIRECTS, $options);
-                Assert::assertEquals(['max' => 15], $options[RequestOptions::ALLOW_REDIRECTS]);
-                Assert::assertArrayHasKey(RequestOptions::IDN_CONVERSION, $options);
-                Assert::assertTrue($options[RequestOptions::IDN_CONVERSION]);
-                Assert::assertArrayHasKey(RequestOptions::HEADERS, $options);
-                Assert::assertArrayHasKey('User-Agent', $options[RequestOptions::HEADERS]);
-
-                return true;
-            }),
-        )->willReturn(new Response());
-
-        $this->urlValidator()->validateUrl($expectedUrl, true);
-    }
-
-    #[Test]
-    public function noCheckIsPerformedWhenUrlValidationIsDisabled(): void
-    {
-        $this->httpClient->expects($this->never())->method('request');
-        $this->urlValidator()->validateUrl('', false);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleReturnsNullWhenRequestFailsAndValidationIsDisabled(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->willThrowException($this->clientException());
-
-        $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false);
-
-        self::assertNull($result);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabled(): void
-    {
-        $this->httpClient->expects($this->never())->method('request');
-
-        $result = $this->urlValidator()->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', false);
-
-        self::assertNull($result);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsDisabledAndValidationIsEnabled(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->with(
-            RequestMethodInterface::METHOD_HEAD,
-            $this->anything(),
-            $this->anything(),
-        )->willReturn($this->respWithTitle());
-
-        $result = $this->urlValidator()->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
-
-        self::assertNull($result);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleResolvesTitleWhenAutoResolutionIsEnabled(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->with(
-            RequestMethodInterface::METHOD_GET,
-            $this->anything(),
-            $this->anything(),
-        )->willReturn($this->respWithTitle());
-
-        $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
-
-        self::assertEquals('Resolved "title"', $result);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsEnabledAndReturnedContentTypeIsInvalid(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->with(
-            RequestMethodInterface::METHOD_GET,
-            $this->anything(),
-            $this->anything(),
-        )->willReturn(new Response('php://memory', 200, ['Content-Type' => 'application/octet-stream']));
-
-        $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
-
-        self::assertNull($result);
-    }
-
-    #[Test]
-    public function validateUrlWithTitleReturnsNullWhenAutoResolutionIsEnabledAndBodyDoesNotContainTitle(): void
-    {
-        $this->httpClient->expects($this->once())->method('request')->with(
-            RequestMethodInterface::METHOD_GET,
-            $this->anything(),
-            $this->anything(),
-        )->willReturn(
-            new Response($this->createStreamWithContent('<body>No title</body>'), 200, ['Content-Type' => 'text/html']),
-        );
-
-        $result = $this->urlValidator(true)->validateUrlWithTitle('http://foobar.com/12345/hello?foo=bar', true);
-
-        self::assertNull($result);
-    }
-
-    private function respWithTitle(): Response
-    {
-        $body = $this->createStreamWithContent('<title data-foo="bar">  Resolved &quot;title&quot; </title>');
-        return new Response($body, 200, ['Content-Type' => 'TEXT/html; charset=utf-8']);
-    }
-
-    private function createStreamWithContent(string $content): Stream
-    {
-        $body = new Stream('php://temp', 'wr');
-        $body->write($content);
-        $body->rewind();
-
-        return $body;
-    }
-
-    private function clientException(): ClientException
-    {
-        return new ClientException(
-            '',
-            new Request(RequestMethodInterface::METHOD_GET, ''),
-            new Response(),
-        );
-    }
-
-    public function urlValidator(bool $autoResolveTitles = false): UrlValidator
-    {
-        return new UrlValidator($this->httpClient, new UrlShortenerOptions(autoResolveTitles: $autoResolveTitles));
-    }
-}