From b4450d17f4726048bd495c70fe1b2c5c64fae328 Mon Sep 17 00:00:00 2001 From: Alexander Schranz Date: Thu, 11 Aug 2022 10:55:49 +0200 Subject: [PATCH] Add LockMiddleware (#2) * Add LockMiddleware * Fix phpstan and code coverage * Fix php-cs * Fix rector and php-cs-fixer * Remove readonly * Fix PHP 8.0 compatibility --- .php-cs-fixer.dist.php | 4 + README.md | 19 ++++ composer.json | 23 +++-- config/services.php | 5 ++ phpstan.neon | 10 --- rector.php | 58 ++++++++----- .../HttpKernel/SuluMessengerBundle.php | 6 +- .../DoctrineFlushMiddleware.php | 2 +- .../LockMiddleware/LockMiddleware.php | 47 ++++++++++ .../Messenger/LockMiddleware/LockStamp.php | 35 ++++++++ tests/Traits/PrivatePropertyTrait.php | 17 ++-- .../DoctrineFlushMiddlewareTest.php | 9 +- .../LockMiddleware/LockMiddlewareTest.php | 86 +++++++++++++++++++ .../LockMiddleware/LockStampTest.php | 43 ++++++++++ .../UnpackExceptionMiddlewareTest.php | 14 +-- tests/bootstrap.php | 4 +- tests/phpstan/object-manager.php | 2 +- 17 files changed, 320 insertions(+), 64 deletions(-) create mode 100644 src/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddleware.php create mode 100644 src/Infrastructure/Symfony/Messenger/LockMiddleware/LockStamp.php create mode 100644 tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddlewareTest.php create mode 100644 tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockStampTest.php diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php index b31feeb..3b48972 100644 --- a/.php-cs-fixer.dist.php +++ b/.php-cs-fixer.dist.php @@ -39,5 +39,9 @@ 'multiline_whitespace_before_semicolons' => true, 'single_line_throw' => false, 'visibility_required' => ['elements' => ['property', 'method', 'const']], + 'phpdoc_to_comment' => [ + 'ignored_tags' => ['todo', 'var', 'see'], + ], + 'trailing_comma_in_multiline' => ['elements' => ['arrays', 'arguments', 'parameters']], ]) ->setFinder($finder); diff --git a/README.md b/README.md index 62d7ccc..385c7b7 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,25 @@ This way we make sure that the real exception is thrown out by this message bus, and a controller can catch or convert it to a specific http status code. This middleware is always activated in the sulu message bus. +### LockMiddleware + +The `LockMiddleware` will allow to lock specific resources by a given key. This is commonly +used to prevent concurrent access to the same resource and avoid race conditions. +The locking can be activated and controlled via the `LockStamp` which supports the same parameters +as the Symfony `LockFactory` to create the Lock. + +```php +use Sulu\Messenger\Infrastructure\Symfony\Messenger\LockMiddleware\LockStamp; + +$this->handle(new Envelope(new YourMessage(), [new LockStamp('lock-key')])); + +# set ttl and autorelease +$this->handle(new Envelope(new YourMessage(), [new LockStamp('lock-key', 300.0, true)])); + +# multiple locks possible all locks need to be acquired before processing the message +$this->handle(new Envelope(new YourMessage(), [new LockStamp('lock-key-1'), new LockStamp('lock-key-2')])); +``` + ### DoctrineFlushMiddleware The `DoctrineFlushMiddleware` is a Middleware which let us flush the Doctrine diff --git a/composer.json b/composer.json index eac7612..7bbb0f0 100644 --- a/composer.json +++ b/composer.json @@ -13,6 +13,7 @@ "symfony/doctrine-bridge": "^5.4 || ^6.0", "symfony/framework-bundle": "^5.4 || ^6.0", "symfony/http-kernel": "^5.4 || ^6.0", + "symfony/lock": "^5.4 || ^6.0", "symfony/messenger": "^5.4 || ^6.0" }, "require-dev": { @@ -22,23 +23,24 @@ "jangregor/phpstan-prophecy": "^1.0", "nunomaduro/collision": "^5.0 || ^6.0", "phpspec/prophecy-phpunit": "^2.0", + "phpstan/extension-installer": "^1.1", "phpstan/phpstan": "^1.4", "phpstan/phpstan-doctrine": "^1.2", "phpstan/phpstan-phpunit": "^1.0", "phpstan/phpstan-symfony": "^1.1", "phpstan/phpstan-webmozart-assert": "^1.0", "phpunit/phpunit": "^9.5", - "qossmic/deptrac-shim": "^0.19.3", - "rector/rector": "^0.12.16", - "schranz/test-generator": "^0.3", + "qossmic/deptrac-shim": "^0.23.0", + "rector/rector": "^0.13.10", + "schranz/test-generator": "^0.4", "symfony/browser-kit": "^5.4 || ^6.0", "symfony/css-selector": "^5.4 || ^6.0", "symfony/debug-bundle": "^5.4 || ^6.0", + "symfony/dotenv": "^5.4 || ^6.0", "symfony/error-handler": "^5.4 || ^6.0", "symfony/phpunit-bridge": "^5.4 || ^6.0", - "thecodingmachine/phpstan-strict-rules": "^1.0", - "symfony/dotenv": "^5.4 || ^6.0", - "symfony/yaml": "^5.4 || ^6.0" + "symfony/yaml": "^5.4 || ^6.0", + "thecodingmachine/phpstan-strict-rules": "^1.0" }, "autoload": { "psr-4": { @@ -85,6 +87,10 @@ "rector": [ "@php vendor/bin/rector process" ], + "fix": [ + "@rector", + "@php-cs-fix" + ], "php-cs": "@php vendor/bin/php-cs-fixer fix --verbose --diff --dry-run", "php-cs-fix": "@php vendor/bin/php-cs-fixer fix", "lint-composer": "@composer validate --no-check-publish --strict", @@ -100,6 +106,9 @@ ] }, "config": { - "sort-packages": true + "sort-packages": true, + "allow-plugins": { + "phpstan/extension-installer": true + } } } diff --git a/config/services.php b/config/services.php index 1c33c75..f77da90 100644 --- a/config/services.php +++ b/config/services.php @@ -5,6 +5,7 @@ namespace Symfony\Component\DependencyInjection\Loader\Configurator; use Sulu\Messenger\Infrastructure\Symfony\Messenger\FlushMiddleware\DoctrineFlushMiddleware; +use Sulu\Messenger\Infrastructure\Symfony\Messenger\LockMiddleware\LockMiddleware; use Sulu\Messenger\Infrastructure\Symfony\Messenger\UnpackExceptionMiddleware\UnpackExceptionMiddleware; return function (ContainerConfigurator $configurator) { @@ -16,4 +17,8 @@ $services->set('sulu_messenger.unpack_exception_middleware') ->class(UnpackExceptionMiddleware::class); + + $services->set('sulu_messenger.lock_middleware') + ->class(LockMiddleware::class) + ->args([service('lock.factory')]); }; diff --git a/phpstan.neon b/phpstan.neon index f6b4b5a..4c5da31 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,13 +1,3 @@ -includes: - - vendor/jangregor/phpstan-prophecy/extension.neon - - vendor/phpstan/phpstan-symfony/extension.neon - - vendor/phpstan/phpstan-doctrine/extension.neon - - vendor/phpstan/phpstan-doctrine/rules.neon - - vendor/phpstan/phpstan-phpunit/extension.neon - - vendor/phpstan/phpstan-phpunit/rules.neon - - vendor/phpstan/phpstan-webmozart-assert/extension.neon - - vendor/thecodingmachine/phpstan-strict-rules/phpstan-strict-rules.neon - parameters: paths: - src diff --git a/rector.php b/rector.php index eb75dca..df1c151 100644 --- a/rector.php +++ b/rector.php @@ -2,42 +2,54 @@ declare(strict_types=1); -use Rector\Core\Configuration\Option; +use Rector\Config\RectorConfig; use Rector\Doctrine\Set\DoctrineSetList; +use Rector\PHPUnit\Set\PHPUnitLevelSetList; +use Rector\PHPUnit\Set\PHPUnitSetList; use Rector\Set\ValueObject\LevelSetList; use Rector\Set\ValueObject\SetList; use Rector\Symfony\Set\SymfonyLevelSetList; use Rector\Symfony\Set\SymfonySetList; -use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; -return static function (ContainerConfigurator $containerConfigurator): void { - $parameters = $containerConfigurator->parameters(); - $services = $containerConfigurator->services(); +return static function (RectorConfig $rectorConfig): void { + $rectorConfig->paths([ + __DIR__ . '/src', + __DIR__ . '/tests', + ]); - $parameters->set(Option::PATHS, [__DIR__ . '/src', __DIR__ . '/tests']); - $parameters->set(Option::SKIP, [ + $rectorConfig->skip([ __DIR__ . '/tests/Application/var', __DIR__ . '/tests/Application/config', ]); - $parameters->set(Option::PHPSTAN_FOR_RECTOR_PATH, __DIR__ . '/phpstan.neon'); + + $rectorConfig->phpstanConfig(__DIR__ . '/phpstan.neon'); // basic rules - $parameters->set(Option::AUTO_IMPORT_NAMES, true); - $parameters->set(Option::IMPORT_DOC_BLOCKS, true); - $parameters->set(Option::IMPORT_SHORT_CLASSES, false); + $rectorConfig->importNames(); + $rectorConfig->importShortClasses(); - $containerConfigurator->import(SetList::CODE_QUALITY); - $containerConfigurator->import(LevelSetList::UP_TO_PHP_80); + $rectorConfig->sets([ + SetList::CODE_QUALITY, + LevelSetList::UP_TO_PHP_80, + ]); // symfony rules - $parameters = $containerConfigurator->parameters(); - $parameters->set( - Option::SYMFONY_CONTAINER_XML_PATH_PARAMETER, - __DIR__ . '/tests/Application/var/cache/dev/Sulu_Messenger_Tests_Application_KernelDevDebugContainer.xml' - ); - $containerConfigurator->import(SymfonySetList::SYMFONY_CODE_QUALITY); - $containerConfigurator->import(SymfonySetList::SYMFONY_CONSTRUCTOR_INJECTION); - $containerConfigurator->import(SymfonyLevelSetList::UP_TO_SYMFONY_54); - - $containerConfigurator->import(DoctrineSetList::DOCTRINE_CODE_QUALITY); + $rectorConfig->symfonyContainerPhp(__DIR__ . '/tests/Application/var/cache/dev/Sulu_Messenger_Tests_Application_KernelDevDebugContainer.xml'); + + $rectorConfig->sets([ + SymfonySetList::SYMFONY_CODE_QUALITY, + SymfonySetList::SYMFONY_CONSTRUCTOR_INJECTION, + SymfonyLevelSetList::UP_TO_SYMFONY_54, + ]); + + // doctrine rules + $rectorConfig->sets([ + DoctrineSetList::DOCTRINE_CODE_QUALITY, + ]); + + // phpunit rules + $rectorConfig->sets([ + PHPUnitLevelSetList::UP_TO_PHPUNIT_90, + PHPUnitSetList::PHPUNIT_91, + ]); }; diff --git a/src/Infrastructure/Symfony/HttpKernel/SuluMessengerBundle.php b/src/Infrastructure/Symfony/HttpKernel/SuluMessengerBundle.php index 2dfd709..dc418ca 100644 --- a/src/Infrastructure/Symfony/HttpKernel/SuluMessengerBundle.php +++ b/src/Infrastructure/Symfony/HttpKernel/SuluMessengerBundle.php @@ -4,6 +4,7 @@ namespace Sulu\Messenger\Infrastructure\Symfony\HttpKernel; +use RuntimeException; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; use Symfony\Component\Config\FileLocator; @@ -52,7 +53,7 @@ public function getConfiguration(array $config, ContainerBuilder $container): Co public function prepend(ContainerBuilder $container): void { if (!$container->hasExtension('framework')) { - throw new \RuntimeException(\sprintf('The "%s" bundle requires "framework" bundle.', self::ALIAS)); + throw new RuntimeException(\sprintf('The "%s" bundle requires "framework" bundle.', self::ALIAS)); } $container->prependExtensionConfig( @@ -64,12 +65,13 @@ public function prepend(ContainerBuilder $container): void 'sulu_message_bus' => [ 'middleware' => [ 'sulu_messenger.unpack_exception_middleware', + 'sulu_messenger.lock_middleware', 'sulu_messenger.doctrine_flush_middleware', ], ], ], ], - ] + ], ); } diff --git a/src/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddleware.php b/src/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddleware.php index bebc6ff..442d9d0 100644 --- a/src/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddleware.php +++ b/src/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddleware.php @@ -12,7 +12,7 @@ class DoctrineFlushMiddleware implements MiddlewareInterface { public function __construct( - private EntityManagerInterface $entityManager + private EntityManagerInterface $entityManager, ) { } diff --git a/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddleware.php b/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddleware.php new file mode 100644 index 0000000..d35cf7b --- /dev/null +++ b/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddleware.php @@ -0,0 +1,47 @@ +all(LockStamp::class); + + foreach ($lockStamps as $lockStamp) { + $lock = $this->lockFactory->createLock( + $lockStamp->getResource(), + $lockStamp->getTtl(), + $lockStamp->getAutoRelease(), + ); + + $lock->acquire(true); + $locks[] = $lock; + } + + $envelope = $stack->next()->handle($envelope, $stack); + } finally { + foreach ($locks as $lock) { + $lock->release(); + } + } + + return $envelope; + } +} diff --git a/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockStamp.php b/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockStamp.php new file mode 100644 index 0000000..ab9ea60 --- /dev/null +++ b/src/Infrastructure/Symfony/Messenger/LockMiddleware/LockStamp.php @@ -0,0 +1,35 @@ +resource; + } + + public function getTtl(): ?float + { + return $this->ttl; + } + + public function getAutoRelease(): bool + { + return $this->autoRelease; + } +} diff --git a/tests/Traits/PrivatePropertyTrait.php b/tests/Traits/PrivatePropertyTrait.php index b4d7b8a..0678a6d 100644 --- a/tests/Traits/PrivatePropertyTrait.php +++ b/tests/Traits/PrivatePropertyTrait.php @@ -4,6 +4,10 @@ namespace Sulu\Messenger\Tests\Traits; +use ReflectionClass; +use ReflectionException; +use ReflectionProperty; + trait PrivatePropertyTrait { /** @@ -11,23 +15,20 @@ trait PrivatePropertyTrait */ protected static function getPrivateProperty(object $object, string $propertyName) { - $reflection = new \ReflectionClass($object); + $reflection = new ReflectionClass($object); $propertyReflection = $reflection->getProperty($propertyName); $propertyReflection->setAccessible(true); return $propertyReflection->getValue($object); } - /** - * @param mixed $value - */ - protected static function setPrivateProperty(object $object, string $propertyName, $value): void + protected static function setPrivateProperty(object $object, string $propertyName, mixed $value): void { - $reflection = new \ReflectionClass($object); + $reflection = new ReflectionClass($object); try { $propertyReflection = $reflection->getProperty($propertyName); self::setValue($propertyReflection, $object, $value); - } catch (\ReflectionException) { + } catch (ReflectionException) { $parent = $reflection->getParentClass(); if ($parent) { $propertyReflection = $parent->getProperty($propertyName); @@ -36,7 +37,7 @@ protected static function setPrivateProperty(object $object, string $propertyNam } } - private static function setValue(\ReflectionProperty $propertyReflection, object $object, mixed $value): void + private static function setValue(ReflectionProperty $propertyReflection, object $object, mixed $value): void { $propertyReflection->setAccessible(true); diff --git a/tests/Unit/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddlewareTest.php b/tests/Unit/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddlewareTest.php index b719780..3e95d93 100644 --- a/tests/Unit/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddlewareTest.php +++ b/tests/Unit/Infrastructure/Symfony/Messenger/FlushMiddleware/DoctrineFlushMiddlewareTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use stdClass; use Sulu\Messenger\Infrastructure\Symfony\Messenger\FlushMiddleware\DoctrineFlushMiddleware; use Sulu\Messenger\Infrastructure\Symfony\Messenger\FlushMiddleware\EnableFlushStamp; use Symfony\Component\Messenger\Envelope; @@ -31,7 +32,7 @@ protected function setUp(): void { $this->entityManager = $this->prophesize(EntityManagerInterface::class); $this->middleware = new DoctrineFlushMiddleware( - $this->entityManager->reveal() + $this->entityManager->reveal(), ); } @@ -45,7 +46,7 @@ public function testHandleWithoutStamp(): void $this->assertSame( $envelope, - $this->middleware->handle($envelope, $stack) + $this->middleware->handle($envelope, $stack), ); } @@ -60,13 +61,13 @@ public function testHandleWithStamp(): void $this->assertSame( $envelope, - $this->middleware->handle($envelope, $stack) + $this->middleware->handle($envelope, $stack), ); } private function createEnvelope(): Envelope { - return new Envelope(new \stdClass()); + return new Envelope(new stdClass()); } private function createStack(): StackMiddleware diff --git a/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddlewareTest.php b/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddlewareTest.php new file mode 100644 index 0000000..82e5cba --- /dev/null +++ b/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockMiddlewareTest.php @@ -0,0 +1,86 @@ + + */ + private ObjectProphecy $lockFactory; + + protected function setUp(): void + { + $this->lockFactory = $this->prophesize(LockFactory::class); + $this->middleware = new LockMiddleware( + $this->lockFactory->reveal(), + ); + } + + public function testHandleWithoutStamp(): void + { + $envelope = $this->createEnvelope(); + $stack = $this->createStack(); + + $this->lockFactory->createLock(Argument::cetera()) + ->shouldNotBeCalled(); + + $this->assertSame( + $envelope, + $this->middleware->handle($envelope, $stack), + ); + } + + public function testHandleWithStamp(): void + { + $envelope = $this->createEnvelope(); + $envelope = $envelope->with(new LockStamp('test', 30, true)); + $stack = $this->createStack(); + + $lock = $this->prophesize(LockInterface::class); + $lock->acquire(true) + ->shouldBeCalled(); + $lock->release() + ->shouldBeCalled(); + + $this->lockFactory->createLock('test', 30, true) + ->willReturn($lock->reveal()) + ->shouldBeCalled(); + + $this->assertSame( + $envelope, + $this->middleware->handle($envelope, $stack), + ); + } + + private function createEnvelope(): Envelope + { + return new Envelope(new stdClass()); + } + + private function createStack(): StackMiddleware + { + return new StackMiddleware([]); + } +} diff --git a/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockStampTest.php b/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockStampTest.php new file mode 100644 index 0000000..9d5176e --- /dev/null +++ b/tests/Unit/Infrastructure/Symfony/Messenger/LockMiddleware/LockStampTest.php @@ -0,0 +1,43 @@ +createInstance(['resource' => 'Resource']); + $this->assertSame('Resource', $model->getResource()); + } + + public function testGetTtl(): void + { + $model = $this->createInstance(['ttl' => 30.0]); + $this->assertSame(30.0, $model->getTtl()); + } + + public function testGetAutoRelease(): void + { + $model = $this->createInstance(['autoRelease' => true]); + $this->assertTrue($model->getAutoRelease()); + } + + /** + * @param array{ + * resource?: string, + * ttl?: float|null, + * autoRelease?: bool, + * } $data + */ + public function createInstance(array $data = []): LockStamp + { + return new LockStamp($data['resource'] ?? 'Resource', $data['ttl'] ?? 300.0, $data['autoRelease'] ?? true); + } +} diff --git a/tests/Unit/Infrastructure/Symfony/Messenger/UnpackExceptionMiddleware/UnpackExceptionMiddlewareTest.php b/tests/Unit/Infrastructure/Symfony/Messenger/UnpackExceptionMiddleware/UnpackExceptionMiddlewareTest.php index 0cd4782..1f2ce21 100644 --- a/tests/Unit/Infrastructure/Symfony/Messenger/UnpackExceptionMiddleware/UnpackExceptionMiddlewareTest.php +++ b/tests/Unit/Infrastructure/Symfony/Messenger/UnpackExceptionMiddleware/UnpackExceptionMiddlewareTest.php @@ -4,7 +4,9 @@ namespace Sulu\Messenger\Tests\Unit\Common\Infrastructure\Symfony\Messenger\UnpackExceptionMiddleware; +use LogicException; use PHPUnit\Framework\TestCase; +use stdClass; use Sulu\Messenger\Infrastructure\Symfony\Messenger\UnpackExceptionMiddleware\UnpackExceptionMiddleware; use Symfony\Component\Messenger\Envelope; use Symfony\Component\Messenger\Exception\HandlerFailedException; @@ -31,29 +33,29 @@ public function testHandleDefault(): void $this->assertSame( $envelope, - $this->middleware->handle($envelope, $stack) + $this->middleware->handle($envelope, $stack), ); } public function testHandleException(): void { - $this->expectException(\LogicException::class); + $this->expectException(LogicException::class); $this->expectExceptionMessage('Specific unpacked exception.'); $envelope = $this->createEnvelope(); - $stack = $this->createStack(function () use ($envelope) { - throw new HandlerFailedException($envelope, [new \LogicException('Specific unpacked exception.')]); + $stack = $this->createStack(function () use ($envelope): never { + throw new HandlerFailedException($envelope, [new LogicException('Specific unpacked exception.')]); }); $this->assertSame( $envelope, - $this->middleware->handle($envelope, $stack) + $this->middleware->handle($envelope, $stack), ); } private function createEnvelope(): Envelope { - return new Envelope(new \stdClass()); + return new Envelope(new stdClass()); } private function createStack(callable $handler = null): StackMiddleware diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 0a418b9..d303684 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -16,6 +16,6 @@ require \dirname(__DIR__) . '/vendor/autoload.php'; (new Dotenv())->bootEnv(__DIR__ . '/Application/.env'); -if (\class_exists(\Locale::class)) { - \Locale::setDefault('en'); +if (\class_exists(Locale::class)) { + Locale::setDefault('en'); } diff --git a/tests/phpstan/object-manager.php b/tests/phpstan/object-manager.php index 24e0e21..25f3234 100644 --- a/tests/phpstan/object-manager.php +++ b/tests/phpstan/object-manager.php @@ -23,7 +23,7 @@ // this is a workaround for the following phpstan issue: https://github.com/phpstan/phpstan-doctrine/issues/98 $resolveTargetEntityListener = \current(\array_filter( $objectManager->getEventManager()->getListeners('loadClassMetadata'), - static fn ($listener) => $listener instanceof ResolveTargetEntityListener + static fn ($listener) => $listener instanceof ResolveTargetEntityListener, )); if ($resolveTargetEntityListener) {