OpenJPA
  1. OpenJPA
  2. OPENJPA-446

Problem when setting String fields of detached objects

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.1
    • Fix Version/s: 1.0.2, 1.1.0
    • Component/s: jpa, kernel
    • Labels:
      None
    • Environment:
      OpenJPA 1.0.1
      Java 1.5.0_13
      MySQL Server 5.0
      MySQL Connector Java 5.0.6

      Description

      I would like to report some strange behavior with detach and merge. To me it looks like a bug.

      I'm trying implement the following strategy:

      persistence context A:
      1. get persistent object
      2. detach the object
      no persistence context:
      3. modify the (detached) object
      persistence context B:
      4. attach (merge) the object

      This is quite simple and straight forward. It generally works except in one situation. When committing the merge() (step 4) an "optimistic locking error" is is thrown under the following condition: (step 3) a (String) field is set to the same text that it already contains but using a different String instance. In other words, when the following expressions are true:

      newString.equals(oldString)
      newString != oldString

      I have written some test code that reproduces this effect (see Attachment).

      The tests are using the same code except for one line:

      Test 1 sets the String to a different one than the object contains:
      record.setContent("a text different than the one in the record");

      Test 2 sets the String to the same instance the object contains:
      record.setContent(record.getContent());

      Test 3 sets the String to the same text but as a different String instance:
      record.setContent(record.getContent()+"");

      This is the result (output of the test run):
      ----------------------------------
      Test 1: SUCCESS
      Test 2: SUCCESS
      Test 3: FAILED (Optimistic locking errors were detected when flushing
      to the data store. The following objects may have been
      concurrently modified in another transaction:
      [test.Record-1])
      ----------------------------------

      While doing some debugging I noticed two things:

      1. When setting the value:

      Class: org.apache.openjpa.kernel.DetachedStateManager
      Line: 555
      Method: settingStringField()

      if (cur == next || !_loaded.get(idx))
      return;

      Here the old and the new value (String) is compared with the == operator.
      The expression can be false with the same text (but different instances). I find this interesting as it matches exacly the problematic condition. I think an .equals() would fix the issue I am having. But I suppose there is something going wrong at another place as well.

      2. Here is the point where execution splits into different ways when calling commit().

      Class: org.apache.openjpa.jdbc.kernel.AbstractUpdateManager
      Line: 151
      Method: populateRowManager()

      } else if ((dirty = ImplHelper.getUpdateFields(sm)) != null) {

      In the tests that succeed "ImplHelper.getUpdateFields(sm)" will return a BitSet. So the condition is true.
      In the tests that fail "ImplHelper.getUpdateFields(sm)" will return null. So the condition is false.

      Note: the problem persist with OpenJPA 1.1.0-SNAPSHOT. When run without enhancing the Record class, all tests will succeed though (printing this message „INFO [main] openjpa.Enhance - Creating subclass for "[class test.Record]". This means that your application will be less efficient and will consume more memory than it would if you ran the OpenJPA enhancer. Additionally, lazy loading will not be available for one-to-one and many-to-one persistent attributes in types using field access; they will be loaded eagerly instead.")

      Regards
      Jonas

      1. detach-attach-fix.patch
        0.7 kB
        Jonas Petersen
      2. detach-attach-fix-proper.patch
        0.7 kB
        Jonas Petersen
      3. DetachAttachTest.zip
        2 kB
        Jonas Petersen
      4. DetachAttachTest-updated.zip
        3 kB
        Jonas Petersen

        Activity

        Hide
        Jonas Petersen added a comment -

        Sourcecode to reproduce the issue.

        Show
        Jonas Petersen added a comment - Sourcecode to reproduce the issue.
        Hide
        Jonas Petersen added a comment -

        I have fixed the DetachedStateManager so it correctly compares the strings (as described in "1. When setting the value:").

        I have rebuilt OpenJPA and ran all maven tests successfully.

        My DetachAttachTest runs all successfull as well now.

        I'm attaching a patch.

        Show
        Jonas Petersen added a comment - I have fixed the DetachedStateManager so it correctly compares the strings (as described in "1. When setting the value:"). I have rebuilt OpenJPA and ran all maven tests successfully. My DetachAttachTest runs all successfull as well now. I'm attaching a patch.
        Hide
        Jonas Petersen added a comment -

        I'm sorry, the patch that I posted (detach-attach-fix.patch) is bad. I'm posting a proper one: detach-attach-fix-proper.patch.

        The problem was that it did not consider a String field set to null before setting it to another value.

        Maybe a TestCase checking all the different situations would be a good idea.

        Show
        Jonas Petersen added a comment - I'm sorry, the patch that I posted (detach-attach-fix.patch) is bad. I'm posting a proper one: detach-attach-fix-proper.patch. The problem was that it did not consider a String field set to null before setting it to another value. Maybe a TestCase checking all the different situations would be a good idea.
        Hide
        Michael Dick added a comment -

        The attached patch looks good, thanks Jonas!

        Show
        Michael Dick added a comment - The attached patch looks good, thanks Jonas!
        Hide
        Patrick Linskey added a comment -

        Thanks for the debugging work on this.

        Is it ok for us to put the DetachAttachTest.zip test files into OpenJPA under the ASL license?

        Show
        Patrick Linskey added a comment - Thanks for the debugging work on this. Is it ok for us to put the DetachAttachTest.zip test files into OpenJPA under the ASL license?
        Hide
        Jonas Petersen added a comment -

        Thanks for reviewing. It's fine for me if you put the DetachAttachTest.zip test files into OpenJPA under the ASL license. You may want to use the extended one that I just attached instead though. It also tests the null conditions and does some assertion for each test.

        Show
        Jonas Petersen added a comment - Thanks for reviewing. It's fine for me if you put the DetachAttachTest.zip test files into OpenJPA under the ASL license. You may want to use the extended one that I just attached instead though. It also tests the null conditions and does some assertion for each test.
        Hide
        Simon Droscher added a comment -

        Any reason why the same change shouldn't be made to the settingObjectField() method of DetachedStateManager ?

        Show
        Simon Droscher added a comment - Any reason why the same change shouldn't be made to the settingObjectField() method of DetachedStateManager ?
        Hide
        Patrick Linskey added a comment -

        I think that this patch should be applied for any non-primitive setting*Field() methods.

        Show
        Patrick Linskey added a comment - I think that this patch should be applied for any non-primitive setting*Field() methods.
        Hide
        Nick Smith added a comment -

        Can someone definately confirm that this has been fixed in v1.0.2? I'm experiencing the same issue but am unable to upgrade to v1.1.0 due to an incompatability with BEA Kodo.

        Nick

        Show
        Nick Smith added a comment - Can someone definately confirm that this has been fixed in v1.0.2? I'm experiencing the same issue but am unable to upgrade to v1.1.0 due to an incompatability with BEA Kodo. Nick
        Hide
        Kevin Sutter added a comment -

        Nick,
        Two things you can look at. One is the "fix versions". On this JIRA Issue, the 1.0.2 version is listed as being a "fix version". The other thing you can look at is the specific SVN Commits. The changes were put into the 1.0.x release via svn revision #596737. The 1.0.2 release was tagged at revision #628944. So, yes, this change has been put into the 1.0.2 release.

        Kevin

        Show
        Kevin Sutter added a comment - Nick, Two things you can look at. One is the "fix versions". On this JIRA Issue, the 1.0.2 version is listed as being a "fix version". The other thing you can look at is the specific SVN Commits. The changes were put into the 1.0.x release via svn revision #596737. The 1.0.2 release was tagged at revision #628944. So, yes, this change has been put into the 1.0.2 release. Kevin

          People

          • Assignee:
            Unassigned
            Reporter:
            Jonas Petersen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development