Improve tests + make GroupService.isMember method more performant for special groups

dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java

@@ -7,6 +7,9 @@
  */
 package org.dspace.eperson;
 
+import com.google.common.base.Function;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -33,6 +36,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 
+import javax.annotation.Nullable;
 import java.sql.SQLException;
 import java.util.*;
 
@@ -153,8 +157,8 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
     }
 
     @Override
-    public boolean isMember(Group owningGroup, Group childGroup) {
+    public boolean isMember(Context context, Group owningGroup, Group childGroup) throws SQLException {
-        return owningGroup.contains(childGroup);
+        return group2GroupCacheDAO.findByParentAndChild(context, owningGroup, childGroup) != null;
     }
 
     @Override
@@ -173,40 +177,40 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
         } else if (StringUtils.equals(group.getName(), Group.ANONYMOUS)) {
             return true;
 
-        } else if(ePerson != null) {
+        } else {
-
             Boolean cachedGroupMembership = context.getCachedGroupMembership(group, ePerson);
+
             if(cachedGroupMembership != null) {
                 return cachedGroupMembership.booleanValue();
 
-                //If there are special groups we need to fetch all relevant groups.
+                //If there are special groups we need to check direct membership or check if the
-                //Note that special groups are only available if the current user == the ePerson.
+                //special group is a subgroup of the provided group.
+                //Note that special groups should only be checked if the current user == the ePerson.
+                //This also works for anonymous users (ePerson == null) if IP authentication used
             } else if(CollectionUtils.isNotEmpty(context.getSpecialGroups()) && isAuthenticatedUser(context, ePerson)) {
-                Set<Group> allMemberGroups = allMemberGroupsSet(context, ePerson);
+                boolean isMember = false;
-                boolean result = allMemberGroups.contains(group);
+                Iterator<Group> it = context.getSpecialGroups().iterator();
 
-                context.cacheGroupMembership(group, ePerson, result);
+                while (it.hasNext() && !isMember) {
-                return result;
+                    Group specialGroup =  it.next();
-            } else {
+                    if (specialGroup.equals(group) || isMember(context, group, specialGroup)) {
-                //lookup eperson in normal groups and subgroups
+                        isMember = true;
-                boolean result = epersonInGroup(context, group.getName(), ePerson);
-                context.cacheGroupMembership(group, ePerson, result);
-                return result;
-            }
-        } else if(isAuthenticatedUser(context, ePerson)) {
-            // Note that special groups are only available if the current user == the ePerson.
-            // Check also for anonymous users if IP authentication used
-            List<Group> specialGroups = context.getSpecialGroups();
-            if(CollectionUtils.isNotEmpty(specialGroups)) {
-                for(Group specialGroup : specialGroups){
-                    if (StringUtils.equals(specialGroup.getName(), group.getName())) {
-                        return true;
                     }
                 }
-            }
-        }
 
-        return false;
+                context.cacheGroupMembership(group, ePerson, isMember);
+                return isMember;
+
+            } else if(ePerson != null){
+                //lookup eperson in normal groups and subgroups with 1 query
+                boolean result = epersonInGroup(context, group.getName(), ePerson);
+
+                context.cacheGroupMembership(group, ePerson, result);
+                return result;
+            }
+
+            return false;
+        }
     }
 
     private boolean isAuthenticatedUser(final Context context, final EPerson ePerson) {

dspace-api/src/main/java/org/dspace/eperson/dao/Group2GroupCacheDAO.java

@@ -28,6 +28,8 @@ public interface Group2GroupCacheDAO extends GenericDAO<Group2GroupCache> {
 
     public List<Group2GroupCache> findByChildren(Context context, Iterable<Group> groups) throws SQLException;
 
+    public Group2GroupCache findByParentAndChild(Context context, Group parent, Group child) throws SQLException;
+
     public Group2GroupCache find(Context context, Group parent, Group child) throws SQLException;
 
     public void deleteAll(Context context) throws SQLException;

dspace-api/src/main/java/org/dspace/eperson/dao/impl/Group2GroupCacheDAOImpl.java

@@ -13,6 +13,7 @@ import org.dspace.eperson.Group;
 import org.dspace.eperson.Group2GroupCache;
 import org.dspace.eperson.dao.Group2GroupCacheDAO;
 import org.hibernate.Criteria;
+import org.hibernate.Query;
 import org.hibernate.criterion.Disjunction;
 import org.hibernate.criterion.Restrictions;
 
@@ -59,6 +60,19 @@ public class Group2GroupCacheDAOImpl extends AbstractHibernateDAO<Group2GroupCac
         return list(criteria);
     }
 
+    @Override
+    public Group2GroupCache findByParentAndChild(Context context, Group parent, Group child) throws SQLException {
+        Query query = createQuery(context,
+                "FROM Group2GroupCache g WHERE g.parent = :parentGroup AND g.child = :childGroup");
+
+        query.setParameter("parentGroup", parent);
+        query.setParameter("childGroup", child);
+
+        query.setCacheable(true);
+
+        return singleResult(query);
+    }
+
     @Override
     public Group2GroupCache find(Context context, Group parent, Group child) throws SQLException {
         Criteria criteria = createCriteria(context, Group2GroupCache.class);

dspace-api/src/main/java/org/dspace/eperson/service/GroupService.java

@@ -115,7 +115,7 @@ public interface GroupService extends DSpaceObjectService<Group>, DSpaceObjectLe
      * @param childGroup child group
      * @return true or false
      */
-    public boolean isMember(Group owningGroup, Group childGroup);
+    public boolean isMember(Context context, Group owningGroup, Group childGroup) throws SQLException;
 
     /**
      * fast check to see if an eperson is a member called with eperson id, does

dspace-api/src/test/java/org/dspace/eperson/GroupTest.java

@@ -329,10 +329,14 @@ public class GroupTest extends AbstractUnitTest {
     @Test
     public void isMemberGroup() throws SQLException
     {
-        assertTrue("isMemberGroup 1", groupService.isMember(topGroup, level1Group));
+        assertTrue("isMemberGroup 1", groupService.isMember(context, topGroup, level1Group));
-        assertTrue("isMemberGroup 2", groupService.isMember(level1Group, level2Group));
+        assertTrue("isMemberGroup 2", groupService.isMember(context, level1Group, level2Group));
-        assertFalse("isMemberGroup 3", groupService.isMember(level1Group, topGroup));
+        assertFalse("isMemberGroup 3", groupService.isMember(context, level1Group, topGroup));
-        assertFalse("isMemberGroup 4", groupService.isMember(level2Group, level1Group));
+        assertFalse("isMemberGroup 4", groupService.isMember(context, level2Group, level1Group));
+
+        //Also check ancestor relations
+        assertTrue("isMemberGroup 5", groupService.isMember(context, topGroup, level2Group));
+        assertFalse("isMemberGroup 6", groupService.isMember(context, level2Group, topGroup));
     }
 
     @Test
@@ -359,9 +363,9 @@ public class GroupTest extends AbstractUnitTest {
             ePerson = createEPersonAndAddToGroup("isMemberContext@dspace.org", level2Group);
 
             context.setCurrentUser(ePerson);
-            assertTrue(groupService.isMember(context, topGroup));
+            assertTrue(groupService.isMember(context, ePerson, topGroup));
-            assertTrue(groupService.isMember(context, level1Group));
+            assertTrue(groupService.isMember(context, ePerson, level1Group));
-            assertTrue(groupService.isMember(context, level2Group));
+            assertTrue(groupService.isMember(context, ePerson, level2Group));
         } finally {
             if(ePerson != null)
             {
@@ -377,10 +381,9 @@ public class GroupTest extends AbstractUnitTest {
         try {
             ePerson = createEPersonAndAddToGroup("isMemberContextGroupId@dspace.org", level2Group);
 
-            context.setCurrentUser(ePerson);
+            assertTrue(groupService.isMember(context, ePerson, topGroup.getName()));
-            assertTrue(groupService.isMember(context, topGroup));
+            assertTrue(groupService.isMember(context, ePerson, level1Group.getName()));
-            assertTrue(groupService.isMember(context, level1Group));
+            assertTrue(groupService.isMember(context, ePerson, level2Group.getName()));
-            assertTrue(groupService.isMember(context, level2Group));
         } finally {
             if(ePerson != null)
             {
@@ -390,6 +393,39 @@ public class GroupTest extends AbstractUnitTest {
         }
     }
 
+    @Test
+    public void isMemberContextSpecialGroup() throws SQLException, AuthorizeException, EPersonDeletionException, IOException {
+        EPerson ePerson = null;
+        Group specialGroup = null;
+        try {
+            specialGroup = createGroup("specialGroup");
+            groupService.addMember(context, level1Group, specialGroup);
+            groupService.update(context, level1Group);
+
+            ePerson = createEPerson("isMemberContextGroupSpecial@dspace.org");
+
+            context.setCurrentUser(ePerson);
+            context.setSpecialGroup(specialGroup.getID());
+
+            assertTrue(groupService.isMember(context, topGroup));
+            assertTrue(groupService.isMember(context, level1Group));
+            assertFalse(groupService.isMember(context, level2Group));
+            assertTrue(groupService.isMember(context, specialGroup));
+
+        } finally {
+            if(ePerson != null)
+            {
+                context.turnOffAuthorisationSystem();
+                ePersonService.delete(context, ePerson);
+            }
+            if(specialGroup != null)
+            {
+                context.turnOffAuthorisationSystem();
+                groupService.delete(context, specialGroup);
+            }
+        }
+    }
+
     @Test
     public void isPermanent()
             throws SQLException
@@ -447,10 +483,11 @@ public class GroupTest extends AbstractUnitTest {
     }
 
     @Test
-    public void removeMemberGroup() throws SQLException {
+    public void removeMemberGroup() throws SQLException, AuthorizeException {
-        assertTrue(groupService.isMember(topGroup, level1Group));
+        assertTrue(groupService.isMember(context, topGroup, level1Group));
         groupService.removeMember(context, topGroup, level1Group);
-        assertFalse(groupService.isMember(topGroup, level1Group));
+        groupService.update(context, topGroup);
+        assertFalse(groupService.isMember(context, topGroup, level1Group));
     }
 
     @Test