diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java b/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java index 8901e6f261..d37674a794 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java @@ -385,48 +385,53 @@ public final class BasicLookupStrategy implements LookupStrategy { Set currentBatchToLoad = new HashSet(); // contains ObjectIdentitys for (int i = 0; i < objects.length; i++) { - // Check we don't already have this ACL in the results + boolean aclFound = false; + + // Check we don't already have this ACL in the results if (result.containsKey(objects[i])) { - continue; // already in results, so move to next element + aclFound = true; } // Check cache for the present ACL entry - Acl acl = aclCache.getFromCache(objects[i]); - - // Ensure any cached element supports all the requested SIDs - // (they should always, as our base impl doesn't filter on SID) - if (acl != null) { - if (acl.isSidLoaded(sids)) { - result.put(acl.getObjectIdentity(), acl); - - continue; // now in results, so move to next element - } else { - throw new IllegalStateException( - "Error: SID-filtered element detected when implementation does not perform SID filtering " - + "- have you added something to the cache manually?"); + if (!aclFound) { + Acl acl = aclCache.getFromCache(objects[i]); + + // Ensure any cached element supports all the requested SIDs + // (they should always, as our base impl doesn't filter on SID) + if (acl != null) { + if (acl.isSidLoaded(sids)) { + result.put(acl.getObjectIdentity(), acl); + aclFound = true; + } else { + throw new IllegalStateException( + "Error: SID-filtered element detected when implementation does not perform SID filtering " + + "- have you added something to the cache manually?"); + } } } - - // To get this far, we have no choice but to retrieve it via JDBC - // (although we don't do it until we get a batch of them to load) - currentBatchToLoad.add(objects[i]); + + // Load the ACL from the database + if (!aclFound) { + currentBatchToLoad.add(objects[i]); + } // Is it time to load from JDBC the currentBatchToLoad? if ((currentBatchToLoad.size() == this.batchSize) || ((i + 1) == objects.length)) { - Map loadedBatch = lookupObjectIdentities((ObjectIdentity[]) currentBatchToLoad.toArray( - new ObjectIdentity[] {}), sids); + if (currentBatchToLoad.size() > 0) { + Map loadedBatch = lookupObjectIdentities((ObjectIdentity[]) currentBatchToLoad.toArray(new ObjectIdentity[] {}), sids); - // Add loaded batch (all elements 100% initialized) to results - result.putAll(loadedBatch); - - // Add the loaded batch to the cache - Iterator loadedAclIterator = loadedBatch.values().iterator(); - - while (loadedAclIterator.hasNext()) { - aclCache.putInCache((AclImpl) loadedAclIterator.next()); - } - - currentBatchToLoad.clear(); + // Add loaded batch (all elements 100% initialized) to results + result.putAll(loadedBatch); + + // Add the loaded batch to the cache + Iterator loadedAclIterator = loadedBatch.values().iterator(); + + while (loadedAclIterator.hasNext()) { + aclCache.putInCache((AclImpl) loadedAclIterator.next()); + } + + currentBatchToLoad.clear(); + } } } diff --git a/acl/src/test/java/org/springframework/security/acls/jdbc/BasicLookupStrategyTests.java b/acl/src/test/java/org/springframework/security/acls/jdbc/BasicLookupStrategyTests.java index 059b50552b..9d25edfd73 100644 --- a/acl/src/test/java/org/springframework/security/acls/jdbc/BasicLookupStrategyTests.java +++ b/acl/src/test/java/org/springframework/security/acls/jdbc/BasicLookupStrategyTests.java @@ -3,9 +3,9 @@ package org.springframework.security.acls.jdbc; import java.util.Map; import junit.framework.Assert; -import net.sf.ehcache.Ehcache; -import net.sf.ehcache.CacheManager; import net.sf.ehcache.Cache; +import net.sf.ehcache.CacheManager; +import net.sf.ehcache.Ehcache; import org.junit.After; import org.junit.AfterClass; @@ -21,6 +21,8 @@ import org.springframework.security.TestDataSource; import org.springframework.security.acls.Acl; import org.springframework.security.acls.AuditableAccessControlEntry; import org.springframework.security.acls.MutableAcl; +import org.springframework.security.acls.NotFoundException; +import org.springframework.security.acls.Permission; import org.springframework.security.acls.domain.AclAuthorizationStrategy; import org.springframework.security.acls.domain.AclAuthorizationStrategyImpl; import org.springframework.security.acls.domain.BasePermission; @@ -247,7 +249,7 @@ public class BasicLookupStrategyTests { /** * Test created from SEC-590. */ -/* @Test + @Test public void testReadAllObjectIdentitiesWhenLastElementIsAlreadyCached() throws Exception { String query = "INSERT INTO acl_object_identity(ID,OBJECT_ID_CLASS,OBJECT_ID_IDENTITY,PARENT_OBJECT,OWNER_SID,ENTRIES_INHERITING) VALUES (4,2,104,null,1,1);" + "INSERT INTO acl_object_identity(ID,OBJECT_ID_CLASS,OBJECT_ID_IDENTITY,PARENT_OBJECT,OWNER_SID,ENTRIES_INHERITING) VALUES (5,2,105,4,1,1);" @@ -286,7 +288,7 @@ public class BasicLookupStrategyTests { Acl foundParent2Acl = (Acl) foundAcls.get(parent2Oid); Assert.assertNotNull(foundParent2Acl); Assert.assertTrue(foundParent2Acl.isGranted(checkPermission, sids, false)); - }*/ + } @Test public void testAclsWithDifferentSerializableTypesAsObjectIdentities() throws Exception {