Merge pull request #21025 from eileenmcnaughton/539

#2725 Fix regression permitting circular group resolution

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"]);
+    }
+  }
+
 }

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', [