Merge pull request #3654 from owncloud/clean_up_exceptions

Introduce more robust SQL execution and exception handling

4 files changed, 133 insertions, 62 deletions

diff --git a/lib/db.php b/lib/db.php
index 61836551833..080e895e352 100644
--- a/lib/db.php
+++ b/lib/db.php
@@ -23,7 +23,8 @@
 class DatabaseException extends Exception{
 	private $query;
 
-	public function __construct($message, $query){
+	//FIXME getQuery seems to be unused, maybe use parent constructor with $message, $code and $previous
+	public function __construct($message, $query = null){
 		parent::__construct($message);
 		$this->query = $query;
 	}
@@ -392,9 +393,59 @@ class OC_DB {
 	}
 
 	/**
+	 * @brief execute a prepared statement, on error write log and throw exception
+	 * @param mixed $stmt PDOStatementWrapper | MDB2_Statement_Common ,
+	 *					  an array with 'sql' and optionally 'limit' and 'offset' keys
+	 *					.. or a simple sql query string
+	 * @param array $parameters
+	 * @return result
+	 * @throws DatabaseException
+	 */
+	static public function executeAudited( $stmt, array $parameters = null) {
+		if (is_string($stmt)) {
+			// convert to an array with 'sql'
+			if (stripos($stmt,'LIMIT') !== false) { //OFFSET requires LIMIT, se we only neet to check for LIMIT
+				// TODO try to convert LIMIT OFFSET notation to parameters, see fixLimitClauseForMSSQL
+				$message = 'LIMIT and OFFSET are forbidden for portability reasons,'
+						 . ' pass an array with \'limit\' and \'offset\' instead';
+				throw new DatabaseException($message);
+			}
+			$stmt = array('sql' => $stmt, 'limit' => null, 'offset' => null);
+		}
+		if (is_array($stmt)){
+			// convert to prepared statement
+			if ( ! array_key_exists('sql', $stmt) ) {
+				$message = 'statement array must at least contain key \'sql\'';
+				throw new DatabaseException($message);
+			}
+			if ( ! array_key_exists('limit', $stmt) ) {
+				$stmt['limit'] = null;
+			}
+			if ( ! array_key_exists('limit', $stmt) ) {
+				$stmt['offset'] = null;
+			}
+			$stmt = self::prepare($stmt['sql'], $stmt['limit'], $stmt['offset']);
+		}
+		self::raiseExceptionOnError($stmt, 'Could not prepare statement');
+		if ($stmt instanceof PDOStatementWrapper || $stmt instanceof MDB2_Statement_Common) {
+			$result = $stmt->execute($parameters);
+			self::raiseExceptionOnError($result, 'Could not execute statement');
+		} else {
+			if (is_object($stmt)) {
+				$message = 'Expected a prepared statement or array got ' . get_class($stmt);
+			} else {
+				$message = 'Expected a prepared statement or array got ' . gettype($stmt);
+			}
+			throw new DatabaseException($message);
+		}
+		return $result;
+	}
+
+	/**
 	 * @brief gets last value of autoincrement
 	 * @param string $table The optional table name (will replace *PREFIX*) and add sequence suffix
 	 * @return int id
+	 * @throws DatabaseException
 	 *
 	 * MDB2 lastInsertID()
 	 *
@@ -404,25 +455,27 @@ class OC_DB {
 	public static function insertid($table=null) {
 		self::connect();
 		$type = OC_Config::getValue( "dbtype", "sqlite" );
-		if( $type == 'pgsql' ) {
-			$query = self::prepare('SELECT lastval() AS id');
-			$row = $query->execute()->fetchRow();
+		if( $type === 'pgsql' ) {
+			$result = self::executeAudited('SELECT lastval() AS id');
+			$row = $result->fetchRow();
+			self::raiseExceptionOnError($row, 'fetching row for insertid failed');
 			return $row['id'];
-		}
-		if( $type == 'mssql' ) {
+		} else if( $type === 'mssql') {
 			if($table !== null) {
 				$prefix = OC_Config::getValue( "dbtableprefix", "oc_" );
 				$table = str_replace( '*PREFIX*', $prefix, $table );
 			}
-			return self::$connection->lastInsertId($table);
-		}else{
+			$result = self::$connection->lastInsertId($table);
+		} else {
 			if($table !== null) {
 				$prefix = OC_Config::getValue( "dbtableprefix", "oc_" );
 				$suffix = OC_Config::getValue( "dbsequencesuffix", "_id_seq" );
 				$table = str_replace( '*PREFIX*', $prefix, $table ).$suffix;
 			}
-			return self::$connection->lastInsertId($table);
+			$result = self::$connection->lastInsertId($table);
 		}
+		self::raiseExceptionOnError($result, 'insertid failed');
+		return $result;
 	}
 
 	/**
@@ -512,6 +565,8 @@ class OC_DB {
 
 		//clean up memory
 		unlink( $file2 );
+		
+		self::raiseExceptionOnError($definition,'Failed to parse the database definition');
 
 		// Die in case something went wrong
 		if( $definition instanceof MDB2_Schema_Error ) {
@@ -528,11 +583,7 @@ class OC_DB {
 
 		$ret=self::$schema->createDatabase( $definition );
 
-		// Die in case something went wrong
-		if( $ret instanceof MDB2_Error ) {
-			OC_Template::printErrorPage( self::$MDB2->getDebugOutput().' '.$ret->getMessage() . ': '
-				. $ret->getUserInfo() );
-		}
+		self::raiseExceptionOnError($ret,'Failed to create the database structure');
 
 		return true;
 	}
@@ -552,13 +603,7 @@ class OC_DB {
 		$content = file_get_contents( $file );
 
 		$previousSchema = self::$schema->getDefinitionFromDatabase();
-		if (PEAR::isError($previousSchema)) {
-			$error = $previousSchema->getMessage();
-			$detail = $previousSchema->getDebugInfo();
-			$message = 'Failed to get existing database structure for updating ('.$error.', '.$detail.')';
-			OC_Log::write('core', $message, OC_Log::FATAL);
-			throw new Exception($message);
-		}
+		self::raiseExceptionOnError($previousSchema,'Failed to get existing database structure for updating');
 
 		// Make changes and save them to an in-memory file
 		$file2 = 'static://db_scheme';
@@ -582,13 +627,7 @@ class OC_DB {
 		//clean up memory
 		unlink( $file2 );
 
-		if (PEAR::isError($op)) {
-			$error = $op->getMessage();
-			$detail = $op->getDebugInfo();
-			$message = 'Failed to update database structure ('.$error.', '.$detail.')';
-			OC_Log::write('core', $message, OC_Log::FATAL);
-			throw new Exception($message);
-		}
+		self::raiseExceptionOnError($op,'Failed to update database structure');
 		return true;
 	}
 
@@ -641,15 +680,9 @@ class OC_DB {
 			}
 			$query = substr($query, 0, strlen($query) - 5);
 			try {
-				$stmt = self::prepare($query);
-				$result = $stmt->execute($inserts);
-
-			} catch(PDOException $e) {
-				$entry = 'DB Error: "'.$e->getMessage() . '"<br />';
-				$entry .= 'Offending command was: ' . $query . '<br />';
-				OC_Log::write('core', $entry, OC_Log::FATAL);
-				error_log('DB error: '.$entry);
-				OC_Template::printErrorPage( $entry );
+				$result = self::executeAudited($query, $inserts);
+			} catch(DatabaseException $e) {
+				OC_Template::printExceptionErrorPage( $e );
 			}
 
 			if((int)$result->numRows() === 0) {
@@ -674,16 +707,12 @@ class OC_DB {
 		}
 
 		try {
-			$result = self::prepare($query);
+			$result = self::executeAudited($query, $inserts);
 		} catch(PDOException $e) {
-			$entry = 'DB Error: "'.$e->getMessage() . '"<br />';
-			$entry .= 'Offending command was: ' . $query.'<br />';
-			OC_Log::write('core', $entry, OC_Log::FATAL);
-			error_log('DB error: ' . $entry);
-			OC_Template::printErrorPage( $entry );
+			OC_Template::printExceptionErrorPage( $e );
 		}
 
-		return $result->execute($inserts);
+		return $result;
 	}
 
 	/**
@@ -891,7 +920,32 @@ class OC_DB {
 			return false;
 		}
 	}
+	/**
+	 * check if a result is an error, writes a log entry and throws an exception, works with MDB2 and PDOException
+	 * @param mixed $result
+	 * @param string message
+	 * @return void
+	 * @throws DatabaseException
+	 */
+	public static function raiseExceptionOnError($result, $message = null) {
+		if(self::isError($result)) {
+			if ($message === null) {
+				$message = self::getErrorMessage($result);
+			} else {
+				$message .= ', Root cause:' . self::getErrorMessage($result);
+			}
+			throw new DatabaseException($message, getErrorCode($result));
+		}
+	}
 
+	public static function getErrorCode($error) {
+		if ( self::$backend==self::BACKEND_MDB2 and PEAR::isError($error) ) {
+			$code = $error->getCode();
+		} elseif ( self::$backend==self::BACKEND_PDO and self::$PDO ) {
+			$code = self::$PDO->errorCode();
+		}
+		return $code;
+	}
 	/**
 	 * returns the error code and message as a string for logging
 	 * works with MDB2 and PDOException
@@ -901,9 +955,7 @@ class OC_DB {
 	public static function getErrorMessage($error) {
 		if ( self::$backend==self::BACKEND_MDB2 and PEAR::isError($error) ) {
 			$msg = $error->getCode() . ': ' . $error->getMessage();
-			if (defined('DEBUG') && DEBUG) {
-				$msg .= '(' . $error->getDebugInfo() . ')';
-			}
+			$msg .= ' (' . $error->getDebugInfo() . ')';
 		} elseif (self::$backend==self::BACKEND_PDO and self::$PDO) {
 			$msg = self::$PDO->errorCode() . ': ';
 			$errorInfo = self::$PDO->errorInfo();
diff --git a/lib/response.php b/lib/response.php
index 49d79fda709..674176d078b 100644
--- a/lib/response.php
+++ b/lib/response.php
@@ -11,6 +11,7 @@ class OC_Response {
 	const STATUS_NOT_MODIFIED = 304;
 	const STATUS_TEMPORARY_REDIRECT = 307;
 	const STATUS_NOT_FOUND = 404;
+	const STATUS_INTERNAL_SERVER_ERROR = 500;
 
 	/**
 	* @brief Enable response caching by sending correct HTTP headers
@@ -70,6 +71,9 @@ class OC_Response {
 			case self::STATUS_NOT_FOUND;
 				$status = $status . ' Not Found';
 				break;
+			case self::STATUS_INTERNAL_SERVER_ERROR;
+				$status = $status . ' Internal Server Error';
+				break;
 		}
 		header($protocol.' '.$status);
 	}
diff --git a/lib/setup.php b/lib/setup.php
index a63cc664dbc..71a2d13937e 100644
--- a/lib/setup.php
+++ b/lib/setup.php
@@ -106,12 +106,6 @@ class OC_Setup {
 						'hint' => $e->getHint()
 					);
 					return($error);
-				} catch (Exception $e) {
-					$error[] = array(
-						'error' => $e->getMessage(),
-						'hint' => ''
-					);
-					return($error);
 				}
 			}
 			elseif($dbtype == 'pgsql') {
@@ -127,7 +121,7 @@ class OC_Setup {
 
 				try {
 					self::setupPostgreSQLDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix, $username);
-				} catch (Exception $e) {
+				} catch (DatabaseSetupException $e) {
 					$error[] = array(
 						'error' => $l->t('PostgreSQL username and/or password not valid'),
 						'hint' => $l->t('You need to enter either an existing account or the administrator.')
@@ -150,7 +144,7 @@ class OC_Setup {
 
 				try {
 					self::setupOCIDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix, $dbtablespace, $username);
-				} catch (Exception $e) {
+				} catch (DatabaseSetupException $e) {
 					$error[] = array(
 						'error' => $l->t('Oracle connection could not be established'),
 						'hint' => $e->getMessage().' Check environment: ORACLE_HOME='.getenv('ORACLE_HOME')
@@ -177,7 +171,7 @@ class OC_Setup {
 
 				try {
 					self::setupMSSQLDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix);
-				} catch (Exception $e) {
+				} catch (DatabaseSetupException $e) {
 					$error[] = array(
 						'error' => 'MS SQL username and/or password not valid',
 						'hint' => 'You need to enter either an existing account or the administrator.'
@@ -326,7 +320,7 @@ class OC_Setup {
 		$connection_string = "host='$e_host' dbname=postgres user='$e_user' password='$e_password'";
 		$connection = @pg_connect($connection_string);
 		if(!$connection) {
-			throw new Exception($l->t('PostgreSQL username and/or password not valid'));
+			throw new DatabaseSetupException($l->t('PostgreSQL username and/or password not valid'));
 		}
 		$e_user = pg_escape_string($dbuser);
 		//check for roles creation rights in postgresql
@@ -371,7 +365,7 @@ class OC_Setup {
 		$connection_string = "host='$e_host' dbname='$e_dbname' user='$e_user' password='$e_password'";
 		$connection = @pg_connect($connection_string);
 		if(!$connection) {
-			throw new Exception($l->t('PostgreSQL username and/or password not valid'));
+			throw new DatabaseSetupException($l->t('PostgreSQL username and/or password not valid'));
 		}
 		$query = "select count(*) FROM pg_class WHERE relname='{$dbtableprefix}users' limit 1";
 		$result = pg_query($connection, $query);
@@ -461,9 +455,9 @@ class OC_Setup {
 		if(!$connection) {
 			$e = oci_error();
 			if (is_array ($e) && isset ($e['message'])) {
-				throw new Exception($e['message']);
+				throw new DatabaseSetupException($e['message']);
 			}
-			throw new Exception($l->t('Oracle username and/or password not valid'));
+			throw new DatabaseSetupException($l->t('Oracle username and/or password not valid'));
 		}
 		//check for roles creation rights in oracle
 
@@ -530,7 +524,7 @@ class OC_Setup {
 		}
 		$connection = @oci_connect($dbuser, $dbpass, $easy_connect_string);
 		if(!$connection) {
-			throw new Exception($l->t('Oracle username and/or password not valid'));
+			throw new DatabaseSetupException($l->t('Oracle username and/or password not valid'));
 		}
 		$query = "SELECT count(*) FROM user_tables WHERE table_name = :un";
 		$stmt = oci_parse($connection, $query);
@@ -641,7 +635,7 @@ class OC_Setup {
 			} else {
 				$entry = '';
 			}
-			throw new Exception($l->t('MS SQL username and/or password not valid: %s', array($entry)));
+			throw new DatabaseSetupException($l->t('MS SQL username and/or password not valid: %s', array($entry)));
 		}
 
 		OC_Config::setValue('dbuser', $dbuser);
diff --git a/lib/template.php b/lib/template.php
index 9467dedb62a..01f0fc28b60 100644
--- a/lib/template.php
+++ b/lib/template.php
@@ -535,4 +535,25 @@ class OC_Template{
 		$content->printPage();
 		die();
 	}
+	
+	/**
+	 * print error page using Exception details
+	 * @param Exception $exception
+	 */
+	
+	public static function printExceptionErrorPage(Exception $exception) {
+		$error_msg = $exception->getMessage();
+		if ($exception->getCode()) {
+			$error_msg = '['.$exception->getCode().'] '.$error_msg;
+		}
+		$hint = $exception->getTraceAsString();
+		while ($exception = $exception->previous()) {
+			$error_msg .= '<br/>Caused by: ';
+			if ($exception->getCode()) {
+				$error_msg .= '['.$exception->getCode().'] ';
+			}
+			$error_msg .= $exception->getMessage();
+		};
+		self::printErrorPage($error_msg, $hint);
+	}
 }