Merge pull request #1558 from acelaya-forks/feature/multisegment-trailing-slash

Feature/multisegment trailing slash

CHANGELOG.md

@@ -4,11 +4,12 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
-## [Unreleased]
+## [3.3.1] - 2022-09-30
 ### Added
-* [#1474](https://github.com/shlinkio/shlink/issues/1474) Added preliminary support for PHP 8.2 during CI workflow.
+* *Nothing*
 
 ### Changed
+* [#1474](https://github.com/shlinkio/shlink/issues/1474) Added preliminary support for PHP 8.2 during CI workflow.
 * [#1551](https://github.com/shlinkio/shlink/issues/1551) Moved services related to geolocating visits to the `Visit\Geolocation` namespace.
 * [#1550](https://github.com/shlinkio/shlink/issues/1550) Reorganized main namespaces from Core module.
 
@@ -19,7 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 * *Nothing*
 
 ### Fixed
-* *Nothing*
+* [#1556](https://github.com/shlinkio/shlink/issues/1556) Fixed trailing slash not working when enabling multi-segment slashes.
 
 
 ## [3.3.0] - 2022-09-18

config/autoload/routes.config.php

@@ -8,6 +8,7 @@ use Fig\Http\Message\RequestMethodInterface;
 use RKA\Middleware\IpAddress;
 use Shlinkio\Shlink\Core\Action as CoreAction;
 use Shlinkio\Shlink\Core\Config\EnvVars;
+use Shlinkio\Shlink\Core\ShortUrl\Middleware\TrimTrailingSlashMiddleware;
 use Shlinkio\Shlink\Rest\Action;
 use Shlinkio\Shlink\Rest\ConfigProvider;
 use Shlinkio\Shlink\Rest\Middleware;
@@ -19,6 +20,8 @@ return (static function (): array {
     $contentNegotiationMiddleware = Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class;
     $dropDomainMiddleware = Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class;
     $overrideDomainMiddleware = Middleware\ShortUrl\OverrideDomainMiddleware::class;
+
+    // TODO This should be based on config, not the env var
     $shortUrlRouteSuffix = EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(false) ? '[/]' : '';
 
     return [
@@ -97,6 +100,7 @@ return (static function (): array {
                 'path' => sprintf('/{shortCode}%s', $shortUrlRouteSuffix),
                 'middleware' => [
                     IpAddress::class,
+                    TrimTrailingSlashMiddleware::class,
                     CoreAction\RedirectAction::class,
                 ],
                 'allowed_methods' => [RequestMethodInterface::METHOD_GET],

config/autoload/url-shortener.global.php

@@ -24,6 +24,7 @@ return (static function (): array {
             'auto_resolve_titles' => (bool) EnvVars::AUTO_RESOLVE_TITLES->loadFromEnv(false),
             'append_extra_path' => (bool) EnvVars::REDIRECT_APPEND_EXTRA_PATH->loadFromEnv(false),
             'multi_segment_slugs_enabled' => (bool) EnvVars::MULTI_SEGMENT_SLUGS_ENABLED->loadFromEnv(false),
+            'trailing_slash_enabled' => (bool) EnvVars::SHORT_URL_TRAILING_SLASH->loadFromEnv(false),
         ],
 
     ];

config/autoload/url-shortener.local.php.dist

@@ -18,6 +18,7 @@ return [
         ],
         'auto_resolve_titles' => true,
 //        'multi_segment_slugs_enabled' => true,
+//        'trailing_slash_enabled' => true,
     ],
 
 ];

module/Core/config/dependencies.config.php

@@ -43,6 +43,7 @@ return [
             ShortUrl\Helper\ShortUrlRedirectionBuilder::class => ConfigAbstractFactory::class,
             ShortUrl\Transformer\ShortUrlDataTransformer::class => ConfigAbstractFactory::class,
             ShortUrl\Middleware\ExtraPathRedirectMiddleware::class => ConfigAbstractFactory::class,
+            ShortUrl\Middleware\TrimTrailingSlashMiddleware::class => ConfigAbstractFactory::class,
 
             Tag\TagService::class => ConfigAbstractFactory::class,
 
@@ -154,6 +155,7 @@ return [
             Util\RedirectResponseHelper::class,
             Options\UrlShortenerOptions::class,
         ],
+        ShortUrl\Middleware\TrimTrailingSlashMiddleware::class => [Options\UrlShortenerOptions::class],
 
         EventDispatcher\PublishingUpdatesGenerator::class => [
             ShortUrl\Transformer\ShortUrlDataTransformer::class,

module/Core/src/Options/UrlShortenerOptions.php

@@ -15,6 +15,7 @@ final class UrlShortenerOptions
         public readonly bool $autoResolveTitles = false,
         public readonly bool $appendExtraPath = false,
         public readonly bool $multiSegmentSlugsEnabled = false,
+        public readonly bool $trailingSlashEnabled = false,
     ) {
     }
 }

module/Core/src/ShortUrl/Middleware/TrimTrailingSlashMiddleware.php

@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\ShortUrl\Middleware;
+
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+
+use function rtrim;
+
+class TrimTrailingSlashMiddleware implements MiddlewareInterface
+{
+    private const SHORT_CODE_ATTR = 'shortCode';
+
+    public function __construct(private readonly UrlShortenerOptions $options)
+    {
+    }
+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    {
+        return $handler->handle($this->resolveRequest($request));
+    }
+
+    private function resolveRequest(ServerRequestInterface $request): ServerRequestInterface
+    {
+        // If multi-segment slugs are enabled together with trailing slashes, the "shortCode" attribute will include
+        // ending slashes that we need to trim for a proper short code matching
+
+        /** @var string|null $shortCode */
+        $shortCode = $request->getAttribute(self::SHORT_CODE_ATTR);
+        $shouldTrimSlash = $shortCode !== null && $this->options->trailingSlashEnabled;
+
+        return  $shouldTrimSlash ? $request->withAttribute(self::SHORT_CODE_ATTR, rtrim($shortCode, '/')) : $request;
+    }
+}

module/Core/test/ShortUrl/Middleware/TrimTrailingSlashMiddlewareTest.php

@@ -0,0 +1,85 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\ShortUrl\Middleware;
+
+use Laminas\Diactoros\Response;
+use Laminas\Diactoros\ServerRequestFactory;
+use PHPUnit\Framework\Assert;
+use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
+use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+use Shlinkio\Shlink\Core\ShortUrl\Middleware\TrimTrailingSlashMiddleware;
+
+use function Functional\compose;
+use function Functional\const_function;
+
+class TrimTrailingSlashMiddlewareTest extends TestCase
+{
+    use ProphecyTrait;
+
+    private ObjectProphecy $requestHandler;
+
+    protected function setUp(): void
+    {
+        $this->requestHandler = $this->prophesize(RequestHandlerInterface::class);
+    }
+
+    /**
+     * @test
+     * @dataProvider provideRequests
+     */
+    public function returnsExpectedResponse(
+        bool $trailingSlashEnabled,
+        ServerRequestInterface $inputRequest,
+        callable $assertions,
+    ): void {
+        $arg = compose($assertions, const_function(true));
+
+        $this->requestHandler->handle(Argument::that($arg))->willReturn(new Response());
+        $this->middleware($trailingSlashEnabled)->process($inputRequest, $this->requestHandler->reveal());
+    }
+
+    public function provideRequests(): iterable
+    {
+        yield 'trailing slash disabled' => [
+            false,
+            $inputReq = ServerRequestFactory::fromGlobals(),
+            function (ServerRequestInterface $request) use ($inputReq): void {
+                Assert::assertSame($inputReq, $request);
+            },
+        ];
+        yield 'trailing slash enabled without shortCode attr' => [
+            true,
+            $inputReq = ServerRequestFactory::fromGlobals(),
+            function (ServerRequestInterface $request) use ($inputReq): void {
+                Assert::assertSame($inputReq, $request);
+            },
+        ];
+        yield 'trailing slash enabled with null shortCode attr' => [
+            true,
+            $inputReq = ServerRequestFactory::fromGlobals()->withAttribute('shortCode', null),
+            function (ServerRequestInterface $request) use ($inputReq): void {
+                Assert::assertSame($inputReq, $request);
+            },
+        ];
+        yield 'trailing slash enabled with non-null shortCode attr' => [
+            true,
+            $inputReq = ServerRequestFactory::fromGlobals()->withAttribute('shortCode', 'foo//'),
+            function (ServerRequestInterface $request) use ($inputReq): void {
+                Assert::assertNotSame($inputReq, $request);
+                Assert::assertEquals('foo', $request->getAttribute('shortCode'));
+            },
+        ];
+    }
+
+    private function middleware(bool $trailingSlashEnabled = false): TrimTrailingSlashMiddleware
+    {
+        return new TrimTrailingSlashMiddleware(new UrlShortenerOptions(trailingSlashEnabled: $trailingSlashEnabled));
+    }
+}

module/Rest/src/Entity/ApiKey.php

@@ -148,7 +148,7 @@ class ApiKey extends AbstractEntity
 
         if ($this->hasRole($role)) {
             /** @var ApiKeyRole $apiKeyRole */
-            $apiKeyRole = $this->roles->get($role);
+            $apiKeyRole = $this->roles->get($role->value);
             $apiKeyRole->updateMeta($meta);
         } else {
             $apiKeyRole = new ApiKeyRole($roleDefinition->role, $roleDefinition->meta, $this);