Merge pull request #758 from nextcloud/feature/api-error-messages

Better API error handling

8 files changed, 167 insertions, 88 deletions

diff --git a/tests/Controller/AccountsControllerTest.php b/tests/Controller/AccountsControllerTest.php
index f16fb406c..752473969 100644
--- a/tests/Controller/AccountsControllerTest.php
+++ b/tests/Controller/AccountsControllerTest.php
@@ -23,10 +23,13 @@
 namespace OCA\Mail\Tests\Controller;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
+use Exception;
 use Horde_Exception;
 use OCA\Mail\Account;
 use OCA\Mail\Contracts\IMailTransmission;
 use OCA\Mail\Controller\AccountsController;
+use OCA\Mail\Exception\ClientException;
+use OCA\Mail\Http\JSONResponse;
 use OCA\Mail\Model\NewMessageData;
 use OCA\Mail\Model\RepliedMessageData;
 use OCA\Mail\Service\AccountService;
@@ -36,7 +39,6 @@ use OCA\Mail\Service\Logger;
 use OCA\Mail\Service\SetupService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
-use OCP\AppFramework\Http\JSONResponse;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\Security\ICrypto;
@@ -150,12 +152,9 @@ class AccountsControllerTest extends TestCase {
 			->method('find')
 			->with($this->equalTo($this->userId), $this->equalTo($this->accountId))
 			->will($this->throwException(new DoesNotExistException('test123')));
+		$this->expectException(DoesNotExistException::class);
 
-		$response = $this->controller->show($this->accountId);
-
-		$expectedResponse = new JSONResponse([]);
-		$expectedResponse->setStatus(404);
-		$this->assertEquals($expectedResponse, $response);
+		$this->controller->show($this->accountId);
 	}
 
 	public function testDestroy() {
@@ -174,11 +173,9 @@ class AccountsControllerTest extends TestCase {
 			->method('delete')
 			->with($this->equalTo($this->userId), $this->equalTo($this->accountId))
 			->will($this->throwException(new DoesNotExistException('test')));
+		$this->expectException(DoesNotExistException::class);
 
-		$response = $this->controller->destroy($this->accountId);
-
-		$expectedResponse = new JSONResponse([], Http::STATUS_NOT_FOUND);
-		$this->assertEquals($expectedResponse, $response);
+		$this->controller->destroy($this->accountId);
 	}
 
 	public function testCreateAutoDetectSuccess() {
@@ -210,19 +207,14 @@ class AccountsControllerTest extends TestCase {
 		$email = 'john@example.com';
 		$password = '123456';
 		$accountName = 'John Doe';
-
 		$this->setupService->expects($this->once())
 			->method('createNewAutoconfiguredAccount')
 			->with($accountName, $email, $password)
 			->willThrowException(new \Exception());
+		$this->expectException(ClientException::class);
 
-		$response = $this->controller->create($accountName, $email, $password, null, null, null, null, null, null, null, null,
+		$this->controller->create($accountName, $email, $password, null, null, null, null, null, null, null, null,
 			null, null, true);
-
-		$expectedResponse = new JSONResponse([
-			'message' => '',
-			], Http::STATUS_BAD_REQUEST);
-		$this->assertEquals($expectedResponse, $response);
 	}
 
 	public function testUpdateAutoDetectSuccess() {
@@ -254,19 +246,14 @@ class AccountsControllerTest extends TestCase {
 		$email = 'john@example.com';
 		$password = '123456';
 		$accountName = 'John Doe';
-
 		$this->setupService->expects($this->once())
 			->method('createNewAutoconfiguredAccount')
 			->with($accountName, $email, $password)
-			->willThrowException(new \Exception());
+			->willThrowException(new Exception());
+		$this->expectException(ClientException::class);
 
-		$response = $this->controller->create($accountName, $email, $password, null, null, null, null, null, null, null, null,
+		$this->controller->create($accountName, $email, $password, null, null, null, null, null, null, null, null,
 			null, null, true);
-
-		$expectedResponse = new JSONResponse([
-			'message' => '',
-			], Http::STATUS_BAD_REQUEST);
-		$this->assertEquals($expectedResponse, $response);
 	}
 
 	public function testCreateManualSuccess() {
@@ -323,15 +310,10 @@ class AccountsControllerTest extends TestCase {
 		$this->setupService->expects($this->once())
 			->method('createNewAccount')
 			->with($accountName, $email, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $this->userId)
-			->willThrowException(new \Exception());
-
-		$response = $this->controller->create($accountName, $email, $password, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $autoDetect);
+			->willThrowException(new Exception());
+		$this->expectException(ClientException::class);
 
-		$expectedResponse = new JSONResponse([
-			'message' => '',
-			], Http::STATUS_BAD_REQUEST);
-
-		$this->assertEquals($expectedResponse, $response);
+		$this->controller->create($accountName, $email, $password, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $autoDetect);
 	}
 
 	public function testUpdateManualSuccess() {
@@ -365,7 +347,7 @@ class AccountsControllerTest extends TestCase {
 			'data' => [
 				'id' => 135,
 			],
-			], Http::STATUS_CREATED);
+			]);
 
 		$this->assertEquals($expectedResponse, $response);
 	}
@@ -390,14 +372,10 @@ class AccountsControllerTest extends TestCase {
 		$this->setupService->expects($this->once())
 			->method('createNewAccount')
 			->with($accountName, $email, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $this->userId, $id)
-			->willThrowException(new \Exception());
-
-		$response = $this->controller->update($id, $accountName, $email, $password, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $autoDetect);
+			->willThrowException(new Exception());
+		$this->expectException(ClientException::class);
 
-		$expectedResponse = new JSONResponse([
-			'message' => '',
-			], Http::STATUS_BAD_REQUEST);
-		$this->assertEquals($expectedResponse, $response);
+		$this->controller->update($id, $accountName, $email, $password, $imapHost, $imapPort, $imapSslMode, $imapUser, $imapPassword, $smtpHost, $smtpPort, $smtpSslMode, $smtpUser, $smtpPassword, $autoDetect);
 	}
 
 	public function testSendNewMessage() {
@@ -428,11 +406,9 @@ class AccountsControllerTest extends TestCase {
 			->method('sendMessage')
 			->with($this->userId, $messageData, $replyData, null, null)
 			->willThrowException(new Horde_Exception('error'));
-		$expected = new JSONResponse(['message' => 'error'], 500);
-
-		$resp = $this->controller->send(13, null, 'sub', 'bod', 'to@d.com', '', '', null, null, [], null);
+		$this->expectException(Horde_Exception::class);
 
-		$this->assertEquals($expected, $resp);
+		$this->controller->send(13, null, 'sub', 'bod', 'to@d.com', '', '', null, null, [], null);
 	}
 
 	public function testSendReply() {
diff --git a/tests/Controller/AliasesControllerTest.php b/tests/Controller/AliasesControllerTest.php
index c75dd91af..3d1364ba8 100644
--- a/tests/Controller/AliasesControllerTest.php
+++ b/tests/Controller/AliasesControllerTest.php
@@ -23,7 +23,8 @@ namespace OCA\Mail\Tests\Controller;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCA\Mail\Controller\AliasesController;
-use OCP\AppFramework\Http\JSONResponse;
+use OCA\Mail\Http\JSONResponse;
+use OCP\AppFramework\Http;
 
 class AliasesControllerTest extends TestCase {
 	private $controller;
@@ -68,9 +69,9 @@ class AliasesControllerTest extends TestCase {
 
 		$response = $this->controller->index($accountId);
 
-		$expectedResponse = [
+		$expectedResponse = new JSONResponse([
 			$this->alias
-		];
+		]);
 		$this->assertEquals($expectedResponse, $response);
 	}
 
@@ -82,7 +83,7 @@ class AliasesControllerTest extends TestCase {
 		$this->aliasService->expects($this->once())
 			->method('delete')
 			->with($this->equalTo($aliasId), $this->equalTo($this->userId))
-			->will($this->returnValue(new JSONResponse()));
+			->will($this->returnValue([]));
 
 		$response = $this->controller->destroy($aliasId);
 
@@ -106,12 +107,12 @@ class AliasesControllerTest extends TestCase {
 
 		$response = $this->controller->create($accountId, $alias, $aliasName);
 
-		$expected = [
+		$expected = new JSONResponse([
 			'accountId' => $accountId,
 			'name' => $aliasName,
 			'alias' => $alias,
 			'id' => 123
-		];
+		], Http::STATUS_CREATED);
 		$this->assertEquals($expected, $response);
 	}
 }
diff --git a/tests/Controller/AutoconfigControllerTest.php b/tests/Controller/AutoconfigControllerTest.php
index 3cf1e67b1..cd25bd13f 100644
--- a/tests/Controller/AutoconfigControllerTest.php
+++ b/tests/Controller/AutoconfigControllerTest.php
@@ -23,6 +23,7 @@ namespace OCA\Mail\Tests\Controller;
 
 use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCA\Mail\Controller\AutoCompleteController;
+use OCA\Mail\Http\JSONResponse;
 
 class AutoConfigControllerTest extends TestCase {
 
@@ -52,7 +53,7 @@ class AutoConfigControllerTest extends TestCase {
 
 		$response = $this->controller->index($term);
 
-		$this->assertEquals($result, $response);
+		$this->assertEquals(new JSONResponse($result), $response);
 	}
 
 }
diff --git a/tests/Controller/FoldersControllerTest.php b/tests/Controller/FoldersControllerTest.php
index 75d03762d..5d8236dd7 100644
--- a/tests/Controller/FoldersControllerTest.php
+++ b/tests/Controller/FoldersControllerTest.php
@@ -21,17 +21,14 @@
 
 namespace OCA\Mail\Tests\Controller;
 
-use Horde_Imap_Client_Exception;
-use Horde_Imap_Client_Socket;
 use OCA\Mail\Account;
 use OCA\Mail\Contracts\IMailManager;
 use OCA\Mail\Controller\FoldersController;
+use OCA\Mail\Exception\NotImplemented;
 use OCA\Mail\Folder;
+use OCA\Mail\Http\JSONResponse;
 use OCA\Mail\Service\AccountService;
 use ChristophWurst\Nextcloud\Testing\TestCase;
-use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Http;
-use OCP\AppFramework\Http\JSONResponse;
 use OCP\IRequest;
 use PHPUnit_Framework_MockObject_MockObject;
 
@@ -86,25 +83,27 @@ class FoldersControllerTest extends TestCase {
 
 		$result = $this->controller->index($accountId);
 
-		$expected = [
+		$expected = new JSONResponse([
 			'id' => 28,
 			'email' => 'user@example.com',
 			'folders' => [
 				$folder,
 			],
 			'delimiter' => '.',
-		];
+		]);
 		$this->assertEquals($expected, $result);
 	}
 
 	public function testShow() {
-		$result = $this->controller->show();
-		$this->assertEquals(Http::STATUS_NOT_IMPLEMENTED, $result->getStatus());
+		$this->expectException(NotImplemented::class);
+
+		$this->controller->show();
 	}
 
 	public function testUpdate() {
-		$result = $this->controller->update();
-		$this->assertEquals(Http::STATUS_NOT_IMPLEMENTED, $result->getStatus());
+		$this->expectException(NotImplemented::class);
+
+		$this->controller->update();
 	}
 
 }
diff --git a/tests/Controller/LocalAttachmentsControllerTest.php b/tests/Controller/LocalAttachmentsControllerTest.php
index f75d068e4..a3db84bd2 100644
--- a/tests/Controller/LocalAttachmentsControllerTest.php
+++ b/tests/Controller/LocalAttachmentsControllerTest.php
@@ -25,8 +25,9 @@ use ChristophWurst\Nextcloud\Testing\TestCase;
 use OCA\Mail\Contracts\IAttachmentService;
 use OCA\Mail\Controller\LocalAttachmentsController;
 use OCA\Mail\Db\LocalAttachment;
+use OCA\Mail\Exception\ClientException;
+use OCA\Mail\Http\JSONResponse;
 use OCA\Mail\Service\Attachment\UploadedFile;
-use OCP\AppFramework\Http\JSONResponse;
 use OCP\IRequest;
 use PHPUnit_Framework_MockObject_MockObject;
 
@@ -59,11 +60,9 @@ class LocalAttachmentsControllerTest extends TestCase {
 			->method('getUploadedFile')
 			->with('attachment')
 			->willReturn(null);
-		$expected = new JSONResponse(null, 400);
+		$this->expectException(ClientException::class);
 
-		$actual = $this->controller->create();
-
-		$this->assertEquals($expected, $actual);
+		$this->controller->create();
 	}
 
 	public function testCreate() {
diff --git a/tests/Controller/MessagesControllerTest.php b/tests/Controller/MessagesControllerTest.php
index 7dc787cce..0c6ed89f4 100644
--- a/tests/Controller/MessagesControllerTest.php
+++ b/tests/Controller/MessagesControllerTest.php
@@ -35,7 +35,6 @@ use OCA\Mail\Model\IMAPMessage;
 use OCA\Mail\Service\AccountService;
 use OCA\Mail\Service\Logger;
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -365,37 +364,30 @@ class MessagesControllerTest extends TestCase {
 		$accountId = 17;
 		$folderId = base64_encode('my folder');
 		$messageId = 123;
-
 		$this->accountService->expects($this->once())
 			->method('find')
 			->with($this->equalTo($this->userId), $this->equalTo($accountId))
 			->will($this->throwException(new DoesNotExistException('')));
+		$this->expectException(DoesNotExistException::class);
 
-		$expected = new JSONResponse([], Http::STATUS_NOT_FOUND);
-		$result = $this->controller->destroy($accountId, $folderId, $messageId);
-
-		$this->assertEquals($expected, $result);
+		$this->controller->destroy($accountId, $folderId, $messageId);
 	}
 
 	public function testDestroyWithFolderOrMessageNotFound() {
 		$accountId = 17;
 		$folderId = base64_encode('my folder');
 		$messageId = 123;
-
 		$this->accountService->expects($this->once())
 			->method('find')
 			->with($this->equalTo($this->userId), $this->equalTo($accountId))
 			->will($this->returnValue($this->account));
-
 		$this->account->expects($this->once())
 			->method('deleteMessage')
 			->with(base64_decode($folderId), $messageId)
 			->will($this->throwException(new DoesNotExistException('')));
+		$this->expectException(DoesNotExistException::class);
 
-		$expected = new JSONResponse([], Http::STATUS_NOT_FOUND);
-		$result = $this->controller->destroy($accountId, $folderId, $messageId);
-
-		$this->assertEquals($expected, $result);
+		$this->controller->destroy($accountId, $folderId, $messageId);
 	}
 
 }
diff --git a/tests/Http/Middleware/ErrorMiddlewareTest.php b/tests/Http/Middleware/ErrorMiddlewareTest.php
new file mode 100644
index 000000000..365911b40
--- /dev/null
+++ b/tests/Http/Middleware/ErrorMiddlewareTest.php
@@ -0,0 +1,111 @@
+<?php
+
+/**
+ * @copyright 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @author 2017 Christoph Wurst <christoph@winzerhof-wurst.at>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\Mail\Tests\Http\Middleware;
+
+use ChristophWurst\Nextcloud\Testing\TestCase;
+use OCA\Mail\Exception\NotImplemented;
+use OCA\Mail\Exception\ServiceException;
+use OCA\Mail\Http\JSONResponse;
+use OCA\Mail\Http\Middleware\ErrorMiddleware;
+use OCA\Mail\Service\Logger;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Utility\IControllerMethodReflector;
+use OCP\IConfig;
+use PHPUnit_Framework_MockObject_MockObject;
+
+class ErrorMiddlewareTest extends TestCase {
+
+	/** @var IConfig|PHPUnit_Framework_MockObject_MockObject */
+	private $config;
+
+	/** @var Logger|PHPUnit_Framework_MockObject_MockObject */
+	private $logger;
+
+	/** @var IControllerMethodReflector|PHPUnit_Framework_MockObject_MockObject */
+	private $reflector;
+
+	/** @var ErrorMiddleware */
+	private $middleware;
+
+	protected function setUp() {
+		parent::setUp();
+
+		$this->config = $this->createMock(IConfig::class);
+		$this->logger = $this->createMock(Logger::class);
+		$this->reflector = $this->createMock(IControllerMethodReflector::class);
+
+		$this->middleware = new ErrorMiddleware($this->config, $this->logger,
+			$this->reflector);
+	}
+
+	public function testDoesNotChangeSuccessfulResponses() {
+		$response = new JSONResponse();
+		$controller = $this->createMock(Controller::class);
+
+		$after = $this->middleware->afterController($controller, 'index', $response);
+
+		$this->assertSame($response, $after);
+	}
+
+	public function testDoesNotChangeUntaggedMethodResponses() {
+		$controller = $this->createMock(Controller::class);
+		$exception = new DoesNotExistException("nope");
+		$this->reflector->expects($this->once())
+			->method('hasAnnotation')
+			->willReturn(false);
+		$this->expectException(DoesNotExistException::class);
+
+		$this->middleware->afterException($controller, 'index', $exception);
+	}
+
+	public function trappedErrorsData() {
+		return [
+			[new DoesNotExistException("does not exist"), Http::STATUS_NOT_FOUND],
+			[new ServiceException(), Http::STATUS_INTERNAL_SERVER_ERROR],
+			[new NotImplemented(), Http::STATUS_NOT_IMPLEMENTED],
+		];
+	}
+
+	/**
+	 * @dataProvider trappedErrorsData
+	 */
+	public function testTrapsErrors($exception, $expectedStatus) {
+		$controller = $this->createMock(Controller::class);
+		$this->reflector->expects($this->once())
+			->method('hasAnnotation')
+			->willReturn(true);
+		$this->logger->expects($this->once())
+			->method('logException')
+			->with($exception);
+
+		$response = $this->middleware->afterException($controller, 'index', $exception);
+
+		$this->assertInstanceOf(JSONResponse::class, $response);
+		$this->assertEquals($expectedStatus, $response->getStatus());
+	}
+
+}
diff --git a/tests/Integration/FolderSynchronizationTest.php b/tests/Integration/FolderSynchronizationTest.php
index 505b3a0e2..e84094564 100644
--- a/tests/Integration/FolderSynchronizationTest.php
+++ b/tests/Integration/FolderSynchronizationTest.php
@@ -47,8 +47,8 @@ class FolderSynchronizationTest extends TestCase {
 		$mailbox = 'INBOX';
 		$syncToken = $this->getMailboxSyncToken($mailbox);
 
-		$sync = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken);
-		$syncJson = $sync->jsonSerialize();
+		$jsonResponse = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken);
+		$syncJson = $jsonResponse->getData()->jsonSerialize();
 
 		$this->assertArrayHasKey('newMessages', $syncJson);
 		$this->assertArrayHasKey('changedMessages', $syncJson);
@@ -71,8 +71,8 @@ class FolderSynchronizationTest extends TestCase {
 			->finish();
 		$this->saveMessage($mailbox, $message);
 
-		$sync = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken);
-		$syncJson = $sync->jsonSerialize();
+		$jsonResponse = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken);
+		$syncJson = $jsonResponse->getData()->jsonSerialize();
 
 		$this->assertCount(1, $syncJson['newMessages']);
 		$this->assertCount(0, $syncJson['changedMessages']);
@@ -93,10 +93,10 @@ class FolderSynchronizationTest extends TestCase {
 		// Third, flag it
 		$this->flagMessage($mailbox, $id);
 
-		$sync = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken, [
+		$jsonResponse = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken, [
 			$id
 		]);
-		$syncJson = $sync->jsonSerialize();
+		$syncJson = $jsonResponse->getData()->jsonSerialize();
 
 		$this->assertCount(0, $syncJson['newMessages']);
 		$this->assertCount(1, $syncJson['changedMessages']);
@@ -117,10 +117,10 @@ class FolderSynchronizationTest extends TestCase {
 		// Third, remove it again
 		$this->deleteMessage($mailbox, $id);
 
-		$sync = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken, [
+		$jsonResponse = $this->foldersController->sync($account->getId(), base64_encode($mailbox), $syncToken, [
 			$id
 		]);
-		$syncJson = $sync->jsonSerialize();
+		$syncJson = $jsonResponse->getData()->jsonSerialize();
 
 		$this->assertCount(0, $syncJson['newMessages']);
 		// TODO: deleted messages are flagged as changed? could be a testing-only issue