OpenJPA
  1. OpenJPA
  2. OPENJPA-1912

enhancer generates invalid code if fetch-groups is activated

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.0, 2.0.1, 2.1.0
    • Fix Version/s: 2.2.0
    • Component/s: Enhance
    • Labels:
      None

      Description

      If openjpa.DetachState =fetch-groups is used, the enhancer will add a 'implements Externalizable' + writeExternal + readExternal.

      The problem is, that writeExternal and readExternal will also try to externalize the private members of any given superclass. Thus we get a runtime Exception that we are not allowed to access those fields.

      Example:

      @Entity
      public abstract class AbstractGroup

      { ... @Temporal(TemporalType.TIMESTAMP) @TrackChanges private Date applicationBegin; ... }

      and

      @Entity
      public class Group extends AbstractGroup

      { ... }

      will result in the following code (decompiled with jad):

      public void writeExternal(ObjectOutput objectoutput)
      throws IOException
      {
      pcWriteUnmanaged(objectoutput);
      if(pcStateManager != null)

      { if(pcStateManager.writeDetached(objectoutput)) return; }

      else

      { objectoutput.writeObject(pcGetDetachedState()); objectoutput.writeObject(null); }

      objectoutput.writeObject(applicationBegin);
      objectoutput.writeObject(applicationEnd);
      objectoutput.writeObject(applicationLocked);
      objectoutput.writeObject(approvalRequired);
      ...

      1. OPENJPA-1912-test.patch
        11 kB
        Mark Struberg
      2. OPENJPA-1912-mdd.diff.txt
        11 kB
        Michael Dick
      3. OPENJPA-1912-fix-wo_cleanup-2.patch
        16 kB
        Mark Struberg
      4. OPENJPA-1912-fix-wo_cleanup.patch
        5 kB
        Mark Struberg
      5. OPENJPA-1912-enhancer.patch
        2 kB
        Mark Struberg

        Issue Links

          Activity

          Hide
          Mark Struberg added a comment -

          my naive question first: why do we need Externalizable at all? In any other case a simple Serializable works well.

          Show
          Mark Struberg added a comment - my naive question first: why do we need Externalizable at all? In any other case a simple Serializable works well.
          Hide
          Mark Struberg added a comment -

          It seems this has nothing to do with fetch-groups, but will always be generated if DetachedStateField=true gets used.

          My configuration currently: <property name="openjpa.DetachState" value="loaded(DetachedStateField=true)"/>

          Show
          Mark Struberg added a comment - It seems this has nothing to do with fetch-groups, but will always be generated if DetachedStateField=true gets used. My configuration currently: <property name="openjpa.DetachState" value="loaded(DetachedStateField=true)"/>
          Hide
          Mark Struberg added a comment -

          This unit test demontstrates the problem. I get the following output:

          java.lang.IllegalAccessError: tried to access field org.apache.openjpa.enhance.EnhancedSuperClass.id from class org.apache.openjpa.enhance.EnhancedSubClass
          at org.apache.openjpa.enhance.EnhancedSubClass.writeExternal(EnhancedSubClass.java)
          at java.io.ObjectOutputStream.writeExternalData(ObjectOutputStream.java:1421)
          at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1390)
          at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1150)
          at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:326)
          at org.apache.openjpa.enhance.TestClassHierarchyEnhancement.serializeObject(TestClassHierarchyEnhancement.java:58)
          at org.apache.openjpa.enhance.TestClassHierarchyEnhancement.testSerialize(TestClassHierarchyEnhancement.java:50)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at junit.framework.TestCase.runTest(TestCase.java:154)
          at org.apache.openjpa.persistence.test.AbstractPersistenceTestCase.runTest(AbstractPersistenceTestCase.java:573)

          Show
          Mark Struberg added a comment - This unit test demontstrates the problem. I get the following output: java.lang.IllegalAccessError: tried to access field org.apache.openjpa.enhance.EnhancedSuperClass.id from class org.apache.openjpa.enhance.EnhancedSubClass at org.apache.openjpa.enhance.EnhancedSubClass.writeExternal(EnhancedSubClass.java) at java.io.ObjectOutputStream.writeExternalData(ObjectOutputStream.java:1421) at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1390) at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1150) at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:326) at org.apache.openjpa.enhance.TestClassHierarchyEnhancement.serializeObject(TestClassHierarchyEnhancement.java:58) at org.apache.openjpa.enhance.TestClassHierarchyEnhancement.testSerialize(TestClassHierarchyEnhancement.java:50) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:154) at org.apache.openjpa.persistence.test.AbstractPersistenceTestCase.runTest(AbstractPersistenceTestCase.java:573)
          Hide
          Mark Struberg added a comment -

          oki, starting with that stuff now.

          current classes just generate a

          readExternal() and writeExternal which first externalizes some OpenJPA specific fields like the pcStateManager, et al and then comes the single fields.

          For providing something like super.readYourOwnStuff() we need to split the fields from the rest.

          So I'll start with introducing 2 new methods readExternalFields() and writeExternalFields() which might then invoke the super.readExternalFields().

          Any objections or tips?

          Show
          Mark Struberg added a comment - oki, starting with that stuff now. current classes just generate a readExternal() and writeExternal which first externalizes some OpenJPA specific fields like the pcStateManager, et al and then comes the single fields. For providing something like super.readYourOwnStuff() we need to split the fields from the rest. So I'll start with introducing 2 new methods readExternalFields() and writeExternalFields() which might then invoke the super.readExternalFields(). Any objections or tips?
          Hide
          Rick Curtis added a comment -

          > Any objections or tips?
          Goodluck... Serp is a fun one!

          I'll try to get some time tomorrow to take a peek at this one.

          Show
          Rick Curtis added a comment - > Any objections or tips? Goodluck... Serp is a fun one! I'll try to get some time tomorrow to take a peek at this one.
          Hide
          Mark Struberg added a comment -

          nah, I already fixed bytecode issues in javassist and did lots of the bc stuff for OpenWebBeans - so yes, it is tricky, but I guess I can make it

          Show
          Mark Struberg added a comment - nah, I already fixed bytecode issues in javassist and did lots of the bc stuff for OpenWebBeans - so yes, it is tricky, but I guess I can make it
          Hide
          Mark Struberg added a comment -

          got it roughly working, but I'm not sure about whether the pcWriteUnmanaged also should get executed on the superclass?

          Show
          Mark Struberg added a comment - got it roughly working, but I'm not sure about whether the pcWriteUnmanaged also should get executed on the superclass?
          Hide
          Mark Struberg added a comment -

          Hi!

          This patch fixes the issue but still needs a cleanup (finally removing old unused code and stuff).

          Please review! It runs fine with a few test cases and I'll test it in my real world project tomorrow morning.

          good night,
          strub

          Show
          Mark Struberg added a comment - Hi! This patch fixes the issue but still needs a cleanup (finally removing old unused code and stuff). Please review! It runs fine with a few test cases and I'll test it in my real world project tomorrow morning. good night, strub
          Hide
          Mark Struberg added a comment -

          figured that I still have a few bugs with deserialisation. Currently investigating...

          Show
          Mark Struberg added a comment - figured that I still have a few bugs with deserialisation. Currently investigating...
          Hide
          Mark Struberg added a comment -

          found the problem. StateManagerImpl#writeDetached writes the fields of the superclass first, and only then the fields of the subclasses.

          Show
          Mark Struberg added a comment - found the problem. StateManagerImpl#writeDetached writes the fields of the superclass first, and only then the fields of the subclasses.
          Hide
          Mark Struberg added a comment -

          this patch now de-externalises in the correct order. I also added a few tests for it

          Show
          Mark Struberg added a comment - this patch now de-externalises in the correct order. I also added a few tests for it
          Hide
          Mark Struberg added a comment -

          oops, I forgot to also attach the enhancer parts for making the test work.

          sorry :/ (have way too many files dirty already ...)

          Show
          Mark Struberg added a comment - oops, I forgot to also attach the enhancer parts for making the test work. sorry :/ (have way too many files dirty already ...)
          Hide
          Mark Struberg added a comment -

          Rick, Mike, did you find a chance to test this patch already? I'm slowly running out of control about all my patches... Since the patches depend on each other to some degree, I cannot really continue anymore. If you don't find the time to work on it then please ping me. In this case I'll continue maintaining my patches via a fork of the github mirror rather than juggling svn patches (which are hell to apply...).

          txs and LieGrue,
          strub

          Show
          Mark Struberg added a comment - Rick, Mike, did you find a chance to test this patch already? I'm slowly running out of control about all my patches... Since the patches depend on each other to some degree, I cannot really continue anymore. If you don't find the time to work on it then please ping me. In this case I'll continue maintaining my patches via a fork of the github mirror rather than juggling svn patches (which are hell to apply...). txs and LieGrue, strub
          Hide
          Michael Dick added a comment -

          Mark, haven't had a chance to look yet. Will take a closer look tonight and at least get you an ETA.

          Show
          Michael Dick added a comment - Mark, haven't had a chance to look yet. Will take a closer look tonight and at least get you an ETA.
          Hide
          Michael Dick added a comment -

          I see what you mean about patch management. I'm not sure I've applied the patches correctly. Is there a specific order, or could you make one all inclusive patch?

          Show
          Michael Dick added a comment - I see what you mean about patch management. I'm not sure I've applied the patches correctly. Is there a specific order, or could you make one all inclusive patch?
          Hide
          Michael Dick added a comment -

          I think this is a unified patch - check that it matches yours.

          If so the patch looks good. I don't know serp well enough for errors to jump out at me, but the generated bytecode looks correct.

          There's some cleanup to be done (e.g. we generally don't use @author tags), but assuming none of the unit tests break I think we can commit this patch.

          Show
          Michael Dick added a comment - I think this is a unified patch - check that it matches yours. If so the patch looks good. I don't know serp well enough for errors to jump out at me, but the generated bytecode looks correct. There's some cleanup to be done (e.g. we generally don't use @author tags), but assuming none of the unit tests break I think we can commit this patch.
          Hide
          Mark Struberg added a comment -

          Txs Mike!
          I'll try to apply it and run a full suite now.

          Show
          Mark Struberg added a comment - Txs Mike! I'll try to apply it and run a full suite now.
          Hide
          Mark Struberg added a comment -

          patch looks fine so far but /org/apache/openjpa/enhance/persistence1.xml is missing. Please see my enhancer.patch for this part.

          A few notes:
          1.) I use //X to comment out 'temporaryily'. This means either a TODO or it needs to be clarified.
          2.) The patch assumes that all parent classes must belong to the same persistence unit and therefore also contains the generated writeExternalFields and readExternalFields methods.Is this assumption true, or are there situations where parent.readExternalFields() is invalid?

          Show
          Mark Struberg added a comment - patch looks fine so far but /org/apache/openjpa/enhance/persistence1.xml is missing. Please see my enhancer.patch for this part. A few notes: 1.) I use //X to comment out 'temporaryily'. This means either a TODO or it needs to be clarified. 2.) The patch assumes that all parent classes must belong to the same persistence unit and therefore also contains the generated writeExternalFields and readExternalFields methods.Is this assumption true, or are there situations where parent.readExternalFields() is invalid?
          Hide
          Mark Struberg added a comment - - edited

          oops comment should have gone to OPENJPA-1933

          Show
          Mark Struberg added a comment - - edited oops comment should have gone to OPENJPA-1933
          Hide
          Mark Struberg added a comment -

          I found a (pretty uncommon but theoretically possible) situation where this might happen. If a superclass is defined in a jar which didn't got rebuilt and the subclass entity got enhanced via a new version of JPA. Do we take care about such pathological situations? I mean it would have crashed with the old system anyway...

          Show
          Mark Struberg added a comment - I found a (pretty uncommon but theoretically possible) situation where this might happen. If a superclass is defined in a jar which didn't got rebuilt and the subclass entity got enhanced via a new version of JPA. Do we take care about such pathological situations? I mean it would have crashed with the old system anyway...
          Hide
          Michael Dick added a comment -

          It would be nice to handle such a case gracefully. In such an environment I think there will be other problems. Rick added some code in OPENJPA-1707 to detect downlevel entities when the enhancer runs, but I don't remember the exact problem he saw.

          Show
          Michael Dick added a comment - It would be nice to handle such a case gracefully. In such an environment I think there will be other problems. Rick added some code in OPENJPA-1707 to detect downlevel entities when the enhancer runs, but I don't remember the exact problem he saw.
          Hide
          Mark Struberg added a comment -

          If OPENJPA-1707 is already implemented then it should also work for this tweak if we increment the PCEnhancer.ENHANCER_VERSION, isn't ?

          Show
          Mark Struberg added a comment - If OPENJPA-1707 is already implemented then it should also work for this tweak if we increment the PCEnhancer.ENHANCER_VERSION, isn't ?
          Hide
          Michael Dick added a comment -

          You're right. I thought it only checked when the PCEnhancer was executed, but after looking at the code it's the MetaDataRepository that triggers the check.

          Show
          Michael Dick added a comment - You're right. I thought it only checked when the PCEnhancer was executed, but after looking at the code it's the MetaDataRepository that triggers the check.
          Hide
          Yves Langisch added a comment -

          Is there any reason why this changeset is not backported to the other branches (1.2.x, 2.0.x)?

          Show
          Yves Langisch added a comment - Is there any reason why this changeset is not backported to the other branches (1.2.x, 2.0.x)?
          Hide
          ASF subversion and git services added a comment -

          Commit 1483996 from hthomann
          [ https://svn.apache.org/r1483996 ]

          OPENJPA-1912: Generate externalizable methods correctly for super and subclasses - back ported to 2.1.x Mark Struberg's trunk changes.

          Show
          ASF subversion and git services added a comment - Commit 1483996 from hthomann [ https://svn.apache.org/r1483996 ] OPENJPA-1912 : Generate externalizable methods correctly for super and subclasses - back ported to 2.1.x Mark Struberg's trunk changes.
          Hide
          ASF subversion and git services added a comment -

          Commit 1484028 from hthomann
          [ https://svn.apache.org/r1484028 ]

          OPENJPA-1912: Generate externalizable methods correctly for super and subclasses - back ported to 2.0.x Mark Struberg's trunk changes.

          Show
          ASF subversion and git services added a comment - Commit 1484028 from hthomann [ https://svn.apache.org/r1484028 ] OPENJPA-1912 : Generate externalizable methods correctly for super and subclasses - back ported to 2.0.x Mark Struberg's trunk changes.

            People

            • Assignee:
              Michael Dick
              Reporter:
              Mark Struberg
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development