From 33d3837795a08f31a5882efc4e9decda228d4e21 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 22 Oct 2020 18:12:22 +0200 Subject: [PATCH 01/12] Added dependency on shlinkio/shlink-importer --- CHANGELOG.md | 1 + composer.json | 1 + config/autoload/dependencies.global.php | 6 ++ config/config.php | 1 + module/Core/config/dependencies.config.php | 9 +++ module/Core/src/Entity/ShortUrl.php | 24 ++++++++ .../src/Importer/ImportedLinksProcessor.php | 55 +++++++++++++++++++ 7 files changed, 97 insertions(+) create mode 100644 module/Core/src/Importer/ImportedLinksProcessor.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ff32f795..7c044b9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#837](https://github.com/shlinkio/shlink/issues/837) Drastically improved performance when creating a new shortUrl and providing `findIfExists = true`. + ## 2.3.0 - 2020-08-09 #### Added diff --git a/composer.json b/composer.json index 99844320..597cc4b1 100644 --- a/composer.json +++ b/composer.json @@ -53,6 +53,7 @@ "shlinkio/shlink-common": "^3.2.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", + "shlinkio/shlink-importer": "^1.0", "shlinkio/shlink-installer": "^5.1.0", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", diff --git a/config/autoload/dependencies.global.php b/config/autoload/dependencies.global.php index 023b3c4e..dbc553f1 100644 --- a/config/autoload/dependencies.global.php +++ b/config/autoload/dependencies.global.php @@ -2,7 +2,9 @@ declare(strict_types=1); +use GuzzleHttp\Client; use Mezzio\Container; +use Psr\Http\Client\ClientInterface; return [ @@ -13,6 +15,10 @@ return [ ], ], + 'aliases' => [ + ClientInterface::class => Client::class, + ], + 'lazy_services' => [ 'proxies_target_dir' => 'data/proxies', 'proxies_namespace' => 'ShlinkProxy', diff --git a/config/config.php b/config/config.php index d7fd6a3b..ba0657fc 100644 --- a/config/config.php +++ b/config/config.php @@ -21,6 +21,7 @@ return (new ConfigAggregator\ConfigAggregator([ Diactoros\ConfigProvider::class, Common\ConfigProvider::class, Config\ConfigProvider::class, + Importer\ConfigProvider::class, IpGeolocation\ConfigProvider::class, EventDispatcher\ConfigProvider::class, Core\ConfigProvider::class, diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 5dcef9a2..09c4d96e 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -10,6 +10,7 @@ use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Domain\Resolver; use Shlinkio\Shlink\Core\ErrorHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; +use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; return [ @@ -42,6 +43,12 @@ return [ Resolver\PersistenceDomainResolver::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, + + Importer\ImportedLinksProcessor::class => ConfigAbstractFactory::class, + ], + + 'aliases' => [ + ImportedLinksProcessorInterface::class => Importer\ImportedLinksProcessor::class, ], ], @@ -96,6 +103,8 @@ return [ Resolver\PersistenceDomainResolver::class => ['em'], Mercure\MercureUpdatesGenerator::class => ['config.url_shortener.domain'], + + Importer\ImportedLinksProcessor::class => ['em', Resolver\PersistenceDomainResolver::class], ], ]; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ba10a44a..6da6562a 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -14,6 +14,8 @@ use Shlinkio\Shlink\Core\Domain\Resolver\SimpleDomainResolver; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; +use Shlinkio\Shlink\Importer\Model\ShlinkUrl; use function count; use function Shlinkio\Shlink\Core\generateRandomShortCode; @@ -33,6 +35,7 @@ class ShortUrl extends AbstractEntity private ?Domain $domain = null; private bool $customSlugWasProvided; private int $shortCodeLength; + private ?string $source = null; public function __construct( string $longUrl, @@ -54,6 +57,27 @@ class ShortUrl extends AbstractEntity $this->domain = ($domainResolver ?? new SimpleDomainResolver())->resolveDomain($meta->getDomain()); } + public static function fromImport( + ShlinkUrl $url, + string $source, + bool $importShortCode, + ?DomainResolverInterface $domainResolver = null + ): self { + $meta = [ + ShortUrlMetaInputFilter::DOMAIN => $url->domain(), + ShortUrlMetaInputFilter::VALIDATE_URL => false, + ]; + if ($importShortCode) { + $meta[ShortUrlMetaInputFilter::CUSTOM_SLUG] = $url->shortCode(); + } + + $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $domainResolver); + $instance->source = $source; + $instance->dateCreated = Chronos::instance($url->createdAt()); + + return $instance; + } + public function getLongUrl(): string { return $this->longUrl; diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php new file mode 100644 index 00000000..cb3ca57b --- /dev/null +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -0,0 +1,55 @@ +em = $em; + $this->domainResolver = $domainResolver; + } + + /** + * @param ShlinkUrl[] $shlinkUrls + */ + public function process(iterable $shlinkUrls, string $source, array $params): void + { + $importShortCodes = $params['import_short_codes']; + $count = 0; + $persistBlock = 100; + + foreach ($shlinkUrls as $url) { + $count++; + + $shortUrl = ShortUrl::fromImport($url, $source, $importShortCodes, $this->domainResolver); + $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); + + // TODO Handle errors while creating short URLs, to avoid making the whole process fail + $this->em->persist($shortUrl); + + // Flush and clear after X iterations + if ($count % $persistBlock === 0) { + $this->em->flush(); + $this->em->clear(); + } + } + + $this->em->flush(); + $this->em->clear(); + } +} From 554d9b092fd80b912f34c4f846f24a4588817c53 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 23 Oct 2020 12:59:39 +0200 Subject: [PATCH 02/12] Added import_source column in ShortUrls --- data/migrations/Version20201023090929.php | 33 +++++++++++++++++++ .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++ module/Core/src/Entity/ShortUrl.php | 4 +-- 3 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 data/migrations/Version20201023090929.php diff --git a/data/migrations/Version20201023090929.php b/data/migrations/Version20201023090929.php new file mode 100644 index 00000000..49a401ba --- /dev/null +++ b/data/migrations/Version20201023090929.php @@ -0,0 +1,33 @@ +getTable('short_urls'); + $this->skipIf($shortUrls->hasColumn(self::IMPORT_SOURCE_COLUMN)); + + $shortUrls->addColumn(self::IMPORT_SOURCE_COLUMN, Types::STRING, [ + 'length' => 255, + 'notnull' => false, + ]); + } + + public function down(Schema $schema): void + { + $shortUrls = $schema->getTable('short_urls'); + $this->skipIf(! $shortUrls->hasColumn(self::IMPORT_SOURCE_COLUMN)); + + $shortUrls->dropColumn(self::IMPORT_SOURCE_COLUMN); + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index 871ac113..55fa230e 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -51,6 +51,11 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->nullable() ->build(); + $builder->createField('importSource', Types::STRING) + ->columnName('import_source') + ->nullable() + ->build(); + $builder->createOneToMany('visits', Entity\Visit::class) ->mappedBy('shortUrl') ->fetchExtraLazy() diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 6da6562a..34883127 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -35,7 +35,7 @@ class ShortUrl extends AbstractEntity private ?Domain $domain = null; private bool $customSlugWasProvided; private int $shortCodeLength; - private ?string $source = null; + private ?string $importSource = null; public function __construct( string $longUrl, @@ -72,7 +72,7 @@ class ShortUrl extends AbstractEntity } $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $domainResolver); - $instance->source = $source; + $instance->importSource = $source; $instance->dateCreated = Chronos::instance($url->createdAt()); return $instance; From ec3e7212b2a078a97a603c5666f59084a392aff9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 24 Oct 2020 13:55:54 +0200 Subject: [PATCH 03/12] =?UTF-8?q?Basic=20short-=C3=BArl=20import=20impleme?= =?UTF-8?q?ntation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- composer.json | 2 +- module/Core/src/Entity/ShortUrl.php | 4 +- .../src/Importer/ImportedLinksProcessor.php | 29 ++++---- .../src/Repository/ShortUrlRepository.php | 13 ++++ .../ShortUrlRepositoryInterface.php | 3 + .../Core/src/Util/DoctrineBatchIterator.php | 67 +++++++++++++++++++ 6 files changed, 100 insertions(+), 18 deletions(-) create mode 100644 module/Core/src/Util/DoctrineBatchIterator.php diff --git a/composer.json b/composer.json index 597cc4b1..922057d8 100644 --- a/composer.json +++ b/composer.json @@ -53,7 +53,7 @@ "shlinkio/shlink-common": "^3.2.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", - "shlinkio/shlink-importer": "^1.0", + "shlinkio/shlink-importer": "^1.0.1", "shlinkio/shlink-installer": "^5.1.0", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 34883127..ec752ba9 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -15,7 +15,7 @@ use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlEdit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; -use Shlinkio\Shlink\Importer\Model\ShlinkUrl; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use function count; use function Shlinkio\Shlink\Core\generateRandomShortCode; @@ -58,7 +58,7 @@ class ShortUrl extends AbstractEntity } public static function fromImport( - ShlinkUrl $url, + ImportedShlinkUrl $url, string $source, bool $importShortCode, ?DomainResolverInterface $domainResolver = null diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index cb3ca57b..cb10ad15 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -7,9 +7,11 @@ namespace Shlinkio\Shlink\Core\Importer; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; -use Shlinkio\Shlink\Importer\Model\ShlinkUrl; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; class ImportedLinksProcessor implements ImportedLinksProcessorInterface { @@ -25,31 +27,28 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface } /** - * @param ShlinkUrl[] $shlinkUrls + * @param iterable|ImportedShlinkUrl[] $shlinkUrls */ public function process(iterable $shlinkUrls, string $source, array $params): void { + /** @var ShortUrlRepositoryInterface $shortUrlRepo */ + $shortUrlRepo = $this->em->getRepository(ShortUrl::class); $importShortCodes = $params['import_short_codes']; - $count = 0; - $persistBlock = 100; + $iterable = new DoctrineBatchIterator($shlinkUrls, $this->em, 100); - foreach ($shlinkUrls as $url) { - $count++; + /** @var ImportedShlinkUrl $url */ + foreach ($iterable as $url) { + // Skip already imported URLs + if ($shortUrlRepo->importedUrlExists($url, $source, $importShortCodes)) { + continue; + } $shortUrl = ShortUrl::fromImport($url, $source, $importShortCodes, $this->domainResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); // TODO Handle errors while creating short URLs, to avoid making the whole process fail + // * Duplicated short code $this->em->persist($shortUrl); - - // Flush and clear after X iterations - if ($count % $persistBlock === 0) { - $this->em->flush(); - $this->em->clear(); - } } - - $this->em->flush(); - $this->em->clear(); } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index e2cf578f..5cf7d997 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use function array_column; use function array_key_exists; @@ -254,4 +255,16 @@ DQL; return $qb->getQuery()->getOneOrNullResult(); } + + public function importedUrlExists(ImportedShlinkUrl $url, string $source, bool $importShortCodes): bool + { + $findConditions = ['importSource' => $source]; + if ($importShortCodes) { + $findConditions['shortCode'] = $url->shortCode(); + } else { + $findConditions['longUrl'] = $url->longUrl(); + } + + return $this->count($findConditions) > 0; + } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 65278a85..fac50980 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -9,6 +9,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; interface ShortUrlRepositoryInterface extends ObjectRepository { @@ -30,4 +31,6 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function shortCodeIsInUse(string $slug, ?string $domain): bool; public function findOneMatching(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl; + + public function importedUrlExists(ImportedShlinkUrl $url, string $source, bool $importShortCodes): bool; } diff --git a/module/Core/src/Util/DoctrineBatchIterator.php b/module/Core/src/Util/DoctrineBatchIterator.php new file mode 100644 index 00000000..05311b09 --- /dev/null +++ b/module/Core/src/Util/DoctrineBatchIterator.php @@ -0,0 +1,67 @@ +resultSet = $resultSet; + $this->em = $em; + $this->batchSize = $batchSize; + } + + /** + * @throws Throwable + */ + public function getIterator(): iterable + { + $iteration = 0; + $resultSet = $this->resultSet; + + $this->em->beginTransaction(); + + try { + foreach ($resultSet as $key => $value) { + $iteration++; + yield $key => $value; + $this->flushAndClearBatch($iteration); + } + } catch (Throwable $e) { + $this->em->rollback(); + + throw $e; + } + + $this->flushAndClearEntityManager(); + $this->em->commit(); + } + + private function flushAndClearBatch(int $iteration): void + { + if ($iteration % $this->batchSize) { + return; + } + + $this->flushAndClearEntityManager(); + } + + private function flushAndClearEntityManager(): void + { + $this->em->flush(); + $this->em->clear(); + } +} From 2256f6a9e77a6af02188d544ec9aeae501deb0d9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 24 Oct 2020 15:08:34 +0200 Subject: [PATCH 04/12] Added feedback to ImportedLinksProcessor --- composer.json | 2 +- module/Core/src/Entity/ShortUrl.php | 3 +-- module/Core/src/Importer/ImportedLinksProcessor.php | 12 +++++++++--- module/Core/src/Repository/ShortUrlRepository.php | 4 ++-- .../src/Repository/ShortUrlRepositoryInterface.php | 2 +- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index 922057d8..c56d584a 100644 --- a/composer.json +++ b/composer.json @@ -53,7 +53,7 @@ "shlinkio/shlink-common": "^3.2.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", - "shlinkio/shlink-importer": "^1.0.1", + "shlinkio/shlink-importer": "^2.0", "shlinkio/shlink-installer": "^5.1.0", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ec752ba9..aba0235f 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -59,7 +59,6 @@ class ShortUrl extends AbstractEntity public static function fromImport( ImportedShlinkUrl $url, - string $source, bool $importShortCode, ?DomainResolverInterface $domainResolver = null ): self { @@ -72,7 +71,7 @@ class ShortUrl extends AbstractEntity } $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $domainResolver); - $instance->importSource = $source; + $instance->importSource = $url->source(); $instance->dateCreated = Chronos::instance($url->createdAt()); return $instance; diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index cb10ad15..81b8e923 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -12,6 +12,8 @@ use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; +use Symfony\Component\Console\Style\StyleInterface; +use function sprintf; class ImportedLinksProcessor implements ImportedLinksProcessorInterface { @@ -29,7 +31,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** * @param iterable|ImportedShlinkUrl[] $shlinkUrls */ - public function process(iterable $shlinkUrls, string $source, array $params): void + public function process(StyleInterface $io, iterable $shlinkUrls, array $params): void { /** @var ShortUrlRepositoryInterface $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); @@ -39,16 +41,20 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** @var ImportedShlinkUrl $url */ foreach ($iterable as $url) { // Skip already imported URLs - if ($shortUrlRepo->importedUrlExists($url, $source, $importShortCodes)) { + if ($shortUrlRepo->importedUrlExists($url, $importShortCodes)) { + $io->text(sprintf('%s: Skipped', $url->longUrl())); continue; } - $shortUrl = ShortUrl::fromImport($url, $source, $importShortCodes, $this->domainResolver); + $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->domainResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); + // TODO Handle errors while creating short URLs, to avoid making the whole process fail // * Duplicated short code $this->em->persist($shortUrl); + + $io->text(sprintf('%s: Imported', $url->longUrl())); } } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 5cf7d997..39e15d79 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -256,9 +256,9 @@ DQL; return $qb->getQuery()->getOneOrNullResult(); } - public function importedUrlExists(ImportedShlinkUrl $url, string $source, bool $importShortCodes): bool + public function importedUrlExists(ImportedShlinkUrl $url, bool $importShortCodes): bool { - $findConditions = ['importSource' => $source]; + $findConditions = ['importSource' => $url->source()]; if ($importShortCodes) { $findConditions['shortCode'] = $url->shortCode(); } else { diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index fac50980..b6790543 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -32,5 +32,5 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function findOneMatching(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl; - public function importedUrlExists(ImportedShlinkUrl $url, string $source, bool $importShortCodes): bool; + public function importedUrlExists(ImportedShlinkUrl $url, bool $importShortCodes): bool; } From b1a073b1abd7c6232b9aecaba44968caf48e348d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 10:26:11 +0100 Subject: [PATCH 05/12] Ensured uniqueness on imported short URLs short code --- module/Core/src/Entity/ShortUrl.php | 4 +- .../src/Importer/ImportedLinksProcessor.php | 62 +++++++++++++++++-- module/Core/test/Entity/ShortUrlTest.php | 18 +++++- 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index aba0235f..ea56fc82 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -135,8 +135,8 @@ class ShortUrl extends AbstractEntity */ public function regenerateShortCode(): self { - // In ShortUrls where a custom slug was provided, do nothing - if ($this->customSlugWasProvided) { + // In ShortUrls where a custom slug was provided, throw error, unless it is an imported one + if ($this->customSlugWasProvided && $this->importSource === null) { throw ShortCodeCannotBeRegeneratedException::forShortUrlWithCustomSlug(); } diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 81b8e923..4dc8d1a4 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -7,12 +7,14 @@ namespace Shlinkio\Shlink\Core\Importer; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Symfony\Component\Console\Style\StyleInterface; + use function sprintf; class ImportedLinksProcessor implements ImportedLinksProcessorInterface @@ -40,21 +42,71 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** @var ImportedShlinkUrl $url */ foreach ($iterable as $url) { + $longUrl = $url->longUrl(); + // Skip already imported URLs if ($shortUrlRepo->importedUrlExists($url, $importShortCodes)) { - $io->text(sprintf('%s: Skipped', $url->longUrl())); + $io->text(sprintf('%s: Skipped', $longUrl)); continue; } $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->domainResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); + if (! $this->handleShortcodeUniqueness($url, $shortUrl, $io, $importShortCodes)) { + continue; + } - // TODO Handle errors while creating short URLs, to avoid making the whole process fail - // * Duplicated short code $this->em->persist($shortUrl); - - $io->text(sprintf('%s: Imported', $url->longUrl())); + $io->text(sprintf('%s: Imported', $longUrl)); } } + + private function handleShortcodeUniqueness( + ImportedShlinkUrl $url, + ShortUrl $shortUrl, + StyleInterface $io, + bool $importShortCodes + ): bool { + if ($this->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') { + $io->text(sprintf('%s: Skipped', $longUrl)); + return false; + } + + return $this->handleShortcodeUniqueness($url, $shortUrl, $io, false); + } + + private function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool + { + $shortCode = $shortUrlToBeCreated->getShortCode(); + $domain = $shortUrlToBeCreated->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; + + /** @var ShortUrlRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); + + if (! $otherShortUrlsExist) { + return true; + } + + if ($hasCustomSlug) { + return false; + } + + $shortUrlToBeCreated->regenerateShortCode(); + return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug); + } } diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php index 054182ff..c143ae76 100644 --- a/module/Core/test/Entity/ShortUrlTest.php +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -4,12 +4,14 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Entity; +use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use function Functional\map; use function range; use function strlen; @@ -44,10 +46,12 @@ class ShortUrlTest extends TestCase ]; } - /** @test */ - public function regenerateShortCodeProperlyChangesTheValueOnValidShortUrls(): void + /** + * @test + * @dataProvider provideValidShortUrls + */ + public function regenerateShortCodeProperlyChangesTheValueOnValidShortUrls(ShortUrl $shortUrl): void { - $shortUrl = new ShortUrl(''); $firstShortCode = $shortUrl->getShortCode(); $shortUrl->regenerateShortCode(); @@ -56,6 +60,14 @@ class ShortUrlTest extends TestCase self::assertNotEquals($firstShortCode, $secondShortCode); } + public function provideValidShortUrls(): iterable + { + yield 'no custom slug' => [new ShortUrl('')]; + yield 'imported with custom slug' => [ + ShortUrl::fromImport(new ImportedShlinkUrl('', '', [], Chronos::now(), null, 'custom-slug'), true), + ]; + } + /** * @test * @dataProvider provideLengths From 786e4f642b3ea913b690322bc851c8d38d309f62 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 11:16:42 +0100 Subject: [PATCH 06/12] Moved short code uniqueness checks to external helper class that is used in UrlShortener and ImportedLinksProcessor --- module/Core/config/dependencies.config.php | 15 +++- module/Core/src/Entity/ShortUrl.php | 3 +- .../src/Importer/ImportedLinksProcessor.php | 35 +++------ .../src/Service/ShortUrl/ShortCodeHelper.php | 41 ++++++++++ .../ShortUrl/ShortCodeHelperInterface.php | 12 +++ module/Core/src/Service/UrlShortener.php | 27 ++++--- module/Core/test/Entity/ShortUrlTest.php | 2 +- .../Service/ShortUrl/ShortCodeHelperTest.php | 77 +++++++++++++++++++ module/Core/test/Service/UrlShortenerTest.php | 50 ++++-------- 9 files changed, 180 insertions(+), 82 deletions(-) create mode 100644 module/Core/src/Service/ShortUrl/ShortCodeHelper.php create mode 100644 module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php create mode 100644 module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 09c4d96e..2f0a6aa8 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -32,6 +32,7 @@ return [ Tag\TagService::class => ConfigAbstractFactory::class, Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class, Service\ShortUrl\ShortUrlResolver::class => ConfigAbstractFactory::class, + Service\ShortUrl\ShortCodeHelper::class => ConfigAbstractFactory::class, Domain\DomainService::class => ConfigAbstractFactory::class, Util\UrlValidator::class => ConfigAbstractFactory::class, @@ -61,7 +62,12 @@ return [ Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Resolver\PersistenceDomainResolver::class], + Service\UrlShortener::class => [ + Util\UrlValidator::class, + 'em', + Resolver\PersistenceDomainResolver::class, + Service\ShortUrl\ShortCodeHelper::class, + ], Service\VisitsTracker::class => [ 'em', EventDispatcherInterface::class, @@ -77,6 +83,7 @@ return [ Service\ShortUrl\ShortUrlResolver::class, ], Service\ShortUrl\ShortUrlResolver::class => ['em'], + Service\ShortUrl\ShortCodeHelper::class => ['em'], Domain\DomainService::class => ['em'], Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], @@ -104,7 +111,11 @@ return [ Mercure\MercureUpdatesGenerator::class => ['config.url_shortener.domain'], - Importer\ImportedLinksProcessor::class => ['em', Resolver\PersistenceDomainResolver::class], + Importer\ImportedLinksProcessor::class => [ + 'em', + Resolver\PersistenceDomainResolver::class, + Service\ShortUrl\ShortCodeHelper::class, + ], ], ]; diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index ea56fc82..8afa8206 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -133,7 +133,7 @@ class ShortUrl extends AbstractEntity /** * @throws ShortCodeCannotBeRegeneratedException */ - public function regenerateShortCode(): self + public function regenerateShortCode(): void { // In ShortUrls where a custom slug was provided, throw error, unless it is an imported one if ($this->customSlugWasProvided && $this->importSource === null) { @@ -146,7 +146,6 @@ class ShortUrl extends AbstractEntity } $this->shortCode = generateRandomShortCode($this->shortCodeLength); - return $this; } public function getValidSince(): ?Chronos diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 4dc8d1a4..90866057 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -7,8 +7,8 @@ namespace Shlinkio\Shlink\Core\Importer; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; @@ -23,11 +23,16 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private EntityManagerInterface $em; private DomainResolverInterface $domainResolver; + private ShortCodeHelperInterface $shortCodeHelper; - public function __construct(EntityManagerInterface $em, DomainResolverInterface $domainResolver) - { + public function __construct( + EntityManagerInterface $em, + DomainResolverInterface $domainResolver, + ShortCodeHelperInterface $shortCodeHelper + ) { $this->em = $em; $this->domainResolver = $domainResolver; + $this->shortCodeHelper = $shortCodeHelper; } /** @@ -68,7 +73,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface StyleInterface $io, bool $importShortCodes ): bool { - if ($this->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { + if ($this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, $importShortCodes)) { return true; } @@ -87,26 +92,4 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface return $this->handleShortcodeUniqueness($url, $shortUrl, $io, false); } - - private function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool - { - $shortCode = $shortUrlToBeCreated->getShortCode(); - $domain = $shortUrlToBeCreated->getDomain(); - $domainAuthority = $domain !== null ? $domain->getAuthority() : null; - - /** @var ShortUrlRepository $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); - - if (! $otherShortUrlsExist) { - return true; - } - - if ($hasCustomSlug) { - return false; - } - - $shortUrlToBeCreated->regenerateShortCode(); - return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug); - } } diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelper.php b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php new file mode 100644 index 00000000..6e4e57ac --- /dev/null +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelper.php @@ -0,0 +1,41 @@ +em = $em; + } + + public function ensureShortCodeUniqueness(ShortUrl $shortUrlToBeCreated, bool $hasCustomSlug): bool + { + $shortCode = $shortUrlToBeCreated->getShortCode(); + $domain = $shortUrlToBeCreated->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; + + /** @var ShortUrlRepository $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domainAuthority); + + if (! $otherShortUrlsExist) { + return true; + } + + if ($hasCustomSlug) { + return false; + } + + $shortUrlToBeCreated->regenerateShortCode(); + return $this->ensureShortCodeUniqueness($shortUrlToBeCreated, $hasCustomSlug); + } +} diff --git a/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php b/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php new file mode 100644 index 00000000..af3f2aa5 --- /dev/null +++ b/module/Core/src/Service/ShortUrl/ShortCodeHelperInterface.php @@ -0,0 +1,12 @@ +urlValidator = $urlValidator; $this->em = $em; $this->domainResolver = $domainResolver; + $this->shortCodeHelper = $shortCodeHelper; } /** @@ -83,20 +86,16 @@ class UrlShortener implements UrlShortenerInterface private function verifyShortCodeUniqueness(ShortUrlMeta $meta, ShortUrl $shortUrlToBeCreated): void { - $shortCode = $shortUrlToBeCreated->getShortCode(); - $domain = $meta->getDomain(); + $couldBeMadeUnique = $this->shortCodeHelper->ensureShortCodeUniqueness( + $shortUrlToBeCreated, + $meta->hasCustomSlug(), + ); - /** @var ShortUrlRepository $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - $otherShortUrlsExist = $repo->shortCodeIsInUse($shortCode, $domain); + if (! $couldBeMadeUnique) { + $domain = $shortUrlToBeCreated->getDomain(); + $domainAuthority = $domain !== null ? $domain->getAuthority() : null; - if ($otherShortUrlsExist && $meta->hasCustomSlug()) { - throw NonUniqueSlugException::fromSlug($shortCode, $domain); - } - - if ($otherShortUrlsExist) { - $shortUrlToBeCreated->regenerateShortCode(); - $this->verifyShortCodeUniqueness($meta, $shortUrlToBeCreated); + throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } } } diff --git a/module/Core/test/Entity/ShortUrlTest.php b/module/Core/test/Entity/ShortUrlTest.php index c143ae76..9f28c41b 100644 --- a/module/Core/test/Entity/ShortUrlTest.php +++ b/module/Core/test/Entity/ShortUrlTest.php @@ -10,8 +10,8 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; - use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; + use function Functional\map; use function range; use function strlen; diff --git a/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php new file mode 100644 index 00000000..d77aecd5 --- /dev/null +++ b/module/Core/test/Service/ShortUrl/ShortCodeHelperTest.php @@ -0,0 +1,77 @@ +em = $this->prophesize(EntityManagerInterface::class); + $this->helper = new ShortCodeHelper($this->em->reveal()); + + $this->shortUrl = $this->prophesize(ShortUrl::class); + $this->shortUrl->getShortCode()->willReturn('abc123'); + } + + /** + * @test + * @dataProvider provideDomains + */ + public function shortCodeIsRegeneratedIfAlreadyInUse(?Domain $domain, ?string $expectedAuthority): void + { + $callIndex = 0; + $expectedCalls = 3; + $repo = $this->prophesize(ShortUrlRepository::class); + $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', $expectedAuthority)->will( + function () use (&$callIndex, $expectedCalls) { + $callIndex++; + return $callIndex < $expectedCalls; + }, + ); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortUrl->getDomain()->willReturn($domain); + + $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), false); + + self::assertTrue($result); + $this->shortUrl->regenerateShortCode()->shouldHaveBeenCalledTimes($expectedCalls - 1); + $getRepo->shouldBeCalledTimes($expectedCalls); + $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls); + } + + public function provideDomains(): iterable + { + yield 'no domain' => [null, null]; + yield 'domain' => [new Domain($authority = 'doma.in'), $authority]; + } + + /** @test */ + public function inUseSlugReturnsError(): void + { + $repo = $this->prophesize(ShortUrlRepository::class); + $shortCodeIsInUse = $repo->shortCodeIsInUse('abc123', null)->willReturn(true); + $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortUrl->getDomain()->willReturn(null); + + $result = $this->helper->ensureShortCodeUniqueness($this->shortUrl->reveal(), true); + + self::assertFalse($result); + $this->shortUrl->regenerateShortCode()->shouldNotHaveBeenCalled(); + $getRepo->shouldBeCalledOnce(); + $shortCodeIsInUse->shouldBeCalledOnce(); + } +} diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index ba7185bf..7945c32b 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -18,6 +18,7 @@ use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; @@ -26,6 +27,7 @@ class UrlShortenerTest extends TestCase private UrlShortener $urlShortener; private ObjectProphecy $em; private ObjectProphecy $urlValidator; + private ObjectProphecy $shortCodeHelper; public function setUp(): void { @@ -51,10 +53,14 @@ class UrlShortenerTest extends TestCase $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class); + $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $this->urlShortener = new UrlShortener( $this->urlValidator->reveal(), $this->em->reveal(), new SimpleDomainResolver(), + $this->shortCodeHelper->reveal(), ); } @@ -71,29 +77,18 @@ class UrlShortenerTest extends TestCase } /** @test */ - public function shortCodeIsRegeneratedIfAlreadyInUse(): void + public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { - $callIndex = 0; - $expectedCalls = 3; - $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse(Argument::cetera())->will( - function () use (&$callIndex, $expectedCalls) { - $callIndex++; - return $callIndex < $expectedCalls; - }, - ); - $repo->findBy(Argument::cetera())->willReturn([]); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(false); - $shortUrl = $this->urlShortener->urlToShortCode( + $ensureUniqueness->shouldBeCalledOnce(); + $this->expectException(NonUniqueSlugException::class); + + $this->urlShortener->urlToShortCode( 'http://foobar.com/12345/hello?foo=bar', [], - ShortUrlMeta::createEmpty(), + ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), ); - - self::assertEquals('http://foobar.com/12345/hello?foo=bar', $shortUrl->getLongUrl()); - $getRepo->shouldBeCalledTimes($expectedCalls); - $shortCodeIsInUse->shouldBeCalledTimes($expectedCalls); } /** @test */ @@ -115,25 +110,6 @@ class UrlShortenerTest extends TestCase ); } - /** @test */ - public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void - { - $repo = $this->prophesize(ShortUrlRepository::class); - $shortCodeIsInUse = $repo->shortCodeIsInUse('custom-slug', null)->willReturn(true); - $repo->findBy(Argument::cetera())->willReturn([]); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - - $shortCodeIsInUse->shouldBeCalledOnce(); - $getRepo->shouldBeCalled(); - $this->expectException(NonUniqueSlugException::class); - - $this->urlShortener->urlToShortCode( - 'http://foobar.com/12345/hello?foo=bar', - [], - ShortUrlMeta::fromRawData(['customSlug' => 'custom-slug']), - ); - } - /** * @test * @dataProvider provideExistingShortUrls From 7c343f42c1282bba04f2df8cdca81b4524e70f07 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 11:57:26 +0100 Subject: [PATCH 07/12] Improved how existing imported short URLs are checked by tracking its original short code --- composer.json | 2 +- data/migrations/Version20201023090929.php | 11 +++++ .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 5 +++ module/Core/src/Entity/ShortUrl.php | 2 + .../src/Importer/ImportedLinksProcessor.php | 8 ++-- .../src/Repository/ShortUrlRepository.php | 40 ++++++++++++------- .../ShortUrlRepositoryInterface.php | 2 +- 7 files changed, 49 insertions(+), 21 deletions(-) diff --git a/composer.json b/composer.json index c56d584a..c346e661 100644 --- a/composer.json +++ b/composer.json @@ -53,7 +53,7 @@ "shlinkio/shlink-common": "^3.2.0", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^1.4", - "shlinkio/shlink-importer": "^2.0", + "shlinkio/shlink-importer": "^2.0.1", "shlinkio/shlink-installer": "^5.1.0", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", diff --git a/data/migrations/Version20201023090929.php b/data/migrations/Version20201023090929.php index 49a401ba..05d16c22 100644 --- a/data/migrations/Version20201023090929.php +++ b/data/migrations/Version20201023090929.php @@ -21,6 +21,15 @@ final class Version20201023090929 extends AbstractMigration 'length' => 255, 'notnull' => false, ]); + $shortUrls->addColumn('import_original_short_code', Types::STRING, [ + 'length' => 255, + 'notnull' => false, + ]); + + $shortUrls->addUniqueIndex( + [self::IMPORT_SOURCE_COLUMN, 'import_original_short_code', 'domain_id'], + 'unique_imports', + ); } public function down(Schema $schema): void @@ -29,5 +38,7 @@ final class Version20201023090929 extends AbstractMigration $this->skipIf(! $shortUrls->hasColumn(self::IMPORT_SOURCE_COLUMN)); $shortUrls->dropColumn(self::IMPORT_SOURCE_COLUMN); + $shortUrls->dropColumn('import_original_short_code'); + $shortUrls->dropIndex('unique_imports'); } } diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index 55fa230e..4f9b3747 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -56,6 +56,11 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->nullable() ->build(); + $builder->createField('importOriginalShortCode', Types::STRING) + ->columnName('import_original_short_code') + ->nullable() + ->build(); + $builder->createOneToMany('visits', Entity\Visit::class) ->mappedBy('shortUrl') ->fetchExtraLazy() diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index 8afa8206..2d5cb6de 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -36,6 +36,7 @@ class ShortUrl extends AbstractEntity private bool $customSlugWasProvided; private int $shortCodeLength; private ?string $importSource = null; + private ?string $importOriginalShortCode = null; public function __construct( string $longUrl, @@ -72,6 +73,7 @@ class ShortUrl extends AbstractEntity $instance = new self($url->longUrl(), ShortUrlMeta::fromRawData($meta), $domainResolver); $instance->importSource = $url->source(); + $instance->importOriginalShortCode = $url->shortCode(); $instance->dateCreated = Chronos::instance($url->createdAt()); return $instance; diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index 90866057..b76ada9a 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -50,7 +50,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface $longUrl = $url->longUrl(); // Skip already imported URLs - if ($shortUrlRepo->importedUrlExists($url, $importShortCodes)) { + if ($shortUrlRepo->importedUrlExists($url)) { $io->text(sprintf('%s: Skipped', $longUrl)); continue; } @@ -58,7 +58,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface $shortUrl = ShortUrl::fromImport($url, $importShortCodes, $this->domainResolver); $shortUrl->setTags($this->tagNamesToEntities($this->em, $url->tags())); - if (! $this->handleShortcodeUniqueness($url, $shortUrl, $io, $importShortCodes)) { + if (! $this->handleShortCodeUniqueness($url, $shortUrl, $io, $importShortCodes)) { continue; } @@ -67,7 +67,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface } } - private function handleShortcodeUniqueness( + private function handleShortCodeUniqueness( ImportedShlinkUrl $url, ShortUrl $shortUrl, StyleInterface $io, @@ -90,6 +90,6 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface return false; } - return $this->handleShortcodeUniqueness($url, $shortUrl, $io, false); + return $this->handleShortCodeUniqueness($url, $shortUrl, $io, false); } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 39e15d79..27dac54b 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -190,13 +190,7 @@ DQL; ->setParameter('slug', $slug) ->setMaxResults(1); - if ($domain !== null) { - $qb->join('s.domain', 'd') - ->andWhere($qb->expr()->eq('d.authority', ':authority')) - ->setParameter('authority', $domain); - } else { - $qb->andWhere($qb->expr()->isNull('s.domain')); - } + $this->whereDomainIs($qb, $domain); return $qb; } @@ -256,15 +250,31 @@ DQL; return $qb->getQuery()->getOneOrNullResult(); } - public function importedUrlExists(ImportedShlinkUrl $url, bool $importShortCodes): bool + public function importedUrlExists(ImportedShlinkUrl $url): bool { - $findConditions = ['importSource' => $url->source()]; - if ($importShortCodes) { - $findConditions['shortCode'] = $url->shortCode(); - } else { - $findConditions['longUrl'] = $url->longUrl(); - } + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('COUNT(DISTINCT s.id)') + ->from(ShortUrl::class, 's') + ->andWhere($qb->expr()->eq('s.importOriginalShortCode', ':shortCode')) + ->setParameter('shortCode', $url->shortCode()) + ->andWhere($qb->expr()->eq('s.importSource', ':importSource')) + ->setParameter('importSource', $url->source()) + ->setMaxResults(1); - return $this->count($findConditions) > 0; + $this->whereDomainIs($qb, $url->domain()); + + $result = (int) $qb->getQuery()->getSingleScalarResult(); + return $result > 0; + } + + private function whereDomainIs(QueryBuilder $qb, ?string $domain): void + { + if ($domain !== null) { + $qb->join('s.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', ':authority')) + ->setParameter('authority', $domain); + } else { + $qb->andWhere($qb->expr()->isNull('s.domain')); + } } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index b6790543..1d6f38a8 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -32,5 +32,5 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function findOneMatching(string $url, array $tags, ShortUrlMeta $meta): ?ShortUrl; - public function importedUrlExists(ImportedShlinkUrl $url, bool $importShortCodes): bool; + public function importedUrlExists(ImportedShlinkUrl $url): bool; } From fdcf88de670524b864af8cefa8f198310a7e72c2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 12:06:48 +0100 Subject: [PATCH 08/12] Added database tests for ShortUrlRepository::importedUrlExists --- .../Repository/ShortUrlRepositoryTest.php | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index ad99a9a3..86eb2aa3 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -17,6 +17,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; +use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; use function count; @@ -320,4 +321,26 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->repo->findOneMatching('foo', $tags, ShortUrlMeta::fromRawData($meta)), ); } + + /** @test */ + public function importedShortUrlsAreSearchedAsExpected(): void + { + $buildImported = static fn (string $shortCode, ?String $domain = null) => + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), $domain, $shortCode); + + $shortUrlWithoutDomain = ShortUrl::fromImport($buildImported('my-cool-slug'), true); + $this->getEntityManager()->persist($shortUrlWithoutDomain); + + $shortUrlWithDomain = ShortUrl::fromImport($buildImported('another-slug', 'doma.in'), true); + $this->getEntityManager()->persist($shortUrlWithDomain); + + $this->getEntityManager()->flush(); + + self::assertTrue($this->repo->importedUrlExists($buildImported('my-cool-slug'))); + self::assertTrue($this->repo->importedUrlExists($buildImported('another-slug', 'doma.in'))); + self::assertFalse($this->repo->importedUrlExists($buildImported('non-existing-slug'))); + self::assertFalse($this->repo->importedUrlExists($buildImported('non-existing-slug', 'doma.in'))); + self::assertFalse($this->repo->importedUrlExists($buildImported('my-cool-slug', 'doma.in'))); + self::assertFalse($this->repo->importedUrlExists($buildImported('another-slug'))); + } } From 03a9697298c1b4ac101a09e38e846c6bfe71f695 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 13:20:34 +0100 Subject: [PATCH 09/12] Created ImportedLinksProcessorTest --- module/Core/config/dependencies.config.php | 3 + .../src/Importer/ImportedLinksProcessor.php | 11 +- ...chIterator.php => DoctrineBatchHelper.php} | 18 +-- .../src/Util/DoctrineBatchHelperInterface.php | 10 ++ .../Importer/ImportedLinksProcessorTest.php | 145 ++++++++++++++++++ 5 files changed, 171 insertions(+), 16 deletions(-) rename module/Core/src/Util/{DoctrineBatchIterator.php => DoctrineBatchHelper.php} (63%) create mode 100644 module/Core/src/Util/DoctrineBatchHelperInterface.php create mode 100644 module/Core/test/Importer/ImportedLinksProcessorTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 2f0a6aa8..4d68101b 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -36,6 +36,7 @@ return [ Domain\DomainService::class => ConfigAbstractFactory::class, Util\UrlValidator::class => ConfigAbstractFactory::class, + Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, Action\RedirectAction::class => ConfigAbstractFactory::class, Action\PixelAction::class => ConfigAbstractFactory::class, @@ -87,6 +88,7 @@ return [ Domain\DomainService::class => ['em'], Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], + Util\DoctrineBatchHelper::class => ['em'], Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, @@ -115,6 +117,7 @@ return [ 'em', Resolver\PersistenceDomainResolver::class, Service\ShortUrl\ShortCodeHelper::class, + Util\DoctrineBatchHelper::class, ], ], diff --git a/module/Core/src/Importer/ImportedLinksProcessor.php b/module/Core/src/Importer/ImportedLinksProcessor.php index b76ada9a..e072fbb8 100644 --- a/module/Core/src/Importer/ImportedLinksProcessor.php +++ b/module/Core/src/Importer/ImportedLinksProcessor.php @@ -9,7 +9,7 @@ use Shlinkio\Shlink\Core\Domain\Resolver\DomainResolverInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortCodeHelperInterface; -use Shlinkio\Shlink\Core\Util\DoctrineBatchIterator; +use Shlinkio\Shlink\Core\Util\DoctrineBatchHelperInterface; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -24,15 +24,18 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface private EntityManagerInterface $em; private DomainResolverInterface $domainResolver; private ShortCodeHelperInterface $shortCodeHelper; + private DoctrineBatchHelperInterface $batchHelper; public function __construct( EntityManagerInterface $em, DomainResolverInterface $domainResolver, - ShortCodeHelperInterface $shortCodeHelper + ShortCodeHelperInterface $shortCodeHelper, + DoctrineBatchHelperInterface $batchHelper ) { $this->em = $em; $this->domainResolver = $domainResolver; $this->shortCodeHelper = $shortCodeHelper; + $this->batchHelper = $batchHelper; } /** @@ -43,7 +46,7 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface /** @var ShortUrlRepositoryInterface $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); $importShortCodes = $params['import_short_codes']; - $iterable = new DoctrineBatchIterator($shlinkUrls, $this->em, 100); + $iterable = $this->batchHelper->wrapIterable($shlinkUrls, 100); /** @var ImportedShlinkUrl $url */ foreach ($iterable as $url) { @@ -90,6 +93,6 @@ class ImportedLinksProcessor implements ImportedLinksProcessorInterface return false; } - return $this->handleShortCodeUniqueness($url, $shortUrl, $io, false); + return $this->shortCodeHelper->ensureShortCodeUniqueness($shortUrl, false); } } diff --git a/module/Core/src/Util/DoctrineBatchIterator.php b/module/Core/src/Util/DoctrineBatchHelper.php similarity index 63% rename from module/Core/src/Util/DoctrineBatchIterator.php rename to module/Core/src/Util/DoctrineBatchHelper.php index 05311b09..207d2093 100644 --- a/module/Core/src/Util/DoctrineBatchIterator.php +++ b/module/Core/src/Util/DoctrineBatchHelper.php @@ -5,32 +5,26 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Util; use Doctrine\ORM\EntityManagerInterface; -use IteratorAggregate; use Throwable; /** * Inspired by ocramius/doctrine-batch-utils https://github.com/Ocramius/DoctrineBatchUtils */ -class DoctrineBatchIterator implements IteratorAggregate +class DoctrineBatchHelper implements DoctrineBatchHelperInterface { - private iterable $resultSet; private EntityManagerInterface $em; - private int $batchSize; - public function __construct(iterable $resultSet, EntityManagerInterface $em, int $batchSize) + public function __construct(EntityManagerInterface $em) { - $this->resultSet = $resultSet; $this->em = $em; - $this->batchSize = $batchSize; } /** * @throws Throwable */ - public function getIterator(): iterable + public function wrapIterable(iterable $resultSet, int $batchSize): iterable { $iteration = 0; - $resultSet = $this->resultSet; $this->em->beginTransaction(); @@ -38,7 +32,7 @@ class DoctrineBatchIterator implements IteratorAggregate foreach ($resultSet as $key => $value) { $iteration++; yield $key => $value; - $this->flushAndClearBatch($iteration); + $this->flushAndClearBatch($iteration, $batchSize); } } catch (Throwable $e) { $this->em->rollback(); @@ -50,9 +44,9 @@ class DoctrineBatchIterator implements IteratorAggregate $this->em->commit(); } - private function flushAndClearBatch(int $iteration): void + private function flushAndClearBatch(int $iteration, int $batchSize): void { - if ($iteration % $this->batchSize) { + if ($iteration % $batchSize) { return; } diff --git a/module/Core/src/Util/DoctrineBatchHelperInterface.php b/module/Core/src/Util/DoctrineBatchHelperInterface.php new file mode 100644 index 00000000..941561ed --- /dev/null +++ b/module/Core/src/Util/DoctrineBatchHelperInterface.php @@ -0,0 +1,10 @@ +em = $this->prophesize(EntityManagerInterface::class); + $this->repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $this->em->getRepository(ShortUrl::class)->willReturn($this->repo->reveal()); + + $this->shortCodeHelper = $this->prophesize(ShortCodeHelperInterface::class); + $batchHelper = $this->prophesize(DoctrineBatchHelperInterface::class); + $batchHelper->wrapIterable(Argument::cetera())->willReturnArgument(0); + + $this->processor = new ImportedLinksProcessor( + $this->em->reveal(), + new SimpleDomainResolver(), + $this->shortCodeHelper->reveal(), + $batchHelper->reveal(), + ); + + $this->io = $this->prophesize(StyleInterface::class); + } + + /** @test */ + public function newUrlsWithNoErrorsAreAllPersisted(): void + { + $urls = [ + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), + ]; + $expectedCalls = count($urls); + + $importedUrlExists = $this->repo->importedUrlExists(Argument::cetera())->willReturn(false); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $persist = $this->em->persist(Argument::type(ShortUrl::class)); + + $this->processor->process($this->io->reveal(), $urls, ['import_short_codes' => true]); + + $importedUrlExists->shouldHaveBeenCalledTimes($expectedCalls); + $ensureUniqueness->shouldHaveBeenCalledTimes($expectedCalls); + $persist->shouldHaveBeenCalledTimes($expectedCalls); + $this->io->text(Argument::type('string'))->shouldHaveBeenCalledTimes($expectedCalls); + } + + /** @test */ + public function alreadyImportedUrlsAreSkipped(): void + { + $urls = [ + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), + new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2'), + new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3'), + ]; + $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); + + $importedUrlExists = $this->repo->importedUrlExists(Argument::cetera())->will(function (array $args): bool { + /** @var ImportedShlinkUrl $url */ + [$url] = $args; + + return contains(['foo', 'baz2', 'baz3'], $url->longUrl()); + }); + $ensureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness(Argument::cetera())->willReturn(true); + $persist = $this->em->persist(Argument::type(ShortUrl::class)); + + $this->processor->process($this->io->reveal(), $urls, ['import_short_codes' => true]); + + $importedUrlExists->shouldHaveBeenCalledTimes(count($urls)); + $ensureUniqueness->shouldHaveBeenCalledTimes(2); + $persist->shouldHaveBeenCalledTimes(2); + $this->io->text(Argument::that($contains('Skipped')))->shouldHaveBeenCalledTimes(3); + $this->io->text(Argument::that($contains('Imported')))->shouldHaveBeenCalledTimes(2); + } + + /** @test */ + public function nonUniqueShortCodesAreAskedToUser(): void + { + $urls = [ + new ImportedShlinkUrl('', 'foo', [], Chronos::now(), null, 'foo'), + new ImportedShlinkUrl('', 'bar', [], Chronos::now(), null, 'bar'), + new ImportedShlinkUrl('', 'baz', [], Chronos::now(), null, 'baz'), + new ImportedShlinkUrl('', 'baz2', [], Chronos::now(), null, 'baz2'), + new ImportedShlinkUrl('', 'baz3', [], Chronos::now(), null, 'baz3'), + ]; + $contains = fn (string $needle) => fn (string $text) => str_contains($text, $needle); + + $importedUrlExists = $this->repo->importedUrlExists(Argument::cetera())->willReturn(false); + $failingEnsureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness( + Argument::any(), + true, + )->willReturn(false); + $successEnsureUniqueness = $this->shortCodeHelper->ensureShortCodeUniqueness( + Argument::any(), + false, + )->willReturn(true); + $choice = $this->io->choice(Argument::cetera())->will(function (array $args) { + /** @var ImportedShlinkUrl $url */ + [$question] = $args; + + return some(['foo', 'baz2', 'baz3'], fn (string $item) => str_contains($question, $item)) ? 'Skip' : ''; + }); + $persist = $this->em->persist(Argument::type(ShortUrl::class)); + + $this->processor->process($this->io->reveal(), $urls, ['import_short_codes' => true]); + + $importedUrlExists->shouldHaveBeenCalledTimes(count($urls)); + $failingEnsureUniqueness->shouldHaveBeenCalledTimes(5); + $successEnsureUniqueness->shouldHaveBeenCalledTimes(2); + $choice->shouldHaveBeenCalledTimes(5); + $persist->shouldHaveBeenCalledTimes(2); + $this->io->text(Argument::that($contains('Skipped')))->shouldHaveBeenCalledTimes(3); + $this->io->text(Argument::that($contains('Imported')))->shouldHaveBeenCalledTimes(2); + } +} From de7096010e9dc896b9a2667ef6df86f298b564d3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 13:30:18 +0100 Subject: [PATCH 10/12] Created DoctrineBatchHelperTest --- .../test/Util/DoctrineBatchHelperTest.php | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 module/Core/test/Util/DoctrineBatchHelperTest.php diff --git a/module/Core/test/Util/DoctrineBatchHelperTest.php b/module/Core/test/Util/DoctrineBatchHelperTest.php new file mode 100644 index 00000000..a89a653f --- /dev/null +++ b/module/Core/test/Util/DoctrineBatchHelperTest.php @@ -0,0 +1,70 @@ +em = $this->prophesize(EntityManagerInterface::class); + $this->helper = new DoctrineBatchHelper($this->em->reveal()); + } + + /** + * @test + * @dataProvider provideIterables + */ + public function entityManagerIsFlushedAndClearedTheExpectedAmountOfTimes( + array $iterable, + int $batchSize, + int $expectedCalls + ): void { + $wrappedIterable = $this->helper->wrapIterable($iterable, $batchSize); + + foreach ($wrappedIterable as $item) { + // Iterable needs to be iterated for the logic to be invoked + } + + $this->em->beginTransaction()->shouldHaveBeenCalledOnce(); + $this->em->commit()->shouldHaveBeenCalledOnce(); + $this->em->rollback()->shouldNotHaveBeenCalled(); + $this->em->flush()->shouldHaveBeenCalledTimes($expectedCalls); + $this->em->clear()->shouldHaveBeenCalledTimes($expectedCalls); + } + + public function provideIterables(): iterable + { + yield [[], 100, 1]; + yield [[1, 2, 3, 4, 5, 6, 7, 8, 9, 10], 3, 4]; + yield [[1, 2, 3, 4, 5, 6, 7, 8, 9, 10], 11, 1]; + } + + /** @test */ + public function transactionIsRolledBackWhenAnErrorOccurs(): void + { + $flush = $this->em->flush()->willThrow(RuntimeException::class); + + $wrappedIterable = $this->helper->wrapIterable([1, 2, 3], 1); + + self::expectException(RuntimeException::class); + $flush->shouldBeCalledOnce(); + $this->em->beginTransaction()->shouldBeCalledOnce(); + $this->em->commit()->shouldNotBeCalled(); + $this->em->rollback()->shouldBeCalledOnce(); + + foreach ($wrappedIterable as $item) { + // Iterable needs to be iterated for the logic to be invoked + } + } +} From 90b4bc9b1a60408af8d11ec3e6468d13ae2a9927 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 13:36:21 +0100 Subject: [PATCH 11/12] Updated changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c044b9b..b2afc50f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#832](https://github.com/shlinkio/shlink/issues/832) Added support to customize the port in which the docker image listens by using the `PORT` env var or the `port` config option. +* [#860](https://github.com/shlinkio/shlink/issues/860) Added support to import links from bit.ly. + + Run the command `short-urls:import bitly` and introduce requested information in order to import all your links. + + Other sources will be supported in future releases. + #### Changed * [#836](https://github.com/shlinkio/shlink/issues/836) Added support for the `-` notation while determining how to order the short URLs list, as in `?orderBy=shortCode-DESC`. This effectively deprecates the array notation (`?orderBy[shortCode]=DESC`), that will be removed in Shlink 3.0.0 From b091bd4e2a324a6d57af4881e2b1583808e1b7da Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Oct 2020 13:46:39 +0100 Subject: [PATCH 12/12] Ensured composer 1 for now --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index c9945281..d45f1d49 100644 --- a/.travis.yml +++ b/.travis.yml @@ -47,7 +47,7 @@ before_install: - yes | pecl install pdo_sqlsrv-5.9.0preview1 swoole-4.5.5 pcov install: - - composer self-update + - composer self-update --1 - composer install --no-interaction --prefer-dist $COMPOSER_FLAGS before_script: