castor
  1. castor
  2. CASTOR-1276

Need a method for forcing the timezone for dates written to the database

    Details

    • Type: New Feature New Feature
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.9.1
    • Fix Version/s: 1.1.1
    • Component/s: JDO types
    • Labels:
      None
    • Number of attachments :
      9

      Description

      Castor has a method for forcing what timezone to read dates with. (See http://www.castor.org/types.html#SQL-Dates-and-Default-Timezone). The problem is that this setting has not effect on what you write to the database. So you don't have round-trip integrity of date, time and timestamp values.

      For example, if I set the timezone to be 'UTC' in the castor.properties. When I write a date to the database it will write it to the database as 'PDT' (my local timezone) cause this is what the JDBC driver defaults too. But when I read it back it will convert it to 'UTC' because castor is is only converting on reads. So my dates will be 8 hours off of what they should be. Anyway, not the clearest explanation...sorry. I've attached a modified SQLTypeInfos.java file that should address the issue.

      1. patch.c1276.20051202.txt
        1 kB
        Werner Guttmann
      2. patch.C1276.20051218.txt
        7 kB
        Werner Guttmann
      3. patch.C1276.20051218-002.txt
        34 kB
        Werner Guttmann
      4. patch.C1276.20051219.test.txt
        9 kB
        Werner Guttmann
      5. patch-C1276-20070130.txt
        11 kB
        Ralf Joachim
      6. patch-C1276-20070205.txt
        16 kB
        Ralf Joachim
      7. patch-C1276-20070307.txt
        33 kB
        Ralf Joachim
      8. SQLTypeInfos.java
        13 kB
        Brian Schlining
      9. TestTemplate.java
        10 kB
        Brian Schlining

        Issue Links

          Activity

          Hide
          Werner Guttmann added a comment -

          Thanks, Brian. In future, when attaching patches, can you please attach a unified patch only ?

          Show
          Werner Guttmann added a comment - Thanks, Brian. In future, when attaching patches, can you please attach a unified patch only ?
          Hide
          Werner Guttmann added a comment -

          Unified patch.

          Show
          Werner Guttmann added a comment - Unified patch.
          Hide
          Brian Schlining added a comment -

          Sorry about not submitting just a patch....future contributions will be in patch form.

          Show
          Brian Schlining added a comment - Sorry about not submitting just a patch....future contributions will be in patch form.
          Hide
          Werner Guttmann added a comment -

          Brian, any ideas towards testing this ?

          Show
          Werner Guttmann added a comment - Brian, any ideas towards testing this ?
          Hide
          Werner Guttmann added a comment -

          Brian, it looks like your patch is a valid one (after inserting a few break; statements throughout your additions .. ), but unfortunately it falls short from providing a complete solution, as with Castor a java.util.Date property of a Java class about to be persisted can be mapped to numeric and string columns as well. In my view, one needs to add code to SQLEngine.toSql() to set the Calendar instance on the java.util.Date object instance before it's being converted subsequently.

          Show
          Werner Guttmann added a comment - Brian, it looks like your patch is a valid one (after inserting a few break; statements throughout your additions .. ), but unfortunately it falls short from providing a complete solution, as with Castor a java.util.Date property of a Java class about to be persisted can be mapped to numeric and string columns as well. In my view, one needs to add code to SQLEngine.toSql() to set the Calendar instance on the java.util.Date object instance before it's being converted subsequently.
          Hide
          Werner Guttmann added a comment -

          Updated patch, incl. a few new break; statements. In addition, this patch carries a TODO statement in SQLEngine.toSql() to get an idea what I am talking about above.

          Show
          Werner Guttmann added a comment - Updated patch, incl. a few new break; statements. In addition, this patch carries a TODO statement in SQLEngine.toSql() to get an idea what I am talking about above.
          Hide
          Ralf Joachim added a comment -

          Werner, you are right that Brian's patch to support timezone at store do not cover Date's that are mapped to char, integer or numeric. If you take at the timezone support when loading these dates the mappings to char, integer and numeric are also ignored. I suggest to commit your original patch and don't add this support yet at least not in SQLEngine. If you really want to support these other mappings I think the much better place are SQLTypeConverters.

          Show
          Ralf Joachim added a comment - Werner, you are right that Brian's patch to support timezone at store do not cover Date's that are mapped to char, integer or numeric. If you take at the timezone support when loading these dates the mappings to char, integer and numeric are also ignored. I suggest to commit your original patch and don't add this support yet at least not in SQLEngine. If you really want to support these other mappings I think the much better place are SQLTypeConverters.
          Hide
          Werner Guttmann added a comment -

          Okay, fine by me, I'll commit the original patch as it is, and add a new issue to cover extended support during conversions. It's funny you are mentioning SQLTypeConverters, as I have been thinking about adding code in some of those convertors. 'Problem' is that you'd have to add the same piece of code over and over again.

          Show
          Werner Guttmann added a comment - Okay, fine by me, I'll commit the original patch as it is, and add a new issue to cover extended support during conversions. It's funny you are mentioning SQLTypeConverters, as I have been thinking about adding code in some of those convertors. 'Problem' is that you'd have to add the same piece of code over and over again.
          Hide
          Werner Guttmann added a comment -

          Final patch, as discussed above. I still have to add a CTF test, though.

          Show
          Werner Guttmann added a comment - Final patch, as discussed above. I still have to add a CTF test, though.
          Hide
          Werner Guttmann added a comment -

          Started to sketch up a small test in src/bugs, but to my surprise it does not fail. I thought setting different timezones for updates/creates and reads should make it possible to test this functionality, but to my surprise this is not the case.

          Show
          Werner Guttmann added a comment - Started to sketch up a small test in src/bugs, but to my surprise it does not fail. I thought setting different timezones for updates/creates and reads should make it possible to test this functionality, but to my surprise this is not the case.
          Hide
          Werner Guttmann added a comment -

          Unless somebody comes up with a bit more structured idea how to test this, I am going to pull this from 1.0M1 and assign this to 1.0 (final).

          Show
          Werner Guttmann added a comment - Unless somebody comes up with a bit more structured idea how to test this, I am going to pull this from 1.0M1 and assign this to 1.0 (final).
          Hide
          Werner Guttmann added a comment -

          Moving to 1.0 (final) as indicated.

          Show
          Werner Guttmann added a comment - Moving to 1.0 (final) as indicated.
          Hide
          Werner Guttmann added a comment -

          Any ideas ? Anybody ?

          Show
          Werner Guttmann added a comment - Any ideas ? Anybody ?
          Hide
          Brian Schlining added a comment -

          Hmmm, somehow my watch on this issue got turned off, so I missed most of the traffic. I'll take a look into this later this week.

          Show
          Brian Schlining added a comment - Hmmm, somehow my watch on this issue got turned off, so I missed most of the traffic. I'll take a look into this later this week.
          Hide
          Brian Schlining added a comment -

          For a workaround to this issue, I have been setting the JVM's timezone to UTC on startup. [Just FYI, for this to work you have to set the timezone on the command line (using -Duser.timzone=UTC) or in your program before you initialize any Date, Calendar or logging objects (using System.setProperty("user.timezone", "UTC");]

          Recently, I've begun deploying my apps using Java Web Start (JWS). Unfortunately, due to security restrictions in JWS, I can no longer set the timezone of the JVM. (see http://forum.java.sun.com/thread.jspa?forumID=38&threadID=735277). So I'm under some pressure to find a resolution to this timezone issue.

          If my reading of the thread is on track, it looks like some work was done on this but it's not clear to me if the patches can be applied to the SVN trunk or even if the patches represent the solution that you intend to implement. If you can point me to the recommended solution (modify SQLEngine or SQLTypeConvertors?) I'm happy to take a stab at it and write up a few tests.

          Show
          Brian Schlining added a comment - For a workaround to this issue, I have been setting the JVM's timezone to UTC on startup. [Just FYI, for this to work you have to set the timezone on the command line (using -Duser.timzone=UTC) or in your program before you initialize any Date, Calendar or logging objects (using System.setProperty("user.timezone", "UTC");] Recently, I've begun deploying my apps using Java Web Start (JWS). Unfortunately, due to security restrictions in JWS, I can no longer set the timezone of the JVM. (see http://forum.java.sun.com/thread.jspa?forumID=38&threadID=735277 ). So I'm under some pressure to find a resolution to this timezone issue. If my reading of the thread is on track, it looks like some work was done on this but it's not clear to me if the patches can be applied to the SVN trunk or even if the patches represent the solution that you intend to implement. If you can point me to the recommended solution (modify SQLEngine or SQLTypeConvertors?) I'm happy to take a stab at it and write up a few tests.
          Hide
          Ralf Joachim added a comment -

          Brian, looking at the patch it seams to me that it should cover date values of your domain objects written to TIMESTAMP, DATE and TIME columns of your database. Date values written to LONG or CHAR columns are not covered yet. As far as I understand Werner's latest comments he had problems to write a test case to reproduce the problem and that it is fixed by the patch.

          I will update the patch to current SVN and hope this will help you to test the patch and provide us a test case to reproduce thze problem and verify the solution.

          Show
          Ralf Joachim added a comment - Brian, looking at the patch it seams to me that it should cover date values of your domain objects written to TIMESTAMP, DATE and TIME columns of your database. Date values written to LONG or CHAR columns are not covered yet. As far as I understand Werner's latest comments he had problems to write a test case to reproduce the problem and that it is fixed by the patch. I will update the patch to current SVN and hope this will help you to test the patch and provide us a test case to reproduce thze problem and verify the solution.
          Hide
          Ralf Joachim added a comment -

          Updated patch against current SVN. Having said that I only updated the patch but did not test anything yet.

          Show
          Ralf Joachim added a comment - Updated patch against current SVN. Having said that I only updated the patch but did not test anything yet.
          Hide
          Brian Schlining added a comment -

          Thanks Ralf,

          I'm checking out SVN trunk right now and will run some tests in the application that depends on this functionality in Castor (http://vars.sourceforge.net). Once I've verified that it works with my application, I'll write a few test cases.

          Show
          Brian Schlining added a comment - Thanks Ralf, I'm checking out SVN trunk right now and will run some tests in the application that depends on this functionality in Castor ( http://vars.sourceforge.net ). Once I've verified that it works with my application, I'll write a few test cases.
          Hide
          Brian Schlining added a comment -

          I found Werner's test case in castor/src/bugs/jdo/bug1276. Rather than wrestle with getting setup to run that test, I created a test using my existing application to check 'datetime'/'timestamp' fields. The test I ran is at http://vars.svn.sourceforge.net/viewvc/vars/trunk/vars/src/test/java/org/mbari/jdo/castor/TestDateStorage.java?view=markup. The patch works, at least for my cases.

          I added one more test method to src/bugs/jdo/bug1276/TestTemplate.java and attached the new file. Sorry I didn't generate a unified patch; it's tough to generate a patch for a patch that hasn't been applied to SVN yet . To commit it to SVN you have to apply your patch (patch-C1276-20070130.txt) then copy the new TestTemplate.java file to src/bugs/jdo/bug1276/. Unfortunately, the added test method only checks 'timestamp' SQL data types. I'm under a dealine crunch so I'll try to get tests for 'date' and 'time' SQL data types done 'REAL SOON'.

          Show
          Brian Schlining added a comment - I found Werner's test case in castor/src/bugs/jdo/bug1276. Rather than wrestle with getting setup to run that test, I created a test using my existing application to check 'datetime'/'timestamp' fields. The test I ran is at http://vars.svn.sourceforge.net/viewvc/vars/trunk/vars/src/test/java/org/mbari/jdo/castor/TestDateStorage.java?view=markup . The patch works, at least for my cases. I added one more test method to src/bugs/jdo/bug1276/TestTemplate.java and attached the new file. Sorry I didn't generate a unified patch; it's tough to generate a patch for a patch that hasn't been applied to SVN yet . To commit it to SVN you have to apply your patch (patch-C1276-20070130.txt) then copy the new TestTemplate.java file to src/bugs/jdo/bug1276/. Unfortunately, the added test method only checks 'timestamp' SQL data types. I'm under a dealine crunch so I'll try to get tests for 'date' and 'time' SQL data types done 'REAL SOON'.
          Hide
          Werner Guttmann added a comment -

          Thanks, Brian. And unless you need that fix for your deadline(s), take yourself time.

          Show
          Werner Guttmann added a comment - Thanks, Brian. And unless you need that fix for your deadline(s), take yourself time.
          Hide
          Brian Schlining added a comment -

          As you may be aware, with the current builds of Castor, setting the timezone will currupt dates, times, and timestamps. For example, if you set the timezone in castor.properties, then calling something like

          // Set timezone as UTC in castor.properties. Local timezone is PDT
          Date d = entity.getTimestamp();
          entity.setTimestamp(d);
          // Start new database transaction
          d = entity.getTimestamp();
          entity.setTimestamp(d);

          will cause the date to 'walk' because Castor is reading from the date from the database as UTC but writting it as PDT. Every read/write cycle will change the date by 7 hours. Commiting this patch, while not covering all date cases, will fix the ones that are currently broken.

          Show
          Brian Schlining added a comment - As you may be aware, with the current builds of Castor, setting the timezone will currupt dates, times, and timestamps. For example, if you set the timezone in castor.properties, then calling something like // Set timezone as UTC in castor.properties. Local timezone is PDT Date d = entity.getTimestamp(); entity.setTimestamp(d); // Start new database transaction d = entity.getTimestamp(); entity.setTimestamp(d); will cause the date to 'walk' because Castor is reading from the date from the database as UTC but writting it as PDT. Every read/write cycle will change the date by 7 hours. Commiting this patch, while not covering all date cases, will fix the ones that are currently broken.
          Hide
          Ralf Joachim added a comment -

          Brian, I am quite willing to include this patch with 1.1 final release but at the moment I am not able to replay the problem even with the test case you provided. Attached a new patch including your test case.

          Show
          Ralf Joachim added a comment - Brian, I am quite willing to include this patch with 1.1 final release but at the moment I am not able to replay the problem even with the test case you provided. Attached a new patch including your test case.
          Hide
          Brian Schlining added a comment -

          Hi Ralf,

          Sorry in the delay getting back to you. Anyway when you ran the test case did you add this line to 'castor.properties':

          org.exolab.castor.jdo.defaultTimeZone=UTC

          If you do this the test case I provided should fail (assuming you aren't actually in the UTC timezone)

          Show
          Brian Schlining added a comment - Hi Ralf, Sorry in the delay getting back to you. Anyway when you ran the test case did you add this line to 'castor.properties': org.exolab.castor.jdo.defaultTimeZone=UTC If you do this the test case I provided should fail (assuming you aren't actually in the UTC timezone)
          Hide
          Ralf Joachim added a comment -

          Now that you mention the timezone setting in castor.properties I know what I missed Will come back with test updated result soon.

          Show
          Ralf Joachim added a comment - Now that you mention the timezone setting in castor.properties I know what I missed Will come back with test updated result soon.
          Hide
          Ralf Joachim added a comment -

          Hi Brian,

          after trying lots of different things I finaly managed to reproduce the problem and also verified that the solution is valid. The reason for my problems was that mysql driver seams to be a bit more inteligent with regard to timezones, as this driver corrects the timezone mismatch himself. As I have no access to microsoft sql server I used maxdb instead which showed the same problem you have with the microsoft one.

          After cleanup and improvement of the test case I'll commit the patch. Having said that I don't think we can integrate the test case with our suite at the moment as we have no possibility to change properties for a single test.

          Show
          Ralf Joachim added a comment - Hi Brian, after trying lots of different things I finaly managed to reproduce the problem and also verified that the solution is valid. The reason for my problems was that mysql driver seams to be a bit more inteligent with regard to timezones, as this driver corrects the timezone mismatch himself. As I have no access to microsoft sql server I used maxdb instead which showed the same problem you have with the microsoft one. After cleanup and improvement of the test case I'll commit the patch. Having said that I don't think we can integrate the test case with our suite at the moment as we have no possibility to change properties for a single test.
          Hide
          Brian Schlining added a comment -

          Hi Ralf,

          Thanks for going through the contortions deed to run this test. This bug has been impacting me in a very painful ways, so I'm very relieved that you'll commit the patch.

          BTW, I have run this same test on Apache Derby, Oracle 10g and XE as well as SQL Server 2000 and they have all exhibited this time zone issue. That's very interesting about the MySQL driver though; I'll have to remember that.

          Show
          Brian Schlining added a comment - Hi Ralf, Thanks for going through the contortions deed to run this test. This bug has been impacting me in a very painful ways, so I'm very relieved that you'll commit the patch. BTW, I have run this same test on Apache Derby, Oracle 10g and XE as well as SQL Server 2000 and they have all exhibited this time zone issue. That's very interesting about the MySQL driver though; I'll have to remember that.
          Hide
          Ralf Joachim added a comment -

          Final patch for review.

          It need to be noted that I had to remove force of timezone for reading and writing SQL dates to omit problems. For SQL time and timestamp Castor will now allow to force a timezone. Still we need to find a solution to force timezone if date values are stored as long or string but that's another issue.

          Show
          Ralf Joachim added a comment - Final patch for review. It need to be noted that I had to remove force of timezone for reading and writing SQL dates to omit problems. For SQL time and timestamp Castor will now allow to force a timezone. Still we need to find a solution to force timezone if date values are stored as long or string but that's another issue.

            People

            • Assignee:
              Ralf Joachim
              Reporter:
              Brian Schlining
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: