From 527faf27a88f78cbfec71852183f95229a0172d8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Jun 2020 20:39:51 +0200 Subject: [PATCH 1/4] Changed how visits for a tag are fetched, avoiding thousands of values to be loaded in memory --- .../Core/src/Repository/VisitRepository.php | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index b3761c9f..5d242241 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -142,26 +142,18 @@ class VisitRepository extends EntityRepository implements VisitRepositoryInterfa private function createVisitsByTagQueryBuilder(string $tag, ?DateRange $dateRange = null): QueryBuilder { - $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('s.id') - ->from(ShortUrl::class, 's') - ->join('s.tags', 't') - ->where($qb->expr()->eq('t.name', ':tag')) - ->setParameter('tag', $tag); - - $shortUrlIds = array_column($qb->getQuery()->getArrayResult(), 'id'); - $shortUrlIds[] = '-1'; // Add an invalid ID, in case the list is empty - // Parameters in this query need to be part of the query itself, as we need to use it a sub-query later // Since they are not strictly provided by the caller, it's reasonably safe - $qb2 = $this->getEntityManager()->createQueryBuilder(); - $qb2->from(Visit::class, 'v') - ->where($qb2->expr()->in('v.shortUrl', $shortUrlIds)); + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->from(Visit::class, 'v') + ->join('v.shortUrl', 's') + ->join('s.tags', 't') + ->where($qb->expr()->eq('t.name', '\'' . $tag . '\'')); // Apply date range filtering - $this->applyDatesInline($qb2, $dateRange); + $this->applyDatesInline($qb, $dateRange); - return $qb2; + return $qb; } private function applyDatesInline(QueryBuilder $qb, ?DateRange $dateRange): void From 296134078cc8f9c4c550b406548773b84083703f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Jun 2020 20:40:47 +0200 Subject: [PATCH 2/4] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f61d06c9..dcf0b004 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed * [#769](https://github.com/shlinkio/shlink/issues/769) Fixed custom slugs not allowing valid URL characters, like `.`, `_` or `~`. +* [#781](https://github.com/shlinkio/shlink/issues/781) Fixed memory leak when loading visits for a tag which is used for big amounts of short URLs. ## 2.2.1 - 2020-05-11 From f3f3ef5c18dc95d298c2637654095afa412bbbb5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Jun 2020 20:42:44 +0200 Subject: [PATCH 3/4] Removed unused import --- module/Core/src/Repository/VisitRepository.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 5d242241..458b8ef2 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -12,8 +12,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use function array_column; - use const PHP_INT_MAX; class VisitRepository extends EntityRepository implements VisitRepositoryInterface From a4eda9d7616d26b9592403984992bc58a4ab3d1a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Jun 2020 22:38:51 +0200 Subject: [PATCH 4/4] Moved execution of API tests outside composer script --- .travis.yml | 2 +- composer.json | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8365f582..9b1c35e6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -46,7 +46,7 @@ before_script: - export DOCKERFILE_CHANGED=$(git diff ${TRAVIS_COMMIT_RANGE:-origin/master} --name-only | grep Dockerfile) script: - - if [[ "${DOCKER_PUBLISH}" == 'false' ]]; then composer ci ; fi + - if [[ "${DOCKER_PUBLISH}" == 'false' ]]; then bin/test/run-api-tests.sh --coverage-php build/coverage-api.cov && composer ci ; fi - if [[ ! -z "${DOCKERFILE_CHANGED}" && "${TRAVIS_PHP_VERSION}" == "7.4" && "${DOCKER_PUBLISH}" == "false" ]]; then docker build -t shlink-docker-image:temp . ; fi - if [[ "${DOCKER_PUBLISH}" == 'true' ]]; then bash ./docker/build ; fi diff --git a/composer.json b/composer.json index 7ba81442..ef9e0e92 100644 --- a/composer.json +++ b/composer.json @@ -58,7 +58,7 @@ "symfony/filesystem": "^5.1", "symfony/lock": "^5.1", "symfony/mercure": "^0.3.0", - "symfony/process": "~5.0.9", + "symfony/process": "^5.1", "symfony/string": "^5.1", "symfony/translation-contracts": "^2.1" }, @@ -112,8 +112,7 @@ ], "test:ci": [ "@test:unit:ci", - "@test:db", - "@test:api:ci" + "@test:db" ], "test:unit": "phpdbg -qrr vendor/bin/phpunit --order-by=random --colors=always --coverage-php build/coverage-unit.cov --testdox", "test:unit:ci": "@test:unit --coverage-clover=build/clover.xml --coverage-xml=build/coverage-xml --log-junit=build/junit.xml",