From e939d4f52c25829c58843be950f0d9fad5e73dbb Mon Sep 17 00:00:00 2001 From: Marc Rubio Date: Thu, 9 Mar 2017 11:01:19 +0100 Subject: [PATCH 1/4] Remove useless try catch block --- .../v26/RabbitMQ/Queue/MessageHandler.php | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/MessageHandler.php b/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/MessageHandler.php index 4deac59..a0314bb 100644 --- a/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/MessageHandler.php +++ b/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/MessageHandler.php @@ -1,16 +1,8 @@ jsonMessageFactory->create($message->body); - call_user_func_array($this->callback, [$task]); - $message->delivery_info['channel']->basic_ack($message->delivery_info['delivery_tag']); - } catch (\Exception $e) { - throw $e; - } + $task = $this->jsonMessageFactory->create($message->body); + call_user_func_array($this->callback, [$task]); + $message->delivery_info['channel']->basic_ack($message->delivery_info['delivery_tag']); } public function setCallback(callable $callback) From 6f7ea66d9419a2dc80dff37d7f9b9904c8804fb1 Mon Sep 17 00:00:00 2001 From: Marc Rubio Date: Thu, 9 Mar 2017 11:03:00 +0100 Subject: [PATCH 2/4] Fix masking all exceptions as TimeoutReaderException Include previous exception when initializing reader --- .../AmqpLib/v26/RabbitMQ/Queue/QueueReader.php | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/QueueReader.php b/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/QueueReader.php index 61b2f90..4a1b1a0 100644 --- a/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/QueueReader.php +++ b/src/Cmp/Queues/Infrastructure/AmqpLib/v26/RabbitMQ/Queue/QueueReader.php @@ -1,11 +1,4 @@ messageHandler->setCallback($callback); try { $this->channel->wait(null, false, $timeout); - } catch(\Exception $e) { - throw new TimeoutReaderException(); + } catch(AMQPTimeoutException $e) { + throw new TimeoutReaderException("Timed out while reading", 0, $e); } } @@ -150,7 +144,7 @@ protected function initialize() ); } catch (\ErrorException $exception) { $this->logger->error('Error trying to connect to rabbitMQ:' . $exception->getMessage()); - throw new ReaderException($exception->getMessage(), $exception->getCode()); + throw new ReaderException("Error initializing queue reader", 0, $exception); } } From 4c273b38596e8788e6d169826c8897a1caf696bc Mon Sep 17 00:00:00 2001 From: Marc Rubio Date: Thu, 9 Mar 2017 15:02:00 +0100 Subject: [PATCH 3/4] Repackage AMQP exceptions into ReaderException --- .../Domain/Queue/Exception/TimeoutReaderException.php | 9 +-------- .../AmqpLib/v26/RabbitMQ/Queue/QueueReader.php | 4 +++- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/Cmp/Queues/Domain/Queue/Exception/TimeoutReaderException.php b/src/Cmp/Queues/Domain/Queue/Exception/TimeoutReaderException.php index a953f25..f3a218e 100644 --- a/src/Cmp/Queues/Domain/Queue/Exception/TimeoutReaderException.php +++ b/src/Cmp/Queues/Domain/Queue/Exception/TimeoutReaderException.php @@ -1,14 +1,7 @@ channel->wait(null, false, $timeout); } catch(AMQPTimeoutException $e) { - throw new TimeoutReaderException("Timed out while reading", 0, $e); + throw new TimeoutReaderException("Timed out at $timeout seconds while reading.", 0, $e); + } catch(\Exception $e) { + throw new ReaderException("Error occurred while reading", 0, $e); } } From acd64469062faa6775fb2b926005f0751eaeec76 Mon Sep 17 00:00:00 2001 From: Marc Rubio Date: Fri, 10 Mar 2017 11:51:03 +0100 Subject: [PATCH 4/4] Added checks to JSONTaskFactory --- .../Queues/Domain/Task/JSONTaskFactory.php | 25 +++++++++++++------ .../Domain/Task/JSONTaskFactorySpec.php | 22 +++++++++++++--- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/Cmp/Queues/Domain/Task/JSONTaskFactory.php b/src/Cmp/Queues/Domain/Task/JSONTaskFactory.php index cff0a87..10ba6df 100644 --- a/src/Cmp/Queues/Domain/Task/JSONTaskFactory.php +++ b/src/Cmp/Queues/Domain/Task/JSONTaskFactory.php @@ -4,18 +4,29 @@ use Cmp\Queues\Domain\Queue\JSONMessageFactory; use Cmp\Queues\Domain\Task\Exception\InvalidJSONTaskException; +use Cmp\Queues\Domain\Task\Exception\TaskException; class JSONTaskFactory implements JSONMessageFactory { - + /** + * @param $json + * + * @return Task + * @throws InvalidJSONTaskException + * @throws TaskException + */ public function create($json) { - try { - $taskArray = json_decode($json, true); - return new Task($taskArray['name'], $taskArray['body'], $taskArray['delay']); - } catch (\Exception $e) { - throw new InvalidJSONTaskException(); + $taskArray = json_decode($json, true); + + if (json_last_error() !== JSON_ERROR_NONE) { + throw new InvalidJSONTaskException("String is not valid JSON"); } - } + if (!isset($taskArray['name'], $taskArray['body'])) { + throw new InvalidJSONTaskException("Cannot reconstruct task. Name or body fields are missing"); + } + + return new Task($taskArray['name'], $taskArray['body'], isset($taskArray['delay']) ? $taskArray['delay'] : 0); + } } \ No newline at end of file diff --git a/tests/spec/Cmp/Queues/Domain/Task/JSONTaskFactorySpec.php b/tests/spec/Cmp/Queues/Domain/Task/JSONTaskFactorySpec.php index 4a64036..9e5b343 100644 --- a/tests/spec/Cmp/Queues/Domain/Task/JSONTaskFactorySpec.php +++ b/tests/spec/Cmp/Queues/Domain/Task/JSONTaskFactorySpec.php @@ -2,6 +2,7 @@ namespace spec\Cmp\Queues\Domain\Task; +use Cmp\Queues\Domain\Task\Exception\InvalidJSONTaskException; use Cmp\Queues\Domain\Task\Task; use PhpSpec\ObjectBehavior; use Prophecy\Argument; @@ -16,9 +17,22 @@ function it_is_initializable() function it_should_convert_from_json_to_Task() { $taskPreFactory = new Task('name', array(1,2,3), 10); - $taskPostFactory = $this->create(json_encode($taskPreFactory)); - $taskPostFactory->getName()->shouldBe($taskPreFactory->getName()); - $taskPostFactory->getBody()->shouldBe($taskPreFactory->getBody()); - $taskPostFactory->getDelay()->shouldBe($taskPreFactory->getDelay()); + $this->create(json_encode($taskPreFactory))->shouldBeLike($taskPreFactory); + } + + function it_throws_exception_for_invalid_json() + { + $invalidJsonString = 'fsadfgkajghksdghdg'; + $this->shouldThrow(InvalidJSONTaskException::class)->duringCreate($invalidJsonString); + } + + function it_throws_exception_when_missing_required_keys() + { + $decodedJsonData = [ + 'foo' => 'bar' + ]; + + $jsonStr = json_encode($decodedJsonData); + $this->shouldThrow(InvalidJSONTaskException::class)->duringCreate($jsonStr); } }