From 6ce2049935f245a124678f1c8b8b1a7e8f0976e7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 25 Jul 2022 12:08:22 +0200 Subject: [PATCH 1/4] Added support for legacy and new publishing of visits in RabbitMQ --- config/autoload/rabbit.global.php | 3 ++ module/Core/config/dependencies.config.php | 2 + .../Core/config/event_dispatcher.config.php | 4 +- module/Core/src/Config/EnvVars.php | 2 + .../RabbitMq/NotifyNewShortUrlToRabbitMq.php | 5 ++- .../RabbitMq/NotifyVisitToRabbitMq.php | 22 +++++----- module/Core/src/Options/RabbitMqOptions.php | 40 +++++++++++++++++++ .../NotifyNewShortUrlToRabbitMqTest.php | 5 ++- .../RabbitMq/NotifyVisitToRabbitMqTest.php | 5 ++- 9 files changed, 70 insertions(+), 18 deletions(-) create mode 100644 module/Core/src/Options/RabbitMqOptions.php diff --git a/config/autoload/rabbit.global.php b/config/autoload/rabbit.global.php index 6f63eca6..ea003809 100644 --- a/config/autoload/rabbit.global.php +++ b/config/autoload/rabbit.global.php @@ -13,6 +13,9 @@ return [ 'user' => EnvVars::RABBITMQ_USER->loadFromEnv(), 'password' => EnvVars::RABBITMQ_PASSWORD->loadFromEnv(), 'vhost' => EnvVars::RABBITMQ_VHOST->loadFromEnv('/'), + + // Deprecated + 'legacy_visits_publishing' => (bool) EnvVars::RABBITMQ_LEGACY_VISITS_PUBLISHING->loadFromEnv(false), ], ]; diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 4afd4805..f4189dde 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -27,6 +27,7 @@ return [ Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => ConfigAbstractFactory::class, Options\QrCodeOptions::class => ConfigAbstractFactory::class, + Options\RabbitMqOptions::class => ConfigAbstractFactory::class, Options\WebhookOptions::class => ConfigAbstractFactory::class, Service\UrlShortener::class => ConfigAbstractFactory::class, @@ -91,6 +92,7 @@ return [ Options\UrlShortenerOptions::class => ['config.url_shortener'], Options\TrackingOptions::class => ['config.tracking'], Options\QrCodeOptions::class => ['config.qr_codes'], + Options\RabbitMqOptions::class => ['config.rabbitmq'], Options\WebhookOptions::class => ['config.visits_webhooks'], Service\UrlShortener::class => [ diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 9ae99e08..96907a5d 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -98,14 +98,14 @@ return [ 'Logger_Shlink', Visit\Transformer\OrphanVisitDataTransformer::class, ShortUrl\Transformer\ShortUrlDataTransformer::class, - 'config.rabbitmq.enabled', + Options\RabbitMqOptions::class, ], EventDispatcher\RabbitMq\NotifyNewShortUrlToRabbitMq::class => [ RabbitMqPublishingHelper::class, 'em', 'Logger_Shlink', ShortUrl\Transformer\ShortUrlDataTransformer::class, - 'config.rabbitmq.enabled', + Options\RabbitMqOptions::class, ], EventDispatcher\UpdateGeoLiteDb::class => [GeolocationDbUpdater::class, 'Logger_Shlink'], ], diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index a85181f2..33abae01 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -33,6 +33,8 @@ enum EnvVars: string case RABBITMQ_USER = 'RABBITMQ_USER'; case RABBITMQ_PASSWORD = 'RABBITMQ_PASSWORD'; case RABBITMQ_VHOST = 'RABBITMQ_VHOST'; + /** @deprecated */ + case RABBITMQ_LEGACY_VISITS_PUBLISHING = 'RABBITMQ_LEGACY_VISITS_PUBLISHING'; case DEFAULT_INVALID_SHORT_URL_REDIRECT = 'DEFAULT_INVALID_SHORT_URL_REDIRECT'; case DEFAULT_REGULAR_404_REDIRECT = 'DEFAULT_REGULAR_404_REDIRECT'; case DEFAULT_BASE_URL_REDIRECT = 'DEFAULT_BASE_URL_REDIRECT'; diff --git a/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php b/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php index 583f9420..22a7582d 100644 --- a/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php +++ b/module/Core/src/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMq.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlCreated; use Shlinkio\Shlink\Core\EventDispatcher\Topic; +use Shlinkio\Shlink\Core\Options\RabbitMqOptions; use Throwable; class NotifyNewShortUrlToRabbitMq @@ -20,13 +21,13 @@ class NotifyNewShortUrlToRabbitMq private readonly EntityManagerInterface $em, private readonly LoggerInterface $logger, private readonly DataTransformerInterface $shortUrlTransformer, - private readonly bool $isEnabled, + private readonly RabbitMqOptions $options, ) { } public function __invoke(ShortUrlCreated $shortUrlCreated): void { - if (! $this->isEnabled) { + if (! $this->options->isEnabled()) { return; } diff --git a/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php b/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php index 897bff29..34a951ec 100644 --- a/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php +++ b/module/Core/src/EventDispatcher/RabbitMq/NotifyVisitToRabbitMq.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\Core\EventDispatcher\Topic; +use Shlinkio\Shlink\Core\Options\RabbitMqOptions; use Throwable; class NotifyVisitToRabbitMq @@ -20,14 +21,14 @@ class NotifyVisitToRabbitMq private readonly EntityManagerInterface $em, private readonly LoggerInterface $logger, private readonly DataTransformerInterface $orphanVisitTransformer, - private readonly DataTransformerInterface $shortUrlTransformer, // @phpstan-ignore-line - private readonly bool $isEnabled, + private readonly DataTransformerInterface $shortUrlTransformer, + private readonly RabbitMqOptions $options, ) { } public function __invoke(VisitLocated $shortUrlLocated): void { - if (! $this->isEnabled) { + if (! $this->options->isEnabled()) { return; } @@ -70,14 +71,15 @@ class NotifyVisitToRabbitMq private function visitToPayload(Visit $visit): array { - // FIXME This was defined incorrectly. - // According to the spec, both the visit and the short URL it belongs to, should be published. - // The shape should be ['visit' => [...], 'shortUrl' => ?[...]] - // However, this would be a breaking change, so we need a flag that determines the shape of the payload. + // This was defined incorrectly. + // According to the spec, both the visit and the short URL it belongs to, should be published. + // The shape should be ['visit' => [...], 'shortUrl' => ?[...]] + // However, this would be a breaking change, so we need a flag that determines the shape of the payload. + if ($this->options->legacyVisitsPublishing()) { + return ! $visit->isOrphan() ? $visit->jsonSerialize() : $this->orphanVisitTransformer->transform($visit); + } - return ! $visit->isOrphan() ? $visit->jsonSerialize() : $this->orphanVisitTransformer->transform($visit); - - if ($visit->isOrphan()) { // @phpstan-ignore-line + if ($visit->isOrphan()) { return ['visit' => $this->orphanVisitTransformer->transform($visit)]; } diff --git a/module/Core/src/Options/RabbitMqOptions.php b/module/Core/src/Options/RabbitMqOptions.php new file mode 100644 index 00000000..388cd2ea --- /dev/null +++ b/module/Core/src/Options/RabbitMqOptions.php @@ -0,0 +1,40 @@ +enabled; + } + + protected function setEnabled(bool $enabled): self + { + $this->enabled = $enabled; + return $this; + } + + /** @deprecated */ + public function legacyVisitsPublishing(): bool + { + return $this->legacyVisitsPublishing; + } + + /** @deprecated */ + protected function setLegacyVisitsPublishing(bool $legacyVisitsPublishing): self + { + $this->legacyVisitsPublishing = $legacyVisitsPublishing; + return $this; + } +} diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php index 51b557af..0112da1a 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlCreated; use Shlinkio\Shlink\Core\EventDispatcher\RabbitMq\NotifyNewShortUrlToRabbitMq; use Shlinkio\Shlink\Core\EventDispatcher\Topic; +use Shlinkio\Shlink\Core\Options\RabbitMqOptions; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; use Throwable; @@ -42,7 +43,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $this->em->reveal(), $this->logger->reveal(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - true, + new RabbitMqOptions(['enabled' => true]), ); } @@ -54,7 +55,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $this->em->reveal(), $this->logger->reveal(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - false, + new RabbitMqOptions(['enabled' => false]), ); $listener(new ShortUrlCreated('123')); diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php index 2558dfc3..04bcdf6a 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php @@ -20,6 +20,7 @@ use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\Core\EventDispatcher\RabbitMq\NotifyVisitToRabbitMq; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Options\RabbitMqOptions; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Core\Visit\Transformer\OrphanVisitDataTransformer; @@ -49,7 +50,7 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->logger->reveal(), new OrphanVisitDataTransformer(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - true, + new RabbitMqOptions(['enabled' => true, 'legacy_visits_publishing' => true]), ); } @@ -62,7 +63,7 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->logger->reveal(), new OrphanVisitDataTransformer(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - false, + new RabbitMqOptions(['enabled' => false, 'legacy_visits_publishing' => true]), ); $listener(new VisitLocated('123')); From 19b0f0d7dc7b174a379755b385891137d8115770 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 25 Jul 2022 12:30:28 +0200 Subject: [PATCH 2/4] Extended NotifyVisitToRabbitMqTest covering legacy and non-legacy use-cases --- .../RabbitMq/NotifyVisitToRabbitMqTest.php | 91 +++++++++++++++++-- 1 file changed, 81 insertions(+), 10 deletions(-) diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php index 04bcdf6a..6d367c17 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php @@ -7,6 +7,7 @@ namespace ShlinkioTest\Shlink\Core\EventDispatcher\RabbitMq; use Doctrine\ORM\EntityManagerInterface; use DomainException; use Exception; +use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -37,12 +38,14 @@ class NotifyVisitToRabbitMqTest extends TestCase private ObjectProphecy $helper; private ObjectProphecy $em; private ObjectProphecy $logger; + private RabbitMqOptions $options; protected function setUp(): void { $this->helper = $this->prophesize(RabbitMqPublishingHelperInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $this->logger = $this->prophesize(LoggerInterface::class); + $this->options = new RabbitMqOptions(['enabled' => true, 'legacy_visits_publishing' => true]); $this->listener = new NotifyVisitToRabbitMq( $this->helper->reveal(), @@ -50,23 +53,16 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->logger->reveal(), new OrphanVisitDataTransformer(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - new RabbitMqOptions(['enabled' => true, 'legacy_visits_publishing' => true]), + $this->options, ); } /** @test */ public function doesNothingWhenTheFeatureIsNotEnabled(): void { - $listener = new NotifyVisitToRabbitMq( - $this->helper->reveal(), - $this->em->reveal(), - $this->logger->reveal(), - new OrphanVisitDataTransformer(), - new ShortUrlDataTransformer(new ShortUrlStringifier([])), - new RabbitMqOptions(['enabled' => false, 'legacy_visits_publishing' => true]), - ); + $this->options->enabled = false; - $listener(new VisitLocated('123')); + ($this->listener)(new VisitLocated('123')); $this->em->find(Argument::cetera())->shouldNotHaveBeenCalled(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); @@ -157,4 +153,79 @@ class NotifyVisitToRabbitMqTest extends TestCase yield [new Exception('Exception Error')]; yield [new DomainException('DomainException Error')]; } + + /** + * @test + * @dataProvider provideLegacyPayloads + */ + public function expectedPayloadIsPublishedDependingOnConfig( + bool $legacy, + Visit $visit, + callable $assertPayload, + ): void { + $this->options->legacyVisitsPublishing = $legacy; + + $visitId = '123'; + $findVisit = $this->em->find(Visit::class, $visitId)->willReturn($visit); + + ($this->listener)(new VisitLocated($visitId)); + + $findVisit->shouldHaveBeenCalledOnce(); + $this->helper->publishPayloadInQueue(Argument::that($assertPayload), Argument::type('string')) + ->shouldHaveBeenCalled(); + } + + public function provideLegacyPayloads(): iterable + { + yield 'non-legacy non-orphan visit' => [ + true, + $visit = Visit::forValidShortUrl(ShortUrl::withLongUrl(''), Visitor::emptyInstance()), + function (array $payload) use ($visit): bool { + Assert::assertEquals($payload, $visit->jsonSerialize()); + Assert::assertArrayNotHasKey('visitedUrl', $payload); + Assert::assertArrayNotHasKey('type', $payload); + Assert::assertArrayNotHasKey('visit', $payload); + Assert::assertArrayNotHasKey('shortUrl', $payload); + + return true; + }, + ]; + yield 'non-legacy orphan visit' => [ + true, + Visit::forBasePath(Visitor::emptyInstance()), + function (array $payload): bool { + Assert::assertArrayHasKey('visitedUrl', $payload); + Assert::assertArrayHasKey('type', $payload); + + return true; + }, + ]; + yield 'legacy non-orphan visit' => [ + false, + $visit = Visit::forValidShortUrl(ShortUrl::withLongUrl(''), Visitor::emptyInstance()), + function (array $payload) use ($visit): bool { + Assert::assertArrayHasKey('visit', $payload); + Assert::assertArrayHasKey('shortUrl', $payload); + Assert::assertIsArray($payload['visit']); + Assert::assertEquals($payload['visit'], $visit->jsonSerialize()); + Assert::assertArrayNotHasKey('visitedUrl', ['visit']); + Assert::assertArrayNotHasKey('type', ['visit']); + + return true; + }, + ]; + yield 'legacy orphan visit' => [ + false, + Visit::forBasePath(Visitor::emptyInstance()), + function (array $payload): bool { + Assert::assertArrayHasKey('visit', $payload); + Assert::assertArrayNotHasKey('shortUrl', $payload); + Assert::assertIsArray($payload['visit']); + Assert::assertArrayHasKey('visitedUrl', $payload['visit']); + Assert::assertArrayHasKey('type', $payload['visit']); + + return true; + }, + ]; + } } From cd27a7298236a66d2288f7e569e1ffdb4d3b0510 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 25 Jul 2022 12:31:32 +0200 Subject: [PATCH 3/4] Reduced duplicated code in NotifyNewShortUrlToRabbitMqTest --- .../RabbitMq/NotifyNewShortUrlToRabbitMqTest.php | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php index 0112da1a..e98e342f 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php @@ -31,34 +31,30 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase private ObjectProphecy $helper; private ObjectProphecy $em; private ObjectProphecy $logger; + private RabbitMqOptions $options; protected function setUp(): void { $this->helper = $this->prophesize(RabbitMqPublishingHelperInterface::class); $this->em = $this->prophesize(EntityManagerInterface::class); $this->logger = $this->prophesize(LoggerInterface::class); + $this->options = new RabbitMqOptions(['enabled' => true]); $this->listener = new NotifyNewShortUrlToRabbitMq( $this->helper->reveal(), $this->em->reveal(), $this->logger->reveal(), new ShortUrlDataTransformer(new ShortUrlStringifier([])), - new RabbitMqOptions(['enabled' => true]), + $this->options, ); } /** @test */ public function doesNothingWhenTheFeatureIsNotEnabled(): void { - $listener = new NotifyNewShortUrlToRabbitMq( - $this->helper->reveal(), - $this->em->reveal(), - $this->logger->reveal(), - new ShortUrlDataTransformer(new ShortUrlStringifier([])), - new RabbitMqOptions(['enabled' => false]), - ); + $this->options->enabled = false; - $listener(new ShortUrlCreated('123')); + ($this->listener)(new ShortUrlCreated('123')); $this->em->find(Argument::cetera())->shouldNotHaveBeenCalled(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); From 122c2fd5e63fdf6aeeed6c645fcb0600af0bb5af Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 25 Jul 2022 12:34:40 +0200 Subject: [PATCH 4/4] Updated changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index de782f25..bd8a5e57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Changed * [#1452](https://github.com/shlinkio/shlink/issues/1452) Updated to monolog 3 +* [#1485](https://github.com/shlinkio/shlink/issues/1485) Changed payload published in RabbitMQ for all visits events, in order to conform with the Async API spec. + + Since this is a breaking change, also provided a new `RABBITMQ_LEGACY_VISITS_PUBLISHING=true` env var that can be provided in order to keep the old payload. + + This env var is considered deprecated and will be removed in Shlink 4, when the legacy format will no longer be supported. ### Deprecated * *Nothing*