Merge pull request #1198 from acelaya-forks/feature/orphan-visits-webhook

Feature/orphan visits webhook
2024-11-27 08:18:24 +03:00 · 2021-10-09 13:08:05 +02:00 · 2021-10-09 13:08:05 +02:00 · db98d811b0
commit db98d811b0
parent bb21ab073f 14ba11e1ab
11 changed files with 139 additions and 37 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -22,6 +22,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
  When they are used in the query, the values are URL encoded.

 * [#1119](https://github.com/shlinkio/shlink/issues/1119) Added support to provide redis sentinel when using redis cache.
+* [#1016](https://github.com/shlinkio/shlink/issues/1016) Added new option to send orphan visits to webhooks, via `NOTIFY_ORPHAN_VISITS_TO_WEBHOOKS` env var or installer tool.
+
+  The option is disabled by default, as the payload is backwards incompatible. You will need to adapt your webhooks to treat the `shortUrl` property as optional before enabling this option.

 ### Changed
 * [#1142](https://github.com/shlinkio/shlink/issues/1142) Replaced `doctrine/cache` package with `symfony/cache`.
--- a/composer.json
+++ b/composer.json
@ -50,7 +50,7 @@
        "shlinkio/shlink-config": "^1.2",
        "shlinkio/shlink-event-dispatcher": "^2.1",
        "shlinkio/shlink-importer": "^2.3.1",
-        "shlinkio/shlink-installer": "dev-develop#2f87995 as 6.2",
+        "shlinkio/shlink-installer": "dev-develop#b45a340 as 6.2",
        "shlinkio/shlink-ip-geolocation": "^2.0",
        "symfony/console": "^5.3",
        "symfony/filesystem": "^5.3",
--- a/config/autoload/installer.global.php
+++ b/config/autoload/installer.global.php
@ -24,6 +24,7 @@ return [
            Option\UrlShortener\ShortDomainSchemaConfigOption::class,
            Option\UrlShortener\ValidateUrlConfigOption::class,
            Option\Visit\VisitsWebhooksConfigOption::class,
+            Option\Visit\OrphanVisitsWebhooksConfigOption::class,
            Option\Redirect\BaseUrlRedirectConfigOption::class,
            Option\Redirect\InvalidShortUrlRedirectConfigOption::class,
            Option\Redirect\Regular404RedirectConfigOption::class,
--- a/config/autoload/redirects.global.php
+++ b/config/autoload/redirects.global.php
@ -4,6 +4,9 @@ declare(strict_types=1);

 use function Shlinkio\Shlink\Common\env;

+use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME;
+use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE;
+
 return [

    'not_found_redirects' => [
@ -12,4 +15,10 @@ return [
        'base_url' => env('BASE_URL_REDIRECT_TO'),
    ],

+    'url_shortener' => [
+        // TODO Move these options to their own config namespace. Maybe "redirects".
+        'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE),
+        'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME),
+    ],
+
 ];
--- a/config/autoload/url-shortener.global.php
+++ b/config/autoload/url-shortener.global.php
@ -4,13 +4,10 @@ declare(strict_types=1);

 use function Shlinkio\Shlink\Common\env;

-use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME;
-use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE;
 use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH;
 use const Shlinkio\Shlink\MIN_SHORT_CODES_LENGTH;

 return (static function (): array {
-    $webhooks = env('VISITS_WEBHOOKS');
    $shortCodesLength = (int) env('DEFAULT_SHORT_CODES_LENGTH', DEFAULT_SHORT_CODES_LENGTH);
    $shortCodesLength = $shortCodesLength < MIN_SHORT_CODES_LENGTH ? MIN_SHORT_CODES_LENGTH : $shortCodesLength;
    $useHttps = env('USE_HTTPS'); // Deprecated. For v3, set this to true by default, instead of null
@ -24,14 +21,9 @@ return (static function (): array {
                'hostname' => env('DEFAULT_DOMAIN', env('SHORT_DOMAIN_HOST', '')),
            ],
            'validate_url' => (bool) env('VALIDATE_URLS', false), // Deprecated
-            'visits_webhooks' => $webhooks === null ? [] : explode(',', $webhooks),
            'default_short_codes_length' => $shortCodesLength,
            'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false),
            'append_extra_path' => (bool) env('REDIRECT_APPEND_EXTRA_PATH', false),
-
-            // TODO Move these two options to their own config namespace. Maybe "redirects".
-            'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE),
-            'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME),
        ],

    ];
--- a/config/autoload/webhooks.global.php
+++ b/config/autoload/webhooks.global.php
@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);
+
+use function Shlinkio\Shlink\Common\env;
+
+return (static function (): array {
+    $webhooks = env('VISITS_WEBHOOKS');
+
+    return [
+
+        'url_shortener' => [
+            // TODO Move these options to their own config namespace
+            'visits_webhooks' => $webhooks === null ? [] : explode(',', $webhooks),
+            'notify_orphan_visits_to_webhooks' => (bool) env('NOTIFY_ORPHAN_VISITS_TO_WEBHOOKS', false),
+        ],
+
+    ];
+})();
--- a/module/Core/config/dependencies.config.php
+++ b/module/Core/config/dependencies.config.php
@ -26,6 +26,7 @@ return [
            Options\UrlShortenerOptions::class => ConfigAbstractFactory::class,
            Options\TrackingOptions::class => ConfigAbstractFactory::class,
            Options\QrCodeOptions::class => ConfigAbstractFactory::class,
+            Options\WebhookOptions::class => ConfigAbstractFactory::class,

            Service\UrlShortener::class => ConfigAbstractFactory::class,
            Service\ShortUrlService::class => ConfigAbstractFactory::class,
@ -88,6 +89,7 @@ return [
        Options\UrlShortenerOptions::class => ['config.url_shortener'],
        Options\TrackingOptions::class => ['config.tracking'],
        Options\QrCodeOptions::class => ['config.qr_codes'],
+        Options\WebhookOptions::class => ['config.url_shortener'], // TODO This config is currently under url_shortener

        Service\UrlShortener::class => [
            ShortUrl\Helper\ShortUrlTitleResolutionHelper::class,
--- a/module/Core/config/event_dispatcher.config.php
+++ b/module/Core/config/event_dispatcher.config.php
@ -58,7 +58,7 @@ return [
            'httpClient',
            'em',
            'Logger_Shlink',
-            'config.url_shortener.visits_webhooks',
+            Options\WebhookOptions::class,
            ShortUrl\Transformer\ShortUrlDataTransformer::class,
            Options\AppOptions::class,
        ],
--- a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php
+++ b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php
@ -4,7 +4,6 @@ declare(strict_types=1);

 namespace Shlinkio\Shlink\Core\EventDispatcher;

-use Closure;
 use Doctrine\ORM\EntityManagerInterface;
 use Fig\Http\Message\RequestMethodInterface;
 use GuzzleHttp\ClientInterface;
@ -17,10 +16,10 @@ use Shlinkio\Shlink\Common\Rest\DataTransformerInterface;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated;
 use Shlinkio\Shlink\Core\Options\AppOptions;
+use Shlinkio\Shlink\Core\Options\WebhookOptions;
 use Throwable;

 use function Functional\map;
-use function Functional\partial_left;

 class NotifyVisitToWebHooks
 {
@ -28,8 +27,7 @@ class NotifyVisitToWebHooks
        private ClientInterface $httpClient,
        private EntityManagerInterface $em,
        private LoggerInterface $logger,
-        /** @var string[] */
-        private array $webhooks,
+        private WebhookOptions $webhookOptions,
        private DataTransformerInterface $transformer,
        private AppOptions $appOptions,
    ) {
@ -37,7 +35,7 @@ class NotifyVisitToWebHooks

    public function __invoke(VisitLocated $shortUrlLocated): void
    {
-        if (empty($this->webhooks)) {
+        if (! $this->webhookOptions->hasWebhooks()) {
            return;
        }

@ -52,6 +50,10 @@ class NotifyVisitToWebHooks
            return;
        }

+        if ($visit->isOrphan() && ! $this->webhookOptions->notifyOrphanVisits()) {
+            return;
+        }
+
        $requestOptions = $this->buildRequestOptions($visit);
        $requestPromises = $this->performRequests($requestOptions, $visitId);

@ -61,15 +63,16 @@ class NotifyVisitToWebHooks

    private function buildRequestOptions(Visit $visit): array
    {
+        $payload = ['visit' => $visit->jsonSerialize()];
+        $shortUrl = $visit->getShortUrl();
+        if ($shortUrl !== null) {
+            $payload['shortUrl'] = $this->transformer->transform($shortUrl);
+        }
+
        return [
            RequestOptions::TIMEOUT => 10,
-            RequestOptions::HEADERS => [
-                'User-Agent' => (string) $this->appOptions,
-            ],
-            RequestOptions::JSON => [
-                'shortUrl' => $this->transformer->transform($visit->getShortUrl()),
-                'visit' => $visit->jsonSerialize(),
-            ],
+            RequestOptions::JSON => $payload,
+            RequestOptions::HEADERS => ['User-Agent' => $this->appOptions->__toString()],
        ];
    }

@ -78,13 +81,11 @@ class NotifyVisitToWebHooks
     */
    private function performRequests(array $requestOptions, string $visitId): array
    {
-        $logWebhookFailure = Closure::fromCallable([$this, 'logWebhookFailure']);
-
        return map(
-            $this->webhooks,
+            $this->webhookOptions->webhooks(),
            fn (string $webhook): PromiseInterface => $this->httpClient
                ->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions)
-                ->otherwise(partial_left($logWebhookFailure, $webhook, $visitId)),
+                ->otherwise(fn (Throwable $e) => $this->logWebhookFailure($webhook, $visitId, $e)),
        );
    }

--- a/module/Core/src/Options/WebhookOptions.php
+++ b/module/Core/src/Options/WebhookOptions.php
@ -0,0 +1,40 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Options;
+
+use Laminas\Stdlib\AbstractOptions;
+
+class WebhookOptions extends AbstractOptions
+{
+    protected $__strictMode__ = false; // phpcs:ignore
+
+    private array $visitsWebhooks = [];
+    private bool $notifyOrphanVisitsToWebhooks = false;
+
+    public function webhooks(): array
+    {
+        return $this->visitsWebhooks;
+    }
+
+    public function hasWebhooks(): bool
+    {
+        return ! empty($this->visitsWebhooks);
+    }
+
+    protected function setVisitsWebhooks(array $visitsWebhooks): void
+    {
+        $this->visitsWebhooks = $visitsWebhooks;
+    }
+
+    public function notifyOrphanVisits(): bool
+    {
+        return $this->notifyOrphanVisitsToWebhooks;
+    }
+
+    protected function setNotifyOrphanVisitsToWebhooks(bool $notifyOrphanVisitsToWebhooks): void
+    {
+        $this->notifyOrphanVisitsToWebhooks = $notifyOrphanVisitsToWebhooks;
+    }
+}
--- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php
+++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php
@ -23,6 +23,7 @@ use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated;
 use Shlinkio\Shlink\Core\EventDispatcher\NotifyVisitToWebHooks;
 use Shlinkio\Shlink\Core\Model\Visitor;
 use Shlinkio\Shlink\Core\Options\AppOptions;
+use Shlinkio\Shlink\Core\Options\WebhookOptions;
 use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier;
 use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer;

@ -76,33 +77,56 @@ class NotifyVisitToWebHooksTest extends TestCase
    }

    /** @test */
-    public function expectedRequestsArePerformedToWebhooks(): void
+    public function orphanVisitDoesNotPerformAnyRequestWhenDisabled(): void
+    {
+        $find = $this->em->find(Visit::class, '1')->willReturn(Visit::forBasePath(Visitor::emptyInstance()));
+        $requestAsync = $this->httpClient->requestAsync(
+            RequestMethodInterface::METHOD_POST,
+            Argument::type('string'),
+            Argument::type('array'),
+        )->willReturn(new FulfilledPromise(''));
+        $logWarning = $this->logger->warning(Argument::cetera());
+
+        $this->createListener(['foo', 'bar'], false)(new VisitLocated('1'));
+
+        $find->shouldHaveBeenCalledOnce();
+        $logWarning->shouldNotHaveBeenCalled();
+        $requestAsync->shouldNotHaveBeenCalled();
+    }
+
+    /**
+     * @test
+     * @dataProvider provideVisits
+     */
+    public function expectedRequestsArePerformedToWebhooks(Visit $visit, array $expectedResponseKeys): void
    {
        $webhooks = ['foo', 'invalid', 'bar', 'baz'];
        $invalidWebhooks = ['invalid', 'baz'];

-        $find = $this->em->find(Visit::class, '1')->willReturn(
-            Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()),
-        );
+        $find = $this->em->find(Visit::class, '1')->willReturn($visit);
        $requestAsync = $this->httpClient->requestAsync(
            RequestMethodInterface::METHOD_POST,
            Argument::type('string'),
-            Argument::that(function (array $requestOptions) {
+            Argument::that(function (array $requestOptions) use ($expectedResponseKeys) {
                Assert::assertArrayHasKey(RequestOptions::HEADERS, $requestOptions);
                Assert::assertArrayHasKey(RequestOptions::JSON, $requestOptions);
                Assert::assertArrayHasKey(RequestOptions::TIMEOUT, $requestOptions);
                Assert::assertEquals($requestOptions[RequestOptions::TIMEOUT], 10);
                Assert::assertEquals($requestOptions[RequestOptions::HEADERS], ['User-Agent' => 'Shlink:v1.2.3']);
-                Assert::assertArrayHasKey('shortUrl', $requestOptions[RequestOptions::JSON]);
-                Assert::assertArrayHasKey('visit', $requestOptions[RequestOptions::JSON]);
+
+                $json = $requestOptions[RequestOptions::JSON];
+                Assert::assertCount(count($expectedResponseKeys), $json);
+                foreach ($expectedResponseKeys as $key) {
+                    Assert::assertArrayHasKey($key, $json);
+                }

                return $requestOptions;
            }),
        )->will(function (array $args) use ($invalidWebhooks) {
            [, $webhook] = $args;
-            $e = new Exception('');
+            $shouldReject = contains($invalidWebhooks, $webhook);

-            return contains($invalidWebhooks, $webhook) ? new RejectedPromise($e) : new FulfilledPromise('');
+            return $shouldReject ? new RejectedPromise(new Exception('')) : new FulfilledPromise('');
        });
        $logWarning = $this->logger->warning(
            'Failed to notify visit with id "{visitId}" to webhook "{webhook}". {e}',
@ -122,13 +146,24 @@ class NotifyVisitToWebHooksTest extends TestCase
        $logWarning->shouldHaveBeenCalledTimes(count($invalidWebhooks));
    }

-    private function createListener(array $webhooks): NotifyVisitToWebHooks
+    public function provideVisits(): iterable
+    {
+        yield 'regular visit' => [
+            Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()),
+            ['shortUrl', 'visit'],
+        ];
+        yield 'orphan visit' => [Visit::forBasePath(Visitor::emptyInstance()), ['visit'],];
+    }
+
+    private function createListener(array $webhooks, bool $notifyOrphanVisits = true): NotifyVisitToWebHooks
    {
        return new NotifyVisitToWebHooks(
            $this->httpClient->reveal(),
            $this->em->reveal(),
            $this->logger->reveal(),
-            $webhooks,
+            new WebhookOptions(
+                ['visits_webhooks' => $webhooks, 'notify_orphan_visits_to_webhooks' => $notifyOrphanVisits],
+            ),
            new ShortUrlDataTransformer(new ShortUrlStringifier([])),
            new AppOptions(['name' => 'Shlink', 'version' => '1.2.3']),
        );