From 877b098b09244b60b3cd4c838a316a67a774701c Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sun, 28 Oct 2018 15:24:41 +0100
Subject: [PATCH] Improved public API in ApiKey entity, reducing anemic model

---
 .../CLI/src/Command/Api/ListKeysCommand.php   | 19 +++++------
 module/Rest/src/Entity/ApiKey.php             | 33 ++++---------------
 module/Rest/src/Service/ApiKeyService.php     |  9 +++--
 .../src/Service/ApiKeyServiceInterface.php    |  3 ++
 .../test/Action/AuthenticateActionTest.php    |  2 +-
 .../Rest/test/Service/ApiKeyServiceTest.php   |  3 +-
 6 files changed, 23 insertions(+), 46 deletions(-)

diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php
index f3d4d30e..ef1c5c29 100644
--- a/module/CLI/src/Command/Api/ListKeysCommand.php
+++ b/module/CLI/src/Command/Api/ListKeysCommand.php
@@ -12,6 +12,7 @@ use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Zend\I18n\Translator\TranslatorInterface;
 use function array_filter;
+use function array_map;
 use function sprintf;
 
 class ListKeysCommand extends Command
@@ -54,24 +55,20 @@ class ListKeysCommand extends Command
     {
         $io = new SymfonyStyle($input, $output);
         $enabledOnly = $input->getOption('enabledOnly');
-        $list = $this->apiKeyService->listKeys($enabledOnly);
-        $rows = [];
 
-        /** @var ApiKey $row */
-        foreach ($list as $row) {
-            $key = $row->getKey();
-            $expiration = $row->getExpirationDate();
-            $messagePattern = $this->determineMessagePattern($row);
+        $rows = array_map(function (ApiKey $apiKey) use ($enabledOnly) {
+            $key = (string) $apiKey;
+            $expiration = $apiKey->getExpirationDate();
+            $messagePattern = $this->determineMessagePattern($apiKey);
 
             // Set columns for this row
             $rowData = [sprintf($messagePattern, $key)];
             if (! $enabledOnly) {
-                $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($row));
+                $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($apiKey));
             }
             $rowData[] = $expiration !== null ? $expiration->toAtomString() : '-';
-
-            $rows[] = $rowData;
-        }
+            return $rowData;
+        }, $this->apiKeyService->listKeys($enabledOnly));
 
         $io->table(array_filter([
             $this->translator->translate('Key'),
diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php
index b13629f8..ea48605f 100644
--- a/module/Rest/src/Entity/ApiKey.php
+++ b/module/Rest/src/Entity/ApiKey.php
@@ -36,21 +36,11 @@ class ApiKey extends AbstractEntity
      */
     private $enabled;
 
-    public function __construct()
+    public function __construct(?Chronos $expirationDate = null)
     {
-        $this->enabled = true;
         $this->key = $this->generateV4Uuid();
-    }
-
-    public function getKey(): string
-    {
-        return $this->key;
-    }
-
-    public function setKey(string $key): self
-    {
-        $this->key = $key;
-        return $this;
+        $this->expirationDate = $expirationDate;
+        $this->enabled = true;
     }
 
     public function getExpirationDate(): ?Chronos
@@ -58,12 +48,6 @@ class ApiKey extends AbstractEntity
         return $this->expirationDate;
     }
 
-    public function setExpirationDate(Chronos $expirationDate): self
-    {
-        $this->expirationDate = $expirationDate;
-        return $this;
-    }
-
     public function isExpired(): bool
     {
         if ($this->expirationDate === null) {
@@ -78,15 +62,10 @@ class ApiKey extends AbstractEntity
         return $this->enabled;
     }
 
-    public function setEnabled(bool $enabled): self
-    {
-        $this->enabled = $enabled;
-        return $this;
-    }
-
     public function disable(): self
     {
-        return $this->setEnabled(false);
+        $this->enabled = false;
+        return $this;
     }
 
     /**
@@ -99,6 +78,6 @@ class ApiKey extends AbstractEntity
 
     public function __toString(): string
     {
-        return $this->getKey();
+        return $this->key;
     }
 }
diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index bad09f3b..d1d6a42f 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -23,11 +23,7 @@ class ApiKeyService implements ApiKeyServiceInterface
 
     public function create(?Chronos $expirationDate = null): ApiKey
     {
-        $key = new ApiKey();
-        if ($expirationDate !== null) {
-            $key->setExpirationDate($expirationDate);
-        }
-
+        $key = new ApiKey($expirationDate);
         $this->em->persist($key);
         $this->em->flush();
 
@@ -57,6 +53,9 @@ class ApiKeyService implements ApiKeyServiceInterface
         return $apiKey;
     }
 
+    /**
+     * @return ApiKey[]
+     */
     public function listKeys(bool $enabledOnly = false): array
     {
         $conditions = $enabledOnly ? ['enabled' => true] : [];
diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php
index 8aaf86c7..82f88c36 100644
--- a/module/Rest/src/Service/ApiKeyServiceInterface.php
+++ b/module/Rest/src/Service/ApiKeyServiceInterface.php
@@ -18,6 +18,9 @@ interface ApiKeyServiceInterface
      */
     public function disable(string $key): ApiKey;
 
+    /**
+     * @return ApiKey[]
+     */
     public function listKeys(bool $enabledOnly = false): array;
 
     public function getByKey(string $key): ?ApiKey;
diff --git a/module/Rest/test/Action/AuthenticateActionTest.php b/module/Rest/test/Action/AuthenticateActionTest.php
index 49fe07cb..123444cc 100644
--- a/module/Rest/test/Action/AuthenticateActionTest.php
+++ b/module/Rest/test/Action/AuthenticateActionTest.php
@@ -74,7 +74,7 @@ class AuthenticateActionTest extends TestCase
      */
     public function invalidApiKeyReturnsErrorResponse()
     {
-        $this->apiKeyService->getByKey('foo')->willReturn((new ApiKey())->setEnabled(false))
+        $this->apiKeyService->getByKey('foo')->willReturn((new ApiKey())->disable())
                                              ->shouldBeCalledTimes(1);
 
         $request = ServerRequestFactory::fromGlobals()->withParsedBody([
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index e6db1a5d..1faa112f 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -87,8 +87,7 @@ class ApiKeyServiceTest extends TestCase
      */
     public function checkReturnsFalseWhenKeyIsExpired()
     {
-        $key = new ApiKey();
-        $key->setExpirationDate(Chronos::now()->subDay());
+        $key = new ApiKey(Chronos::now()->subDay());
         $repo = $this->prophesize(EntityRepository::class);
         $repo->findOneBy(['key' => '12345'])->willReturn($key)
                                             ->shouldBeCalledTimes(1);