From b992b3aea5d1ddf8309d8d0a0c0158568353fab4 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 11 Apr 2018 15:56:44 -0700
Subject: [PATCH] rdbms: make select() warn when FOR UPDATE is used with
 aggregation

Using FOR UPDATE or LOCK IN SHARE MODE with aggregation leads to
query errors with PostgreSQL.

Bug: T160910
Change-Id: Iaed964e7e59468365cbc62cb4bfd3ad44b898452
---
 includes/libs/rdbms/database/Database.php     | 68 +++++++++++++++++--
 .../libs/rdbms/database/DatabaseSQLTest.php   | 14 +++-
 2 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 1779880d27..31ff3dcfdc 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -1641,8 +1641,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		return '';
 	}
 
-	public function select( $table, $vars, $conds = '', $fname = __METHOD__,
-		$options = [], $join_conds = [] ) {
+	public function select(
+		$table, $vars, $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+	) {
 		$sql = $this->selectSQLText( $table, $vars, $conds, $fname, $options, $join_conds );
 
 		return $this->query( $sql, $fname );
@@ -1652,7 +1653,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		$options = [], $join_conds = []
 	) {
 		if ( is_array( $vars ) ) {
-			$vars = implode( ',', $this->fieldNamesWithAlias( $vars ) );
+			$fields = implode( ',', $this->fieldNamesWithAlias( $vars ) );
+		} else {
+			$fields = $vars;
 		}
 
 		$options = (array)$options;
@@ -1666,6 +1669,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			? $options['IGNORE INDEX']
 			: [];
 
+		if (
+			$this->selectOptionsIncludeLocking( $options ) &&
+			$this->selectFieldsOrOptionsAggregate( $vars, $options )
+		) {
+			// Some DB types (postgres/oracle) disallow FOR UPDATE with aggregate
+			// functions. Discourage use of such queries to encourage compatibility.
+			call_user_func(
+				$this->deprecationLogger,
+				__METHOD__ . ": aggregation used with a locking SELECT ($fname)."
+			);
+		}
+
 		if ( is_array( $table ) ) {
 			$from = ' FROM ' .
 				$this->tableNamesWithIndexClauseOrJOIN(
@@ -1696,9 +1711,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		}
 
 		if ( $conds === '' ) {
-			$sql = "SELECT $startOpts $vars $from $useIndex $ignoreIndex $preLimitTail";
+			$sql = "SELECT $startOpts $fields $from $useIndex $ignoreIndex $preLimitTail";
 		} elseif ( is_string( $conds ) ) {
-			$sql = "SELECT $startOpts $vars $from $useIndex $ignoreIndex " .
+			$sql = "SELECT $startOpts $fields $from $useIndex $ignoreIndex " .
 				"WHERE $conds $preLimitTail";
 		} else {
 			throw new DBUnexpectedError( $this, __METHOD__ . ' called with incorrect parameters' );
@@ -1783,6 +1798,49 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		return isset( $row['rowcount'] ) ? (int)$row['rowcount'] : 0;
 	}
 
+	/**
+	 * @param string|array $options
+	 * @return bool
+	 */
+	private function selectOptionsIncludeLocking( $options ) {
+		$options = (array)$options;
+		foreach ( [ 'FOR UPDATE', 'LOCK IN SHARE MODE' ] as $lock ) {
+			if ( in_array( $lock, $options, true ) ) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+	/**
+	 * @param array|string $fields
+	 * @param array|string $options
+	 * @return bool
+	 */
+	private function selectFieldsOrOptionsAggregate( $fields, $options ) {
+		foreach ( (array)$options as $key => $value ) {
+			if ( is_string( $key ) ) {
+				if ( preg_match( '/^(?:GROUP BY|HAVING)$/i', $key ) ) {
+					return true;
+				}
+			} elseif ( is_string( $value ) ) {
+				if ( preg_match( '/^(?:DISTINCT|DISTINCTROW)$/i', $value ) ) {
+					return true;
+				}
+			}
+		}
+
+		$regex = '/^(?:COUNT|MIN|MAX|SUM|GROUP_CONCAT|LISTAGG|ARRAY_AGG)\s*\\(/i';
+		foreach ( (array)$fields as $field ) {
+			if ( is_string( $field ) && preg_match( $regex, $field ) ) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * @param array|string $conds
 	 * @param string $fname
diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index 3756da2613..cfe9ad0ccf 100644
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -46,6 +46,8 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
 	 * @covers Wikimedia\Rdbms\Database::makeSelectOptions
 	 * @covers Wikimedia\Rdbms\Database::makeOrderBy
 	 * @covers Wikimedia\Rdbms\Database::makeGroupByWithHaving
+	 * @covers Wikimedia\Rdbms\Database::selectFieldsOrOptionsAggregate
+	 * @covers Wikimedia\Rdbms\Database::selectOptionsIncludeLocking
 	 */
 	public function testSelect( $sql, $sqlText ) {
 		$this->database->select(
@@ -223,9 +225,17 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
 				[
 					'tables' => 'table',
 					'fields' => [ 'field' ],
-					'options' => [ 'DISTINCT', 'LOCK IN SHARE MODE' ],
+					'options' => [ 'DISTINCT' ],
 				],
-				"SELECT DISTINCT field FROM table      LOCK IN SHARE MODE"
+				"SELECT DISTINCT field FROM table"
+			],
+			[
+				[
+					'tables' => 'table',
+					'fields' => [ 'field' ],
+					'options' => [ 'LOCK IN SHARE MODE' ],
+				],
+				"SELECT field FROM table      LOCK IN SHARE MODE"
 			],
 			[
 				[
-- 
2.20.1