Wicket
  1. Wicket
  2. WICKET-3510

DateTimeField improperly converts time causing wrong dates when the server's current date is different from the client's date.

    Details

      Description

      The bug is in DateTimeField#convertInput().
      <code>
      // Get year, month and day ignoring any timezone of the Date object
      Calendar cal = Calendar.getInstance();
      cal.setTime(dateFieldInput);
      int year = cal.get(Calendar.YEAR);
      int month = cal.get(Calendar.MONTH) + 1;
      int day = cal.get(Calendar.DAY_OF_MONTH);
      int hours = (hoursInput == null ? 0 : hoursInput % 24);
      int minutes = (minutesInput == null ? 0 : minutesInput);

      // Use the input to create a date object with proper timezone
      MutableDateTime date = new MutableDateTime(year, month, day, hours, minutes, 0, 0,
      DateTimeZone.forTimeZone(getClientTimeZone()));
      </code>
      If the server's current date is different from the client's, this produces wrong output. I attached a patch with a test case that simulates this condition.

      I don't know why this "casting" of day, month, year is done.

      1. dateModelConversionFix_TimeField_NoEnclosure.patch
        17 kB
        Bertrand Guay-Paquet
      2. dateModelConversionFix_TimeField.patch
        16 kB
        Bertrand Guay-Paquet
      3. dateModelConversionFix.patch
        11 kB
        Andrea Del Bene
      4. testsWithDateModels.patch
        9 kB
        Bertrand Guay-Paquet
      5. DateConverterFix.patch
        4 kB
        Andrea Del Bene
      6. doubleConversionFix.patch
        4 kB
        Andrea Del Bene
      7. FailingTest.patch
        2 kB
        Bertrand Guay-Paquet
      8. TestDateConverter.java
        1 kB
        Bertrand Guay-Paquet

        Activity

        Hide
        Igor Vaynberg added a comment -

        latest patch applied

        Show
        Igor Vaynberg added a comment - latest patch applied
        Hide
        Bertrand Guay-Paquet added a comment -

        New patch without wicket:enclosure

        Show
        Bertrand Guay-Paquet added a comment - New patch without wicket:enclosure
        Hide
        Bertrand Guay-Paquet added a comment -

        Thanks for your patch, it looks good!

        dateModelConversionFix_TimeField.patch contains your changes
        + 2 new tests
        + TimeField does not modify day-month-year of its model
        + ":" character between hour and minute text fields removed when using a DateField.

        The 2 tests validate am/pm conversion and "TimeField does not modify day-month-year of its model".

        Show
        Bertrand Guay-Paquet added a comment - Thanks for your patch, it looks good! dateModelConversionFix_TimeField.patch contains your changes + 2 new tests + TimeField does not modify day-month-year of its model + ":" character between hour and minute text fields removed when using a DateField. The 2 tests validate am/pm conversion and "TimeField does not modify day-month-year of its model".
        Hide
        Bertrand Guay-Paquet added a comment -

        Builds on dateModelConversionFix.patch to also fix TimeField issue.

        Show
        Bertrand Guay-Paquet added a comment - Builds on dateModelConversionFix.patch to also fix TimeField issue.
        Hide
        Bertrand Guay-Paquet added a comment -

        I just realized there is a problem with the design of the TimeField. In order to properly convert between the client's and server's time zone, a date (day,month,year) for which the conversion is to be done must be provided. Otherwise, the conversion cannot happen since we can't determine whether we should consider the time zones with or without daylight savings. Currently, TimeField does the conversion using the current date but this is not valid.

        Continuing investigation...

        Show
        Bertrand Guay-Paquet added a comment - I just realized there is a problem with the design of the TimeField. In order to properly convert between the client's and server's time zone, a date (day,month,year) for which the conversion is to be done must be provided. Otherwise, the conversion cannot happen since we can't determine whether we should consider the time zones with or without daylight savings. Currently, TimeField does the conversion using the current date but this is not valid. Continuing investigation...
        Hide
        Bertrand Guay-Paquet added a comment -

        Hi Andera,

        I thought changeset 1079694 introduced this bug because while tracing execution, I found that DateTimeField#newDateTextField() was changed to create a DateTextField which didn't apply the client time zone conversion. I could be wrong however because it seems that the conversion is also performed elsewhere.

        I will take a look during the day and post back.

        Show
        Bertrand Guay-Paquet added a comment - Hi Andera, I thought changeset 1079694 introduced this bug because while tracing execution, I found that DateTimeField#newDateTextField() was changed to create a DateTextField which didn't apply the client time zone conversion. I could be wrong however because it seems that the conversion is also performed elsewhere. I will take a look during the day and post back.
        Hide
        Andrea Del Bene added a comment -

        Hi Bertrand,

        are you sure that Changeset 1079694 introduced this bug? I think that it was present since Changeset 1077866, related to issue 3501. Anyway after some googling I've found an insightful post by Vinod Singh here: http://blog.vinodsingh.com/2009/03/date-and-timezone-in-java.html.
        So I've used his function 'Date changeTimeZone(Date date, TimeZone zone)' to fix this issue. But I'm not sure where to put this function. For now I've inserted it directly into class DateTimeField.java but I'm afraid it's not the right place. Is there in Wicket a better place ( maybe an utility class) to put this function in?

        Any suggestion?

        PS: I've attached the fix including testsWithDateModels.patch

        Show
        Andrea Del Bene added a comment - Hi Bertrand, are you sure that Changeset 1079694 introduced this bug? I think that it was present since Changeset 1077866, related to issue 3501. Anyway after some googling I've found an insightful post by Vinod Singh here: http://blog.vinodsingh.com/2009/03/date-and-timezone-in-java.html . So I've used his function 'Date changeTimeZone(Date date, TimeZone zone)' to fix this issue. But I'm not sure where to put this function. For now I've inserted it directly into class DateTimeField.java but I'm afraid it's not the right place. Is there in Wicket a better place ( maybe an utility class) to put this function in? Any suggestion? PS: I've attached the fix including testsWithDateModels.patch
        Hide
        Bertrand Guay-Paquet added a comment - - edited

        Please reopen this issue.

        Changeset 1079694 introduced a bug when displaying a Date in a DateField or a DateTimeField. The day is not converted to the client's time zone, but the time is. See testsWithDateModels.patch.

        The patch also includes some clean-up to the test class.

        Show
        Bertrand Guay-Paquet added a comment - - edited Please reopen this issue. Changeset 1079694 introduced a bug when displaying a Date in a DateField or a DateTimeField. The day is not converted to the client's time zone, but the time is. See testsWithDateModels.patch. The patch also includes some clean-up to the test class.
        Hide
        Bertrand Guay-Paquet added a comment -

        Demonstrates wrong conversion between server's and client's time zones when displayer a Date.

        Show
        Bertrand Guay-Paquet added a comment - Demonstrates wrong conversion between server's and client's time zones when displayer a Date.
        Hide
        Bertrand Guay-Paquet added a comment -

        Ah yes that "new" bug I found was actually issue 3249 we previously discussed!

        Regarding my test-case comments, I only asked to make sure it wasn't against some Wicket test-case policy to include them.

        Thanks again!

        Show
        Bertrand Guay-Paquet added a comment - Ah yes that "new" bug I found was actually issue 3249 we previously discussed! Regarding my test-case comments, I only asked to make sure it wasn't against some Wicket test-case policy to include them. Thanks again!
        Hide
        Igor Vaynberg added a comment -

        latest patch applied

        Show
        Igor Vaynberg added a comment - latest patch applied
        Hide
        Andrea Del Bene added a comment -

        Sorry Bertrand, I think I have accidentally removed your comment from patch.
        Anyway the last problem you've found should be fixed by the patch I've attached to issue 3249. But before you run your last method test (testStyleDateConverterTimeZoneDifference) you should modify it a little because it is misleading.
        You should add client time zone to web session and reset calendar milliseconds to zero.

        I've attached the patch including my previous patch, your last test method and your missing comment

        Show
        Andrea Del Bene added a comment - Sorry Bertrand, I think I have accidentally removed your comment from patch. Anyway the last problem you've found should be fixed by the patch I've attached to issue 3249. But before you run your last method test (testStyleDateConverterTimeZoneDifference) you should modify it a little because it is misleading. You should add client time zone to web session and reset calendar milliseconds to zero. I've attached the patch including my previous patch, your last test method and your missing comment
        Hide
        Bertrand Guay-Paquet added a comment -

        Thank you for the patch.

        However, I found another problem in the Date conversion pipeline:
        public void testStyleDateConverterTimeZoneDifference() throws ParseException

        { TimeZone origJvmDef = TimeZone.getDefault(); DateTimeZone origJodaDef = DateTimeZone.getDefault(); TimeZone tzClient = TimeZone.getTimeZone("Etc/GMT-14"); TimeZone tzServer = TimeZone.getTimeZone("Etc/GMT+12"); TimeZone.setDefault(tzServer); DateTimeZone.setDefault(DateTimeZone.forTimeZone(tzServer)); Locale.setDefault(Locale.GERMAN); StyleDateConverter converter = new StyleDateConverter(true); Calendar cal = Calendar.getInstance(tzClient); cal.set(2011, 10, 5, 0, 0, 0); Date dateRef = cal.getTime(); Date date = converter.convertToObject("05.11.2011", Locale.GERMAN); log.error("ref: " + dateRef.getTime() + "; converted: " + date.getTime()); log.error("ref: " + dateRef + "; date: " + date); assertEquals(0, dateRef.compareTo(date)); TimeZone.setDefault(origJvmDef); DateTimeZone.setDefault(origJodaDef); }

        This means that the patch did solve part of the problem (double time zone conversion), but the converter is still broken.

        What is the procedure for reopening an issue?

        Also, is there a reason the test case method comment I submitted as part of the patch was removed? I thought it provided a clear explanation of why the test needed to be present.

        Show
        Bertrand Guay-Paquet added a comment - Thank you for the patch. However, I found another problem in the Date conversion pipeline: public void testStyleDateConverterTimeZoneDifference() throws ParseException { TimeZone origJvmDef = TimeZone.getDefault(); DateTimeZone origJodaDef = DateTimeZone.getDefault(); TimeZone tzClient = TimeZone.getTimeZone("Etc/GMT-14"); TimeZone tzServer = TimeZone.getTimeZone("Etc/GMT+12"); TimeZone.setDefault(tzServer); DateTimeZone.setDefault(DateTimeZone.forTimeZone(tzServer)); Locale.setDefault(Locale.GERMAN); StyleDateConverter converter = new StyleDateConverter(true); Calendar cal = Calendar.getInstance(tzClient); cal.set(2011, 10, 5, 0, 0, 0); Date dateRef = cal.getTime(); Date date = converter.convertToObject("05.11.2011", Locale.GERMAN); log.error("ref: " + dateRef.getTime() + "; converted: " + date.getTime()); log.error("ref: " + dateRef + "; date: " + date); assertEquals(0, dateRef.compareTo(date)); TimeZone.setDefault(origJvmDef); DateTimeZone.setDefault(origJodaDef); } This means that the patch did solve part of the problem (double time zone conversion), but the converter is still broken. What is the procedure for reopening an issue? Also, is there a reason the test case method comment I submitted as part of the patch was removed? I thought it provided a clear explanation of why the test needed to be present.
        Hide
        Igor Vaynberg added a comment -

        applied doubleConversionFix.patch, thanks

        Show
        Igor Vaynberg added a comment - applied doubleConversionFix.patch, thanks
        Hide
        Andrea Del Bene added a comment -

        I think I've found where is the problem. Time zone conversion of user input is performed twice: the first time in DateTextField control and the second time in method DateTimeField#convertInput().
        That's because in method DateTimeField#newDateTextField we build a DateTextField with a date converter which take into account time zone difference between the server and the client.
        The problem arises only when client and server have a different date at the same time (like in your test). That's why existing test method 'test3' doesn't fail.

        I've attached a patch that solve this problem. It modifies method DateTextField#forShortStyle adding a boolean flag that switches on/off time zone conversion.

        Show
        Andrea Del Bene added a comment - I think I've found where is the problem. Time zone conversion of user input is performed twice: the first time in DateTextField control and the second time in method DateTimeField#convertInput(). That's because in method DateTimeField#newDateTextField we build a DateTextField with a date converter which take into account time zone difference between the server and the client. The problem arises only when client and server have a different date at the same time (like in your test). That's why existing test method 'test3' doesn't fail. I've attached a patch that solve this problem. It modifies method DateTextField#forShortStyle adding a boolean flag that switches on/off time zone conversion.
        Hide
        Bertrand Guay-Paquet added a comment -

        Hi Andera,

        Terribly sorry, I attached the wrong file to the issue. The correct file is FailingTest.patch (based on 1.5-rc2).

        'TestDateConverter.java' is indeed from issue WICKET-3249 which I examined in detail first to make sure I wasn't reporting the same problem. It wasn't meant to be attached to this issue.

        Show
        Bertrand Guay-Paquet added a comment - Hi Andera, Terribly sorry, I attached the wrong file to the issue. The correct file is FailingTest.patch (based on 1.5-rc2). 'TestDateConverter.java' is indeed from issue WICKET-3249 which I examined in detail first to make sure I wasn't reporting the same problem. It wasn't meant to be attached to this issue.
        Hide
        Bertrand Guay-Paquet added a comment -

        Test case demonstrating the problem

        Show
        Bertrand Guay-Paquet added a comment - Test case demonstrating the problem
        Hide
        Andrea Del Bene added a comment -

        Hi Bertrand,

        I think that this issue could be related with this one: https://issues.apache.org/jira/browse/WICKET-3249
        Did you take 'TestDateConverter.java' from this issue?
        Anyway, can you post the code of method testDifferentDateTimeZoneConversion ?

        Show
        Andrea Del Bene added a comment - Hi Bertrand, I think that this issue could be related with this one: https://issues.apache.org/jira/browse/WICKET-3249 Did you take 'TestDateConverter.java' from this issue? Anyway, can you post the code of method testDifferentDateTimeZoneConversion ?
        Hide
        Bertrand Guay-Paquet added a comment - - edited

        Sample output:

        ERROR - DatePickerTest - =========== testDifferentDateTimeZoneConversion() =================
        ERROR - DatePickerTest - orig: 1288951200000; date: 1288864800000; dateTime: 1288864800000
        ERROR - DatePickerTest - orig: Thu Nov 04 22:00:00 GMT-12:00 2010; date: Wed Nov 03 22:00:00 GMT-12:00 2010; dateTime: Wed Nov 03 22:00:00 GMT-12:00 2010

        As you can see, the hours are correct but the date is one day off.

        Show
        Bertrand Guay-Paquet added a comment - - edited Sample output: ERROR - DatePickerTest - =========== testDifferentDateTimeZoneConversion() ================= ERROR - DatePickerTest - orig: 1288951200000; date: 1288864800000; dateTime: 1288864800000 ERROR - DatePickerTest - orig: Thu Nov 04 22:00:00 GMT-12:00 2010; date: Wed Nov 03 22:00:00 GMT-12:00 2010; dateTime: Wed Nov 03 22:00:00 GMT-12:00 2010 As you can see, the hours are correct but the date is one day off.
        Hide
        Bertrand Guay-Paquet added a comment -

        test case demonstrating the problem

        Show
        Bertrand Guay-Paquet added a comment - test case demonstrating the problem

          People

          • Assignee:
            Igor Vaynberg
            Reporter:
            Bertrand Guay-Paquet
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development