OpenJPA
  1. OpenJPA
  2. OPENJPA-396

Cloning Calendar proxies doesn't detach from StateManager

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0, 1.0.1, 1.1.0
    • Fix Version/s: 1.0.1, 1.1.0
    • Component/s: kernel
    • Labels:
      None

      Description

      This problem was first discussed on our dev mailing list: http://www.nabble.com/Cloning-Calendar-proxies-tf4571181.html

      Per the discussion on that thread, I am proposing to modify the generated proxy code to override the clone() method. This clone() method will do the necessary copying of data from the original object, but then also null out the sm (StateManager) and zero out the field attributes. This action detaches the cloned object from the StateManager (and associated EntityManager).

      Instead of limiting this action to the Calendar proxy, I am adding the clone() method implementation to all of our proxy objects that we generate. Granted, some of the object types do not directly support the clone() method, but that will be detected when or if anybody attempts to use the clone() method on these types (compiler generated error message).

      I'll be posting a patch shortly and I plan to commit the changes later today (unless there is opposition).

      Thanks,
      Kevin

        Activity

        Hide
        Kevin Sutter added a comment -

        Proposed patch for OpenJPA-396. It was built against the 1.0.x branch, but also seems to work with the trunk (1.1.0).

        When testing this patch, I found that TestProxyManager had a couple of problems that needed correcting. After these changes were made, I had to re-factor a few of the "assert" methods so that we were testing for proper equality when copying vs cloning. That's why the patch for TestProxyManager looks larger than expected.

        I plan to commit these changes later today unless concerns are raised with the patch.

        Thanks,
        Kevin

        Show
        Kevin Sutter added a comment - Proposed patch for OpenJPA-396. It was built against the 1.0.x branch, but also seems to work with the trunk (1.1.0). When testing this patch, I found that TestProxyManager had a couple of problems that needed correcting. After these changes were made, I had to re-factor a few of the "assert" methods so that we were testing for proper equality when copying vs cloning. That's why the patch for TestProxyManager looks larger than expected. I plan to commit these changes later today unless concerns are raised with the patch. Thanks, Kevin
        Hide
        Craig L Russell added a comment -

        Hi Kevin,

        One question. In the generated clone method, after calling super.clone(), why do you not simply invoke stateManager = null; pcState = 0; instead of calling the setOwner(null, 0) method?

        Seems like there is additional code in setOwner that you want to avoid because there is not yet any relationship between the owner and the sco. The effect of calling this method from the clone might be to disassociate the original sco.

        Show
        Craig L Russell added a comment - Hi Kevin, One question. In the generated clone method, after calling super.clone(), why do you not simply invoke stateManager = null; pcState = 0; instead of calling the setOwner(null, 0) method? Seems like there is additional code in setOwner that you want to avoid because there is not yet any relationship between the owner and the sco. The effect of calling this method from the clone might be to disassociate the original sco.
        Hide
        Kevin Sutter added a comment -

        [ Show » ]
        Craig Russell - 08/Oct/07 10:25 AM Hi Kevin, One question. In the generated clone method, after calling super.clone(), why do you not simply invoke stateManager = null; pcState = 0; instead of calling the setOwner(null, 0) method? Seems like there is additional code in setOwner that you want to avoid because there is not yet any relationship between the owner and the sco. The effect of calling this method from the clone might be to disassociate the original sco.

        Craig,
        The original reason is that I couldn't figure out the proper serp invocations to just set those two fields. Then, I found the setOwner method on the Proxy (also generated code) that did just what I was looking for. So, it was more straight-forward to just call this method than to repeat the same code.

        As far as I can tell, the setOwner has no other side effects. I am calling setOwner on the Proxy, not the StateManager. The code is generated in the ProxyManagerImpl class (addProxyMethods method). And, the javadoc for this method is as follows:

        /**

        • Reset the state of the proxy, and set the owning instance of the
        • proxy and the name of the field it is assigned to. Set to null to
        • indicate that the proxy is no longer managed.
          */
          public void setOwner(OpenJPAStateManager sm, int field);

        Kevin

        Show
        Kevin Sutter added a comment - [ Show » ] Craig Russell - 08/Oct/07 10:25 AM Hi Kevin, One question. In the generated clone method, after calling super.clone(), why do you not simply invoke stateManager = null; pcState = 0; instead of calling the setOwner(null, 0) method? Seems like there is additional code in setOwner that you want to avoid because there is not yet any relationship between the owner and the sco. The effect of calling this method from the clone might be to disassociate the original sco. Craig, The original reason is that I couldn't figure out the proper serp invocations to just set those two fields. Then, I found the setOwner method on the Proxy (also generated code) that did just what I was looking for. So, it was more straight-forward to just call this method than to repeat the same code. As far as I can tell, the setOwner has no other side effects. I am calling setOwner on the Proxy, not the StateManager. The code is generated in the ProxyManagerImpl class (addProxyMethods method). And, the javadoc for this method is as follows: /** Reset the state of the proxy, and set the owning instance of the proxy and the name of the field it is assigned to. Set to null to indicate that the proxy is no longer managed. */ public void setOwner(OpenJPAStateManager sm, int field); Kevin
        Hide
        Craig L Russell added a comment -

        > As far as I can tell, the setOwner has no other side effects.

        I hope someone else can comment on this, as it doesn't seem right. You don't want to <allow> any code in the clone to access the StateManager, since the StateManager knows about the original and can't tell from the APIs which proxy is calling it.

        I think we're asking for trouble, either now or in future if we call this method from the clone.

        Show
        Craig L Russell added a comment - > As far as I can tell, the setOwner has no other side effects. I hope someone else can comment on this, as it doesn't seem right. You don't want to <allow> any code in the clone to access the StateManager, since the StateManager knows about the original and can't tell from the APIs which proxy is calling it. I think we're asking for trouble, either now or in future if we call this method from the clone.
        Hide
        Craig L Russell added a comment -

        One other comment:

        In openjpa-kernel/src/test/java/org/apache/openjpa/util/TestProxyManager.java,

        + private static void assertSortedSetsEquals(SortedSet s1, SortedSet s2) {
        ...
        + assertTrue(s1.equals(s2));

        I'm curious what this is testing, after you verified that the comparators are equivalent, the sizes are the same, and the contents are identical. What else is being tested here, except that the equals method is implemented correctly? Is that it?

        Show
        Craig L Russell added a comment - One other comment: In openjpa-kernel/src/test/java/org/apache/openjpa/util/TestProxyManager.java, + private static void assertSortedSetsEquals(SortedSet s1, SortedSet s2) { ... + assertTrue(s1.equals(s2)); I'm curious what this is testing, after you verified that the comparators are equivalent, the sizes are the same, and the contents are identical. What else is being tested here, except that the equals method is implemented correctly? Is that it?
        Hide
        Kevin Sutter added a comment -

        [ Show » ]
        Craig Russell - 08/Oct/07 12:38 PM One other comment: In openjpa-kernel/src/test/java/org/apache/openjpa/util/TestProxyManager.java, + private static void assertSortedSetsEquals(SortedSet s1, SortedSet s2) { ... + assertTrue(s1.equals(s2)); I'm curious what this is testing, after you verified that the comparators are equivalent, the sizes are the same, and the contents are identical. What else is being tested here, except that the equals method is implemented correctly? Is that it?

        Correct. The original problem surfaced because of a .equals() invocation between two Calendar objects. So, even though the Javadoc explains what attributes of a given Calendar, Date, Time, etc object are examined for .equals(), I just wanted to ensure that we didn't hit this problem again in the future because some JVM decided to change their implementation.

        Show
        Kevin Sutter added a comment - [ Show » ] Craig Russell - 08/Oct/07 12:38 PM One other comment: In openjpa-kernel/src/test/java/org/apache/openjpa/util/TestProxyManager.java, + private static void assertSortedSetsEquals(SortedSet s1, SortedSet s2) { ... + assertTrue(s1.equals(s2)); I'm curious what this is testing, after you verified that the comparators are equivalent, the sizes are the same, and the contents are identical. What else is being tested here, except that the equals method is implemented correctly? Is that it? Correct. The original problem surfaced because of a .equals() invocation between two Calendar objects. So, even though the Javadoc explains what attributes of a given Calendar, Date, Time, etc object are examined for .equals(), I just wanted to ensure that we didn't hit this problem again in the future because some JVM decided to change their implementation.
        Hide
        Kevin Sutter added a comment -

        Resolved via svn revision #582974 for both the 1.0.1 and 1.1.0 branches.

        Show
        Kevin Sutter added a comment - Resolved via svn revision #582974 for both the 1.0.1 and 1.1.0 branches.

          People

          • Assignee:
            Kevin Sutter
            Reporter:
            Kevin Sutter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development