You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jackrabbit.apache.org by md...@apache.org on 2010/08/25 15:31:25 UTC
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...
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 {