SAK-31130 - improved performance for retrieving submissions on the Gr…

…ade Report tab of Assignments (sakaiproject#2457)

* SAK-31130 - refactored duplicated code to retrieve group submissions in BaseAssignmentService

assignment/assignment-api/api/src/java/org/sakaiproject/assignment/api/AssignmentService.java

@@ -730,6 +730,14 @@ public void removeAssignmentContent(AssignmentContentEdit content) throws Assign
 	 *         if the current user is not allowed to read this.
 	 */
 	public AssignmentSubmission getSubmission(String assignmentReference, String submitterId);
+
+	/**
+	 * Access a mapping of users to their corresponding submission on the specified assignment for the specified users
+	 * @param assignment the assignment with which the submissions should be associated
+	 * @param users the returned map's keys will contain only members of this list of users who have submissions on the specified assignment
+	 * @return a mapping of Users to their corresponding submissions, never null
+	 */
+	public Map<User, AssignmentSubmission> getUserSubmissionMap(Assignment assignment, List<User> users);
 
 	/**
 	 * Access a User's AssignmentSubmission inside a list of AssignmentSubmission object.

assignment/assignment-impl/impl/src/java/org/sakaiproject/assignment/impl/BaseAssignmentService.java

@@ -22,6 +22,7 @@
 package org.sakaiproject.assignment.impl;
 
 import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.lang.StringUtils;
 import org.apache.commons.lang.math.NumberUtils;
@@ -3488,23 +3489,7 @@ public AssignmentSubmission getSubmission(String assignmentReference, User perso
 			try {
 				Assignment a = getAssignment(assignmentReference);
 				if (a.isGroup()) {
-					M_log.debug(a.getTitle() + " is a group, getting groups");     
-					Site _site = SiteService.getSite( a.getContext() );
-					Collection groups = _site.getGroupsWithMember(person.getId());
-					if (groups != null) {
-						Iterator<Group> itgroup = groups.iterator();
-						while (itgroup.hasNext()) {
-							Group _g = itgroup.next();
-							M_log.debug("Checking submission for group:"+_g.getTitle());
-							submission = getSubmission(assignmentReference, _g.getId());
-							if (submission != null && allowGetSubmission(submission.getReference())) {
-								return submission;
-							}
-						}
-					}
-					else {
-						M_log.info("Assignment {} is grouped but {} is not in any of the site groups", a.getId(), person.getId());
-					}
+					return getUserGroupSubmissionMap(a, Collections.singletonList(person)).get(person);
 				}
 			} catch (IdUnusedException | PermissionException e) {
 				M_log.debug(e.getMessage());
@@ -3516,6 +3501,76 @@ public AssignmentSubmission getSubmission(String assignmentReference, User perso
 		return submission;
 	}
 
+	/** 
+	 * Gets a map of users to their submissions for the specified assignment
+	 * @param a the assignment in question
+	 * @param users the users making up the key set
+	 */
+	public Map<User, AssignmentSubmission> getUserSubmissionMap(Assignment a, List<User> users)
+	{
+		Map<User, AssignmentSubmission> userSubmissionMap = new HashMap<>();
+		if (a != null && !CollectionUtils.isEmpty(users))
+		{
+			if (a.isGroup())
+			{
+				userSubmissionMap.putAll(getUserGroupSubmissionMap(a, users));
+			}
+			else
+			{
+				// Get all submissions for these users with a single query
+				return m_submissionStorage.getUserSubmissionMap(a, users);
+			}
+		}
+		return userSubmissionMap;
+	}
+
+	/**
+	 * Gets a map of users to their submissions for the specified group assignment.
+	 * @param a the group assignment in question
+	 * @param users the users making up the key set
+	 */
+	private Map<User, AssignmentSubmission> getUserGroupSubmissionMap(Assignment a, List<User> users)
+	{
+		Map<User, AssignmentSubmission> userSubmissionMap = new HashMap<>();
+		if (a == null || !a.isGroup())
+		{
+			throw new IllegalArgumentException("'a' must be a group assignment");
+		}
+
+		try
+		{
+			Site _site = SiteService.getSite(a.getContext());
+			for (User user : users)
+			{
+				AssignmentSubmission submission = null;
+				Collection<Group> groups = (Collection<Group>) _site.getGroupsWithMember(user.getId());
+				if (groups != null)
+				{
+					for (Group _g : groups)
+					{
+						M_log.debug("Checking submission for group: " + _g.getTitle());
+						submission = getSubmission(a.getReference(), _g.getId());
+						if (submission != null && allowGetSubmission(submission.getReference()))
+						{
+							userSubmissionMap.put(user, submission);
+							break;
+						}
+					}
+				}
+				else
+				{
+					M_log.info("Assignment {} is grouped but {} is not in any of the site groups", a.getId(), user.getId());
+				}
+			}
+		}
+		catch (IdUnusedException e)
+		{
+			M_log.error("getUserGroupSubmissionMap invoked with an argument whose 'context' value doesn't match any siteId in the system");
+		}
+
+		return userSubmissionMap;
+	}
+
 	/**
          * 
 	 * Access a Group or User's AssignmentSubmission to a particular Assignment.
@@ -4847,11 +4902,12 @@ public Map<User, AssignmentSubmission> getSubmitterMap(String searchFilterOnly,
 			if (!rvUsers.isEmpty())
 			{
 				List<String> groupRefs = new ArrayList<String>();
+				Map<User, AssignmentSubmission> userSubmissionMap = getUserSubmissionMap(a, rvUsers);
 				for (Iterator uIterator = rvUsers.iterator(); uIterator.hasNext();)
 				{
 					User u = (User) uIterator.next();
 
-					AssignmentSubmission uSubmission = getSubmission(aRef, u);
+					AssignmentSubmission uSubmission = userSubmissionMap.get(u);
 
 					if (uSubmission != null)
 					{
@@ -13036,6 +13092,12 @@ protected interface AssignmentSubmissionStorage
 		 * @return The AssignmentSubmission with this id, or null if not found.
 		 */
 		public AssignmentSubmission get(String assignmentId, String userId);
+
+		/**
+		 * Gets a map of users to their corresponding submission on this non-group assignment for the specified users.
+		 * NB: This method does not support gorup assignments - it's intended for perfromance in retrieving submissions for non-group assignments (e. where 1 submission has 1 submitterId)
+		 */
+		public Map<User, AssignmentSubmission> getUserSubmissionMap(Assignment assignment, List<User> users);
 
 		/**
 		 * Get the number of submissions which has been submitted.

assignment/assignment-impl/impl/src/java/org/sakaiproject/assignment/impl/DbAssignmentService.java

@@ -25,9 +25,12 @@
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
+import org.apache.commons.collections.CollectionUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sakaiproject.assignment.api.Assignment;
@@ -44,6 +47,7 @@
 import org.sakaiproject.db.api.SqlReader;
 import org.sakaiproject.db.api.SqlService;
 import org.sakaiproject.entity.api.Entity;
+import org.sakaiproject.user.api.User;
 import org.sakaiproject.util.BaseDbSingleStorage;
 import org.sakaiproject.util.Xml;
 import org.w3c.dom.Document;
@@ -77,6 +81,9 @@ public class DbAssignmentService extends BaseAssignmentService
 	/** Extra fields to store in the db with the XML in ASSIGNMENT_SUBMISSION table */
 	protected static final String[] SUBMISSION_FIELDS = { "CONTEXT", "SUBMITTER_ID", "SUBMIT_TIME", "SUBMITTED", "GRADED"};
 
+	/** Oracle in clause limit */
+	protected static final int MAX_IN_CLAUSE_SIZE = 1000;
+
 	/**********************************************************************************************************************************************************************************************************************************************************
 	 * Constructors, Dependencies and their setter methods
 	 *********************************************************************************************************************************************************************************************************************************************************/
@@ -434,6 +441,85 @@ public AssignmentSubmission get (String assignmentId, String userId)
 				return null;
 			}
 		}
+
+		/**
+		 * {@inheritDoc}
+		 */
+		public Map<User, AssignmentSubmission> getUserSubmissionMap(Assignment assignment, List<User> users)
+		{
+			if (assignment == null || assignment.isGroup())
+			{
+				throw new IllegalArgumentException("getSubmissionForUsers invoked with null of group assignment");
+			}
+
+			Map<User, AssignmentSubmission> userSubmissionMap = new HashMap<>();
+
+			if (CollectionUtils.isEmpty(users))
+			{
+				return userSubmissionMap;
+			}
+
+			// Work in batches of 1000 users (due to Oracle's in clause limit)
+			int minUser = 0;
+			int maxUser = Math.min(users.size(), MAX_IN_CLAUSE_SIZE);
+			while (minUser < users.size())
+			{
+				List<User> userSublist = users.subList(minUser, maxUser);
+
+				/* 
+				 * Build a query like: 
+				 * select XML from assignment_submission where (CONTEXT = ? AND SUBMITTER_ID in (?, ?, ...));
+				 */
+				// The sql string
+				StringBuilder sql = new StringBuilder();
+				// fields are the values to be passed in. 1st param is assignment ID, the rest are userIDs
+				String fields[] = new String[1 + userSublist.size()];
+
+				String param = "?";
+				sql.append("select XML from ").append(m_submissionsTableName)
+					.append(" where (").append(SUBMISSION_FIELDS[0]).append(" = ").append(param).append(" AND ")
+					.append(SUBMISSION_FIELDS[1]).append(" in (");
+				fields[0] = caseId(assignment.getId());
+
+				// This map will be useful to retrieve Users from submission.getSubmitterId()
+				Map<String, User> userIdUserMap = new HashMap<>();
+				for (int i = 0; i < userSublist.size(); i++)
+				{
+					User u = userSublist.get(i);
+					String userId = u.getId();
+					userIdUserMap.put(userId, u);
+
+					sql.append(param);
+					// compiler optimizes this (first iteration appends "?", subsequent iterations append ",?")
+					param = ",?";
+					// first field is assignmentId, all userId fields' indices are shifted up 1
+					fields[i + 1] = userId;
+				}
+				// append "))" to close "in (" and "where("
+				sql.append("))");
+
+				List xmlResources = m_sql.dbRead(sql.toString(), fields, null);
+				for (Object xml : xmlResources)
+				{
+					AssignmentSubmission submission = (AssignmentSubmission) readResource((String) xml);
+					String submitterId = submission.getSubmitterId();
+					User u = userIdUserMap.get(submitterId);
+					if (u == null)
+					{
+						M_log.warn("getUserSubmissionMap() - submission's submitterId not found in the original user list");
+					}
+					else
+					{
+						userSubmissionMap.put(u, submission);
+					}
+				}
+
+				minUser += MAX_IN_CLAUSE_SIZE;
+				maxUser = Math.min(users.size(), minUser + MAX_IN_CLAUSE_SIZE);
+			}
+
+			return userSubmissionMap;
+		}
 
 		/**
 		 * Helper method to exclude inactive site members from submissions count