From 2ca46d4d5a8cd15929ac0939ca2bb380a3de027e Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 14 Aug 2018 15:12:57 -0700 Subject: [PATCH 1/4] (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. --- CRM/Contact/BAO/Query.php | 40 ++++++++++++++++++------- CRM/Contact/Selector.php | 5 +++- CRM/Core/PrevNextCache/Interface.php | 11 +++++++ CRM/Core/PrevNextCache/Sql.php | 23 ++++++++++++++ tests/phpunit/E2E/Core/PrevNextTest.php | 13 ++++++++ 5 files changed, 80 insertions(+), 12 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index e118fd7fc681..71497d2e2490 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -4949,29 +4949,47 @@ public function searchQuery( } /** - * 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 f048612ab711..f60d434fbdf3 100644 --- a/CRM/Contact/Selector.php +++ b/CRM/Contact/Selector.php @@ -578,8 +578,11 @@ public function &getRows($action, $offset, $rowCount, $sort, $output = NULL) { // 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 edaf0d6778ce..5f3bfabd1e13 100644 --- a/CRM/Core/PrevNextCache/Interface.php +++ b/CRM/Core/PrevNextCache/Interface.php @@ -114,4 +114,15 @@ public function deleteItem($id = NULL, $cacheKey = NULL); */ 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 5a5897353332..fc73de4408bf 100644 --- a/CRM/Core/PrevNextCache/Sql.php +++ b/CRM/Core/PrevNextCache/Sql.php @@ -244,4 +244,27 @@ public function getCount($cacheKey) { 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 3ca3c48aa68b..d416e564b814 100644 --- a/tests/phpunit/E2E/Core/PrevNextTest.php +++ b/tests/phpunit/E2E/Core/PrevNextTest.php @@ -93,6 +93,19 @@ public function testFillArray() { $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'], From 751f3d98eae7c9b9675d219e0c4795da6b2ce98a Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Sun, 8 Jul 2018 18:02:01 -0700 Subject: [PATCH 2/4] (dev/core#217) Implement Redis driver for PrevNext handling --- CRM/Core/PrevNextCache/Redis.php | 256 +++++++++++++++++++++++++++++++ Civi/Core/Container.php | 15 ++ 2 files changed, 271 insertions(+) create mode 100644 CRM/Core/PrevNextCache/Redis.php diff --git a/CRM/Core/PrevNextCache/Redis.php b/CRM/Core/PrevNextCache/Redis.php new file mode 100644 index 000000000000..bc7d05b4d7b3 --- /dev/null +++ b/CRM/Core/PrevNextCache/Redis.php @@ -0,0 +1,256 @@ +redis = CRM_Utils_Cache_Redis::connect($settings); + $this->prefix = isset($settings['prefix']) ? $settings['prefix'] : ''; + $this->prefix .= \CRM_Utils_Cache::DELIMITER . 'prevnext' . \CRM_Utils_Cache::DELIMITER; + } + + public function fillWithSql($cacheKey, $sql) { + $dao = CRM_Core_DAO::executeQuery($sql, [], FALSE, NULL, FALSE, TRUE, TRUE); + if (is_a($dao, 'DB_Error')) { + throw new CRM_Core_Exception($dao->message); + } + + list($allKey, $dataKey, , $maxScore) = $this->initCacheKey($cacheKey); + + while ($dao->fetch()) { + list (, $entity_id, $data) = array_values($dao->toArray()); + $maxScore++; + $this->redis->zAdd($allKey, $maxScore, $entity_id); + $this->redis->hSet($dataKey, $entity_id, $data); + } + + $dao->free(); + return TRUE; + } + + public function fillWithArray($cacheKey, $rows) { + list($allKey, $dataKey, , $maxScore) = $this->initCacheKey($cacheKey); + + foreach ($rows as $row) { + $maxScore++; + $this->redis->zAdd($allKey, $maxScore, $row['entity_id1']); + $this->redis->hSet($dataKey, $row['entity_id1'], $row['data']); + } + + return TRUE; + } + + public function fetch($cacheKey, $offset, $rowCount) { + $allKey = $this->key($cacheKey, 'all'); + return $this->redis->zRange($allKey, $offset, $offset + $rowCount - 1); + } + + public function markSelection($cacheKey, $action, $ids = NULL) { + $allKey = $this->key($cacheKey, 'all'); + $selKey = $this->key($cacheKey, 'sel'); + + if ($action === 'select') { + foreach ((array) $ids as $id) { + $score = $this->redis->zScore($allKey, $id); + $this->redis->zAdd($selKey, $score, $id); + } + } + elseif ($action === 'unselect' && $ids === NULL) { + $this->redis->delete($selKey); + $this->redis->setTimeout($selKey, self::TTL); + } + elseif ($action === 'unselect' && $ids !== NULL) { + foreach ((array) $ids as $id) { + $this->redis->zDelete($selKey, $id); + } + } + } + + public function getSelection($cacheKey, $action = 'get') { + $allKey = $this->key($cacheKey, 'all'); + $selKey = $this->key($cacheKey, 'sel'); + + if ($action === 'get') { + $result = []; + foreach ($this->redis->zRange($selKey, 0, -1) as $entity_id) { + $result[$entity_id] = 1; + } + return [$cacheKey => $result]; + } + elseif ($action === 'getall') { + $result = []; + foreach ($this->redis->zRange($allKey, 0, -1) as $entity_id) { + $result[$entity_id] = 1; + } + return [$cacheKey => $result]; + } + else { + throw new \CRM_Core_Exception("Unrecognized action: $action"); + } + } + + public function getPositions($cacheKey, $id1) { + $allKey = $this->key($cacheKey, 'all'); + $dataKey = $this->key($cacheKey, 'data'); + + $rank = $this->redis->zRank($allKey, $id1); + if (!is_int($rank) || $rank < 0) { + return ['foundEntry' => 0]; + } + + $pos = ['foundEntry' => 1]; + + if ($rank > 0) { + $pos['prev'] = []; + foreach ($this->redis->zRange($allKey, $rank - 1, $rank - 1) as $value) { + $pos['prev']['id1'] = $value; + } + $pos['prev']['data'] = $this->redis->hGet($dataKey, $pos['prev']['id1']); + } + + $count = $this->getCount($cacheKey); + if ($count > $rank + 1) { + $pos['next'] = []; + foreach ($this->redis->zRange($allKey, $rank + 1, $rank + 1) as $value) { + $pos['next']['id1'] = $value; + } + $pos['next']['data'] = $this->redis->hGet($dataKey, $pos['next']['id1']); + } + + return $pos; + } + + public function deleteItem($id = NULL, $cacheKey = NULL) { + if ($id === NULL && $cacheKey !== NULL) { + // Delete by cacheKey. + $allKey = $this->key($cacheKey, 'all'); + $selKey = $this->key($cacheKey, 'sel'); + $dataKey = $this->key($cacheKey, 'data'); + $this->redis->delete($allKey, $selKey, $dataKey); + } + elseif ($id === NULL && $cacheKey === NULL) { + // Delete everything. + $keys = $this->redis->keys($this->prefix . '*'); + $this->redis->del($keys); + } + elseif ($id !== NULL && $cacheKey !== NULL) { + // Delete a specific contact, within a specific cache. + $this->redis->zDelete($this->key($cacheKey, 'all'), $id); + $this->redis->zDelete($this->key($cacheKey, 'sel'), $id); + $this->redis->hDel($this->key($cacheKey, 'data'), $id); + } + elseif ($id !== NULL && $cacheKey === NULL) { + // Delete a specific contact, across all prevnext caches. + $allKeys = $this->redis->keys($this->key('*', 'all')); + foreach ($allKeys as $allKey) { + $parts = explode(\CRM_Utils_Cache::DELIMITER, $allKey); + array_pop($parts); + $tmpCacheKey = array_pop($parts); + $this->deleteItem($id, $tmpCacheKey); + } + } + else { + throw new CRM_Core_Exception("Not implemented: Redis::deleteItem"); + } + } + + public function getCount($cacheKey) { + $allKey = $this->key($cacheKey, 'all'); + return $this->redis->zSize($allKey); + } + + /** + * Construct the full path to a cache item. + * + * @param string $cacheKey + * Identifier for this saved search. + * Ex: 'abcd1234abcd1234'. + * @param string $item + * Ex: 'list', 'rel', 'data'. + * @return string + * Ex: 'dmaster/prevnext/abcd1234abcd1234/list' + */ + private function key($cacheKey, $item) { + return $this->prefix . $cacheKey . \CRM_Utils_Cache::DELIMITER . $item; + } + + /** + * Initialize any data-structures or timeouts for the cache-key. + * + * This is non-destructive -- if data already exists, it's preserved. + * + * @return array + * 0 => string $allItemsCacheKey, + * 1 => string $dataItemsCacheKey, + * 2 => string $selectedItemsCacheKey, + * 3 => int $maxExistingScore + */ + private function initCacheKey($cacheKey) { + $allKey = $this->key($cacheKey, 'all'); + $selKey = $this->key($cacheKey, 'sel'); + $dataKey = $this->key($cacheKey, 'data'); + + $this->redis->setTimeout($allKey, self::TTL); + $this->redis->setTimeout($dataKey, self::TTL); + $this->redis->setTimeout($selKey, self::TTL); + + $maxScore = 0; + foreach ($this->redis->zRange($allKey, -1, -1, TRUE) as $lastElem => $lastScore) { + $maxScore = $lastScore; + } + return array($allKey, $dataKey, $selKey, $maxScore); + } + +} diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index 9ca66a67c1c1..059be408b68b 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -230,6 +230,14 @@ public function createContainer() { [] )); + $container->setDefinition('prevnext.driver.redis', new Definition( + 'CRM_Core_PrevNextCache_Redis', + [new Reference('cache_config')] + )); + + $container->setDefinition('cache_config', new Definition('ArrayObject')) + ->setFactory(array(new Reference(self::SELF), 'createCacheConfig')); + $container->setDefinition('civi.mailing.triggers', new Definition( 'Civi\Core\SqlTrigger\TimestampTriggers', array('civicrm_mailing', 'Mailing') @@ -440,6 +448,13 @@ public static function createPrevNextCache($container) { : $container->get('prevnext.driver.sql'); } + public static function createCacheConfig() { + $driver = \CRM_Utils_Cache::getCacheDriver(); + $settings = \CRM_Utils_Cache::getCacheSettings($driver); + $settings['driver'] = $driver; + return new \ArrayObject($settings); + } + /** * Get a list of boot services. * From 514813c01ff2eb53c58d55b672510f54ca3b46bb Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 26 Jul 2018 16:00:54 -0700 Subject: [PATCH 3/4] (dev/core#217) PrevNext - Cleanup parameter name in Sql::markSelection The new name is prettier and matches the names in `CRM_Core_PrevNextCache_{Interface,Redis}`. --- CRM/Core/PrevNextCache/Sql.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/CRM/Core/PrevNextCache/Sql.php b/CRM/Core/PrevNextCache/Sql.php index fc73de4408bf..8771cb6c39c3 100644 --- a/CRM/Core/PrevNextCache/Sql.php +++ b/CRM/Core/PrevNextCache/Sql.php @@ -78,19 +78,19 @@ public function fillWithArray($cacheKey, $rows) { * @param string $cacheKey * @param string $action * Ex: 'select', 'unselect'. - * @param array|int|NULL $cIds + * @param array|int|NULL $ids * A list of contact IDs to (un)select. * To unselect all contact IDs, use NULL. */ - public function markSelection($cacheKey, $action, $cIds = NULL) { + public function markSelection($cacheKey, $action, $ids = NULL) { if (!$cacheKey) { return; } $params = array(); - if ($cIds && $cacheKey && $action) { - if (is_array($cIds)) { - $cIdFilter = "(" . implode(',', $cIds) . ")"; + if ($ids && $cacheKey && $action) { + if (is_array($ids)) { + $cIdFilter = "(" . implode(',', $ids) . ")"; $whereClause = " WHERE cacheKey = %1 AND (entity_id1 IN {$cIdFilter} OR entity_id2 IN {$cIdFilter}) @@ -101,7 +101,7 @@ public function markSelection($cacheKey, $action, $cIds = NULL) { WHERE cacheKey = %1 AND (entity_id1 = %2 OR entity_id2 = %2) "; - $params[2] = array("{$cIds}", 'Integer'); + $params[2] = array("{$ids}", 'Integer'); } if ($action == 'select') { $whereClause .= "AND is_selected = 0"; @@ -115,7 +115,7 @@ public function markSelection($cacheKey, $action, $cIds = NULL) { } // default action is reseting } - elseif (!$cIds && $cacheKey && $action == 'unselect') { + elseif (!$ids && $cacheKey && $action == 'unselect') { $sql = " UPDATE civicrm_prevnext_cache SET is_selected = 0 From e28bf654ec76cfc205e0ef5a7a5093959893361c Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Tue, 14 Aug 2018 16:08:16 -0700 Subject: [PATCH 4/4] (dev/core#217) PrevNext - Add settings for admin to choose backend The auto-detection is a good default policy. However, this is new functionality. If some bug gets through the review/RC cycles, then this option provides an escape path. --- CRM/Admin/Form/Setting/Miscellaneous.php | 2 ++ CRM/Core/BAO/PrevNextCache.php | 14 ++++++++++++++ Civi/Core/Container.php | 16 +++++++++++----- settings/Search.setting.php | 21 +++++++++++++++++++++ 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/CRM/Admin/Form/Setting/Miscellaneous.php b/CRM/Admin/Form/Setting/Miscellaneous.php index 31ef53a873c1..f741af3802cb 100644 --- a/CRM/Admin/Form/Setting/Miscellaneous.php +++ b/CRM/Admin/Form/Setting/Miscellaneous.php @@ -56,6 +56,7 @@ class CRM_Admin_Form_Setting_Miscellaneous extends CRM_Admin_Form_Setting { 'dedupe_default_limit' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, 'remote_profile_submissions' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, 'allow_alert_autodismissal' => CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, + 'prevNextBackend' => CRM_Core_BAO_Setting::SEARCH_PREFERENCES_NAME, ); public $_uploadMaxSize; @@ -77,6 +78,7 @@ public function preProcess() { 'recentItemsMaxCount', 'recentItemsProviders', 'dedupe_default_limit', + 'prevNextBackend', )); } diff --git a/CRM/Core/BAO/PrevNextCache.php b/CRM/Core/BAO/PrevNextCache.php index c3078140ac42..8a902d01c98a 100644 --- a/CRM/Core/BAO/PrevNextCache.php +++ b/CRM/Core/BAO/PrevNextCache.php @@ -483,4 +483,18 @@ public static function flipPair(array $prevNextId, $onlySelected) { } } + /** + * Get a list of available backend services. + * + * @return array + * Array(string $id => string $label). + */ + public static function getPrevNextBackends() { + return [ + 'default' => ts('Default (Auto-detect)'), + 'sql' => ts('SQL'), + 'redis' => ts('Redis'), + ]; + } + } diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index 059be408b68b..d75a9a13dd95 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -441,11 +441,17 @@ public static function createResources($container) { * @return \CRM_Core_PrevNextCache_Interface */ public static function createPrevNextCache($container) { - $cacheDriver = \CRM_Utils_Cache::getCacheDriver(); - $service = 'prevnext.driver.' . strtolower($cacheDriver); - return $container->has($service) - ? $container->get($service) - : $container->get('prevnext.driver.sql'); + $setting = \Civi::settings()->get('prevNextBackend'); + if ($setting === 'default') { + $cacheDriver = \CRM_Utils_Cache::getCacheDriver(); + $service = 'prevnext.driver.' . strtolower($cacheDriver); + return $container->has($service) + ? $container->get($service) + : $container->get('prevnext.driver.sql'); + } + else { + return $container->get('prevnext.driver.' . $setting); + } } public static function createCacheConfig() { diff --git a/settings/Search.setting.php b/settings/Search.setting.php index ab784705b9ff..6ab72238ca88 100644 --- a/settings/Search.setting.php +++ b/settings/Search.setting.php @@ -197,6 +197,27 @@ 'description' => 'If set, this will be the default profile used for contact search.', 'help_text' => NULL, ), + 'prevNextBackend' => array( + 'group_name' => 'Search Preferences', + 'group' => 'Search Preferences', + 'name' => 'prevNextBackend', + 'type' => 'String', + 'quick_form_type' => 'Select', + 'html_type' => 'Select', + 'html_attributes' => array( + //'class' => 'crm-select2', + ), + 'default' => 'default', + 'add' => '5.6', + 'title' => 'PrevNext Cache', + 'is_domain' => 1, + 'is_contact' => 0, + 'pseudoconstant' => array( + 'callback' => 'CRM_Core_BAO_PrevNextCache::getPrevNextBackends', + ), + 'description' => 'When performing a search, how should the search-results be cached?', + 'help_text' => '', + ), 'searchPrimaryDetailsOnly' => array( 'group_name' => 'Search Preferences', 'group' => 'Search Preferences',