OpenJPA
  1. OpenJPA
  2. OPENJPA-968

Change in default detach() behavior for JPA 2.0

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-M2
    • Fix Version/s: 2.0.0-M3
    • Component/s: kernel
    • Labels:
      None

      Description

      JPA 2.0 specification has introduced a detach() method in EntityManager. OpenJPA already has a detach() method prior to JPA 2.0. There are several major differences between the new method introduced in JPA 2.0 spec versus existing OpenJPA detach method.

      1. The method signatures are different
      JPA 2.0: void detach(Object entity);
      OpenJPA 1.x : <T> T detach(T entity);

      2. This signature difference also points to a basic difference in behavior. OpenJPA detach() creates a copy of the given input entity and returns it. While JPA 2.0 specifies in-place detach and hence returns a void.

      3. The other basic difference is OpenJPA detach does not remove the input entity from the persistence context. While JPA 2.0 specifies that the input entity be removed from the context.

      4. OpenJPA detach flushes a dirty instance before detaching. This implicit flush behavior can be configured but flushing is the default.

      5. OpenJPA detach() provides several options on which related instances will become detached. They are 'loaded', 'fetch-group' and 'all'. With 'loaded' being the default.
      Whereas JPA 2.0 introduces a DEATCH cascade type and specified that the relationships that are cascaded with DETACH or ALL be traversed during detachment.
      It is not clear from the spec, however,
      a) whether an unloaded relation which has DETACH cascade will get loaded as a side-effect of detach().
      b) whether a relation that is currently loaded but not cascaded for DETACH will be included in the detached graph.
      If answer to (a) is no and answer to (b) is yes, (which the spec does seems to suggest) then we can effectively continue with 'loaded' as the default behavior.

      To accommodate these differences following actions are proposed:

      A1. Request JPA Spec committee to change the API method in JPA 2.0 to OpenJPA detach() method signature. If that request is not met, then change OpenJPA API according to JPA 2.0 spec. It will break backward compatibility of OpenJPAEntityManager API. There seems to be no way out.
      A2. The default behavior will change in the following way
      a) the detach will be in-place
      b) no implict flush of dirty instances
      c) based on condition, how JPA 2.0 spec clarifies the DETACH cascade and loaded fields for inclusion in the detached graph, change 'loaded' default to 'cascade' default.

      1. problemPatch.txt
        2 kB
        Dianne Richards

        Issue Links

          Activity

          Pinaki Poddar created issue -
          Hide
          Craig L Russell added a comment -

          I'd suggest that OpenJPA implement both detach() according to the JPA rules and rename the original OpenJPA detach() to detachCopy(). Preserve the original rules for detachCopy and for detach, follow whatever rules the JPA expert group decides on.

          We can do this incompatible change with a major version, so it's unfortunate but not a disaster. Users who fail to read the release notes will discover that their applications don't compile and don't run, so it's not a silent failure.

          Note that in JDO, the explicit detach method is named detachCopy, which has the same semantics as OpenJPA detach. The decision was made to make it explicit that the detachCopy method creates a copy of the instance in the context and leaves the original instance intact.

          By the way, there was a lot of discussion in the JDO expert group about whether explicit detach should make a copy or not, and it was pretty clear at the end that detach in place is tricky, if not dangerous.

          Show
          Craig L Russell added a comment - I'd suggest that OpenJPA implement both detach() according to the JPA rules and rename the original OpenJPA detach() to detachCopy(). Preserve the original rules for detachCopy and for detach, follow whatever rules the JPA expert group decides on. We can do this incompatible change with a major version, so it's unfortunate but not a disaster. Users who fail to read the release notes will discover that their applications don't compile and don't run, so it's not a silent failure. Note that in JDO, the explicit detach method is named detachCopy, which has the same semantics as OpenJPA detach. The decision was made to make it explicit that the detachCopy method creates a copy of the instance in the context and leaves the original instance intact. By the way, there was a lot of discussion in the JDO expert group about whether explicit detach should make a copy or not, and it was pretty clear at the end that detach in place is tricky, if not dangerous.
          Hide
          Dianne Richards added a comment -

          Pinaki - I think I've discovered a problem with one of the changes you made, in the ValueMetaDataImpl, with the new getCascadeDetach() method. With the way you did it, the result returns CASCADE_IMMEDIATE for all simple types (non-Entities). This doesn't make sense and is also not done for the other cascade types (persist, attach, etc.). I've updated your test case with some print statements to confirm this. I'm including an updated ValueMetaDataImpl and TestFlush in this patch. I copied the additional code in getCascadeDetach() from getCascadeAttach(). But, there are also differences in the other getCascade.... methods, so I'm not entirely sure I'm doing the right thing. So, please look at this and comment.

          Show
          Dianne Richards added a comment - Pinaki - I think I've discovered a problem with one of the changes you made, in the ValueMetaDataImpl, with the new getCascadeDetach() method. With the way you did it, the result returns CASCADE_IMMEDIATE for all simple types (non-Entities). This doesn't make sense and is also not done for the other cascade types (persist, attach, etc.). I've updated your test case with some print statements to confirm this. I'm including an updated ValueMetaDataImpl and TestFlush in this patch. I copied the additional code in getCascadeDetach() from getCascadeAttach(). But, there are also differences in the other getCascade.... methods, so I'm not entirely sure I'm doing the right thing. So, please look at this and comment.
          Dianne Richards made changes -
          Field Original Value New Value
          Attachment problemPatch.txt [ 12402060 ]
          Donald Woods made changes -
          Fix Version/s 2.0.0 [ 12314019 ]
          Fix Version/s 2.0.0-M2 [ 12313483 ]
          Fix Version/s 2.0.0-M1 [ 12313624 ]
          Pinaki Poddar made changes -
          Affects Version/s 2.0.0 [ 12314019 ]
          Affects Version/s 2.0.0-M2 [ 12313483 ]
          Affects Version/s 2.0.0-M1 [ 12313624 ]
          Pinaki Poddar made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Fix Version/s 2.0.0-M3 [ 12314148 ]
          Fix Version/s 2.0.0-M4 [ 12314149 ]
          Donald Woods made changes -
          Fix Version/s 2.0.0 [ 12314019 ]
          Fix Version/s 2.0.0-M4 [ 12314149 ]
          Affects Version/s 2.0.0-M2 [ 12313483 ]
          Affects Version/s 2.0.0 [ 12314019 ]
          Donald Woods made changes -
          Link This issue relates to OPENJPA-1097 [ OPENJPA-1097 ]

            People

            • Assignee:
              Pinaki Poddar
              Reporter:
              Pinaki Poddar
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development