From 6ba05f843e4928601d5a8024c285c8bc219fc20b Mon Sep 17 00:00:00 2001
From: colemanw <coleman@civicrm.org>
Date: Wed, 14 Jun 2023 15:27:06 -0400
Subject: [PATCH] CRM_Utils_SQL_Select - Fix missing derived table alias and
 add stricter validation

Don't accept sql strings, all sets must be a CRM_Utils_SQL_Select object.
---
 CRM/Utils/SQL/Select.php                   | 22 ++++++++++++++--------
 tests/phpunit/CRM/Utils/SQL/SelectTest.php | 10 +++++-----
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/CRM/Utils/SQL/Select.php b/CRM/Utils/SQL/Select.php
index 2f32fdb437..d7a52b002e 100644
--- a/CRM/Utils/SQL/Select.php
+++ b/CRM/Utils/SQL/Select.php
@@ -349,11 +349,12 @@ class CRM_Utils_SQL_Select extends CRM_Utils_SQL_BaseParamQuery {
    * Ex: CRM_Utils_SQL_Select::fromSet()->union([$subQuery1, $subQuery2])
    * Ex: CRM_Utils_SQL_Select::fromSet()->union($subQuery1)->union($subQuery2);
    *
-   * @param string|array|\CRM_Utils_SQL_Select $subQueries
+   * @param string $type "DISTINCT"|"ALL"
+   * @param CRM_Utils_SQL_Select[]|CRM_Utils_SQL_Select $subQueries
    * @return $this
    */
-  public function union($subQueries) {
-    return $this->setOp('UNION', $subQueries);
+  public function union(string $type, $subQueries) {
+    return $this->setOp("UNION $type", $subQueries);
   }
 
   /**
@@ -362,15 +363,20 @@ class CRM_Utils_SQL_Select extends CRM_Utils_SQL_BaseParamQuery {
    * Ex: CRM_Utils_SQL_Select::fromSet()->setOp('INTERSECT', [$subQuery1, $subQuery2])
    *
    * @param string $setOperation
-   *   Ex: 'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'
-   *   NOTE: The query-builder supports any set-operation. However, MySQL 5.7 only supports UNION.
-   * @param string|array|\CRM_Utils_SQL_Select $subQueries
+   *   Ex: 'UNION DISTINCT', 'UNION ALL'.
+   *   TODO: 'INTERSECT', 'EXCEPT' when moving to MySQL 8.
+   * @param CRM_Utils_SQL_Select[]|CRM_Utils_SQL_Select $subQueries
    * @return $this
    * @see https://dev.mysql.com/doc/refman/8.0/en/set-operations.html
    */
   public function setOp(string $setOperation, $subQueries) {
+    // TODO: Support more ops like 'INTERSECT' & 'EXCEPT'
+    $supportedOps = ['UNION DISTINCT', 'UNION ALL'];
+    if (!in_array($setOperation, $supportedOps, TRUE)) {
+      throw new CRM_Core_Exception("Unsupported set-operation '$setOperation'. Must be one of (" . implode(', ', $supportedOps) . ')');
+    }
     if ($this->from !== NULL || !is_array($this->setOps)) {
-      throw new CRM_Core_Exception("Set-operation ($setOperation) must have a list of subqueries. Primitive FROM is not supported.");
+      throw new CRM_Core_Exception("Set-operation '$setOperation' must have a list of subqueries. Primitive FROM is not supported.");
     }
     $subQueries = is_array($subQueries) ? $subQueries : [$subQueries]; /* Simple (array)cast would mishandle objects. */
     foreach ($subQueries as $subQuery) {
@@ -555,7 +561,7 @@ class CRM_Utils_SQL_Select extends CRM_Utils_SQL_BaseParamQuery {
         $sql .= $setOp[0];
         $sql .= '(' . (is_object($setOp[1]) ? $setOp[1]->toSQL() : $setOp[1]) . ')';
       }
-      $sql .= ")\n";
+      $sql .= ") a\n";
     }
     foreach ($this->joins as $join) {
       $sql .= $join . "\n";
diff --git a/tests/phpunit/CRM/Utils/SQL/SelectTest.php b/tests/phpunit/CRM/Utils/SQL/SelectTest.php
index 931b1ab33b..386a522b6f 100644
--- a/tests/phpunit/CRM/Utils/SQL/SelectTest.php
+++ b/tests/phpunit/CRM/Utils/SQL/SelectTest.php
@@ -311,10 +311,10 @@ class CRM_Utils_SQL_SelectTest extends CiviUnitTestCase {
   public function testUnion() {
     $a = CRM_Utils_SQL_Select::from('a')->select('a_name')->where('a1 = !num')->param('num', 100);
     $b = CRM_Utils_SQL_Select::from('b')->select('b_name')->where('b2 = @val')->param('val', "ab cd");
-    $u = CRM_Utils_SQL_Select::fromSet()->union([$a, $b])->limit(100)->orderBy('a_name');
+    $u = CRM_Utils_SQL_Select::fromSet()->union('ALL', [$a, $b])->limit(100)->orderBy('a_name');
     $expectA = 'SELECT a_name FROM a WHERE (a1 = 100) ';
     $expectB = 'SELECT b_name FROM b WHERE (b2 = "ab cd") ';
-    $expectUnion = "SELECT * FROM (($expectA) UNION ($expectB)) ORDER BY a_name LIMIT 100 OFFSET 0";
+    $expectUnion = "SELECT * FROM (($expectA) UNION ALL ($expectB)) a ORDER BY a_name LIMIT 100 OFFSET 0";
     $this->assertLike($expectUnion, $u->toSQL());
   }
 
@@ -323,14 +323,14 @@ class CRM_Utils_SQL_SelectTest extends CiviUnitTestCase {
     $b = CRM_Utils_SQL_Select::from('b')->select('b_name')->where('b2 = @val')->param('val', "bb bb");
     $c = CRM_Utils_SQL_Select::from('c')->select('c_name')->where('c3 = @val')->param('val', "cc cc");
     $u = CRM_Utils_SQL_Select::fromSet()
-      ->union([$a, $b])
-      ->setOp('INTERSECT', $c)
+      ->union('ALL', [$a, $b])
+      ->union('DISTINCT', $c)
       ->limit(100)
       ->orderBy('a_name');
     $expectA = 'SELECT a_name FROM a WHERE (a1 = 100) ';
     $expectB = 'SELECT b_name FROM b WHERE (b2 = "bb bb") ';
     $expectC = 'SELECT c_name FROM c WHERE (c3 = "cc cc") ';
-    $expectUnion = "SELECT * FROM (($expectA) UNION ($expectB) INTERSECT ($expectC)) ORDER BY a_name LIMIT 100 OFFSET 0";
+    $expectUnion = "SELECT * FROM (($expectA) UNION ALL ($expectB) UNION DISTINCT ($expectC)) a ORDER BY a_name LIMIT 100 OFFSET 0";
     $this->assertLike($expectUnion, $u->toSQL());
   }
 
-- 
2.25.1