jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mdue...@apache.org
Subject svn commit: r989102 - in /jackrabbit/trunk: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/ jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/ jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/common...
Date Wed, 25 Aug 2010 13:31:25 GMT
Author: mduerig
Date: Wed Aug 25 13:31:24 2010
New Revision: 989102

URL: http://svn.apache.org/viewvc?rev=989102&view=rev
Log:
JCR-2726: Make collecting group membership information lazy

Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
    jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/ItemSequence.java
    jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/TreeTraverser.java
    jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/iterator/LazyIteratorChain.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java?rev=989102&r1=989101&r2=989102&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java
Wed Aug 25 13:31:24 2010
@@ -18,17 +18,18 @@ package org.apache.jackrabbit.core.secur
 
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
-import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.api.security.user.UserManager;
 import org.apache.jackrabbit.commons.flat.BTreeManager;
 import org.apache.jackrabbit.commons.flat.ItemSequence;
 import org.apache.jackrabbit.commons.flat.PropertySequence;
 import org.apache.jackrabbit.commons.flat.Rank;
 import org.apache.jackrabbit.commons.flat.TreeManager;
+import org.apache.jackrabbit.commons.iterator.LazyIteratorChain;
 import org.apache.jackrabbit.core.NodeImpl;
 import org.apache.jackrabbit.core.PropertyImpl;
 import org.apache.jackrabbit.core.session.SessionContext;
 import org.apache.jackrabbit.core.session.SessionOperation;
+import org.apache.jackrabbit.spi.commons.iterator.Iterators;
 import org.apache.jackrabbit.util.Text;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -52,6 +53,7 @@ import java.util.Enumeration;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 /**
@@ -92,14 +94,14 @@ class GroupImpl extends AuthorizableImpl
      * @see Group#getDeclaredMembers()
      */
     public Iterator<Authorizable> getDeclaredMembers() throws RepositoryException {
-        return getMembers(false, UserManager.SEARCH_TYPE_AUTHORIZABLE).iterator();
+        return getMembers(false, UserManager.SEARCH_TYPE_AUTHORIZABLE);
     }
 
     /**
      * @see Group#getMembers()
      */
     public Iterator<Authorizable> getMembers() throws RepositoryException {
-        return getMembers(true, UserManager.SEARCH_TYPE_AUTHORIZABLE).iterator();
+        return getMembers(true, UserManager.SEARCH_TYPE_AUTHORIZABLE);
     }
 
     /**
@@ -189,7 +191,7 @@ class GroupImpl extends AuthorizableImpl
      * @return A collection of members of this group.
      * @throws RepositoryException If an error occurs while collecting the members.
      */
-    private Collection<Authorizable> getMembers(boolean includeIndirect, int type)
throws RepositoryException {
+    private Iterator<Authorizable> getMembers(boolean includeIndirect, int type) throws
RepositoryException {
         return getMembershipProvider(getNode()).getMembers(includeIndirect, type);
     }
 
@@ -205,7 +207,8 @@ class GroupImpl extends AuthorizableImpl
     private boolean isCyclicMembership(AuthorizableImpl newMember) throws RepositoryException
{
         if (newMember.isGroup()) {
             GroupImpl gr = (GroupImpl) newMember;
-            for (Authorizable member : gr.getMembers(true, UserManager.SEARCH_TYPE_GROUP))
{
+            for (Iterator<Authorizable> it = gr.getMembers(true, UserManager.SEARCH_TYPE_GROUP);
it.hasNext(); ) {
+                Authorizable member = it.next();
                 GroupImpl grMemberImpl = (GroupImpl) member;
                 if (getNode().getUUID().equals(grMemberImpl.getNode().getUUID())) {
                     // found cyclic group membership
@@ -228,38 +231,6 @@ class GroupImpl extends AuthorizableImpl
         return ItemSequence.createPropertySequence(treeManager);
     }
 
-    private void collectMembers(Value memberRef, Collection<Authorizable> members,
boolean includeIndirect,
-                                int type) throws RepositoryException {
-
-        try {
-            NodeImpl member = (NodeImpl) getSession().getNodeByIdentifier(memberRef.getString());
-            if (member.isNodeType(NT_REP_GROUP)) {
-                if (type != UserManager.SEARCH_TYPE_USER) {
-                    Group group = userManager.createGroup(member);
-                    // only retrieve indirect group-members if the group is not
-                    // yet present (detected eventual circular membership).
-                    if (members.add(group) && includeIndirect) {
-                        members.addAll(((GroupImpl) group).getMembers(true, type));
-                    }
-                } // else: groups are ignored
-            } else if (member.isNodeType(NT_REP_USER)) {
-                if (type != UserManager.SEARCH_TYPE_GROUP) {
-                    User user = userManager.createUser(member);
-                    members.add(user);
-                }
-            } else {
-                // reference does point to an authorizable node -> not a
-                // member of this group -> ignore
-                log.debug("Group member entry with invalid node type {} -> " +
-                        "Not included in member set.", member.getPrimaryNodeType().getName());
-            }
-        } catch (ItemNotFoundException e) {
-            // dangling weak reference -> clean upon next write.
-            log.debug("Authorizable node referenced by {} doesn't exist any more -> "
+
-                    "Ignored from member list.", getID());
-        }
-    }
-
     //------------------------------------------------------< inner classes >---
 
     private class NodeBasedGroup extends NodeBasedPrincipal implements java.security.acl.Group
{
@@ -357,10 +328,8 @@ class GroupImpl extends AuthorizableImpl
 
     private interface MembershipProvider {
         boolean addMember(AuthorizableImpl authorizable) throws RepositoryException;
-
         boolean removeMember(AuthorizableImpl authorizable) throws RepositoryException;
-
-        Collection<Authorizable> getMembers(boolean includeIndirect, int type) throws
RepositoryException;
+        Iterator<Authorizable> getMembers(boolean includeIndirect, int type) throws
RepositoryException;
     }
 
     private class PropertyBasedMembershipProvider implements MembershipProvider {
@@ -428,14 +397,20 @@ class GroupImpl extends AuthorizableImpl
             }
         }
 
-        public Collection<Authorizable> getMembers(boolean includeIndirect, int type)
throws RepositoryException {
-            Collection<Authorizable> members = new HashSet<Authorizable>();
+        public Iterator<Authorizable> getMembers(boolean includeIndirect, int type)
throws RepositoryException {
             if (node.hasProperty(P_MEMBERS)) {
-                for (Value member : node.getProperty(P_MEMBERS).getValues()) {
-                    collectMembers(member, members, includeIndirect, type);
+                Value[] members = node.getProperty(P_MEMBERS).getValues();
+
+                if (includeIndirect) {
+                    return includeIndirect(toAuthorizables(members, type));
+                }
+                else {
+                    return toAuthorizables(members, type);
                 }
             }
-            return members;
+            else {
+                return Iterators.empty();
+            }
         }
 
     }
@@ -520,18 +495,171 @@ class GroupImpl extends AuthorizableImpl
             });
         }
 
-        public Collection<Authorizable> getMembers(boolean includeIndirect, int type)
-                throws RepositoryException {
-
-            Collection<Authorizable> members = new HashSet<Authorizable>();
+        public Iterator<Authorizable> getMembers(boolean includeIndirect, int type)
throws RepositoryException {
             if (node.hasNode(N_MEMBERS)) {
-                for (Property member : getPropertySequence(node.getNode(N_MEMBERS), userManager))
{
-                    collectMembers(member.getValue(), members, includeIndirect, type);
+                PropertySequence members = getPropertySequence(node.getNode(N_MEMBERS), userManager);
+                if (includeIndirect) {
+                    return includeIndirect(toAuthorizables(members.iterator(), type));
                 }
+                else {
+                    return toAuthorizables(members.iterator(), type);
+                }
+            }
+            else {
+                return Iterators.empty();
             }
+        }
 
-            return members;
+    }
+
+    // -----------------------------------------------------< utility >---
+
+    /**
+     * Returns an iterator of authorizables which includes all indirect members of the given
iterator
+     * of authorizables.
+     */
+    private Iterator<Authorizable> includeIndirect(final Iterator<Authorizable>
authorizables) {
+        Iterator<Iterator<Authorizable>> indirectMembers = new Iterator<Iterator<Authorizable>>()
{
+            public boolean hasNext() {
+                return authorizables.hasNext();
+            }
+
+            public Iterator<Authorizable> next() {
+                Authorizable next = authorizables.next();
+                return Iterators.iteratorChain(Iterators.singleton(next), indirect(next));
+            }
+
+            public void remove() {
+                throw new UnsupportedOperationException();
+            }
+
+            /**
+             * Returns the transitive closure over the members of this authorizable
+             */
+            private Iterator<Authorizable> indirect(Authorizable authorizable) {
+                if (authorizable.isGroup()) {
+                    try {
+                        return ((Group) authorizable).getMembers();
+                    }
+                    catch (RepositoryException e) {
+                        log.warn("Could not determine members of " + authorizable, e);
+                    }
+                }
+
+                return Iterators.empty();
+            }
+        };
+
+        return new LazyIteratorChain<Authorizable>(indirectMembers);
+    }
+
+    /**
+     * Map an array of values to an iterator of authorizables.
+     */
+    private Iterator<Authorizable> toAuthorizables(final Value[] members, int type)
{
+        return new AuthorizableIterator(type) {
+            private int pos;
+
+            @Override
+            protected String getNextMemberRef() throws RepositoryException {
+                return pos < members.length
+                    ? members[pos++].getString()
+                    : null;
+            }
+        };
+    }
+
+    /**
+     * Map an iterator of properties to an iterator of authorizables.
+     */
+    private Iterator<Authorizable> toAuthorizables(final Iterator<Property> members,
int type) {
+        return new AuthorizableIterator(type) {
+            @Override
+            protected String getNextMemberRef() throws RepositoryException {
+                return members.hasNext()
+                    ? members.next().getString()
+                    : null;
+            }
+        };
+    }
+
+    /**
+     * Iterator of authorizables of a specific type.
+     */
+    private abstract class AuthorizableIterator implements Iterator<Authorizable> {
+        private Authorizable next;
+        private final int type;
+
+        public AuthorizableIterator(int type) {
+            super();
+            this.type = type;
+        }
+
+        public boolean hasNext() {
+            prefetch();
+            return next != null;
+        }
+
+        public Authorizable next() {
+            prefetch();
+            if (next == null) {
+                throw new NoSuchElementException();
+            }
+
+            Authorizable element = next;
+            next = null;
+            return element;
         }
 
+        public void remove() {
+            throw new UnsupportedOperationException();
+        }
+
+        /**
+         * Returns the reference value of the next node representing the next authorizable
or
+         * <code>null</code> if there there are no more.
+         */
+        protected abstract String getNextMemberRef() throws RepositoryException;
+
+        private void prefetch() {
+            while (next == null) {
+                try {
+                    String memberRef = getNextMemberRef();
+                    if (memberRef == null) {
+                        return;
+                    }
+
+                    NodeImpl member = (NodeImpl) getSession().getNodeByIdentifier(memberRef);
+                    if (type != UserManager.SEARCH_TYPE_USER && member.isNodeType(NT_REP_GROUP))
{
+                        next = userManager.createGroup(member);
+                    }
+                    else if (type != UserManager.SEARCH_TYPE_GROUP && member.isNodeType(NT_REP_USER))
{
+                        next = userManager.createUser(member);
+                    }
+                    else {
+                        log.debug("Group member entry with invalid node type {} -> " +
+                                "Not included in member set.", member.getPrimaryNodeType().getName());
+                    }
+                }
+                catch (ItemNotFoundException e) {
+                    log.debug("Authorizable node referenced by {} doesn't exist any more
-> " +
+                            "Ignored from member list.", safeGetID());
+                }
+                catch (RepositoryException e) {
+                    log.debug("Error pre-fetching member for " + safeGetID(), e);
+                }
+
+            }
+        }
+    }
+
+    private String safeGetID() {
+        try {
+            return getID();
+        }
+        catch (RepositoryException e) {
+            return getNode().toString();
+        }
     }
+
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java?rev=989102&r1=989101&r2=989102&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/api/security/user/GroupTest.java
Wed Aug 25 13:31:24 2010
@@ -21,8 +21,10 @@ import org.apache.jackrabbit.test.NotExe
 import javax.jcr.RepositoryException;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Set;
 
 /**
  * <code>GroupTest</code>...
@@ -364,6 +366,45 @@ public class GroupTest extends AbstractU
         }
     }
 
+    public void testDeeplyNestedGroups() throws NotExecutableException, RepositoryException
{
+        Set<Group> groups = new HashSet<Group>();
+        try {
+            User auth = getTestUser(superuser);
+            Group topGroup = userMgr.createGroup(getTestPrincipal());
+
+            // Create chain of nested groups with auth member of bottom group
+            Group bottomGroup = topGroup;
+            for (int k = 0; k < 100; k++) {
+                Group g = userMgr.createGroup(getTestPrincipal());
+                groups.add(g);
+                bottomGroup.addMember(g);
+                bottomGroup = g;
+            }
+            bottomGroup.addMember(auth);
+
+            // Check that every groups has exactly one member
+            for (Group g : groups) {
+                Iterator<Authorizable> declaredMembers = g.getDeclaredMembers();
+                assertTrue(declaredMembers.hasNext());
+                declaredMembers.next();
+                assertFalse(declaredMembers.hasNext());
+            }
+
+            // Check that we get all members from the getMembers call
+            HashSet<Group> allGroups = new HashSet<Group>(groups);
+            for (Iterator<Authorizable> it = topGroup.getMembers(); it.hasNext(); )
{
+                Authorizable a = it.next();
+                assertTrue(a.equals(auth) || allGroups.remove(a));
+            }
+            assertTrue(allGroups.isEmpty());
+        }
+        finally {
+            for (Group g : groups) {
+                g.remove();
+            }
+        }
+    }
+
     public void testRemoveMemberTwice() throws NotExecutableException, RepositoryException
{
         User auth = getTestUser(superuser);
         Group newGroup = null;

Modified: jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/ItemSequence.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/ItemSequence.java?rev=989102&r1=989101&r2=989102&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/ItemSequence.java
(original)
+++ jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/ItemSequence.java
Wed Aug 25 13:31:24 2010
@@ -16,9 +16,8 @@
  */
 package org.apache.jackrabbit.commons.flat;
 
-import java.util.Comparator;
-import java.util.Iterator;
-import java.util.Set;
+import org.apache.jackrabbit.commons.flat.TreeTraverser.ErrorHandler;
+import org.apache.jackrabbit.commons.flat.TreeTraverser.InclusionPolicy;
 
 import javax.jcr.ItemExistsException;
 import javax.jcr.Node;
@@ -29,8 +28,9 @@ import javax.jcr.RepositoryException;
 import javax.jcr.Session;
 import javax.jcr.Value;
 
-import org.apache.jackrabbit.commons.flat.TreeTraverser.ErrorHandler;
-import org.apache.jackrabbit.commons.flat.TreeTraverser.InclusionPolicy;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.Set;
 
 /**
  * <p>
@@ -361,8 +361,13 @@ public abstract class ItemSequence {
 
     protected static class NodeSequenceImpl extends ItemSequence implements NodeSequence
{
         private final InclusionPolicy<Node> inclusionPolicy = new InclusionPolicy<Node>()
{
-            public boolean include(Node node) throws RepositoryException {
-                return treeManager.isLeaf(node);
+            public boolean include(Node node) {
+                try {
+                    return treeManager.isLeaf(node);
+                }
+                catch (RepositoryException e) {
+                    return false;
+                }
             }
         };
 
@@ -446,8 +451,13 @@ public abstract class ItemSequence {
     protected static class PropertySequenceImpl extends ItemSequence implements PropertySequence
{
         private final InclusionPolicy<Property> inclusionPolicy = new InclusionPolicy<Property>()
{
             private final Set<String> ignoredProperties = treeManager.getIgnoredProperties();
-            public boolean include(Property property) throws RepositoryException {
-                return !ignoredProperties.contains(property.getName());
+            public boolean include(Property property) {
+                try {
+                    return !ignoredProperties.contains(property.getName());
+                }
+                catch (RepositoryException e) {
+                    return false;
+                }
             }
         };
 

Modified: jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/TreeTraverser.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/TreeTraverser.java?rev=989102&r1=989101&r2=989102&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/TreeTraverser.java
(original)
+++ jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/flat/TreeTraverser.java
Wed Aug 25 13:31:24 2010
@@ -129,8 +129,13 @@ public final class TreeTraverser impleme
          * node is a node which does not have child nodes.
          */
         public static InclusionPolicy<Node> LEAVES = new InclusionPolicy<Node>()
{
-            public boolean include(Node node) throws RepositoryException {
-                return !node.hasNodes();
+            public boolean include(Node node) {
+                try {
+                    return !node.hasNodes();
+                }
+                catch (RepositoryException e) {
+                    return false;
+                }
             }
         };
 
@@ -140,10 +145,8 @@ public final class TreeTraverser impleme
          * @param item The item under consideration
          * @return <code>true</code> when <code>item</code> should
be included.
          *         <code>false</code> otherwise.
-         *
-         * @throws RepositoryException
          */
-        boolean include(T item) throws RepositoryException;
+        boolean include(T item);
     }
 
     /**
@@ -254,16 +257,11 @@ public final class TreeTraverser impleme
      */
     @SuppressWarnings("unchecked")
     private Iterator<Node> iterator(Node node) {
-        try {
-            if (inclusionPolicy.include(node)) {
-                return chain(singleton(node), chain(childIterators(node)));
-            }
-            else {
-                return chain(childIterators(node));
-            }
-        } catch (RepositoryException e) {
-            errorHandler.call(node, e);
-            return empty();
+        if (inclusionPolicy.include(node)) {
+            return chain(singleton(node), chain(childIterators(node)));
+        }
+        else {
+            return chain(childIterators(node));
         }
     }
 
@@ -341,12 +339,7 @@ public final class TreeTraverser impleme
         return new FilterIterator<T>(iterator, new Predicate() {
             @SuppressWarnings("unchecked")
             public boolean evaluate(Object object) {
-                try {
-                    return inclusionPolicy.include((T) object);
-                }
-                catch (RepositoryException ignore) {
-                    return true;
-                }
+                return inclusionPolicy.include((T) object);
             }
         });
     }

Modified: jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/iterator/LazyIteratorChain.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/iterator/LazyIteratorChain.java?rev=989102&r1=989101&r2=989102&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/iterator/LazyIteratorChain.java
(original)
+++ jackrabbit/trunk/jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/iterator/LazyIteratorChain.java
Wed Aug 25 13:31:24 2010
@@ -31,6 +31,7 @@ import java.util.NoSuchElementException;
 public class LazyIteratorChain<T> implements Iterator<T> {
     private final Iterator<Iterator<T>> iterators;
     private Iterator<T> currentIterator;
+    private Boolean hasNext;
 
     /**
      * Returns the concatenation of all iterators in <code>iterators</code>.
@@ -65,14 +66,20 @@ public class LazyIteratorChain<T> implem
     }
 
     public boolean hasNext() {
-        while ((currentIterator == null || !currentIterator.hasNext()) && iterators.hasNext())
{
-            currentIterator = iterators.next();
+        // Memoizing the result of hasNext is crucial to performance when recursively
+        // traversing tree structures.
+        if (hasNext == null) {
+            while ((currentIterator == null || !currentIterator.hasNext()) && iterators.hasNext())
{
+                currentIterator = iterators.next();
+            }
+            hasNext = currentIterator != null && currentIterator.hasNext();
         }
-        return currentIterator != null && currentIterator.hasNext();
+        return hasNext;
     }
 
     public T next() {
         if (hasNext()) {
+            hasNext = null;
             return currentIterator.next();
         }
         else {



Mime
View raw message