Added db test for filtering of disabled short URLs

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;
     }

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)'),

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
     {