From: Eileen McNaughton Date: Wed, 4 Aug 2021 21:39:59 +0000 (+1200) Subject: dev/core#2725 Fix regression permitting circular group resolution X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=0a0b59bd44966769d281423c61009eade2acbe85;p=civicrm-core.git dev/core#2725 Fix regression permitting circular group resolution --- diff --git a/CRM/Contact/BAO/GroupContactCache.php b/CRM/Contact/BAO/GroupContactCache.php index 740ba9b704..537ef19567 100644 --- a/CRM/Contact/BAO/GroupContactCache.php +++ b/CRM/Contact/BAO/GroupContactCache.php @@ -374,14 +374,14 @@ WHERE id IN ( $groupIDs ) self::invalidateGroupContactCache($group->id); } - $locks = self::getLocksForRefreshableGroupsTo([$groupID]); - foreach ($locks as $groupID => $lock) { + $lockedGroups = self::getLocksForRefreshableGroupsTo([$groupID]); + foreach ($lockedGroups as $groupID) { $groupContactsTempTable = CRM_Utils_SQL_TempTable::build() ->setCategory('gccache') ->setMemory(); self::buildGroupContactTempTable([$groupID], $groupContactsTempTable); self::updateCacheFromTempTable($groupContactsTempTable, [$groupID]); - $lock->release(); + self::releaseGroupLocks([$groupID]); } } @@ -401,9 +401,8 @@ WHERE id IN ( $groupIDs ) $locks = []; $groupIDs = self::getGroupsNeedingRefreshing($groupIDs); foreach ($groupIDs as $groupID) { - $lock = Civi::lockManager()->acquire("data.core.group.{$groupID}"); - if ($lock->isAcquired()) { - $locks[$groupID] = $lock; + if (self::getGroupLock($groupID)) { + $locks[] = $groupID; } } return $locks; @@ -675,9 +674,9 @@ ORDER BY gc.contact_id, g.children $groupContactsTempTable = CRM_Utils_SQL_TempTable::build() ->setCategory('gccache') ->setMemory(); - $locks = self::getLocksForRefreshableGroupsTo($smartGroups); - if (!empty($locks)) { - self::buildGroupContactTempTable(array_keys($locks), $groupContactsTempTable); + $lockedGroups = self::getLocksForRefreshableGroupsTo($smartGroups); + if (!empty($lockedGroups)) { + self::buildGroupContactTempTable($lockedGroups, $groupContactsTempTable); // Note in theory we could do this transfer from the temp // table to the group_contact_cache table out-of-process - possibly by // continuing on after the browser is released (which seems to be @@ -691,11 +690,8 @@ ORDER BY gc.contact_id, g.children // Also - if we switched to the 'triple union' approach described above // we could throw a try-catch around this line since best-effort would // be good enough & potentially improve user experience. - self::updateCacheFromTempTable($groupContactsTempTable, array_keys($locks)); - - foreach ($locks as $lock) { - $lock->release(); - } + self::updateCacheFromTempTable($groupContactsTempTable, $lockedGroups); + self::releaseGroupLocks($lockedGroups); } $smartGroups = implode(',', $smartGroups); @@ -874,4 +870,42 @@ AND civicrm_group_contact.group_id = $groupID "; } } + /** + * Get a lock, if available, for the given group. + * + * @param int $groupID + * + * @return bool + * @throws \CRM_Core_Exception + */ + protected static function getGroupLock(int $groupID): bool { + $cacheKey = "data.core.group.$groupID"; + if (isset(Civi::$statics["data.core.group.$groupID"])) { + // Loop avoidance for a circular parent-child situation. + // This would occur where the parent is a criteria of the child + // but needs to resolve the child to resolve itself. + // This has a unit test - testGroupWithParentInCriteria + return FALSE; + } + $lock = Civi::lockManager()->acquire($cacheKey); + if ($lock->isAcquired()) { + Civi::$statics["data.core.group.$groupID"] = $lock; + return TRUE; + } + return FALSE; + } + + /** + * Release locks on the groups. + * + * @param array $groupIDs + */ + protected static function releaseGroupLocks(array $groupIDs): void { + foreach ($groupIDs as $groupID) { + $lock = Civi::$statics["data.core.group.$groupID"]; + $lock->release(); + unset(Civi::$statics["data.core.group.$groupID"]); + } + } + } diff --git a/tests/phpunit/CRM/Contact/BAO/GroupTest.php b/tests/phpunit/CRM/Contact/BAO/GroupTest.php index 14262dda0f..e013805d06 100644 --- a/tests/phpunit/CRM/Contact/BAO/GroupTest.php +++ b/tests/phpunit/CRM/Contact/BAO/GroupTest.php @@ -9,6 +9,9 @@ +--------------------------------------------------------------------+ */ +use Civi\Api4\Group; +use Civi\Api4\SavedSearch; + /** * Test class for CRM_Contact_BAO_Group BAO * @@ -51,13 +54,13 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { * Test case to ensure child group is present in the hierarchy * if it has multiple parent groups and not all are disabled. */ - public function testGroupHirearchy() { + public function testGroupHirearchy(): void { // Use-case : // 1. Create two parent group A and B and disable B // 2. Create a child group C // 3. Ensure that Group C is present in the group hierarchy $params = [ - 'name' => uniqid(), + 'name' => 'parent group a', 'title' => 'Parent Group A', 'description' => 'Parent Group One', 'visibility' => 'User and User Admin Only', @@ -66,7 +69,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { $group1 = CRM_Contact_BAO_Group::create($params); $params = array_merge($params, [ - 'name' => uniqid(), + 'name' => 'parent group b', 'title' => 'Parent Group B', 'description' => 'Parent Group Two', // disable @@ -75,7 +78,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { $group2 = CRM_Contact_BAO_Group::create($params); $params = array_merge($params, [ - 'name' => uniqid(), + 'name' => 'parent group c', 'title' => 'Child Group C', 'description' => 'Child Group C', 'parents' => [ @@ -103,9 +106,9 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { /** * Test nestedGroup pseudoconstant */ - public function testNestedGroup() { + public function testNestedGroup(): void { $params = [ - 'name' => 'groupa', + 'name' => 'group a', 'title' => 'Parent Group A', 'description' => 'Parent Group One', 'visibility' => 'User and User Admin Only', @@ -116,7 +119,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { $group1 = CRM_Contact_BAO_Group::create($params); $params = [ - 'name' => 'groupb', + 'name' => 'group b', 'title' => 'Parent Group B', 'description' => 'Parent Group Two', 'visibility' => 'User and User Admin Only', @@ -125,7 +128,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { $group2 = CRM_Contact_BAO_Group::create($params); $params = [ - 'name' => 'groupc', + 'name' => 'group c', 'title' => 'Child Group C', 'description' => 'Child Group C', 'visibility' => 'User and User Admin Only', @@ -154,10 +157,35 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { ], $nestedGroup); } + /** + * Test that parents as criteria don't cause loops. + * + * @throws \API_Exception + */ + public function testGroupWithParentInCriteria(): void { + $parentGroupID = Group::create()->setValues([ + 'name' => 'Parent', + 'title' => 'Parent', + ])->execute()->first()['id']; + $savedSearchID = SavedSearch::create()->setValues([ + 'form_values' => [ + ['group', '=', ['IN' => [$parentGroupID]], 0, 0], + ], + 'name' => 'child', + ])->execute()->first()['id']; + $childGroupID = Group::create()->setValues([ + 'name' => 'Child', + 'title' => 'Child', + 'saved_search_id' => $savedSearchID, + 'parents' => [$parentGroupID], + ])->execute()->first()['id']; + $this->callAPISuccess('Contact', 'get', ['group' => $childGroupID]); + } + /** * Test adding a smart group. */ - public function testAddSmart() { + public function testAddSmart(): void { $checkParams = $params = [ 'title' => 'Group Dos', @@ -289,7 +317,7 @@ class CRM_Contact_BAO_GroupTest extends CiviUnitTestCase { 'search_custom_id' => NULL, 'search_context' => 'advanced', ]; - list($smartGroupID, $savedSearchID) = CRM_Contact_BAO_Group::createHiddenSmartGroup($hiddenSmartParams); + [$smartGroupID, $savedSearchID] = CRM_Contact_BAO_Group::createHiddenSmartGroup($hiddenSmartParams); $mailingID = $this->callAPISuccess('Mailing', 'create', [])['id']; $this->callAPISuccess('MailingGroup', 'create', [