From e91a3f1d29ecb07dca42535e796023d0db0fe7b5 Mon Sep 17 00:00:00 2001 From: Bartosz Kubicki Date: Fri, 21 Feb 2020 16:32:02 +0100 Subject: [PATCH 1/3] Passing arguments for queues as expected --- .../Config/QueueConfigItem/DataMapperTest.php | 60 +++++++++++++------ .../Config/QueueConfigItem/DataMapper.php | 45 +++++++++----- 2 files changed, 70 insertions(+), 35 deletions(-) diff --git a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php index cc5c4ac84440d..5fdf1db436fcd 100644 --- a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php +++ b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php @@ -1,29 +1,35 @@ configData = $this->createMock(Data::class); $this->communicationConfig = $this->createMock(CommunicationConfig::class); @@ -40,7 +49,12 @@ protected function setUp() $this->model = new DataMapper($this->configData, $this->communicationConfig, $this->queueNameBuilder); } - public function testGetMappedData() + /** + * @return void + * + * @throws LocalizedException + */ + public function testGetMappedData(): void { $data = [ 'ex01' => [ @@ -96,7 +110,9 @@ public function testGetMappedData() ['topic02', ['name' => 'topic02', 'is_synchronous' => false]], ]; - $this->communicationConfig->expects($this->exactly(2))->method('getTopic')->willReturnMap($communicationMap); + $this->communicationConfig->expects($this->exactly(2)) + ->method('getTopic') + ->willReturnMap($communicationMap); $this->configData->expects($this->once())->method('get')->willReturn($data); $this->queueNameBuilder->expects($this->once()) ->method('getQueueName') @@ -110,23 +126,27 @@ public function testGetMappedData() 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'some.queue--amqp' => [ 'name' => 'some.queue', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], ]; $this->assertEquals($expectedResult, $actualResult); } /** + * @return void + * + * @throws LocalizedException + * * @SuppressWarnings(PHPMD.ExcessiveMethodLength) */ - public function testGetMappedDataForWildcard() + public function testGetMappedDataForWildcard(): void { $data = [ 'ex01' => [ @@ -200,7 +220,9 @@ public function testGetMappedDataForWildcard() ->method('getTopic') ->with('topic01') ->willReturn(['name' => 'topic01', 'is_synchronous' => true]); - $this->communicationConfig->expects($this->any())->method('getTopics')->willReturn($communicationData); + $this->communicationConfig->expects($this->any()) + ->method('getTopics') + ->willReturn($communicationData); $this->configData->expects($this->once())->method('get')->willReturn($data); $this->queueNameBuilder->expects($this->any()) ->method('getQueueName') @@ -215,49 +237,49 @@ public function testGetMappedDataForWildcard() 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'some.queue--amqp' => [ 'name' => 'some.queue', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'responseQueue.topic02--amqp' => [ 'name' => 'responseQueue.topic02', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'responseQueue.topic03--amqp' => [ 'name' => 'responseQueue.topic03', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'responseQueue.topic04.04.04--amqp' => [ 'name' => 'responseQueue.topic04.04.04', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'responseQueue.topic05.05--amqp' => [ 'name' => 'responseQueue.topic05.05', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ], 'responseQueue.topic08.part2.some.test--amqp' => [ 'name' => 'responseQueue.topic08.part2.some.test', 'connection' => 'amqp', 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => ['some' => 'arguments'], ] ]; $this->assertEquals($expectedResult, $actualResult); diff --git a/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php b/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php index 7e8d35fb0940f..627dca68d14a4 100644 --- a/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php +++ b/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php @@ -1,14 +1,17 @@ mappedData) { $this->mappedData = []; @@ -68,12 +72,18 @@ public function getMappedData() $connection = $exchange['connection']; foreach ($exchange['bindings'] as $binding) { if ($binding['destinationType'] === 'queue') { - $queueItems = $this->createQueueItems($binding['destination'], $binding['topic'], $connection); - $this->mappedData = array_merge($this->mappedData, $queueItems); + $queueItems = $this->createQueueItems( + (string) $binding['destination'], + (string) $binding['topic'], + (array) $binding['arguments'], + (string) $connection + ); + $this->mappedData += $queueItems; } } } } + return $this->mappedData; } @@ -82,10 +92,12 @@ public function getMappedData() * * @param string $name * @param string $topic + * @param array $arguments * @param string $connection * @return array + * @throws LocalizedException */ - private function createQueueItems($name, $topic, $connection) + private function createQueueItems(string $name, string $topic, array $arguments, string $connection): array { $output = []; $synchronousTopics = []; @@ -103,7 +115,7 @@ private function createQueueItems($name, $topic, $connection) 'connection' => $connection, 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => $arguments, ]; } @@ -112,8 +124,9 @@ private function createQueueItems($name, $topic, $connection) 'connection' => $connection, 'durable' => true, 'autoDelete' => false, - 'arguments' => [], + 'arguments' => $arguments, ]; + return $output; } @@ -124,15 +137,14 @@ private function createQueueItems($name, $topic, $connection) * @return bool * @throws LocalizedException */ - private function isSynchronousTopic($topicName) + private function isSynchronousTopic(string $topicName): bool { try { $topic = $this->communicationConfig->getTopic($topicName); - $isSync = (bool)$topic[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; - } catch (LocalizedException $e) { + return (bool) $topic[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; + } catch (LocalizedException $exception) { throw new LocalizedException(new Phrase('Error while checking if topic is synchronous')); } - return $isSync; } /** @@ -141,22 +153,24 @@ private function isSynchronousTopic($topicName) * @param string $wildcard * @return array */ - private function matchSynchronousTopics($wildcard) + private function matchSynchronousTopics(string $wildcard): array { $topicDefinitions = array_filter( $this->communicationConfig->getTopics(), function ($item) { - return (bool)$item[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; + return (bool) $item[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; } ); $topics = []; $pattern = $this->buildWildcardPattern($wildcard); + foreach (array_keys($topicDefinitions) as $topicName) { if (preg_match($pattern, $topicName)) { $topics[$topicName] = $topicName; } } + return $topics; } @@ -166,11 +180,10 @@ function ($item) { * @param string $wildcardKey * @return string */ - private function buildWildcardPattern($wildcardKey) + private function buildWildcardPattern(string $wildcardKey): string { $pattern = '/^' . str_replace('.', '\.', $wildcardKey); - $pattern = str_replace('#', '.+', $pattern); - $pattern = str_replace('*', '[^\.]+', $pattern); + $pattern = str_replace(['#', '*'], ['.+', '[^\.]+'], $pattern); $pattern .= strpos($wildcardKey, '#') === strlen($wildcardKey) ? '/' : '$/'; return $pattern; } From 00a03e958a24a2d7872b4510be8544acef6af813 Mon Sep 17 00:00:00 2001 From: Bartosz Kubicki Date: Sat, 3 Oct 2020 10:03:32 +0200 Subject: [PATCH 2/3] Fixes after CR --- .../Config/QueueConfigItem/DataMapperTest.php | 33 ++++++++++--------- .../Config/QueueConfigItem/DataMapper.php | 12 +++---- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php index 5870e7cd80e67..62581ad13a84b 100644 --- a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php +++ b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php @@ -6,7 +6,6 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ -declare(strict_types=1); namespace Magento\Framework\MessageQueue\Test\Unit\Topology\Config\QueueConfigItem; @@ -23,17 +22,17 @@ class DataMapperTest extends TestCase /** * @var Data|MockObject */ - private $configData; + private $configDataMock; /** * @var CommunicationConfig|MockObject */ - private $communicationConfig; + private $communicationConfigMock; /** * @var ResponseQueueNameBuilder|MockObject */ - private $queueNameBuilder; + private $queueNameBuilderMock; /** * @var DataMapper @@ -45,10 +44,14 @@ class DataMapperTest extends TestCase */ protected function setUp(): void { - $this->configData = $this->createMock(Data::class); - $this->communicationConfig = $this->createMock(CommunicationConfig::class); - $this->queueNameBuilder = $this->createMock(ResponseQueueNameBuilder::class); - $this->model = new DataMapper($this->configData, $this->communicationConfig, $this->queueNameBuilder); + $this->configDataMock = $this->createMock(Data::class); + $this->communicationConfigMock = $this->createMock(CommunicationConfig::class); + $this->queueNameBuilderMock = $this->createMock(ResponseQueueNameBuilder::class); + $this->model = new DataMapper( + $this->configDataMock, + $this->communicationConfigMock, + $this->queueNameBuilderMock + ); } /** @@ -112,11 +115,11 @@ public function testGetMappedData(): void ['topic02', ['name' => 'topic02', 'is_synchronous' => false]], ]; - $this->communicationConfig->expects($this->exactly(2)) + $this->communicationConfigMock->expects($this->exactly(2)) ->method('getTopic') ->willReturnMap($communicationMap); - $this->configData->expects($this->once())->method('get')->willReturn($data); - $this->queueNameBuilder->expects($this->once()) + $this->configDataMock->expects($this->once())->method('get')->willReturn($data); + $this->queueNameBuilderMock->expects($this->once()) ->method('getQueueName') ->with('topic01') ->willReturn('responseQueue.topic01'); @@ -218,15 +221,15 @@ public function testGetMappedDataForWildcard(): void 'topic08.part2.some.test' => ['name' => 'topic08.part2.some.test', 'is_synchronous' => true], ]; - $this->communicationConfig->expects($this->once()) + $this->communicationConfigMock->expects($this->once()) ->method('getTopic') ->with('topic01') ->willReturn(['name' => 'topic01', 'is_synchronous' => true]); - $this->communicationConfig->expects($this->any()) + $this->communicationConfigMock->expects($this->any()) ->method('getTopics') ->willReturn($communicationData); - $this->configData->expects($this->once())->method('get')->willReturn($data); - $this->queueNameBuilder->expects($this->any()) + $this->configDataMock->expects($this->once())->method('get')->willReturn($data); + $this->queueNameBuilderMock->expects($this->any()) ->method('getQueueName') ->willReturnCallback(function ($value) { return 'responseQueue.' . $value; diff --git a/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php b/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php index 627dca68d14a4..d48fa637fd885 100644 --- a/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php +++ b/lib/internal/Magento/Framework/MessageQueue/Topology/Config/QueueConfigItem/DataMapper.php @@ -73,12 +73,12 @@ public function getMappedData(): array foreach ($exchange['bindings'] as $binding) { if ($binding['destinationType'] === 'queue') { $queueItems = $this->createQueueItems( - (string) $binding['destination'], - (string) $binding['topic'], - (array) $binding['arguments'], - (string) $connection + (string)$binding['destination'], + (string)$binding['topic'], + (array)$binding['arguments'], + (string)$connection ); - $this->mappedData += $queueItems; + $this->mappedData = array_merge($this->mappedData, $queueItems); } } } @@ -141,7 +141,7 @@ private function isSynchronousTopic(string $topicName): bool { try { $topic = $this->communicationConfig->getTopic($topicName); - return (bool) $topic[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; + return (bool)$topic[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; } catch (LocalizedException $exception) { throw new LocalizedException(new Phrase('Error while checking if topic is synchronous')); } From d8023289262ecadfd3e2dbbe6ac3b5d012f5123a Mon Sep 17 00:00:00 2001 From: Bartosz Kubicki Date: Mon, 5 Oct 2020 23:07:07 +0200 Subject: [PATCH 3/3] Another fix after CR --- .../Config/QueueConfigItem/DataMapperTest.php | 4 ++-- .../Topology/Config/QueueConfigItem/DataMapper.php | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php index 62581ad13a84b..46ea82b887db6 100644 --- a/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php +++ b/lib/internal/Magento/Framework/MessageQueue/Test/Unit/Topology/Config/QueueConfigItem/DataMapperTest.php @@ -1,12 +1,12 @@ mappedData) { - $this->mappedData = []; + $mappedData = []; foreach ($this->configData->get() as $exchange) { $connection = $exchange['connection']; foreach ($exchange['bindings'] as $binding) { @@ -78,10 +79,11 @@ public function getMappedData(): array (array)$binding['arguments'], (string)$connection ); - $this->mappedData = array_merge($this->mappedData, $queueItems); + $mappedData[] = $queueItems; } } } + $this->mappedData = array_merge([], ...$mappedData); } return $this->mappedData; @@ -158,7 +160,7 @@ private function matchSynchronousTopics(string $wildcard): array $topicDefinitions = array_filter( $this->communicationConfig->getTopics(), function ($item) { - return (bool) $item[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; + return (bool)$item[CommunicationConfig::TOPIC_IS_SYNCHRONOUS]; } );