#229 CommonTS.getValueAsCalendar problems with positive GMT offsets and DST

[Rolf Thorup]

When a CommonTS is constructed with positive gmtOffset, depending on the default timezone and the date the question the CommonTS.getValueAsCalendar() method returns a Calendar that is off by one or more hours. The attached test cased highlights the problems.

The testcases construct CommonTS objects using the setValue(Calendar) and when I'm in the following talking about the tested timezone, it means the timezone this Calendar is initialized with.

The culprit is the CommonTM.getValueAsCalendar method which constructs a Calendar implicitly based on the default timezone. The testcases reveals three problems. :

1. When the tested timezone and the default timezone agrees on the raw gmtOffset but the default timezone has useDaylightSavings: The method returns an off-by-one hour Calendar when the tested date is within the DST range (we never enter the if-block).
2. (Related to 1.) When the raw gmtOffset of tested timezone is one hour behind the default timezone but the tested timezone has useDaylightSavings: The method returns an off-by-one hour Calendar when the tested date is within the DST range (we never enter the if-block).
3. When getting into the if-block, if the constructed GMT offset is to be positive, the returned Calendar is off by the GMT-offset of the default timezone, as the constructed ID misses a '+' and TimeZone.getTimeZone returns "GMT".

Some examples:
Default timezone: Europe/Copenhagen (GMT+01:00 AND uses daylight savings)
Tested timezone: GMT+01:00
Date tested: 2015-06-01, i.e DST is active

Returned Calendar is off by one hour as it has been constructed with default timezone which has DST.

Default timezone: Europe/Copenhagen (GMT+01:00 AND uses daylight savings)
Tested timezone: Europe/London (GMT AND uses daylight savings)
Date tested: 2015-06-01, i.e DST is active

Returned Calendar is off by one hour as it has been constructed with default timezone which has DST.

Default timezone: Europe/London (GMT AND uses daylight savings)
Tested timezone: Europe/London (GMT AND uses daylight savings)
Date tested: 2015-06-01, i.e DST is active

The returned Calendar is off one by as we enter the if-block but the constructed ZoneID is invalid.

A final note: when I'm comparing calendars, I'm actually comparing the Date returned by Calendar.getTime().

[Christian Ohr]

Great input.
Obviously, fixing the Timezone string for positive offsets makes most of your test configurations run.

The default calendar timezone is somewhat harder to tackle; mostly due to the dumb fact that HL7 allows plain TMs together with (implicit) timezone information. And it's impossible to derive DST from just having a time and not a full date.
Any ideas?

[Rolf Thorup]

Thank you for the answer and for looking at this issue.

I have an idea, but I need to make some further investigations. I'll return shortly.

[Rolf Thorup]

Instead of starting with Calendar.getInstance(), construct a Calendar based on gmtOffset and only use a default constructed Calendar as a fallback; see suggested code below.

In the below I have also slightly modified the timezone ID generation logic:

• if hrOffset is negative tzBuilder.append(hrOffset) will automatically insert a '-'.
• minOffset must be an absolute value otherwise the ID generated for the imaginary timezone with gmtOffset -530 becomes 'GMT-5:-30'.

The following makes both the existing tests and my attached tests run without errors:

public Calendar getValueAsCalendar() {
    int gmtOff = getGMTOffset();
    Calendar retVal;
    if (gmtOff != GMT_OFFSET_NOT_SET_VALUE && !omitOffsetFg) {
        int hrOffset = gmtOff / 100;
        int minOffset = Math.abs(gmtOff % 100);
        StringBuilder tzBuilder = new StringBuilder("GMT");

        if (hrOffset >= 0) {
            tzBuilder.append('+');
        }
        tzBuilder.append(hrOffset);
        tzBuilder.append(':');
        if (minOffset < 10) {
            tzBuilder.append('0');
        }
        tzBuilder.append(minOffset);
        retVal = new GregorianCalendar(TimeZone.getTimeZone(tzBuilder.toString()));
    } else {
        retVal = Calendar.getInstance();
    }

    retVal.set(Calendar.HOUR_OF_DAY, getHour());
    retVal.set(Calendar.MINUTE, getMinute());
    retVal.set(Calendar.SECOND, getSecond());
    float fractSecond = getFractSecond();
    retVal.set(Calendar.MILLISECOND, (int) Math.round(fractSecond * 1000.0));

    return retVal;
}

.

Unrelated to this particularly issue: the .classpath file for both the hapi-base and hapi-test Eclipse projects references some slf4j jar files in version 1.6.6. These entries must be removed in order for Eclipse to work. Additionally hapi-test references velocity-1.6.3.jar which also must removed from the .classpath file.

[Christian Ohr]

Oh cool. I will look into this.
The .project files should probably not checked in at all

[Christian Ohr]

Verfied and comitted your fix except that I chose to construct the time zone string using string formatter. I hope it's ok that I added your test cases as well to prevent regressions.
Thanks again for your effort!