OO-753: remove the optimistic locking from SecurityGroupMemebership and make...

OO-753: remove the optimistic locking from SecurityGroupMemebership and make almost all field immutable

src/main/java/org/olat/basesecurity/BaseSecurityManager.java

@@ -530,15 +530,17 @@ public class BaseSecurityManager extends BasicManager implements BaseSecurity {
 	}
 
 	/**
+	 * 
+	 * 
 	 * @see org.olat.basesecurity.Manager#addIdentityToSecurityGroup(org.olat.core.id.Identity, org.olat.basesecurity.SecurityGroup)
 	 */
+	@Override
 	public void addIdentityToSecurityGroup(Identity identity, SecurityGroup secGroup) {
 		SecurityGroupMembershipImpl sgmsi = new SecurityGroupMembershipImpl();
 		sgmsi.setIdentity(identity);
 		sgmsi.setSecurityGroup(secGroup);
 		sgmsi.setLastModified(new Date());
-		DBFactory.getInstance().saveObject(sgmsi);
-		//TODO: tracing
+		dbInstance.getCurrentEntityManager().persist(sgmsi);
 	}
 
 	/**

src/main/java/org/olat/basesecurity/SecurityGroupMembershipImpl.hbm.xml

@@ -8,16 +8,16 @@
       <generator class="hilo"/>
     </id>
     
-    <version name="version" access="field" column="version" type="int"/>
-    <property  name="lastModified" column="lastmodified" type="timestamp" />
-	<property  name="creationDate" column="creationdate" type="timestamp" />   
+    <property name="version" access="field" column="version" insert="true" update="false" type="int"/>
+    <property name="lastModified" column="lastmodified" type="timestamp" />
+		<property name="creationDate" column="creationdate" insert="true" update="false" type="timestamp" />   
          
-    <many-to-one name="securityGroup" class="org.olat.basesecurity.SecurityGroupImpl" fetch="join" cascade="none">  
-		<column name="secgroup_id" not-null="true"	unique-key="groupmembership_unique" />    	
+    <many-to-one name="securityGroup" class="org.olat.basesecurity.SecurityGroupImpl" fetch="join" insert="true" update="false" cascade="none">  
+			<column name="secgroup_id" not-null="true"	unique-key="groupmembership_unique" />    	
     </many-to-one>  	
    
-	<many-to-one name="identity" class="org.olat.basesecurity.IdentityImpl" fetch="join" cascade="none">  
-		<column name="identity_id" not-null="true"	unique-key="groupmembership_unique" />    	
+		<many-to-one name="identity" class="org.olat.basesecurity.IdentityImpl" fetch="join" insert="true" update="false" cascade="none">  
+			<column name="identity_id" not-null="true"	unique-key="groupmembership_unique" />    	
     </many-to-one>  	
         	
   </class>

src/main/java/org/olat/basesecurity/SecurityGroupMembershipImpl.java

@@ -33,10 +33,16 @@ import org.olat.core.id.ModifiedInfo;
 
 /**
  * Description: <br>
+ * Relation between the security group and identity. The object is almost immutable
+ * on the hibernate mapping.
+ * 
  * 
  * @author Felix Jost
  */
 public class SecurityGroupMembershipImpl extends PersistentObject implements ModifiedInfo {
+
+	private static final long serialVersionUID = 2466302280763907357L;
+	
 	private Identity identity;
 	private SecurityGroup securityGroup;
 	private Date lastModified;
@@ -63,7 +69,8 @@ public class SecurityGroupMembershipImpl extends PersistentObject implements Mod
 	}
 
 	/**
-	 * Sets the identity.
+	 * Sets the identity. The identity cannot be changed and updated.
+	 * The identity is only inserted to the database but never updated
 	 * 
 	 * @param identity The identity to set
 	 */
@@ -72,7 +79,8 @@ public class SecurityGroupMembershipImpl extends PersistentObject implements Mod
 	}
 
 	/**
-	 * Sets the securityGroup.
+	 * Sets the securityGroup. The security group cannot be changed and
+	 * updated. It is only inserted to the dabatase but never updated.
 	 * 
 	 * @param securityGroup The securityGroup to set
 	 */

src/test/java/org/olat/basesecurity/BaseSecurityManagerTest.java

@@ -24,7 +24,9 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import java.util.ArrayList;
+import java.util.Calendar;
 import java.util.Collections;
+import java.util.Date;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -845,4 +847,47 @@ public class BaseSecurityManagerTest extends OlatTestCase {
 		String credential = securityManager.findCredentials(id, BaseSecurityModule.getDefaultAuthProviderIdentifier());
 		Assert.assertNotNull(credential);	
 	}
+	
+	/**
+	 * Dummy test to make sure all works as wanted
+	 */
+	@Test
+	public void createSecurityGroupMembership() {
+		//create a user with the default provider
+		Identity identity = JunitTestHelper.createAndPersistIdentityAsUser("update-membership-" + UUID.randomUUID().toString());
+		SecurityGroup secGroup = securityManager.createAndPersistSecurityGroup();
+		securityManager.addIdentityToSecurityGroup(identity, secGroup);
+		dbInstance.commitAndCloseSession();
+
+		boolean member = securityManager.isIdentityInSecurityGroup(identity, secGroup);
+		Assert.assertTrue(member);
+	}
+	
+	/**
+	 * We remove the optimistic locking from SecurityGroupMembershipImpl mapping
+	 */
+	@Test
+	public void createAndUpdateSecurityGroupMembership_lastCommitWin() {
+		//create a user with the default provider
+		Identity identity = JunitTestHelper.createAndPersistIdentityAsUser("update-membership-" + UUID.randomUUID().toString());
+		SecurityGroup secGroup = securityManager.createAndPersistSecurityGroup();
+		
+		SecurityGroupMembershipImpl sgmsi = new SecurityGroupMembershipImpl();
+		sgmsi.setIdentity(identity);
+		sgmsi.setSecurityGroup(secGroup);
+		sgmsi.setLastModified(new Date());
+		dbInstance.getCurrentEntityManager().persist(sgmsi);
+		dbInstance.commitAndCloseSession();
+
+		Calendar cal = Calendar.getInstance();
+		cal.add(Calendar.DATE, -1);
+		sgmsi.setLastModified(cal.getTime());
+		dbInstance.getCurrentEntityManager().merge(sgmsi);
+		dbInstance.commitAndCloseSession();
+	
+		cal.add(Calendar.DATE, -1);
+		sgmsi.setLastModified(cal.getTime());
+		dbInstance.getCurrentEntityManager().merge(sgmsi);
+		dbInstance.commitAndCloseSession();	
+	}
 }

src/test/java/org/olat/group/test/BusinessGroupConcurrentTest.java

+/**
+ * <a href="http://www.openolat.org">
+ * OpenOLAT - Online Learning and Training</a><br>
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License"); <br>
+ * you may not use this file except in compliance with the License.<br>
+ * You may obtain a copy of the License at the
+ * <a href="http://www.apache.org/licenses/LICENSE-2.0">Apache homepage</a>
+ * <p>
+ * Unless required by applicable law or agreed to in writing,<br>
+ * software distributed under the License is distributed on an "AS IS" BASIS, <br>
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. <br>
+ * See the License for the specific language governing permissions and <br>
+ * limitations under the License.
+ * <p>
+ * Initial code contributed and copyrighted by<br>
+ * frentix GmbH, http://www.frentix.com
+ * <p>
+ */
+package org.olat.group.test;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import junit.framework.Assert;
+
+import org.junit.Test;
+import org.olat.basesecurity.BaseSecurity;
+import org.olat.core.CoreSpringFactory;
+import org.olat.core.commons.persistence.DB;
+import org.olat.core.id.Identity;
+import org.olat.group.BusinessGroup;
+import org.olat.group.BusinessGroupService;
+import org.olat.group.manager.BusinessGroupDAO;
+import org.olat.test.JunitTestHelper;
+import org.olat.test.OlatTestCase;
+import org.springframework.beans.factory.annotation.Autowired;
+
+/**
+ * 
+ * Initial date: 28.08.2013<br>
+ * @author srosse, stephane.rosse@frentix.com, http://www.frentix.com
+ *
+ */
+public class BusinessGroupConcurrentTest extends OlatTestCase {
+	
+	@Autowired
+	private DB dbInstance;
+	@Autowired
+	private BaseSecurity securityManager;
+	@Autowired
+	private BusinessGroupDAO businessGroupDao;
+	
+
+	
+	@Test
+	public void concurrentSetLastUsageFor_multipleUser() {
+		BusinessGroup group = businessGroupDao.createAndPersist(null, "gdao", "gdao-desc", -1, -1, false, false, false, false, false);
+		dbInstance.commit();
+
+		int numOfThreads = 25;
+		final CountDownLatch doneSignal = new CountDownLatch(numOfThreads);
+		
+		SetLastUsageThread[] threads = new SetLastUsageThread[numOfThreads];
+		for(int i=numOfThreads; i-->0; ) {
+			Identity id = JunitTestHelper.createAndPersistIdentityAsUser("group-concurent-" + i + "-" + UUID.randomUUID().toString());
+			securityManager.addIdentityToSecurityGroup(id, group.getPartipiciantGroup());
+			threads[i] = new SetLastUsageThread(group.getKey(), id, doneSignal);
+		}
+		dbInstance.commitAndCloseSession();;
+		
+		for(int i=numOfThreads; i-->0; ) {
+			threads[i].start();
+		}
+
+		try {
+			boolean interrupt = doneSignal.await(240, TimeUnit.SECONDS);
+			assertTrue("Test takes too long (more than 10s)", interrupt);
+		} catch (InterruptedException e) {
+			fail("" + e.getMessage());
+		}
+		
+		int errorCount = 0;
+		for(int i=numOfThreads; i-->0; ) {
+			errorCount += threads[i].getErrorCount();
+		}
+		Assert.assertEquals(0, errorCount);
+	}
+	
+	@Test
+	public void concurrentSetLastUsageFor_singleUser() {
+		Identity id = JunitTestHelper.createAndPersistIdentityAsUser("group-cc-single-" + UUID.randomUUID().toString());
+		BusinessGroup group = businessGroupDao.createAndPersist(id, "gdao", "gdao-desc", -1, -1, false, false, false, false, false);
+		dbInstance.commitAndCloseSession();
+
+		int numOfThreads = 25;
+		final CountDownLatch doneSignal = new CountDownLatch(numOfThreads);
+		
+		SetLastUsageThread[] threads = new SetLastUsageThread[numOfThreads];
+		for(int i=numOfThreads; i-->0; ) {
+			threads[i] = new SetLastUsageThread(group.getKey(), id, doneSignal);
+		}
+		
+		for(int i=numOfThreads; i-->0; ) {
+			threads[i].start();
+		}
+
+		try {
+			boolean interrupt = doneSignal.await(240, TimeUnit.SECONDS);
+			assertTrue("Test takes too long (more than 10s)", interrupt);
+		} catch (InterruptedException e) {
+			fail("" + e.getMessage());
+		}
+		
+		int errorCount = 0;
+		for(int i=numOfThreads; i-->0; ) {
+			errorCount += threads[i].getErrorCount();
+		}
+		Assert.assertEquals(0, errorCount);
+	}
+	
+	
+	private class SetLastUsageThread extends Thread {
+		
+		private AtomicInteger errorCount = new AtomicInteger();
+		
+		private final Long key;
+		private final Identity identity;
+		private final CountDownLatch doneSignal;
+		
+		public SetLastUsageThread(Long key, Identity identity, CountDownLatch doneSignal) {
+			this.key = key;
+			this.identity = identity;
+			this.doneSignal = doneSignal;
+		}
+		
+		public int getErrorCount() {
+			return errorCount.get();
+		}
+	
+		@Override
+		public void run() {
+			try {
+				Thread.sleep(10);
+			} catch (InterruptedException e) {
+				e.printStackTrace();
+			}
+			BusinessGroupService service = CoreSpringFactory.getImpl(BusinessGroupService.class);
+			try {
+				BusinessGroup group = service.loadBusinessGroup(key);
+				for(int i=50; i-->0; ) {
+					group = service.setLastUsageFor(identity, group);
+				}
+			} catch (Exception e) {
+				e.printStackTrace();
+				errorCount.incrementAndGet();
+			} finally {
+				doneSignal.countDown();
+			}
+		}
+	}
+}
\ No newline at end of file

src/test/java/org/olat/test/AllTestsJunit4.java

@@ -83,6 +83,7 @@ import org.junit.runners.Suite;
 	org.olat.group.test.BusinessGroupServiceTest.class,//ok
 	org.olat.group.test.BusinessGroupDAOTest.class,//ok
 	org.olat.group.test.BusinessGroupRelationDAOTest.class,//ok
+	org.olat.group.test.BusinessGroupConcurrentTest.class,//ok
 	org.olat.group.test.ContactDAOTest.class,//ok
 	org.olat.resource.lock.pessimistic.PLockTest.class,//ok
 	org.olat.resource.references.ReferenceManagerTest.class,//ok