SEC-589: Fine-tune NotFoundException handling and JavaDocs generally.

acl/src/main/java/org/springframework/security/acls/AclService.java

@@ -34,66 +34,67 @@ public interface AclService {
      *
      * @param parentIdentity to locate children of
      *
-     * @return the children (or <code>null</code> if none were found)
+     * @return the children (or <tt>null</tt> if none were found)
      */
     ObjectIdentity[] findChildren(ObjectIdentity parentIdentity);
 
     /**
      * Same as {@link #readAclsById(ObjectIdentity[])} except it returns only a single Acl.<p>This method
      * should not be called as it does not leverage the underlaying implementation's potential ability to filter
-     * <code>Acl</code> entries based on a {@link Sid} parameter.</p>
+     * <tt>Acl</tt> entries based on a {@link Sid} parameter.</p>
      *
-     * @param object DOCUMENT ME!
+     * @param object to locate an {@link Acl} for
      *
-     * @return DOCUMENT ME!
+     * @return the {@link Acl} for the requested {@link ObjectIdentity} (never <tt>null</tt>)
      *
-     * @throws NotFoundException DOCUMENT ME!
+     * @throws NotFoundException if an {@link Acl} was not found for the requested {@link ObjectIdentity}
      */
     Acl readAclById(ObjectIdentity object) throws NotFoundException;
 
     /**
      * Same as {@link #readAclsById(ObjectIdentity[], Sid[])} except it returns only a single Acl.
      *
-     * @param object DOCUMENT ME!
+     * @param object to locate an {@link Acl} for
-     * @param sids DOCUMENT ME!
+     * @param sids the security identities for which  {@link Acl} information is required 
+     *        (may be <tt>null</tt> to denote all entries)
      *
-     * @return DOCUMENT ME!
+     * @return the {@link Acl} for the requested {@link ObjectIdentity} (never <tt>null</tt>)
      *
-     * @throws NotFoundException DOCUMENT ME!
+     * @throws NotFoundException if an {@link Acl} was not found for the requested {@link ObjectIdentity}
      */
     Acl readAclById(ObjectIdentity object, Sid[] sids)
         throws NotFoundException;
 
     /**
-     * Obtains all the <code>Acl</code>s that apply for the passed <code>Object</code>s.<p>The returned map is
+     * Obtains all the <tt>Acl</tt>s that apply for the passed <tt>Object</tt>s.<p>The returned map is
-     * keyed on the passed objects, with the values being the <code>Acl</code> instances. Any unknown objects will not
+     * keyed on the passed objects, with the values being the <tt>Acl</tt> instances. Any unknown objects will not
      * have a map key.</p>
      *
-     * @param objects the objects to find ACL information for
+     * @param objects the objects to find {@link Acl} information for
      *
-     * @return a map with zero or more elements (never <code>null</code>)
+     * @return a map with exactly one element for each {@link ObjectIdentity} passed as an argument (never <tt>null</tt>)
      *
-     * @throws NotFoundException DOCUMENT ME!
+     * @throws NotFoundException if an {@link Acl} was not found for each requested {@link ObjectIdentity}
      */
     Map readAclsById(ObjectIdentity[] objects) throws NotFoundException;
 
     /**
-     * Obtains all the <code>Acl</code>s that apply for the passed <code>Object</code>s, but only for the
+     * Obtains all the <tt>Acl</tt>s that apply for the passed <tt>Object</tt>s, but only for the
      * security identifies passed.<p>Implementations <em>MAY</em> provide a subset of the ACLs via this method
      * although this is NOT a requirement. This is intended to allow performance optimisations within implementations.
      * Callers should therefore use this method in preference to the alternative overloaded version which does not
      * have performance optimisation opportunities.</p>
-     *  <p>The returned map is keyed on the passed objects, with the values being the <code>Acl</code>
+     *  <p>The returned map is keyed on the passed objects, with the values being the <tt>Acl</tt>
-     * instances. Any unknown objects (or objects for which the interested <code>Sid</code>s do not have entries) will
+     * instances. Any unknown objects (or objects for which the interested <tt>Sid</tt>s do not have entries) will
      * not have a map key.</p>
      *
-     * @param objects the objects to find ACL information for
+     * @param objects the objects to find {@link Acl} information for
-     * @param sids the security identities for which ACL information is required (may be <code>null</code> to denote
+     * @param sids the security identities for which  {@link Acl} information is required 
-     *        all entries)
+     *        (may be <tt>null</tt> to denote all entries)
      *
-     * @return a map with zero or more elements (never <code>null</code>)
+     * @return a map with exactly one element for each {@link ObjectIdentity} passed as an argument (never <tt>null</tt>)
      *
-     * @throws NotFoundException DOCUMENT ME!
+     * @throws NotFoundException if an {@link Acl} was not found for each requested {@link ObjectIdentity}
      */
     Map readAclsById(ObjectIdentity[] objects, Sid[] sids)
         throws NotFoundException;

acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java

@@ -14,6 +14,23 @@
  */
 package org.springframework.security.acls.jdbc;
 
+import java.lang.reflect.Field;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import javax.sql.DataSource;
+
+import org.springframework.dao.DataAccessException;
+import org.springframework.jdbc.core.JdbcTemplate;
+import org.springframework.jdbc.core.PreparedStatementSetter;
+import org.springframework.jdbc.core.ResultSetExtractor;
 import org.springframework.security.acls.AccessControlEntry;
 import org.springframework.security.acls.Acl;
 import org.springframework.security.acls.MutableAcl;
@@ -30,32 +47,9 @@ import org.springframework.security.acls.objectidentity.ObjectIdentityImpl;
 import org.springframework.security.acls.sid.GrantedAuthoritySid;
 import org.springframework.security.acls.sid.PrincipalSid;
 import org.springframework.security.acls.sid.Sid;
-
 import org.springframework.security.util.FieldUtils;
-
-import org.springframework.dao.DataAccessException;
-
-import org.springframework.jdbc.core.JdbcTemplate;
-import org.springframework.jdbc.core.PreparedStatementSetter;
-import org.springframework.jdbc.core.ResultSetExtractor;
-
 import org.springframework.util.Assert;
 
-import java.lang.reflect.Field;
-
-import java.sql.PreparedStatement;
-import java.sql.ResultSet;
-import java.sql.SQLException;
-
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
-import javax.sql.DataSource;
-
 
 /**
  * Performs lookups in a manner that is compatible with ANSI SQL.<p>NB: This implementation does attempt to provide
@@ -346,22 +340,21 @@ public final class BasicLookupStrategy implements LookupStrategy {
     }
 
     /**
-     * The main method.<p>WARNING: This implementation completely disregards the "sids" parameter! Every item
+     * The main method.<p>WARNING: This implementation completely disregards the "sids" argument! Every item
      * in the cache is expected to contain all SIDs. If you have serious performance needs (eg a very large number of
      * SIDs per object identity), you'll probably want to develop a custom {@link LookupStrategy} implementation
      * instead.</p>
      *  <p>The implementation works in batch sizes specfied by {@link #batchSize}.</p>
      *
-     * @param objects DOCUMENT ME!
+     * @param objects the identities to lookup (required)
-     * @param sids DOCUMENT ME!
+     * @param sids the SIDs for which identities are required (ignored by this implementation)
      *
-     * @return DOCUMENT ME!
+     * @return a <tt>Map</tt> where keys represent the {@link ObjectIdentity} of the located {@link Acl} and values
-     *
+     *         are the located {@link Acl} (never <tt>null</tt> although some entries may be missing; this method
-     * @throws NotFoundException DOCUMENT ME!
+     *         should not throw {@link NotFoundException}, as a chain of {@link LookupStrategy}s may be used
-     * @throws IllegalStateException DOCUMENT ME!
+     *         to automatically create entries if required) 
      */
-    public Map readAclsById(ObjectIdentity[] objects, Sid[] sids)
+    public Map readAclsById(ObjectIdentity[] objects, Sid[] sids) {
-        throws NotFoundException {
         Assert.isTrue(batchSize >= 1, "BatchSize must be >= 1");
         Assert.notEmpty(objects, "Objects to lookup required");
 
@@ -416,14 +409,6 @@ public final class BasicLookupStrategy implements LookupStrategy {
             }
         }
 
-        // Now we're done, check every requested object identity was found (throw NotFoundException if needed)
-        for (int i = 0; i < objects.length; i++) {
-            if (!result.containsKey(objects[i])) {
-                throw new NotFoundException("Unable to find ACL information for object identity '"
-                    + objects[i].toString() + "'");
-            }
-        }
-
         return result;
     }
 

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

@@ -28,6 +28,7 @@ import org.springframework.jdbc.core.JdbcTemplate;
 import org.springframework.jdbc.core.RowMapper;
 
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 
 import java.sql.ResultSet;
 import java.sql.SQLException;
@@ -96,10 +97,7 @@ public class JdbcAclService implements AclService {
 
     public Acl readAclById(ObjectIdentity object, Sid[] sids) throws NotFoundException {
         Map map = readAclsById(new ObjectIdentity[] {object}, sids);
-
+        Assert.isTrue(map.containsKey(object), "There should have been an Acl entry for ObjectIdentity " + object);
-        if (map.size() == 0) {
-            throw new NotFoundException("Could not find ACL");
-        }
 
         return (Acl) map.get(object);
     }
@@ -108,11 +106,21 @@ public class JdbcAclService implements AclService {
         return readAclById(object, null);
     }
 
-    public Map readAclsById(ObjectIdentity[] objects) {
+    public Map readAclsById(ObjectIdentity[] objects) throws NotFoundException {
         return readAclsById(objects, null);
     }
 
     public Map readAclsById(ObjectIdentity[] objects, Sid[] sids) throws NotFoundException {
-        return lookupStrategy.readAclsById(objects, sids);
+        Map result = lookupStrategy.readAclsById(objects, sids);
+        
+        // Check every requested object identity was found (throw NotFoundException if needed)
+        for (int i = 0; i < objects.length; i++) {
+            if (!result.containsKey(objects[i])) {
+                throw new NotFoundException("Unable to find ACL information for object identity '"
+                    + objects[i].toString() + "'");
+            }
+        }
+        
+        return result;
     }
 }

acl/src/main/java/org/springframework/security/acls/jdbc/LookupStrategy.java

@@ -14,6 +14,7 @@
  */
 package org.springframework.security.acls.jdbc;
 
+import org.springframework.security.acls.NotFoundException;
 import org.springframework.security.acls.objectidentity.ObjectIdentity;
 import org.springframework.security.acls.sid.Sid;
 
@@ -33,11 +34,13 @@ public interface LookupStrategy {
      * Perform database-specific optimized lookup.
      *
      * @param objects the identities to lookup (required)
-     * @param sids the SIDs for which identities are required (may be <code>null</code> - implementations may elect not
+     * @param sids the SIDs for which identities are required (may be <tt>null</tt> - implementations may elect not
      *        to provide SID optimisations)
      *
-     * @return the <code>Map</code> pursuant to the interface contract for {@link
+     * @return a <tt>Map</tt> where keys represent the {@link ObjectIdentity} of the located {@link Acl} and values
-     *         org.springframework.security.acls.AclService#readAclsById(ObjectIdentity[], Sid[])}
+     *         are the located {@link Acl} (never <tt>null</tt> although some entries may be missing; this method
+     *         should not throw {@link NotFoundException}, as a chain of {@link LookupStrategy}s may be used
+     *         to automatically create entries if required) 
      */
     Map readAclsById(ObjectIdentity[] objects, Sid[] sids);
 }