Merge pull request #553 from ministryofjustice/bugfix/email-timeout

DST-9458 Fix timeouts due to unnecessary `expandEmailInSearch` calls

src/main/java/uk/co/bconline/ndelius/service/GroupService.java

@@ -11,8 +11,9 @@
 public interface GroupService {
     Map<String, Set<GroupEntry>> getGroups();
     Set<GroupEntry> getGroups(String type);
-    Set<GroupEntry> getGroups(Collection<Name> groupNames);
+	Set<GroupEntry> getGroups(Collection<Name> groupNames);
     Optional<GroupEntry> getGroup(String name);
     Optional<GroupEntry> getGroup(String type, String name);
+	Set<String> getAllUsersInGroups(Map<String, Set<String>> groups);
     void save(GroupEntry group);
 }

src/main/java/uk/co/bconline/ndelius/service/impl/GroupServiceImpl.java

@@ -80,6 +80,16 @@ public Set<GroupEntry> getGroups(Collection<Name> groupNames) {
 				.collect(toSet());
 	}
 
+	@Override
+	public Set<String> getAllUsersInGroups(Map<String, Set<String>> groups) {
+		return groups.keySet().parallelStream()
+				.flatMap(type -> groups.get(type).parallelStream().map(name -> getGroup(type, name)))
+				.flatMap(Optionals::toStream)
+				.flatMap(group -> group.getMembers().stream())
+				.map(name -> LdapUtils.getStringValue(name, "cn").toLowerCase())
+				.collect(toSet());
+	}
+
 	@Override
 	public void save(GroupEntry group) {
 		groupRepository.save(group);

src/main/java/uk/co/bconline/ndelius/service/impl/UserEntryServiceImpl.java

@@ -119,7 +119,7 @@ public boolean usernameExists(String username) {
 	public List<SearchResult> search(String query, boolean includeInactiveUsers, Set<String> datasets) {
 		// For a search with no dataset filtering, we don't need to search LDAP as all the users will be returned by the DB search
 		// (We only ever need to search LDAP to find users that match on userHomeArea or email - as these attributes do not exist in the DB)
-		if (datasets.isEmpty() && !includeInactiveUsers && !query.contains("@")) return emptyList();
+		if (datasets.isEmpty() && !includeInactiveUsers && !SearchUtils.isEmailSearch(query)) return emptyList();
 
 		// Build up tokenized filter on givenName (forenames), sn (surname) and cn (username)
 		AndFilter filter = Stream.of(query.trim().split("\\s+"))

src/main/java/uk/co/bconline/ndelius/service/impl/UserServiceImpl.java

@@ -6,7 +6,6 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.core.task.TaskExecutor;
 import org.springframework.data.util.Optionals;
-import org.springframework.ldap.support.LdapUtils;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
 import uk.co.bconline.ndelius.exception.AppException;
@@ -100,23 +99,15 @@ public List<SearchResult> search(String query, Map<String, Set<String>> groupFil
 			}
 		}
 
-		// Create a Future to fetch the members of all the groups that are being filtered on
-		val groupMembersFuture = supplyAsync(() -> groupFilter.keySet().parallelStream()
-				.flatMap(type -> groupFilter.get(type).parallelStream()
-						.map(name -> groupService.getGroup(type, name)))
-				.flatMap(Optionals::toStream)
-				.flatMap(group -> group.getMembers().stream())
-				.map(name -> LdapUtils.getStringValue(name, "cn").toLowerCase())
-				.collect(toSet()), taskExecutor);
-
 		// Create Futures to search the LDAP and Database asynchronously
 		val dbFuture = supplyAsync(() -> userEntityService.search(query, includeInactiveUsers, datasetFilter).stream(), taskExecutor);
 		val ldapFuture = supplyAsync(() -> userEntryService.search(query, includeInactiveUsers, datasetFilter).stream(), taskExecutor);
 		val roleFuture = supplyAsync(() -> userRoleService.getAllUsersWithRole(role), taskExecutor);
+		val groupFuture = supplyAsync(() -> groupService.getAllUsersInGroups(groupFilter), taskExecutor);
 
 		try {
 			// fetch and combine
-			var stream = allOf(groupMembersFuture, ldapFuture, dbFuture, roleFuture)
+			var stream = allOf(ldapFuture, dbFuture, roleFuture, groupFuture)
 					.thenApply(v -> Stream.concat(
 							ldapFuture.join().map(sr -> expandEmailSearchToDB(sr, query)),
 							dbFuture.join()
@@ -129,7 +120,7 @@ public List<SearchResult> search(String query, Map<String, Set<String>> groupFil
 				stream = stream.filter(result -> result.getEndDate() == null || !result.getEndDate().isBefore(now()));
 			}
 			if (!groupFilterIsEmpty) {
-				stream = stream.filter(result -> groupMembersFuture.join().contains(result.getUsername().toLowerCase()));
+				stream = stream.filter(result -> groupFuture.join().contains(result.getUsername().toLowerCase()));
 			}
 			if (!roleIsEmpty) {
 				stream = stream.filter(result -> roleFuture.join().contains(result.getUsername().toLowerCase()));
@@ -281,10 +272,10 @@ private Set<String> myDatasets() {
 		return myDatasets;
 	}
 
-
 	private SearchResult expandEmailSearchToDB(SearchResult ldapResult, String query) {
-		if (ldapResult.getEmail() != null && SearchUtils.streamTokens(query).anyMatch(ldapResult.getEmail()::contains)) {
-			// Search the database to obtain staff and team records if the token is a substring of the search result's email
+		// If a search result was matched on email address only (from the LDAP), then we may need to fetch additional details from the Database
+		if (SearchUtils.isEmailSearch(query) && SearchUtils.resultMatchedOnEmail(query, ldapResult)) {
+			// Search the database to obtain staff and team records
 			return userEntityService.getUser(ldapResult.getUsername())
 					.map(searchResultTransformer::map)
 					.map(dbResult -> searchResultTransformer.reduce(ldapResult, dbResult))

src/main/java/uk/co/bconline/ndelius/util/SearchUtils.java

@@ -1,6 +1,7 @@
 package uk.co.bconline.ndelius.util;
 
 import lombok.experimental.UtilityClass;
+import uk.co.bconline.ndelius.model.SearchResult;
 
 import java.util.Arrays;
 import java.util.Locale;
@@ -17,4 +18,15 @@ public static String[] tokenize(String query) {
 	public static Stream<String> streamTokens(String query) {
 		return Arrays.stream(tokenize(query));
 	}
+
+	public static boolean isEmailSearch(String query) {
+		// If the search query contains an '@' symbol, then assume the user is searching for an email address
+		// (this is a very rough heuristic, but seems to work well in practice)
+		return query != null && query.contains("@");
+	}
+
+	public static boolean resultMatchedOnEmail(String query, SearchResult result) {
+		// If the result's email address contains one of the search tokens, then assume it was matched on email
+		return result.getEmail() != null && SearchUtils.streamTokens(query).anyMatch(result.getEmail()::contains);
+	}
 }