OO-1853: deduplicates the list of collections for security, remove a possible...

OO-1853: deduplicates the list of collections for security, remove a possible QuestionItemImpl from the cache for the lock

src/main/java/org/olat/modules/qpool/manager/CollectionDAO.java

@@ -21,7 +21,9 @@ package org.olat.modules.qpool.manager;
 
 import java.util.ArrayList;
 import java.util.Date;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import javax.persistence.TypedQuery;
 
@@ -84,12 +86,13 @@ public class CollectionDAO {
 	 * @param collection
 	 * @return true if the item is in the collection after the call
 	 */
-	public boolean addItemToCollection(Long itemKey, List<QuestionItemCollection> collections) {
-		QuestionItemImpl lockedItem = questionItemDao.loadForUpdate(itemKey);
+	public boolean addItemToCollection(QuestionItemShort item, List<QuestionItemCollection> collections) {
+		QuestionItemImpl lockedItem = questionItemDao.loadForUpdate(item);
 		if(lockedItem == null) {
 			return false;
 		}
-		for(QuestionItemCollection collection:collections) {
+		Set<QuestionItemCollection> uniqueCollections = new HashSet<>(collections);
+		for(QuestionItemCollection collection:uniqueCollections) {
 			if(!isInCollection(collection, lockedItem)) {
 				CollectionToItem coll2Item = new CollectionToItem();
 				coll2Item.setCreationDate(new Date());

src/main/java/org/olat/modules/qpool/manager/PoolDAO.java

@@ -207,7 +207,7 @@ public class PoolDAO {
 	}
 	
 	public void addItemToPool(QuestionItemShort item, List<Pool> pools, boolean editable) {
-		QuestionItem lockedItem = questionItemDao.loadForUpdate(item.getKey());
+		QuestionItem lockedItem = questionItemDao.loadForUpdate(item);
 		for(Pool pool:pools) {
 			if(!isInPool(lockedItem, pool)) {
 				PoolToItem p2i = new PoolToItem();

src/main/java/org/olat/modules/qpool/manager/QuestionItemDAO.java

@@ -161,8 +161,8 @@ public class QuestionItemDAO {
 		return (QuestionItemImpl)dbInstance.getCurrentEntityManager().merge(item);
 	}
 	
-	public void addAuthors(List<Identity> authors, Long itemKey) {
-		QuestionItemImpl lockedItem = loadForUpdate(itemKey);
+	public void addAuthors(List<Identity> authors, QuestionItemShort item) {
+		QuestionItemImpl lockedItem = loadForUpdate(item);
 		SecurityGroup secGroup = lockedItem.getOwnerGroup();
 		for(Identity author:authors) {
 			if(!securityManager.isIdentityInSecurityGroup(author, secGroup)) {
@@ -172,8 +172,8 @@ public class QuestionItemDAO {
 		dbInstance.commit();
 	}
 	
-	public void removeAuthors(List<Identity> authors, Long itemKey) {
-		QuestionItemImpl lockedItem = loadForUpdate(itemKey);
+	public void removeAuthors(List<Identity> authors, QuestionItemShort item) {
+		QuestionItemImpl lockedItem = loadForUpdate(item);
 		SecurityGroup secGroup = lockedItem.getOwnerGroup();
 		for(Identity author:authors) {
 			if(securityManager.isIdentityInSecurityGroup(author, secGroup)) {
@@ -259,15 +259,19 @@ public class QuestionItemDAO {
 		return items;
 	}
 	
-	public QuestionItemImpl loadForUpdate(Long key) {
+	public QuestionItemImpl loadForUpdate(QuestionItemShort item) {
+		if(item instanceof QuestionItemImpl) {
+			//remove from the cache
+			dbInstance.getCurrentEntityManager().detach(item);
+		}
 		StringBuilder sb = new StringBuilder();
 		sb.append("select item from questionitem item where item.key=:key");
-		QuestionItemImpl item = dbInstance.getCurrentEntityManager()
+		QuestionItemImpl lockedItem = dbInstance.getCurrentEntityManager()
 				.createQuery(sb.toString(), QuestionItemImpl.class)
-				.setParameter("key", key)
+				.setParameter("key", item.getKey())
 				.setLockMode(LockModeType.PESSIMISTIC_WRITE)
 				.getSingleResult();
-		return item;
+		return lockedItem;
 	}
 	
 	public int getNumOfQuestions() {
@@ -289,7 +293,7 @@ public class QuestionItemDAO {
 	}
 	
 	public void share(QuestionItem item, OLATResource resource) {
-		QuestionItem lockedItem = loadForUpdate(item.getKey());
+		QuestionItem lockedItem = loadForUpdate(item);
 		if(!isShared(item, resource)) {
 			EntityManager em = dbInstance.getCurrentEntityManager();
 			ResourceShareImpl share = new ResourceShareImpl();
@@ -301,9 +305,9 @@ public class QuestionItemDAO {
 		dbInstance.commit();//release the lock asap
 	}
 	
-	public void share(Long itemKey, List<OLATResource> resources, boolean editable) {
+	public void share(QuestionItemShort item, List<OLATResource> resources, boolean editable) {
 		EntityManager em = dbInstance.getCurrentEntityManager();
-		QuestionItem lockedItem = loadForUpdate(itemKey);
+		QuestionItem lockedItem = loadForUpdate(item);
 		for(OLATResource resource:resources) {
 			if(!isShared(lockedItem, resource)) {
 				ResourceShareImpl share = new ResourceShareImpl();

src/main/java/org/olat/modules/qpool/manager/QuestionPoolServiceImpl.java

@@ -149,7 +149,7 @@ public class QuestionPoolServiceImpl implements QPoolService {
 		}
 		
 		for(QuestionItemShort item:items) {
-			questionItemDao.addAuthors(authors, item.getKey());
+			questionItemDao.addAuthors(authors, item);
 		}
 	}
 	
@@ -160,7 +160,7 @@ public class QuestionPoolServiceImpl implements QPoolService {
 		}
 		
 		for(QuestionItemShort item:items) {
-			questionItemDao.removeAuthors(authors, item.getKey());
+			questionItemDao.removeAuthors(authors, item);
 		}
 	}
 
@@ -537,7 +537,7 @@ public class QuestionPoolServiceImpl implements QPoolService {
 		}
 		
 		for(QuestionItemShort item:items) {
-			questionItemDao.share(item.getKey(), resources, editable);
+			questionItemDao.share(item, resources, editable);
 		}
 	}
 
@@ -595,7 +595,7 @@ public class QuestionPoolServiceImpl implements QPoolService {
 		QuestionItemCollection coll = collectionDao.createCollection(collectionName, owner);
 		List<Long> keys = new ArrayList<>(initialItems.size());
 		for(QuestionItemShort item:initialItems) {
-			collectionDao.addItemToCollection(item.getKey(), Collections.singletonList(coll));
+			collectionDao.addItemToCollection(item, Collections.singletonList(coll));
 			keys.add(item.getKey());
 		}
 		lifeIndexer.indexDocument(QItemDocument.TYPE, keys);
@@ -616,7 +616,7 @@ public class QuestionPoolServiceImpl implements QPoolService {
 	public void addItemToCollection(List<? extends QuestionItemShort> items, List<QuestionItemCollection> collections) {
 		List<Long> keys = new ArrayList<>(items.size());
 		for(QuestionItemShort item:items) {
-			collectionDao.addItemToCollection(item.getKey(), collections);
+			collectionDao.addItemToCollection(item, collections);
 			keys.add(item.getKey());
 		}
 		lifeIndexer.indexDocument(QItemDocument.TYPE, keys);

src/test/java/org/olat/modules/qpool/manager/CollectionDAOTest.java

@@ -95,7 +95,7 @@ public class CollectionDAOTest extends OlatTestCase {
 		dbInstance.commitAndCloseSession();
 		
 		//add the item to the collection
-		collectionDao.addItemToCollection(item.getKey(), singletonList(coll));
+		collectionDao.addItemToCollection(item, singletonList(coll));
 		dbInstance.commit();//check if it's alright
 	}
 	
@@ -107,8 +107,8 @@ public class CollectionDAOTest extends OlatTestCase {
 		QuestionItemCollection coll = collectionDao.createCollection("NGC collection 4", id);
 		QuestionItem item1 = questionDao.createAndPersist(null, "NGC 99", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
 		QuestionItem item2 = questionDao.createAndPersist(null, "NGC 101", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
-		collectionDao.addItemToCollection(item1.getKey(), singletonList(coll));
-		collectionDao.addItemToCollection(item2.getKey(), singletonList(coll));
+		collectionDao.addItemToCollection(item1, singletonList(coll));
+		collectionDao.addItemToCollection(item2, singletonList(coll));
 		dbInstance.commit();//check if it's alright
 		
 		//load the items of the collection
@@ -143,10 +143,10 @@ public class CollectionDAOTest extends OlatTestCase {
 		QuestionItemCollection coll2 = collectionDao.createCollection("NGC collection 9", id);
 		QuestionItem item1 = questionDao.createAndPersist(null, "NGC 103", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
 		QuestionItem item2 = questionDao.createAndPersist(null, "NGC 104", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
-		collectionDao.addItemToCollection(item1.getKey(), singletonList(coll1));
-		collectionDao.addItemToCollection(item1.getKey(), singletonList(coll2));
-		collectionDao.addItemToCollection(item2.getKey(), singletonList(coll1));
-		collectionDao.addItemToCollection(item2.getKey(), singletonList(coll2));
+		collectionDao.addItemToCollection(item1, singletonList(coll1));
+		collectionDao.addItemToCollection(item1, singletonList(coll2));
+		collectionDao.addItemToCollection(item2, singletonList(coll1));
+		collectionDao.addItemToCollection(item2, singletonList(coll2));
 		dbInstance.commit();
 		
 		//check if it's alright
@@ -177,8 +177,8 @@ public class CollectionDAOTest extends OlatTestCase {
 		QuestionItemCollection coll = collectionDao.createCollection("NGC collection 10", id);
 		QuestionItem item1 = questionDao.createAndPersist(null, "NGC 107", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
 		QuestionItem item2 = questionDao.createAndPersist(null, "NGC 108", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
-		collectionDao.addItemToCollection(item1.getKey(), singletonList(coll));
-		collectionDao.addItemToCollection(item2.getKey(), singletonList(coll));
+		collectionDao.addItemToCollection(item1, singletonList(coll));
+		collectionDao.addItemToCollection(item2, singletonList(coll));
 		dbInstance.commit();
 		
 		//check if it's alright

src/test/java/org/olat/modules/qpool/manager/QItemQueriesDAOTest.java

@@ -141,8 +141,8 @@ public class QItemQueriesDAOTest extends OlatTestCase  {
 		QuestionItemCollection coll = collectionDao.createCollection("NGC collection 3", id);
 		QuestionItem item1 = questionDao.createAndPersist(null, "NGC 92", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
 		QuestionItem item2 = questionDao.createAndPersist(null, "NGC 97", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
-		collectionDao.addItemToCollection(item1.getKey(), singletonList(coll));
-		collectionDao.addItemToCollection(item2.getKey(), singletonList(coll));
+		collectionDao.addItemToCollection(item1, singletonList(coll));
+		collectionDao.addItemToCollection(item2, singletonList(coll));
 		dbInstance.commit();//check if it's alright
 		
 		//load the items of the collection
@@ -173,7 +173,7 @@ public class QItemQueriesDAOTest extends OlatTestCase  {
 		Identity id = JunitTestHelper.createAndPersistIdentityAsUser("Coll-Onwer-3-" + UUID.randomUUID().toString());
 		QuestionItemCollection coll = collectionDao.createCollection("NGC collection 3", id);
 		QuestionItem item = questionDao.createAndPersist(null, "NGC 92", QTIConstants.QTI_12_FORMAT, Locale.GERMAN.getLanguage(), null, null, null, fibType);
-		collectionDao.addItemToCollection(item.getKey(), singletonList(coll));
+		collectionDao.addItemToCollection(item, singletonList(coll));
 		dbInstance.commit();//check if it's alright
 		
 		//test order by
@@ -315,7 +315,7 @@ public class QItemQueriesDAOTest extends OlatTestCase  {
 		List<OLATResource> resources = new ArrayList<OLATResource>();
 		resources.add(group1.getResource());
 		resources.add(group2.getResource());
-		questionDao.share(item.getKey(), resources, false);
+		questionDao.share(item, resources, false);
 		
 		//retrieve them
 		List<QuestionItemView> sharedItems1 = qItemQueriesDao.getSharedItemByResource(id, group1.getResource(), null, 0, -1);
@@ -340,7 +340,7 @@ public class QItemQueriesDAOTest extends OlatTestCase  {
 		//share them
 		List<OLATResource> resources = new ArrayList<OLATResource>();
 		resources.add(group.getResource());
-		questionDao.share(item.getKey(), resources, false);
+		questionDao.share(item, resources, false);
 		dbInstance.commitAndCloseSession();