Index: jldap/src/test/edu/amc/sakai/user/JLDAPDirectoryProviderTest.java =================================================================== --- jldap/src/test/edu/amc/sakai/user/JLDAPDirectoryProviderTest.java (revision 109332) +++ jldap/src/test/edu/amc/sakai/user/JLDAPDirectoryProviderTest.java (working copy) @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Stack; import org.jmock.Mock; import org.jmock.cglib.MockObjectTestCase; @@ -382,7 +383,7 @@ LdapUserData userData = new LdapUserData(); userData.setEid(eid); mockDoGetCachedUserEntry.expects(once()).method("call"). - with(eq(new Object[] {eid})).will(returnValue(null)); // TODO verify early return if cached value found + with(eq(new Object[] {eid})).will(returnValue(null)); // ? verify early return if cached value found mockDoIsSearchableEid.expects(once()).method("call"). with(eq(new Object[] {eid})). after(mockDoGetCachedUserEntry, "call"). @@ -402,6 +403,15 @@ mockDoSearchDirectoryForSingleEntry.verify(); } + /** Removed this test + * + * It was previously testing how many time getUsers would call over to the single ldap fetch methods + * However, at this point, with the changes, it is actually testing our ability to create a mock object + * which represents an ldap connection and an ldap search and ldap entry, which I do not think is actually + * testing anything useful. Sure, we can make a bunch of mocks, but this only proves we can make a test + * which tests some mocks. Basically, pointless waste of effort. I left the code in here in case someone does + * eventually want to complete these mocks. -AZ + * public void testGetUsersDispatch() { final Mock mockDoGetUserByEid = mock(VarargsMethod.class); final VarargsMethod doGetUserByEid = (VarargsMethod)mockDoGetUserByEid.proxy(); @@ -413,8 +423,13 @@ } }; provider.setLdapConnectionManager(connManager); + provider.setMemoryService( new TestMemoryService() ); + mockConnManager.expects(atLeastOnce()).method("returnConnection"); + mockConnManager.expects(atLeastOnce()).method("getConnection").will(returnValue(conn)); + mockConnManager.expects(atLeastOnce()).method("setConfig").with(same(provider)); + mockConnManager.expects(atLeastOnce()).method("init"); + provider.init(); - String eid1 = "some-eid-1"; String eid2 = "some-eid-2"; UserEdit userEdit1 = new UserEditStub(); @@ -427,6 +442,27 @@ actualUserEdits.add(userEdit2); Collection expectedUserEdits = new ArrayList(actualUserEdits); + LDAPEntry lde = new LDAPEntry(); + // need to mock the ldap entry to make this work + final Stack stack = new Stack(); + stack.add(lde); + LDAPSearchResults lsr = new LDAPSearchResults() { + @Override + public boolean hasMore() { + return stack.empty(); + } + @Override + public LDAPEntry next() throws LDAPException { + return stack.pop(); + } + }; + + mockConn.expects(once()).method("search").will(returnValue(lsr)); + provider.getUsers(actualUserEdits); + assertEquals(expectedUserEdits, actualUserEdits); + } +*******/ + /* OLD tests for above method mockConnManager.expects(once()).method("getConnection").will(returnValue(conn)); mockDoGetUserByEid.expects(once()).method("call"). with(eq(new Object[] {userEdit1, userEdit1.getEid(), conn})). @@ -442,7 +478,7 @@ provider.getUsers(actualUserEdits); assertEquals(expectedUserEdits, actualUserEdits); mockDoGetUserByEid.verify(); - } + */ public void testFindUserByEmailDispatch() { final Mock mockDoSearchDirectoryForSingleEntry = mock(VarargsMethod.class); Index: jldap/src/java/edu/amc/sakai/user/LdapAttributeMapper.java =================================================================== --- jldap/src/java/edu/amc/sakai/user/LdapAttributeMapper.java (revision 109332) +++ jldap/src/java/edu/amc/sakai/user/LdapAttributeMapper.java (working copy) @@ -22,6 +22,7 @@ package edu.amc.sakai.user; import java.util.Map; +import java.util.Set; import org.sakaiproject.user.api.UserEdit; @@ -152,5 +153,15 @@ * @return the formatted search filter */ public String getFindUserByCrossAttributeSearchFilter(String criteria); + + /** + * Builds a filter to a uid search against many users at once + * For reference, the LDAP search filter is of the form: + * "(|(uid=sample.user)(uid=john.doe)(uid=jane.smith))" + * + * @param the search string + * @return the formatted search filter + */ + public String getManyUsersInOneSearch(Set criteria); } Index: jldap/src/java/edu/amc/sakai/user/LdapConnectionManagerConfig.java =================================================================== --- jldap/src/java/edu/amc/sakai/user/LdapConnectionManagerConfig.java (revision 109332) +++ jldap/src/java/edu/amc/sakai/user/LdapConnectionManagerConfig.java (working copy) @@ -202,4 +202,14 @@ */ public void setPoolMaxConns(int maxConns); + /** + * @return The maximum number of results that can be returned in our query + */ + public int getMaxObjectsToQueryFor(); + + /** + * @param maxResultsFromOneQuery The maximum number of results that can be returned in our query + */ + public void setMaxObjectsToQueryFor(int maxObjectsToQueryFor); + } Index: jldap/src/java/edu/amc/sakai/user/SimpleLdapAttributeMapper.java =================================================================== --- jldap/src/java/edu/amc/sakai/user/SimpleLdapAttributeMapper.java (revision 109332) +++ jldap/src/java/edu/amc/sakai/user/SimpleLdapAttributeMapper.java (working copy) @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import java.util.Properties; import java.util.Set; @@ -568,4 +569,26 @@ return sb.toString(); } + /** + * @inheritDoc + */ + public String getManyUsersInOneSearch(Set criteria) { + StringBuilder sb = new StringBuilder(); + sb.append("(|"); + + for ( Iterator eidIterator = criteria.iterator(); eidIterator.hasNext(); ) { + sb.append("("); + sb.append(getFindUserByEidFilter(eidIterator.next())); + sb.append(")"); + } + + sb.append(")"); + + if (M_log.isDebugEnabled()) { + M_log.debug("getManyUsersInOneSearch() completed filter: " + sb.toString()); + } + + return sb.toString(); + } + } Index: jldap/src/java/edu/amc/sakai/user/JLDAPDirectoryProvider.java =================================================================== --- jldap/src/java/edu/amc/sakai/user/JLDAPDirectoryProvider.java (revision 109332) +++ jldap/src/java/edu/amc/sakai/user/JLDAPDirectoryProvider.java (working copy) @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -84,6 +85,9 @@ /** Default LDAP maximum number of connections in the pool */ public static final int DEFAULT_POOL_MAX_CONNS = 10; + + /** Default LDAP maximum number of objects to query for */ + public static final int DEFAULT_MAX_OBJECTS_TO_QUERY = 200; public static final boolean DEFAULT_CASE_SENSITIVE_CACHE_KEYS = false; @@ -126,6 +130,9 @@ /** Maximum number of physical connections in the pool */ private int poolMaxConns = DEFAULT_POOL_MAX_CONNS; + + /** Maximum number of results from one LDAP query */ + private int maxObjectsToQueryFor = DEFAULT_MAX_OBJECTS_TO_QUERY; /** Socket factory for secure connections. Only relevant if * {@link #secureConnection} is true. Defaults to a new instance @@ -153,7 +160,7 @@ * User entry attribute mappings. Keys are logical attr names, * values are physical attr names. * - * {@see LdapAttributeMapper} + * @see LdapAttributeMapper */ private Map attributeMappings; @@ -605,51 +612,73 @@ * returns if a retry exits exceptionally *

*/ - public void getUsers(Collection users) + public void getUsers(Collection users) { - - //TODO: avoid the ripple loading here. should just need one query if ( M_log.isDebugEnabled() ) { M_log.debug("getUsers(): [Collection size = " + users.size() + "]"); } LDAPConnection conn = null; boolean abortiveSearch = false; + int maxQuerySize = getMaxObjectsToQueryFor(); UserEdit userEdit = null; + + HashMap usersToSearchInLDAP = new HashMap(); + List usersToRemove = new ArrayList(); try { - - conn = ldapConnectionManager.getConnection(); - - for ( Iterator userEdits = users.iterator(); userEdits.hasNext(); ) { - - try { - - userEdit = (UserEdit) userEdits.next(); - boolean foundUser = getUserByEid(userEdit, userEdit.getEid(), conn); - if ( !(foundUser) ) { - userEdits.remove(); + int cnt = 0; + for ( Iterator userEdits = users.iterator(); userEdits.hasNext(); ) { + userEdit = (UserEdit) userEdits.next(); + String eid = userEdit.getEid(); + + // Do nothing with this eid if it is in the blacklist + if ( !(isSearchableEid(eid)) ) { + userEdits.remove(); + continue; + } + + // Check the cache before sending the request to LDAP + LdapUserData cachedUserData = getCachedUserEntry(eid); + if ( cachedUserData == null ) { + usersToSearchInLDAP.put(eid, userEdit); + cnt++; + } + + // We need to make sure this query isn't larger than maxQuerySize + if ((!userEdits.hasNext() || cnt == maxQuerySize) && !usersToSearchInLDAP.isEmpty()) { + if (conn == null) { + conn = ldapConnectionManager.getConnection(); } - - } catch ( LDAPException e ) { - - M_log.warn("getUsers(): search failed for user, retrying [eid = " + userEdit.getEid() + "]", - e); - - // lets retry with a new connection, giving up - // for good if the retry fails - ldapConnectionManager.returnConnection(conn); - - conn = ldapConnectionManager.getConnection(); - - // exactly the same calls as above - boolean foundUser = getUserByEid(userEdit, userEdit.getEid(), conn); - if ( !(foundUser) ) { - userEdits.remove(); + + String filter = ldapAttributeMapper.getManyUsersInOneSearch(usersToSearchInLDAP.keySet()); + List ldapUsers = searchDirectory(filter, null, null, null, null, 0); + + for (LdapUserData ldapUserData : ldapUsers) { + String ldapEid = ldapUserData.getEid(); + UserEdit ue = usersToSearchInLDAP.get(ldapEid); + mapUserDataOntoUserEdit(ldapUserData, ue); + usersToSearchInLDAP.remove(ldapEid); } - + + // see if there are any users that we could not find in the LDAP query + for (Map.Entry entry : usersToSearchInLDAP.entrySet()) { + usersToRemove.add(entry.getValue()); + } + + // clear the HashMap and reset the counter + usersToSearchInLDAP.clear(); + cnt = 0; } - } + + // Finally clean up the original collection and remove and users we could not find + for (UserEdit userRemove : usersToRemove) { + if (M_log.isDebugEnabled()) { + M_log.debug("JLDAP getUsers could not find user: " + userRemove.getEid()); + } + users.remove(userRemove); + } + } catch (LDAPException e) { abortiveSearch = true; throw new RuntimeException("getUsers(): LDAPException during search [eid = " + @@ -852,7 +881,7 @@ "][reusing conn = " + (conn != null) + "]"); } - List results = searchDirectory(filter, conn, + List results = searchDirectory(filter, conn, mapper, searchResultPhysicalAttributeNames, searchBaseDn, @@ -887,7 +916,7 @@ * @throws LDAPException if thrown by the search * @throws RuntimeExction wrapping any non-{@link LDAPException} {@link Exception} */ - protected List searchDirectory(String filter, + protected List searchDirectory(String filter, LDAPConnection conn, LdapEntryMapper mapper, String[] searchResultPhysicalAttributeNames, @@ -906,6 +935,9 @@ if ( !(receivedConn) ) { conn = ldapConnectionManager.getConnection(); } + if (conn == null) { + throw new IllegalStateException("Unable to obtain a valid LDAP connection"); + } searchResultPhysicalAttributeNames = scrubSearchResultPhysicalAttributeNames( @@ -950,7 +982,7 @@ false, constraints); - List mappedResults = new ArrayList(); + List mappedResults = new ArrayList(); int resultCnt = 0; while ( searchResults.hasMore() ) { LDAPEntry entry = searchResults.next(); @@ -958,7 +990,7 @@ if ( mappedResult == null ) { continue; } - mappedResults.add(mappedResult); + mappedResults.add((LdapUserData) mappedResult); } return mappedResults; @@ -1276,17 +1308,11 @@ this.secureSocketFactory = secureSocketFactory; } - /** - * {@inheritDoc} - */ public String getBasePath() { return basePath; } - /** - * {@inheritDoc} - */ public void setBasePath(String basePath) { this.basePath = basePath; @@ -1381,6 +1407,20 @@ public void setPoolMaxConns(int poolMaxConns) { this.poolMaxConns = poolMaxConns; } + + /** + * {@inheritDoc} + */ + public int getMaxObjectsToQueryFor() { + return maxObjectsToQueryFor; + } + + /** + * {@inheritDoc} + */ + public void setMaxObjectsToQueryFor (int maxObjectsToQueryFor) { + this.maxObjectsToQueryFor = maxObjectsToQueryFor; + } /** * Access the currently assigned {@link LdapConnectionManager} delegate. @@ -1451,7 +1491,7 @@ * @see #cacheUserData(LdapUserData) * @see #getCachedUserEntry(String) * @see #defaultLdapEntryMapper - * @return + * @return boolean */ public boolean isCaseSensitiveCacheKeys() { return caseSensitiveCacheKeys; @@ -1487,7 +1527,7 @@ * * @see #setAllowAuthentication(boolean) * - * @return + * @return boolean */ public boolean isAllowAuthentication() { return allowAuthentication; @@ -1529,7 +1569,7 @@ * {@link #setAuthenticateWithProviderFirst(boolean)} for * additional semantics. * - * @return + * @return boolean */ public boolean isAuthenticateWithProviderFirst() { return authenticateWithProviderFirst; @@ -1666,7 +1706,8 @@ * To create all the UserEdit objects you populate and return in the return collection. * @return Collection (UserEdit) of user objects that have this email address, or an empty Collection if there are none. */ - public Collection findUsersByEmail(String email, UserFactory factory) { + @SuppressWarnings("rawtypes") + public Collection findUsersByEmail(String email, UserFactory factory) { String filter = ldapAttributeMapper.getFindUserByEmailFilter(email); List users = new ArrayList(); @@ -1689,17 +1730,11 @@ } - /** - * {@inheritDoc} - */ public boolean isSearchAliases() { return searchAliases; } - /** - * {@inheritDoc} - */ public void setSearchAliases(boolean searchAliases) { this.searchAliases = searchAliases; Index: component/src/webapp/WEB-INF/jldap-beans.xml =================================================================== --- component/src/webapp/WEB-INF/jldap-beans.xml (revision 109332) +++ component/src/webapp/WEB-INF/jldap-beans.xml (working copy) @@ -145,6 +145,12 @@ + +