Derby
  1. Derby
  2. DERBY-790

SQLException used by the networked interface to Derby is not serializable

    Details

      Description

      When running RMI client tests with Derby, many tests failed with the following message:

      Caused by: java.rmi.UnmarshalException: Failed to marshal error response:
      'org.apache.derby.client.am.SqlException: 'DROP TABLE' cannot be performed on
      'SDF014B7' because it does not exist.' because exception ; nested exception
      is:
      java.io.NotSerializableException:
      org.apache.derby.client.net.NetSqlca
      at weblogic.rjvm.ResponseImpl.unmarshalReturn(ResponseImpl.java:191)
      at
      weblogic.rmi.internal.BasicRemoteRef.invoke(BasicRemoteRef.java:176)

      This issue is a blocking issue for us.

      1. Serialize.java
        2 kB
        Knut Anders Hatlen
      2. DERBY-790-v5.stat
        0.3 kB
        Francois Orsini
      3. DERBY-790-v5.diff
        12 kB
        Francois Orsini
      4. DERBY-790-v3.stat
        0.3 kB
        Francois Orsini
      5. DERBY-790-v3.diff
        11 kB
        Francois Orsini
      6. DERBY-790-v2.stat
        0.3 kB
        Francois Orsini
      7. DERBY-790-v2.diff
        9 kB
        Francois Orsini
      8. DERBY-790-v1.stat
        0.2 kB
        Francois Orsini
      9. DERBY-790-v1.diff
        6 kB
        Francois Orsini
      10. derby-790-10.2.diff
        12 kB
        Knut Anders Hatlen

        Activity

        Hide
        Francois Orsini added a comment -

        Neither of org.apache.derby.client.am.Sqlca (which NetSqlca extends) and org.apache.derby.client.net.NetSqlca are serializable - Since org.apache.derby.client.am.Sqlca is a member of org.apache.derby.client.am.SqlException, during serialization, the java.io.NotSerializableException gets raised...

        Show
        Francois Orsini added a comment - Neither of org.apache.derby.client.am.Sqlca (which NetSqlca extends) and org.apache.derby.client.net.NetSqlca are serializable - Since org.apache.derby.client.am.Sqlca is a member of org.apache.derby.client.am.SqlException, during serialization, the java.io.NotSerializableException gets raised...
        Hide
        Kathey Marsden added a comment -

        I was wondering if there was an ETA for the fix for this issue.

        Show
        Kathey Marsden added a comment - I was wondering if there was an ETA for the fix for this issue.
        Hide
        Andrew McIntyre added a comment -

        Francois, you are currently assigned to this issue. Do you think you will have time to complete it for 10.2?

        Show
        Andrew McIntyre added a comment - Francois, you are currently assigned to this issue. Do you think you will have time to complete it for 10.2?
        Hide
        Kathey Marsden added a comment -

        I think this would be a good 10.2 fix candidate but did not change it to 10.2 because it is already assigned. Francois, I was wondering if you were actively working on this issue? If not it might be good to unassign yourself so that someone else can pick it up.

        I think it is a JDBC compliance issue and while it does have only one vote, we have had users asking about it on the list.

        Show
        Kathey Marsden added a comment - I think this would be a good 10.2 fix candidate but did not change it to 10.2 because it is already assigned. Francois, I was wondering if you were actively working on this issue? If not it might be good to unassign yourself so that someone else can pick it up. I think it is a JDBC compliance issue and while it does have only one vote, we have had users asking about it on the list.
        Hide
        Francois Orsini added a comment -

        Sorry for missing the queries earlier - missed them in my emails somehow.

        I have a fix for this issue. I have added some tests in SqlExceptionTest.java as well.

        I'm going to re-verify the fix and post some patch.

        Show
        Francois Orsini added a comment - Sorry for missing the queries earlier - missed them in my emails somehow. I have a fix for this issue. I have added some tests in SqlExceptionTest.java as well. I'm going to re-verify the fix and post some patch.
        Hide
        Knut Anders Hatlen added a comment -

        Hi Francois,
        Did your re-verification show any problems? Do you think the fix will make it into 10.2?

        Show
        Knut Anders Hatlen added a comment - Hi Francois, Did your re-verification show any problems? Do you think the fix will make it into 10.2?
        Hide
        Francois Orsini added a comment -

        > Did your re-verification show any problems?

        I had expanded the current SqlExceptionTest.java but realized the added logic was not proper in order to verify the fix is working. I added a test to SqlExceptionTest.java which is aimed at running in a true client/server environment and this is what I need to verify to ensure the fix is tested in a proper manner.

        > Do you think the fix will make it into 10.2?

        Yes.

        Show
        Francois Orsini added a comment - > Did your re-verification show any problems? I had expanded the current SqlExceptionTest.java but realized the added logic was not proper in order to verify the fix is working. I added a test to SqlExceptionTest.java which is aimed at running in a true client/server environment and this is what I need to verify to ensure the fix is tested in a proper manner. > Do you think the fix will make it into 10.2? Yes.
        Hide
        Rick Hillegas added a comment -

        Moving to 10.2.2.0.

        Show
        Rick Hillegas added a comment - Moving to 10.2.2.0.
        Hide
        Francois Orsini added a comment -

        I have attached some patch to address DERBY-790.

        I have added a new test as part of derbynet/SqlExceptionTest.java (junit test) and made sure it ran fine under the derbynet and derbynetclient frameworks - I have also made sure that the issue reported by DERBY-790 is indeed fixed by the change reflected in - This fix is much simpler than it originally was and thanks to the changes into client/am/SqlException.java which recursively create a SQLException object out of a SqlException one...The risk of this fix is minimal.

        Basically, I have made Sqlca SqlException attribute a transient one as it is mostly and only required when the SQLException gets created through recursion before returning it to the client - Sqlca does not need to be serialized, once the SQLException has been constructed out of getSQLException() in SqlException.java; hence a client can go ahead and serialize it.

        The cause of the NotSeriazableException was due to the fact that we stick the SqlException object in the init 'cause' of the SQLException (Throwable), and during serialization, the exception would be raised against Sqlca.

        Cheers. --francois

        Show
        Francois Orsini added a comment - I have attached some patch to address DERBY-790 . I have added a new test as part of derbynet/SqlExceptionTest.java (junit test) and made sure it ran fine under the derbynet and derbynetclient frameworks - I have also made sure that the issue reported by DERBY-790 is indeed fixed by the change reflected in - This fix is much simpler than it originally was and thanks to the changes into client/am/SqlException.java which recursively create a SQLException object out of a SqlException one...The risk of this fix is minimal. Basically, I have made Sqlca SqlException attribute a transient one as it is mostly and only required when the SQLException gets created through recursion before returning it to the client - Sqlca does not need to be serialized, once the SQLException has been constructed out of getSQLException() in SqlException.java; hence a client can go ahead and serialize it. The cause of the NotSeriazableException was due to the fact that we stick the SqlException object in the init 'cause' of the SQLException (Throwable), and during serialization, the exception would be raised against Sqlca. Cheers. --francois
        Hide
        Knut Anders Hatlen added a comment -

        Hi Francois,

        I think the patch might be a little too simple. When sqlca_ is made
        transient (and thereby null after deserialization), getMessage() could
        make the message text grow. For instance, I tried to insert two rows
        with identical primary keys into a table with addBatch() and
        executeBatch(). When I extracted the SqlException from the
        BatchUpdateException, serialized it and deserialized it, getMessage()
        on the deserialized exception returned

        Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL060927031919790' defined on 'T'.

        Calling getMessage() a second time gave

        Error for batch element #1: Error for batch element #1: Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL060927031919790' defined on 'T'.

        Each time getMessage() is called, "Error for batch element #1: " is
        added to the message string.

        A couple of minor comments/suggestions to the JUnit test:

        • testSerializedException() should call fail() after
          executeQuery(). Otherwise, the test succeeds if the query
          succeeds.
        • recreateSQLException() could throw Exception instead of catching
          all exceptions and calling fail(). This would make the code
          simpler, and it would preserve stack trace and nested exceptions
          if an unexpected error occurred.
        • maybe assertThrowableEquals() could be placed in BaseTestCase
          instead of BaseJDBCTestCase? It is not tied to JDBC as far as I
          can see.

        Thanks.

        Show
        Knut Anders Hatlen added a comment - Hi Francois, I think the patch might be a little too simple. When sqlca_ is made transient (and thereby null after deserialization), getMessage() could make the message text grow. For instance, I tried to insert two rows with identical primary keys into a table with addBatch() and executeBatch(). When I extracted the SqlException from the BatchUpdateException, serialized it and deserialized it, getMessage() on the deserialized exception returned Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL060927031919790' defined on 'T'. Calling getMessage() a second time gave Error for batch element #1: Error for batch element #1: Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL060927031919790' defined on 'T'. Each time getMessage() is called, "Error for batch element #1: " is added to the message string. A couple of minor comments/suggestions to the JUnit test: testSerializedException() should call fail() after executeQuery(). Otherwise, the test succeeds if the query succeeds. recreateSQLException() could throw Exception instead of catching all exceptions and calling fail(). This would make the code simpler, and it would preserve stack trace and nested exceptions if an unexpected error occurred. maybe assertThrowableEquals() could be placed in BaseTestCase instead of BaseJDBCTestCase? It is not tied to JDBC as far as I can see. Thanks.
        Hide
        Francois Orsini added a comment -

        Knut,

        Thanks for the review on this patch.

        I've incorporated all the suggestions and increased the amount of test logic as far as comparing the original exception with the serialized one.

        As part of the test logic in SqlExceptionTest.java, I'm now also using a batch to execute several batch elements and hence generate a few chained / nested exceptions.

        I've tested under 1.3, 1.4, 1.5 & 1.6 Java runtime environments as well as under DerbyNetClient and DerbyNet frameworks as well as well as no framework.

        I have tested and called getMessage() several times for each of the serialized exceptions generated from the BatchUpdateException and I'm not getting the behavior you described - I get the exact same message than the original exception with no added string. The comparison of both the original and the serialized exceptions is thoroughly tested in BaseJDBCTestCase.java:assertSQLExceptionEquals() and that includes any chained exceptions.

        Hence, I'm not sure how you got into the case you described and I would appreciate if you could post the logic for the test you ran as well as your java and platform environment.

        In the meantime, I have attached the new changes.

        Thanks,

        Show
        Francois Orsini added a comment - Knut, Thanks for the review on this patch. I've incorporated all the suggestions and increased the amount of test logic as far as comparing the original exception with the serialized one. As part of the test logic in SqlExceptionTest.java, I'm now also using a batch to execute several batch elements and hence generate a few chained / nested exceptions. I've tested under 1.3, 1.4, 1.5 & 1.6 Java runtime environments as well as under DerbyNetClient and DerbyNet frameworks as well as well as no framework. I have tested and called getMessage() several times for each of the serialized exceptions generated from the BatchUpdateException and I'm not getting the behavior you described - I get the exact same message than the original exception with no added string. The comparison of both the original and the serialized exceptions is thoroughly tested in BaseJDBCTestCase.java:assertSQLExceptionEquals() and that includes any chained exceptions. Hence, I'm not sure how you got into the case you described and I would appreciate if you could post the logic for the test you ran as well as your java and platform environment. In the meantime, I have attached the new changes. Thanks,
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, the test changes look great.

        The attached repro (Serialize.java) shows the problem with getMessage(). Note that I used getCause() to get the actual derby.client.am.SqlException object (which is not a java.sql.SQLException), whereas your tests use getNextException() which returns a java.sql.SQLException. I ran the repro with Sun J2SE 5.0 on Solaris 10 and got the following output:

            • Message from original exception ***

        Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL061009023735980' defined on 'T'.

            • Message from deserialized exception ***

        Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL061009023735980' defined on 'T'.

        Show
        Knut Anders Hatlen added a comment - Thanks, the test changes look great. The attached repro (Serialize.java) shows the problem with getMessage(). Note that I used getCause() to get the actual derby.client.am.SqlException object (which is not a java.sql.SQLException), whereas your tests use getNextException() which returns a java.sql.SQLException. I ran the repro with Sun J2SE 5.0 on Solaris 10 and got the following output: Message from original exception *** Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL061009023735980' defined on 'T'. Message from deserialized exception *** Error for batch element #1: Error for batch element #1: The statement was aborted because it would have caused a duplicate key value in a unique or primary key constraint or unique index identified by 'SQL061009023735980' defined on 'T'.
        Hide
        Rick Hillegas added a comment -

        Unknown release vehicle.

        Show
        Rick Hillegas added a comment - Unknown release vehicle.
        Hide
        Francois Orsini added a comment -

        Comments for this patch have been taken care of - The reproducible test case (Serialize.java) is also passing. New tests that got added are also passing.

        If someone could test a full and partial build with the changes I would appreciate - at some point, it uses to compile fine with a full clean build and then I saw some class dependencies build issue on a partial build - I can no longer reproduce it but I'd like to make sure there is no build issue.

        Thanks in advance,

        Show
        Francois Orsini added a comment - Comments for this patch have been taken care of - The reproducible test case (Serialize.java) is also passing. New tests that got added are also passing. If someone could test a full and partial build with the changes I would appreciate - at some point, it uses to compile fine with a full clean build and then I saw some class dependencies build issue on a partial build - I can no longer reproduce it but I'd like to make sure there is no build issue. Thanks in advance,
        Hide
        Knut Anders Hatlen added a comment -

        I don't see the build issue if I do an "ant clean" after applying your patch. If I don't do "ant clean", or if I run "touch java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java" before I run "ant all", I see this failure:

        junitcomponents:
        [javac] Compiling 2 source files to /export/home/kah/derby/trunk/classes
        [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:727: cannot find symbol
        [javac] symbol : method getCause()
        [javac] location: class java.sql.SQLException
        [javac] if (se1.getCause() != (Throwable) null)
        [javac] ^
        [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:728: cannot find symbol
        [javac] symbol : method getCause()
        [javac] location: class java.sql.SQLException
        [javac] assertThrowableEquals(se1.getCause(), se2.getCause());
        [javac] ^
        [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:728: cannot find symbol
        [javac] symbol : method getCause()
        [javac] location: class java.sql.SQLException
        [javac] assertThrowableEquals(se1.getCause(), se2.getCause());
        [javac] ^
        [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:730: cannot find symbol
        [javac] symbol : method getCause()
        [javac] location: class java.sql.SQLException
        [javac] assertNull(se2.getCause());
        [javac] ^
        [javac] 4 errors

        BUILD FAILED

        I think this is because of a problem with the dependencies in the build scripts. build.xml in the junit directory specifies that BaseJDBCTestCase should be compiled against the 1.3 libraries, and that is what happens if BaseJDBCTestCase is the only file that has been modified. However, it seems like it is compiled against the 1.4 libraries after a clean/clobber.

        BaseJDBCTestCase.assertSQLState() uses reflection to invoke Throwable.initCause(). I guess that could also be done for getCause() in assertSQLExceptionEquals() to avoid the problem.

        Show
        Knut Anders Hatlen added a comment - I don't see the build issue if I do an "ant clean" after applying your patch. If I don't do "ant clean", or if I run "touch java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java" before I run "ant all", I see this failure: junitcomponents: [javac] Compiling 2 source files to /export/home/kah/derby/trunk/classes [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:727: cannot find symbol [javac] symbol : method getCause() [javac] location: class java.sql.SQLException [javac] if (se1.getCause() != (Throwable) null) [javac] ^ [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:728: cannot find symbol [javac] symbol : method getCause() [javac] location: class java.sql.SQLException [javac] assertThrowableEquals(se1.getCause(), se2.getCause()); [javac] ^ [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:728: cannot find symbol [javac] symbol : method getCause() [javac] location: class java.sql.SQLException [javac] assertThrowableEquals(se1.getCause(), se2.getCause()); [javac] ^ [javac] /export/home/kah/derby/trunk/java/testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java:730: cannot find symbol [javac] symbol : method getCause() [javac] location: class java.sql.SQLException [javac] assertNull(se2.getCause()); [javac] ^ [javac] 4 errors BUILD FAILED I think this is because of a problem with the dependencies in the build scripts. build.xml in the junit directory specifies that BaseJDBCTestCase should be compiled against the 1.3 libraries, and that is what happens if BaseJDBCTestCase is the only file that has been modified. However, it seems like it is compiled against the 1.4 libraries after a clean/clobber. BaseJDBCTestCase.assertSQLState() uses reflection to invoke Throwable.initCause(). I guess that could also be done for getCause() in assertSQLExceptionEquals() to avoid the problem.
        Hide
        Knut Anders Hatlen added a comment -

        Apart from the build issue, I think the patch looks good. My only minor comment is that the println statement in BaseTestCase which is commented out could be removed.

        Show
        Knut Anders Hatlen added a comment - Apart from the build issue, I think the patch looks good. My only minor comment is that the println statement in BaseTestCase which is commented out could be removed.
        Hide
        Francois Orsini added a comment -

        Thanks for the great review Knut.

        I've removed the println() tracing statement and I'm now using reflection - I think this is fine as this is within some test method and as you mentioned, it is already used in some other method within that class.

        Am running DerbyAll again and in the meantime I'm posting the new changes.

        Cheers

        Show
        Francois Orsini added a comment - Thanks for the great review Knut. I've removed the println() tracing statement and I'm now using reflection - I think this is fine as this is within some test method and as you mentioned, it is already used in some other method within that class. Am running DerbyAll again and in the meantime I'm posting the new changes. Cheers
        Hide
        Knut Anders Hatlen added a comment -

        Thanks Francois! I have committed v5 to trunk with revision 504630. Should the fix be ported to 10.2 as well?

        Show
        Knut Anders Hatlen added a comment - Thanks Francois! I have committed v5 to trunk with revision 504630. Should the fix be ported to 10.2 as well?
        Hide
        Francois Orsini added a comment -

        Thanks for the commit and great help Knut.

        Yes I think it should be ported to 10.2 branch as well - This is a blocking issue for frameworks that need to do JDBC objects serialization (i.e. via RMI). Changes are fairly low-risk as well. +1

        Show
        Francois Orsini added a comment - Thanks for the commit and great help Knut. Yes I think it should be ported to 10.2 branch as well - This is a blocking issue for frameworks that need to do JDBC objects serialization (i.e. via RMI). Changes are fairly low-risk as well. +1
        Hide
        Knut Anders Hatlen added a comment -

        There was a conflict in SqlExceptionTest so the fix couldn't be merged automatically, but the conflict was easy to resolve. The attached patch applies cleanly on 10.2. I'm running derbyall on it now and will commit if there are no problems.

        Show
        Knut Anders Hatlen added a comment - There was a conflict in SqlExceptionTest so the fix couldn't be merged automatically, but the conflict was easy to resolve. The attached patch applies cleanly on 10.2. I'm running derbyall on it now and will commit if there are no problems.
        Hide
        Knut Anders Hatlen added a comment -

        Committed fix to 10.2 with revision 504880.

        Show
        Knut Anders Hatlen added a comment - Committed fix to 10.2 with revision 504880.
        Hide
        Dag H. Wanvik added a comment -

        Been resolved since Feb, and reporter hasn't closed it, so closing.

        Show
        Dag H. Wanvik added a comment - Been resolved since Feb, and reporter hasn't closed it, so closing.

          People

          • Assignee:
            Francois Orsini
            Reporter:
            David Cabelus
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development