From 12150f775ddacace6c02c8cb0c812ce9826d950a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Jan 2023 13:45:39 +0100 Subject: [PATCH 01/21] Created persistence for device long URLs --- composer.json | 1 + config/config.php | 3 +- data/migrations/Version20230103105343.php | 53 +++++++++++++++++++ ...ink.Core.ShortUrl.Entity.DeviceLongUrl.php | 41 ++++++++++++++ ...o.Shlink.Core.ShortUrl.Entity.ShortUrl.php | 2 +- module/Core/functions/functions.php | 18 +++++++ module/Core/src/Config/EnvVars.php | 10 ---- module/Core/src/Model/DeviceType.php | 28 ++++++++++ .../src/ShortUrl/Entity/DeviceLongUrl.php | 18 +++++++ .../src/ShortUrl/Model/OrderableField.php | 9 ---- module/Core/src/ShortUrl/Model/TagsMode.php | 7 --- .../Validation/ShortUrlsParamsInputFilter.php | 6 ++- module/Core/test/Config/EnvVarsTest.php | 8 --- module/Core/test/Functions/FunctionsTest.php | 43 +++++++++++++++ 14 files changed, 209 insertions(+), 38 deletions(-) create mode 100644 data/migrations/Version20230103105343.php create mode 100644 module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.DeviceLongUrl.php create mode 100644 module/Core/src/Model/DeviceType.php create mode 100644 module/Core/src/ShortUrl/Entity/DeviceLongUrl.php create mode 100644 module/Core/test/Functions/FunctionsTest.php diff --git a/composer.json b/composer.json index 85608742..1ad2e7ab 100644 --- a/composer.json +++ b/composer.json @@ -40,6 +40,7 @@ "mezzio/mezzio-problem-details": "^1.7", "mezzio/mezzio-swoole": "^4.5", "mlocati/ip-lib": "^1.18", + "mobiledetect/mobiledetectlib": "^3.74", "ocramius/proxy-manager": "^2.14", "pagerfanta/core": "^3.6", "php-middleware/request-id": "^4.1", diff --git a/config/config.php b/config/config.php index 8fe311a0..e0ec6c23 100644 --- a/config/config.php +++ b/config/config.php @@ -15,6 +15,7 @@ use function class_exists; use function Shlinkio\Shlink\Config\env; use function Shlinkio\Shlink\Config\openswooleIsInstalled; use function Shlinkio\Shlink\Config\runningInRoadRunner; +use function Shlinkio\Shlink\Core\enumValues; use const PHP_SAPI; @@ -23,7 +24,7 @@ $enableSwoole = PHP_SAPI === 'cli' && openswooleIsInstalled() && ! runningInRoad return (new ConfigAggregator\ConfigAggregator([ ! $isTestEnv - ? new EnvVarLoaderProvider('config/params/generated_config.php', Core\Config\EnvVars::values()) + ? new EnvVarLoaderProvider('config/params/generated_config.php', enumValues(Core\Config\EnvVars::class)) : new ConfigAggregator\ArrayProvider([]), Mezzio\ConfigProvider::class, Mezzio\Router\ConfigProvider::class, diff --git a/data/migrations/Version20230103105343.php b/data/migrations/Version20230103105343.php new file mode 100644 index 00000000..c61a8a94 --- /dev/null +++ b/data/migrations/Version20230103105343.php @@ -0,0 +1,53 @@ +skipIf($schema->hasTable(self::TABLE_NAME)); + + $table = $schema->createTable(self::TABLE_NAME); + $table->addColumn('id', Types::BIGINT, [ + 'unsigned' => true, + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->setPrimaryKey(['id']); + + $table->addColumn('device_type', Types::STRING, ['length' => 255]); + $table->addColumn('long_url', Types::STRING, ['length' => 2048]); + $table->addColumn('short_url_id', Types::BIGINT, [ + 'unsigned' => true, + 'notnull' => true, + ]); + + $table->addForeignKeyConstraint('short_urls', ['short_url_id'], ['id'], [ + 'onDelete' => 'CASCADE', + 'onUpdate' => 'RESTRICT', + ]); + + $table->addUniqueIndex(['device_type', 'short_url_id'], 'UQ_device_type_per_short_url'); + } + + public function down(Schema $schema): void + { + $this->skipIf(! $schema->hasTable(self::TABLE_NAME)); + $schema->dropTable(self::TABLE_NAME); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.DeviceLongUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.DeviceLongUrl.php new file mode 100644 index 00000000..8de69c18 --- /dev/null +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.DeviceLongUrl.php @@ -0,0 +1,41 @@ +setTable(determineTableName('device_long_urls', $emConfig)); + + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); + + (new FieldBuilder($builder, [ + 'fieldName' => 'deviceType', + 'type' => Types::STRING, + 'enumType' => DeviceType::class, + ]))->columnName('device_type') + ->length(255) + ->build(); + + fieldWithUtf8Charset($builder->createField('longUrl', Types::STRING), $emConfig) + ->columnName('long_url') + ->length(2048) + ->build(); + + $builder->createManyToOne('shortUrl', ShortUrl\Entity\ShortUrl::class) + ->addJoinColumn('short_url_id', 'id', false, false, 'CASCADE') + ->build(); +}; diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php index 6b769f34..b67996ae 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php @@ -24,7 +24,7 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->build(); fieldWithUtf8Charset($builder->createField('longUrl', Types::STRING), $emConfig) - ->columnName('original_url') + ->columnName('original_url') // Rename to long_url some day? ¯\_(ツ)_/¯ ->length(2048) ->build(); diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 9d0b8d68..574d604c 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; +use BackedEnum; use Cake\Chronos\Chronos; use Cake\Chronos\ChronosInterface; use DateTimeInterface; @@ -16,6 +17,7 @@ use PUGX\Shortid\Factory as ShortIdFactory; use Shlinkio\Shlink\Common\Util\DateRange; use function date_default_timezone_get; +use function Functional\map; use function Functional\reduce_left; use function is_array; use function print_r; @@ -159,3 +161,19 @@ function toProblemDetailsType(string $errorCode): string { return sprintf('https://shlink.io/api/error/%s', $errorCode); } + +/** + * @param class-string $enum + * @return string[] + */ +function enumValues(string $enum): array +{ + static $cache; + if ($cache === null) { + $cache = []; + } + + return $cache[$enum] ?? ( + $cache[$enum] = map($enum::cases(), static fn (BackedEnum $type) => (string) $type->value) + ); +} diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index 228a5921..75454ecc 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Config; -use function Functional\map; use function Shlinkio\Shlink\Config\env; enum EnvVars: string @@ -77,13 +76,4 @@ enum EnvVars: string { return $this->loadFromEnv() !== null; } - - /** - * @return string[] - */ - public static function values(): array - { - static $values; - return $values ?? ($values = map(self::cases(), static fn (EnvVars $envVar) => $envVar->value)); - } } diff --git a/module/Core/src/Model/DeviceType.php b/module/Core/src/Model/DeviceType.php new file mode 100644 index 00000000..df4a1838 --- /dev/null +++ b/module/Core/src/Model/DeviceType.php @@ -0,0 +1,28 @@ +is('iOS') && $detect->isTablet() => self::IOS, // TODO To detect iPad only +// $detect->is('iOS') && ! $detect->isTablet() => self::IOS, // TODO To detect iPhone only +// $detect->is('androidOS') && $detect->isTablet() => self::ANDROID, // TODO To detect Android tablets +// $detect->is('androidOS') && ! $detect->isTablet() => self::ANDROID, // TODO To detect Android phones + $detect->is('iOS') => self::IOS, // Detects both iPhone and iPad + $detect->is('androidOS') => self::ANDROID, // Detects both android phones and android tablets + ! $detect->isMobile() && ! $detect->isTablet() => self::DESKTOP, + default => null, + }; + } +} diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php new file mode 100644 index 00000000..faf9bcc3 --- /dev/null +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -0,0 +1,18 @@ + $field->value); - } - public static function isBasicField(string $value): bool { return contains( diff --git a/module/Core/src/ShortUrl/Model/TagsMode.php b/module/Core/src/ShortUrl/Model/TagsMode.php index 01cdcc3b..593d6d83 100644 --- a/module/Core/src/ShortUrl/Model/TagsMode.php +++ b/module/Core/src/ShortUrl/Model/TagsMode.php @@ -4,15 +4,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Model; -use function Functional\map; - enum TagsMode: string { case ANY = 'any'; case ALL = 'all'; - - public static function values(): array - { - return map(self::cases(), static fn (TagsMode $mode) => $mode->value); - } } diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlsParamsInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlsParamsInputFilter.php index cb120e8e..d7cda41e 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlsParamsInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlsParamsInputFilter.php @@ -11,6 +11,8 @@ use Shlinkio\Shlink\Common\Validation; use Shlinkio\Shlink\Core\ShortUrl\Model\OrderableField; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; +use function Shlinkio\Shlink\Core\enumValues; + class ShortUrlsParamsInputFilter extends InputFilter { use Validation\InputFactoryTrait; @@ -46,12 +48,12 @@ class ShortUrlsParamsInputFilter extends InputFilter $tagsMode = $this->createInput(self::TAGS_MODE, false); $tagsMode->getValidatorChain()->attach(new InArray([ - 'haystack' => TagsMode::values(), + 'haystack' => enumValues(TagsMode::class), 'strict' => InArray::COMPARE_STRICT, ])); $this->add($tagsMode); - $this->add($this->createOrderByInput(self::ORDER_BY, OrderableField::values())); + $this->add($this->createOrderByInput(self::ORDER_BY, enumValues(OrderableField::class))); $this->add($this->createBooleanInput(self::EXCLUDE_MAX_VISITS_REACHED, false)); $this->add($this->createBooleanInput(self::EXCLUDE_PAST_VALID_UNTIL, false)); diff --git a/module/Core/test/Config/EnvVarsTest.php b/module/Core/test/Config/EnvVarsTest.php index ff4878de..6d4b1394 100644 --- a/module/Core/test/Config/EnvVarsTest.php +++ b/module/Core/test/Config/EnvVarsTest.php @@ -7,7 +7,6 @@ namespace ShlinkioTest\Shlink\Core\Config; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Config\EnvVars; -use function Functional\map; use function putenv; class EnvVarsTest extends TestCase @@ -59,11 +58,4 @@ class EnvVarsTest extends TestCase yield 'DB_DRIVER without default' => [EnvVars::DB_DRIVER, null, null]; yield 'DB_DRIVER with default' => [EnvVars::DB_DRIVER, 'foobar', 'foobar']; } - - /** @test */ - public function allValuesCanBeListed(): void - { - $expected = map(EnvVars::cases(), static fn (EnvVars $envVar) => $envVar->value); - self::assertEquals(EnvVars::values(), $expected); - } } diff --git a/module/Core/test/Functions/FunctionsTest.php b/module/Core/test/Functions/FunctionsTest.php new file mode 100644 index 00000000..5ba6a7db --- /dev/null +++ b/module/Core/test/Functions/FunctionsTest.php @@ -0,0 +1,43 @@ + [EnvVars::class, map(EnvVars::cases(), static fn (EnvVars $envVar) => $envVar->value)]; + yield VisitType::class => [ + VisitType::class, + map(VisitType::cases(), static fn (VisitType $envVar) => $envVar->value), + ]; + yield DeviceType::class => [ + DeviceType::class, + map(DeviceType::cases(), static fn (DeviceType $envVar) => $envVar->value), + ]; + yield OrderableField::class => [ + OrderableField::class, + map(OrderableField::cases(), static fn (OrderableField $envVar) => $envVar->value), + ]; + } +} From 1447687ebe9ae18af58ef4882e06f52efe56da65 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 14 Jan 2023 15:19:47 +0100 Subject: [PATCH 02/21] Add deviceLongUrls to short URL creation --- .../ShortUrl/CreateShortUrlCommandTest.php | 7 +- .../src/ShortUrl/Entity/DeviceLongUrl.php | 14 +- module/Core/src/ShortUrl/Entity/ShortUrl.php | 27 +-- .../src/ShortUrl/Model/ShortUrlCreation.php | 198 +++++++----------- .../Model/Validation/ShortUrlInputFilter.php | 49 ++++- .../Repository/ShortUrlRepository.php | 28 +-- .../ShortUrlRepositoryInterface.php | 2 +- module/Core/src/ShortUrl/UrlShortener.php | 6 +- .../Tag/Repository/TagRepositoryTest.php | 6 +- .../Visit/Repository/VisitRepositoryTest.php | 20 +- .../NotifyNewShortUrlToMercureTest.php | 4 +- .../PublishingUpdatesGeneratorTest.php | 8 +- .../NotifyNewShortUrlToRabbitMqTest.php | 4 +- .../RabbitMq/NotifyVisitToRabbitMqTest.php | 4 +- .../NotifyNewShortUrlToRedisTest.php | 2 +- module/Core/test/Functions/FunctionsTest.php | 2 + .../test/ShortUrl/Entity/ShortUrlTest.php | 6 +- .../Helper/ShortUrlStringifierTest.php | 2 +- .../ExtraPathRedirectMiddlewareTest.php | 2 +- .../ShortUrl/Model/ShortUrlCreationTest.php | 32 +-- .../test/ShortUrl/ShortUrlResolverTest.php | 8 +- .../ShortUrlDataTransformerTest.php | 15 +- .../test-api/Action/CreateShortUrlTest.php | 5 +- 23 files changed, 222 insertions(+), 229 deletions(-) diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index 734089c9..1a8df888 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -100,9 +100,8 @@ class CreateShortUrlCommandTest extends TestCase { $shortUrl = ShortUrl::createEmpty(); $this->urlShortener->expects($this->once())->method('shorten')->with( - $this->callback(function (ShortUrlCreation $meta) { - $tags = $meta->getTags(); - Assert::assertEquals(['foo', 'bar', 'baz', 'boo', 'zar'], $tags); + $this->callback(function (ShortUrlCreation $creation) { + Assert::assertEquals(['foo', 'bar', 'baz', 'boo', 'zar'], $creation->tags); return true; }), )->willReturn($shortUrl); @@ -128,7 +127,7 @@ class CreateShortUrlCommandTest extends TestCase { $this->urlShortener->expects($this->once())->method('shorten')->with( $this->callback(function (ShortUrlCreation $meta) use ($expectedDomain) { - Assert::assertEquals($expectedDomain, $meta->getDomain()); + Assert::assertEquals($expectedDomain, $meta->domain); return true; }), )->willReturn(ShortUrl::createEmpty()); diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php index faf9bcc3..b1dc1086 100644 --- a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -9,10 +9,20 @@ use Shlinkio\Shlink\Core\Model\DeviceType; class DeviceLongUrl extends AbstractEntity { - private function __construct( + public function __construct( public readonly ShortUrl $shortUrl, public readonly DeviceType $deviceType, - public readonly string $longUrl, + private string $longUrl, ) { } + + public function longUrl(): string + { + return $this->longUrl; + } + + public function updateLongUrl(string $longUrl): void + { + $this->longUrl = $longUrl; + } } diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 0ebdeb24..6c49e1c3 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -60,6 +60,9 @@ class ShortUrl extends AbstractEntity return self::create(ShortUrlCreation::createEmpty()); } + /** + * @param non-empty-string $longUrl + */ public static function withLongUrl(string $longUrl): self { return self::create(ShortUrlCreation::fromRawData([ShortUrlInputFilter::LONG_URL => $longUrl])); @@ -75,19 +78,19 @@ class ShortUrl extends AbstractEntity $instance->longUrl = $creation->getLongUrl(); $instance->dateCreated = Chronos::now(); $instance->visits = new ArrayCollection(); - $instance->tags = $relationResolver->resolveTags($creation->getTags()); - $instance->validSince = $creation->getValidSince(); - $instance->validUntil = $creation->getValidUntil(); - $instance->maxVisits = $creation->getMaxVisits(); + $instance->tags = $relationResolver->resolveTags($creation->tags); + $instance->validSince = $creation->validSince; + $instance->validUntil = $creation->validUntil; + $instance->maxVisits = $creation->maxVisits; $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(); + $instance->shortCodeLength = $creation->shortCodeLength; + $instance->shortCode = $creation->customSlug ?? generateRandomShortCode($instance->shortCodeLength); + $instance->domain = $relationResolver->resolveDomain($creation->domain); + $instance->authorApiKey = $creation->apiKey; + $instance->title = $creation->title; + $instance->titleWasAutoResolved = $creation->titleWasAutoResolved; + $instance->crawlable = $creation->crawlable; + $instance->forwardQuery = $creation->forwardQuery; return $instance; } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php index bbdd9ab0..e2af5cf1 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php @@ -6,85 +6,106 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function Functional\map; use function Shlinkio\Shlink\Core\getNonEmptyOptionalValueFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalBoolFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalIntFromInputFilter; use function Shlinkio\Shlink\Core\normalizeOptionalDate; +use function trim; use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; final class ShortUrlCreation implements TitleResolutionModelInterface { - private string $longUrl; - private ?Chronos $validSince = null; - private ?Chronos $validUntil = null; - private ?string $customSlug = null; - private ?int $maxVisits = null; - private ?bool $findIfExists = null; - private ?string $domain = null; - private int $shortCodeLength = 5; - private bool $validateUrl = false; - private ?ApiKey $apiKey = null; - private array $tags = []; - private ?string $title = null; - private bool $titleWasAutoResolved = false; - private bool $crawlable = false; - private bool $forwardQuery = true; - - private function __construct() - { + /** + * @param string[] $tags + * @param array{DeviceType, string}[] $deviceLongUrls + */ + private function __construct( + public readonly string $longUrl, + public readonly array $deviceLongUrls = [], + public readonly ?Chronos $validSince = null, + public readonly ?Chronos $validUntil = null, + public readonly ?string $customSlug = null, + public readonly ?int $maxVisits = null, + public readonly bool $findIfExists = false, + public readonly ?string $domain = null, + public readonly int $shortCodeLength = 5, + public readonly bool $validateUrl = false, + public readonly ?ApiKey $apiKey = null, + public readonly array $tags = [], + public readonly ?string $title = null, + public readonly bool $titleWasAutoResolved = false, + public readonly bool $crawlable = false, + public readonly bool $forwardQuery = true, + ) { } public static function createEmpty(): self { - $instance = new self(); - $instance->longUrl = ''; - - return $instance; + return new self(''); } /** * @throws ValidationException */ public static function fromRawData(array $data): self - { - $instance = new self(); - $instance->validateAndInit($data); - - return $instance; - } - - /** - * @throws ValidationException - */ - private function validateAndInit(array $data): void { $inputFilter = ShortUrlInputFilter::withRequiredLongUrl($data); if (! $inputFilter->isValid()) { throw ValidationException::fromInputFilter($inputFilter); } - $this->longUrl = $inputFilter->getValue(ShortUrlInputFilter::LONG_URL); - $this->validSince = normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)); - $this->validUntil = normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)); - $this->customSlug = $inputFilter->getValue(ShortUrlInputFilter::CUSTOM_SLUG); - $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS); - $this->findIfExists = $inputFilter->getValue(ShortUrlInputFilter::FIND_IF_EXISTS); - $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false; - $this->domain = getNonEmptyOptionalValueFromInputFilter($inputFilter, ShortUrlInputFilter::DOMAIN); - $this->shortCodeLength = getOptionalIntFromInputFilter( - $inputFilter, - ShortUrlInputFilter::SHORT_CODE_LENGTH, - ) ?? DEFAULT_SHORT_CODES_LENGTH; - $this->apiKey = $inputFilter->getValue(ShortUrlInputFilter::API_KEY); - $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS); - $this->title = $inputFilter->getValue(ShortUrlInputFilter::TITLE); - $this->crawlable = $inputFilter->getValue(ShortUrlInputFilter::CRAWLABLE); - $this->forwardQuery = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::FORWARD_QUERY) ?? true; + return new self( + longUrl: $inputFilter->getValue(ShortUrlInputFilter::LONG_URL), + deviceLongUrls: map( + $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], + static fn (string $longUrl, string $deviceType) => [DeviceType::from($deviceType), trim($longUrl)], + ), + validSince: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)), + validUntil: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)), + customSlug: $inputFilter->getValue(ShortUrlInputFilter::CUSTOM_SLUG), + maxVisits: getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS), + findIfExists: $inputFilter->getValue(ShortUrlInputFilter::FIND_IF_EXISTS) ?? false, + domain: getNonEmptyOptionalValueFromInputFilter($inputFilter, ShortUrlInputFilter::DOMAIN), + shortCodeLength: getOptionalIntFromInputFilter( + $inputFilter, + ShortUrlInputFilter::SHORT_CODE_LENGTH, + ) ?? DEFAULT_SHORT_CODES_LENGTH, + validateUrl: getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false, + apiKey: $inputFilter->getValue(ShortUrlInputFilter::API_KEY), + tags: $inputFilter->getValue(ShortUrlInputFilter::TAGS), + title: $inputFilter->getValue(ShortUrlInputFilter::TITLE), + crawlable: $inputFilter->getValue(ShortUrlInputFilter::CRAWLABLE), + forwardQuery: getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::FORWARD_QUERY) ?? true, + ); + } + + public function withResolvedTitle(string $title): self + { + return new self( + $this->longUrl, + $this->deviceLongUrls, + $this->validSince, + $this->validUntil, + $this->customSlug, + $this->maxVisits, + $this->findIfExists, + $this->domain, + $this->shortCodeLength, + $this->validateUrl, + $this->apiKey, + $this->tags, + $title, + true, + $this->crawlable, + $this->forwardQuery, + ); } public function getLongUrl(): string @@ -92,115 +113,38 @@ final class ShortUrlCreation implements TitleResolutionModelInterface return $this->longUrl; } - public function getValidSince(): ?Chronos - { - return $this->validSince; - } - public function hasValidSince(): bool { return $this->validSince !== null; } - public function getValidUntil(): ?Chronos - { - return $this->validUntil; - } - public function hasValidUntil(): bool { return $this->validUntil !== null; } - public function getCustomSlug(): ?string - { - return $this->customSlug; - } - public function hasCustomSlug(): bool { return $this->customSlug !== null; } - public function getMaxVisits(): ?int - { - return $this->maxVisits; - } - public function hasMaxVisits(): bool { return $this->maxVisits !== null; } - public function findIfExists(): bool - { - return (bool) $this->findIfExists; - } - public function hasDomain(): bool { return $this->domain !== null; } - public function getDomain(): ?string - { - return $this->domain; - } - - public function getShortCodeLength(): int - { - return $this->shortCodeLength; - } - public function doValidateUrl(): bool { return $this->validateUrl; } - public function getApiKey(): ?ApiKey - { - return $this->apiKey; - } - - /** - * @return string[] - */ - public function getTags(): array - { - return $this->tags; - } - - public function getTitle(): ?string - { - return $this->title; - } - public function hasTitle(): bool { return $this->title !== null; } - - public function titleWasAutoResolved(): bool - { - return $this->titleWasAutoResolved; - } - - public function withResolvedTitle(string $title): self - { - $copy = clone $this; - $copy->title = $title; - $copy->titleWasAutoResolved = true; - - return $copy; - } - - public function isCrawlable(): bool - { - return $this->crawlable; - } - - public function forwardQuery(): bool - { - return $this->forwardQuery; - } } diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index a6c5627f..f31ee294 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -6,14 +6,20 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model\Validation; use DateTime; use Laminas\Filter; -use Laminas\InputFilter\Input; use Laminas\InputFilter\InputFilter; use Laminas\Validator; use Shlinkio\Shlink\Common\Validation; use Shlinkio\Shlink\Core\Config\EnvVars; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function array_keys; +use function array_values; +use function Functional\contains; +use function Functional\every; +use function is_array; use function is_string; +use function Shlinkio\Shlink\Core\enumValues; use function str_replace; use function substr; use function trim; @@ -32,6 +38,7 @@ class ShortUrlInputFilter extends InputFilter public const DOMAIN = 'domain'; public const SHORT_CODE_LENGTH = 'shortCodeLength'; public const LONG_URL = 'longUrl'; + public const DEVICE_LONG_URLS = 'deviceLongUrls'; public const VALIDATE_URL = 'validateUrl'; public const API_KEY = 'apiKey'; public const TAGS = 'tags'; @@ -57,16 +64,40 @@ class ShortUrlInputFilter extends InputFilter private function initialize(bool $requireLongUrl, bool $multiSegmentEnabled): void { - $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); - $longUrlInput->getValidatorChain()->attach(new Validator\NotEmpty([ + $notEmptyValidator = new Validator\NotEmpty([ Validator\NotEmpty::OBJECT, Validator\NotEmpty::SPACE, Validator\NotEmpty::NULL, Validator\NotEmpty::EMPTY_ARRAY, Validator\NotEmpty::BOOLEAN, - ])); + Validator\NotEmpty::STRING, + ]); + + $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); + $longUrlInput->getValidatorChain()->attach($notEmptyValidator); $this->add($longUrlInput); + $deviceLongUrlsInput = $this->createInput(self::DEVICE_LONG_URLS, false); + $deviceLongUrlsInput->getValidatorChain()->attach( + new Validator\Callback(function (mixed $value) use ($notEmptyValidator): bool { + if (! is_array($value)) { + // TODO Set proper error: Not array + return false; + } + + $validValues = enumValues(DeviceType::class); + $keys = array_keys($value); + if (! every($keys, static fn ($key) => contains($validValues, $key))) { + // TODO Set proper error: Provided invalid device type + return false; + } + + $longUrls = array_values($value); + return every($longUrls, $notEmptyValidator->isValid(...)); + }), + ); + $this->add($deviceLongUrlsInput); + $validSince = $this->createInput(self::VALID_SINCE, false); $validSince->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); $this->add($validSince); @@ -75,8 +106,8 @@ class ShortUrlInputFilter extends InputFilter $validUntil->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); $this->add($validUntil); - // FIXME The only way to enforce the NotEmpty validator to be evaluated when the value is provided but it's - // empty, is by using the deprecated setContinueIfEmpty + // The only way to enforce the NotEmpty validator to be evaluated when the key is present with an empty value + // is by using the deprecated setContinueIfEmpty $customSlug = $this->createInput(self::CUSTOM_SLUG, false)->setContinueIfEmpty(true); $customSlug->getFilterChain()->attach(new Filter\Callback(match ($multiSegmentEnabled) { true => static fn (mixed $v) => is_string($v) ? trim(str_replace(' ', '-', $v), '/') : $v, @@ -102,10 +133,8 @@ class ShortUrlInputFilter extends InputFilter $domain->getValidatorChain()->attach(new Validation\HostAndPortValidator()); $this->add($domain); - $apiKeyInput = new Input(self::API_KEY); - $apiKeyInput - ->setRequired(false) - ->getValidatorChain()->attach(new Validator\IsInstanceOf(['className' => ApiKey::class])); + $apiKeyInput = $this->createInput(self::API_KEY, false); + $apiKeyInput->getValidatorChain()->attach(new Validator\IsInstanceOf(['className' => ApiKey::class])); $this->add($apiKeyInput); $this->add($this->createTagsInput(self::TAGS, false)); diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php index 5e95f777..ee2f7389 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php @@ -101,45 +101,45 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $qb; } - public function findOneMatching(ShortUrlCreation $meta): ?ShortUrl + public function findOneMatching(ShortUrlCreation $creation): ?ShortUrl { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->select('s') ->from(ShortUrl::class, 's') ->where($qb->expr()->eq('s.longUrl', ':longUrl')) - ->setParameter('longUrl', $meta->getLongUrl()) + ->setParameter('longUrl', $creation->longUrl) ->setMaxResults(1) ->orderBy('s.id'); - if ($meta->hasCustomSlug()) { + if ($creation->hasCustomSlug()) { $qb->andWhere($qb->expr()->eq('s.shortCode', ':slug')) - ->setParameter('slug', $meta->getCustomSlug()); + ->setParameter('slug', $creation->customSlug); } - if ($meta->hasMaxVisits()) { + if ($creation->hasMaxVisits()) { $qb->andWhere($qb->expr()->eq('s.maxVisits', ':maxVisits')) - ->setParameter('maxVisits', $meta->getMaxVisits()); + ->setParameter('maxVisits', $creation->maxVisits); } - if ($meta->hasValidSince()) { + if ($creation->hasValidSince()) { $qb->andWhere($qb->expr()->eq('s.validSince', ':validSince')) - ->setParameter('validSince', $meta->getValidSince(), ChronosDateTimeType::CHRONOS_DATETIME); + ->setParameter('validSince', $creation->validSince, ChronosDateTimeType::CHRONOS_DATETIME); } - if ($meta->hasValidUntil()) { + if ($creation->hasValidUntil()) { $qb->andWhere($qb->expr()->eq('s.validUntil', ':validUntil')) - ->setParameter('validUntil', $meta->getValidUntil(), ChronosDateTimeType::CHRONOS_DATETIME); + ->setParameter('validUntil', $creation->validUntil, ChronosDateTimeType::CHRONOS_DATETIME); } - if ($meta->hasDomain()) { + if ($creation->hasDomain()) { $qb->join('s.domain', 'd') ->andWhere($qb->expr()->eq('d.authority', ':domain')) - ->setParameter('domain', $meta->getDomain()); + ->setParameter('domain', $creation->domain); } - $apiKey = $meta->getApiKey(); + $apiKey = $creation->apiKey; if ($apiKey !== null) { $this->applySpecification($qb, $apiKey->spec(), 's'); } - $tags = $meta->getTags(); + $tags = $creation->tags; $tagsAmount = count($tags); if ($tagsAmount === 0) { return $qb->getQuery()->getOneOrNullResult(); diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php index cc574ac5..18a4ec71 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php @@ -22,7 +22,7 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specification $spec = null): bool; - public function findOneMatching(ShortUrlCreation $meta): ?ShortUrl; + public function findOneMatching(ShortUrlCreation $creation): ?ShortUrl; public function findOneByImportedUrl(ImportedShlinkUrl $url): ?ShortUrl; } diff --git a/module/Core/src/ShortUrl/UrlShortener.php b/module/Core/src/ShortUrl/UrlShortener.php index d3f54650..4236189c 100644 --- a/module/Core/src/ShortUrl/UrlShortener.php +++ b/module/Core/src/ShortUrl/UrlShortener.php @@ -57,15 +57,15 @@ class UrlShortener implements UrlShortenerInterface return $newShortUrl; } - private function findExistingShortUrlIfExists(ShortUrlCreation $meta): ?ShortUrl + private function findExistingShortUrlIfExists(ShortUrlCreation $creation): ?ShortUrl { - if (! $meta->findIfExists()) { + if (! $creation->findIfExists) { return null; } /** @var ShortUrlRepositoryInterface $repo */ $repo = $this->em->getRepository(ShortUrl::class); - return $repo->findOneMatching($meta); + return $repo->findOneMatching($creation); } private function verifyShortCodeUniqueness(ShortUrlCreation $meta, ShortUrl $shortUrlToBeCreated): void diff --git a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php index 57b3a795..4365731a 100644 --- a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php @@ -75,7 +75,7 @@ class TagRepositoryTest extends DatabaseTestCase [$firstUrlTags] = array_chunk($names, 3); $secondUrlTags = [$names[0]]; $metaWithTags = static fn (array $tags, ?ApiKey $apiKey) => ShortUrlCreation::fromRawData( - ['longUrl' => '', 'tags' => $tags, 'apiKey' => $apiKey], + ['longUrl' => 'longUrl', 'tags' => $tags, 'apiKey' => $apiKey], ); $shortUrl = ShortUrl::create($metaWithTags($firstUrlTags, $apiKey), $this->relationResolver); @@ -242,14 +242,14 @@ class TagRepositoryTest extends DatabaseTestCase [$firstUrlTags, $secondUrlTags] = array_chunk($names, 3); $shortUrl = ShortUrl::create( - ShortUrlCreation::fromRawData(['apiKey' => $authorApiKey, 'longUrl' => '', 'tags' => $firstUrlTags]), + ShortUrlCreation::fromRawData(['apiKey' => $authorApiKey, 'longUrl' => 'longUrl', 'tags' => $firstUrlTags]), $this->relationResolver, ); $this->getEntityManager()->persist($shortUrl); $shortUrl2 = ShortUrl::create( ShortUrlCreation::fromRawData( - ['domain' => $domain->getAuthority(), 'longUrl' => '', 'tags' => $secondUrlTags], + ['domain' => $domain->getAuthority(), 'longUrl' => 'longUrl', 'tags' => $secondUrlTags], ), $this->relationResolver, ); diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 2e509aa2..df4c5334 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -264,7 +264,9 @@ class VisitRepositoryTest extends DatabaseTestCase $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); $this->getEntityManager()->persist($apiKey1); $shortUrl = ShortUrl::create( - ShortUrlCreation::fromRawData(['apiKey' => $apiKey1, 'domain' => $domain->getAuthority(), 'longUrl' => '']), + ShortUrlCreation::fromRawData( + ['apiKey' => $apiKey1, 'domain' => $domain->getAuthority(), 'longUrl' => 'longUrl'], + ), $this->relationResolver, ); $this->getEntityManager()->persist($shortUrl); @@ -272,12 +274,14 @@ class VisitRepositoryTest extends DatabaseTestCase $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); $this->getEntityManager()->persist($apiKey2); - $shortUrl2 = ShortUrl::create(ShortUrlCreation::fromRawData(['apiKey' => $apiKey2, 'longUrl' => ''])); + $shortUrl2 = ShortUrl::create(ShortUrlCreation::fromRawData(['apiKey' => $apiKey2, 'longUrl' => 'longUrl'])); $this->getEntityManager()->persist($shortUrl2); $this->createVisitsForShortUrl($shortUrl2, 5); $shortUrl3 = ShortUrl::create( - ShortUrlCreation::fromRawData(['apiKey' => $apiKey2, 'domain' => $domain->getAuthority(), 'longUrl' => '']), + ShortUrlCreation::fromRawData( + ['apiKey' => $apiKey2, 'domain' => $domain->getAuthority(), 'longUrl' => 'longUrl'], + ), $this->relationResolver, ); $this->getEntityManager()->persist($shortUrl3); @@ -315,7 +319,7 @@ class VisitRepositoryTest extends DatabaseTestCase /** @test */ public function findOrphanVisitsReturnsExpectedResult(): void { - $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => ''])); + $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => 'longUrl'])); $this->getEntityManager()->persist($shortUrl); $this->createVisitsForShortUrl($shortUrl, 7); @@ -364,7 +368,7 @@ class VisitRepositoryTest extends DatabaseTestCase /** @test */ public function countOrphanVisitsReturnsExpectedResult(): void { - $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => ''])); + $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['longUrl' => 'longUrl'])); $this->getEntityManager()->persist($shortUrl); $this->createVisitsForShortUrl($shortUrl, 7); @@ -460,7 +464,7 @@ class VisitRepositoryTest extends DatabaseTestCase } /** - * @return array{string, string, \Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl} + * @return array{string, string, ShortUrl} */ private function createShortUrlsAndVisits( bool|string $withDomain = true, @@ -468,7 +472,7 @@ class VisitRepositoryTest extends DatabaseTestCase ?ApiKey $apiKey = null, ): array { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ - ShortUrlInputFilter::LONG_URL => '', + ShortUrlInputFilter::LONG_URL => 'longUrl', ShortUrlInputFilter::TAGS => $tags, ShortUrlInputFilter::API_KEY => $apiKey, ]), $this->relationResolver); @@ -482,7 +486,7 @@ class VisitRepositoryTest extends DatabaseTestCase $shortUrlWithDomain = ShortUrl::create(ShortUrlCreation::fromRawData([ 'customSlug' => $shortCode, 'domain' => $domain, - 'longUrl' => '', + 'longUrl' => 'longUrl', ])); $this->getEntityManager()->persist($shortUrlWithDomain); $this->createVisitsForShortUrl($shortUrlWithDomain, 3); diff --git a/module/Core/test/EventDispatcher/Mercure/NotifyNewShortUrlToMercureTest.php b/module/Core/test/EventDispatcher/Mercure/NotifyNewShortUrlToMercureTest.php index c42bd915..855f6c14 100644 --- a/module/Core/test/EventDispatcher/Mercure/NotifyNewShortUrlToMercureTest.php +++ b/module/Core/test/EventDispatcher/Mercure/NotifyNewShortUrlToMercureTest.php @@ -57,7 +57,7 @@ class NotifyNewShortUrlToMercureTest extends TestCase /** @test */ public function expectedNotificationIsPublished(): void { - $shortUrl = ShortUrl::withLongUrl(''); + $shortUrl = ShortUrl::withLongUrl('longUrl'); $update = Update::forTopicAndPayload('', []); $this->em->expects($this->once())->method('find')->with(ShortUrl::class, '123')->willReturn($shortUrl); @@ -74,7 +74,7 @@ class NotifyNewShortUrlToMercureTest extends TestCase /** @test */ public function messageIsPrintedIfPublishingFails(): void { - $shortUrl = ShortUrl::withLongUrl(''); + $shortUrl = ShortUrl::withLongUrl('longUrl'); $update = Update::forTopicAndPayload('', []); $e = new Exception('Error'); diff --git a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php index cda8fe98..9611df99 100644 --- a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php +++ b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php @@ -38,7 +38,7 @@ class PublishingUpdatesGeneratorTest extends TestCase { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'customSlug' => 'foo', - 'longUrl' => '', + 'longUrl' => 'longUrl', 'title' => $title, ])); $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); @@ -51,7 +51,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'shortUrl' => [ 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), - 'longUrl' => '', + 'longUrl' => 'longUrl', 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'visitsCount' => 0, 'tags' => [], @@ -118,7 +118,7 @@ class PublishingUpdatesGeneratorTest extends TestCase { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'customSlug' => 'foo', - 'longUrl' => '', + 'longUrl' => 'longUrl', 'title' => 'The title', ])); @@ -128,7 +128,7 @@ class PublishingUpdatesGeneratorTest extends TestCase self::assertEquals(['shortUrl' => [ 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), - 'longUrl' => '', + 'longUrl' => 'longUrl', 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'visitsCount' => 0, 'tags' => [], diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php index 764f7949..52e9630d 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyNewShortUrlToRabbitMqTest.php @@ -68,7 +68,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $shortUrlId = '123'; $update = Update::forTopicAndPayload(Topic::NEW_SHORT_URL->value, []); $this->em->expects($this->once())->method('find')->with(ShortUrl::class, $shortUrlId)->willReturn( - ShortUrl::withLongUrl(''), + ShortUrl::withLongUrl('longUrl'), ); $this->updatesGenerator->expects($this->once())->method('newShortUrlUpdate')->with( $this->isInstanceOf(ShortUrl::class), @@ -88,7 +88,7 @@ class NotifyNewShortUrlToRabbitMqTest extends TestCase $shortUrlId = '123'; $update = Update::forTopicAndPayload(Topic::NEW_SHORT_URL->value, []); $this->em->expects($this->once())->method('find')->with(ShortUrl::class, $shortUrlId)->willReturn( - ShortUrl::withLongUrl(''), + ShortUrl::withLongUrl('longUrl'), ); $this->updatesGenerator->expects($this->once())->method('newShortUrlUpdate')->with( $this->isInstanceOf(ShortUrl::class), diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php index 8b7b392c..6211ad2b 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php @@ -159,7 +159,7 @@ class NotifyVisitToRabbitMqTest extends TestCase { yield 'legacy non-orphan visit' => [ true, - $visit = Visit::forValidShortUrl(ShortUrl::withLongUrl(''), Visitor::emptyInstance()), + $visit = Visit::forValidShortUrl(ShortUrl::withLongUrl('longUrl'), Visitor::emptyInstance()), noop(...), function (MockObject & PublishingHelperInterface $helper) use ($visit): void { $helper->method('publishUpdate')->with($this->callback(function (Update $update) use ($visit): bool { @@ -190,7 +190,7 @@ class NotifyVisitToRabbitMqTest extends TestCase ]; yield 'non-legacy non-orphan visit' => [ false, - Visit::forValidShortUrl(ShortUrl::withLongUrl(''), Visitor::emptyInstance()), + Visit::forValidShortUrl(ShortUrl::withLongUrl('longUrl'), Visitor::emptyInstance()), function (MockObject & PublishingUpdatesGeneratorInterface $updatesGenerator): void { $update = Update::forTopicAndPayload('', []); $updatesGenerator->expects($this->never())->method('newOrphanVisitUpdate'); diff --git a/module/Core/test/EventDispatcher/RedisPubSub/NotifyNewShortUrlToRedisTest.php b/module/Core/test/EventDispatcher/RedisPubSub/NotifyNewShortUrlToRedisTest.php index 0b5dfd27..a913de15 100644 --- a/module/Core/test/EventDispatcher/RedisPubSub/NotifyNewShortUrlToRedisTest.php +++ b/module/Core/test/EventDispatcher/RedisPubSub/NotifyNewShortUrlToRedisTest.php @@ -55,7 +55,7 @@ class NotifyNewShortUrlToRedisTest extends TestCase $shortUrlId = '123'; $update = Update::forTopicAndPayload(Topic::NEW_SHORT_URL->value, []); $this->em->expects($this->once())->method('find')->with(ShortUrl::class, $shortUrlId)->willReturn( - ShortUrl::withLongUrl(''), + ShortUrl::withLongUrl('longUrl'), ); $this->updatesGenerator->expects($this->once())->method('newShortUrlUpdate')->with( $this->isInstanceOf(ShortUrl::class), diff --git a/module/Core/test/Functions/FunctionsTest.php b/module/Core/test/Functions/FunctionsTest.php index 5ba6a7db..ad45812f 100644 --- a/module/Core/test/Functions/FunctionsTest.php +++ b/module/Core/test/Functions/FunctionsTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Functions; +use BackedEnum; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Config\EnvVars; use Shlinkio\Shlink\Core\Model\DeviceType; @@ -16,6 +17,7 @@ use function Shlinkio\Shlink\Core\enumValues; class FunctionsTest extends TestCase { /** + * @param class-string $enum * @test * @dataProvider provideEnums */ diff --git a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php index 026778ae..fd4515fb 100644 --- a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php +++ b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php @@ -38,7 +38,7 @@ class ShortUrlTest extends TestCase public function provideInvalidShortUrls(): iterable { yield 'with custom slug' => [ - ShortUrl::create(ShortUrlCreation::fromRawData(['customSlug' => 'custom-slug', 'longUrl' => ''])), + ShortUrl::create(ShortUrlCreation::fromRawData(['customSlug' => 'custom-slug', 'longUrl' => 'longUrl'])), 'The short code cannot be regenerated on ShortUrls where a custom slug was provided.', ]; yield 'already persisted' => [ @@ -66,7 +66,7 @@ class ShortUrlTest extends TestCase { yield 'no custom slug' => [ShortUrl::createEmpty()]; yield 'imported with custom slug' => [ShortUrl::fromImport( - new ImportedShlinkUrl(ImportSource::BITLY, '', [], Chronos::now(), null, 'custom-slug', null), + new ImportedShlinkUrl(ImportSource::BITLY, 'longUrl', [], Chronos::now(), null, 'custom-slug', null), true, )]; } @@ -78,7 +78,7 @@ class ShortUrlTest extends TestCase public function shortCodesHaveExpectedLength(?int $length, int $expectedLength): void { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData( - [ShortUrlInputFilter::SHORT_CODE_LENGTH => $length, 'longUrl' => ''], + [ShortUrlInputFilter::SHORT_CODE_LENGTH => $length, 'longUrl' => 'longUrl'], )); self::assertEquals($expectedLength, strlen($shortUrl->getShortCode())); diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlStringifierTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlStringifierTest.php index b6d5a123..fc8c7579 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlStringifierTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlStringifierTest.php @@ -30,7 +30,7 @@ class ShortUrlStringifierTest extends TestCase { $shortUrlWithShortCode = fn (string $shortCode, ?string $domain = null) => ShortUrl::create( ShortUrlCreation::fromRawData([ - 'longUrl' => '', + 'longUrl' => 'longUrl', 'customSlug' => $shortCode, 'domain' => $domain, ]), diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php index 835d1487..696a47ab 100644 --- a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -142,7 +142,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase $type->method('isInvalidShortUrl')->willReturn(true); $request = ServerRequestFactory::fromGlobals()->withAttribute(NotFoundType::class, $type) ->withUri(new Uri('https://s.test/shortCode/bar/baz')); - $shortUrl = ShortUrl::withLongUrl(''); + $shortUrl = ShortUrl::withLongUrl('longUrl'); $currentIteration = 1; $this->resolver->expects($this->exactly($expectedResolveCalls))->method('resolveEnabledShortUrl')->with( diff --git a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php index ee9e540a..33380ecf 100644 --- a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php +++ b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php @@ -80,24 +80,24 @@ class ShortUrlCreationTest extends TestCase string $expectedSlug, bool $multiSegmentEnabled = false, ): void { - $meta = ShortUrlCreation::fromRawData([ + $creation = ShortUrlCreation::fromRawData([ 'validSince' => Chronos::parse('2015-01-01')->toAtomString(), 'customSlug' => $customSlug, - 'longUrl' => '', + 'longUrl' => 'longUrl', EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value => $multiSegmentEnabled, ]); - self::assertTrue($meta->hasValidSince()); - self::assertEquals(Chronos::parse('2015-01-01'), $meta->getValidSince()); + self::assertTrue($creation->hasValidSince()); + self::assertEquals(Chronos::parse('2015-01-01'), $creation->validSince); - self::assertFalse($meta->hasValidUntil()); - self::assertNull($meta->getValidUntil()); + self::assertFalse($creation->hasValidUntil()); + self::assertNull($creation->validUntil); - self::assertTrue($meta->hasCustomSlug()); - self::assertEquals($expectedSlug, $meta->getCustomSlug()); + self::assertTrue($creation->hasCustomSlug()); + self::assertEquals($expectedSlug, $creation->customSlug); - self::assertFalse($meta->hasMaxVisits()); - self::assertNull($meta->getMaxVisits()); + self::assertFalse($creation->hasMaxVisits()); + self::assertNull($creation->maxVisits); } public function provideCustomSlugs(): iterable @@ -127,12 +127,12 @@ class ShortUrlCreationTest extends TestCase */ public function titleIsCroppedIfTooLong(?string $title, ?string $expectedTitle): void { - $meta = ShortUrlCreation::fromRawData([ + $creation = ShortUrlCreation::fromRawData([ 'title' => $title, - 'longUrl' => '', + 'longUrl' => 'longUrl', ]); - self::assertEquals($expectedTitle, $meta->getTitle()); + self::assertEquals($expectedTitle, $creation->title); } public function provideTitles(): iterable @@ -153,12 +153,12 @@ class ShortUrlCreationTest extends TestCase */ public function emptyDomainIsDiscarded(?string $domain, ?string $expectedDomain): void { - $meta = ShortUrlCreation::fromRawData([ + $creation = ShortUrlCreation::fromRawData([ 'domain' => $domain, - 'longUrl' => '', + 'longUrl' => 'longUrl', ]); - self::assertSame($expectedDomain, $meta->getDomain()); + self::assertSame($expectedDomain, $creation->domain); } public function provideDomains(): iterable diff --git a/module/Core/test/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/ShortUrl/ShortUrlResolverTest.php index 9c2bcab3..177e432e 100644 --- a/module/Core/test/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/ShortUrl/ShortUrlResolverTest.php @@ -114,7 +114,7 @@ class ShortUrlResolverTest extends TestCase $now = Chronos::now(); yield 'maxVisits reached' => [(function () { - $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['maxVisits' => 3, 'longUrl' => ''])); + $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData(['maxVisits' => 3, 'longUrl' => 'longUrl'])); $shortUrl->setVisits(new ArrayCollection(map( range(0, 4), fn () => Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()), @@ -123,16 +123,16 @@ class ShortUrlResolverTest extends TestCase return $shortUrl; })()]; yield 'future validSince' => [ShortUrl::create(ShortUrlCreation::fromRawData( - ['validSince' => $now->addMonth()->toAtomString(), 'longUrl' => ''], + ['validSince' => $now->addMonth()->toAtomString(), 'longUrl' => 'longUrl'], ))]; yield 'past validUntil' => [ShortUrl::create(ShortUrlCreation::fromRawData( - ['validUntil' => $now->subMonth()->toAtomString(), 'longUrl' => ''], + ['validUntil' => $now->subMonth()->toAtomString(), 'longUrl' => 'longUrl'], ))]; yield 'mixed' => [(function () use ($now) { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'maxVisits' => 3, 'validUntil' => $now->subMonth()->toAtomString(), - 'longUrl' => '', + 'longUrl' => 'longUrl', ])); $shortUrl->setVisits(new ArrayCollection(map( range(0, 4), diff --git a/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php b/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php index c9df4e38..7a97d4da 100644 --- a/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php +++ b/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php @@ -45,7 +45,7 @@ class ShortUrlDataTransformerTest extends TestCase ]]; yield 'max visits only' => [ShortUrl::create(ShortUrlCreation::fromRawData([ 'maxVisits' => $maxVisits, - 'longUrl' => '', + 'longUrl' => 'longUrl', ])), [ 'validSince' => null, 'validUntil' => null, @@ -53,7 +53,7 @@ class ShortUrlDataTransformerTest extends TestCase ]]; yield 'max visits and valid since' => [ ShortUrl::create(ShortUrlCreation::fromRawData( - ['validSince' => $now, 'maxVisits' => $maxVisits, 'longUrl' => ''], + ['validSince' => $now, 'maxVisits' => $maxVisits, 'longUrl' => 'longUrl'], )), [ 'validSince' => $now->toAtomString(), @@ -63,7 +63,7 @@ class ShortUrlDataTransformerTest extends TestCase ]; yield 'both dates' => [ ShortUrl::create(ShortUrlCreation::fromRawData( - ['validSince' => $now, 'validUntil' => $now->subDays(10), 'longUrl' => ''], + ['validSince' => $now, 'validUntil' => $now->subDays(10), 'longUrl' => 'longUrl'], )), [ 'validSince' => $now->toAtomString(), @@ -72,9 +72,12 @@ class ShortUrlDataTransformerTest extends TestCase ], ]; yield 'everything' => [ - ShortUrl::create(ShortUrlCreation::fromRawData( - ['validSince' => $now, 'validUntil' => $now->subDays(5), 'maxVisits' => $maxVisits, 'longUrl' => ''], - )), + ShortUrl::create(ShortUrlCreation::fromRawData([ + 'validSince' => $now, + 'validUntil' => $now->subDays(5), + 'maxVisits' => $maxVisits, + 'longUrl' => 'longUrl', + ])), [ 'validSince' => $now->toAtomString(), 'validUntil' => $now->subDays(5)->toAtomString(), diff --git a/module/Rest/test-api/Action/CreateShortUrlTest.php b/module/Rest/test-api/Action/CreateShortUrlTest.php index 5190000e..19bd6c74 100644 --- a/module/Rest/test-api/Action/CreateShortUrlTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlTest.php @@ -261,9 +261,8 @@ class CreateShortUrlTest extends ApiTestCase public function provideInvalidUrls(): iterable { - yield 'empty URL' => ['', '2', 'INVALID_URL']; - yield 'non-reachable URL' => ['https://this-has-to-be-invalid.com', '2', 'INVALID_URL']; - yield 'API version 3' => ['', '3', 'https://shlink.io/api/error/invalid-url']; + yield 'API version 2' => ['https://this-has-to-be-invalid.com', '2', 'INVALID_URL']; + yield 'API version 3' => ['https://this-has-to-be-invalid.com', '3', 'https://shlink.io/api/error/invalid-url']; } /** From 822652cac3daff23c6710efaa90f474ef0d7542a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 14 Jan 2023 15:44:12 +0100 Subject: [PATCH 03/21] Allow providing device long URLs during short URL edition --- module/Core/src/ShortUrl/Entity/ShortUrl.php | 19 +- .../src/ShortUrl/Model/DeviceLongUrlPair.php | 35 ++++ .../src/ShortUrl/Model/ShortUrlCreation.php | 43 +++-- .../src/ShortUrl/Model/ShortUrlEdition.php | 167 +++++++----------- .../Model/Validation/ShortUrlInputFilter.php | 2 +- .../test/ShortUrl/ShortUrlServiceTest.php | 8 +- 6 files changed, 140 insertions(+), 134 deletions(-) create mode 100644 module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 6c49e1c3..51bd09ea 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -55,6 +55,9 @@ class ShortUrl extends AbstractEntity { } + /** + * @deprecated This should not be allowed + */ public static function createEmpty(): self { return self::create(ShortUrlCreation::createEmpty()); @@ -226,34 +229,34 @@ class ShortUrl extends AbstractEntity ?ShortUrlRelationResolverInterface $relationResolver = null, ): void { if ($shortUrlEdit->validSinceWasProvided()) { - $this->validSince = $shortUrlEdit->validSince(); + $this->validSince = $shortUrlEdit->validSince; } if ($shortUrlEdit->validUntilWasProvided()) { - $this->validUntil = $shortUrlEdit->validUntil(); + $this->validUntil = $shortUrlEdit->validUntil; } if ($shortUrlEdit->maxVisitsWasProvided()) { - $this->maxVisits = $shortUrlEdit->maxVisits(); + $this->maxVisits = $shortUrlEdit->maxVisits; } if ($shortUrlEdit->longUrlWasProvided()) { - $this->longUrl = $shortUrlEdit->longUrl() ?? $this->longUrl; + $this->longUrl = $shortUrlEdit->longUrl ?? $this->longUrl; } if ($shortUrlEdit->tagsWereProvided()) { $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); - $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags()); + $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags); } if ($shortUrlEdit->crawlableWasProvided()) { - $this->crawlable = $shortUrlEdit->crawlable(); + $this->crawlable = $shortUrlEdit->crawlable; } if ( $this->title === null || $shortUrlEdit->titleWasProvided() || ($this->titleWasAutoResolved && $shortUrlEdit->titleWasAutoResolved()) ) { - $this->title = $shortUrlEdit->title(); + $this->title = $shortUrlEdit->title; $this->titleWasAutoResolved = $shortUrlEdit->titleWasAutoResolved(); } if ($shortUrlEdit->forwardQueryWasProvided()) { - $this->forwardQuery = $shortUrlEdit->forwardQuery(); + $this->forwardQuery = $shortUrlEdit->forwardQuery; } } diff --git a/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php b/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php new file mode 100644 index 00000000..6d0234ec --- /dev/null +++ b/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php @@ -0,0 +1,35 @@ + $map + * @return self[] + */ + public static function fromMapToList(array $map): array + { + return array_values(map( + $map, + fn (string $longUrl, string $deviceType) => self::fromRawTypeAndLongUrl($deviceType, $longUrl), + )); + } +} diff --git a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php index e2af5cf1..d63482ec 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php @@ -6,17 +6,14 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; -use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Rest\Entity\ApiKey; -use function Functional\map; use function Shlinkio\Shlink\Core\getNonEmptyOptionalValueFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalBoolFromInputFilter; use function Shlinkio\Shlink\Core\getOptionalIntFromInputFilter; use function Shlinkio\Shlink\Core\normalizeOptionalDate; -use function trim; use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; @@ -24,7 +21,7 @@ final class ShortUrlCreation implements TitleResolutionModelInterface { /** * @param string[] $tags - * @param array{DeviceType, string}[] $deviceLongUrls + * @param DeviceLongUrlPair[] $deviceLongUrls */ private function __construct( public readonly string $longUrl, @@ -46,6 +43,9 @@ final class ShortUrlCreation implements TitleResolutionModelInterface ) { } + /** + * @deprecated This should not be allowed + */ public static function createEmpty(): self { return new self(''); @@ -63,9 +63,8 @@ final class ShortUrlCreation implements TitleResolutionModelInterface return new self( longUrl: $inputFilter->getValue(ShortUrlInputFilter::LONG_URL), - deviceLongUrls: map( + deviceLongUrls: DeviceLongUrlPair::fromMapToList( $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], - static fn (string $longUrl, string $deviceType) => [DeviceType::from($deviceType), trim($longUrl)], ), validSince: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)), validUntil: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)), @@ -89,22 +88,22 @@ final class ShortUrlCreation implements TitleResolutionModelInterface public function withResolvedTitle(string $title): self { return new self( - $this->longUrl, - $this->deviceLongUrls, - $this->validSince, - $this->validUntil, - $this->customSlug, - $this->maxVisits, - $this->findIfExists, - $this->domain, - $this->shortCodeLength, - $this->validateUrl, - $this->apiKey, - $this->tags, - $title, - true, - $this->crawlable, - $this->forwardQuery, + longUrl: $this->longUrl, + deviceLongUrls: $this->deviceLongUrls, + validSince: $this->validSince, + validUntil: $this->validUntil, + customSlug: $this->customSlug, + maxVisits: $this->maxVisits, + findIfExists: $this->findIfExists, + domain: $this->domain, + shortCodeLength: $this->shortCodeLength, + validateUrl: $this->validateUrl, + apiKey: $this->apiKey, + tags: $this->tags, + title: $title, + titleWasAutoResolved: true, + crawlable: $this->crawlable, + forwardQuery: $this->forwardQuery, ); } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php index fadc9b1e..32451d2e 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php @@ -16,77 +16,93 @@ use function Shlinkio\Shlink\Core\normalizeOptionalDate; final class ShortUrlEdition implements TitleResolutionModelInterface { - private bool $longUrlPropWasProvided = false; - private ?string $longUrl = null; - private bool $validSincePropWasProvided = false; - private ?Chronos $validSince = null; - private bool $validUntilPropWasProvided = false; - private ?Chronos $validUntil = null; - private bool $maxVisitsPropWasProvided = false; - private ?int $maxVisits = null; - private bool $tagsPropWasProvided = false; - private array $tags = []; - private bool $titlePropWasProvided = false; - private ?string $title = null; - private bool $titleWasAutoResolved = false; - private bool $validateUrl = false; - private bool $crawlablePropWasProvided = false; - private bool $crawlable = false; - private bool $forwardQueryPropWasProvided = false; - private bool $forwardQuery = true; - - private function __construct() - { + /** + * @param string[] $tags + */ + private function __construct( + private readonly bool $longUrlPropWasProvided = false, + public readonly ?string $longUrl = null, + public readonly array $deviceLongUrls = [], + private readonly bool $validSincePropWasProvided = false, + public readonly ?Chronos $validSince = null, + private readonly bool $validUntilPropWasProvided = false, + public readonly ?Chronos $validUntil = null, + private readonly bool $maxVisitsPropWasProvided = false, + public readonly ?int $maxVisits = null, + private readonly bool $tagsPropWasProvided = false, + public readonly array $tags = [], + private readonly bool $titlePropWasProvided = false, + public readonly ?string $title = null, + public readonly bool $titleWasAutoResolved = false, + public readonly bool $validateUrl = false, + private readonly bool $crawlablePropWasProvided = false, + public readonly bool $crawlable = false, + private readonly bool $forwardQueryPropWasProvided = false, + public readonly bool $forwardQuery = true, + ) { } /** * @throws ValidationException */ public static function fromRawData(array $data): self - { - $instance = new self(); - $instance->validateAndInit($data); - return $instance; - } - - /** - * @throws ValidationException - */ - private function validateAndInit(array $data): void { $inputFilter = ShortUrlInputFilter::withNonRequiredLongUrl($data); if (! $inputFilter->isValid()) { throw ValidationException::fromInputFilter($inputFilter); } - $this->longUrlPropWasProvided = array_key_exists(ShortUrlInputFilter::LONG_URL, $data); - $this->validSincePropWasProvided = array_key_exists(ShortUrlInputFilter::VALID_SINCE, $data); - $this->validUntilPropWasProvided = array_key_exists(ShortUrlInputFilter::VALID_UNTIL, $data); - $this->maxVisitsPropWasProvided = array_key_exists(ShortUrlInputFilter::MAX_VISITS, $data); - $this->tagsPropWasProvided = array_key_exists(ShortUrlInputFilter::TAGS, $data); - $this->titlePropWasProvided = array_key_exists(ShortUrlInputFilter::TITLE, $data); - $this->crawlablePropWasProvided = array_key_exists(ShortUrlInputFilter::CRAWLABLE, $data); - $this->forwardQueryPropWasProvided = array_key_exists(ShortUrlInputFilter::FORWARD_QUERY, $data); - - $this->longUrl = $inputFilter->getValue(ShortUrlInputFilter::LONG_URL); - $this->validSince = normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)); - $this->validUntil = normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)); - $this->maxVisits = getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS); - $this->validateUrl = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false; - $this->tags = $inputFilter->getValue(ShortUrlInputFilter::TAGS); - $this->title = $inputFilter->getValue(ShortUrlInputFilter::TITLE); - $this->crawlable = $inputFilter->getValue(ShortUrlInputFilter::CRAWLABLE); - $this->forwardQuery = getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::FORWARD_QUERY) ?? true; + return new self( + longUrlPropWasProvided: array_key_exists(ShortUrlInputFilter::LONG_URL, $data), + longUrl: $inputFilter->getValue(ShortUrlInputFilter::LONG_URL), + deviceLongUrls: DeviceLongUrlPair::fromMapToList( + $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], + ), + validSincePropWasProvided: array_key_exists(ShortUrlInputFilter::VALID_SINCE, $data), + validSince: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)), + validUntilPropWasProvided: array_key_exists(ShortUrlInputFilter::VALID_UNTIL, $data), + validUntil: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)), + maxVisitsPropWasProvided: array_key_exists(ShortUrlInputFilter::MAX_VISITS, $data), + maxVisits: getOptionalIntFromInputFilter($inputFilter, ShortUrlInputFilter::MAX_VISITS), + tagsPropWasProvided: array_key_exists(ShortUrlInputFilter::TAGS, $data), + tags: $inputFilter->getValue(ShortUrlInputFilter::TAGS), + titlePropWasProvided: array_key_exists(ShortUrlInputFilter::TITLE, $data), + title: $inputFilter->getValue(ShortUrlInputFilter::TITLE), + validateUrl: getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::VALIDATE_URL) ?? false, + crawlablePropWasProvided: array_key_exists(ShortUrlInputFilter::CRAWLABLE, $data), + crawlable: $inputFilter->getValue(ShortUrlInputFilter::CRAWLABLE), + forwardQueryPropWasProvided: array_key_exists(ShortUrlInputFilter::FORWARD_QUERY, $data), + forwardQuery: getOptionalBoolFromInputFilter($inputFilter, ShortUrlInputFilter::FORWARD_QUERY) ?? true, + ); } - public function longUrl(): ?string + public function withResolvedTitle(string $title): self { - return $this->longUrl; + return new self( + longUrlPropWasProvided: $this->longUrlPropWasProvided, + longUrl: $this->longUrl, + validSincePropWasProvided: $this->validSincePropWasProvided, + validSince: $this->validSince, + validUntilPropWasProvided: $this->validUntilPropWasProvided, + validUntil: $this->validUntil, + maxVisitsPropWasProvided: $this->maxVisitsPropWasProvided, + maxVisits: $this->maxVisits, + tagsPropWasProvided: $this->tagsPropWasProvided, + tags: $this->tags, + titlePropWasProvided: $this->titlePropWasProvided, + title: $title, + titleWasAutoResolved: true, + validateUrl: $this->validateUrl, + crawlablePropWasProvided: $this->crawlablePropWasProvided, + crawlable: $this->crawlable, + forwardQueryPropWasProvided: $this->forwardQueryPropWasProvided, + forwardQuery: $this->forwardQuery, + ); } public function getLongUrl(): string { - return $this->longUrl() ?? ''; + return $this->longUrl ?? ''; } public function longUrlWasProvided(): bool @@ -94,54 +110,26 @@ final class ShortUrlEdition implements TitleResolutionModelInterface return $this->longUrlPropWasProvided && $this->longUrl !== null; } - public function validSince(): ?Chronos - { - return $this->validSince; - } - public function validSinceWasProvided(): bool { return $this->validSincePropWasProvided; } - public function validUntil(): ?Chronos - { - return $this->validUntil; - } - public function validUntilWasProvided(): bool { return $this->validUntilPropWasProvided; } - public function maxVisits(): ?int - { - return $this->maxVisits; - } - public function maxVisitsWasProvided(): bool { return $this->maxVisitsPropWasProvided; } - /** - * @return string[] - */ - public function tags(): array - { - return $this->tags; - } - public function tagsWereProvided(): bool { return $this->tagsPropWasProvided; } - public function title(): ?string - { - return $this->title; - } - public function titleWasProvided(): bool { return $this->titlePropWasProvided; @@ -157,35 +145,16 @@ final class ShortUrlEdition implements TitleResolutionModelInterface return $this->titleWasAutoResolved; } - public function withResolvedTitle(string $title): self - { - $copy = clone $this; - $copy->title = $title; - $copy->titleWasAutoResolved = true; - - return $copy; - } - public function doValidateUrl(): bool { return $this->validateUrl; } - public function crawlable(): bool - { - return $this->crawlable; - } - public function crawlableWasProvided(): bool { return $this->crawlablePropWasProvided; } - public function forwardQuery(): bool - { - return $this->forwardQuery; - } - public function forwardQueryWasProvided(): bool { return $this->forwardQueryPropWasProvided; diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index f31ee294..72708250 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -78,7 +78,7 @@ class ShortUrlInputFilter extends InputFilter $this->add($longUrlInput); $deviceLongUrlsInput = $this->createInput(self::DEVICE_LONG_URLS, false); - $deviceLongUrlsInput->getValidatorChain()->attach( + $deviceLongUrlsInput->getValidatorChain()->attach( // TODO Extract callback to own validator new Validator\Callback(function (mixed $value) use ($notEmptyValidator): bool { if (! is_array($value)) { // TODO Set proper error: Not array diff --git a/module/Core/test/ShortUrl/ShortUrlServiceTest.php b/module/Core/test/ShortUrl/ShortUrlServiceTest.php index 9cc0d955..96e0c9f5 100644 --- a/module/Core/test/ShortUrl/ShortUrlServiceTest.php +++ b/module/Core/test/ShortUrl/ShortUrlServiceTest.php @@ -73,10 +73,10 @@ class ShortUrlServiceTest extends TestCase ); self::assertSame($shortUrl, $result); - self::assertEquals($shortUrlEdit->validSince(), $shortUrl->getValidSince()); - self::assertEquals($shortUrlEdit->validUntil(), $shortUrl->getValidUntil()); - self::assertEquals($shortUrlEdit->maxVisits(), $shortUrl->getMaxVisits()); - self::assertEquals($shortUrlEdit->longUrl() ?? $originalLongUrl, $shortUrl->getLongUrl()); + self::assertEquals($shortUrlEdit->validSince, $shortUrl->getValidSince()); + self::assertEquals($shortUrlEdit->validUntil, $shortUrl->getValidUntil()); + self::assertEquals($shortUrlEdit->maxVisits, $shortUrl->getMaxVisits()); + self::assertEquals($shortUrlEdit->longUrl ?? $originalLongUrl, $shortUrl->getLongUrl()); } public function provideShortUrlEdits(): iterable From 3e26f1113d5ca301bc553fd79a0dacb70b6e8b77 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 14 Jan 2023 16:50:42 +0100 Subject: [PATCH 04/21] Extract device long URL validation to its own validation class --- .../src/ShortUrl/Entity/DeviceLongUrl.php | 11 +++- .../Helper/ShortUrlTitleResolutionHelper.php | 9 ++- ...ShortUrlTitleResolutionHelperInterface.php | 3 + .../Helper/TitleResolutionModelInterface.php | 2 +- .../src/ShortUrl/Model/ShortUrlCreation.php | 2 +- .../src/ShortUrl/Model/ShortUrlEdition.php | 2 +- .../Validation/DeviceLongUrlsValidator.php | 57 +++++++++++++++++++ .../Model/Validation/ShortUrlInputFilter.php | 34 ++--------- .../PersistenceShortUrlRelationResolver.php | 2 +- .../ShortUrlRelationResolverInterface.php | 2 +- .../SimpleShortUrlRelationResolver.php | 2 +- module/Core/src/ShortUrl/ShortUrlService.php | 1 - module/Core/src/ShortUrl/UrlShortener.php | 13 ++--- .../src/ShortUrl/UrlShortenerInterface.php | 2 +- module/Rest/src/Entity/ApiKey.php | 2 +- 15 files changed, 96 insertions(+), 48 deletions(-) create mode 100644 module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php index b1dc1086..b3db666d 100644 --- a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -6,16 +6,23 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Entity; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\Model\DeviceType; +use Shlinkio\Shlink\Core\ShortUrl\Model\DeviceLongUrlPair; class DeviceLongUrl extends AbstractEntity { - public function __construct( - public readonly ShortUrl $shortUrl, + private ShortUrl $shortUrl; // @phpstan-ignore-line + + private function __construct( public readonly DeviceType $deviceType, private string $longUrl, ) { } + public static function fromPair(DeviceLongUrlPair $pair): self + { + return new self($pair->deviceType, $pair->longUrl); + } + public function longUrl(): string { return $this->longUrl; diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php index 00eecc61..a4920cdd 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelper.php @@ -4,14 +4,21 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Helper; +use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; class ShortUrlTitleResolutionHelper implements ShortUrlTitleResolutionHelperInterface { - public function __construct(private UrlValidatorInterface $urlValidator) + public function __construct(private readonly UrlValidatorInterface $urlValidator) { } + /** + * @template T of TitleResolutionModelInterface + * @param T $data + * @return T + * @throws InvalidUrlException + */ public function processTitleAndValidateUrl(TitleResolutionModelInterface $data): TitleResolutionModelInterface { if ($data->hasTitle()) { diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php index 50022746..6989140a 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlTitleResolutionHelperInterface.php @@ -9,6 +9,9 @@ use Shlinkio\Shlink\Core\Exception\InvalidUrlException; interface ShortUrlTitleResolutionHelperInterface { /** + * @template T of TitleResolutionModelInterface + * @param T $data + * @return T * @throws InvalidUrlException */ public function processTitleAndValidateUrl(TitleResolutionModelInterface $data): TitleResolutionModelInterface; diff --git a/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php b/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php index 8af28706..1c834331 100644 --- a/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php +++ b/module/Core/src/ShortUrl/Helper/TitleResolutionModelInterface.php @@ -12,5 +12,5 @@ interface TitleResolutionModelInterface public function doValidateUrl(): bool; - public function withResolvedTitle(string $title): self; + public function withResolvedTitle(string $title): static; } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php index d63482ec..f2e156f4 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php @@ -85,7 +85,7 @@ final class ShortUrlCreation implements TitleResolutionModelInterface ); } - public function withResolvedTitle(string $title): self + public function withResolvedTitle(string $title): static { return new self( longUrl: $this->longUrl, diff --git a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php index 32451d2e..fb0f9bb0 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php @@ -76,7 +76,7 @@ final class ShortUrlEdition implements TitleResolutionModelInterface ); } - public function withResolvedTitle(string $title): self + public function withResolvedTitle(string $title): static { return new self( longUrlPropWasProvided: $this->longUrlPropWasProvided, diff --git a/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php b/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php new file mode 100644 index 00000000..1e9d9824 --- /dev/null +++ b/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php @@ -0,0 +1,57 @@ + 'Provided value is not an array.', + self::INVALID_DEVICE => 'You have provided at least one invalid device identifier.', + self::INVALID_LONG_URL => 'At least one of the long URLs are invalid.', + ]; + + public function __construct(private readonly ValidatorChain $longUrlValidators) + { + parent::__construct(); + } + + public function isValid(mixed $value): bool + { + if (! is_array($value)) { + $this->error(self::NOT_ARRAY); + return false; + } + + $validValues = enumValues(DeviceType::class); + $keys = array_keys($value); + if (! every($keys, static fn ($key) => contains($validValues, $key))) { + $this->error(self::INVALID_DEVICE); + return false; + } + + $longUrls = array_values($value); + $result = every($longUrls, $this->longUrlValidators->isValid(...)); + if (! $result) { + $this->error(self::INVALID_LONG_URL); + } + + return $result; + } +} diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index 72708250..7b01841b 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -10,16 +10,9 @@ use Laminas\InputFilter\InputFilter; use Laminas\Validator; use Shlinkio\Shlink\Common\Validation; use Shlinkio\Shlink\Core\Config\EnvVars; -use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Rest\Entity\ApiKey; -use function array_keys; -use function array_values; -use function Functional\contains; -use function Functional\every; -use function is_array; use function is_string; -use function Shlinkio\Shlink\Core\enumValues; use function str_replace; use function substr; use function trim; @@ -64,37 +57,20 @@ class ShortUrlInputFilter extends InputFilter private function initialize(bool $requireLongUrl, bool $multiSegmentEnabled): void { - $notEmptyValidator = new Validator\NotEmpty([ + $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); + $longUrlInput->getValidatorChain()->attach(new Validator\NotEmpty([ Validator\NotEmpty::OBJECT, Validator\NotEmpty::SPACE, Validator\NotEmpty::NULL, Validator\NotEmpty::EMPTY_ARRAY, Validator\NotEmpty::BOOLEAN, Validator\NotEmpty::STRING, - ]); - - $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); - $longUrlInput->getValidatorChain()->attach($notEmptyValidator); + ])); $this->add($longUrlInput); $deviceLongUrlsInput = $this->createInput(self::DEVICE_LONG_URLS, false); - $deviceLongUrlsInput->getValidatorChain()->attach( // TODO Extract callback to own validator - new Validator\Callback(function (mixed $value) use ($notEmptyValidator): bool { - if (! is_array($value)) { - // TODO Set proper error: Not array - return false; - } - - $validValues = enumValues(DeviceType::class); - $keys = array_keys($value); - if (! every($keys, static fn ($key) => contains($validValues, $key))) { - // TODO Set proper error: Provided invalid device type - return false; - } - - $longUrls = array_values($value); - return every($longUrls, $notEmptyValidator->isValid(...)); - }), + $deviceLongUrlsInput->getValidatorChain()->attach( + new DeviceLongUrlsValidator($longUrlInput->getValidatorChain()), ); $this->add($deviceLongUrlsInput); diff --git a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php index 971ef932..db6721d5 100644 --- a/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/PersistenceShortUrlRelationResolver.php @@ -49,7 +49,7 @@ class PersistenceShortUrlRelationResolver implements ShortUrlRelationResolverInt /** * @param string[] $tags - * @return Collection|Tag[] + * @return Collection */ public function resolveTags(array $tags): Collections\Collection { diff --git a/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php index a71f2ccc..b5228214 100644 --- a/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php +++ b/module/Core/src/ShortUrl/Resolver/ShortUrlRelationResolverInterface.php @@ -14,7 +14,7 @@ interface ShortUrlRelationResolverInterface /** * @param string[] $tags - * @return Collection|Tag[] + * @return Collection */ public function resolveTags(array $tags): Collection; } diff --git a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php index f25ff8a1..2048aba9 100644 --- a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php @@ -20,7 +20,7 @@ class SimpleShortUrlRelationResolver implements ShortUrlRelationResolverInterfac /** * @param string[] $tags - * @return Collection|Tag[] + * @return Collection */ public function resolveTags(array $tags): Collections\Collection { diff --git a/module/Core/src/ShortUrl/ShortUrlService.php b/module/Core/src/ShortUrl/ShortUrlService.php index 163989d8..95561fc5 100644 --- a/module/Core/src/ShortUrl/ShortUrlService.php +++ b/module/Core/src/ShortUrl/ShortUrlService.php @@ -34,7 +34,6 @@ class ShortUrlService implements ShortUrlServiceInterface ?ApiKey $apiKey = null, ): ShortUrl { if ($shortUrlEdit->longUrlWasProvided()) { - /** @var ShortUrlEdition $shortUrlEdit */ $shortUrlEdit = $this->titleResolutionHelper->processTitleAndValidateUrl($shortUrlEdit); } diff --git a/module/Core/src/ShortUrl/UrlShortener.php b/module/Core/src/ShortUrl/UrlShortener.php index 4236189c..27e96262 100644 --- a/module/Core/src/ShortUrl/UrlShortener.php +++ b/module/Core/src/ShortUrl/UrlShortener.php @@ -31,22 +31,21 @@ class UrlShortener implements UrlShortenerInterface * @throws NonUniqueSlugException * @throws InvalidUrlException */ - public function shorten(ShortUrlCreation $meta): ShortUrl + public function shorten(ShortUrlCreation $creation): ShortUrl { // First, check if a short URL exists for all provided params - $existingShortUrl = $this->findExistingShortUrlIfExists($meta); + $existingShortUrl = $this->findExistingShortUrlIfExists($creation); if ($existingShortUrl !== null) { return $existingShortUrl; } - /** @var \Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation $meta */ - $meta = $this->titleResolutionHelper->processTitleAndValidateUrl($meta); + $creation = $this->titleResolutionHelper->processTitleAndValidateUrl($creation); /** @var ShortUrl $newShortUrl */ - $newShortUrl = $this->em->wrapInTransaction(function () use ($meta) { - $shortUrl = ShortUrl::create($meta, $this->relationResolver); + $newShortUrl = $this->em->wrapInTransaction(function () use ($creation): ShortUrl { + $shortUrl = ShortUrl::create($creation, $this->relationResolver); - $this->verifyShortCodeUniqueness($meta, $shortUrl); + $this->verifyShortCodeUniqueness($creation, $shortUrl); $this->em->persist($shortUrl); return $shortUrl; diff --git a/module/Core/src/ShortUrl/UrlShortenerInterface.php b/module/Core/src/ShortUrl/UrlShortenerInterface.php index c15b7ebf..d7ae45f0 100644 --- a/module/Core/src/ShortUrl/UrlShortenerInterface.php +++ b/module/Core/src/ShortUrl/UrlShortenerInterface.php @@ -15,5 +15,5 @@ interface UrlShortenerInterface * @throws NonUniqueSlugException * @throws InvalidUrlException */ - public function shorten(ShortUrlCreation $meta): ShortUrl; + public function shorten(ShortUrlCreation $creation): ShortUrl; } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index beb9e0f9..297cdb45 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -21,7 +21,7 @@ class ApiKey extends AbstractEntity private string $key; private ?Chronos $expirationDate = null; private bool $enabled; - /** @var Collection|ApiKeyRole[] */ + /** @var Collection */ private Collection $roles; private ?string $name = null; From fdadf3ba07ccb01f980c017d393043c6e6451105 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 14 Jan 2023 22:37:19 +0100 Subject: [PATCH 05/21] Created unit test for DeviceLongUrlsValidator --- .../DeviceLongUrlsValidatorTest.php | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php diff --git a/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php b/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php new file mode 100644 index 00000000..42ad720b --- /dev/null +++ b/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php @@ -0,0 +1,75 @@ +attach(new NotEmpty()); + + $this->validator = new DeviceLongUrlsValidator($longUrlValidators); + } + + /** + * @test + * @dataProvider provideNonArrayValues + */ + public function nonArrayValuesAreNotValid(mixed $invalidValue): void + { + self::assertFalse($this->validator->isValid($invalidValue)); + self::assertEquals(['NOT_ARRAY' => 'Provided value is not an array.'], $this->validator->getMessages()); + } + + public function provideNonArrayValues(): iterable + { + yield 'int' => [0]; + yield 'float' => [100.45]; + yield 'string' => ['foo']; + yield 'boolean' => [true]; + yield 'object' => [new stdClass()]; + yield 'null' => [null]; + } + + /** @test */ + public function unrecognizedKeysAreNotValid(): void + { + self::assertFalse($this->validator->isValid(['foo' => 'bar'])); + self::assertEquals( + ['INVALID_DEVICE' => 'You have provided at least one invalid device identifier.'], + $this->validator->getMessages(), + ); + } + + /** @test */ + public function everyUrlMustMatchLongUrlValidator(): void + { + self::assertFalse($this->validator->isValid([DeviceType::ANDROID->value => ''])); + self::assertEquals( + ['INVALID_LONG_URL' => 'At least one of the long URLs are invalid.'], + $this->validator->getMessages(), + ); + } + + /** @test */ + public function validValuesResultInValidResult(): void + { + self::assertTrue($this->validator->isValid([ + DeviceType::ANDROID->value => 'foo', + DeviceType::IOS->value => 'bar', + DeviceType::DESKTOP->value => 'baz', + ])); + } +} From a93edf158ea621627f837b4583e51b9cd45aabd6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Jan 2023 13:08:21 +0100 Subject: [PATCH 06/21] Added logic to persist device long URLs while creating/editing a short URL --- ...o.Shlink.Core.ShortUrl.Entity.ShortUrl.php | 5 + module/Core/src/Domain/DomainService.php | 2 +- module/Core/src/Domain/Entity/Domain.php | 4 +- module/Core/src/Domain/Model/DomainItem.php | 2 +- .../src/ShortUrl/Entity/DeviceLongUrl.php | 7 +- module/Core/src/ShortUrl/Entity/ShortUrl.php | 101 +++++++++++------- .../ShortUrl/Helper/ShortUrlStringifier.php | 4 +- .../src/ShortUrl/Model/ShortUrlEdition.php | 1 + .../src/ShortUrl/Model/ShortUrlIdentifier.php | 2 +- .../SimpleShortUrlRelationResolver.php | 3 +- module/Core/src/ShortUrl/UrlShortener.php | 2 +- module/Core/src/Visit/VisitsTracker.php | 9 +- .../Repository/DomainRepositoryTest.php | 2 +- .../Repository/ShortUrlRepositoryTest.php | 10 +- .../Tag/Repository/TagRepositoryTest.php | 2 +- .../Visit/Repository/VisitRepositoryTest.php | 4 +- ...ersistenceShortUrlRelationResolverTest.php | 2 +- .../SimpleShortUrlRelationResolverTest.php | 2 +- .../test/ShortUrl/ShortUrlServiceTest.php | 63 ++++++----- .../Rest/src/ApiKey/Model/RoleDefinition.php | 2 +- module/Rest/src/Entity/ApiKey.php | 7 +- .../ShortUrl/OverrideDomainMiddleware.php | 4 +- 22 files changed, 142 insertions(+), 98 deletions(-) diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php index b67996ae..13aa36f6 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php @@ -67,6 +67,11 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->fetchExtraLazy() ->build(); + $builder->createOneToMany('deviceLongUrls', ShortUrl\Entity\DeviceLongUrl::class) + ->mappedBy('shortUrl') + ->cascadePersist() + ->build(); + $builder->createManyToMany('tags', Tag\Entity\Tag::class) ->setJoinTable(determineTableName('short_urls_in_tags', $emConfig)) ->addInverseJoinColumn('tag_id', 'id', true, false, 'CASCADE') diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 3ecc9f03..29afa110 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -51,7 +51,7 @@ class DomainService implements DomainServiceInterface $repo = $this->em->getRepository(Domain::class); $groups = group( $repo->findDomains($apiKey), - fn (Domain $domain) => $domain->getAuthority() === $this->defaultDomain ? 'default' : 'domains', + fn (Domain $domain) => $domain->authority() === $this->defaultDomain ? 'default' : 'domains', ); return [first($groups['default'] ?? []), $groups['domains'] ?? []]; diff --git a/module/Core/src/Domain/Entity/Domain.php b/module/Core/src/Domain/Entity/Domain.php index ab33ae17..b9b5c334 100644 --- a/module/Core/src/Domain/Entity/Domain.php +++ b/module/Core/src/Domain/Entity/Domain.php @@ -24,14 +24,14 @@ class Domain extends AbstractEntity implements JsonSerializable, NotFoundRedirec return new self($authority); } - public function getAuthority(): string + public function authority(): string { return $this->authority; } public function jsonSerialize(): string { - return $this->getAuthority(); + return $this->authority; } public function invalidShortUrlRedirect(): ?string diff --git a/module/Core/src/Domain/Model/DomainItem.php b/module/Core/src/Domain/Model/DomainItem.php index 72ea3e1f..2a1c7fcf 100644 --- a/module/Core/src/Domain/Model/DomainItem.php +++ b/module/Core/src/Domain/Model/DomainItem.php @@ -20,7 +20,7 @@ final class DomainItem implements JsonSerializable public static function forNonDefaultDomain(Domain $domain): self { - return new self($domain->getAuthority(), $domain, false); + return new self($domain->authority(), $domain, false); } public static function forDefaultDomain(string $defaultDomain, NotFoundRedirectConfigInterface $config): self diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php index b3db666d..668741e8 100644 --- a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -10,17 +10,16 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\DeviceLongUrlPair; class DeviceLongUrl extends AbstractEntity { - private ShortUrl $shortUrl; // @phpstan-ignore-line - private function __construct( + private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine public readonly DeviceType $deviceType, private string $longUrl, ) { } - public static function fromPair(DeviceLongUrlPair $pair): self + public static function fromShortUrlAndPair(ShortUrl $shortUrl, DeviceLongUrlPair $pair): self { - return new self($pair->deviceType, $pair->longUrl); + return new self($shortUrl, $pair->deviceType, $pair->longUrl); } public function longUrl(): string diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 51bd09ea..644c52eb 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -12,6 +12,7 @@ use Doctrine\Common\Collections\Selectable; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; +use Shlinkio\Shlink\Core\ShortUrl\Model\DeviceLongUrlPair; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlEdition; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; @@ -24,6 +25,7 @@ use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function count; +use function Functional\map; use function Shlinkio\Shlink\Core\generateRandomShortCode; use function Shlinkio\Shlink\Core\normalizeDate; use function Shlinkio\Shlink\Core\normalizeOptionalDate; @@ -35,6 +37,8 @@ class ShortUrl extends AbstractEntity private Chronos $dateCreated; /** @var Collection */ private Collection $visits; + /** @var Collection */ + private Collection $deviceLongUrls; /** @var Collection */ private Collection $tags; private ?Chronos $validSince = null; @@ -81,6 +85,10 @@ class ShortUrl extends AbstractEntity $instance->longUrl = $creation->getLongUrl(); $instance->dateCreated = Chronos::now(); $instance->visits = new ArrayCollection(); + $instance->deviceLongUrls = new ArrayCollection(map( + $creation->deviceLongUrls, + fn (DeviceLongUrlPair $pair) => DeviceLongUrl::fromShortUrlAndPair($instance, $pair), + )); $instance->tags = $relationResolver->resolveTags($creation->tags); $instance->validSince = $creation->validSince; $instance->validUntil = $creation->validUntil; @@ -126,6 +134,53 @@ class ShortUrl extends AbstractEntity return $instance; } + public function update( + ShortUrlEdition $shortUrlEdit, + ?ShortUrlRelationResolverInterface $relationResolver = null, + ): void { + if ($shortUrlEdit->validSinceWasProvided()) { + $this->validSince = $shortUrlEdit->validSince; + } + if ($shortUrlEdit->validUntilWasProvided()) { + $this->validUntil = $shortUrlEdit->validUntil; + } + if ($shortUrlEdit->maxVisitsWasProvided()) { + $this->maxVisits = $shortUrlEdit->maxVisits; + } + if ($shortUrlEdit->longUrlWasProvided()) { + $this->longUrl = $shortUrlEdit->longUrl ?? $this->longUrl; + } + if ($shortUrlEdit->tagsWereProvided()) { + $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); + $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags); + } + if ($shortUrlEdit->crawlableWasProvided()) { + $this->crawlable = $shortUrlEdit->crawlable; + } + if ( + $this->title === null + || $shortUrlEdit->titleWasProvided() + || ($this->titleWasAutoResolved && $shortUrlEdit->titleWasAutoResolved()) + ) { + $this->title = $shortUrlEdit->title; + $this->titleWasAutoResolved = $shortUrlEdit->titleWasAutoResolved(); + } + if ($shortUrlEdit->forwardQueryWasProvided()) { + $this->forwardQuery = $shortUrlEdit->forwardQuery; + } + foreach ($shortUrlEdit->deviceLongUrls as $deviceLongUrlPair) { + $deviceLongUrl = $this->deviceLongUrls->findFirst( + fn ($_, DeviceLongUrl $d) => $d->deviceType === $deviceLongUrlPair->deviceType, + ); + + if ($deviceLongUrl !== null) { + $deviceLongUrl->updateLongUrl($deviceLongUrlPair->longUrl); + } else { + $this->deviceLongUrls->add(DeviceLongUrl::fromShortUrlAndPair($this, $deviceLongUrlPair)); + } + } + } + public function getLongUrl(): string { return $this->longUrl; @@ -224,42 +279,6 @@ class ShortUrl extends AbstractEntity return $this->forwardQuery; } - public function update( - ShortUrlEdition $shortUrlEdit, - ?ShortUrlRelationResolverInterface $relationResolver = null, - ): void { - if ($shortUrlEdit->validSinceWasProvided()) { - $this->validSince = $shortUrlEdit->validSince; - } - if ($shortUrlEdit->validUntilWasProvided()) { - $this->validUntil = $shortUrlEdit->validUntil; - } - if ($shortUrlEdit->maxVisitsWasProvided()) { - $this->maxVisits = $shortUrlEdit->maxVisits; - } - if ($shortUrlEdit->longUrlWasProvided()) { - $this->longUrl = $shortUrlEdit->longUrl ?? $this->longUrl; - } - if ($shortUrlEdit->tagsWereProvided()) { - $relationResolver = $relationResolver ?? new SimpleShortUrlRelationResolver(); - $this->tags = $relationResolver->resolveTags($shortUrlEdit->tags); - } - if ($shortUrlEdit->crawlableWasProvided()) { - $this->crawlable = $shortUrlEdit->crawlable; - } - if ( - $this->title === null - || $shortUrlEdit->titleWasProvided() - || ($this->titleWasAutoResolved && $shortUrlEdit->titleWasAutoResolved()) - ) { - $this->title = $shortUrlEdit->title; - $this->titleWasAutoResolved = $shortUrlEdit->titleWasAutoResolved(); - } - if ($shortUrlEdit->forwardQueryWasProvided()) { - $this->forwardQuery = $shortUrlEdit->forwardQuery; - } - } - /** * @throws ShortCodeCannotBeRegeneratedException */ @@ -298,4 +317,14 @@ class ShortUrl extends AbstractEntity return true; } + + public function deviceLongUrls(): array + { + $data = []; + foreach ($this->deviceLongUrls as $deviceUrl) { + $data[$deviceUrl->deviceType->value] = $deviceUrl->longUrl(); + } + + return $data; + } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php b/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php index 719f82b8..795b2490 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php @@ -11,7 +11,7 @@ use function sprintf; class ShortUrlStringifier implements ShortUrlStringifierInterface { - public function __construct(private array $domainConfig, private string $basePath = '') + public function __construct(private readonly array $domainConfig, private readonly string $basePath = '') { } @@ -28,6 +28,6 @@ class ShortUrlStringifier implements ShortUrlStringifierInterface private function resolveDomain(ShortUrl $shortUrl): string { - return $shortUrl->getDomain()?->getAuthority() ?? $this->domainConfig['hostname'] ?? ''; + return $shortUrl->getDomain()?->authority() ?? $this->domainConfig['hostname'] ?? ''; } } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php index fb0f9bb0..25645437 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php @@ -18,6 +18,7 @@ final class ShortUrlEdition implements TitleResolutionModelInterface { /** * @param string[] $tags + * @param DeviceLongUrlPair[] $deviceLongUrls */ private function __construct( private readonly bool $longUrlPropWasProvided = false, diff --git a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php index d7b49c68..fc930de5 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php @@ -45,7 +45,7 @@ final class ShortUrlIdentifier public static function fromShortUrl(ShortUrl $shortUrl): self { $domain = $shortUrl->getDomain(); - $domainAuthority = $domain?->getAuthority(); + $domainAuthority = $domain?->authority(); return new self($shortUrl->getShortCode(), $domainAuthority); } diff --git a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php index 2048aba9..609a300c 100644 --- a/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php +++ b/module/Core/src/ShortUrl/Resolver/SimpleShortUrlRelationResolver.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Resolver; use Doctrine\Common\Collections; -use Doctrine\Common\Collections\Collection; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Core\Tag\Entity\Tag; @@ -20,7 +19,7 @@ class SimpleShortUrlRelationResolver implements ShortUrlRelationResolverInterfac /** * @param string[] $tags - * @return Collection + * @return Collections\Collection */ public function resolveTags(array $tags): Collections\Collection { diff --git a/module/Core/src/ShortUrl/UrlShortener.php b/module/Core/src/ShortUrl/UrlShortener.php index 27e96262..4720809e 100644 --- a/module/Core/src/ShortUrl/UrlShortener.php +++ b/module/Core/src/ShortUrl/UrlShortener.php @@ -76,7 +76,7 @@ class UrlShortener implements UrlShortenerInterface if (! $couldBeMadeUnique) { $domain = $shortUrlToBeCreated->getDomain(); - $domainAuthority = $domain?->getAuthority(); + $domainAuthority = $domain?->authority(); throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index f97fc618..dd5fff91 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -15,9 +15,9 @@ use Shlinkio\Shlink\Core\Visit\Model\Visitor; class VisitsTracker implements VisitsTrackerInterface { public function __construct( - private ORM\EntityManagerInterface $em, - private EventDispatcherInterface $eventDispatcher, - private TrackingOptions $options, + private readonly ORM\EntityManagerInterface $em, + private readonly EventDispatcherInterface $eventDispatcher, + private readonly TrackingOptions $options, ) { } @@ -62,6 +62,9 @@ class VisitsTracker implements VisitsTrackerInterface $this->trackVisit($createVisit, $visitor); } + /** + * @param callable(Visitor $visitor): Visit $createVisit + */ private function trackVisit(callable $createVisit, Visitor $visitor): void { if ($this->options->disableTracking) { diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index c96d70ff..0db35974 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -131,7 +131,7 @@ class DomainRepositoryTest extends DatabaseTestCase { return ShortUrl::create( ShortUrlCreation::fromRawData( - ['domain' => $domain->getAuthority(), 'apiKey' => $apiKey, 'longUrl' => 'foo'], + ['domain' => $domain->authority(), 'apiKey' => $apiKey, 'longUrl' => 'foo'], ), new class ($domain) implements ShortUrlRelationResolverInterface { public function __construct(private Domain $domain) diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index ed500349..01c6c326 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -270,7 +270,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'validSince' => $start, 'apiKey' => $apiKey, - 'domain' => $rightDomain->getAuthority(), + 'domain' => $rightDomain->authority(), 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], ]), $this->relationResolver); @@ -313,7 +313,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->getAuthority(), + 'domain' => $rightDomain->authority(), 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], ])), @@ -322,7 +322,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->getAuthority(), + 'domain' => $rightDomain->authority(), 'apiKey' => $rightDomainApiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], @@ -332,7 +332,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->getAuthority(), + 'domain' => $rightDomain->authority(), 'apiKey' => $apiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], @@ -341,7 +341,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase self::assertNull( $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->getAuthority(), + 'domain' => $rightDomain->authority(), 'apiKey' => $wrongDomainApiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], diff --git a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php index 4365731a..24a8f516 100644 --- a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php @@ -249,7 +249,7 @@ class TagRepositoryTest extends DatabaseTestCase $shortUrl2 = ShortUrl::create( ShortUrlCreation::fromRawData( - ['domain' => $domain->getAuthority(), 'longUrl' => 'longUrl', 'tags' => $secondUrlTags], + ['domain' => $domain->authority(), 'longUrl' => 'longUrl', 'tags' => $secondUrlTags], ), $this->relationResolver, ); diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index df4c5334..01c8e590 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -265,7 +265,7 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($apiKey1); $shortUrl = ShortUrl::create( ShortUrlCreation::fromRawData( - ['apiKey' => $apiKey1, 'domain' => $domain->getAuthority(), 'longUrl' => 'longUrl'], + ['apiKey' => $apiKey1, 'domain' => $domain->authority(), 'longUrl' => 'longUrl'], ), $this->relationResolver, ); @@ -280,7 +280,7 @@ class VisitRepositoryTest extends DatabaseTestCase $shortUrl3 = ShortUrl::create( ShortUrlCreation::fromRawData( - ['apiKey' => $apiKey2, 'domain' => $domain->getAuthority(), 'longUrl' => 'longUrl'], + ['apiKey' => $apiKey2, 'domain' => $domain->authority(), 'longUrl' => 'longUrl'], ), $this->relationResolver, ); diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index 8734b95c..fed61862 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -52,7 +52,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase self::assertSame($result, $foundDomain); } self::assertInstanceOf(Domain::class, $result); - self::assertEquals($authority, $result->getAuthority()); + self::assertEquals($authority, $result->authority()); } public function provideFoundDomains(): iterable diff --git a/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php index f0cc7023..f1925c68 100644 --- a/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php @@ -30,7 +30,7 @@ class SimpleShortUrlRelationResolverTest extends TestCase self::assertNull($result); } else { self::assertInstanceOf(Domain::class, $result); - self::assertEquals($domain, $result->getAuthority()); + self::assertEquals($domain, $result->authority()); } } diff --git a/module/Core/test/ShortUrl/ShortUrlServiceTest.php b/module/Core/test/ShortUrl/ShortUrlServiceTest.php index 96e0c9f5..903c3d3d 100644 --- a/module/Core/test/ShortUrl/ShortUrlServiceTest.php +++ b/module/Core/test/ShortUrl/ShortUrlServiceTest.php @@ -7,7 +7,9 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl; use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\MockObject\Rule\InvocationOrder; use PHPUnit\Framework\TestCase; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlTitleResolutionHelperInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlEdition; @@ -23,21 +25,20 @@ class ShortUrlServiceTest extends TestCase use ApiKeyHelpersTrait; private ShortUrlService $service; - private MockObject & EntityManagerInterface $em; private MockObject & ShortUrlResolverInterface $urlResolver; private MockObject & ShortUrlTitleResolutionHelperInterface $titleResolutionHelper; protected function setUp(): void { - $this->em = $this->createMock(EntityManagerInterface::class); - $this->em->method('persist')->willReturn(null); - $this->em->method('flush')->willReturn(null); + $em = $this->createMock(EntityManagerInterface::class); + $em->method('persist')->willReturn(null); + $em->method('flush')->willReturn(null); $this->urlResolver = $this->createMock(ShortUrlResolverInterface::class); $this->titleResolutionHelper = $this->createMock(ShortUrlTitleResolutionHelperInterface::class); $this->service = new ShortUrlService( - $this->em, + $em, $this->urlResolver, $this->titleResolutionHelper, new SimpleShortUrlRelationResolver(), @@ -49,7 +50,7 @@ class ShortUrlServiceTest extends TestCase * @dataProvider provideShortUrlEdits */ public function updateShortUrlUpdatesProvidedData( - int $expectedValidateCalls, + InvocationOrder $expectedValidateCalls, ShortUrlEdition $shortUrlEdit, ?ApiKey $apiKey, ): void { @@ -61,7 +62,7 @@ class ShortUrlServiceTest extends TestCase $apiKey, )->willReturn($shortUrl); - $this->titleResolutionHelper->expects($this->exactly($expectedValidateCalls)) + $this->titleResolutionHelper->expects($expectedValidateCalls) ->method('processTitleAndValidateUrl') ->with($shortUrlEdit) ->willReturn($shortUrlEdit); @@ -72,34 +73,44 @@ class ShortUrlServiceTest extends TestCase $apiKey, ); + $resolveDeviceLongUrls = function () use ($shortUrlEdit): array { + $result = []; + foreach ($shortUrlEdit->deviceLongUrls ?? [] as $longUrl) { + $result[$longUrl->deviceType->value] = $longUrl->longUrl; + } + + return $result; + }; + self::assertSame($shortUrl, $result); self::assertEquals($shortUrlEdit->validSince, $shortUrl->getValidSince()); self::assertEquals($shortUrlEdit->validUntil, $shortUrl->getValidUntil()); self::assertEquals($shortUrlEdit->maxVisits, $shortUrl->getMaxVisits()); self::assertEquals($shortUrlEdit->longUrl ?? $originalLongUrl, $shortUrl->getLongUrl()); + self::assertEquals($resolveDeviceLongUrls(), $shortUrl->deviceLongUrls()); } public function provideShortUrlEdits(): iterable { - yield 'no long URL' => [0, ShortUrlEdition::fromRawData( - [ - 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), - 'validUntil' => Chronos::parse('2017-01-05 00:00:00')->toAtomString(), - 'maxVisits' => 5, + yield 'no long URL' => [$this->never(), ShortUrlEdition::fromRawData([ + 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), + 'validUntil' => Chronos::parse('2017-01-05 00:00:00')->toAtomString(), + 'maxVisits' => 5, + ]), null]; + yield 'long URL and API key' => [$this->once(), ShortUrlEdition::fromRawData([ + 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), + 'maxVisits' => 10, + 'longUrl' => 'modifiedLongUrl', + ]), ApiKey::create()]; + yield 'long URL with validation' => [$this->once(), ShortUrlEdition::fromRawData([ + 'longUrl' => 'modifiedLongUrl', + 'validateUrl' => true, + ]), null]; + yield 'device redirects' => [$this->never(), ShortUrlEdition::fromRawData([ + 'deviceLongUrls' => [ + DeviceType::IOS->value => 'iosLongUrl', + DeviceType::ANDROID->value => 'androidLongUrl', ], - ), null]; - yield 'long URL' => [1, ShortUrlEdition::fromRawData( - [ - 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), - 'maxVisits' => 10, - 'longUrl' => 'modifiedLongUrl', - ], - ), ApiKey::create()]; - yield 'long URL with validation' => [1, ShortUrlEdition::fromRawData( - [ - 'longUrl' => 'modifiedLongUrl', - 'validateUrl' => true, - ], - ), null]; + ]), null]; } } diff --git a/module/Rest/src/ApiKey/Model/RoleDefinition.php b/module/Rest/src/ApiKey/Model/RoleDefinition.php index 6132772a..bc868f41 100644 --- a/module/Rest/src/ApiKey/Model/RoleDefinition.php +++ b/module/Rest/src/ApiKey/Model/RoleDefinition.php @@ -22,7 +22,7 @@ final class RoleDefinition { return new self( Role::DOMAIN_SPECIFIC, - ['domain_id' => $domain->getId(), 'authority' => $domain->getAuthority()], + ['domain_id' => $domain->getId(), 'authority' => $domain->authority()], ); } } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 297cdb45..57fecdd0 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -147,12 +147,9 @@ class ApiKey extends AbstractEntity $meta = $roleDefinition->meta; if ($this->hasRole($role)) { - /** @var ApiKeyRole $apiKeyRole */ - $apiKeyRole = $this->roles->get($role->value); - $apiKeyRole->updateMeta($meta); + $this->roles->get($role->value)?->updateMeta($meta); } else { - $apiKeyRole = new ApiKeyRole($roleDefinition->role, $roleDefinition->meta, $this); - $this->roles[$role->value] = $apiKeyRole; + $this->roles->set($role->value, new ApiKeyRole($role, $meta, $this)); } } } diff --git a/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php index f4d01e97..ab92c77a 100644 --- a/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php +++ b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php @@ -34,11 +34,11 @@ class OverrideDomainMiddleware implements MiddlewareInterface if ($requestMethod === RequestMethodInterface::METHOD_POST) { /** @var array $payload */ $payload = $request->getParsedBody(); - $payload[ShortUrlInputFilter::DOMAIN] = $domain->getAuthority(); + $payload[ShortUrlInputFilter::DOMAIN] = $domain->authority(); return $handler->handle($request->withParsedBody($payload)); } - return $handler->handle($request->withAttribute(ShortUrlInputFilter::DOMAIN, $domain->getAuthority())); + return $handler->handle($request->withAttribute(ShortUrlInputFilter::DOMAIN, $domain->authority())); } } From d8add9291f0184c29ec6c1b7e082915e4dafce6a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 15 Jan 2023 13:12:17 +0100 Subject: [PATCH 07/21] Removed public readonly prop from entity, as it can cause errors when a proxy is generated --- module/Core/src/ShortUrl/Entity/DeviceLongUrl.php | 7 ++++++- module/Core/src/ShortUrl/Entity/ShortUrl.php | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php index 668741e8..315f7f38 100644 --- a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -12,7 +12,7 @@ class DeviceLongUrl extends AbstractEntity { private function __construct( private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine - public readonly DeviceType $deviceType, + private readonly DeviceType $deviceType, private string $longUrl, ) { } @@ -27,6 +27,11 @@ class DeviceLongUrl extends AbstractEntity return $this->longUrl; } + public function deviceType(): DeviceType + { + return $this->deviceType; + } + public function updateLongUrl(string $longUrl): void { $this->longUrl = $longUrl; diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 644c52eb..3a1b7329 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -170,7 +170,7 @@ class ShortUrl extends AbstractEntity } foreach ($shortUrlEdit->deviceLongUrls as $deviceLongUrlPair) { $deviceLongUrl = $this->deviceLongUrls->findFirst( - fn ($_, DeviceLongUrl $d) => $d->deviceType === $deviceLongUrlPair->deviceType, + fn ($_, DeviceLongUrl $d) => $d->deviceType() === $deviceLongUrlPair->deviceType, ); if ($deviceLongUrl !== null) { @@ -322,7 +322,7 @@ class ShortUrl extends AbstractEntity { $data = []; foreach ($this->deviceLongUrls as $deviceUrl) { - $data[$deviceUrl->deviceType->value] = $deviceUrl->longUrl(); + $data[$deviceUrl->deviceType()->value] = $deviceUrl->longUrl(); } return $data; From c1b7c6ba6c4bfaf8cf36c1e0d088501245179c73 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 16 Jan 2023 23:41:14 +0100 Subject: [PATCH 08/21] Updated to shlink-common with support for proxies for entities with public readonly props --- composer.json | 2 +- module/Core/src/Domain/DomainService.php | 2 +- module/Core/src/Domain/Entity/Domain.php | 7 +------ module/Core/src/Domain/Model/DomainItem.php | 2 +- .../Core/src/ShortUrl/Helper/ShortUrlStringifier.php | 2 +- module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php | 2 +- module/Core/src/ShortUrl/UrlShortener.php | 2 +- .../test-db/Domain/Repository/DomainRepositoryTest.php | 2 +- .../ShortUrl/Repository/ShortUrlRepositoryTest.php | 10 +++++----- .../Core/test-db/Tag/Repository/TagRepositoryTest.php | 2 +- .../test-db/Visit/Repository/VisitRepositoryTest.php | 4 ++-- .../PersistenceShortUrlRelationResolverTest.php | 2 +- .../Resolver/SimpleShortUrlRelationResolverTest.php | 2 +- module/Rest/src/ApiKey/Model/RoleDefinition.php | 2 +- .../Middleware/ShortUrl/OverrideDomainMiddleware.php | 4 ++-- 15 files changed, 21 insertions(+), 26 deletions(-) diff --git a/composer.json b/composer.json index 1ad2e7ab..d7741611 100644 --- a/composer.json +++ b/composer.json @@ -46,7 +46,7 @@ "php-middleware/request-id": "^4.1", "pugx/shortid-php": "^1.1", "ramsey/uuid": "^4.5", - "shlinkio/shlink-common": "^5.2", + "shlinkio/shlink-common": "dev-main#61d26e7 as 5.3", "shlinkio/shlink-config": "dev-main#2a5b5c2 as 2.4", "shlinkio/shlink-event-dispatcher": "^2.6", "shlinkio/shlink-importer": "^5.0", diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 29afa110..703f77fd 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -51,7 +51,7 @@ class DomainService implements DomainServiceInterface $repo = $this->em->getRepository(Domain::class); $groups = group( $repo->findDomains($apiKey), - fn (Domain $domain) => $domain->authority() === $this->defaultDomain ? 'default' : 'domains', + fn (Domain $domain) => $domain->authority === $this->defaultDomain ? 'default' : 'domains', ); return [first($groups['default'] ?? []), $groups['domains'] ?? []]; diff --git a/module/Core/src/Domain/Entity/Domain.php b/module/Core/src/Domain/Entity/Domain.php index b9b5c334..4e6ea865 100644 --- a/module/Core/src/Domain/Entity/Domain.php +++ b/module/Core/src/Domain/Entity/Domain.php @@ -15,7 +15,7 @@ class Domain extends AbstractEntity implements JsonSerializable, NotFoundRedirec private ?string $regular404Redirect = null; private ?string $invalidShortUrlRedirect = null; - private function __construct(private string $authority) + private function __construct(public readonly string $authority) { } @@ -24,11 +24,6 @@ class Domain extends AbstractEntity implements JsonSerializable, NotFoundRedirec return new self($authority); } - public function authority(): string - { - return $this->authority; - } - public function jsonSerialize(): string { return $this->authority; diff --git a/module/Core/src/Domain/Model/DomainItem.php b/module/Core/src/Domain/Model/DomainItem.php index 2a1c7fcf..53f2b6f7 100644 --- a/module/Core/src/Domain/Model/DomainItem.php +++ b/module/Core/src/Domain/Model/DomainItem.php @@ -20,7 +20,7 @@ final class DomainItem implements JsonSerializable public static function forNonDefaultDomain(Domain $domain): self { - return new self($domain->authority(), $domain, false); + return new self($domain->authority, $domain, false); } public static function forDefaultDomain(string $defaultDomain, NotFoundRedirectConfigInterface $config): self diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php b/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php index 795b2490..9d21cb58 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlStringifier.php @@ -28,6 +28,6 @@ class ShortUrlStringifier implements ShortUrlStringifierInterface private function resolveDomain(ShortUrl $shortUrl): string { - return $shortUrl->getDomain()?->authority() ?? $this->domainConfig['hostname'] ?? ''; + return $shortUrl->getDomain()?->authority ?? $this->domainConfig['hostname'] ?? ''; } } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php index fc930de5..bb3b4af6 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php @@ -45,7 +45,7 @@ final class ShortUrlIdentifier public static function fromShortUrl(ShortUrl $shortUrl): self { $domain = $shortUrl->getDomain(); - $domainAuthority = $domain?->authority(); + $domainAuthority = $domain?->authority; return new self($shortUrl->getShortCode(), $domainAuthority); } diff --git a/module/Core/src/ShortUrl/UrlShortener.php b/module/Core/src/ShortUrl/UrlShortener.php index 4720809e..7477052f 100644 --- a/module/Core/src/ShortUrl/UrlShortener.php +++ b/module/Core/src/ShortUrl/UrlShortener.php @@ -76,7 +76,7 @@ class UrlShortener implements UrlShortenerInterface if (! $couldBeMadeUnique) { $domain = $shortUrlToBeCreated->getDomain(); - $domainAuthority = $domain?->authority(); + $domainAuthority = $domain?->authority; throw NonUniqueSlugException::fromSlug($shortUrlToBeCreated->getShortCode(), $domainAuthority); } diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index 0db35974..2b005947 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -131,7 +131,7 @@ class DomainRepositoryTest extends DatabaseTestCase { return ShortUrl::create( ShortUrlCreation::fromRawData( - ['domain' => $domain->authority(), 'apiKey' => $apiKey, 'longUrl' => 'foo'], + ['domain' => $domain->authority, 'apiKey' => $apiKey, 'longUrl' => 'foo'], ), new class ($domain) implements ShortUrlRelationResolverInterface { public function __construct(private Domain $domain) diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index 01c6c326..99e3add9 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -270,7 +270,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'validSince' => $start, 'apiKey' => $apiKey, - 'domain' => $rightDomain->authority(), + 'domain' => $rightDomain->authority, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], ]), $this->relationResolver); @@ -313,7 +313,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->authority(), + 'domain' => $rightDomain->authority, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], ])), @@ -322,7 +322,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->authority(), + 'domain' => $rightDomain->authority, 'apiKey' => $rightDomainApiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], @@ -332,7 +332,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $shortUrl, $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->authority(), + 'domain' => $rightDomain->authority, 'apiKey' => $apiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], @@ -341,7 +341,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase self::assertNull( $this->repo->findOneMatching(ShortUrlCreation::fromRawData([ 'validSince' => $start, - 'domain' => $rightDomain->authority(), + 'domain' => $rightDomain->authority, 'apiKey' => $wrongDomainApiKey, 'longUrl' => 'foo', 'tags' => ['foo', 'bar'], diff --git a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php index 24a8f516..97873b20 100644 --- a/module/Core/test-db/Tag/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Tag/Repository/TagRepositoryTest.php @@ -249,7 +249,7 @@ class TagRepositoryTest extends DatabaseTestCase $shortUrl2 = ShortUrl::create( ShortUrlCreation::fromRawData( - ['domain' => $domain->authority(), 'longUrl' => 'longUrl', 'tags' => $secondUrlTags], + ['domain' => $domain->authority, 'longUrl' => 'longUrl', 'tags' => $secondUrlTags], ), $this->relationResolver, ); diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 01c8e590..f1fed415 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -265,7 +265,7 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($apiKey1); $shortUrl = ShortUrl::create( ShortUrlCreation::fromRawData( - ['apiKey' => $apiKey1, 'domain' => $domain->authority(), 'longUrl' => 'longUrl'], + ['apiKey' => $apiKey1, 'domain' => $domain->authority, 'longUrl' => 'longUrl'], ), $this->relationResolver, ); @@ -280,7 +280,7 @@ class VisitRepositoryTest extends DatabaseTestCase $shortUrl3 = ShortUrl::create( ShortUrlCreation::fromRawData( - ['apiKey' => $apiKey2, 'domain' => $domain->authority(), 'longUrl' => 'longUrl'], + ['apiKey' => $apiKey2, 'domain' => $domain->authority, 'longUrl' => 'longUrl'], ), $this->relationResolver, ); diff --git a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php index fed61862..fedfd96f 100644 --- a/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/PersistenceShortUrlRelationResolverTest.php @@ -52,7 +52,7 @@ class PersistenceShortUrlRelationResolverTest extends TestCase self::assertSame($result, $foundDomain); } self::assertInstanceOf(Domain::class, $result); - self::assertEquals($authority, $result->authority()); + self::assertEquals($authority, $result->authority); } public function provideFoundDomains(): iterable diff --git a/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php index f1925c68..443710bb 100644 --- a/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php +++ b/module/Core/test/ShortUrl/Resolver/SimpleShortUrlRelationResolverTest.php @@ -30,7 +30,7 @@ class SimpleShortUrlRelationResolverTest extends TestCase self::assertNull($result); } else { self::assertInstanceOf(Domain::class, $result); - self::assertEquals($domain, $result->authority()); + self::assertEquals($domain, $result->authority); } } diff --git a/module/Rest/src/ApiKey/Model/RoleDefinition.php b/module/Rest/src/ApiKey/Model/RoleDefinition.php index bc868f41..403e6214 100644 --- a/module/Rest/src/ApiKey/Model/RoleDefinition.php +++ b/module/Rest/src/ApiKey/Model/RoleDefinition.php @@ -22,7 +22,7 @@ final class RoleDefinition { return new self( Role::DOMAIN_SPECIFIC, - ['domain_id' => $domain->getId(), 'authority' => $domain->authority()], + ['domain_id' => $domain->getId(), 'authority' => $domain->authority], ); } } diff --git a/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php index ab92c77a..8a88e340 100644 --- a/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php +++ b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php @@ -34,11 +34,11 @@ class OverrideDomainMiddleware implements MiddlewareInterface if ($requestMethod === RequestMethodInterface::METHOD_POST) { /** @var array $payload */ $payload = $request->getParsedBody(); - $payload[ShortUrlInputFilter::DOMAIN] = $domain->authority(); + $payload[ShortUrlInputFilter::DOMAIN] = $domain->authority; return $handler->handle($request->withParsedBody($payload)); } - return $handler->handle($request->withAttribute(ShortUrlInputFilter::DOMAIN, $domain->authority())); + return $handler->handle($request->withAttribute(ShortUrlInputFilter::DOMAIN, $domain->authority)); } } From 237fb95b4b732a097ec26d6c566f9edd802e548d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 21 Jan 2023 10:37:12 +0100 Subject: [PATCH 09/21] Update ShortUrlRedirectionBuilder to accept a request object instead of a raw query array --- module/Core/src/Action/RedirectAction.php | 6 +++--- .../ShortUrl/Helper/ShortUrlRedirectionBuilder.php | 11 ++++++++--- .../Helper/ShortUrlRedirectionBuilderInterface.php | 7 ++++++- .../Middleware/ExtraPathRedirectMiddleware.php | 3 +-- .../Helper/ShortUrlRedirectionBuilderTest.php | 7 ++++++- .../Middleware/ExtraPathRedirectMiddlewareTest.php | 2 +- module/Rest/test-api/Fixtures/ShortUrlsFixture.php | 5 +++++ 7 files changed, 30 insertions(+), 11 deletions(-) diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 725e402d..942cf550 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -18,15 +18,15 @@ class RedirectAction extends AbstractTrackingAction implements StatusCodeInterfa public function __construct( ShortUrlResolverInterface $urlResolver, RequestTrackerInterface $requestTracker, - private ShortUrlRedirectionBuilderInterface $redirectionBuilder, - private RedirectResponseHelperInterface $redirectResponseHelper, + private readonly ShortUrlRedirectionBuilderInterface $redirectionBuilder, + private readonly RedirectResponseHelperInterface $redirectResponseHelper, ) { parent::__construct($urlResolver, $requestTracker); } protected function createSuccessResp(ShortUrl $shortUrl, ServerRequestInterface $request): Response { - $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request->getQueryParams()); + $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index f003318d..4f457659 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Helper; use GuzzleHttp\Psr7\Query; use Laminas\Stdlib\ArrayUtils; use League\Uri\Uri; +use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; @@ -14,12 +15,16 @@ use function sprintf; class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderInterface { - public function __construct(private TrackingOptions $trackingOptions) + public function __construct(private readonly TrackingOptions $trackingOptions) { } - public function buildShortUrlRedirect(ShortUrl $shortUrl, array $currentQuery, ?string $extraPath = null): string - { + public function buildShortUrlRedirect( + ShortUrl $shortUrl, + ServerRequestInterface $request, + ?string $extraPath = null, + ): string { + $currentQuery = $request->getQueryParams(); $uri = Uri::createFromString($shortUrl->getLongUrl()); $shouldForwardQuery = $shortUrl->forwardQuery(); diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php index 44bd9ccb..7f79e98a 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php @@ -4,9 +4,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Helper; +use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; interface ShortUrlRedirectionBuilderInterface { - public function buildShortUrlRedirect(ShortUrl $shortUrl, array $currentQuery, ?string $extraPath = null): string; + public function buildShortUrlRedirect( + ShortUrl $shortUrl, + ServerRequestInterface $request, + ?string $extraPath = null, + ): string; } diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index 66105779..c8f96bba 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -68,7 +68,6 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface int $shortCodeSegments = 1, ): ResponseInterface { $uri = $request->getUri(); - $query = $request->getQueryParams(); [$potentialShortCode, $extraPath] = $this->resolvePotentialShortCodeAndExtraPath($uri, $shortCodeSegments); $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($potentialShortCode, $uri->getAuthority()); @@ -76,7 +75,7 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier); $this->requestTracker->trackIfApplicable($shortUrl, $request); - $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); + $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } catch (ShortUrlNotFoundException) { if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled) { diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index cb94a9f1..342ba1ff 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; +use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; @@ -34,7 +35,11 @@ class ShortUrlRedirectionBuilderTest extends TestCase 'longUrl' => 'https://domain.com/foo/bar?some=thing', 'forwardQuery' => $forwardQuery, ])); - $result = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); + $result = $this->redirectionBuilder->buildShortUrlRedirect( + $shortUrl, + ServerRequestFactory::fromGlobals()->withQueryParams($query), + $extraPath, + ); self::assertEquals($expectedUrl, $result); } diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php index 696a47ab..c157403e 100644 --- a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -159,7 +159,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase ); $this->redirectionBuilder->expects($this->once())->method('buildShortUrlRedirect')->with( $shortUrl, - [], + $this->isInstanceOf(ServerRequestInterface::class), $expectedExtraPath, )->willReturn('the_built_long_url'); $this->redirectResponseHelper->expects($this->once())->method('buildRedirectResponse')->with( diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index 9a876463..2d45a7bb 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -9,6 +9,7 @@ use Doctrine\Common\DataFixtures\AbstractFixture; use Doctrine\Common\DataFixtures\DependentFixtureInterface; use Doctrine\Persistence\ObjectManager; use ReflectionObject; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; @@ -48,6 +49,10 @@ class ShortUrlsFixture extends AbstractFixture implements DependentFixtureInterf 'apiKey' => $authorApiKey, 'longUrl' => 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', + 'deviceLongUrls' => [ + DeviceType::ANDROID->value => 'https://blog.alejandrocelaya.com/android', + DeviceType::IOS->value => 'https://blog.alejandrocelaya.com/ios', + ], 'tags' => ['foo', 'bar'], ]), $relationResolver), '2019-01-01 00:00:10'); $manager->persist($defShortUrl); From b1b67c497ebe997edc34977e09e31d303ccf1d46 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 21 Jan 2023 11:15:38 +0100 Subject: [PATCH 10/21] Add logic to dynamically resolve the long URL to redirect to based on requesting device --- composer.json | 5 +- config/test/constants.php | 7 ++ .../src/ShortUrl/Entity/DeviceLongUrl.php | 7 +- module/Core/src/ShortUrl/Entity/ShortUrl.php | 14 +++- .../Helper/ShortUrlRedirectionBuilder.php | 4 +- module/Core/test-api/Action/RedirectTest.php | 38 ++++++++++ .../Helper/ShortUrlRedirectionBuilderTest.php | 70 ++++++++++++------- 7 files changed, 109 insertions(+), 36 deletions(-) create mode 100644 module/Core/test-api/Action/RedirectTest.php diff --git a/composer.json b/composer.json index d7741611..6a279051 100644 --- a/composer.json +++ b/composer.json @@ -74,7 +74,7 @@ "phpunit/phpunit": "^9.5", "roave/security-advisories": "dev-master", "shlinkio/php-coding-standard": "~2.3.0", - "shlinkio/shlink-test-utils": "^3.3", + "shlinkio/shlink-test-utils": "^3.4", "symfony/var-dumper": "^6.1", "veewee/composer-run-parallel": "^1.1" }, @@ -97,7 +97,8 @@ "ShlinkioApiTest\\Shlink\\Rest\\": "module/Rest/test-api", "ShlinkioDbTest\\Shlink\\Rest\\": "module/Rest/test-db", "ShlinkioTest\\Shlink\\Core\\": "module/Core/test", - "ShlinkioDbTest\\Shlink\\Core\\": "module/Core/test-db" + "ShlinkioDbTest\\Shlink\\Core\\": "module/Core/test-db", + "ShlinkioApiTest\\Shlink\\Core\\": "module/Core/test-api" }, "files": [ "config/test/constants.php" diff --git a/config/test/constants.php b/config/test/constants.php index c767abc9..bce232f3 100644 --- a/config/test/constants.php +++ b/config/test/constants.php @@ -6,3 +6,10 @@ namespace ShlinkioTest\Shlink; const API_TESTS_HOST = '127.0.0.1'; const API_TESTS_PORT = 9999; + +const ANDROID_USER_AGENT = 'Mozilla/5.0 (Linux; Android 13) AppleWebKit/537.36 (KHTML, like Gecko) ' + . 'Chrome/109.0.5414.86 Mobile Safari/537.36'; +const IOS_USER_AGENT = 'Mozilla/5.0 (iPhone; CPU iPhone OS 16_2 like Mac OS X) AppleWebKit/605.1.15 ' + . '(KHTML, like Gecko) FxiOS/109.0 Mobile/15E148 Safari/605.1.15'; +const DESKTOP_USER_AGENT = 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like ' + . 'Gecko) Chrome/109.0.0.0 Safari/537.36 Edg/109.0.1518.61'; diff --git a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php index 315f7f38..668741e8 100644 --- a/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php +++ b/module/Core/src/ShortUrl/Entity/DeviceLongUrl.php @@ -12,7 +12,7 @@ class DeviceLongUrl extends AbstractEntity { private function __construct( private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine - private readonly DeviceType $deviceType, + public readonly DeviceType $deviceType, private string $longUrl, ) { } @@ -27,11 +27,6 @@ class DeviceLongUrl extends AbstractEntity return $this->longUrl; } - public function deviceType(): DeviceType - { - return $this->deviceType; - } - public function updateLongUrl(string $longUrl): void { $this->longUrl = $longUrl; diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 3a1b7329..9063bdd7 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -12,6 +12,7 @@ use Doctrine\Common\Collections\Selectable; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Model\DeviceLongUrlPair; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlEdition; @@ -170,7 +171,7 @@ class ShortUrl extends AbstractEntity } foreach ($shortUrlEdit->deviceLongUrls as $deviceLongUrlPair) { $deviceLongUrl = $this->deviceLongUrls->findFirst( - fn ($_, DeviceLongUrl $d) => $d->deviceType() === $deviceLongUrlPair->deviceType, + fn ($_, DeviceLongUrl $d) => $d->deviceType === $deviceLongUrlPair->deviceType, ); if ($deviceLongUrl !== null) { @@ -186,6 +187,15 @@ class ShortUrl extends AbstractEntity return $this->longUrl; } + public function longUrlForDevice(?DeviceType $deviceType): string + { + $deviceLongUrl = $this->deviceLongUrls->findFirst( + static fn ($_, DeviceLongUrl $longUrl) => $longUrl->deviceType === $deviceType, + ); + + return $deviceLongUrl?->longUrl() ?? $this->longUrl; + } + public function getShortCode(): string { return $this->shortCode; @@ -322,7 +332,7 @@ class ShortUrl extends AbstractEntity { $data = []; foreach ($this->deviceLongUrls as $deviceUrl) { - $data[$deviceUrl->deviceType()->value] = $deviceUrl->longUrl(); + $data[$deviceUrl->deviceType->value] = $deviceUrl->longUrl(); } return $data; diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 4f457659..c322f195 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -8,6 +8,7 @@ use GuzzleHttp\Psr7\Query; use Laminas\Stdlib\ArrayUtils; use League\Uri\Uri; use Psr\Http\Message\ServerRequestInterface; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; @@ -25,7 +26,8 @@ class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderInterface ?string $extraPath = null, ): string { $currentQuery = $request->getQueryParams(); - $uri = Uri::createFromString($shortUrl->getLongUrl()); + $device = DeviceType::matchFromUserAgent($request->getHeaderLine('User-Agent')); + $uri = Uri::createFromString($shortUrl->longUrlForDevice($device)); $shouldForwardQuery = $shortUrl->forwardQuery(); return $uri diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php new file mode 100644 index 00000000..73b6a1cc --- /dev/null +++ b/module/Core/test-api/Action/RedirectTest.php @@ -0,0 +1,38 @@ +callShortUrl('def456', $userAgent); + self::assertEquals($expectedRedirect, $response->getHeaderLine('Location')); + } + + public function provideUserAgents(): iterable + { + yield 'android' => [ANDROID_USER_AGENT, 'https://blog.alejandrocelaya.com/android']; + yield 'ios' => [IOS_USER_AGENT, 'https://blog.alejandrocelaya.com/ios']; + yield 'desktop' => [ + DESKTOP_USER_AGENT, + 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', + ]; + yield 'unknown' => [ + null, + 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', + ]; + } +} diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index 342ba1ff..341ff6bf 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -6,11 +6,17 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\TestCase; +use Psr\Http\Message\ServerRequestInterface; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilder; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; +use const ShlinkioTest\Shlink\ANDROID_USER_AGENT; +use const ShlinkioTest\Shlink\DESKTOP_USER_AGENT; +use const ShlinkioTest\Shlink\IOS_USER_AGENT; + class ShortUrlRedirectionBuilderTest extends TestCase { private ShortUrlRedirectionBuilder $redirectionBuilder; @@ -27,78 +33,92 @@ class ShortUrlRedirectionBuilderTest extends TestCase */ public function buildShortUrlRedirectBuildsExpectedUrl( string $expectedUrl, - array $query, + ServerRequestInterface $request, ?string $extraPath, ?bool $forwardQuery, ): void { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'longUrl' => 'https://domain.com/foo/bar?some=thing', 'forwardQuery' => $forwardQuery, + 'deviceLongUrls' => [ + DeviceType::ANDROID->value => 'https://domain.com/android', + DeviceType::IOS->value => 'https://domain.com/ios', + ], ])); - $result = $this->redirectionBuilder->buildShortUrlRedirect( - $shortUrl, - ServerRequestFactory::fromGlobals()->withQueryParams($query), - $extraPath, - ); + $result = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath); self::assertEquals($expectedUrl, $result); } public function provideData(): iterable { - yield ['https://domain.com/foo/bar?some=thing', [], null, true]; - yield ['https://domain.com/foo/bar?some=thing', [], null, null]; - yield ['https://domain.com/foo/bar?some=thing', [], null, false]; - yield ['https://domain.com/foo/bar?some=thing&else', ['else' => null], null, true]; - yield ['https://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar'], null, true]; - yield ['https://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar'], null, null]; - yield ['https://domain.com/foo/bar?some=thing', ['foo' => 'bar'], null, false]; - yield ['https://domain.com/foo/bar?some=thing&123=foo', ['123' => 'foo'], null, true]; - yield ['https://domain.com/foo/bar?some=thing&456=foo', [456 => 'foo'], null, true]; - yield ['https://domain.com/foo/bar?some=thing&456=foo', [456 => 'foo'], null, null]; - yield ['https://domain.com/foo/bar?some=thing', [456 => 'foo'], null, false]; + $request = static fn (array $query = []) => ServerRequestFactory::fromGlobals()->withQueryParams($query); + + yield ['https://domain.com/foo/bar?some=thing', $request(), null, true]; + yield ['https://domain.com/foo/bar?some=thing', $request(), null, null]; + yield ['https://domain.com/foo/bar?some=thing', $request(), null, false]; + yield ['https://domain.com/foo/bar?some=thing&else', $request(['else' => null]), null, true]; + yield ['https://domain.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, true]; + yield ['https://domain.com/foo/bar?some=thing&foo=bar', $request(['foo' => 'bar']), null, null]; + yield ['https://domain.com/foo/bar?some=thing', $request(['foo' => 'bar']), null, false]; + yield ['https://domain.com/foo/bar?some=thing&123=foo', $request(['123' => 'foo']), null, true]; + yield ['https://domain.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, true]; + yield ['https://domain.com/foo/bar?some=thing&456=foo', $request([456 => 'foo']), null, null]; + yield ['https://domain.com/foo/bar?some=thing', $request([456 => 'foo']), null, false]; yield [ 'https://domain.com/foo/bar?some=overwritten&foo=bar', - ['foo' => 'bar', 'some' => 'overwritten'], + $request(['foo' => 'bar', 'some' => 'overwritten']), null, true, ]; yield [ 'https://domain.com/foo/bar?some=overwritten', - ['foobar' => 'notrack', 'some' => 'overwritten'], + $request(['foobar' => 'notrack', 'some' => 'overwritten'])->withHeader('User-Agent', 'Unknown'), null, true, ]; yield [ 'https://domain.com/foo/bar?some=overwritten', - ['foobar' => 'notrack', 'some' => 'overwritten'], + $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, null, ]; yield [ 'https://domain.com/foo/bar?some=thing', - ['foobar' => 'notrack', 'some' => 'overwritten'], + $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, false, ]; - yield ['https://domain.com/foo/bar/something/else-baz?some=thing', [], '/something/else-baz', true]; + yield ['https://domain.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; yield [ 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', - ['hello' => 'world'], + $request(['hello' => 'world'])->withHeader('User-Agent', DESKTOP_USER_AGENT), '/something/else-baz', true, ]; yield [ 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', - ['hello' => 'world'], + $request(['hello' => 'world']), '/something/else-baz', null, ]; yield [ 'https://domain.com/foo/bar/something/else-baz?some=thing', - ['hello' => 'world'], + $request(['hello' => 'world']), '/something/else-baz', false, ]; + yield [ + 'https://domain.com/android/something', + $request(['foo' => 'bar'])->withHeader('User-Agent', ANDROID_USER_AGENT), + '/something', + false, + ]; + yield [ + 'https://domain.com/ios?foo=bar', + $request(['foo' => 'bar'])->withHeader('User-Agent', IOS_USER_AGENT), + null, + null, + ]; } } From 48bd97fe418f3eaffa700738c0111352ad687a54 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 21 Jan 2023 12:05:54 +0100 Subject: [PATCH 11/21] Return deviceLongUrls as part of the short URL data and document API changes --- docs/swagger/definitions/DeviceLongUrls.json | 20 +++ .../definitions/DeviceLongUrlsResp.json | 7 + docs/swagger/definitions/ShortUrl.json | 4 + docs/swagger/definitions/ShortUrlEdition.json | 3 + docs/swagger/paths/v1_short-urls.json | 20 +++ docs/swagger/paths/v1_short-urls_shorten.json | 6 + .../paths/v1_short-urls_{shortCode}.json | 10 ++ module/Core/src/ShortUrl/Entity/ShortUrl.php | 4 +- .../Transformer/ShortUrlDataTransformer.php | 1 + .../PublishingUpdatesGeneratorTest.php | 2 + .../test/ShortUrl/ShortUrlServiceTest.php | 5 +- .../test-api/Action/ListShortUrlsTest.php | 142 ++++++++++-------- 12 files changed, 159 insertions(+), 65 deletions(-) create mode 100644 docs/swagger/definitions/DeviceLongUrls.json create mode 100644 docs/swagger/definitions/DeviceLongUrlsResp.json diff --git a/docs/swagger/definitions/DeviceLongUrls.json b/docs/swagger/definitions/DeviceLongUrls.json new file mode 100644 index 00000000..25e7f322 --- /dev/null +++ b/docs/swagger/definitions/DeviceLongUrls.json @@ -0,0 +1,20 @@ +{ + "type": "object", + "properties": { + "android": { + "description": "The long URL to redirect to when the short URL is visited from a device running Android", + "type": "string", + "nullable": true + }, + "ios": { + "description": "The long URL to redirect to when the short URL is visited from a device running iOS", + "type": "string", + "nullable": true + }, + "desktop": { + "description": "The long URL to redirect to when the short URL is visited from a desktop browser", + "type": "string", + "nullable": true + } + } +} diff --git a/docs/swagger/definitions/DeviceLongUrlsResp.json b/docs/swagger/definitions/DeviceLongUrlsResp.json new file mode 100644 index 00000000..64a56c01 --- /dev/null +++ b/docs/swagger/definitions/DeviceLongUrlsResp.json @@ -0,0 +1,7 @@ +{ + "type": "object", + "required": ["android", "ios", "desktop"], + "allOf": [{ + "$ref": "./DeviceLongUrls.json" + }] +} diff --git a/docs/swagger/definitions/ShortUrl.json b/docs/swagger/definitions/ShortUrl.json index 4d5d9f2d..4060e2f2 100644 --- a/docs/swagger/definitions/ShortUrl.json +++ b/docs/swagger/definitions/ShortUrl.json @@ -4,6 +4,7 @@ "shortCode", "shortUrl", "longUrl", + "deviceLongUrls", "dateCreated", "visitsCount", "visitsSummary", @@ -27,6 +28,9 @@ "type": "string", "description": "The original long URL." }, + "deviceLongUrls": { + "$ref": "./DeviceLongUrlsResp.json" + }, "dateCreated": { "type": "string", "format": "date-time", diff --git a/docs/swagger/definitions/ShortUrlEdition.json b/docs/swagger/definitions/ShortUrlEdition.json index 94ef6135..7a9aca7b 100644 --- a/docs/swagger/definitions/ShortUrlEdition.json +++ b/docs/swagger/definitions/ShortUrlEdition.json @@ -5,6 +5,9 @@ "description": "The long URL this short URL will redirect to", "type": "string" }, + "deviceLongUrls": { + "$ref": "./DeviceLongUrls.json" + }, "validSince": { "description": "The date (in ISO-8601 format) from which this short code will be valid", "type": "string", diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index 2bd461d8..12afe6f4 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -163,6 +163,11 @@ "shortCode": "12C18", "shortUrl": "https://s.test/12C18", "longUrl": "https://store.steampowered.com", + "deviceLongUrls": { + "android": null, + "ios": null, + "desktop": null + }, "dateCreated": "2016-08-21T20:34:16+02:00", "visitsSummary": { "total": 328, @@ -186,6 +191,11 @@ "shortCode": "12Kb3", "shortUrl": "https://s.test/12Kb3", "longUrl": "https://shlink.io", + "deviceLongUrls": { + "android": null, + "ios": "https://shlink.io/ios", + "desktop": null + }, "dateCreated": "2016-05-01T20:34:16+02:00", "visitsSummary": { "total": 1029, @@ -208,6 +218,11 @@ "shortCode": "123bA", "shortUrl": "https://example.com/123bA", "longUrl": "https://www.google.com", + "deviceLongUrls": { + "android": null, + "ios": null, + "desktop": null + }, "dateCreated": "2015-10-01T20:34:16+02:00", "visitsSummary": { "total": 25, @@ -320,6 +335,11 @@ "shortCode": "12C18", "shortUrl": "https://s.test/12C18", "longUrl": "https://store.steampowered.com", + "deviceLongUrls": { + "android": null, + "ios": null, + "desktop": null + }, "dateCreated": "2016-08-21T20:34:16+02:00", "visitsSummary": { "total": 0, diff --git a/docs/swagger/paths/v1_short-urls_shorten.json b/docs/swagger/paths/v1_short-urls_shorten.json index e0257c59..bf2889e5 100644 --- a/docs/swagger/paths/v1_short-urls_shorten.json +++ b/docs/swagger/paths/v1_short-urls_shorten.json @@ -1,6 +1,7 @@ { "get": { "operationId": "shortenUrl", + "deprecated": true, "tags": [ "Short URLs" ], @@ -52,6 +53,11 @@ }, "example": { "longUrl": "https://github.com/shlinkio/shlink", + "deviceLongUrls": { + "android": null, + "ios": null, + "desktop": null + }, "shortUrl": "https://s.test/abc123", "shortCode": "abc123", "dateCreated": "2016-08-21T20:34:16+02:00", diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}.json b/docs/swagger/paths/v1_short-urls_{shortCode}.json index bd69b4ab..e639f362 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}.json @@ -40,6 +40,11 @@ "shortCode": "12Kb3", "shortUrl": "https://s.test/12Kb3", "longUrl": "https://shlink.io", + "deviceLongUrls": { + "android": null, + "ios": null, + "desktop": null + }, "dateCreated": "2016-05-01T20:34:16+02:00", "visitsSummary": { "total": 1029, @@ -162,6 +167,11 @@ "shortCode": "12Kb3", "shortUrl": "https://s.test/12Kb3", "longUrl": "https://shlink.io", + "deviceLongUrls": { + "android": "https://shlink.io/android", + "ios": null, + "desktop": null + }, "dateCreated": "2016-05-01T20:34:16+02:00", "visitsSummary": { "total": 1029, diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 9063bdd7..4e93a916 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -25,8 +25,10 @@ use Shlinkio\Shlink\Core\Visit\Model\VisitType; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function array_fill_keys; use function count; use function Functional\map; +use function Shlinkio\Shlink\Core\enumValues; use function Shlinkio\Shlink\Core\generateRandomShortCode; use function Shlinkio\Shlink\Core\normalizeDate; use function Shlinkio\Shlink\Core\normalizeOptionalDate; @@ -330,7 +332,7 @@ class ShortUrl extends AbstractEntity public function deviceLongUrls(): array { - $data = []; + $data = array_fill_keys(enumValues(DeviceType::class), null); foreach ($this->deviceLongUrls as $deviceUrl) { $data[$deviceUrl->deviceType->value] = $deviceUrl->longUrl(); } diff --git a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php index 08327a98..9de5c408 100644 --- a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php @@ -27,6 +27,7 @@ class ShortUrlDataTransformer implements DataTransformerInterface 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => $this->stringifier->stringify($shortUrl), 'longUrl' => $shortUrl->getLongUrl(), + 'deviceLongUrls' => $shortUrl->deviceLongUrls(), 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'tags' => invoke($shortUrl->getTags(), '__toString'), 'meta' => $this->buildMeta($shortUrl), diff --git a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php index 9611df99..924996f9 100644 --- a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php +++ b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php @@ -52,6 +52,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), 'longUrl' => 'longUrl', + 'deviceLongUrls' => $shortUrl->deviceLongUrls(), 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'visitsCount' => 0, 'tags' => [], @@ -129,6 +130,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), 'longUrl' => 'longUrl', + 'deviceLongUrls' => $shortUrl->deviceLongUrls(), 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'visitsCount' => 0, 'tags' => [], diff --git a/module/Core/test/ShortUrl/ShortUrlServiceTest.php b/module/Core/test/ShortUrl/ShortUrlServiceTest.php index 903c3d3d..7851aa9b 100644 --- a/module/Core/test/ShortUrl/ShortUrlServiceTest.php +++ b/module/Core/test/ShortUrl/ShortUrlServiceTest.php @@ -20,6 +20,9 @@ use Shlinkio\Shlink\Core\ShortUrl\ShortUrlService; use Shlinkio\Shlink\Rest\Entity\ApiKey; use ShlinkioTest\Shlink\Core\Util\ApiKeyHelpersTrait; +use function array_fill_keys; +use function Shlinkio\Shlink\Core\enumValues; + class ShortUrlServiceTest extends TestCase { use ApiKeyHelpersTrait; @@ -74,7 +77,7 @@ class ShortUrlServiceTest extends TestCase ); $resolveDeviceLongUrls = function () use ($shortUrlEdit): array { - $result = []; + $result = array_fill_keys(enumValues(DeviceType::class), null); foreach ($shortUrlEdit->deviceLongUrls ?? [] as $longUrl) { $result[$longUrl->deviceType->value] = $longUrl->longUrl; } diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 43d466e3..5cddfc50 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -6,6 +6,7 @@ namespace ShlinkioApiTest\Shlink\Rest\Action; use Cake\Chronos\Chronos; use GuzzleHttp\RequestOptions; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; use function count; @@ -169,109 +170,124 @@ class ListShortUrlsTest extends ApiTestCase public function provideFilteredLists(): iterable { + // FIXME Cannot use enums in constants in PHP 8.1. Change this once support for PHP 8.1 is dropped + $withDeviceLongUrls = static fn (array $shortUrl, ?array $longUrls = null) => [ + ...$shortUrl, + 'deviceLongUrls' => $longUrls ?? [ + DeviceType::ANDROID->value => null, + DeviceType::IOS->value => null, + DeviceType::DESKTOP->value => null, + ], + ]; + $shortUrlMeta = $withDeviceLongUrls(self::SHORT_URL_META, [ + DeviceType::ANDROID->value => 'https://blog.alejandrocelaya.com/android', + DeviceType::IOS->value => 'https://blog.alejandrocelaya.com/ios', + DeviceType::DESKTOP->value => null, + ]); + yield [[], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_META, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_SHLINK_WITH_TITLE, - self::SHORT_URL_DOCS, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), + $withDeviceLongUrls(self::SHORT_URL_DOCS), ], 'valid_api_key']; yield [['excludePastValidUntil' => 'true'], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_META, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['excludeMaxVisitsReached' => 'true'], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_META, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_DOCS, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_DOCS), ], 'valid_api_key']; yield [['orderBy' => 'shortCode'], [ - self::SHORT_URL_SHLINK_WITH_TITLE, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_DOCS, - self::SHORT_URL_CUSTOM_DOMAIN, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_DOCS), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), ], 'valid_api_key']; yield [['orderBy' => 'shortCode-DESC'], [ - self::SHORT_URL_DOCS, - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_DOCS), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['orderBy' => 'title-DESC'], [ - self::SHORT_URL_META, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_DOCS, - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_SHLINK_WITH_TITLE, + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $withDeviceLongUrls(self::SHORT_URL_DOCS), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['startDate' => Chronos::parse('2018-12-01')->toAtomString()], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_META, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $shortUrlMeta, ], 'valid_api_key']; yield [['endDate' => Chronos::parse('2018-12-01')->toAtomString()], [ - self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, - self::SHORT_URL_SHLINK_WITH_TITLE, - self::SHORT_URL_DOCS, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN), + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), + $withDeviceLongUrls(self::SHORT_URL_DOCS), ], 'valid_api_key']; yield [['tags' => ['foo']], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['tags' => ['bar']], [ - self::SHORT_URL_META, + $shortUrlMeta, ], 'valid_api_key']; yield [['tags' => ['foo', 'bar']], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['tags' => ['foo', 'bar'], 'tagsMode' => 'any'], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['tags' => ['foo', 'bar'], 'tagsMode' => 'all'], [ - self::SHORT_URL_META, + $shortUrlMeta, ], 'valid_api_key']; yield [['tags' => ['foo', 'bar', 'baz']], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['tags' => ['foo', 'bar', 'baz'], 'tagsMode' => 'all'], [], 'valid_api_key']; yield [['tags' => ['foo'], 'endDate' => Chronos::parse('2018-12-01')->toAtomString()], [ - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['searchTerm' => 'alejandro'], [ - self::SHORT_URL_CUSTOM_DOMAIN, - self::SHORT_URL_META, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), + $shortUrlMeta, ], 'valid_api_key']; yield [['searchTerm' => 'cool'], [ - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'valid_api_key']; yield [['searchTerm' => 'example.com'], [ - self::SHORT_URL_CUSTOM_DOMAIN, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), ], 'valid_api_key']; yield [[], [ - self::SHORT_URL_CUSTOM_SLUG, - self::SHORT_URL_META, - self::SHORT_URL_SHLINK_WITH_TITLE, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_SLUG), + $shortUrlMeta, + $withDeviceLongUrls(self::SHORT_URL_SHLINK_WITH_TITLE), ], 'author_api_key']; yield [[], [ - self::SHORT_URL_CUSTOM_DOMAIN, + $withDeviceLongUrls(self::SHORT_URL_CUSTOM_DOMAIN), ], 'domain_api_key']; } From 34129b8d24a8e3a8efc663b187406ff91b70b07c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 21 Jan 2023 12:09:38 +0100 Subject: [PATCH 12/21] Update async API docs with device long URLs --- docs/async-api/async-api.json | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/async-api/async-api.json b/docs/async-api/async-api.json index 418409cf..d45dae2b 100644 --- a/docs/async-api/async-api.json +++ b/docs/async-api/async-api.json @@ -111,6 +111,9 @@ "type": "string", "description": "The original long URL." }, + "deviceLongUrls": { + "$ref": "#/components/schemas/DeviceLongUrls" + }, "dateCreated": { "type": "string", "format": "date-time", @@ -152,6 +155,11 @@ "shortCode": "12C18", "shortUrl": "https://s.test/12C18", "longUrl": "https://store.steampowered.com", + "deviceLongUrls": { + "android": "https://store.steampowered.com/android", + "ios": "https://store.steampowered.com/ios", + "desktop": null + }, "dateCreated": "2016-08-21T20:34:16+02:00", "visitsSummary": { "total": 328, @@ -215,6 +223,24 @@ } } }, + "DeviceLongUrls": { + "type": "object", + "required": ["android", "ios", "desktop"], + "properties": { + "android": { + "description": "The long URL to redirect to when the short URL is visited from a device running Android", + "type": "string" + }, + "ios": { + "description": "The long URL to redirect to when the short URL is visited from a device running iOS", + "type": "string" + }, + "desktop": { + "description": "The long URL to redirect to when the short URL is visited from a desktop browser", + "type": "string" + } + } + }, "Visit": { "type": "object", "properties": { From 45961144b937e2f9010485dce961352476ef2c80 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 21 Jan 2023 12:13:42 +0100 Subject: [PATCH 13/21] Update changelog --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55ce701d..58031200 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ## [Unreleased] ### Added +* [#1557](https://github.com/shlinkio/shlink/issues/1557) Added support to dynamically redirect to different long URLs based on the visitor's device type. + + For the moment, only `android`, `ios` and `desktop` can have their own specific long URL, and when the visitor cannot be matched against any of them, the regular long URL will be used. + + In the future, more granular device types could be added if appropriate (iOS tablet, android table, tablet, mobile phone, Linux, Mac, Windows, etc). + + In order to match the visitor's device, the `User-Agent` header is used. + * [#1632](https://github.com/shlinkio/shlink/issues/1632) Added amount of bots, non-bots and total visits to the visits summary endpoint. * [#1633](https://github.com/shlinkio/shlink/issues/1633) Added amount of bots, non-bots and total visits to the tag stats endpoint. * [#1653](https://github.com/shlinkio/shlink/issues/1653) Added support for all HTTP methods in short URLs, together with two new redirect status codes, 307 and 308. @@ -20,7 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *Nothing* ### Deprecated -* *Nothing* +* [#1676](https://github.com/shlinkio/shlink/issues/1676) Deprecated `GET /short-urls/shorten` endpoint. Use `POST /short-urls` to create short URLs instead ### Removed * *Nothing* From 13e443880a66e73f35443721b072e6cdb8ac57fd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 11:03:05 +0100 Subject: [PATCH 14/21] Allow device long URLs to be removed from short URLs by providing null value --- ...o.Shlink.Core.ShortUrl.Entity.ShortUrl.php | 2 + module/Core/src/ShortUrl/Entity/ShortUrl.php | 16 +++--- .../src/ShortUrl/Model/DeviceLongUrlPair.php | 22 ++++++-- .../src/ShortUrl/Model/ShortUrlCreation.php | 8 +-- .../src/ShortUrl/Model/ShortUrlEdition.php | 14 +++-- .../Validation/DeviceLongUrlsValidator.php | 4 +- .../Model/Validation/ShortUrlInputFilter.php | 22 +++++--- .../ShortUrl/Model/ShortUrlCreationTest.php | 35 ++++++++++++ .../ShortUrl/Model/ShortUrlEditionTest.php | 54 +++++++++++++++++++ .../DeviceLongUrlsValidatorTest.php | 6 +-- 10 files changed, 150 insertions(+), 33 deletions(-) create mode 100644 module/Core/test/ShortUrl/Model/ShortUrlEditionTest.php diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php index 13aa36f6..746ac3fd 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php @@ -70,6 +70,8 @@ return static function (ClassMetadata $metadata, array $emConfig): void { $builder->createOneToMany('deviceLongUrls', ShortUrl\Entity\DeviceLongUrl::class) ->mappedBy('shortUrl') ->cascadePersist() + ->orphanRemoval() + ->setIndexBy('deviceType') ->build(); $builder->createManyToMany('tags', Tag\Entity\Tag::class) diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 4e93a916..e6da743e 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -40,7 +40,7 @@ class ShortUrl extends AbstractEntity private Chronos $dateCreated; /** @var Collection */ private Collection $visits; - /** @var Collection */ + /** @var Collection */ private Collection $deviceLongUrls; /** @var Collection */ private Collection $tags; @@ -171,10 +171,13 @@ class ShortUrl extends AbstractEntity if ($shortUrlEdit->forwardQueryWasProvided()) { $this->forwardQuery = $shortUrlEdit->forwardQuery; } + + // Update device long URLs, removing, editing or creating where appropriate + foreach ($shortUrlEdit->devicesToRemove as $deviceType) { + $this->deviceLongUrls->remove($deviceType->value); + } foreach ($shortUrlEdit->deviceLongUrls as $deviceLongUrlPair) { - $deviceLongUrl = $this->deviceLongUrls->findFirst( - fn ($_, DeviceLongUrl $d) => $d->deviceType === $deviceLongUrlPair->deviceType, - ); + $deviceLongUrl = $this->deviceLongUrls->get($deviceLongUrlPair->deviceType->value); if ($deviceLongUrl !== null) { $deviceLongUrl->updateLongUrl($deviceLongUrlPair->longUrl); @@ -191,10 +194,7 @@ class ShortUrl extends AbstractEntity public function longUrlForDevice(?DeviceType $deviceType): string { - $deviceLongUrl = $this->deviceLongUrls->findFirst( - static fn ($_, DeviceLongUrl $longUrl) => $longUrl->deviceType === $deviceType, - ); - + $deviceLongUrl = $deviceType === null ? null : $this->deviceLongUrls->get($deviceType->value); return $deviceLongUrl?->longUrl() ?? $this->longUrl; } diff --git a/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php b/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php index 6d0234ec..d017c7e5 100644 --- a/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php +++ b/module/Core/src/ShortUrl/Model/DeviceLongUrlPair.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; use Shlinkio\Shlink\Core\Model\DeviceType; use function array_values; +use function Functional\group; use function Functional\map; use function trim; @@ -22,14 +23,25 @@ final class DeviceLongUrlPair } /** + * Returns an array with two values. + * * The first one is a list of mapped instances for those entries in the map with non-null value + * * The second is a list of DeviceTypes which have been provided with value null + * * @param array $map - * @return self[] + * @return array{array, DeviceType[]} */ - public static function fromMapToList(array $map): array + public static function fromMapToChangeSet(array $map): array { - return array_values(map( - $map, - fn (string $longUrl, string $deviceType) => self::fromRawTypeAndLongUrl($deviceType, $longUrl), + $typesWithNullUrl = group($map, static fn (?string $longUrl) => $longUrl === null ? 'remove' : 'keep'); + $deviceTypesToRemove = array_values(map( + $typesWithNullUrl['remove'] ?? [], + static fn ($_, string $deviceType) => DeviceType::from($deviceType), )); + $pairsToKeep = map( + $typesWithNullUrl['keep'] ?? [], + fn (string $longUrl, string $deviceType) => self::fromRawTypeAndLongUrl($deviceType, $longUrl), + ); + + return [$pairsToKeep, $deviceTypesToRemove]; } } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php index f2e156f4..a5d20bfb 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php @@ -61,11 +61,13 @@ final class ShortUrlCreation implements TitleResolutionModelInterface throw ValidationException::fromInputFilter($inputFilter); } + [$deviceLongUrls] = DeviceLongUrlPair::fromMapToChangeSet( + $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], + ); + return new self( longUrl: $inputFilter->getValue(ShortUrlInputFilter::LONG_URL), - deviceLongUrls: DeviceLongUrlPair::fromMapToList( - $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], - ), + deviceLongUrls: $deviceLongUrls, validSince: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)), validUntil: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_UNTIL)), customSlug: $inputFilter->getValue(ShortUrlInputFilter::CUSTOM_SLUG), diff --git a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php index 25645437..6bc157c7 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlEdition.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlEdition.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Helper\TitleResolutionModelInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; @@ -19,11 +20,13 @@ final class ShortUrlEdition implements TitleResolutionModelInterface /** * @param string[] $tags * @param DeviceLongUrlPair[] $deviceLongUrls + * @param DeviceType[] $devicesToRemove */ private function __construct( private readonly bool $longUrlPropWasProvided = false, public readonly ?string $longUrl = null, public readonly array $deviceLongUrls = [], + public readonly array $devicesToRemove = [], private readonly bool $validSincePropWasProvided = false, public readonly ?Chronos $validSince = null, private readonly bool $validUntilPropWasProvided = false, @@ -53,12 +56,15 @@ final class ShortUrlEdition implements TitleResolutionModelInterface throw ValidationException::fromInputFilter($inputFilter); } + [$deviceLongUrls, $devicesToRemove] = DeviceLongUrlPair::fromMapToChangeSet( + $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], + ); + return new self( longUrlPropWasProvided: array_key_exists(ShortUrlInputFilter::LONG_URL, $data), longUrl: $inputFilter->getValue(ShortUrlInputFilter::LONG_URL), - deviceLongUrls: DeviceLongUrlPair::fromMapToList( - $inputFilter->getValue(ShortUrlInputFilter::DEVICE_LONG_URLS) ?? [], - ), + deviceLongUrls: $deviceLongUrls, + devicesToRemove: $devicesToRemove, validSincePropWasProvided: array_key_exists(ShortUrlInputFilter::VALID_SINCE, $data), validSince: normalizeOptionalDate($inputFilter->getValue(ShortUrlInputFilter::VALID_SINCE)), validUntilPropWasProvided: array_key_exists(ShortUrlInputFilter::VALID_UNTIL, $data), @@ -82,6 +88,8 @@ final class ShortUrlEdition implements TitleResolutionModelInterface return new self( longUrlPropWasProvided: $this->longUrlPropWasProvided, longUrl: $this->longUrl, + deviceLongUrls: $this->deviceLongUrls, + devicesToRemove: $this->devicesToRemove, validSincePropWasProvided: $this->validSincePropWasProvided, validSince: $this->validSince, validUntilPropWasProvided: $this->validUntilPropWasProvided, diff --git a/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php b/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php index 1e9d9824..9fda1809 100644 --- a/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php +++ b/module/Core/src/ShortUrl/Model/Validation/DeviceLongUrlsValidator.php @@ -5,7 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Model\Validation; use Laminas\Validator\AbstractValidator; -use Laminas\Validator\ValidatorChain; +use Laminas\Validator\ValidatorInterface; use Shlinkio\Shlink\Core\Model\DeviceType; use function array_keys; @@ -27,7 +27,7 @@ class DeviceLongUrlsValidator extends AbstractValidator self::INVALID_LONG_URL => 'At least one of the long URLs are invalid.', ]; - public function __construct(private readonly ValidatorChain $longUrlValidators) + public function __construct(private readonly ValidatorInterface $longUrlValidators) { parent::__construct(); } diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index 7b01841b..4a0e2d7b 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Model\Validation; -use DateTime; +use DateTimeInterface; use Laminas\Filter; use Laminas\InputFilter\InputFilter; use Laminas\Validator; @@ -41,6 +41,7 @@ class ShortUrlInputFilter extends InputFilter private function __construct(array $data, bool $requireLongUrl) { + // FIXME The multi-segment slug option should be injected $this->initialize($requireLongUrl, $data[EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->value] ?? false); $this->setData($data); } @@ -57,29 +58,36 @@ class ShortUrlInputFilter extends InputFilter private function initialize(bool $requireLongUrl, bool $multiSegmentEnabled): void { - $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); - $longUrlInput->getValidatorChain()->attach(new Validator\NotEmpty([ + $longUrlNotEmptyCommonOptions = [ Validator\NotEmpty::OBJECT, Validator\NotEmpty::SPACE, - Validator\NotEmpty::NULL, Validator\NotEmpty::EMPTY_ARRAY, Validator\NotEmpty::BOOLEAN, Validator\NotEmpty::STRING, + ]; + + $longUrlInput = $this->createInput(self::LONG_URL, $requireLongUrl); + $longUrlInput->getValidatorChain()->attach(new Validator\NotEmpty([ + ...$longUrlNotEmptyCommonOptions, + Validator\NotEmpty::NULL, ])); $this->add($longUrlInput); $deviceLongUrlsInput = $this->createInput(self::DEVICE_LONG_URLS, false); $deviceLongUrlsInput->getValidatorChain()->attach( - new DeviceLongUrlsValidator($longUrlInput->getValidatorChain()), + new DeviceLongUrlsValidator(new Validator\NotEmpty([ + ...$longUrlNotEmptyCommonOptions, + ...($requireLongUrl ? [Validator\NotEmpty::NULL] : []), + ])), ); $this->add($deviceLongUrlsInput); $validSince = $this->createInput(self::VALID_SINCE, false); - $validSince->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); + $validSince->getValidatorChain()->attach(new Validator\Date(['format' => DateTimeInterface::ATOM])); $this->add($validSince); $validUntil = $this->createInput(self::VALID_UNTIL, false); - $validUntil->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); + $validUntil->getValidatorChain()->attach(new Validator\Date(['format' => DateTimeInterface::ATOM])); $this->add($validUntil); // The only way to enforce the NotEmpty validator to be evaluated when the key is present with an empty value diff --git a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php index 33380ecf..4d11289c 100644 --- a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php +++ b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php @@ -8,6 +8,7 @@ use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Config\EnvVars; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use stdClass; @@ -69,6 +70,40 @@ class ShortUrlCreationTest extends TestCase yield [[ ShortUrlInputFilter::LONG_URL => [], ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => null, + ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'foo', + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + 'invalid' => 'https://shlink.io', + ], + ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'foo', + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::DESKTOP->value => '', + ], + ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'foo', + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::DESKTOP->value => null, + ], + ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'foo', + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::IOS->value => ' ', + ], + ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'foo', + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::IOS->value => 'bar', + DeviceType::ANDROID->value => [], + ], + ]]; } /** diff --git a/module/Core/test/ShortUrl/Model/ShortUrlEditionTest.php b/module/Core/test/ShortUrl/Model/ShortUrlEditionTest.php new file mode 100644 index 00000000..e03bb1ac --- /dev/null +++ b/module/Core/test/ShortUrl/Model/ShortUrlEditionTest.php @@ -0,0 +1,54 @@ + $deviceLongUrls]); + + self::assertEquals($expectedDeviceLongUrls, $edition->deviceLongUrls); + self::assertEquals($expectedDevicesToRemove, $edition->devicesToRemove); + } + + public function provideDeviceLongUrls(): iterable + { + yield 'null' => [null, [], []]; + yield 'empty' => [[], [], []]; + yield 'only new urls' => [[ + DeviceType::DESKTOP->value => 'foo', + DeviceType::IOS->value => 'bar', + ], [ + DeviceType::DESKTOP->value => DeviceLongUrlPair::fromRawTypeAndLongUrl(DeviceType::DESKTOP->value, 'foo'), + DeviceType::IOS->value => DeviceLongUrlPair::fromRawTypeAndLongUrl(DeviceType::IOS->value, 'bar'), + ], []]; + yield 'only urls to remove' => [[ + DeviceType::ANDROID->value => null, + DeviceType::IOS->value => null, + ], [], [DeviceType::ANDROID, DeviceType::IOS]]; + yield 'both' => [[ + DeviceType::DESKTOP->value => 'bar', + DeviceType::IOS->value => 'foo', + DeviceType::ANDROID->value => null, + ], [ + DeviceType::DESKTOP->value => DeviceLongUrlPair::fromRawTypeAndLongUrl(DeviceType::DESKTOP->value, 'bar'), + DeviceType::IOS->value => DeviceLongUrlPair::fromRawTypeAndLongUrl(DeviceType::IOS->value, 'foo'), + ], [DeviceType::ANDROID]]; + } +} diff --git a/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php b/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php index 42ad720b..8bac2f98 100644 --- a/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php +++ b/module/Core/test/ShortUrl/Model/Validation/DeviceLongUrlsValidatorTest.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ShortUrl\Model\Validation; use Laminas\Validator\NotEmpty; -use Laminas\Validator\ValidatorChain; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\DeviceLongUrlsValidator; @@ -17,10 +16,7 @@ class DeviceLongUrlsValidatorTest extends TestCase protected function setUp(): void { - $longUrlValidators = new ValidatorChain(); - $longUrlValidators->attach(new NotEmpty()); - - $this->validator = new DeviceLongUrlsValidator($longUrlValidators); + $this->validator = new DeviceLongUrlsValidator(new NotEmpty()); } /** From 39adef8ab828271855813a6cac100fe506f8553f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 11:27:16 +0100 Subject: [PATCH 15/21] Make it impossible to create a short URL with an empty long URL --- .../Command/Domain/GetDomainVisitsCommandTest.php | 2 +- .../Command/ShortUrl/CreateShortUrlCommandTest.php | 8 ++++---- .../ShortUrl/GetShortUrlVisitsCommandTest.php | 2 +- .../CLI/test/Command/Tag/GetTagVisitsCommandTest.php | 2 +- .../Command/Visit/GetNonOrphanVisitsCommandTest.php | 2 +- .../test/Command/Visit/LocateVisitsCommandTest.php | 6 +++--- module/Core/src/ShortUrl/Entity/ShortUrl.php | 7 ++----- module/Core/src/ShortUrl/Model/ShortUrlCreation.php | 8 -------- .../ShortUrl/Repository/ShortUrlRepositoryTest.php | 1 - .../Visit/Repository/VisitLocationRepositoryTest.php | 2 +- module/Core/test/Action/QrCodeActionTest.php | 6 +++--- module/Core/test/EventDispatcher/LocateVisitTest.php | 12 ++++++------ .../Mercure/NotifyVisitToMercureTest.php | 4 ++-- .../EventDispatcher/NotifyVisitToWebHooksTest.php | 2 +- .../test/Importer/ImportedLinksProcessorTest.php | 6 +++--- .../Core/test/ShortUrl/DeleteShortUrlServiceTest.php | 4 ++-- module/Core/test/ShortUrl/Entity/ShortUrlTest.php | 4 ++-- .../Core/test/ShortUrl/ShortUrlListServiceTest.php | 8 ++++---- .../Transformer/ShortUrlDataTransformerTest.php | 2 +- module/Core/test/Visit/Entity/VisitTest.php | 4 ++-- module/Core/test/Visit/VisitsStatsHelperTest.php | 10 +++++----- module/Core/test/Visit/VisitsTrackerTest.php | 2 +- .../Action/ShortUrl/CreateShortUrlActionTest.php | 2 +- .../test/Action/ShortUrl/EditShortUrlActionTest.php | 2 +- .../ShortUrl/SingleStepCreateShortUrlActionTest.php | 2 +- 25 files changed, 49 insertions(+), 61 deletions(-) diff --git a/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php b/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php index 913e00dc..5cda6dc3 100644 --- a/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php +++ b/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetDomainVisitsCommandTest extends TestCase /** @test */ public function outputIsProperlyGenerated(): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); diff --git a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php index 1a8df888..69ce0c72 100644 --- a/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/CreateShortUrlCommandTest.php @@ -48,7 +48,7 @@ class CreateShortUrlCommandTest extends TestCase /** @test */ public function properShortCodeIsCreatedIfLongUrlIsCorrect(): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $this->urlShortener->expects($this->once())->method('shorten')->withAnyParameters()->willReturn($shortUrl); $this->stringifier->expects($this->once())->method('stringify')->with($shortUrl)->willReturn( 'stringified_short_url', @@ -98,7 +98,7 @@ class CreateShortUrlCommandTest extends TestCase /** @test */ public function properlyProcessesProvidedTags(): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $this->urlShortener->expects($this->once())->method('shorten')->with( $this->callback(function (ShortUrlCreation $creation) { Assert::assertEquals(['foo', 'bar', 'baz', 'boo', 'zar'], $creation->tags); @@ -130,7 +130,7 @@ class CreateShortUrlCommandTest extends TestCase Assert::assertEquals($expectedDomain, $meta->domain); return true; }), - )->willReturn(ShortUrl::createEmpty()); + )->willReturn(ShortUrl::createFake()); $this->stringifier->method('stringify')->with($this->isInstanceOf(ShortUrl::class))->willReturn(''); $input['longUrl'] = 'http://domain.com/foo/bar'; @@ -153,7 +153,7 @@ class CreateShortUrlCommandTest extends TestCase */ public function urlValidationHasExpectedValueBasedOnProvidedFlags(array $options, ?bool $expectedValidateUrl): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $this->urlShortener->expects($this->once())->method('shorten')->with( $this->callback(function (ShortUrlCreation $meta) use ($expectedValidateUrl) { Assert::assertEquals($expectedValidateUrl, $meta->doValidateUrl()); diff --git a/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php index 8706699b..bd2be187 100644 --- a/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php @@ -94,7 +94,7 @@ class GetShortUrlVisitsCommandTest extends TestCase /** @test */ public function outputIsProperlyGenerated(): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('bar', 'foo', '', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $shortCode = 'abc123'; diff --git a/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php b/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php index be56cdee..b7255d0a 100644 --- a/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php +++ b/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetTagVisitsCommandTest extends TestCase /** @test */ public function outputIsProperlyGenerated(): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); diff --git a/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php b/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php index 90147541..c780208a 100644 --- a/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetNonOrphanVisitsCommandTest extends TestCase /** @test */ public function outputIsProperlyGenerated(): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 518d9f45..44638249 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -66,7 +66,7 @@ class LocateVisitsCommandTest extends TestCase bool $expectWarningPrint, array $args, ): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); $mockMethodBehavior = $this->invokeHelperMethods($visit, $location); @@ -113,7 +113,7 @@ class LocateVisitsCommandTest extends TestCase */ public function localhostAndEmptyAddressesAreIgnored(IpCannotBeLocatedException $e, string $message): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance()); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); @@ -140,7 +140,7 @@ class LocateVisitsCommandTest extends TestCase /** @test */ public function errorWhileLocatingIpIsDisplayed(): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index e6da743e..9b266b4a 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -62,12 +62,9 @@ class ShortUrl extends AbstractEntity { } - /** - * @deprecated This should not be allowed - */ - public static function createEmpty(): self + public static function createFake(): self { - return self::create(ShortUrlCreation::createEmpty()); + return self::withLongUrl('foo'); } /** diff --git a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php index a5d20bfb..d5078f7b 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlCreation.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlCreation.php @@ -43,14 +43,6 @@ final class ShortUrlCreation implements TitleResolutionModelInterface ) { } - /** - * @deprecated This should not be allowed - */ - public static function createEmpty(): self - { - return new self(''); - } - /** * @throws ValidationException */ diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index 99e3add9..0d90675a 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -134,7 +134,6 @@ class ShortUrlRepositoryTest extends DatabaseTestCase /** @test */ public function findOneMatchingReturnsNullForNonExistingShortUrls(): void { - self::assertNull($this->repo->findOneMatching(ShortUrlCreation::createEmpty())); self::assertNull($this->repo->findOneMatching(ShortUrlCreation::fromRawData(['longUrl' => 'foobar']))); self::assertNull($this->repo->findOneMatching( ShortUrlCreation::fromRawData(['longUrl' => 'foobar', 'tags' => ['foo', 'bar']]), diff --git a/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php index 77f4c1e6..6b7c1e0d 100644 --- a/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php @@ -31,7 +31,7 @@ class VisitLocationRepositoryTest extends DatabaseTestCase */ public function findVisitsReturnsProperVisits(int $blockSize): void { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $this->getEntityManager()->persist($shortUrl); for ($i = 0; $i < 6; $i++) { diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 599a09d9..684e9217 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -56,7 +56,7 @@ class QrCodeActionTest extends TestCase $shortCode = 'abc123'; $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->with( ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, ''), - )->willReturn(ShortUrl::createEmpty()); + )->willReturn(ShortUrl::createFake()); $delegate = $this->createMock(RequestHandlerInterface::class); $delegate->expects($this->never())->method('handle'); @@ -78,7 +78,7 @@ class QrCodeActionTest extends TestCase $code = 'abc123'; $this->urlResolver->method('resolveEnabledShortUrl')->with( ShortUrlIdentifier::fromShortCodeAndDomain($code, ''), - )->willReturn(ShortUrl::createEmpty()); + )->willReturn(ShortUrl::createFake()); $delegate = $this->createMock(RequestHandlerInterface::class); $req = (new ServerRequest())->withAttribute('shortCode', $code)->withQueryParams($query); @@ -111,7 +111,7 @@ class QrCodeActionTest extends TestCase $code = 'abc123'; $this->urlResolver->method('resolveEnabledShortUrl')->with( ShortUrlIdentifier::fromShortCodeAndDomain($code, ''), - )->willReturn(ShortUrl::createEmpty()); + )->willReturn(ShortUrl::createFake()); $delegate = $this->createMock(RequestHandlerInterface::class); $resp = $this->action($defaultOptions)->process($req->withAttribute('shortCode', $code), $delegate); diff --git a/module/Core/test/EventDispatcher/LocateVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php index cad6d164..d538bff0 100644 --- a/module/Core/test/EventDispatcher/LocateVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateVisitTest.php @@ -70,7 +70,7 @@ class LocateVisitTest extends TestCase { $event = new UrlVisited('123'); $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), ); $this->em->expects($this->never())->method('flush'); $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(false); @@ -89,7 +89,7 @@ class LocateVisitTest extends TestCase { $event = new UrlVisited('123'); $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), ); $this->em->expects($this->never())->method('flush'); $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); @@ -110,7 +110,7 @@ class LocateVisitTest extends TestCase { $event = new UrlVisited('123'); $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), ); $this->em->expects($this->never())->method('flush'); $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); @@ -148,7 +148,7 @@ class LocateVisitTest extends TestCase public function provideNonLocatableVisits(): iterable { - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); yield 'null IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', null, ''))]; yield 'empty IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', '', ''))]; @@ -183,11 +183,11 @@ class LocateVisitTest extends TestCase public function provideIpAddresses(): iterable { yield 'no original IP address' => [ - Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), null, ]; yield 'original IP address' => [ - Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', ]; yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4']; diff --git a/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php b/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php index 1cecada7..23450fd3 100644 --- a/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php +++ b/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php @@ -59,7 +59,7 @@ class NotifyVisitToMercureTest extends TestCase public function notificationsAreSentWhenVisitIsFound(): void { $visitId = '123'; - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance()); $update = Update::forTopicAndPayload('', []); $this->em->expects($this->once())->method('find')->with(Visit::class, $visitId)->willReturn($visit); @@ -79,7 +79,7 @@ class NotifyVisitToMercureTest extends TestCase public function debugIsLoggedWhenExceptionIsThrown(): void { $visitId = '123'; - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance()); $update = Update::forTopicAndPayload('', []); $e = new RuntimeException('Error'); diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index 7a5cb888..17b26a74 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -123,7 +123,7 @@ class NotifyVisitToWebHooksTest extends TestCase public function provideVisits(): iterable { yield 'regular visit' => [ - Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()), + Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance()), ['shortUrl', 'visit'], ]; yield 'orphan visit' => [Visit::forBasePath(Visitor::emptyInstance()), ['visit'],]; diff --git a/module/Core/test/Importer/ImportedLinksProcessorTest.php b/module/Core/test/Importer/ImportedLinksProcessorTest.php index c480e11a..f1b2f3bb 100644 --- a/module/Core/test/Importer/ImportedLinksProcessorTest.php +++ b/module/Core/test/Importer/ImportedLinksProcessorTest.php @@ -210,7 +210,7 @@ class ImportedLinksProcessorTest extends TestCase ]), 'Skipped. Imported 4 visits', 4, - ShortUrl::createEmpty(), + ShortUrl::createFake(), ]; yield 'existing short URL with previous imported visits' => [ $createImportedUrl([ @@ -222,8 +222,8 @@ class ImportedLinksProcessorTest extends TestCase ]), 'Skipped. Imported 2 visits', 2, - ShortUrl::createEmpty()->setVisits(new ArrayCollection([ - Visit::fromImport(ShortUrl::createEmpty(), new ImportedShlinkVisit('', '', $now, null)), + ShortUrl::createFake()->setVisits(new ArrayCollection([ + Visit::fromImport(ShortUrl::createFake(), new ImportedShlinkVisit('', '', $now, null)), ])), ]; } diff --git a/module/Core/test/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/ShortUrl/DeleteShortUrlServiceTest.php index be036264..3173e2ee 100644 --- a/module/Core/test/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/ShortUrl/DeleteShortUrlServiceTest.php @@ -29,8 +29,8 @@ class DeleteShortUrlServiceTest extends TestCase protected function setUp(): void { - $shortUrl = ShortUrl::createEmpty()->setVisits(new ArrayCollection( - map(range(0, 10), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())), + $shortUrl = ShortUrl::createFake()->setVisits(new ArrayCollection( + map(range(0, 10), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())), )); $this->shortCode = $shortUrl->getShortCode(); diff --git a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php index fd4515fb..d83ff3d7 100644 --- a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php +++ b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php @@ -42,7 +42,7 @@ class ShortUrlTest extends TestCase 'The short code cannot be regenerated on ShortUrls where a custom slug was provided.', ]; yield 'already persisted' => [ - ShortUrl::createEmpty()->setId('1'), + ShortUrl::createFake()->setId('1'), 'The short code can be regenerated only on new ShortUrls which have not been persisted yet.', ]; } @@ -64,7 +64,7 @@ class ShortUrlTest extends TestCase public function provideValidShortUrls(): iterable { - yield 'no custom slug' => [ShortUrl::createEmpty()]; + yield 'no custom slug' => [ShortUrl::createFake()]; yield 'imported with custom slug' => [ShortUrl::fromImport( new ImportedShlinkUrl(ImportSource::BITLY, 'longUrl', [], Chronos::now(), null, 'custom-slug', null), true, diff --git a/module/Core/test/ShortUrl/ShortUrlListServiceTest.php b/module/Core/test/ShortUrl/ShortUrlListServiceTest.php index be8eb852..446e95eb 100644 --- a/module/Core/test/ShortUrl/ShortUrlListServiceTest.php +++ b/module/Core/test/ShortUrl/ShortUrlListServiceTest.php @@ -36,10 +36,10 @@ class ShortUrlListServiceTest extends TestCase public function listedUrlsAreReturnedFromEntityManager(?ApiKey $apiKey): void { $list = [ - ShortUrl::createEmpty(), - ShortUrl::createEmpty(), - ShortUrl::createEmpty(), - ShortUrl::createEmpty(), + ShortUrl::createFake(), + ShortUrl::createFake(), + ShortUrl::createFake(), + ShortUrl::createFake(), ]; $this->repo->expects($this->once())->method('findList')->willReturn($list); diff --git a/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php b/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php index 7a97d4da..6159294b 100644 --- a/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php +++ b/module/Core/test/ShortUrl/Transformer/ShortUrlDataTransformerTest.php @@ -38,7 +38,7 @@ class ShortUrlDataTransformerTest extends TestCase $maxVisits = random_int(1, 1000); $now = Chronos::now(); - yield 'no metadata' => [ShortUrl::createEmpty(), [ + yield 'no metadata' => [ShortUrl::createFake(), [ 'validSince' => null, 'validUntil' => null, 'maxVisits' => null, diff --git a/module/Core/test/Visit/Entity/VisitTest.php b/module/Core/test/Visit/Entity/VisitTest.php index 7024c946..5ae22005 100644 --- a/module/Core/test/Visit/Entity/VisitTest.php +++ b/module/Core/test/Visit/Entity/VisitTest.php @@ -18,7 +18,7 @@ class VisitTest extends TestCase */ public function isProperlyJsonSerialized(string $userAgent, bool $expectedToBePotentialBot): void { - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor($userAgent, 'some site', '1.2.3.4', '')); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor($userAgent, 'some site', '1.2.3.4', '')); self::assertEquals([ 'referer' => 'some site', @@ -48,7 +48,7 @@ class VisitTest extends TestCase public function addressIsAnonymizedWhenRequested(bool $anonymize, ?string $address, ?string $expectedAddress): void { $visit = Visit::forValidShortUrl( - ShortUrl::createEmpty(), + ShortUrl::createFake(), new Visitor('Chrome', 'some site', $address, ''), $anonymize, ); diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index 1774ba6a..0ed06e7d 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -86,7 +86,7 @@ class VisitsStatsHelperTest extends TestCase $repo = $this->createMock(ShortUrlRepositoryInterface::class); $repo->expects($this->once())->method('shortCodeIsInUse')->with($identifier, $spec)->willReturn(true); - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())); $repo2 = $this->createMock(VisitRepository::class); $repo2->method('findVisitsByShortCode')->with( $identifier, @@ -146,7 +146,7 @@ class VisitsStatsHelperTest extends TestCase $repo = $this->createMock(TagRepository::class); $repo->expects($this->once())->method('tagExists')->with($tag, $apiKey)->willReturn(true); - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())); $repo2 = $this->createMock(VisitRepository::class); $repo2->method('findVisitsByTag')->with($tag, $this->isInstanceOf(VisitsListFiltering::class))->willReturn( $list, @@ -187,7 +187,7 @@ class VisitsStatsHelperTest extends TestCase $repo = $this->createMock(DomainRepository::class); $repo->expects($this->once())->method('domainExists')->with($domain, $apiKey)->willReturn(true); - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())); $repo2 = $this->createMock(VisitRepository::class); $repo2->method('findVisitsByDomain')->with( $domain, @@ -217,7 +217,7 @@ class VisitsStatsHelperTest extends TestCase $repo = $this->createMock(DomainRepository::class); $repo->expects($this->never())->method('domainExists'); - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())); $repo2 = $this->createMock(VisitRepository::class); $repo2->method('findVisitsByDomain')->with( 'DEFAULT', @@ -259,7 +259,7 @@ class VisitsStatsHelperTest extends TestCase /** @test */ public function nonOrphanVisitsAreReturnedAsExpected(): void { - $list = map(range(0, 3), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 3), fn () => Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::emptyInstance())); $repo = $this->createMock(VisitRepository::class); $repo->expects($this->once())->method('countNonOrphanVisits')->with( $this->isInstanceOf(VisitsCountFiltering::class), diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index d981f755..9c27d5df 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -58,7 +58,7 @@ class VisitsTrackerTest extends TestCase public function provideTrackingMethodNames(): iterable { - yield 'track' => ['track', [ShortUrl::createEmpty(), Visitor::emptyInstance()]]; + yield 'track' => ['track', [ShortUrl::createFake(), Visitor::emptyInstance()]]; yield 'trackInvalidShortUrlVisit' => ['trackInvalidShortUrlVisit', [Visitor::emptyInstance()]]; yield 'trackBaseUrlVisit' => ['trackBaseUrlVisit', [Visitor::emptyInstance()]]; yield 'trackRegularNotFoundVisit' => ['trackRegularNotFoundVisit', [Visitor::emptyInstance()]]; diff --git a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php index 246b2edf..15ce5389 100644 --- a/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/CreateShortUrlActionTest.php @@ -37,7 +37,7 @@ class CreateShortUrlActionTest extends TestCase public function properShortcodeConversionReturnsData(): void { $apiKey = ApiKey::create(); - $shortUrl = ShortUrl::createEmpty(); + $shortUrl = ShortUrl::createFake(); $expectedMeta = $body = [ 'longUrl' => 'http://www.domain.com/foo/bar', 'validSince' => Chronos::now()->toAtomString(), diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php index dde17ca6..ac788fa7 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlActionTest.php @@ -49,7 +49,7 @@ class EditShortUrlActionTest extends TestCase ->withParsedBody([ 'maxVisits' => 5, ]); - $this->shortUrlService->expects($this->once())->method('updateShortUrl')->willReturn(ShortUrl::createEmpty()); + $this->shortUrlService->expects($this->once())->method('updateShortUrl')->willReturn(ShortUrl::createFake()); $resp = $this->action->handle($request); diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index 14848696..42c185c9 100644 --- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php @@ -43,7 +43,7 @@ class SingleStepCreateShortUrlActionTest extends TestCase ])->withAttribute(ApiKey::class, $apiKey); $this->urlShortener->expects($this->once())->method('shorten')->with( ShortUrlCreation::fromRawData(['apiKey' => $apiKey, 'longUrl' => 'http://foobar.com']), - )->willReturn(ShortUrl::createEmpty()); + )->willReturn(ShortUrl::createFake()); $resp = $this->action->handle($request); From d3590234a37e2c396e37867644bdfbaef322ac36 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 11:36:00 +0100 Subject: [PATCH 16/21] Add API test for short URL creation with device long URLs --- .../test-api/Action/CreateShortUrlTest.php | 54 +++++++++++++++---- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/module/Rest/test-api/Action/CreateShortUrlTest.php b/module/Rest/test-api/Action/CreateShortUrlTest.php index 19bd6c74..4300c852 100644 --- a/module/Rest/test-api/Action/CreateShortUrlTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlTest.php @@ -118,7 +118,7 @@ class CreateShortUrlTest extends ApiTestCase public function provideMaxVisits(): array { - return map(range(10, 15), fn (int $i) => [$i]); + return map(range(10, 15), fn(int $i) => [$i]); } /** @test */ @@ -172,12 +172,14 @@ class CreateShortUrlTest extends ApiTestCase yield 'only long URL' => [['longUrl' => $longUrl]]; yield 'long URL and tags' => [['longUrl' => $longUrl, 'tags' => ['boo', 'far']]]; yield 'long URL and custom slug' => [['longUrl' => $longUrl, 'customSlug' => 'my cool slug']]; - yield 'several params' => [[ - 'longUrl' => $longUrl, - 'tags' => ['boo', 'far'], - 'validSince' => Chronos::now()->toAtomString(), - 'maxVisits' => 7, - ]]; + yield 'several params' => [ + [ + 'longUrl' => $longUrl, + 'tags' => ['boo', 'far'], + 'validSince' => Chronos::now()->toAtomString(), + 'maxVisits' => 7, + ] + ]; } /** @@ -269,12 +271,12 @@ class CreateShortUrlTest extends ApiTestCase * @test * @dataProvider provideInvalidArgumentApiVersions */ - public function failsToCreateShortUrlWithoutLongUrl(string $version, string $expectedType): void + public function failsToCreateShortUrlWithoutLongUrl(array $payload, string $version, string $expectedType): void { $resp = $this->callApiWithKey( self::METHOD_POST, sprintf('/rest/v%s/short-urls', $version), - [RequestOptions::JSON => []], + [RequestOptions::JSON => $payload], ); $payload = $this->getJsonResponsePayload($resp); @@ -287,8 +289,22 @@ class CreateShortUrlTest extends ApiTestCase public function provideInvalidArgumentApiVersions(): iterable { - yield ['2', 'INVALID_ARGUMENT']; - yield ['3', 'https://shlink.io/api/error/invalid-data']; + yield 'missing long url v2' => [[], '2', 'INVALID_ARGUMENT']; + yield 'missing long url v3' => [[], '3', 'https://shlink.io/api/error/invalid-data']; + yield 'empty long url v2' => [['longUrl' => null], '2', 'INVALID_ARGUMENT']; + yield 'empty long url v3' => [['longUrl' => ' '], '3', 'https://shlink.io/api/error/invalid-data']; + yield 'empty device long url v2' => [[ + 'longUrl' => 'foo', + 'deviceLongUrls' => [ + 'android' => null, + ], + ], '2', 'INVALID_ARGUMENT']; + yield 'empty device long url v3' => [[ + 'longUrl' => 'foo', + 'deviceLongUrls' => [ + 'ios' => ' ', + ], + ], '3', 'https://shlink.io/api/error/invalid-data']; } /** @test */ @@ -361,6 +377,22 @@ class CreateShortUrlTest extends ApiTestCase self::assertEquals('http://s.test/🦣🦣🦣', $payload['shortUrl']); } + /** @test */ + public function canCreateShortUrlsWithDeviceLongUrls(): void + { + [$statusCode, $payload] = $this->createShortUrl([ + 'longUrl' => 'https://github.com/shlinkio/shlink/issues/1557', + 'deviceLongUrls' => [ + 'ios' => 'https://github.com/shlinkio/shlink/ios', + 'android' => 'https://github.com/shlinkio/shlink/android', + ], + ]); + + self::assertEquals(self::STATUS_OK, $statusCode); + self::assertEquals('https://github.com/shlinkio/shlink/ios', $payload['deviceLongUrls']['ios'] ?? null); + self::assertEquals('https://github.com/shlinkio/shlink/android', $payload['deviceLongUrls']['android'] ?? null); + } + /** * @return array{int, array} */ From b18c9e495fa5260eb5f756f764556962171d326f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 11:47:45 +0100 Subject: [PATCH 17/21] Add API test for short URL edition with device long URLs --- .../Rest/test-api/Action/EditShortUrlTest.php | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/module/Rest/test-api/Action/EditShortUrlTest.php b/module/Rest/test-api/Action/EditShortUrlTest.php index fefbdcba..74fdebc5 100644 --- a/module/Rest/test-api/Action/EditShortUrlTest.php +++ b/module/Rest/test-api/Action/EditShortUrlTest.php @@ -154,7 +154,7 @@ class EditShortUrlTest extends ApiTestCase $editResp = $this->callApiWithKey(self::METHOD_PATCH, (string) $url, [RequestOptions::JSON => [ 'maxVisits' => 100, ]]); - $editedShortUrl = $this->getJsonResponsePayload($this->callApiWithKey(self::METHOD_GET, (string) $url)); + $editedShortUrl = $this->getJsonResponsePayload($editResp); self::assertEquals(self::STATUS_OK, $editResp->getStatusCode()); self::assertEquals($domain, $editedShortUrl['domain']); @@ -170,4 +170,27 @@ class EditShortUrlTest extends ApiTestCase ]; yield 'no domain' => [null, 'https://shlink.io/documentation/']; } + + /** @test */ + public function deviceLongUrlsCanBeEdited(): void + { + $shortCode = 'def456'; + $url = new Uri(sprintf('/short-urls/%s', $shortCode)); + $editResp = $this->callApiWithKey(self::METHOD_PATCH, (string) $url, [RequestOptions::JSON => [ + 'deviceLongUrls' => [ + 'android' => null, // This one will get removed + 'ios' => 'https://blog.alejandrocelaya.com/ios/edited', // This one will be edited + 'desktop' => 'https://blog.alejandrocelaya.com/desktop', // This one is new and will be created + ], + ]]); + $deviceLongUrls = $this->getJsonResponsePayload($editResp)['deviceLongUrls'] ?? []; + + self::assertEquals(self::STATUS_OK, $editResp->getStatusCode()); + self::assertArrayHasKey('ios', $deviceLongUrls); + self::assertEquals('https://blog.alejandrocelaya.com/ios/edited', $deviceLongUrls['ios']); + self::assertArrayHasKey('desktop', $deviceLongUrls); + self::assertEquals('https://blog.alejandrocelaya.com/desktop', $deviceLongUrls['desktop']); + self::assertArrayHasKey('android', $deviceLongUrls); + self::assertNull($deviceLongUrls['android']); + } } From 5aa8de11f492147e474a3575dfea48809435c55a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 12:00:16 +0100 Subject: [PATCH 18/21] =?UTF-8?q?Update=20version=20on=20user=20agent=20us?= =?UTF-8?q?ed=20to=20validate=20URLs=C3=A7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- module/Core/src/Util/UrlValidator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index 0057660a..8a7d0b89 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -26,7 +26,7 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface { private const MAX_REDIRECTS = 15; private const CHROME_USER_AGENT = 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) ' - . 'Chrome/51.0.2704.103 Safari/537.36'; + . 'Chrome/108.0.0.0 Safari/537.36'; public function __construct(private ClientInterface $httpClient, private UrlShortenerOptions $options) { From b0b9902f404a60a5112ef2ab7255dc4be0137c98 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 12:15:29 +0100 Subject: [PATCH 19/21] Add unit test to cover device URLs edition, and fix bug thanks to it --- module/Core/src/ShortUrl/Entity/ShortUrl.php | 5 ++- .../test/ShortUrl/Entity/ShortUrlTest.php | 44 +++++++++++++++++++ .../test-api/Action/CreateShortUrlTest.php | 2 +- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 9b266b4a..d0e9cba4 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -174,12 +174,13 @@ class ShortUrl extends AbstractEntity $this->deviceLongUrls->remove($deviceType->value); } foreach ($shortUrlEdit->deviceLongUrls as $deviceLongUrlPair) { - $deviceLongUrl = $this->deviceLongUrls->get($deviceLongUrlPair->deviceType->value); + $key = $deviceLongUrlPair->deviceType->value; + $deviceLongUrl = $this->deviceLongUrls->get($key); if ($deviceLongUrl !== null) { $deviceLongUrl->updateLongUrl($deviceLongUrlPair->longUrl); } else { - $this->deviceLongUrls->add(DeviceLongUrl::fromShortUrlAndPair($this, $deviceLongUrlPair)); + $this->deviceLongUrls->set($key, DeviceLongUrl::fromShortUrlAndPair($this, $deviceLongUrlPair)); } } } diff --git a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php index d83ff3d7..2d950d5f 100644 --- a/module/Core/test/ShortUrl/Entity/ShortUrlTest.php +++ b/module/Core/test/ShortUrl/Entity/ShortUrlTest.php @@ -7,8 +7,10 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Entity; use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\ShortCodeCannotBeRegeneratedException; +use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlEdition; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; use Shlinkio\Shlink\Importer\Sources\ImportSource; @@ -89,4 +91,46 @@ class ShortUrlTest extends TestCase yield [null, DEFAULT_SHORT_CODES_LENGTH]; yield from map(range(4, 10), fn (int $value) => [$value, $value]); } + + /** @test */ + public function deviceLongUrlsAreUpdated(): void + { + $shortUrl = ShortUrl::withLongUrl('foo'); + + $shortUrl->update(ShortUrlEdition::fromRawData([ + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::ANDROID->value => 'android', + DeviceType::IOS->value => 'ios', + ], + ])); + self::assertEquals([ + DeviceType::ANDROID->value => 'android', + DeviceType::IOS->value => 'ios', + DeviceType::DESKTOP->value => null, + ], $shortUrl->deviceLongUrls()); + + $shortUrl->update(ShortUrlEdition::fromRawData([ + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::ANDROID->value => null, + DeviceType::DESKTOP->value => 'desktop', + ], + ])); + self::assertEquals([ + DeviceType::ANDROID->value => null, + DeviceType::IOS->value => 'ios', + DeviceType::DESKTOP->value => 'desktop', + ], $shortUrl->deviceLongUrls()); + + $shortUrl->update(ShortUrlEdition::fromRawData([ + ShortUrlInputFilter::DEVICE_LONG_URLS => [ + DeviceType::ANDROID->value => null, + DeviceType::IOS->value => null, + ], + ])); + self::assertEquals([ + DeviceType::ANDROID->value => null, + DeviceType::IOS->value => null, + DeviceType::DESKTOP->value => 'desktop', + ], $shortUrl->deviceLongUrls()); + } } diff --git a/module/Rest/test-api/Action/CreateShortUrlTest.php b/module/Rest/test-api/Action/CreateShortUrlTest.php index 4300c852..0bb02c9e 100644 --- a/module/Rest/test-api/Action/CreateShortUrlTest.php +++ b/module/Rest/test-api/Action/CreateShortUrlTest.php @@ -178,7 +178,7 @@ class CreateShortUrlTest extends ApiTestCase 'tags' => ['boo', 'far'], 'validSince' => Chronos::now()->toAtomString(), 'maxVisits' => 7, - ] + ], ]; } From 9949bb654d799f6acf34764f068692682d20789e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 12:35:07 +0100 Subject: [PATCH 20/21] Set more accurate swagger docs in terms of what props are required/nullable for device long URLs --- docs/swagger/definitions/DeviceLongUrls.json | 6 +++--- .../swagger/definitions/DeviceLongUrlsEdit.json | 17 +++++++++++++++++ .../swagger/definitions/DeviceLongUrlsResp.json | 2 +- docs/swagger/definitions/ShortUrlEdition.json | 2 +- docs/swagger/paths/v1_short-urls.json | 3 +++ docs/swagger/paths/v1_short-urls_shorten.json | 2 +- 6 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 docs/swagger/definitions/DeviceLongUrlsEdit.json diff --git a/docs/swagger/definitions/DeviceLongUrls.json b/docs/swagger/definitions/DeviceLongUrls.json index 25e7f322..1a56d9ef 100644 --- a/docs/swagger/definitions/DeviceLongUrls.json +++ b/docs/swagger/definitions/DeviceLongUrls.json @@ -4,17 +4,17 @@ "android": { "description": "The long URL to redirect to when the short URL is visited from a device running Android", "type": "string", - "nullable": true + "nullable": false }, "ios": { "description": "The long URL to redirect to when the short URL is visited from a device running iOS", "type": "string", - "nullable": true + "nullable": false }, "desktop": { "description": "The long URL to redirect to when the short URL is visited from a desktop browser", "type": "string", - "nullable": true + "nullable": false } } } diff --git a/docs/swagger/definitions/DeviceLongUrlsEdit.json b/docs/swagger/definitions/DeviceLongUrlsEdit.json new file mode 100644 index 00000000..78f77e46 --- /dev/null +++ b/docs/swagger/definitions/DeviceLongUrlsEdit.json @@ -0,0 +1,17 @@ +{ + "type": "object", + "allOf": [{ + "$ref": "./DeviceLongUrls.json" + }], + "properties": { + "android": { + "nullable": true + }, + "ios": { + "nullable": true + }, + "desktop": { + "nullable": true + } + } +} diff --git a/docs/swagger/definitions/DeviceLongUrlsResp.json b/docs/swagger/definitions/DeviceLongUrlsResp.json index 64a56c01..95724581 100644 --- a/docs/swagger/definitions/DeviceLongUrlsResp.json +++ b/docs/swagger/definitions/DeviceLongUrlsResp.json @@ -2,6 +2,6 @@ "type": "object", "required": ["android", "ios", "desktop"], "allOf": [{ - "$ref": "./DeviceLongUrls.json" + "$ref": "./DeviceLongUrlsEdit.json" }] } diff --git a/docs/swagger/definitions/ShortUrlEdition.json b/docs/swagger/definitions/ShortUrlEdition.json index 7a9aca7b..28fa71bc 100644 --- a/docs/swagger/definitions/ShortUrlEdition.json +++ b/docs/swagger/definitions/ShortUrlEdition.json @@ -6,7 +6,7 @@ "type": "string" }, "deviceLongUrls": { - "$ref": "./DeviceLongUrls.json" + "$ref": "./DeviceLongUrlsEdit.json" }, "validSince": { "description": "The date (in ISO-8601 format) from which this short code will be valid", diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index 12afe6f4..76d87659 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -296,6 +296,9 @@ "type": "object", "required": ["longUrl"], "properties": { + "deviceLongUrls": { + "$ref": "../definitions/DeviceLongUrls.json" + }, "customSlug": { "description": "A unique custom slug to be used instead of the generated short code", "type": "string" diff --git a/docs/swagger/paths/v1_short-urls_shorten.json b/docs/swagger/paths/v1_short-urls_shorten.json index bf2889e5..cacb00bb 100644 --- a/docs/swagger/paths/v1_short-urls_shorten.json +++ b/docs/swagger/paths/v1_short-urls_shorten.json @@ -6,7 +6,7 @@ "Short URLs" ], "summary": "Create a short URL", - "description": "Creates a short URL in a single API call. Useful for third party integrations.", + "description": "**[Deprecated]** Use [Create short URL](#/Short%20URLs/createShortUrl) instead", "parameters": [ { "$ref": "../parameters/version.json" From 81393a76b4c4f52b671798e663efcfffb9069fda Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 22 Jan 2023 12:43:03 +0100 Subject: [PATCH 21/21] Ensure GITHUB_TOKEN is exposed to roadrunner api tests workflow --- .github/workflows/ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a95f8f21..2ccb5ae7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,8 @@ jobs: strategy: matrix: php-version: ['8.1', '8.2'] + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # rr get-binary picks this env automatically steps: - uses: actions/checkout@v3 - run: docker-compose -f docker-compose.yml -f docker-compose.ci.yml up -d shlink_db_postgres