Throw proper error codes when creating invalid folders through the API, fix #297

11 files changed, 169 insertions, 23 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 7ab264e5b..d769166d9 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,7 @@ owncloud-news (1.401)
 * Add an option route for the API which handles the CORS headers to allow webapplications to access the API
 * Also allow youtube and vimeo embeds that start with // and https:// instead of only allowing http
 * ownCloud 6 compability fixes
+* Throw proper error codes when creating invalid folders through the API
 
 owncloud-news (1.206)
 * Also handle URLErrors in updater script that are thrown when the domain of a feed is not found
diff --git a/businesslayer/businesslayerexistsexception.php b/businesslayer/businesslayerconflictexception.php
index 2e2fd2665..a65bbf2d6 100644
--- a/businesslayer/businesslayerexistsexception.php
+++ b/businesslayer/businesslayerconflictexception.php
@@ -26,7 +26,7 @@
 namespace OCA\News\BusinessLayer;
 
 
-class BusinessLayerExistsException extends BusinessLayerException {
+class BusinessLayerConflictException extends BusinessLayerException {
 
 	/**
 	 * Constructor
diff --git a/businesslayer/businesslayervalidationexception.php b/businesslayer/businesslayervalidationexception.php
new file mode 100644
index 000000000..c4f2c2387
--- /dev/null
+++ b/businesslayer/businesslayervalidationexception.php
@@ -0,0 +1,39 @@
+<?php
+
+/**
+* ownCloud - News
+*
+* @author Alessandro Cosentino
+* @author Bernhard Posselt
+* @copyright 2012 Alessandro Cosentino cosenal@gmail.com
+* @copyright 2012 Bernhard Posselt dev@bernhard-posselt.com
+*
+* This library is free software; you can redistribute it and/or
+* modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
+* License as published by the Free Software Foundation; either
+* version 3 of the License, or any later version.
+*
+* This library 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 library.  If not, see <http://www.gnu.org/licenses/>.
+*
+*/
+
+namespace OCA\News\BusinessLayer;
+
+
+class BusinessLayerValidationException extends BusinessLayerException {
+
+	/**
+	 * Constructor
+	 * @param string $msg the error message
+	 */
+	public function __construct($msg){
+		parent::__construct($msg);
+	}
+
+}
\ No newline at end of file
diff --git a/businesslayer/feedbusinesslayer.php b/businesslayer/feedbusinesslayer.php
index cf79d0b22..034209add 100644
--- a/businesslayer/feedbusinesslayer.php
+++ b/businesslayer/feedbusinesslayer.php
@@ -88,7 +88,7 @@ class FeedBusinessLayer extends BusinessLayer {
 	 * @param string $feedUrl the url to the feed
 	 * @param int $folderId the folder where it should be put into, 0 for root folder
 	 * @param string $userId for which user the feed should be created
-	 * @throws BusinessLayerExistsException if the feed exists already
+	 * @throws BusinessLayerConflictException if the feed exists already
 	 * @throws BusinessLayerException if the url points to an invalid feed
 	 * @return Feed the newly created feed
 	 */
@@ -100,7 +100,7 @@ class FeedBusinessLayer extends BusinessLayer {
 			// try again if feed exists depending on the reported link
 			try {
 				$this->mapper->findByUrlHash($feed->getUrlHash(), $userId);
-				throw new BusinessLayerExistsException(
+				throw new BusinessLayerConflictException(
 					$this->api->getTrans()->t('Can not add feed: Exists already'));
 			} catch(DoesNotExistException $ex){}
 
diff --git a/businesslayer/folderbusinesslayer.php b/businesslayer/folderbusinesslayer.php
index ee0f8c8af..e43c705c6 100644
--- a/businesslayer/folderbusinesslayer.php
+++ b/businesslayer/folderbusinesslayer.php
@@ -58,13 +58,17 @@ class FolderBusinessLayer extends BusinessLayer {
 	}
 
 
-	private function allowNoNameTwice($folderName, $userId){
+	private function validateFolder($folderName, $userId){
 		$existingFolders = $this->mapper->findByName($folderName, $userId);
 		if(count($existingFolders) > 0){
 
-			throw new BusinessLayerExistsException(
+			throw new BusinessLayerConflictException(
 				$this->api->getTrans()->t('Can not add folder: Exists already'));
 		}
+
+		if(mb_strlen($folderName) === 0) {
+			throw new BusinessLayerValidationException('Folder name can not be empty');
+		}
 	}
 
 
@@ -73,11 +77,12 @@ class FolderBusinessLayer extends BusinessLayer {
 	 * @param string $folderName the name of the folder
 	 * @param string $userId the name of the user for whom it should be created
 	 * @param int $parentId the parent folder id, deprecated we dont nest folders
-	 * @throws BusinessLayerExistsException if name exists already
+	 * @throws BusinessLayerConflictException if name exists already
+	 * @throws BusinessLayerValidationException if the folder has invalid parameters
 	 * @return Folder the newly created folder
 	 */
 	public function create($folderName, $userId, $parentId=0) {
-		$this->allowNoNameTwice($folderName, $userId);
+		$this->validateFolder($folderName, $userId);
 
 		$folder = new Folder();
 		$folder->setName($folderName);
@@ -87,6 +92,7 @@ class FolderBusinessLayer extends BusinessLayer {
 		return $this->mapper->insert($folder);
 	}
 
+
 	/**
 	 * @throws BusinessLayerException if the folder does not exist
 	 */
@@ -102,13 +108,13 @@ class FolderBusinessLayer extends BusinessLayer {
 	 * @param int $folderId the id of the folder that should be deleted
 	 * @param string $folderName the new name of the folder
 	 * @param string $userId the name of the user for security reasons
-	 * @throws BusinessLayerExistsException if name exists already
-	 * @throws BusinessLayerInvalidParameterException if the foldername is invalid
+	 * @throws BusinessLayerConflictException if name exists already
+	 * @throws BusinessLayerValidationException if the folder has invalid parameters
 	 * @throws BusinessLayerException if the folder does not exist
 	 * @return Folder the updated folder
 	 */
 	public function rename($folderId, $folderName, $userId){
-		$this->allowNoNameTwice($folderName, $userId);
+		$this->validateFolder($folderName, $userId);
 
 		$folder = $this->find($folderId, $userId);
 		$folder->setName($folderName);
diff --git a/external/feedapi.php b/external/feedapi.php
index a9fdb4c70..be228d490 100644
--- a/external/feedapi.php
+++ b/external/feedapi.php
@@ -35,7 +35,7 @@ use \OCA\News\BusinessLayer\FeedBusinessLayer;
 use \OCA\News\BusinessLayer\FolderBusinessLayer;
 use \OCA\News\BusinessLayer\ItemBusinessLayer;
 use \OCA\News\BusinessLayer\BusinessLayerException;
-use \OCA\News\BusinessLayer\BusinessLayerExistsException;
+use \OCA\News\BusinessLayer\BusinessLayerConflictException;
 
 
 class FeedAPI extends Controller {
@@ -112,7 +112,7 @@ class FeedAPI extends Controller {
 
 			return new JSONResponse($result);
 
-		} catch(BusinessLayerExistsException $ex) {
+		} catch(BusinessLayerConflictException $ex) {
 			return new JSONResponse(array('message' => $ex->getMessage()),
 				Http::STATUS_CONFLICT);
 		} catch(BusinessLayerException $ex) {
diff --git a/external/folderapi.php b/external/folderapi.php
index 31a151c0d..5d2555e10 100644
--- a/external/folderapi.php
+++ b/external/folderapi.php
@@ -34,7 +34,8 @@ use \OCA\AppFramework\Http\Http;
 use \OCA\News\BusinessLayer\FolderBusinessLayer;
 use \OCA\News\BusinessLayer\ItemBusinessLayer;
 use \OCA\News\BusinessLayer\BusinessLayerException;
-use \OCA\News\BusinessLayer\BusinessLayerExistsException;
+use \OCA\News\BusinessLayer\BusinessLayerConflictException;
+use \OCA\News\BusinessLayer\BusinessLayerValidationException;
 
 
 class FolderAPI extends Controller {
@@ -93,7 +94,12 @@ class FolderAPI extends Controller {
 			array_push($result['folders'], $folder->toAPI());
 
 			return new JSONResponse($result);
-		} catch(BusinessLayerExistsException $ex) {
+		
+		} catch(BusinessLayerValidationException $ex) {
+			return new JSONResponse(array('message' => $ex->getMessage()),
+				Http::STATUS_UNPROCESSABLE_ENTITY);
+
+		} catch(BusinessLayerConflictException $ex) {
 			return new JSONResponse(array('message' => $ex->getMessage()),
 				Http::STATUS_CONFLICT);
 		}
@@ -137,7 +143,11 @@ class FolderAPI extends Controller {
 			$this->folderBusinessLayer->rename($folderId, $folderName, $userId);
 			return new JSONResponse();
 
-		} catch(BusinessLayerExistsException $ex) {
+		} catch(BusinessLayerValidationException $ex) {
+			return new JSONResponse(array('message' => $ex->getMessage()),
+				Http::STATUS_UNPROCESSABLE_ENTITY);
+
+		} catch(BusinessLayerConflictException $ex) {
 			return new JSONResponse(array('message' => $ex->getMessage()),
 				Http::STATUS_CONFLICT);
 
diff --git a/tests/unit/businesslayer/FolderBusinessLayerTest.php b/tests/unit/businesslayer/FolderBusinessLayerTest.php
index d28c078a5..7f512920e 100644
--- a/tests/unit/businesslayer/FolderBusinessLayerTest.php
+++ b/tests/unit/businesslayer/FolderBusinessLayerTest.php
@@ -115,6 +115,22 @@ class FolderBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility {
 	}
 
 
+	public function testCreateThrowsExWhenFolderNameEmpty(){
+		$folderName = '';
+		$rows = array(
+			array('id' => 1)
+		);
+
+		$this->folderMapper->expects($this->once())
+			->method('findByName')
+			->with($this->equalTo($folderName))
+			->will($this->returnValue(array()));
+
+		$this->setExpectedException('\OCA\News\BusinessLayer\BusinessLayerValidationException');
+		$result = $this->folderBusinessLayer->create($folderName, 'john', 3);
+	}
+
+
 	public function testOpen(){
 		$folder = new Folder();
 
@@ -175,6 +191,22 @@ class FolderBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility {
 	}
 
 
+	public function testRenameThrowsExWhenFolderNameEmpty(){
+		$folderName = '';
+		$rows = array(
+			array('id' => 1)
+		);
+		
+		$this->folderMapper->expects($this->once())
+			->method('findByName')
+			->with($this->equalTo($folderName))
+			->will($this->returnValue(array()));
+
+		$this->setExpectedException('\OCA\News\BusinessLayer\BusinessLayerException');
+		$result = $this->folderBusinessLayer->rename(3, $folderName, 'john');
+	}
+
+
 	public function testMarkDeleted() {
 		$id = 3;
 		$folder = new Folder();
diff --git a/tests/unit/controller/FolderControllerTest.php b/tests/unit/controller/FolderControllerTest.php
index f273c9ec7..e4295583b 100644
--- a/tests/unit/controller/FolderControllerTest.php
+++ b/tests/unit/controller/FolderControllerTest.php
@@ -34,7 +34,7 @@ use \OCA\AppFramework\Db\MultipleObjectsReturnedException;
 use \OCA\News\Db\Folder;
 use \OCA\News\Db\Feed;
 use \OCA\News\BusinessLayer\BusinessLayerException;
-use \OCA\News\BusinessLayer\BusinessLayerExistsException;
+use \OCA\News\BusinessLayer\BusinessLayerConflictException;
 
 require_once(__DIR__ . "/../../classloader.php");
 
@@ -256,7 +256,7 @@ class FolderControllerTest extends ControllerTestUtility {
 
 	public function testCreateReturnsErrorForInvalidCreate(){
 		$msg = 'except';
-		$ex = new BusinessLayerExistsException($msg);
+		$ex = new BusinessLayerConflictException($msg);
 		$this->api->expects($this->once())
 			->method('getUserId')
 			->will($this->returnValue($this->user));
@@ -342,7 +342,7 @@ class FolderControllerTest extends ControllerTestUtility {
 
 	public function testRenameReturnsErrorForInvalidCreate(){
 		$msg = 'except';
-		$ex = new BusinessLayerExistsException($msg);
+		$ex = new BusinessLayerConflictException($msg);
 		$this->folderBusinessLayer->expects($this->once())
 			->method('rename')
 			->will($this->throwException($ex));
diff --git a/tests/unit/external/FeedAPITest.php b/tests/unit/external/FeedAPITest.php
index fbc608cac..ec6aedf19 100644
--- a/tests/unit/external/FeedAPITest.php
+++ b/tests/unit/external/FeedAPITest.php
@@ -31,7 +31,7 @@ use \OCA\AppFramework\Http\Http;
 use \OCA\AppFramework\Utility\ControllerTestUtility;
 
 use \OCA\News\BusinessLayer\BusinessLayerException;
-use \OCA\News\BusinessLayer\BusinessLayerExistsException;
+use \OCA\News\BusinessLayer\BusinessLayerConflictException;
 use \OCA\News\Db\Folder;
 use \OCA\News\Db\Feed;
 use \OCA\News\Db\Item;
@@ -343,7 +343,7 @@ class FeedAPITest extends ControllerTestUtility {
 			->with($this->equalTo($this->user), $this->equalTo(false));
 		$this->feedBusinessLayer->expects($this->once())
 			->method('create')
-			->will($this->throwException(new BusinessLayerExistsException($this->msg)));
+			->will($this->throwException(new BusinessLayerConflictException($this->msg)));
 
 		$response = $this->feedAPI->create();
 
diff --git a/tests/unit/external/FolderAPITest.php b/tests/unit/external/FolderAPITest.php
index 3e5bbabb6..145cc6720 100644
--- a/tests/unit/external/FolderAPITest.php
+++ b/tests/unit/external/FolderAPITest.php
@@ -31,7 +31,8 @@ use \OCA\AppFramework\Utility\ControllerTestUtility;
 use \OCA\AppFramework\Http\Http;
 
 use \OCA\News\BusinessLayer\BusinessLayerException;
-use \OCA\News\BusinessLayer\BusinessLayerExistsException;
+use \OCA\News\BusinessLayer\BusinessLayerConflictException;
+use \OCA\News\BusinessLayer\BusinessLayerValidationException;
 
 use \OCA\News\Db\Folder;
 use \OCA\News\Db\Feed;
@@ -177,7 +178,7 @@ class FolderAPITest extends ControllerTestUtility {
 			->with($this->equalTo($this->user), $this->equalTo(false));
 		$this->folderBusinessLayer->expects($this->once())
 			->method('create')
-			->will($this->throwException(new BusinessLayerExistsException($msg)));
+			->will($this->throwException(new BusinessLayerConflictException($msg)));
 
 		$response = $this->folderAPI->create();
 
@@ -187,6 +188,27 @@ class FolderAPITest extends ControllerTestUtility {
 	}
 
 
+	public function testCreateInvalidFolderName() {
+		$msg = 'exists';
+
+		$this->api->expects($this->once())
+			->method('getUserId')
+			->will($this->returnValue($this->user));
+		$this->folderBusinessLayer->expects($this->once())
+			->method('purgeDeleted')
+			->with($this->equalTo($this->user), $this->equalTo(false));
+		$this->folderBusinessLayer->expects($this->once())
+			->method('create')
+			->will($this->throwException(new BusinessLayerValidationException($msg)));
+
+		$response = $this->folderAPI->create();
+
+		$data = $response->getData();
+		$this->assertEquals($msg, $data['message']);
+		$this->assertEquals(Http::STATUS_UNPROCESSABLE_ENTITY, $response->getStatus());
+	}
+
+
 	public function testDelete() {
 		$folderId = 23;
 
@@ -337,7 +359,7 @@ class FolderAPITest extends ControllerTestUtility {
 			->will($this->returnValue($this->user));
 		$this->folderBusinessLayer->expects($this->once())
 			->method('rename')
-			->will($this->throwException(new BusinessLayerExistsException($this->msg)));
+			->will($this->throwException(new BusinessLayerConflictException($this->msg)));
 
 		$response = $this->folderAPI->update();
 
@@ -347,6 +369,42 @@ class FolderAPITest extends ControllerTestUtility {
 	}
 
 
+	public function testUpdateInvalidFolderName() {
+		$folderId = 23;
+		$folderName = '';
+
+		$this->folderAPI = new FolderAPI(
+			$this->api,
+			new Request(
+				array(
+					'urlParams' => array(
+						'folderId' => $folderId
+					),
+
+					'params' => array(
+						'name' => $folderName
+					)
+				)
+			),
+			$this->folderBusinessLayer,
+			$this->itemBusinessLayer
+		);
+
+		$this->api->expects($this->once())
+			->method('getUserId')
+			->will($this->returnValue($this->user));
+		$this->folderBusinessLayer->expects($this->once())
+			->method('rename')
+			->will($this->throwException(new BusinessLayerValidationException($this->msg)));
+
+		$response = $this->folderAPI->update();
+
+		$data = $response->getData();
+		$this->assertEquals($this->msg, $data['message']);
+		$this->assertEquals(Http::STATUS_UNPROCESSABLE_ENTITY, $response->getStatus());
+	}
+
+
 	public function testRead() {
 		$request = new Request(array(
 			'urlParams' => array(