OpenJPA
  1. OpenJPA
  2. OPENJPA-1562

EntityManager:Refresh on Removed entity does not trigger IllegalArgumentException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta2
    • Fix Version/s: 2.0.0-beta3
    • Component/s: None
    • Labels:
      None
    • Environment:
      Mac OS X, OpenJPA 2.0.0-beta2, Spring 3.0

      Description

      My application unit tests turned up some unexpected behavior from basic OpenJPA entity manager functions. It appears, for instance, that a detached entity can still be refreshed. I decided to write a few basic tests (attached) which report five potential errors. This does not go into the testing of cascading behavior which would require additional work.

      1. patch.txt
        6 kB
        Dianne Richards
      2. openjpa1562.tar.zip
        7 kB
        Jerry Carter
      3. simple_entity_tests.txt
        9 kB
        Jerry Carter

        Activity

        Jerry Carter created issue -
        Hide
        Jerry Carter added a comment -

        Here are the test cases and their output.

        Show
        Jerry Carter added a comment - Here are the test cases and their output.
        Jerry Carter made changes -
        Field Original Value New Value
        Attachment simple_entity_tests.txt [ 12438524 ]
        Dianne Richards made changes -
        Assignee Dianne Richards [ dianner ]
        Hide
        Jerry Carter added a comment -

        Basic unit test showing the failure. Note that there is a parallel email thread in which Fay Wang could not reproduce the issue. <http://n2.nabble.com/Refreshing-detached-entities-tp4717312p4718095.html>

        Show
        Jerry Carter added a comment - Basic unit test showing the failure. Note that there is a parallel email thread in which Fay Wang could not reproduce the issue. < http://n2.nabble.com/Refreshing-detached-entities-tp4717312p4718095.html >
        Jerry Carter made changes -
        Attachment openjpa1562.tar.zip [ 12438530 ]
        Hide
        Donald Woods added a comment -

        Which Spec level did you specify in your persistence.xml and what properties did you set?
        A JPA 1.0 level persistence.xml will automatically use compatibility settings to behave like OpenJPA 1.x -
        http://openjpa.apache.org/builds/latest/docs/manual/ref_guide_remote.html#ref_guide_detach

        Show
        Donald Woods added a comment - Which Spec level did you specify in your persistence.xml and what properties did you set? A JPA 1.0 level persistence.xml will automatically use compatibility settings to behave like OpenJPA 1.x - http://openjpa.apache.org/builds/latest/docs/manual/ref_guide_remote.html#ref_guide_detach
        Hide
        Donald Woods added a comment -

        Add post 2.0.0-beta2, the fix for OPENJPA-1097 has been integrated, which fixes a bug where EM.clear() was not removing some $proxy class wrappers from the entities....

        Show
        Donald Woods added a comment - Add post 2.0.0-beta2, the fix for OPENJPA-1097 has been integrated, which fixes a bug where EM.clear() was not removing some $proxy class wrappers from the entities....
        Jerry Carter made changes -
        Attachment openjpa1562.tar.zip [ 12438530 ]
        Hide
        Jerry Carter added a comment -

        Donald: interesting suggestions. My persistence.xml file is pulling in the 2.0 namespace and specifies version 2.0. I am now running against the OpenJPA-2.0.0.SNAPSHOT.jar and the behavior is unchanged.

        Show
        Jerry Carter added a comment - Donald: interesting suggestions. My persistence.xml file is pulling in the 2.0 namespace and specifies version 2.0. I am now running against the OpenJPA-2.0.0.SNAPSHOT.jar and the behavior is unchanged.
        Jerry Carter made changes -
        Attachment openjpa1562.tar.zip [ 12438534 ]
        Hide
        Jerry Carter added a comment - - edited

        Donald: I think you are on the right track. At DEBUG level, I see 'PersistenceVersion=1.0' during OpenJPA initialization. Now to figure out why.

        Show
        Jerry Carter added a comment - - edited Donald: I think you are on the right track. At DEBUG level, I see 'PersistenceVersion=1.0' during OpenJPA initialization. Now to figure out why.
        Hide
        Jerry Carter added a comment -

        Spring 3.0 is reporting the wrong version (1.0) for the PersistenceVersion. <http://jira.springframework.org/browse/SPR-6975>

        Show
        Jerry Carter added a comment - Spring 3.0 is reporting the wrong version (1.0) for the PersistenceVersion. < http://jira.springframework.org/browse/SPR-6975 >
        Jerry Carter made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Invalid [ 6 ]
        Hide
        Jerry Carter added a comment -

        Considerable progress. Now only one of the twenty tests fails.

        TestEntity refreshRemoved = new TestEntity("refresh removed");
        em.persist(refreshRemoved);
        em.flush();
        em.remove(refreshRemoved);
        em.flush();
        em.refresh(refreshRemoved);

        This should throw an IllegalArgumentException, but does not.

        Show
        Jerry Carter added a comment - Considerable progress. Now only one of the twenty tests fails. TestEntity refreshRemoved = new TestEntity("refresh removed"); em.persist(refreshRemoved); em.flush(); em.remove(refreshRemoved); em.flush(); em.refresh(refreshRemoved); This should throw an IllegalArgumentException, but does not.
        Jerry Carter made changes -
        Resolution Invalid [ 6 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Dianne Richards added a comment -

        I have reproduced this latest problem with remove/refresh and am looking into it.

        Show
        Dianne Richards added a comment - I have reproduced this latest problem with remove/refresh and am looking into it.
        Hide
        Jerry Carter added a comment -

        Revised the bug description to reflect the narrowed scope and changed the state to minor as there are clear workarounds.

        Show
        Jerry Carter added a comment - Revised the bug description to reflect the narrowed scope and changed the state to minor as there are clear workarounds.
        Jerry Carter made changes -
        Summary Basic negative tests of the EntityManager fail EntityManager:Refresh on Removed entity does not trigger IllegalArgumentException
        Priority Major [ 3 ] Minor [ 4 ]
        Hide
        Dianne Richards added a comment -

        Attached patch for this problem.

        Show
        Dianne Richards added a comment - Attached patch for this problem.
        Dianne Richards made changes -
        Attachment patch.txt [ 12438840 ]
        Hide
        Michael Dick added a comment -

        Thanks for the patch Dianne, but I have a couple of questions.

        1. The javadoc for lock() and refresh() is very similar regarding detached entities :

        For lock() :
        @throws IllegalArgumentException if the instance is not an

        entity or is a detached entity

        For refresh() :
        @throws IllegalArgumentException if the instance is not

        an entity or the entity is not managed

        Seems like we'd want to the same thing for refresh and lock (basically the if check for REFRESH can be removed).

        2. This is more of a general question - your patch didn't introduce it but the assertValidAttchedEntity() method is very similar to contains(). There is a subtle difference in that contains checks whether the type is managed by this persistence context, and assertValidAttachedEntity checks whether the StateManager is persistent. There's also a slight difference in the exceptions thrown. Still it seems like we'd want to reuse the logic, and rather than duplicating the code.

        Show
        Michael Dick added a comment - Thanks for the patch Dianne, but I have a couple of questions. 1. The javadoc for lock() and refresh() is very similar regarding detached entities : For lock() : @throws IllegalArgumentException if the instance is not an entity or is a detached entity For refresh() : @throws IllegalArgumentException if the instance is not an entity or the entity is not managed Seems like we'd want to the same thing for refresh and lock (basically the if check for REFRESH can be removed). 2. This is more of a general question - your patch didn't introduce it but the assertValidAttchedEntity() method is very similar to contains(). There is a subtle difference in that contains checks whether the type is managed by this persistence context, and assertValidAttachedEntity checks whether the StateManager is persistent. There's also a slight difference in the exceptions thrown. Still it seems like we'd want to reuse the logic, and rather than duplicating the code.
        Hide
        Dianne Richards added a comment -

        Thanks for reviewing this patch Mike.

        For question #1:

        When I first made the change and ran all of the tests, one set of lock tests failed because of remove followed by lock.

        So, I searched through the spec related to this. Section 3.2.5 says, for a refresh operation, "If X is a new, detched, or removed entity, the IllegalArgumentException is thrown." In other parts of the spec (including the javadoc that you quoted), it says an IllegalArgumentException is thrown for a Lock request on a "detached" entity. But, I couldn't find any such statement fot a "removed" entity.

        For question #2

        At this point, I don't have an answer. I'll do a little digging.

        Show
        Dianne Richards added a comment - Thanks for reviewing this patch Mike. For question #1: When I first made the change and ran all of the tests, one set of lock tests failed because of remove followed by lock. So, I searched through the spec related to this. Section 3.2.5 says, for a refresh operation, "If X is a new, detched, or removed entity, the IllegalArgumentException is thrown." In other parts of the spec (including the javadoc that you quoted), it says an IllegalArgumentException is thrown for a Lock request on a "detached" entity. But, I couldn't find any such statement fot a "removed" entity. For question #2 At this point, I don't have an answer. I'll do a little digging.
        Hide
        Michael Dick added a comment -

        Hi Dianne, you're right per the spec a managed entity is one that has a persistent identity and is associated with the persistence context. A removed entity fits that description.

        I still think we can be a bit smarted about code reuse, but that's secondary to this issue - consider both my questions answered

        Show
        Michael Dick added a comment - Hi Dianne, you're right per the spec a managed entity is one that has a persistent identity and is associated with the persistence context. A removed entity fits that description. I still think we can be a bit smarted about code reuse, but that's secondary to this issue - consider both my questions answered
        Hide
        Jeremy Bauer added a comment -

        Committed patch.txt dated 2010-03-15 04:44 PM for Dianne under revision 923849.

        Show
        Jeremy Bauer added a comment - Committed patch.txt dated 2010-03-15 04:44 PM for Dianne under revision 923849.
        Hide
        Jerry Carter added a comment -

        The patch works for me. I tested with beta3 build from the staging area.

        Show
        Jerry Carter added a comment - The patch works for me. I tested with beta3 build from the staging area.
        Donald Woods made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 2.0.0-beta3 [ 12314857 ]
        Resolution Fixed [ 1 ]
        Hide
        Michael Dick added a comment -

        Closing on Jerry's behalf.

        Show
        Michael Dick added a comment - Closing on Jerry's behalf.
        Michael Dick made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        8h 41m 1 Jerry Carter 12/Mar/10 02:34
        Resolved Resolved Reopened Reopened
        1h 31m 1 Jerry Carter 12/Mar/10 04:05
        Reopened Reopened Resolved Resolved
        12d 13h 28m 1 Donald Woods 24/Mar/10 17:34
        Resolved Resolved Closed Closed
        10h 10m 1 Michael Dick 25/Mar/10 03:44

          People

          • Assignee:
            Dianne Richards
            Reporter:
            Jerry Carter
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development