Bug 4069 - query returns Result even when query result is FALSE (#4176)

* Initial commit fixing wrong return value in Database/BaseConnection::query. When underlying resultID if false, this should return false. Added BadQueryTest to tests.

* changes to make query() function return boolean for write-type queries. centralize isWriteType functionality in one place. Change tests impacted by this change

* improved BadQueryTest, simplified return value in BaseUtils::optimizeTable

* tiny change made by record...remove of one line in ConnectionInterface

* Removed static sqlIsWriteType method from Query class and instead to mimic CI3 added isWriteType declaration to ConnectionInterface, defined isWriteType fn in BaseConnection, added specific isWriteType to Postgres, modified various unit tests, and added WriteTypeQueryTest.

* Added RENAME back into BaseConnectin::isWriteType check and moved EXEC sp_rename into SQLSRV\Connection, tweaked WriteTypeQueryTest

* Tweaked BadQueryTest and DBUtilsTest to alter/restore DBDebug value for certain tests. Added changelog and upgrade notes.

* Update upgrade_412.rst

remove incorrect markdown

Author: sneakyimp

system/BaseModel.php

@@ -357,7 +357,7 @@ abstract protected function doFirst();
 	 *
 	 * @param array $data Data
 	 *
-	 * @return object|integer|string|false
+	 * @return integer|string|boolean
 	 */
 	abstract protected function doInsert(array $data);
 
@@ -407,7 +407,7 @@ abstract protected function doUpdateBatch(array $set = null, string $index = nul
 	 * @param integer|string|array|null $id    The rows primary key(s)
 	 * @param boolean                   $purge Allows overriding the soft deletes setting.
 	 *
-	 * @return object|boolean
+	 * @return string|boolean
 	 *
 	 * @throws DatabaseException
 	 */
@@ -666,11 +666,7 @@ public function save($data): bool
 		{
 			$response = $this->insert($data, false);
 
-			if ($response instanceof BaseResult)
-			{
-				$response = $response->resultID !== false;
-			}
-			elseif ($response !== false)
+			if ($response !== false)
 			{
 				$response = true;
 			}
@@ -708,7 +704,7 @@ public function getInsertID()
 	 * @param array|object|null $data     Data
 	 * @param boolean           $returnID Whether insert ID should be returned or not.
 	 *
-	 * @return BaseResult|object|integer|string|false
+	 * @return integer|string|boolean
 	 *
 	 * @throws ReflectionException
 	 */

system/Database/BaseBuilder.php

@@ -2252,7 +2252,7 @@ public function getCompiledInsert(bool $reset = true)
 	 *
 	 * @throws DatabaseException
 	 *
-	 * @return BaseResult|Query|false
+	 * @return Query|boolean
 	 */
 	public function insert(array $set = null, bool $escape = null)
 	{
@@ -2483,7 +2483,7 @@ public function update(array $set = null, $where = null, int $limit = null): boo
 
 			$result = $this->db->query($sql, $this->binds, false);
 
-			if ($result->resultID !== false)
+			if ($result !== false)
 			{
 				// Clear our binds so we don't eat up memory
 				$this->binds = [];
@@ -2835,7 +2835,7 @@ public function getCompiledDelete(bool $reset = true): string
 	 * @param integer $limit     The limit clause
 	 * @param boolean $resetData
 	 *
-	 * @return mixed
+	 * @return string|boolean
 	 * @throws DatabaseException
 	 */
 	public function delete($where = '', int $limit = null, bool $resetData = true)

system/Database/BaseConnection.php

@@ -612,7 +612,7 @@ abstract protected function execute(string $sql);
 	 * @param boolean $setEscapeFlags
 	 * @param string  $queryClass
 	 *
-	 * @return BaseResult|Query|false
+	 * @return BaseResult|Query|boolean
 	 *
 	 * @todo BC set $queryClass default as null in 4.1
 	 */
@@ -625,7 +625,6 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 			$this->initialize();
 		}
 
-		$resultClass = str_replace('Connection', 'Result', get_class($this));
 		/**
 		 * @var Query $query
 		 */
@@ -682,7 +681,7 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 				Events::trigger('DBQuery', $query);
 			}
 
-			return new $resultClass($this->connID, $this->resultID);
+			return false;
 		}
 
 		$query->setDuration($startTime);
@@ -696,7 +695,20 @@ public function query(string $sql, $binds = null, bool $setEscapeFlags = true, s
 		// If $pretend is true, then we just want to return
 		// the actual query object here. There won't be
 		// any results to return.
-		return $this->pretend ? $query : new $resultClass($this->connID, $this->resultID);
+		if ($this->pretend)
+		{
+			return $query;
+		}
+
+		// resultID is not false, so it must be successful
+		if ($this->isWriteType($sql))
+		{
+			return true;
+		}
+
+		// query is not write-type, so it must be read-type query; return QueryResult
+		$resultClass = str_replace('Connection', 'Result', get_class($this));
+		return new $resultClass($this->connID, $this->resultID);
 	}
 
 	//--------------------------------------------------------------------
@@ -1755,6 +1767,19 @@ public function resetDataCache()
 
 	//--------------------------------------------------------------------
 
+	/**
+	 * Determines if the statement is a write-type query or not.
+	 *
+	 * @param  string $sql
+	 * @return boolean
+	 */
+	public function isWriteType($sql): bool
+	{
+		return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql);
+	}
+
+	//--------------------------------------------------------------------
+
 	/**
 	 * Returns the last error code and message.
 	 *

system/Database/BaseUtils.php

@@ -120,7 +120,7 @@ public function databaseExists(string $databaseName): bool
 	 * Optimize Table
 	 *
 	 * @param  string $tableName
-	 * @return mixed
+	 * @return boolean
 	 * @throws DatabaseException
 	 */
 	public function optimizeTable(string $tableName)
@@ -135,13 +135,8 @@ public function optimizeTable(string $tableName)
 		}
 
 		$query = $this->db->query(sprintf($this->optimizeTable, $this->db->escapeIdentifiers($tableName)));
-		if ($query !== false)
-		{
-			$query = $query->getResultArray();
-			return current($query);
-		}
 
-		return false;
+		return ($query !== false) ? true : false;
 	}
 
 	//--------------------------------------------------------------------

system/Database/ConnectionInterface.php

@@ -116,7 +116,6 @@ public function getPlatform(): string;
 	public function getVersion(): string;
 
 	//--------------------------------------------------------------------
-
 	/**
 	 * Orchestrates a query against the database. Queries must use
 	 * Database\Statement objects to store the query and build it.
@@ -128,7 +127,7 @@ public function getVersion(): string;
 	 * @param string $sql
 	 * @param mixed  ...$binds
 	 *
-	 * @return mixed
+	 * @return BaseResult|Query|boolean
 	 */
 	public function query(string $sql, $binds = null);
 
@@ -193,4 +192,12 @@ public function escape($str);
 	public function callFunction(string $functionName, ...$params);
 
 	//--------------------------------------------------------------------
+
+	/**
+	 * Determines if the statement is a write-type query or not.
+	 *
+	 * @param  string $sql
+	 * @return boolean
+	 */
+	public function isWriteType($sql): bool;
 }

system/Database/Postgre/Connection.php

@@ -609,4 +609,24 @@ protected function _transRollback(): bool
 	}
 
 	// --------------------------------------------------------------------
+
+	/**
+	 * Determines if a query is a "write" type.
+	 *
+	 * Overrides BaseConnection::isWriteType, adding additional read query types.
+	 *
+	 * @param  string $sql An SQL query string
+	 * @return boolean
+	 */
+	public function isWriteType($sql): bool
+	{
+		if (preg_match('#^(INSERT|UPDATE).*RETURNING\s.+(\,\s?.+)*$#is', $sql))
+		{
+			return false;
+		}
+
+		return parent::isWriteType($sql);
+	}
+
+	// --------------------------------------------------------------------
 }

system/Database/Query.php

@@ -280,10 +280,7 @@ public function getErrorMessage(): string
 	 */
 	public function isWriteType(): bool
 	{
-		return (bool) preg_match(
-			'/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX)\s/i',
-			$this->originalQueryString
-		);
+		return $this->db->isWriteType($this->originalQueryString);
 	}
 
 	/**

system/Database/SQLSRV/Connection.php

@@ -11,6 +11,7 @@
 
 namespace CodeIgniter\Database\SQLSRV;
 
+use CodeIgniter\Database\Query;
 use CodeIgniter\Database\BaseConnection;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use Exception;
@@ -501,17 +502,6 @@ public function execute(string $sql)
 		return $stmt;
 	}
 
-	/**
-	 * Determines if a query is a "write" type.
-	 *
-	 * @param  string $sql An SQL query string
-	 * @return boolean
-	 */
-	public function isWriteType($sql)
-	{
-		return (bool) preg_match('/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX|MERGE)\s/i', $sql);
-	}
-
 	/**
 	 * Returns the last error encountered by this connection.
 	 *
@@ -578,4 +568,27 @@ public function getVersion(): string
 
 		return isset($info['SQLServerVersion']) ? $this->dataCache['version'] = $info['SQLServerVersion'] : false;
 	}
+
+	// --------------------------------------------------------------------
+
+	/**
+	 * Determines if a query is a "write" type.
+	 *
+	 * Overrides BaseConnection::isWriteType, adding additional read query types.
+	 *
+	 * @param  string $sql An SQL query string
+	 * @return boolean
+	 */
+	public function isWriteType($sql): bool
+	{
+		if (preg_match('/^\s*"?(EXEC\s*sp_rename)\s/i', $sql))
+		{
+			return true;
+		}
+
+		return parent::isWriteType($sql);
+	}
+
+	// --------------------------------------------------------------------
+
 }

system/Database/SQLite3/Connection.php

@@ -11,6 +11,7 @@
 
 namespace CodeIgniter\Database\SQLite3;
 
+use CodeIgniter\Database\Query;
 use CodeIgniter\Database\BaseConnection;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use ErrorException;
@@ -488,20 +489,6 @@ protected function _transRollback(): bool
 
 	//--------------------------------------------------------------------
 
-	/**
-	 * Determines if the statement is a write-type query or not.
-	 *
-	 * @return boolean
-	 */
-	public function isWriteType($sql): bool
-	{
-		return (bool) preg_match(
-			'/^\s*"?(SET|INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|TRUNCATE|LOAD|COPY|ALTER|RENAME|GRANT|REVOKE|LOCK|UNLOCK|REINDEX)\s/i',
-			$sql);
-	}
-
-	//--------------------------------------------------------------------
-
 	/**
 	 * Checks to see if the current install supports Foreign Keys
 	 * and has them enabled.

system/Model.php

@@ -19,6 +19,7 @@
 use CodeIgniter\Database\ConnectionInterface;
 use CodeIgniter\Database\Exceptions\DatabaseException;
 use CodeIgniter\Database\Exceptions\DataException;
+use CodeIgniter\Database\Query;
 use CodeIgniter\Exceptions\ModelException;
 use CodeIgniter\I18n\Time;
 use CodeIgniter\Validation\ValidationInterface;
@@ -250,7 +251,7 @@ protected function doFirst()
 	 *
 	 * @param array $data Data
 	 *
-	 * @return BaseResult|integer|string|false
+	 * @return Query|boolean
 	 */
 	protected function doInsert(array $data)
 	{
@@ -275,7 +276,7 @@ protected function doInsert(array $data)
 		$result = $builder->insert();
 
 		// If insertion succeeded then save the insert ID
-		if ($result->resultID)
+		if ($result)
 		{
 			if (! $this->useAutoIncrement)
 			{
@@ -376,7 +377,7 @@ protected function doUpdateBatch(array $set = null, string $index = null, int $b
 	 * @param integer|string|array|null $id    The rows primary key(s)
 	 * @param boolean                   $purge Allows overriding the soft deletes setting.
 	 *
-	 * @return BaseResult|boolean
+	 * @return string|boolean
 	 *
 	 * @throws DatabaseException
 	 */