From 1bf0e1d6acc1df4da77cfcf315bfe16f58a12def Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Mon, 17 Sep 2018 14:41:02 -0400 Subject: don't begin new transaction Persistence code was not checking to see if a transaction was already in progress before beginning a transaction. Fix checkstyle issue with comments and parameter for EntityMgrException. Restore other checkstyle fixes. More checkstyle fixes. Rename some variables. Change-Id: Ic820944781571ba2ef411cb86d12fa32fb206124 Issue-ID: POLICY-1106 Signed-off-by: Jim Hahn --- .../policy/drools/persistence/EntityMgrTrans.java | 62 +++-- .../drools/persistence/EntityMgrTransTest.java | 295 ++++++++++++++------- 2 files changed, 248 insertions(+), 109 deletions(-) (limited to 'feature-session-persistence') diff --git a/feature-session-persistence/src/main/java/org/onap/policy/drools/persistence/EntityMgrTrans.java b/feature-session-persistence/src/main/java/org/onap/policy/drools/persistence/EntityMgrTrans.java index 3d276519..988cd9c2 100644 --- a/feature-session-persistence/src/main/java/org/onap/policy/drools/persistence/EntityMgrTrans.java +++ b/feature-session-persistence/src/main/java/org/onap/policy/drools/persistence/EntityMgrTrans.java @@ -28,27 +28,40 @@ import javax.transaction.RollbackException; import javax.transaction.Status; import javax.transaction.SystemException; import javax.transaction.UserTransaction; - import org.onap.policy.common.utils.jpa.EntityMgrCloser; /** - * Wrapper for an EntityManager that creates a JTA transaction that is auto-rolled back when - * closed. + * Wrapper for an EntityManager that creates a JTA transaction that is auto-rolled + * back when closed. Note: commit() and rollback() actions do nothing if a transaction was + * already in progress when this object was constructed. */ public class EntityMgrTrans extends EntityMgrCloser { - /** Transaction to be rolled back. */ + /** + * Transaction to be rolled back. + */ private static UserTransaction userTrans = com.arjuna.ats.jta.UserTransaction.userTransaction(); + /** + * {@code True} if a transaction had already been started before this object was + * constructed, {@code false} otherwise. + */ + private boolean transInProgress; + /** * Constructor. - * - * @param em entity for which a transaction is to be begun */ + * + * @param em entity for which a transaction is to be begun + */ public EntityMgrTrans(EntityManager em) { super(em); try { - userTrans.begin(); + transInProgress = (userTrans.getStatus() == Status.STATUS_ACTIVE); + if (!transInProgress) { + userTrans.begin(); + } + em.joinTransaction(); } catch (RuntimeException | NotSupportedException | SystemException e) { @@ -75,24 +88,32 @@ public class EntityMgrTrans extends EntityMgrCloser { EntityMgrTrans.userTrans = userTrans; } - /** Commits the transaction. */ + /** + * Commits the transaction. + */ public void commit() { + if (transInProgress) { + return; + } + try { userTrans.commit(); - } catch (SecurityException - | IllegalStateException - | RollbackException - | HeuristicMixedException - | HeuristicRollbackException - | SystemException e) { + } catch (SecurityException | IllegalStateException | RollbackException | HeuristicMixedException + | HeuristicRollbackException | SystemException e) { throw new EntityMgrException(e); } } - /** Rolls back the transaction. */ + /** + * Rolls back the transaction. + */ public void rollback() { + if (transInProgress) { + return; + } + try { userTrans.rollback(); @@ -104,7 +125,7 @@ public class EntityMgrTrans extends EntityMgrCloser { @Override public void close() { try { - if (userTrans.getStatus() == Status.STATUS_ACTIVE) { + if (!transInProgress && userTrans.getStatus() == Status.STATUS_ACTIVE) { userTrans.rollback(); } @@ -117,12 +138,17 @@ public class EntityMgrTrans extends EntityMgrCloser { } /** - * Runtime exceptions generated by this class. Wraps exceptions generated by delegated operations, - * particularly when they are not, themselves, Runtime exceptions. + * Runtime exceptions generated by this class. Wraps exceptions generated by delegated + * operations, particularly when they are not, themselves, Runtime exceptions. */ public static class EntityMgrException extends RuntimeException { private static final long serialVersionUID = 1L; + /** + * Constructor. + * + * @param ex exception to be wrapped + */ public EntityMgrException(Exception ex) { super(ex); } diff --git a/feature-session-persistence/src/test/java/org/onap/policy/drools/persistence/EntityMgrTransTest.java b/feature-session-persistence/src/test/java/org/onap/policy/drools/persistence/EntityMgrTransTest.java index aba1d80e..c622a933 100644 --- a/feature-session-persistence/src/test/java/org/onap/policy/drools/persistence/EntityMgrTransTest.java +++ b/feature-session-persistence/src/test/java/org/onap/policy/drools/persistence/EntityMgrTransTest.java @@ -1,4 +1,4 @@ -/*- +/* * ============LICENSE_START======================================================= * feature-session-persistence * ================================================================================ @@ -21,7 +21,7 @@ package org.onap.policy.drools.persistence; import static org.junit.Assert.assertEquals; - +import static org.junit.Assert.fail; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -29,7 +29,11 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.HashMap; +import java.util.Map; import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; +import javax.persistence.Persistence; import javax.transaction.HeuristicMixedException; import javax.transaction.HeuristicRollbackException; import javax.transaction.NotSupportedException; @@ -37,7 +41,6 @@ import javax.transaction.RollbackException; import javax.transaction.Status; import javax.transaction.SystemException; import javax.transaction.UserTransaction; - import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -56,8 +59,7 @@ public class EntityMgrTransTest { private EntityManager mgr; /** - * Setup before the class. - * + * Configure properties for JTA. */ @BeforeClass public static void setUpBeforeClass() { @@ -73,40 +75,68 @@ public class EntityMgrTransTest { } /** - * Setup. - * - * @throws Exception exception + * Creates a mock transaction and entity manager. Resets the "userTrans" field of the + * class under test. + * + * @throws Exception if an error occurs */ @Before public void setUp() throws Exception { trans = mock(UserTransaction.class); mgr = mock(EntityManager.class); + when(trans.getStatus()).thenReturn(Status.STATUS_NO_TRANSACTION, Status.STATUS_ACTIVE); + EntityMgrTrans.setUserTrans(trans); } /** - * Verifies that the constructor starts a transaction, but does not do anything extra before being - * closed. + * Verifies that the constructor starts a transaction, but does not do anything extra + * before being closed. + * + * @throws Exception if an error occurs + */ + @Test + public void testEntityMgrTrans_Inactive() throws Exception { + EntityMgrTrans emt = new EntityMgrTrans(mgr); + + // verify that transaction was started + verify(trans).begin(); + verify(mgr).joinTransaction(); + + // verify not closed, committed, or rolled back yet + verify(trans, never()).commit(); + verify(trans, never()).rollback(); + verify(mgr, never()).close(); + + emt.close(); + } + + /** + * Verifies that the constructor does not start a transaction, because one is already + * active. * - * @throws Exception exception + * @throws Exception if an error occurs */ @Test - public void testEntityMgrTrans() throws Exception { + public void testEntityMgrTrans_Active() throws Exception { when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); - final EntityMgrTrans newTrans = new EntityMgrTrans(mgr); + EntityMgrTrans emt = new EntityMgrTrans(mgr); - // verify that transaction was started - verify(trans).begin(); + // verify that transaction was not re-started started + verify(trans, never()).begin(); + + // verify that transaction was joined + verify(mgr).joinTransaction(); // verify not closed, committed, or rolled back yet verify(trans, never()).commit(); verify(trans, never()).rollback(); verify(mgr, never()).close(); - newTrans.close(); + emt.close(); } @Test(expected = EntityMgrException.class) @@ -114,7 +144,7 @@ public class EntityMgrTransTest { doThrow(new IllegalArgumentException("expected exception")).when(trans).begin(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } @@ -124,7 +154,7 @@ public class EntityMgrTransTest { doThrow(new NotSupportedException("expected exception")).when(trans).begin(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } @@ -134,22 +164,37 @@ public class EntityMgrTransTest { doThrow(new SystemException("expected exception")).when(trans).begin(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } /** - * Verifies that the transaction is rolled back and the manager is closed when and a transaction - * is active. + * Verifies that the transaction is not rolled back, but the manager is closed when a + * transaction is already active. */ @Test public void testClose_Active() throws Exception { - EntityMgrTrans newTrans = new EntityMgrTrans(mgr); - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); - newTrans.close(); + EntityMgrTrans emt = new EntityMgrTrans(mgr); + emt.close(); + + // closed and rolled back, but not committed + verify(trans, never()).commit(); + verify(trans, never()).rollback(); + verify(mgr).close(); + } + + /** + * Verifies that the transaction is rolled back and the manager is closed when a + * transaction is begun by the constructor. + */ + @Test + public void testClose_Begun() throws Exception { + EntityMgrTrans emt = new EntityMgrTrans(mgr); + + emt.close(); // closed and rolled back, but not committed verify(trans, never()).commit(); @@ -158,16 +203,16 @@ public class EntityMgrTransTest { } /** - * Verifies that the manager is closed, but that the transaction is not rolled back when - * and no transaction is active. + * Verifies that the manager is closed, but that the transaction is not rolled + * back when no transaction is active. */ @Test public void testClose_Inactive() throws Exception { - EntityMgrTrans newTrans = new EntityMgrTrans(mgr); - when(trans.getStatus()).thenReturn(Status.STATUS_NO_TRANSACTION); - newTrans.close(); + EntityMgrTrans emt = new EntityMgrTrans(mgr); + + emt.close(); // closed, but not committed or rolled back verify(mgr).close(); @@ -178,10 +223,9 @@ public class EntityMgrTransTest { @Test(expected = EntityMgrException.class) public void testClose_IllStateEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new IllegalStateException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } @@ -189,10 +233,9 @@ public class EntityMgrTransTest { @Test(expected = EntityMgrException.class) public void testClose_SecEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SecurityException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } @@ -200,23 +243,20 @@ public class EntityMgrTransTest { @Test(expected = EntityMgrException.class) public void testClose_SysEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SystemException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } } /** - * Verifies that the manager is closed and the transaction rolled back when "try" block exits - * normally and a transaction is active. + * Verifies that the manager is closed and the transaction rolled back when "try" + * block exits normally and a transaction is active. */ @Test public void testClose_TryWithoutExcept_Active() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); - - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } @@ -227,15 +267,15 @@ public class EntityMgrTransTest { } /** - * Verifies that the manager is closed, but that the transaction is not rolled back when - * "try" block exits normally and no transaction is active. + * Verifies that the manager is closed, but that the transaction is not rolled + * back when "try" block exits normally and no transaction is active. */ @Test public void testClose_TryWithoutExcept_Inactive() throws Exception { when(trans.getStatus()).thenReturn(Status.STATUS_NO_TRANSACTION); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { // Empty } @@ -246,16 +286,13 @@ public class EntityMgrTransTest { } /** - * Verifies that the manager is closed and the transaction rolled back when "try" block throws an - * exception and a transaction is active. + * Verifies that the manager is closed and the transaction rolled back when "try" + * block throws an exception and a transaction is active. */ @Test public void testClose_TryWithExcept_Active() throws Exception { - - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); - try { - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { throw new SystemException("expected exception"); } @@ -270,8 +307,8 @@ public class EntityMgrTransTest { } /** - * Verifies that the manager is closed, but that the transaction is not rolled back when - * "try" block throws an exception and no transaction is active. + * Verifies that the manager is closed, but that the transaction is not rolled + * back when "try" block throws an exception and no transaction is active. */ @Test public void testClose_TryWithExcept_Inactive() throws Exception { @@ -279,7 +316,7 @@ public class EntityMgrTransTest { when(trans.getStatus()).thenReturn(Status.STATUS_NO_TRANSACTION); try { - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { throw new SystemException("expected exception"); } @@ -293,13 +330,15 @@ public class EntityMgrTransTest { verify(mgr).close(); } - /** Verifies that commit() only commits, and that the subsequent close() does not re-commit. */ + /** + * Verifies that commit() only commits, and that the subsequent close() does not + * re-commit. + */ @Test public void testCommit() throws Exception { - EntityMgrTrans newTrans = new EntityMgrTrans(mgr); - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); + EntityMgrTrans emt = new EntityMgrTrans(mgr); - newTrans.commit(); + emt.commit(); when(trans.getStatus()).thenReturn(Status.STATUS_COMMITTED); @@ -309,88 +348,107 @@ public class EntityMgrTransTest { verify(mgr, never()).close(); // closed, but not re-committed - newTrans.close(); + emt.close(); verify(trans, times(1)).commit(); verify(mgr).close(); } + /** + * Verifies that commit() does nothing, and that the subsequent close() does not + * re-commit when a transaction is already active. + */ + @Test + public void testCommit_Active() throws Exception { + when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); + + EntityMgrTrans emt = new EntityMgrTrans(mgr); + + emt.commit(); + + // nothing happened yet + verify(trans, never()).commit(); + verify(trans, never()).rollback(); + verify(mgr, never()).close(); + + // closed, but not re-committed + emt.close(); + + // still no commit or rollback + verify(trans, never()).commit(); + verify(trans, never()).rollback(); + verify(mgr).close(); + } + @Test(expected = EntityMgrException.class) public void testCommit_SecEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SecurityException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } @Test(expected = EntityMgrException.class) public void testCommit_IllStateEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new IllegalStateException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } @Test(expected = EntityMgrException.class) public void testCommit_RbEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new RollbackException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } @Test(expected = EntityMgrException.class) public void testCommit_HmEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new HeuristicMixedException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } @Test(expected = EntityMgrException.class) public void testCommit_HrbEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new HeuristicRollbackException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } @Test(expected = EntityMgrException.class) public void testCommit_SysEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SystemException("expected exception")).when(trans).commit(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.commit(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.commit(); } } /** - * Verifies that rollback() only rolls back, and that the subsequent close() does not re-roll - * back. + * Verifies that rollback() only rolls back, and that the subsequent close() does not + * re-roll back. */ @Test public void testRollback() throws Exception { - EntityMgrTrans newTrans = new EntityMgrTrans(mgr); - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); + EntityMgrTrans emt = new EntityMgrTrans(mgr); - newTrans.rollback(); + emt.rollback(); when(trans.getStatus()).thenReturn(Status.STATUS_ROLLEDBACK); @@ -400,42 +458,65 @@ public class EntityMgrTransTest { verify(mgr, never()).close(); // closed, but not re-rolled back - newTrans.close(); + emt.close(); - verify(trans, times(1)).rollback(); + // still no commit or rollback + verify(trans, never()).commit(); + verify(trans).rollback(); + verify(mgr).close(); + } + + /** + * Verifies that rollback() does nothing, and that the subsequent close() does not + * re-roll back when a transaction is already active. + */ + @Test + public void testRollback_Active() throws Exception { + when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); + EntityMgrTrans emt = new EntityMgrTrans(mgr); + + emt.rollback(); + + // nothing happens + verify(trans, never()).commit(); + verify(trans, never()).rollback(); + verify(mgr, never()).close(); + + emt.close(); + + // still no commit or rollback + verify(trans, never()).commit(); + verify(trans, never()).rollback(); verify(mgr).close(); } @Test(expected = EntityMgrException.class) public void testRollback_IllStateEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new IllegalStateException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.rollback(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.rollback(); } } @Test(expected = EntityMgrException.class) public void testRollback_SecEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SecurityException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.rollback(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.rollback(); } } @Test(expected = EntityMgrException.class) public void testRollback_SysEx() throws Exception { - when(trans.getStatus()).thenReturn(Status.STATUS_ACTIVE); doThrow(new SystemException("expected exception")).when(trans).rollback(); - try (EntityMgrTrans t = new EntityMgrTrans(mgr)) { - t.rollback(); + try (EntityMgrTrans emt = new EntityMgrTrans(mgr)) { + emt.rollback(); } } @@ -445,5 +526,37 @@ public class EntityMgrTransTest { EntityMgrException ex = new EntityMgrException(secex); assertEquals(secex, ex.getCause()); + + } + + /** + * Tests using real (i.e., not mocked) Persistence classes. + */ + @Test + public void testReal() { + EntityMgrTrans.setUserTrans(savetrans); + + Map propMap = new HashMap<>(); + + propMap.put("javax.persistence.jdbc.driver", "org.h2.Driver"); + propMap.put("javax.persistence.jdbc.url", "jdbc:h2:mem:EntityMgrTransTest"); + + EntityManagerFactory emf = Persistence.createEntityManagerFactory("junitDroolsSessionEntityPU", propMap); + + try (EntityMgrTrans trans1 = new EntityMgrTrans(emf.createEntityManager())) { + + // nest a transaction - should still be OK + + try (EntityMgrTrans trans2 = new EntityMgrTrans(emf.createEntityManager())) { + // Empty + } + + } catch (Exception e) { + logger.info("persistence error", e); + emf.close(); + fail("persistence error"); + } + + emf.close(); } } -- cgit 1.2.3-korg