From: Tim Otten 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'],