diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 7316c50b..28647bdc 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -65,29 +65,29 @@ class ShortUrl extends AbstractEntity return self::fromMeta(ShortUrlCreation::fromRawData([ShortUrlInputFilter::LONG_URL => $longUrl])); } - public static function fromMeta( - ShortUrlCreation $meta, + public static function fromMeta( // TODO Rename to create(...) + ShortUrlCreation $creation, ?ShortUrlRelationResolverInterface $relationResolver = null, ): self { $instance = new self(); $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); - $instance->longUrl = $meta->getLongUrl(); + $instance->longUrl = $creation->getLongUrl(); $instance->dateCreated = Chronos::now(); $instance->visits = new ArrayCollection(); - $instance->tags = $relationResolver->resolveTags($meta->getTags()); - $instance->validSince = $meta->getValidSince(); - $instance->validUntil = $meta->getValidUntil(); - $instance->maxVisits = $meta->getMaxVisits(); - $instance->customSlugWasProvided = $meta->hasCustomSlug(); - $instance->shortCodeLength = $meta->getShortCodeLength(); - $instance->shortCode = $meta->getCustomSlug() ?? generateRandomShortCode($instance->shortCodeLength); - $instance->domain = $relationResolver->resolveDomain($meta->getDomain()); - $instance->authorApiKey = $meta->getApiKey(); - $instance->title = $meta->getTitle(); - $instance->titleWasAutoResolved = $meta->titleWasAutoResolved(); - $instance->crawlable = $meta->isCrawlable(); - $instance->forwardQuery = $meta->forwardQuery(); + $instance->tags = $relationResolver->resolveTags($creation->getTags()); + $instance->validSince = $creation->getValidSince(); + $instance->validUntil = $creation->getValidUntil(); + $instance->maxVisits = $creation->getMaxVisits(); + $instance->customSlugWasProvided = $creation->hasCustomSlug(); + $instance->shortCodeLength = $creation->getShortCodeLength(); + $instance->shortCode = $creation->getCustomSlug() ?? generateRandomShortCode($instance->shortCodeLength); + $instance->domain = $relationResolver->resolveDomain($creation->getDomain()); + $instance->authorApiKey = $creation->getApiKey(); + $instance->title = $creation->getTitle(); + $instance->titleWasAutoResolved = $creation->titleWasAutoResolved(); + $instance->crawlable = $creation->isCrawlable(); + $instance->forwardQuery = $creation->forwardQuery(); return $instance; } diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php index 2e18bc8c..37121bdb 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php @@ -36,6 +36,11 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU ->setMaxResults($filtering->limit) ->setFirstResult($filtering->offset); + // Some DB engines (postgres and ms) require aggregates in the select for ordering and filtering + if ($filtering->excludeMaxVisitsReached || $filtering->orderBy->field === 'visits') { + $qb->addSelect('COUNT(DISTINCT v)'); + } + // In case the ordering has been specified, the query could be more complex. Process it if ($filtering->orderBy->hasOrderField()) { $this->processOrderByForList($qb, $filtering); @@ -59,10 +64,10 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU // Diagnostic: It might need to use a sub-query, as done with the tags list query. if (! $filtering->excludeMaxVisitsReached) { // Left join only if this was not true, otherwise this left join already happened - $this->leftJoinShortUrlsWithVisitsCount($qb); + $this->leftJoinShortUrlsWithVisits($qb); } - $qb->orderBy('totalVisits', $order); + $qb->orderBy('COUNT(DISTINCT v)', $order); } elseif (contains(['longUrl', 'shortCode', 'dateCreated', 'title'], $fieldName)) { $qb->orderBy('s.' . $fieldName, $order); } else { @@ -77,7 +82,10 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU $qb->select('COUNT(DISTINCT s)'); $query = $qb->getQuery(); -// dump($query->getSQL()); + // TODO Check if this is needed +// if ($filtering->excludeMaxVisitsReached) { +// $qb->addSelect('COUNT(DISTINCT v)'); +// } // TODO This is crap... return $filtering->excludeMaxVisitsReached @@ -85,10 +93,9 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU : (int) $query->getSingleScalarResult(); } - private function leftJoinShortUrlsWithVisitsCount(QueryBuilder $qb): void + private function leftJoinShortUrlsWithVisits(QueryBuilder $qb): void { - $qb->addSelect('COUNT(DISTINCT v) AS totalVisits') - ->leftJoin('s.visits', 'v') + $qb->leftJoin('s.visits', 'v') ->groupBy('s'); } @@ -150,7 +157,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU } if ($filtering->excludeMaxVisitsReached) { - $this->leftJoinShortUrlsWithVisitsCount($qb); + $this->leftJoinShortUrlsWithVisits($qb); $qb->having($qb->expr()->orX( $qb->expr()->isNull('s.maxVisits'), $qb->expr()->gt('s.maxVisits', 'COUNT(DISTINCT v)'), diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index e41eee15..186a4ecf 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -339,6 +339,50 @@ class ShortUrlRepositoryTest extends DatabaseTestCase self::assertCount(0, $this->repo->findList($buildFiltering('no results'))); } + /** @test */ + public function findListReturnsOnlyThoseWithoutExcludedUrls(): void + { + $shortUrl1 = ShortUrl::fromMeta(ShortUrlCreation::fromRawData([ + 'longUrl' => 'foo1', + 'validUntil' => Chronos::now()->addDays(1)->toAtomString(), + 'maxVisits' => 100, + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl1); + $shortUrl2 = ShortUrl::fromMeta(ShortUrlCreation::fromRawData([ + 'longUrl' => 'foo2', + 'validUntil' => Chronos::now()->subDays(1)->toAtomString(), + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl2); + $shortUrl3 = ShortUrl::fromMeta(ShortUrlCreation::fromRawData([ + 'longUrl' => 'foo3', + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl3); + $shortUrl4 = ShortUrl::fromMeta(ShortUrlCreation::fromRawData([ + 'longUrl' => 'foo4', + 'maxVisits' => 3, + ]), $this->relationResolver); + $this->getEntityManager()->persist($shortUrl4); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl4, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl4, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl4, Visitor::emptyInstance())); + + $this->getEntityManager()->flush(); + + $filtering = static fn (bool $excludeMaxVisitsReached, bool $excludePastValidUntil) => + new ShortUrlsListFiltering( + null, + null, + Ordering::emptyInstance(), + excludeMaxVisitsReached: $excludeMaxVisitsReached, + excludePastValidUntil: $excludePastValidUntil, + ); + + self::assertCount(4, $this->repo->findList($filtering(false, false))); + self::assertCount(3, $this->repo->findList($filtering(true, false))); + self::assertCount(3, $this->repo->findList($filtering(false, true))); + self::assertCount(2, $this->repo->findList($filtering(true, true))); + } + /** @test */ public function shortCodeIsInUseLooksForShortUrlInProperSetOfTables(): void {