diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 9c24bdc3..3fe2932b 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -166,12 +166,18 @@ class ShortUrl extends AbstractEntity return count($this->visits); } - public function importedVisitsCount(): int + public function mostRecentImportedVisitDate(): ?Chronos { /** @var Selectable $visits */ $visits = $this->visits; - $criteria = Criteria::create()->where(Criteria::expr()->eq('type', Visit::TYPE_IMPORTED)); - return count($visits->matching($criteria)); + $criteria = Criteria::create()->where(Criteria::expr()->eq('type', Visit::TYPE_IMPORTED)) + ->orderBy(['id' => 'DESC']) + ->setMaxResults(1); + + /** @var Visit|false $visit */ + $visit = $visits->matching($criteria)->last(); + + return $visit === false ? null : $visit->getDate(); } /** @@ -189,7 +195,7 @@ class ShortUrl extends AbstractEntity return $this->maxVisits; } - public function getTitle(): ?string + public function title(): ?string { return $this->title; } diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 802e33b4..4091906d 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Importer; +use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; @@ -23,6 +24,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private ShortUrlRelationResolverInterface $relationResolver; private ShortCodeHelperInterface $shortCodeHelper; private DoctrineBatchHelperInterface $batchHelper; + private ShortUrlRepositoryInterface $shortUrlRepo; public function __construct( EntityManagerInterface $em, @@ -34,6 +36,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface $this->relationResolver = $relationResolver; $this->shortCodeHelper = $shortCodeHelper; $this->batchHelper = $batchHelper; + $this->shortUrlRepo = $this->em->getRepository(ShortUrl::class); // @phpstan-ignore-line } /** @@ -41,8 +44,6 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface */ public function process(StyleInterface $io, iterable $shlinkUrls, array $params): void { - /** @var ShortUrlRepositoryInterface $shortUrlRepo */ - $shortUrlRepo = $this->em->getRepository(ShortUrl::class); $importShortCodes = $params['import_short_codes']; $iterable = $this->batchHelper->wrapIterable($shlinkUrls, 100); @@ -50,54 +51,74 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface foreach ($iterable as $importedUrl) { $longUrl = $importedUrl->longUrl(); - // Skip already imported URLs - if ($shortUrlRepo->findOneByImportedUrl($importedUrl) !== null) { - // TODO If the URL exists, allow to merge visits instead of just skipping completely - $io->text(sprintf('%s: Skipped', $longUrl)); + $generateNewIfDuplicated = static function () use ($io, $importedUrl): bool { + $action = $io->choice(sprintf( + 'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate ' + . 'a new one or skip it?', + $importedUrl->longUrl(), + $importedUrl->shortCode(), + ), ['Generate new short-code', 'Skip'], 1); + + return $action !== 'Skip'; + }; + [$shortUrl, $isNew] = $this->getOrCreateShortUrl($importedUrl, $importShortCodes, $generateNewIfDuplicated); + + if ($shortUrl === null) { + $io->text(sprintf('%s: Error', $longUrl)); continue; } - $shortUrl = ShortUrl::fromImport($importedUrl, $importShortCodes, $this->relationResolver); - if (! $this->handleShortCodeUniqueness($importedUrl, $shortUrl, $io, $importShortCodes)) { - $io->text(sprintf('%s: Skipped', $longUrl)); - continue; - } - - $this->em->persist($shortUrl); $importedVisits = $this->importVisits($importedUrl, $shortUrl); - $io->text( - $importedVisits === 0 - ? sprintf('%s: Imported', $longUrl) - : sprintf('%s: Imported with %s visits', $longUrl, $importedVisits), - ); + if ($importedVisits === 0) { + $io->text( + $isNew + ? sprintf('%s: Imported', $longUrl) + : sprintf('%s: Skipped', $longUrl), + ); + } else { + $io->text( + $isNew + ? sprintf('%s: Imported with %s visits', $longUrl, $importedVisits) + : sprintf( + '%s: Skipped. Imported %s visits', + $longUrl, + $importedVisits, + ), + ); + } } } -// private function getOrCreateShortUrl(ImportedShlinkUrl $url, bool $importShortCodes): ?ShortUrl -// { -// -// } + private function getOrCreateShortUrl( + ImportedShlinkUrl $importedUrl, + bool $importShortCodes, + callable $generateNewIfDuplicated + ): array { + $existingShortUrl = $this->shortUrlRepo->findOneByImportedUrl($importedUrl); + if ($existingShortUrl !== null) { + return [$existingShortUrl, false]; + } + + $shortUrl = ShortUrl::fromImport($importedUrl, $importShortCodes, $this->relationResolver); + if (! $this->handleShortCodeUniqueness($shortUrl, $importShortCodes, $generateNewIfDuplicated)) { + return [null, false]; + } + + $this->em->persist($shortUrl); + return [$shortUrl, true]; + } private function handleShortCodeUniqueness( - ImportedShlinkUrl $url, ShortUrl $shortUrl, - StyleInterface $io, - bool $importShortCodes + bool $importShortCodes, + callable $generateNewIfDuplicated ): bool { if ($this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { return true; } - $longUrl = $url->longUrl(); - $action = $io->choice(sprintf( - 'Failed to import URL "%s" because its short-code "%s" is already in use. Do you want to generate a new ' - . 'one or skip it?', - $longUrl, - $url->shortCode(), - ), ['Generate new short-code', 'Skip'], 1); - - if ($action === 'Skip') { + if (! $generateNewIfDuplicated()) { return false; } @@ -106,14 +127,16 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private function importVisits(ImportedShlinkUrl $importedUrl, ShortUrl $shortUrl): int { - // If we know the amount of visits that can be imported, import only those left. Import all otherwise. - $importVisitsCount = $importedUrl->visitsCount(); - $visitsLeft = $importVisitsCount !== null ? $importVisitsCount - $shortUrl->importedVisitsCount() : null; + $mostRecentImportedDate = $shortUrl->mostRecentImportedVisitDate(); $importedVisits = 0; foreach ($importedUrl->visits() as $importedVisit) { - if ($visitsLeft !== null && $importedVisits >= $visitsLeft) { - break; + // Skip visits which are older than the most recent already imported visit's date + if ( + $mostRecentImportedDate !== null + && $mostRecentImportedDate->gte(Chronos::instance($importedVisit->date())) + ) { + continue; } $this->em->persist(Visit::fromImport($shortUrl, $importedVisit)); diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index cd51f60d..d9c18977 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -203,6 +203,9 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo private function resolveVisitsWithNativeQuery(QueryBuilder $qb, ?int $limit, ?int $offset): array { + // TODO Order by date and ID, not just by ID (order by date DESC, id DESC). + // That ensures imported visits are properly ordered even if inserted in wrong chronological order. + $qb->select('v.id') ->orderBy('v.id', 'DESC') // Falling back to values that will behave as no limit/offset, but will workaround MS SQL not allowing diff --git a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php index ce459714..49918867 100644 --- a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php @@ -34,7 +34,7 @@ class ShortUrlDataTransformer implements DataTransformerInterface 'tags' => invoke($shortUrl->getTags(), '__toString'), 'meta' => $this->buildMeta($shortUrl), 'domain' => $shortUrl->getDomain(), - 'title' => $shortUrl->getTitle(), + 'title' => $shortUrl->title(), ]; }