Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.3, 1.1.0, 1.2.0
    • Fix Version/s: 1.3.0, 2.0.0-M2
    • Component/s: kernel
    • Labels:
      None

      Description

      EntityManager.clear() now don't flush new object but only detach it.
      But DetachManager still flush dirty object and assume detached objects are in clean state.
      When the "new" object is merged back and transaction commit, because the object state lost its original state PNEW, it will not be added to insert list and not flushed to DB.

      According to the EntityManager.clear() API, changes made to entities that have not been flushed to the database will not be persisted. When they merges back to persistent context, they all should kept there original state.

      I added the following test to org.apache.openjpa.persistence.simple.TestEntityManagerClear.

      public void testClearMerge()

      { // Create EntityManager and Start a transaction (1) begin(); // Insert a new object then clear persistent context AllFieldTypes testObject1 = new AllFieldTypes(); testObject1.setStringField("my test object1"); persist(testObject1); //Object1 is not flushed to DB but only detached by clear(). em.clear(); em.merge(testObject1); //expect the PCState is same as before detached, //so it is PNew instead of PCLEAN and is add to insert list. commit(); //Start a new transaction begin(); // Attempt retrieve of Object1 from previous PC (should exist) assertEquals(1, query("select x from AllFieldTypes x " + "where x.stringField = 'my test object1'"). getResultList().size()); // Rollback the transaction and close everything rollback(); }
      1. openjpa-722-fromQin.patch
        14 kB
        Xiaoqin Feng
      2. openjpa-722-V2.patch
        7 kB
        Xiaoqin Feng
      3. openjpa-722-V3.patch
        7 kB
        Xiaoqin Feng
      4. TestEntityManagerClear.java
        4 kB
        Xiaoqin Feng
      5. TestEntityManagerClear-V2.java
        7 kB
        Xiaoqin Feng

        Activity

        Hide
        Abe White added a comment -

        Applied V3 patch in SVN revision 700158.

        Show
        Abe White added a comment - Applied V3 patch in SVN revision 700158.
        Hide
        Xiaoqin Feng added a comment -

        2 tests failed with V2 patch when I run all test suites.
        Move "!(sm.isNew() && !sm.isDeleted() && !sm.isFlushed())" from DetachManager.useDetachedStateManager(StateManagerImpl, DetachOptions) to detachInternal(Object) since useDetachedStateManager is used by other calls.
        Now all tests passed.
        Please use V3 patch.

        Show
        Xiaoqin Feng added a comment - 2 tests failed with V2 patch when I run all test suites. Move "!(sm.isNew() && !sm.isDeleted() && !sm.isFlushed())" from DetachManager.useDetachedStateManager(StateManagerImpl, DetachOptions) to detachInternal(Object) since useDetachedStateManager is used by other calls. Now all tests passed. Please use V3 patch.
        Hide
        Xiaoqin Feng added a comment -

        Yes, we shouldn't save PCState in pcDetachedState since pcDetachedStaet is used for loaded field, version info to make attach process more efficient.
        V2 patch is attached.
        Here is my solution:
        1. Our assumption according to EntityManager.clear() API is ""EntityManager.clear() will not flush new, dirty, removed entities in context but just detach all managed entities".

        At first I thought we need to pass along flush=false from BrokerImpl.detachAll(OpCallbacks call, boolean flush) to DetachManager _flushBeforeDetach as openjpa-591 mentioned. Then I realized in BrokerImpl.detachAllInternal(), new DetachManager(this, true, call).detachAll (new ManagedObjectCollection(states)) actually pass in full=true so _flushBeforeDetach has no effect in this case and flush logic in DetachManager will be skipped.

        So I reverted my change in V1 patch for this part.

        2. To make not flushed new entity to be inserted after merge, we don't save DetachedStateManager in detached object. When it merged back, exisitng openjpa framework could handle it correctly and add it to insert list.

        3. To make not flushed dirty entity to be updated after merge, we save dirty flag BitSet when it is detached.

        4. As for removed entity, according to EJB3.0 persist spec 3.2.5, remove call actually remove the entity from context, em.contains(obj) return false. It is not a managed entity and shouldn't be detached for EntityManager.clear() and not suppose to merge it back and be removed from DB in next commit/flush. User suppose to flush it before calling clear().
        Note:
        Although I found openjpa may detach deleted-not-flushed entity as a side effect, we don't need to support merge scenario as I claimed above.

        sm.isNew() && !sm.isDeleted() && !sm.isFlushed() in DetachManager.useDetachedStateManager() also make sure PNewDeletedState and PNewFlushedDeletedState will not be merged and take as a PNEW state and get inserted.

        Show
        Xiaoqin Feng added a comment - Yes, we shouldn't save PCState in pcDetachedState since pcDetachedStaet is used for loaded field, version info to make attach process more efficient. V2 patch is attached. Here is my solution: 1. Our assumption according to EntityManager.clear() API is ""EntityManager.clear() will not flush new, dirty, removed entities in context but just detach all managed entities". At first I thought we need to pass along flush=false from BrokerImpl.detachAll(OpCallbacks call, boolean flush) to DetachManager _flushBeforeDetach as openjpa-591 mentioned. Then I realized in BrokerImpl.detachAllInternal(), new DetachManager(this, true, call).detachAll (new ManagedObjectCollection(states)) actually pass in full=true so _flushBeforeDetach has no effect in this case and flush logic in DetachManager will be skipped. So I reverted my change in V1 patch for this part. 2. To make not flushed new entity to be inserted after merge, we don't save DetachedStateManager in detached object. When it merged back, exisitng openjpa framework could handle it correctly and add it to insert list. 3. To make not flushed dirty entity to be updated after merge, we save dirty flag BitSet when it is detached. 4. As for removed entity, according to EJB3.0 persist spec 3.2.5, remove call actually remove the entity from context, em.contains(obj) return false. It is not a managed entity and shouldn't be detached for EntityManager.clear() and not suppose to merge it back and be removed from DB in next commit/flush. User suppose to flush it before calling clear(). Note: Although I found openjpa may detach deleted-not-flushed entity as a side effect, we don't need to support merge scenario as I claimed above. sm.isNew() && !sm.isDeleted() && !sm.isFlushed() in DetachManager.useDetachedStateManager() also make sure PNewDeletedState and PNewFlushedDeletedState will not be merged and take as a PNEW state and get inserted.
        Hide
        Xiaoqin Feng added a comment -

        Jeremy, Good thoughts about test.
        I am fine with the other proposed solution.
        I am looking forward to look at your solution.

        Show
        Xiaoqin Feng added a comment - Jeremy, Good thoughts about test. I am fine with the other proposed solution. I am looking forward to look at your solution.
        Hide
        Jeremy Bauer added a comment -

        I conversed with a couple of the committers and the consensus was that we should not be storing persistence context-type state in the detached entities. Instead, the detached entity should only manage information about the state of its persistent fields.

        So, when we re-merge entities we should not depend on the "NEW" state in a detached object. The entity could have been inserted into the database outside of the application, in which case we'd fail trying to do an insert (which should not happen on a merge). I think the ideal solution is to re-query for the entity and then perform the insert or update based on whether the entity was or was not found - similar to merging an actual new entity.

        One additional item: The test code has a query immediately following the clear:

        em.clear();
        assertEquals(1, query("select x from Item x "
        + "where x.itemName = 'cup'").
        getResultList().size());

        This essentially loads the returned item in the L1 cache, changing the behavior of the internal merge processing (the corresponding merge finds the item in the L1). While this is a good scenario to test, we should also test the scenario where the item isn't cached before the merge.

        Show
        Jeremy Bauer added a comment - I conversed with a couple of the committers and the consensus was that we should not be storing persistence context-type state in the detached entities. Instead, the detached entity should only manage information about the state of its persistent fields. So, when we re-merge entities we should not depend on the "NEW" state in a detached object. The entity could have been inserted into the database outside of the application, in which case we'd fail trying to do an insert (which should not happen on a merge). I think the ideal solution is to re-query for the entity and then perform the insert or update based on whether the entity was or was not found - similar to merging an actual new entity. One additional item: The test code has a query immediately following the clear: em.clear(); assertEquals(1, query("select x from Item x " + "where x.itemName = 'cup'"). getResultList().size()); This essentially loads the returned item in the L1 cache, changing the behavior of the internal merge processing (the corresponding merge finds the item in the L1). While this is a good scenario to test, we should also test the scenario where the item isn't cached before the merge.
        Hide
        Xiaoqin Feng added a comment -

        Here are some clarifications for the code change. Jeremy, please review it and let me know your opinion.
        I don't have permission to submit to SVN yet. If you have, could you submit tit after it is finalized? Thanks.

        1. Added _flushBeforeDetach flag in Broker for EntityManagerImpl.clear().
        This has same effect as setting openjpa.Compatibility property but it is used internally.
        Added getter and setter in Broker interface.

        2. Save PCState and dirty flags with detached object if it is not flushed.
        See change in DetachManager.getDetachedState(StateManagerImpl, BitSet).
        Before my change, it made state object array one size longer but empty in that positon if isNew.
        Now I put PCState there if it is not flushed.
        I don't know if the previous trick is used somewhere later at attach stage. At least in openjpa, I didn't see it.
        And all openjpa tests passed.

        3. Use saved PCState and dirty flags to restore StateManager and other state during attachment.
        See change in DetachedStateManager.attach().
        For delete case, BrokerImpl.copy(OpenJPAStateManager copy, PCState state) could find StateManager with same OID in cache
        so it doesn't set the new state with detached state.
        I changed StateManagerImpl.setPCState(PCState state) from private to protected so BrokerImpl could set it.
        I think this change is fine but just highlight for review.

        Show
        Xiaoqin Feng added a comment - Here are some clarifications for the code change. Jeremy, please review it and let me know your opinion. I don't have permission to submit to SVN yet. If you have, could you submit tit after it is finalized? Thanks. 1. Added _flushBeforeDetach flag in Broker for EntityManagerImpl.clear(). This has same effect as setting openjpa.Compatibility property but it is used internally. Added getter and setter in Broker interface. 2. Save PCState and dirty flags with detached object if it is not flushed. See change in DetachManager.getDetachedState(StateManagerImpl, BitSet). Before my change, it made state object array one size longer but empty in that positon if isNew. Now I put PCState there if it is not flushed. I don't know if the previous trick is used somewhere later at attach stage. At least in openjpa, I didn't see it. And all openjpa tests passed. 3. Use saved PCState and dirty flags to restore StateManager and other state during attachment. See change in DetachedStateManager.attach(). For delete case, BrokerImpl.copy(OpenJPAStateManager copy, PCState state) could find StateManager with same OID in cache so it doesn't set the new state with detached state. I changed StateManagerImpl.setPCState(PCState state) from private to protected so BrokerImpl could set it. I think this change is fine but just highlight for review.
        Hide
        Xiaoqin Feng added a comment -

        In this case, since the persistence context was cleared, the entity to merge to is not a removed entity, yet the toAttach is marked as delete.
        So I think the about delete test case is still expected.

        Show
        Xiaoqin Feng added a comment - In this case, since the persistence context was cleared, the entity to merge to is not a removed entity, yet the toAttach is marked as delete. So I think the about delete test case is still expected.
        Hide
        Jeremy Bauer added a comment -

        Regarding the delete test. Should we expect the merge operation to delete an entity that was marked as delete and then detached when the persistence context was cleared?

        According to the spec (3.2.4.1 Merging Detached Entity State):

        • If X is a removed entity instance, an IllegalArgumentException will be thrown by the
        merge operation (or the transaction commit will fail).

        Does this still hold true after the em has been cleared without a commit? If not, wouldn't you expect the test query:

        assertEquals(0, query("select x from Item x "
        + "where x.itemName = 'cup'").
        getResultList().size());

        to return one item instead of zero? It seems like we should either merge the entity or throw an exception. My current opinion is that we should throw an exception. What do you think?

        Other opinions?

        Show
        Jeremy Bauer added a comment - Regarding the delete test. Should we expect the merge operation to delete an entity that was marked as delete and then detached when the persistence context was cleared? According to the spec (3.2.4.1 Merging Detached Entity State): • If X is a removed entity instance, an IllegalArgumentException will be thrown by the merge operation (or the transaction commit will fail). Does this still hold true after the em has been cleared without a commit? If not, wouldn't you expect the test query: assertEquals(0, query("select x from Item x " + "where x.itemName = 'cup'"). getResultList().size()); to return one item instead of zero? It seems like we should either merge the entity or throw an exception. My current opinion is that we should throw an exception. What do you think? Other opinions?
        Hide
        Jeremy Bauer added a comment -

        I also got the new and update to work, but ran into the delete problem. After one/both of us gets the delete working, let's compare code and post a common solution and jUnits. I have a few additional tests in the works which use cascade relationships and explicit detachment. I'll post them when they are ready so you can try with your version of the fix.

        Show
        Jeremy Bauer added a comment - I also got the new and update to work, but ran into the delete problem. After one/both of us gets the delete working, let's compare code and post a common solution and jUnits. I have a few additional tests in the works which use cascade relationships and explicit detachment. I'll post them when they are ready so you can try with your version of the fix.
        Hide
        Xiaoqin Feng added a comment -

        Here is the updated tests. I have added new-clear-merge, update-clear-merge and delete-clear-merge tests.
        I think the expected behavior for em.clear() is just detach, it shouldn't flush for all new, updated, delete objects.
        Instead it should keep PCState during detachment and are able to merge it back and flush/commit correctly after that.

        I am also working on this issue. Now still need a bit more work to make delete case work.

        Show
        Xiaoqin Feng added a comment - Here is the updated tests. I have added new-clear-merge, update-clear-merge and delete-clear-merge tests. I think the expected behavior for em.clear() is just detach, it shouldn't flush for all new, updated, delete objects. Instead it should keep PCState during detachment and are able to merge it back and flush/commit correctly after that. I am also working on this issue. Now still need a bit more work to make delete case work.
        Hide
        Xiaoqin Feng added a comment -

        Jeremy, thanks for looking at it.
        Besides, I think according to EntityManager.clear() API, we shouldn't flush both new object and dirty object.
        Do you agree with it?

        Show
        Xiaoqin Feng added a comment - Jeremy, thanks for looking at it. Besides, I think according to EntityManager.clear() API, we shouldn't flush both new object and dirty object. Do you agree with it?
        Hide
        Jeremy Bauer added a comment -

        I was able to reproduce this problem with your test and it does look like a bug. As a workaround, if you explicitly flush before calling clear, the test should pass. As you mention, the state of the entity doesn't get set appropriately on the detach/attach in order for it to get tagged as an insert on the commit. I tried tagging it as "clean, but not yet flushed" in order to trigger an insert, but quickly ran into other state-related issues. I'll post a patch if/when I work through those issues.

        Show
        Jeremy Bauer added a comment - I was able to reproduce this problem with your test and it does look like a bug. As a workaround, if you explicitly flush before calling clear, the test should pass. As you mention, the state of the entity doesn't get set appropriately on the detach/attach in order for it to get tagged as an insert on the commit. I tried tagging it as "clean, but not yet flushed" in order to trigger an insert, but quickly ran into other state-related issues. I'll post a patch if/when I work through those issues.
        Hide
        Xiaoqin Feng added a comment -

        Test case is attached.

        Show
        Xiaoqin Feng added a comment - Test case is attached.

          People

          • Assignee:
            Jeremy Bauer
            Reporter:
            Xiaoqin Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development