OpenJPA
  1. OpenJPA
  2. OPENJPA-956 OpenJPA 2.0 iteration 5 primary task
  3. OPENJPA-885

Support clear methods on EntityManager, including new CascadeType.CLEAR

    Details

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

      Description

      Support the 2.0 new EntityManager clear() methods. Also support the new CascadeType.CLEAR.

      1. patch.txt
        17 kB
        Dianne Richards
      2. CompatibilityProblemPatch.txt
        7 kB
        Dianne Richards
      3. detachTestPatch.txt
        47 kB
        Dianne Richards
      4. detachCodePatch.txt
        15 kB
        Dianne Richards
      5. cascadeTestPatch.txt
        8 kB
        Dianne Richards
      6. DetachStateCascadeTestPatch.txt
        20 kB
        Dianne Richards
      7. DetachStateCascadeTestPatch2.txt
        20 kB
        Dianne Richards

        Activity

        Hide
        Dianne Richards added a comment -

        This patch provides the clear(Entity) operation specified in the JPA 2.0 spec. It implements some new internal detach method to provide the no-flush and no-copy behaviors.

        Show
        Dianne Richards added a comment - This patch provides the clear(Entity) operation specified in the JPA 2.0 spec. It implements some new internal detach method to provide the no-flush and no-copy behaviors.
        Hide
        Dianne Richards added a comment -

        On the previous comment, with the patch, I forgot to mention that this patch does not include the cascade support. That will be provided later.

        Show
        Dianne Richards added a comment - On the previous comment, with the patch, I forgot to mention that this patch does not include the cascade support. That will be provided later.
        Hide
        Pinaki Poddar added a comment -

        I will suggest to look at Compatibility.setFlushBeforeDetach(boolean).
        Appropriate setting of this flag may itself be sufficient to address the needs for JPA 2.0 clear() methods which dictates no flushing before detach() while OpenJPA, by default, behaves otherwise.

        If you decide to go by setFlushBeforeDetach(boolean) route, then consider setting it in appropriate ProductDerivation (possibly with new versioning capability of OpenJPAConfiguration.getSpecification() to ensure backward compatibility).

        Show
        Pinaki Poddar added a comment - I will suggest to look at Compatibility.setFlushBeforeDetach(boolean). Appropriate setting of this flag may itself be sufficient to address the needs for JPA 2.0 clear() methods which dictates no flushing before detach() while OpenJPA, by default, behaves otherwise. If you decide to go by setFlushBeforeDetach(boolean) route, then consider setting it in appropriate ProductDerivation (possibly with new versioning capability of OpenJPAConfiguration.getSpecification() to ensure backward compatibility).
        Hide
        Dianne Richards added a comment -

        The clear(Object) method will use an existing Broker detach method. There are several differences in behavior between those required for the clear method and the current detach method. As a result, the default 2.0 behavior will change. However, compatibility options will be provided to revert to the 1.x behaviors.

        The first difference has to do with flush. The current behavior is to flush before detaching. However, the JPA 2.0 spec specifically says that the current data will not be persisted. So, this will be the default. The using application can use the Compatibility setFlushBeforeDetach() method to change this behavior.

        With the current detach methods, copies are made of the entities and returned. Does anybody know why this copy is made? It's not necessary for the new clear method since it has a void return. We'd like to change the default behavior for the current detach methods to return the original enties with a state change. Does anybody know of any impacts this might have? Again, there will be a compatibility option to change this in special circumstances.

        With the current detach methods, the entities still remain in the persistent context. However, for the new clear method, the specified entities must be removed. I assume the impact will be in performance. As with the previous 2 differences, a compatibility option will be provided.

        The last difference deals with cascading. The current detach behavior does a complete cascade. The clear method requires the cascade to be explicitely specified. Does anyone see a need for providing a compatibility option for this also?

        Show
        Dianne Richards added a comment - The clear(Object) method will use an existing Broker detach method. There are several differences in behavior between those required for the clear method and the current detach method. As a result, the default 2.0 behavior will change. However, compatibility options will be provided to revert to the 1.x behaviors. The first difference has to do with flush. The current behavior is to flush before detaching. However, the JPA 2.0 spec specifically says that the current data will not be persisted. So, this will be the default. The using application can use the Compatibility setFlushBeforeDetach() method to change this behavior. With the current detach methods, copies are made of the entities and returned. Does anybody know why this copy is made? It's not necessary for the new clear method since it has a void return. We'd like to change the default behavior for the current detach methods to return the original enties with a state change. Does anybody know of any impacts this might have? Again, there will be a compatibility option to change this in special circumstances. With the current detach methods, the entities still remain in the persistent context. However, for the new clear method, the specified entities must be removed. I assume the impact will be in performance. As with the previous 2 differences, a compatibility option will be provided. The last difference deals with cascading. The current detach behavior does a complete cascade. The clear method requires the cascade to be explicitely specified. Does anyone see a need for providing a compatibility option for this also?
        Hide
        Jeremy Bauer added a comment -

        Regarding your question about a full cascading detach, in order to allow backward compatibility and reuse as much of the existing code path as possible, a compatibility option sounds like the way to go. That would entail enhancing the existing detach logic to do a path-driven cascade & making that the default behavior when in a JPA 2 environment. If not in a JPA 2 environment, perform a full cascade by default. In either environment, allow the default to be overridden via a compatibility flag.

        Show
        Jeremy Bauer added a comment - Regarding your question about a full cascading detach, in order to allow backward compatibility and reuse as much of the existing code path as possible, a compatibility option sounds like the way to go. That would entail enhancing the existing detach logic to do a path-driven cascade & making that the default behavior when in a JPA 2 environment. If not in a JPA 2 environment, perform a full cascade by default. In either environment, allow the default to be overridden via a compatibility flag.
        Hide
        Dianne Richards added a comment -

        In testing the new function in this JIRA, I've run into a problem that I could use some input for. First, let me explain what I'm doing. There are 4 compatibility options (identified in a previous post). We need to change the default values for openjpa only, without affecting other derived products. So, I've put code in the PersistentProductDerivation to retrieve the Compatibility instance and set the new default values for openjpa.

        In my unit test, I initially changed these individual values in my test code, by retrieving the Compatibility instance and using the appropriate setter method, and things worked fine. But, when I specified a change using a property, ex: openjpa.Compatibility=default(flushBeforeDetach=false), the other 3 default values reverted to the original values, not the ones that I set in the PPD.

        Here's what I discovered through running the debugger: THe OpenJpaConfigurationImpl.fromProperties() is called to store the new properties. this calls PluginValue.setString(). This has the following code:

        if (_singleton)
        set(null, true)

        which effectively changes the _value from the current Compatibility object to null. Thus, when the next getCompatibilityInstance() method is called, a new object is instantiated, with the original default values. Is this a bug? Are there other properties that this could happen to in the future? ANy suggestions on how to fix this without breaking things?

        Show
        Dianne Richards added a comment - In testing the new function in this JIRA, I've run into a problem that I could use some input for. First, let me explain what I'm doing. There are 4 compatibility options (identified in a previous post). We need to change the default values for openjpa only, without affecting other derived products. So, I've put code in the PersistentProductDerivation to retrieve the Compatibility instance and set the new default values for openjpa. In my unit test, I initially changed these individual values in my test code, by retrieving the Compatibility instance and using the appropriate setter method, and things worked fine. But, when I specified a change using a property, ex: openjpa.Compatibility=default(flushBeforeDetach=false), the other 3 default values reverted to the original values, not the ones that I set in the PPD. Here's what I discovered through running the debugger: THe OpenJpaConfigurationImpl.fromProperties() is called to store the new properties. this calls PluginValue.setString(). This has the following code: if (_singleton) set(null, true) which effectively changes the _value from the current Compatibility object to null. Thus, when the next getCompatibilityInstance() method is called, a new object is instantiated, with the original default values. Is this a bug? Are there other properties that this could happen to in the future? ANy suggestions on how to fix this without breaking things?
        Hide
        Dianne Richards added a comment -

        One option that Jeremy suggested is to check the providers for updates when Compatibility is instantiated. Is this a vaible option? Is there an order specified for the providers?

        Show
        Dianne Richards added a comment - One option that Jeremy suggested is to check the providers for updates when Compatibility is instantiated. Is this a vaible option? Is there an order specified for the providers?
        Hide
        Pinaki Poddar added a comment -

        > when I specified a change using a property, ex: openjpa.Compatibility=default(flushBeforeDetach=false),

        When is 'when' exactly? After an EntityManagerFactory is obtained? After an EntityManager is created?

        Compatibility is singleton and non-dynamic. Changing its value later (say after em is created) itself should be prohibited and if that is not happening that is of concern.

        Can you post some test case (and configuration artifacts) that demonstrates the use case?

        > when Compatibility is instantiated.
        Plugin instantiation is lazy and on-demand – unless you overwrite with openjpa.InitializeEagerly.

        > Is there an order specified for the providers?
        I am assuming from the context 'providers' imply ProductDerivation. If that is the case then yes. Follow the trails of ProductDerivation.getType().

        Show
        Pinaki Poddar added a comment - > when I specified a change using a property, ex: openjpa.Compatibility=default(flushBeforeDetach=false), When is 'when' exactly? After an EntityManagerFactory is obtained? After an EntityManager is created? Compatibility is singleton and non-dynamic. Changing its value later (say after em is created) itself should be prohibited and if that is not happening that is of concern. Can you post some test case (and configuration artifacts) that demonstrates the use case? > when Compatibility is instantiated. Plugin instantiation is lazy and on-demand – unless you overwrite with openjpa.InitializeEagerly. > Is there an order specified for the providers? I am assuming from the context 'providers' imply ProductDerivation. If that is the case then yes. Follow the trails of ProductDerivation.getType().
        Hide
        Dianne Richards added a comment -

        I'm attaching the CompatibilityProblemPatch file. This contains 2 test cases. The first one, TestCompatibilityChanges, show that things are working fine. The secondOne, TestCompatibilityProblem, shows the problem. In the second one, I've added a property change to the setUp call. This apparently ends up passing the property in on the createEntityManagerFactory().

        Also in this patch are 2 code changes. The Compatibility class has several new instance variables with associated setter/getter methods. The PersistenceProductDerivation class has calls to the setter methods to change the default values for openjpa.

        So, to repeat what is happening, the PPD changes are working correctly initially, as seen in the TestCompatibilityChanges test case. But, when a change to Compatibility is specified on the createEMF(), these changes are lost because the entire Compatibility object is lost when the _value variable is set to null. I realize the this is a singleton. However, this property seems to be a special case because iterative changes can be made to it.

        Show
        Dianne Richards added a comment - I'm attaching the CompatibilityProblemPatch file. This contains 2 test cases. The first one, TestCompatibilityChanges, show that things are working fine. The secondOne, TestCompatibilityProblem, shows the problem. In the second one, I've added a property change to the setUp call. This apparently ends up passing the property in on the createEntityManagerFactory(). Also in this patch are 2 code changes. The Compatibility class has several new instance variables with associated setter/getter methods. The PersistenceProductDerivation class has calls to the setter methods to change the default values for openjpa. So, to repeat what is happening, the PPD changes are working correctly initially, as seen in the TestCompatibilityChanges test case. But, when a change to Compatibility is specified on the createEMF(), these changes are lost because the entire Compatibility object is lost when the _value variable is set to null. I realize the this is a singleton. However, this property seems to be a special case because iterative changes can be made to it.
        Hide
        Dianne Richards added a comment -

        I'm attaching both a code patch and test patch, which should be committed together. Documentation updates will be included in a future patch.

        This code provides support for the clear(Object) method in the JPA 2.0 spec draft. In order to provide the correct semantics, some default behaviors of the detach methods was changed. We no longer flush before detach, nor copy the detached object, nor automatically cascade. (Cascade is based on the cascade annotation by default.) Also, to comply with the spec, the object is now removed from the persistent context.

        In order to revert to the previous detach behavior, the openjpa.Compatibility property may be changed by setting the following attributes:
        flushBeforeDetach=true
        copyDetachedObjects=true (will also keep the object in the persistent context)
        cascadeWithDetach=true

        Show
        Dianne Richards added a comment - I'm attaching both a code patch and test patch, which should be committed together. Documentation updates will be included in a future patch. This code provides support for the clear(Object) method in the JPA 2.0 spec draft. In order to provide the correct semantics, some default behaviors of the detach methods was changed. We no longer flush before detach, nor copy the detached object, nor automatically cascade. (Cascade is based on the cascade annotation by default.) Also, to comply with the spec, the object is now removed from the persistent context. In order to revert to the previous detach behavior, the openjpa.Compatibility property may be changed by setting the following attributes: flushBeforeDetach=true copyDetachedObjects=true (will also keep the object in the persistent context) cascadeWithDetach=true
        Hide
        Jeremy Bauer added a comment -

        I've reviewed detachCodePatch.txt and detachTestPatch.txt dated 16/Mar/09 07:16 AM. Nice work. The changes look good to me. Unless someone has issues with any of the changes, I'll commit them later today.

        Some comments:

        • Don't forget to provide documentation updates
        • Tests that use cascade defined in xml - for both cascade clear and all need to be provided. I can partially see why they weren't provided (or even obvious), since the current orm.xml doesn't have the cascade-clear element defined. I'll mention the missing element to Kevin so he can help make sure it gets included in a future revision of the 2.0 spec. I recommend creating a new JIRA for the future work (using cascade-clear defined in orm.xml & corresponding updates in the XML parser) so this work item can be resolved.
        Show
        Jeremy Bauer added a comment - I've reviewed detachCodePatch.txt and detachTestPatch.txt dated 16/Mar/09 07:16 AM. Nice work. The changes look good to me. Unless someone has issues with any of the changes, I'll commit them later today. Some comments: Don't forget to provide documentation updates Tests that use cascade defined in xml - for both cascade clear and all need to be provided. I can partially see why they weren't provided (or even obvious), since the current orm.xml doesn't have the cascade-clear element defined. I'll mention the missing element to Kevin so he can help make sure it gets included in a future revision of the 2.0 spec. I recommend creating a new JIRA for the future work (using cascade-clear defined in orm.xml & corresponding updates in the XML parser) so this work item can be resolved.
        Hide
        Jeremy Bauer added a comment -

        Committed detachTestPatch.txt and detachCodePatch.txt (with updates) for Dianne under revisions 755468, 755469, and 755470. I used the existing compatibility option name "CopyOnDetach" because a) the code comments were more extensive, b) there is existing documentation that makes reference to this option and c) it is seemed more appropriate. I'm open to using "CopyDetachedObjects" if there is a strong argument for using this name vs. the other.

        Show
        Jeremy Bauer added a comment - Committed detachTestPatch.txt and detachCodePatch.txt (with updates) for Dianne under revisions 755468, 755469, and 755470. I used the existing compatibility option name "CopyOnDetach" because a) the code comments were more extensive, b) there is existing documentation that makes reference to this option and c) it is seemed more appropriate. I'm open to using "CopyDetachedObjects" if there is a strong argument for using this name vs. the other.
        Hide
        Pinaki Poddar added a comment -

        /**

        • Whether openjpa will always cascade on detach, regardless of the
        • cascade setting.
        • @return true if cascade will always occur, false if cascade will only
        • occur if it is specified in metadata
        • @since 2.0.0
          */
          public boolean getCascadeWithDetach() { return _cascadeWithDetach; }

        1. What does this new option mean?
        2. What will a detached graph of x will look like when this option is set to true? Or false? What will be the difference?
        3. How does this option differ from three existing option of 'loaded', 'fetch-group' and 'all'?
        4. Why is this option in Compatibility? What previous behavior that has now changed so that user can fall back upon to access old behavior?

        Show
        Pinaki Poddar added a comment - /** Whether openjpa will always cascade on detach, regardless of the cascade setting. @return true if cascade will always occur, false if cascade will only occur if it is specified in metadata @since 2.0.0 */ public boolean getCascadeWithDetach() { return _cascadeWithDetach; } 1. What does this new option mean? 2. What will a detached graph of x will look like when this option is set to true? Or false? What will be the difference? 3. How does this option differ from three existing option of 'loaded', 'fetch-group' and 'all'? 4. Why is this option in Compatibility? What previous behavior that has now changed so that user can fall back upon to access old behavior?
        Hide
        Dianne Richards added a comment -

        I think there are 2 issues here:
        1 - whether or not to the cascade compatibility option is needed
        2 - whether or not the options loaded, fetch-group, and all are handled

        Although these may be interrelated, let's discuss them individually at first.

        For the cascade compatibility option question, we first have to agree on what the old behavior does. I have created a simple test case for this, with the new patch cascadeTestPatch.txt. The cascade relationship of the entities is:
        Entity1 to Entity2 - ALL
        Entity2 to Entity3 - PERSIST only

        When you run this test on an openjpa version without my changes, you'll see that all entities are detached when detach(e1) is run. So, it automatically cascades. (Note: I know this is with all entities loaded. Again, I'll address that later.)

        Second, what would the spec expect in this scenario with the new clear(Object) option. Here's what the spec says in section 3.2.6:

        The semantics of the clear operation, applied to an entity X are as follows:
        •If X is a managed entity, the clear operation causes it to become detached. The clear operation is cascaded to entities referenced by X, if the relationships from X to these other entities is annotated with the cascade=CLEAR or cascade=ALL annotation element value. Entities which previously referenced X will continue to reference X.
        •If X is a new or detached entity, it is ignored by the clear operation.
        •If X is a removed entity, the clear operation is cascaded to entities referenced by X, if the relationships
        from X to these other entities is annotated with the cascade=CLEAR or cascade=
        ALL annotation element value. Entities which previously referenced X will continue to reference X. Portable applications should not pass removed entities

        It seems to me that bullet 1 implies that the cascade should ONLY occur if cascade=CLEAR or or cascade=ALL. Therefore, in my test case, Entity3 should NOT be detached. So, that needs to be the default behavior. In order to provide backward compatibility for the pre-existing detach behavior, we need to provide the cascadeWithDetach compatibility option.

        In order to address item 2 above, I need to write some more test cases. (Admittedly I should have done that.) I'll address that in a subsequent post.

        Show
        Dianne Richards added a comment - I think there are 2 issues here: 1 - whether or not to the cascade compatibility option is needed 2 - whether or not the options loaded, fetch-group, and all are handled Although these may be interrelated, let's discuss them individually at first. For the cascade compatibility option question, we first have to agree on what the old behavior does. I have created a simple test case for this, with the new patch cascadeTestPatch.txt. The cascade relationship of the entities is: Entity1 to Entity2 - ALL Entity2 to Entity3 - PERSIST only When you run this test on an openjpa version without my changes, you'll see that all entities are detached when detach(e1) is run. So, it automatically cascades. (Note: I know this is with all entities loaded. Again, I'll address that later.) Second, what would the spec expect in this scenario with the new clear(Object) option. Here's what the spec says in section 3.2.6: The semantics of the clear operation, applied to an entity X are as follows: •If X is a managed entity, the clear operation causes it to become detached. The clear operation is cascaded to entities referenced by X, if the relationships from X to these other entities is annotated with the cascade=CLEAR or cascade=ALL annotation element value. Entities which previously referenced X will continue to reference X. •If X is a new or detached entity, it is ignored by the clear operation. •If X is a removed entity, the clear operation is cascaded to entities referenced by X, if the relationships from X to these other entities is annotated with the cascade=CLEAR or cascade= ALL annotation element value. Entities which previously referenced X will continue to reference X. Portable applications should not pass removed entities It seems to me that bullet 1 implies that the cascade should ONLY occur if cascade=CLEAR or or cascade=ALL. Therefore, in my test case, Entity3 should NOT be detached. So, that needs to be the default behavior. In order to provide backward compatibility for the pre-existing detach behavior, we need to provide the cascadeWithDetach compatibility option. In order to address item 2 above, I need to write some more test cases. (Admittedly I should have done that.) I'll address that in a subsequent post.
        Hide
        Dianne Richards added a comment -

        For the DetachState options, I think there needs to be a spec clarification in section 3.2.6 (Evicting an Entity Instance from the Persistence Context) regarding what to do about cascading and unloaded fields and associations. After several of us have looked at the spec, we feel that section 3.2.7 (Detached Entities) implies that we should not load unloaded fields during detach(). We will be asking for spec clarification on this. In the meantime, we're proceeding with our assumption.

        Thanks to Pinaki for the following illustration of this assumption:

        Consider an entity y that is reachable from x, the root detached entity. What happens to y when we call detach?
        Two determining factors :
        is y loaded?
        is y reachable via DEATCH cascade?

        Let us call

        {D(x)} is the set of entities that are reachable from x after detach call. That is to say if I now serialize x and take that serialized object x' to another remote tier the paths I can access from serialized x' is designated by {D(x)}

        .

        And here are the choices for y.
        ------------------------------------------------------------------------------------------------------------------
        Loaded? DEATCH cascade? Effect of detach on y
        -------------------------------------------------------------------------------------------------------------------
        1. yes yes – y is in

        {D(x)} and y is removed from persistence context
        2. yes no – y is in {D(x)}

        but y is not removed from the persistence context (i.e. this is the impact of DEATCH cascade and OpenJPA did not have that feature)
        3. no yes – y is not in

        {D(x)}

        and y was never in persistence context (i.e. detach does not load additional state even when DEATCH cascade is specified)
        4. no no – same as (3) above
        --------------------------------------------------------------------------------------------------------------------

        Now, to address the DetachState options. During the DetachManager preDetach() method, the appropriate fields/associations are loaded based on the DetachState option. Then, the detach processing occurs for those for the loaded associations where the DETACH cascade is specified. The attached patch, DetacheStateCascadeTestPatch.txt, tests these options and their relationship to the table above.

        Show
        Dianne Richards added a comment - For the DetachState options, I think there needs to be a spec clarification in section 3.2.6 (Evicting an Entity Instance from the Persistence Context) regarding what to do about cascading and unloaded fields and associations. After several of us have looked at the spec, we feel that section 3.2.7 (Detached Entities) implies that we should not load unloaded fields during detach(). We will be asking for spec clarification on this. In the meantime, we're proceeding with our assumption. Thanks to Pinaki for the following illustration of this assumption: Consider an entity y that is reachable from x, the root detached entity. What happens to y when we call detach ? Two determining factors : is y loaded? is y reachable via DEATCH cascade? Let us call {D(x)} is the set of entities that are reachable from x after detach call. That is to say if I now serialize x and take that serialized object x' to another remote tier the paths I can access from serialized x' is designated by {D(x)} . And here are the choices for y. ------------------------------------------------------------------------------------------------------------------ Loaded? DEATCH cascade? Effect of detach on y ------------------------------------------------------------------------------------------------------------------- 1. yes yes – y is in {D(x)} and y is removed from persistence context 2. yes no – y is in {D(x)} but y is not removed from the persistence context (i.e. this is the impact of DEATCH cascade and OpenJPA did not have that feature) 3. no yes – y is not in {D(x)} and y was never in persistence context (i.e. detach does not load additional state even when DEATCH cascade is specified) 4. no no – same as (3) above -------------------------------------------------------------------------------------------------------------------- Now, to address the DetachState options. During the DetachManager preDetach() method, the appropriate fields/associations are loaded based on the DetachState option. Then, the detach processing occurs for those for the loaded associations where the DETACH cascade is specified. The attached patch, DetacheStateCascadeTestPatch.txt, tests these options and their relationship to the table above.
        Hide
        Dianne Richards added a comment -

        Attaching an updated patch, DetachStateCascadeTestPatch2.txt to handle some minor problems running from within TeamCity

        Show
        Dianne Richards added a comment - Attaching an updated patch, DetachStateCascadeTestPatch2.txt to handle some minor problems running from within TeamCity
        Hide
        Jeremy Bauer added a comment -

        Committed DetachStateCascadeTestPatch2.txt for Dianne under revision 758442.

        Show
        Jeremy Bauer added a comment - Committed DetachStateCascadeTestPatch2.txt for Dianne under revision 758442.
        Hide
        Dianne Richards added a comment -

        Doc changes will be provided under OPENJPA-1027

        Show
        Dianne Richards added a comment - Doc changes will be provided under OPENJPA-1027

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development