Merge pull request #1145 from acelaya-forks/feature/update-cache

Feature/update cache
2025-02-25 18:45:27 -06:00 · 2021-08-05 17:07:41 +02:00 · 2021-08-05 17:07:41 +02:00 · 60d6314262
commit 60d6314262
parent 555e6f804c eff7445804
17 changed files with 110 additions and 37 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file.

 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).

+## [Unreleased]
+### Added
+* *Nothing*
+
+### Changed
+* [#1142](https://github.com/shlinkio/shlink/issues/1142) Replaced `doctrine/cache` package with `symfony/cache`.
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1098](https://github.com/shlinkio/shlink/issues/1098) Fixed errors when using a Redis Cluster for caching, caused by `doctrine/cache` not fully supporting clusters.
+
+
 ## [2.8.0] - 2021-08-04
 ### Added
 * [#1089](https://github.com/shlinkio/shlink/issues/1089) Added new `ENABLE_PERIODIC_VISIT_LOCATE` env var to docker image which schedules the `visit:locate` command every hour when provided with value `true`.
--- a/composer.json
+++ b/composer.json
@ -18,7 +18,6 @@
        "akrabat/ip-address-middleware": "^2.0",
        "cakephp/chronos": "^2.2",
        "cocur/slugify": "^4.0",
-        "doctrine/cache": "^1.12",
        "doctrine/migrations": "^3.2",
        "doctrine/orm": "^2.9",
        "endroid/qr-code": "^4.2",
@ -47,7 +46,7 @@
        "predis/predis": "^1.1",
        "pugx/shortid-php": "^0.7",
        "ramsey/uuid": "^3.9",
-        "shlinkio/shlink-common": "^3.7",
+        "shlinkio/shlink-common": "dev-main#2832a4a as 4.0",
        "shlinkio/shlink-config": "^1.2",
        "shlinkio/shlink-event-dispatcher": "^2.1",
        "shlinkio/shlink-importer": "^2.3.1",
--- a/config/autoload/locks.global.php
+++ b/config/autoload/locks.global.php
@ -3,7 +3,7 @@
 declare(strict_types=1);

 use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory;
-use Shlinkio\Shlink\Common\Cache\RedisFactory;
+use Predis\ClientInterface as PredisClient;
 use Shlinkio\Shlink\Common\Lock\RetryLockStoreDelegatorFactory;
 use Shlinkio\Shlink\Common\Logger\LoggerAwareDelegatorFactory;
 use Symfony\Component\Lock;
@ -42,7 +42,7 @@ return [

    ConfigAbstractFactory::class => [
        Lock\Store\FlockStore::class => ['config.locks.locks_dir'],
-        Lock\Store\RedisStore::class => [RedisFactory::SERVICE_NAME],
+        Lock\Store\RedisStore::class => [PredisClient::class],
        Lock\LockFactory::class => ['lock_store'],
        LOCAL_LOCK_FACTORY => ['local_lock_store'],
    ],
--- a/docs/adr/2021-08-05-migrate-to-a-new-caching-library.md
+++ b/docs/adr/2021-08-05-migrate-to-a-new-caching-library.md
@ -0,0 +1,59 @@
+# Migrate to a new caching library
+
+* Status: Accepted
+* Date: 2021-08-05
+
+## Context and problem statement
+
+Shlink has always used the `doctrine/cache` library to handle anything related with cache.
+
+It was convenient, as it provided several adapters, and it was the library used by other doctrine packages.
+
+However, after the creation of the caching PSRs ([PSR-6 - Cache](https://www.php-fig.org/psr/psr-6) and [PSR-16 - Simple cache](https://www.php-fig.org/psr/psr-16)), most library authors have moved to those interfaces, and the doctrine team has decided to recommend using any other existing package and decommission their own solution.
+
+Also, Shlink needs support for Redis clusters and Redis sentinels, which is not supported by `doctrine/cache` Redis adapters.
+
+## Considered option
+
+After some research, the only packages that seem to support the capabilities required by Shlink and also seem healthy, are these:
+
+* [Symfony cache](https://symfony.com/doc/current/components/cache.html)
+    * 🟢 PSR-6 compliant: **yes**
+    * 🟢 PSR-16 compliant: **yes**
+    * 🟢 APCu support: **yes**
+    * 🟢 Redis support: **yes**
+    * 🟢 Redis cluster support: **yes**
+    * 🟢 Redis sentinel support: **yes**
+    * 🟢 Can use redis through Predis: **yes**
+    * 🔴 Individual packages per adapter: **no**
+* [Laminas cache](https://docs.laminas.dev/laminas-cache/)
+    * 🟢 PSR-6 compliant: **yes**
+    * 🟢 PSR-16 compliant: **yes**
+    * 🟢 APCu support: **yes**
+    * 🟢 Redis support: **yes**
+    * 🟢 Redis cluster support: **yes**
+    * 🔴 Redis sentinel support: **no**
+    * 🔴 Can use redis through Predis: **no**
+    * 🟢 Individual packages per adapter: **yes**
+
+## Decision outcome
+
+Even though Symfony packs all their adapters in a single component, which means we will install some code that will never be used, Laminas relies on the native redis extension for anything related with redis.
+
+That would make Shlink more complex to install, so it seems Symfony's package is the option where it's easier to migrate to.
+
+Also, it's important that the cache component can share the Redis integration (through `Predis`, in this case), as it's also used by other components (the lock component, to name one).
+
+## Pros and Cons of the Options
+
+### Symfony cache
+
+* Good because it supports Redis Sentinel.
+* Good because it allows using a external `Predis` instance.
+* Bad because it packs all the adapters in a single component.
+
+### Laminas cache
+
+* Good because allows installing only the adapters you are going to use, through separated packages.
+* Bad because it requires the php-redis native extension in order to interact with Redis.
+* Bad because it does ot seem to support Redis Sentinels.
--- a/docs/adr/README.md
+++ b/docs/adr/README.md
@ -2,5 +2,6 @@

 Here listed you will find the different architectural decisions taken in the project, including all the reasoning behind it, options considered, and final outcome.

+* [2021-08-05 Migrate to a new caching library](2021-08-05-migrate-to-a-new-caching-library.md)
 * [2021-02-07 Track visits to 'base_url', 'invalid_short_url' and 'regular_404'](2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md)
 * [2021-01-17 Support restrictions and permissions in API keys](2021-01-17-support-restrictions-and-permissions-in-api-keys.md)
--- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php
+++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php
@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\CLI\Command\ShortUrl;
 use Shlinkio\Shlink\CLI\Command\Util\AbstractWithDateRangeCommand;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Shlinkio\Shlink\CLI\Util\ShlinkTable;
-use Shlinkio\Shlink\Common\Util\DateRange;
 use Shlinkio\Shlink\Core\Entity\Visit;
 use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\Model\VisitsParams;
@ -21,6 +20,7 @@ use Symfony\Component\Console\Style\SymfonyStyle;

 use function Functional\map;
 use function Functional\select_keys;
+use function Shlinkio\Shlink\Common\buildDateRange;
 use function sprintf;

 class GetVisitsCommand extends AbstractWithDateRangeCommand
@ -73,7 +73,7 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand

        $paginator = $this->visitsHelper->visitsForShortUrl(
            $identifier,
-            new VisitsParams(new DateRange($startDate, $endDate)),
+            new VisitsParams(buildDateRange($startDate, $endDate)),
        );

        $rows = map($paginator->getCurrentPageResults(), function (Visit $visit) {
--- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php
+++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php
@ -45,7 +45,7 @@ class GetVisitsCommandTest extends TestCase
        $shortCode = 'abc123';
        $this->visitsHelper->visitsForShortUrl(
            new ShortUrlIdentifier($shortCode),
-            new VisitsParams(new DateRange(null, null)),
+            new VisitsParams(DateRange::emptyInstance()),
        )
            ->willReturn(new Paginator(new ArrayAdapter([])))
            ->shouldBeCalledOnce();
@ -61,7 +61,7 @@ class GetVisitsCommandTest extends TestCase
        $endDate = '2016-02-01';
        $this->visitsHelper->visitsForShortUrl(
            new ShortUrlIdentifier($shortCode),
-            new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))),
+            new VisitsParams(DateRange::withStartAndEndDate(Chronos::parse($startDate), Chronos::parse($endDate))),
        )
            ->willReturn(new Paginator(new ArrayAdapter([])))
            ->shouldBeCalledOnce();
@ -80,7 +80,7 @@ class GetVisitsCommandTest extends TestCase
        $startDate = 'foo';
        $info = $this->visitsHelper->visitsForShortUrl(
            new ShortUrlIdentifier($shortCode),
-            new VisitsParams(new DateRange()),
+            new VisitsParams(DateRange::emptyInstance()),
        )->willReturn(new Paginator(new ArrayAdapter([])));

        $this->commandTester->execute([
--- a/module/Core/functions/functions.php
+++ b/module/Core/functions/functions.php
@ -16,6 +16,7 @@ use function Functional\reduce_left;
 use function is_array;
 use function lcfirst;
 use function print_r;
+use function Shlinkio\Shlink\Common\buildDateRange;
 use function sprintf;
 use function str_repeat;
 use function str_replace;
@ -51,12 +52,7 @@ function parseDateRangeFromQuery(array $query, string $startDateName, string $en
    $startDate = parseDateFromQuery($query, $startDateName);
    $endDate = parseDateFromQuery($query, $endDateName);

-    return match (true) {
-        $startDate === null && $endDate === null => DateRange::emptyInstance(),
-        $startDate !== null && $endDate !== null => DateRange::withStartAndEndDate($startDate, $endDate),
-        $startDate !== null => DateRange::withStartDate($startDate),
-        default => DateRange::withEndDate($endDate),
-    };
+    return buildDateRange($startDate, $endDate);
 }

 /**
--- a/module/Core/src/Model/ShortUrlsParams.php
+++ b/module/Core/src/Model/ShortUrlsParams.php
@ -8,6 +8,7 @@ use Shlinkio\Shlink\Common\Util\DateRange;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\Validation\ShortUrlsParamsInputFilter;

+use function Shlinkio\Shlink\Common\buildDateRange;
 use function Shlinkio\Shlink\Core\parseDateField;

 final class ShortUrlsParams
@ -54,7 +55,7 @@ final class ShortUrlsParams
        $this->page = (int) ($inputFilter->getValue(ShortUrlsParamsInputFilter::PAGE) ?? 1);
        $this->searchTerm = $inputFilter->getValue(ShortUrlsParamsInputFilter::SEARCH_TERM);
        $this->tags = (array) $inputFilter->getValue(ShortUrlsParamsInputFilter::TAGS);
-        $this->dateRange = new DateRange(
+        $this->dateRange = buildDateRange(
            parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::START_DATE)),
            parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::END_DATE)),
        );
--- a/module/Core/src/Model/VisitsParams.php
+++ b/module/Core/src/Model/VisitsParams.php
@ -23,7 +23,7 @@ final class VisitsParams
        ?int $itemsPerPage = null,
        private bool $excludeBots = false
    ) {
-        $this->dateRange = $dateRange ?? new DateRange();
+        $this->dateRange = $dateRange ?? DateRange::emptyInstance();
        $this->page = $this->determinePage($page);
        $this->itemsPerPage = $this->determineItemsPerPage($itemsPerPage);
    }
--- a/module/Core/src/Repository/ShortUrlRepository.php
+++ b/module/Core/src/Repository/ShortUrlRepository.php
@ -105,13 +105,13 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU
        $qb->from(ShortUrl::class, 's')
           ->where('1=1');

-        if ($dateRange?->getStartDate() !== null) {
+        if ($dateRange?->startDate() !== null) {
            $qb->andWhere($qb->expr()->gte('s.dateCreated', ':startDate'));
-            $qb->setParameter('startDate', $dateRange->getStartDate(), ChronosDateTimeType::CHRONOS_DATETIME);
+            $qb->setParameter('startDate', $dateRange->startDate(), ChronosDateTimeType::CHRONOS_DATETIME);
        }
-        if ($dateRange?->getEndDate() !== null) {
+        if ($dateRange?->endDate() !== null) {
            $qb->andWhere($qb->expr()->lte('s.dateCreated', ':endDate'));
-            $qb->setParameter('endDate', $dateRange->getEndDate(), ChronosDateTimeType::CHRONOS_DATETIME);
+            $qb->setParameter('endDate', $dateRange->endDate(), ChronosDateTimeType::CHRONOS_DATETIME);
        }

        // Apply search term to every searchable field if not empty
--- a/module/Core/src/Repository/VisitRepository.php
+++ b/module/Core/src/Repository/VisitRepository.php
@ -187,11 +187,11 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo

    private function applyDatesInline(QueryBuilder $qb, ?DateRange $dateRange): void
    {
-        if ($dateRange?->getStartDate() !== null) {
-            $qb->andWhere($qb->expr()->gte('v.date', '\'' . $dateRange->getStartDate()->toDateTimeString() . '\''));
+        if ($dateRange?->startDate() !== null) {
+            $qb->andWhere($qb->expr()->gte('v.date', '\'' . $dateRange->startDate()->toDateTimeString() . '\''));
        }
-        if ($dateRange?->getEndDate() !== null) {
-            $qb->andWhere($qb->expr()->lte('v.date', '\'' . $dateRange->getEndDate()->toDateTimeString() . '\''));
+        if ($dateRange?->endDate() !== null) {
+            $qb->andWhere($qb->expr()->lte('v.date', '\'' . $dateRange->endDate()->toDateTimeString() . '\''));
        }
    }

--- a/module/Core/src/Spec/InDateRange.php
+++ b/module/Core/src/Spec/InDateRange.php
@ -20,12 +20,12 @@ class InDateRange extends BaseSpecification
    {
        $criteria = [];

-        if ($this->dateRange?->getStartDate() !== null) {
-            $criteria[] = Spec::gte($this->field, $this->dateRange->getStartDate()->toDateTimeString());
+        if ($this->dateRange?->startDate() !== null) {
+            $criteria[] = Spec::gte($this->field, $this->dateRange->startDate()->toDateTimeString());
        }

-        if ($this->dateRange?->getEndDate() !== null) {
-            $criteria[] = Spec::lte($this->field, $this->dateRange->getEndDate()->toDateTimeString());
+        if ($this->dateRange?->endDate() !== null) {
+            $criteria[] = Spec::lte($this->field, $this->dateRange->endDate()->toDateTimeString());
        }

        return Spec::andX(...$criteria);
--- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php
+++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php
@ -133,16 +133,16 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
        self::assertCount(3, $result);
        self::assertSame($bar, $result[0]);

-        $result = $this->repo->findList(null, null, null, [], null, new DateRange(null, Chronos::now()->subDays(2)));
+        $result = $this->repo->findList(null, null, null, [], null, DateRange::withEndDate(Chronos::now()->subDays(2)));
        self::assertCount(1, $result);
-        self::assertEquals(1, $this->repo->countList(null, [], new DateRange(null, Chronos::now()->subDays(2))));
+        self::assertEquals(1, $this->repo->countList(null, [], DateRange::withEndDate(Chronos::now()->subDays(2))));
        self::assertSame($foo2, $result[0]);

        self::assertCount(
            2,
-            $this->repo->findList(null, null, null, [], null, new DateRange(Chronos::now()->subDays(2))),
+            $this->repo->findList(null, null, null, [], null, DateRange::withStartDate(Chronos::now()->subDays(2))),
        );
-        self::assertEquals(2, $this->repo->countList(null, [], new DateRange(Chronos::now()->subDays(2))));
+        self::assertEquals(2, $this->repo->countList(null, [], DateRange::withStartDate(Chronos::now()->subDays(2))));
    }

    /** @test */
--- a/module/Core/test/Paginator/Adapter/VisitsForTagPaginatorAdapterTest.php
+++ b/module/Core/test/Paginator/Adapter/VisitsForTagPaginatorAdapterTest.php
@ -35,7 +35,7 @@ class VisitsForTagPaginatorAdapterTest extends TestCase
        $adapter = $this->createAdapter(null);
        $findVisits = $this->repo->findVisitsByTag(
            'foo',
-            new VisitsListFiltering(new DateRange(), false, null, $limit, $offset),
+            new VisitsListFiltering(DateRange::emptyInstance(), false, null, $limit, $offset),
        )->willReturn([]);

        for ($i = 0; $i < $count; $i++) {
--- a/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php
+++ b/module/Core/test/Paginator/Adapter/VisitsPaginatorAdapterTest.php
@ -36,7 +36,7 @@ class VisitsPaginatorAdapterTest extends TestCase
        $adapter = $this->createAdapter(null);
        $findVisits = $this->repo->findVisitsByShortCode(
            ShortUrlIdentifier::fromShortCodeAndDomain(''),
-            new VisitsListFiltering(new DateRange(), false, null, $limit, $offset),
+            new VisitsListFiltering(DateRange::emptyInstance(), false, null, $limit, $offset),
        )->willReturn([]);

        for ($i = 0; $i < $count; $i++) {
@ -54,7 +54,7 @@ class VisitsPaginatorAdapterTest extends TestCase
        $adapter = $this->createAdapter($apiKey);
        $countVisits = $this->repo->countVisitsByShortCode(
            ShortUrlIdentifier::fromShortCodeAndDomain(''),
-            new VisitsCountFiltering(new DateRange(), false, $apiKey->spec()),
+            new VisitsCountFiltering(DateRange::emptyInstance(), false, $apiKey->spec()),
        )->willReturn(3);

        for ($i = 0; $i < $count; $i++) {
--- a/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php
+++ b/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php
@ -53,7 +53,7 @@ class ShortUrlVisitsActionTest extends TestCase
    {
        $shortCode = 'abc123';
        $this->visitsHelper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams(
-            new DateRange(null, Chronos::parse('2016-01-01 00:00:00')),
+            DateRange::withEndDate(Chronos::parse('2016-01-01 00:00:00')),
            3,
            10,
        ), Argument::type(ApiKey::class))