Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java (revision 1538062) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/GroupImpl.java (working copy) @@ -417,6 +417,9 @@ } values[values.length - 1] = toAdd; + // we need to inform the user manager before the property is set in case the manager is 'autosave'. + // otherwise the cache would miss the modified membership + userManager.onMemberAdded(GroupImpl.this, authorizable); userManager.setProtectedProperty(node, P_MEMBERS, values, PropertyType.WEAKREFERENCE); return true; } @@ -437,6 +440,9 @@ if (valList.remove(toRemove)) { try { + // we need to inform the user manager before the property is set in case the manager is 'autosave'. + // otherwise the cache would miss the modified membership + userManager.onMemberRemoved(GroupImpl.this, authorizable); if (valList.isEmpty()) { userManager.removeProtectedItem(property, node); } else { @@ -523,7 +529,9 @@ Value newMember = getSession().getValueFactory().createValue(authorizable.getNode(), true); properties.addProperty(propName, newMember); } - + // we need to inform the user manager before the property is set otherwise the cache would + // miss the modified membership + userManager.onMemberAdded(GroupImpl.this, authorizable); if (userManager.isAutoSave()) { node.save(); } @@ -566,6 +574,9 @@ return false; } + // we need to inform the user manager before the property is set otherwise the cache would + // miss the modified membership + userManager.onMemberRemoved(GroupImpl.this, authorizable); if (userManager.isAutoSave()) { node.save(); } Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/MembershipCache.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/MembershipCache.java (revision 1538062) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/MembershipCache.java (working copy) @@ -25,7 +25,6 @@ import javax.jcr.ItemNotFoundException; import javax.jcr.Node; import javax.jcr.NodeIterator; -import javax.jcr.Property; import javax.jcr.PropertyIterator; import javax.jcr.PropertyType; import javax.jcr.RepositoryException; @@ -39,10 +38,7 @@ import org.apache.jackrabbit.core.SessionImpl; import org.apache.jackrabbit.core.SessionListener; import org.apache.jackrabbit.core.cache.ConcurrentCache; -import org.apache.jackrabbit.core.nodetype.NodeTypeImpl; import org.apache.jackrabbit.core.observation.SynchronousEventListener; -import org.apache.jackrabbit.spi.Name; -import org.apache.jackrabbit.util.Text; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -65,15 +61,16 @@ private final SessionImpl systemSession; private final String groupsPath; private final boolean useMembersNode; - private final String pMembers; + private final ConcurrentCache> cache; + private final Set pending = new HashSet(); + MembershipCache(SessionImpl systemSession, String groupsPath, boolean useMembersNode) throws RepositoryException { this.systemSession = systemSession; this.groupsPath = (groupsPath == null) ? UserConstants.GROUPS_PATH : groupsPath; this.useMembersNode = useMembersNode; - pMembers = systemSession.getJCRName(UserManagerImpl.P_MEMBERS); cache = new ConcurrentCache>("MembershipCache", 16); cache.setMaxMemorySize(MAX_CACHE_SIZE); @@ -101,46 +98,17 @@ * @see javax.jcr.observation.EventListener#onEvent(javax.jcr.observation.EventIterator) */ public void onEvent(EventIterator eventIterator) { - // evaluate if the membership cache needs to be cleared; - boolean clear = false; - while (eventIterator.hasNext() && !clear) { - Event ev = eventIterator.nextEvent(); - try { - if (pMembers.equals(Text.getName(ev.getPath()))) { - // simple case: a rep:members property that is affected - clear = true; - } else if (useMembersNode) { - // test if it affects a property defined by rep:Members node type. - int type = ev.getType(); - if (type == Event.PROPERTY_ADDED || type == Event.PROPERTY_CHANGED) { - Property p = systemSession.getProperty(ev.getPath()); - Name declNtName = ((NodeTypeImpl) p.getDefinition().getDeclaringNodeType()).getQName(); - clear = NT_REP_MEMBERS.equals(declNtName); - } else { - // PROPERTY_REMOVED - // test if the primary node type of the parent node is rep:Members - // this could potentially by some other property as well as the - // rep:Members node are not protected and could changed by - // adding a mixin type. - // ignoring this and simply clear the cache - String parentId = ev.getIdentifier(); - Node n = systemSession.getNodeByIdentifier(parentId); - Name ntName = ((NodeTypeImpl) n.getPrimaryNodeType()).getQName(); - clear = (UserConstants.NT_REP_MEMBERS.equals(ntName)); - } - } - } catch (RepositoryException e) { - log.warn(e.getMessage()); - // exception while processing the event -> clear the cache to - // be sure it isn't outdated. - clear = true; + // if a modification on a member node occurrs and we recorded pending memberships, then + // we need to flush those from the cache. + synchronized (pending) { + for (String id: pending) { + cache.remove(id); } + if (log.isDebugEnabled()) { + log.debug("Membership cache invalidated {} entries.", pending.size()); + } + pending.clear(); } - - if (clear) { - cache.clear(); - log.debug("Membership cache cleared because of observation event."); - } } //----------------------------------------------------< SessionListener >--- @@ -204,6 +172,16 @@ } /** + * Marks the authorizable with the given id as pending. pending authorizables will be flushed in the next save call. + * @param authorizableNodeIdentifier the id of the authorizable + */ + void markPending(String authorizableNodeIdentifier) { + synchronized (pending) { + pending.add(authorizableNodeIdentifier); + } + } + + /** * Collects the declared memberships for the specified identifier of an * authorizable using the specified session. * @@ -558,4 +536,5 @@ } return refs; } + } \ No newline at end of file Index: jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserManagerImpl.java =================================================================== --- jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserManagerImpl.java (revision 1538062) +++ jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/user/UserManagerImpl.java (working copy) @@ -1164,6 +1164,34 @@ return false; } + /** + * Is called by the {@link org.apache.jackrabbit.core.security.user.GroupImpl.MembershipProvider} when a member + * was removed from the group + * @param group the group that was modified + * @param authorizable the member that was removed + */ + public void onMemberRemoved(GroupImpl group, AuthorizableImpl authorizable) { + try { + membershipCache.markPending(authorizable.getNode().getIdentifier()); + } catch (RepositoryException e) { + log.warn("Error while retrieving the node identifier of " + authorizable, e); + } + } + + /** + * Is called by the {@link org.apache.jackrabbit.core.security.user.GroupImpl.MembershipProvider} when a new member + * is added to a group. + * @param group the group that was modified + * @param authorizable the member that was added + */ + public void onMemberAdded(GroupImpl group, AuthorizableImpl authorizable) { + try { + membershipCache.markPending(authorizable.getNode().getIdentifier()); + } catch (RepositoryException e) { + log.warn("Error while retrieving the node identifier of " + authorizable, e); + } + } + //------------------------------------------------------< inner classes >--- /** * Inner class Index: jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/MembershipAutoSaveTest.java =================================================================== --- jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/MembershipAutoSaveTest.java (revision 0) +++ jackrabbit-core/src/test/java/org/apache/jackrabbit/core/security/user/MembershipAutoSaveTest.java (working copy) @@ -0,0 +1,326 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.jackrabbit.core.security.user; + +import java.io.File; +import java.security.Principal; +import java.util.UUID; + +import javax.jcr.RepositoryException; +import javax.jcr.Session; +import javax.jcr.SimpleCredentials; + +import org.apache.commons.io.FileUtils; +import org.apache.jackrabbit.api.JackrabbitSession; +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.core.RepositoryImpl; +import org.apache.jackrabbit.core.config.RepositoryConfig; +import org.apache.jackrabbit.core.security.TestPrincipal; +import org.apache.jackrabbit.test.JUnitTest; +import org.apache.jackrabbit.test.NotExecutableException; + +/** + * Performance test for JCR-3658. + */ +public class MembershipAutoSaveTest extends JUnitTest { + + private static final String REPO_HOME = new File("target", MembershipAutoSaveTest.class.getSimpleName()).getPath(); + + private RepositoryImpl repo; + private JackrabbitSession session; + private UserManager userMgr; + + @Override + protected void setUp() throws Exception { + super.setUp(); + FileUtils.deleteDirectory(new File(REPO_HOME)); + RepositoryConfig config = RepositoryConfig.create( + getClass().getResourceAsStream("repository.xml"), REPO_HOME); + repo = RepositoryImpl.create(config); + session = createSession(); + userMgr = session.getUserManager(); + } + + @Override + protected void tearDown() throws Exception { + session.logout(); + repo.shutdown(); + repo = null; + FileUtils.deleteDirectory(new File(REPO_HOME)); + super.tearDown(); + } + + private Principal getTestPrincipal() throws RepositoryException { + String pn = "any_principal" + UUID.randomUUID(); + return new TestPrincipal(pn); + } + + private JackrabbitSession createSession() throws RepositoryException { + return (JackrabbitSession) repo.login( + new SimpleCredentials("admin", "admin".toCharArray())); + } + + public void testMembershipAutoSave() throws RepositoryException, NotExecutableException { + userMgr.autoSave(true); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + grp.addMember(user); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + + grp.removeMember(user); + assertFalse(testGroup.getDeclaredMembers().hasNext()); + + } finally { + if (user != null) { + user.remove(); + } + if (grp != null) { + grp.remove(); + } + session.save(); + testSession.logout(); + } + } + + public void testMembershipDeleteAutoSave() throws RepositoryException, NotExecutableException { + userMgr.autoSave(true); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + grp.addMember(user); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + User testUser = (User) testMgr.getAuthorizable(p); + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + assertTrue(testUser.declaredMemberOf().hasNext()); + + grp.remove(); + grp = null; + assertFalse(testUser.declaredMemberOf().hasNext()); + + } finally { + if (user != null) { + user.remove(); + } + if (grp != null) { + grp.remove(); + } + session.save(); + testSession.logout(); + } + } + + public void testMembershipNoAutoSave() throws RepositoryException, NotExecutableException { + userMgr.autoSave(false); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + session.save(); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + + grp.addMember(user); + assertFalse(testGroup.getDeclaredMembers().hasNext()); + session.save(); + + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + + grp.removeMember(user); + assertTrue(testGroup.getDeclaredMembers().hasNext()); + + session.save(); + assertFalse(testGroup.getDeclaredMembers().hasNext()); + + } finally { + if (user != null) { + user.remove(); + } + if (grp != null) { + grp.remove(); + } + session.save(); + testSession.logout(); + } + } + + public void testMembershipDeleteNoAutoSave() throws RepositoryException, NotExecutableException { + userMgr.autoSave(false); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + grp.addMember(user); + session.save(); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + User testUser = (User) testMgr.getAuthorizable(p); + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + assertTrue(testUser.declaredMemberOf().hasNext()); + + grp.remove(); + grp = null; + assertTrue(testUser.declaredMemberOf().hasNext()); + session.save(); + assertFalse(testUser.declaredMemberOf().hasNext()); + + + } finally { + if (user != null) { + user.remove(); + } + if (grp != null) { + grp.remove(); + } + session.save(); + testSession.logout(); + } + } + + public void testMembershipNoAutoSaveRevert() throws RepositoryException, NotExecutableException { + userMgr.autoSave(false); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + session.save(); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + + grp.addMember(user); + assertTrue(grp.getDeclaredMembers().hasNext()); + assertFalse(testGroup.getDeclaredMembers().hasNext()); + + session.refresh(false); + assertFalse(grp.getDeclaredMembers().hasNext()); + assertFalse(testGroup.getDeclaredMembers().hasNext()); + + grp.addMember(user); + session.save(); + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + + grp.removeMember(user); + assertFalse(grp.getDeclaredMembers().hasNext()); + assertTrue(testGroup.getDeclaredMembers().hasNext()); + session.refresh(false); + assertTrue(grp.getDeclaredMembers().hasNext()); + assertTrue(testGroup.getDeclaredMembers().hasNext()); + + } finally { + if (grp != null) { + grp.remove(); + } + if (user != null) { + user.remove(); + } + session.save(); + testSession.logout(); + } + } + + public void testMembershipDeleteNoAutoSaveRevert() throws RepositoryException, NotExecutableException { + userMgr.autoSave(false); + + JackrabbitSession testSession = createSession(); + UserManager testMgr = testSession.getUserManager(); + + Principal p = getTestPrincipal(); + Principal g = getTestPrincipal(); + + User user = null; + Group grp = null; + try { + user = userMgr.createUser(p.getName(), "foo"); + grp = userMgr.createGroup(g); + grp.addMember(user); + session.save(); + + Group testGroup = (Group) testMgr.getAuthorizable(g); + User testUser = (User) testMgr.getAuthorizable(p); + assertEquals(p.getName(), testGroup.getDeclaredMembers().next().getID()); + assertTrue(testUser.declaredMemberOf().hasNext()); + + grp.remove(); + grp = null; + assertFalse(user.declaredMemberOf().hasNext()); + assertTrue(testUser.declaredMemberOf().hasNext()); + session.refresh(false); + + assertTrue(testUser.declaredMemberOf().hasNext()); + assertTrue(user.declaredMemberOf().hasNext()); + + + } finally { + if (user != null) { + user.remove(); + } + if (grp != null) { + grp.remove(); + } + session.save(); + testSession.logout(); + } + } + +}