From c718b94937c0acc372e20ec9b7a5934236ec88f6 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sat, 9 Oct 2021 10:35:10 +0200
Subject: [PATCH] Fixed crash when notifying orphan visits to a webhook

---
 .../EventDispatcher/NotifyVisitToWebHooks.php | 21 +++++-------
 .../NotifyVisitToWebHooksTest.php             | 34 +++++++++++++------
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php
index dcd69b21..ad8381be 100644
--- 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;
@@ -20,7 +19,6 @@ use Shlinkio\Shlink\Core\Options\AppOptions;
 use Throwable;
 
 use function Functional\map;
-use function Functional\partial_left;
 
 class NotifyVisitToWebHooks
 {
@@ -61,15 +59,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 +77,11 @@ class NotifyVisitToWebHooks
      */
     private function performRequests(array $requestOptions, string $visitId): array
     {
-        $logWebhookFailure = Closure::fromCallable([$this, 'logWebhookFailure']);
-
         return map(
             $this->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)),
         );
     }
 
diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php
index fcd97d2d..9e884753 100644
--- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php
+++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php
@@ -75,34 +75,39 @@ class NotifyVisitToWebHooksTest extends TestCase
         $requestAsync->shouldNotHaveBeenCalled();
     }
 
-    /** @test */
-    public function expectedRequestsArePerformedToWebhooks(): void
+    /**
+     * @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,6 +127,15 @@ class NotifyVisitToWebHooksTest extends TestCase
         $logWarning->shouldHaveBeenCalledTimes(count($invalidWebhooks));
     }
 
+    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): NotifyVisitToWebHooks
     {
         return new NotifyVisitToWebHooks(