From: Tim Otten <totten@civicrm.org>
Date: Tue, 14 Aug 2018 22:12:57 +0000 (-0700)
Subject: (dev/core#217) Query::getCachedContacts - Use swappable fetch() instead of SQL JOIN
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=2ca46d4d5a8cd15929ac0939ca2bb380a3de027e;p=civicrm-core.git

(dev/core#217) Query::getCachedContacts - Use swappable fetch() instead of SQL JOIN

The general context of this code is roughly as follows:

* We've already filled up the prevnext cache with a bunch of contact-IDs.
* The user wants to view a page of 50 contacts.
* We want to lookup full information about 50 specific contacts for this page.

It does makes sense to use `CRM_Contact_BAO_Query` for looking up the "full information"
about contacts. However, the function `Query::getCachedContacts()` is hard-coded to
read from the SQL-based prevnext cache.

Before
------

* In `getCachedContacts()`, it grabbed the full SQL for `CRM_Contact_BAO_Query`
  and munged the query to:
    * Add an extra JOIN on `civicrm_prevnext_cache` (with a constraint on `cacheKey`)
    * Respect pagination (LIMIT/OFFSET)
    * Order results based on their position in the prevnext cache

After
-----

* In `CRM_Core_PrevNextCache_Interface`, the `fetch()` function provides one page-worth
  of contact IDs (in order). The `fetch()` function is tested by `E2E_Core_PrevNextTest`.
* In `getCachedContacts()`, it doesn't know anything about `civicrm_prevnext_cache`
  or `cacheKey` or pagination.  Instead, it just accepts CIDs for one page-worth of
  contacts.  It returns contacts in the same order that was given.
---

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index e118fd7fc6..71497d2e24 100644
--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -4949,29 +4949,47 @@ civicrm_relationship.start_date > {$today}
   }
 
   /**
-   * Fetch a list of contacts from the prev/next cache for displaying a search results page
+   * Fetch a list of contacts for displaying a search results page
    *
-   * @param string $cacheKey
-   * @param int $offset
-   * @param int $rowCount
+   * @param array $cids
+   *   List of contact IDs
    * @param bool $includeContactIds
    * @return CRM_Core_DAO
    */
-  public function getCachedContacts($cacheKey, $offset, $rowCount, $includeContactIds) {
+  public function getCachedContacts($cids, $includeContactIds) {
+    CRM_Utils_Type::validateAll($cids, 'Positive');
     $this->_includeContactIds = $includeContactIds;
     $onlyDeleted = in_array(array('deleted_contacts', '=', '1', '0', '0'), $this->_params);
     list($select, $from, $where) = $this->query(FALSE, FALSE, FALSE, $onlyDeleted);
-    $from = " FROM civicrm_prevnext_cache pnc INNER JOIN civicrm_contact contact_a ON contact_a.id = pnc.entity_id1 AND pnc.cacheKey = '$cacheKey' " . substr($from, 31);
-    $order = " ORDER BY pnc.id";
-    $groupByCol = array('contact_a.id', 'pnc.id');
-    $select = self::appendAnyValueToSelect($this->_select, $groupByCol, 'GROUP_CONCAT');
-    $groupBy = " GROUP BY " . implode(', ', $groupByCol);
-    $limit = " LIMIT $offset, $rowCount";
+    $select .= sprintf(", (%s) AS _wgt", $this->createSqlCase('contact_a.id', $cids));
+    $where .= sprintf(' AND contact_a.id IN (%s)', implode(',', $cids));
+    $order = 'ORDER BY _wgt';
+    $groupBy = '';
+    $limit = '';
     $query = "$select $from $where $groupBy $order $limit";
 
     return CRM_Core_DAO::executeQuery($query);
   }
 
+  /**
+   * Construct a SQL CASE expression.
+   *
+   * @param string $idCol
+   *   The name of a column with ID's (eg 'contact_a.id').
+   * @param array $cids
+   *   Array(int $weight => int $id).
+   * @return string
+   *   CASE WHEN id=123 THEN 1 WHEN id=456 THEN 2 END
+   */
+  private function createSqlCase($idCol, $cids) {
+    $buf = "CASE\n";
+    foreach ($cids as $weight => $cid) {
+      $buf .= " WHEN $idCol = $cid THEN $weight \n";
+    }
+    $buf .= "END\n";
+    return $buf;
+  }
+
   /**
    * Populate $this->_permissionWhereClause with permission related clause and update other
    * query related properties.
diff --git a/CRM/Contact/Selector.php b/CRM/Contact/Selector.php
index f048612ab7..f60d434fbd 100644
--- a/CRM/Contact/Selector.php
+++ b/CRM/Contact/Selector.php
@@ -578,8 +578,11 @@ class CRM_Contact_Selector extends CRM_Core_Selector_Base implements CRM_Core_Se
     // and contain the search criteria (parameters)
     // note that the default action is basic
     if ($rowCount) {
+      /** @var CRM_Core_PrevNextCache_Interface $prevNext */
+      $prevNext = Civi::service('prevnext');
       $cacheKey = $this->buildPrevNextCache($sort);
-      $resultSet = $this->_query->getCachedContacts($cacheKey, $offset, $rowCount, $includeContactIds)->fetchGenerator();
+      $cids = $prevNext->fetch($cacheKey, $offset, $rowCount);
+      $resultSet = empty($cids) ? [] : $this->_query->getCachedContacts($cids, $includeContactIds)->fetchGenerator();
     }
     else {
       $resultSet = $this->_query->searchQuery($offset, $rowCount, $sort, FALSE, $includeContactIds)->fetchGenerator();
diff --git a/CRM/Core/PrevNextCache/Interface.php b/CRM/Core/PrevNextCache/Interface.php
index edaf0d6778..5f3bfabd1e 100644
--- a/CRM/Core/PrevNextCache/Interface.php
+++ b/CRM/Core/PrevNextCache/Interface.php
@@ -114,4 +114,15 @@ interface CRM_Core_PrevNextCache_Interface {
    */
   public function getCount($cacheKey);
 
+  /**
+   * Fetch a list of contacts from the prev/next cache for displaying a search results page
+   *
+   * @param string $cacheKey
+   * @param int $offset
+   * @param int $rowCount
+   * @return array
+   *   List of contact IDs (entity_id1).
+   */
+  public function fetch($cacheKey, $offset, $rowCount);
+
 }
diff --git a/CRM/Core/PrevNextCache/Sql.php b/CRM/Core/PrevNextCache/Sql.php
index 5a58973533..fc73de4408 100644
--- a/CRM/Core/PrevNextCache/Sql.php
+++ b/CRM/Core/PrevNextCache/Sql.php
@@ -244,4 +244,27 @@ ORDER BY id
     return (int) CRM_Core_DAO::singleValueQuery($query, $params, TRUE, FALSE);
   }
 
+  /**
+   * Fetch a list of contacts from the prev/next cache for displaying a search results page
+   *
+   * @param string $cacheKey
+   * @param int $offset
+   * @param int $rowCount
+   * @return array
+   *   List of contact IDs.
+   */
+  public function fetch($cacheKey, $offset, $rowCount) {
+    $cids = array();
+    $dao = CRM_Utils_SQL_Select::from('civicrm_prevnext_cache pnc')
+      ->where('pnc.cacheKey = @cacheKey', ['cacheKey' => $cacheKey])
+      ->select('pnc.entity_id1 as cid')
+      ->orderBy('pnc.id')
+      ->limit($rowCount, $offset)
+      ->execute();
+    while ($dao->fetch()) {
+      $cids[] = $dao->cid;
+    }
+    return $cids;
+  }
+
 }
diff --git a/tests/phpunit/E2E/Core/PrevNextTest.php b/tests/phpunit/E2E/Core/PrevNextTest.php
index 3ca3c48aa6..d416e564b8 100644
--- a/tests/phpunit/E2E/Core/PrevNextTest.php
+++ b/tests/phpunit/E2E/Core/PrevNextTest.php
@@ -93,6 +93,19 @@ class PrevNextTest extends \CiviEndToEndTestCase {
     $this->assertSelections([]);
   }
 
+  public function testFetch() {
+    $this->testFillArray();
+
+    $cids = $this->prevNext->fetch($this->cacheKey, 0, 2);
+    $this->assertEquals([100, 400], $cids);
+
+    $cids = $this->prevNext->fetch($this->cacheKey, 0, 4);
+    $this->assertEquals([100, 400, 200, 300], $cids);
+
+    $cids = $this->prevNext->fetch($this->cacheKey, 2, 2);
+    $this->assertEquals([200, 300], $cids);
+  }
+
   public function getFillFunctions() {
     return [
       ['testFillSql'],