OPENJPA-293. Fixed problem with transactional state maintenance that was preventing lifecycle tests from passing.

git-svn-id: https://svn.apache.org/repos/asf/openjpa/trunk@560601 13f79535-47bb-0310-9956-ffa450edef68

openjpa-kernel/src/main/java/org/apache/openjpa/kernel/BrokerImpl.java

@@ -1584,7 +1584,7 @@ public class BrokerImpl
             Collection saved = save.rollback(_savepoints.values());
             if (_savepointCache != null)
                 _savepointCache.clear();
-            if (_transCache != null) {
+            if (hasTransactionalObjects()) {
                 // build up a new collection of states
                 TransactionalCache oldTransCache = _transCache;
                 TransactionalCache newTransCache = new TransactionalCache
@@ -2156,8 +2156,10 @@ public class BrokerImpl
         _fc.setWriteLockLevel(LOCK_NONE);
         _fc.setLockTimeout(-1);
 
-        Collection transStates = _transCache;
+        Collection transStates;
-        if (transStates == null)
+        if (hasTransactionalObjects())
+            transStates = _transCache;
+        else
             transStates = Collections.EMPTY_LIST;
 
         // fire after rollback/commit event
@@ -3750,16 +3752,26 @@ public class BrokerImpl
      * Return a copy of all transactional state managers.
      */
     protected Collection getTransactionalStates() {
-        if (_transCache == null)
+        if (!hasTransactionalObjects())
             return Collections.EMPTY_LIST;
         return _transCache.copy();
     }
 
+    /**
+     * Whether or not there are any transactional objects in the current
+     * persistence context. If there are any instances with untracked state,
+     * this method will cause those instances to be scanned.
+     */
+    private boolean hasTransactionalObjects() {
+        _cache.dirtyCheck();
+        return _transCache != null;
+    }
+
     /**
      * Return a copy of all dirty state managers.
      */
     protected Collection getDirtyStates() {
-        if (_transCache == null)
+        if (!hasTransactionalObjects())
             return Collections.EMPTY_LIST;
 
         return _transCache.copyDirty();
@@ -3823,7 +3835,7 @@ public class BrokerImpl
 
         lock();
         try {
-            if (_transCache == null)
+            if (!hasTransactionalObjects())
                 _transCache = new TransactionalCache(_orderDirty);
             _transCache.addClean(sm);
         } finally {
@@ -3839,6 +3851,8 @@ public class BrokerImpl
         lock();
         try {
             if (_transCache != null)
+                // intentional direct access; we don't want to recompute
+                // dirtiness while removing instances from the transaction
                 _transCache.remove(sm);
             if (_derefCache != null && !sm.isPersistent())
                 _derefCache.remove(sm);
@@ -3866,7 +3880,7 @@ public class BrokerImpl
             lock();
             try {
                 // cache dirty instance
-                if (_transCache == null)
+                if (!hasTransactionalObjects())
                     _transCache = new TransactionalCache(_orderDirty);
                 _transCache.addDirty(sm);
 
@@ -4427,6 +4441,12 @@ public class BrokerImpl
          * Call this method when a new state manager initializes itself.
          */
         public void add(StateManagerImpl sm) {
+            if (!sm.isIntercepting()) {
+                if (_untracked == null)
+                    _untracked = new HashSet();
+                _untracked.add(sm);
+            }
+
             if (!sm.isPersistent() || sm.isEmbedded()) {
                 if (_embeds == null)
                     _embeds = new ReferenceHashSet(ReferenceHashSet.WEAK);
@@ -4453,12 +4473,6 @@ public class BrokerImpl
                     (orig.getManagedInstance()))).
                     setFailedObject(sm.getManagedInstance());
             }
-
-            if (!sm.isIntercepting()) {
-                if (_untracked == null)
-                    _untracked = new HashSet();
-                _untracked.add(sm);
-            }
         }
 
         /**
@@ -4624,6 +4638,14 @@ public class BrokerImpl
             if (_news != null)
                 _news.clear();
         }
+
+        private void dirtyCheck() {
+            if (_untracked == null)
+                return;
+
+            for (Iterator iter = _untracked.iterator(); iter.hasNext(); )
+                ((StateManagerImpl) iter.next()).dirtyCheck();
+        }
     }
 
     /**

openjpa-kernel/src/main/java/org/apache/openjpa/kernel/SaveFieldManager.java

@@ -177,12 +177,13 @@ public class SaveFieldManager
         // if the field is not available, assume that it has changed.
         if (_saved == null || !_saved.get(field))
             return false;
-        if (!(_state.pcGetStateManager() instanceof OpenJPAStateManager))
+        if (!(_state.pcGetStateManager() instanceof StateManagerImpl))
             return false;
 
-        OpenJPAStateManager sm = (OpenJPAStateManager)
+        StateManagerImpl sm = (StateManagerImpl) _state.pcGetStateManager();
-            _state.pcGetStateManager();
+        SingleFieldManager single = new SingleFieldManager(sm, sm.getBroker());
-        Object old = sm.fetch(field);
+        sm.provideField(_state, single, field);
+        Object old = single.fetchObjectField(field);
         return current == old || current != null && current.equals(old);
     }
 

openjpa-kernel/src/main/java/org/apache/openjpa/kernel/StateManagerImpl.java

@@ -795,8 +795,8 @@ public class StateManagerImpl
         for (int i = 0; i < fmds.length; i++) {
             // pk and version fields cannot be mutated; don't mark them
             // as such. ##### validate?
-            if (!fmds[i].isPrimaryKey()
+            if (!fmds[i].isPrimaryKey() && !fmds[i].isVersion()
-                && !fmds[i].isVersion()) {
+                && _loaded.get(i)) {
                 if (!saved.isFieldEqual(i, fetch(i))) {
                     dirty(i);
                 }
@@ -811,9 +811,6 @@ public class StateManagerImpl
             return false;
         if (isNew() && !isFlushed())
             return false;
-        if (getMetaData().getAccessType() != ClassMetaData.ACCESS_FIELD
-            && !(isNew() && isFlushed()))
-            return false;
         return true;
     }
 

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/enhance/AbstractUnenhancedClassTest.java

@@ -33,6 +33,8 @@ import org.apache.openjpa.persistence.OpenJPAPersistence;
 import org.apache.openjpa.kernel.OpenJPAStateManager;
 import org.apache.openjpa.meta.ClassMetaData;
 import org.apache.openjpa.util.ImplHelper;
+import org.apache.openjpa.event.AbstractLifecycleListener;
+import org.apache.openjpa.event.LifecycleEvent;
 
 public abstract class AbstractUnenhancedClassTest
     extends SingleEMFTestCase {
@@ -60,6 +62,11 @@ public abstract class AbstractUnenhancedClassTest
 
     protected abstract UnenhancedSubtype newUnenhancedSubclassInstance();
 
+    private UnenhancedType newInstance(boolean sub) {
+        return sub ? newUnenhancedSubclassInstance()
+            : newUnenhancedInstance();
+    }
+
     public void testMetaData() {
         ClassMetaData meta = OpenJPAPersistence.getMetaData(emf,
             getUnenhancedClass());
@@ -136,15 +143,23 @@ public abstract class AbstractUnenhancedClassTest
         assertNotNull(em.getObjectId(un));
     }
 
-    public void testOperations() {
+    public void testOperationsOnUserDefined() {
-        opsHelper(false);
+        opsHelper(false, true);
     }
 
-    public void testSubclassOperations() {
+    public void testSubclassOperationsOnUserDefined() {
-        opsHelper(true);
+        opsHelper(true, true);
     }
 
-    private void opsHelper(boolean sub) {
+    public void testOperationsOnOpenJPADefined() {
+        opsHelper(false, false);
+    }
+
+    public void testSubclassOperationsOnOpenJPADefined() {
+        opsHelper(true, false);
+    }
+
+    private void opsHelper(boolean sub, boolean userDefined) {
         OpenJPAEntityManager em = null;
         try {
             UnenhancedType un = newInstance(sub);
@@ -154,22 +169,30 @@ public abstract class AbstractUnenhancedClassTest
             em.persist(un);
             un.setStringField("bar");
             assertEquals("bar", un.getStringField());
+            assertPersistenceContext(em, un, true, true, sub);
             em.flush();
+            assertPersistenceContext(em, un, true, true, sub);
             assertTrue(un.getId() != 0);
             UnenhancedType un2 = em.find(getUnenhancedClass(), un.getId());
             assertSame(un, un2);
             em.getTransaction().commit();
+            assertPersistenceContext(em, un, false, false, sub);
             un2 = em.find(getUnenhancedClass(), un.getId());
             assertSame(un, un2);
-            em.close();
 
-            em = emf.createEntityManager();
+            if (!userDefined) {
+                em.close();
+                em = emf.createEntityManager();
+            }
+
             un = em.find(getUnenhancedClass(), un.getId());
             assertNotNull(un);
-            assertTrue(un instanceof PersistenceCapable);
+            if (!userDefined)
+                assertTrue(un instanceof PersistenceCapable);
             assertEquals("bar", un.getStringField());
             em.getTransaction().begin();
             un.setStringField("baz");
+            assertPersistenceContext(em, un, true, true, sub);
             assertEquals("baz", un.getStringField());
 
             if (sub)
@@ -178,6 +201,11 @@ public abstract class AbstractUnenhancedClassTest
             assertTrue(em.isDirty(un));
             
             em.getTransaction().commit();
+
+            // make sure that the values are still up-to-date after
+            // the commit happens
+            assertEquals("baz", un.getStringField());
+            
             em.close();
 
             em = emf.createEntityManager();
@@ -196,6 +224,17 @@ public abstract class AbstractUnenhancedClassTest
         }
     }
 
+    private void assertPersistenceContext(OpenJPAEntityManager em,
+        UnenhancedType un, boolean transactional, boolean dirty, boolean sub) {
+        assertEquals(transactional, em.getTransactionalObjects().contains(un));
+        assertEquals(dirty, em.getDirtyObjects().contains(un));
+        if (dirty) {
+            Class cls = sub ? getUnenhancedSubclass() : getUnenhancedClass();
+            assertTrue(em.getUpdatedClasses().contains(cls)
+                || em.getPersistedClasses().contains(cls));
+        }
+    }
+
     public void testRelations() {
         OpenJPAEntityManager em = emf.createEntityManager();
         em.getTransaction().begin();
@@ -480,9 +519,51 @@ public abstract class AbstractUnenhancedClassTest
         em.getTransaction().commit();
     }
 
-    private UnenhancedType newInstance(boolean sub) {
+    public void testListenersOnUserDefinedInstance()
-        return sub ? newUnenhancedSubclassInstance()
+        throws IOException, ClassNotFoundException, CloneNotSupportedException {
-            : newUnenhancedInstance();
+        listenerHelper(true, false);
+    }
+
+    public void testListenersOnUserDefinedSubclassInstance()
+        throws IOException, ClassNotFoundException, CloneNotSupportedException {
+        listenerHelper(true, true);
+    }
+
+    public void testListenersOnOpenJPADefinedInstance()
+        throws IOException, ClassNotFoundException, CloneNotSupportedException {
+        listenerHelper(false, false);
+    }
+
+    public void testListenersOnOpenJPADefinedSubclassInstance()
+        throws IOException, ClassNotFoundException, CloneNotSupportedException {
+        listenerHelper(false, true);
+    }
+
+    private void listenerHelper(boolean userDefined, boolean sub)
+        throws IOException, ClassNotFoundException, CloneNotSupportedException {
+        ListenerImpl listener = new ListenerImpl();
+        emf.addLifecycleListener(listener, (Class[]) null);
+        OpenJPAEntityManager em = emf.createEntityManager();
+        UnenhancedType un = newInstance(sub);
+        em.getTransaction().begin();
+        em.persist(un);
+        em.getTransaction().commit();
+
+        if (!userDefined) {
+            em.close();
+            em = emf.createEntityManager();
+        }
+
+        listener.invoked = false;
+
+        un = em.find(getUnenhancedClass(), un.getId());
+        em.getTransaction().begin();
+        un.setStringField("updated");
+        em.getTransaction().commit();
+        assertTrue(listener.invoked);
+        em.close();
+
+        assertEquals("updated", listener.stringField);
     }
 
     public void testGetMetaDataOfSubtype() {
@@ -499,4 +580,17 @@ public abstract class AbstractUnenhancedClassTest
             Collections.singleton(getUnenhancedSubclass()), null);
         assertSame(meta, OpenJPAPersistence.getMetaData(emf, subs.get(0)));
     }
+
+    private class ListenerImpl
+        extends AbstractLifecycleListener {
+
+        String stringField;
+        boolean invoked;
+
+        @Override
+        public void afterStore(LifecycleEvent event) {
+            invoked = true;
+            stringField = ((UnenhancedType) event.getSource()).getStringField();
+        }
+    }
 }