Repository: amzn/ion-java (java)

Issue:
Timestamp.getMillis() returns wrong value when Timestamp was constructed via forCalendar()
The instant returned by `Timestamp.getMillis()` is incorrect when constructed via `Timestamp.forCalendar()` with a Calendar that is offset from UTC.  For example, the following code:

```java
Date now = new Date();
Calendar cal = Calendar.getInstance();
cal.setTimeZone(TimeZone.getTimeZone("GMT+02:00"));
cal.setTime(now);
Timestamp ts = Timestamp.forCalendar(cal);
System.out.println(now.getTime());
System.out.println(ts.getMillis());
```

incorrectly produces:
```
1549646861615
1549639661615
```

These epoch values should be identical.

Pull request:
fixes getMillis() and getDecimalMillis() behavior for Timestamps cons…
…tructed via forCalendar()

If the incoming Calendar has an offset from UTC, Timestamp rolls the hours and minutes of its internal Calendar instance such that the fields represent a UTC time, with the side-effect of *changing the instant (ms) the Calendar represents* resulting in wrong values being returned by `Timestamp.getMillis()` and `Timestamp.getDecimalMillis()`.

The changes proposed here calculate a _calendarCompensationOffsetMs so that `getMillis()` and `getDecimalMillis()` can return the correct instant.

Resolves #212 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/src/software/amazon/ion/Timestamp.java b/src/software/amazon/ion/Timestamp.java
index 62984161..eb1bc13b 100644
--- a/src/software/amazon/ion/Timestamp.java
+++ b/src/software/amazon/ion/Timestamp.java
@@ -166,6 +166,9 @@ public final class Timestamp
     /**
      * Calendar to hold the Timestamp's year, month, day, hour, minute, second, and calendar system. Fractional
      * seconds are left to {@link #_fraction}, while local offset is left to {@link #_offset}.
+     * <p>
+     * This calendar instance is adjusted such that it is always in UTC;  see _calendarCompensationOffsetMs
+     * if the original "instant" millis is needed.
      */
     private final Calendar _calendar;
 
@@ -180,6 +183,12 @@ public final class Timestamp
      */
     private Integer     _offset;
 
+    /**
+     * Number of milliseconds _calendar has been adjusted so that it is in UTC.
+     * Allows getMillis() and getDecimalMillis() to return the correct response.
+     */
+    private int _calendarCompensationOffsetMs;
+
     // Minimum millis under the calendar system provided by the default GregorianCalendar implementation.
     private static final long MINIMUM_TIMESTAMP_IN_MILLIS = Timestamp.valueOf("0001-01-01T00:00:00.000Z").getMillis();
     static final BigDecimal MINIMUM_TIMESTAMP_IN_MILLIS_DECIMAL = new BigDecimal(MINIMUM_TIMESTAMP_IN_MILLIS);
@@ -206,12 +215,20 @@ public final class Timestamp
         offset = -offset;
         int hour_offset = offset / 60;
         int min_offset = offset - (hour_offset * 60);
+
+        // When we add hour_offset and min_offset to _calendar below, the underlying millis "instant"
+        // is changed.  To compensate, we set _calendarCompensationOffsetMs here and use it to
+        // recalculate the correct millis if/when needed
+        if (_calendar.isSet(Calendar.ZONE_OFFSET) || _calendar.isSet(Calendar.DST_OFFSET)) {
+            _calendarCompensationOffsetMs = -offset * 60 * 1000;
+        }
+
         // First, clear the offsets that are already set. Otherwise, the 'add' calls will add them in, which will
         // result in a double add.
         _calendar.clear(Calendar.ZONE_OFFSET);
         _calendar.clear(Calendar.DST_OFFSET);
-        _calendar.add(Calendar.MINUTE, min_offset);
         _calendar.add(Calendar.HOUR_OF_DAY, hour_offset);
+        _calendar.add(Calendar.MINUTE, min_offset);
     }
 
     /**
@@ -1341,7 +1358,7 @@ public final class Timestamp
             int frac = isIntegralZero(fracAsDecimal) ? 0 : fracAsDecimal.intValue();
             millis += frac;
         }
-        return millis;
+        return millis + _calendarCompensationOffsetMs;
 
     }
 
@@ -1365,7 +1382,7 @@ public final class Timestamp
         case DAY:
         case MINUTE:
         case SECOND:
-            long millis = _calendar.getTimeInMillis();
+            long millis = _calendar.getTimeInMillis() + _calendarCompensationOffsetMs;
             BigDecimal dec = BigDecimal.valueOf(millis);
             if (_fraction != null) {
                 dec = dec.add(this._fraction.movePointRight(3));


Test patch:
diff --git a/test/software/amazon/ion/TimestampTest.java b/test/software/amazon/ion/TimestampTest.java
index 5893f274..ecf86dff 100644
--- a/test/software/amazon/ion/TimestampTest.java
+++ b/test/software/amazon/ion/TimestampTest.java
@@ -30,6 +30,7 @@ import static software.amazon.ion.impl.PrivateUtils.UTC;
 
 import java.math.BigDecimal;
 import java.text.DateFormat;
+import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.time.Instant;
 import java.time.ZoneId;
@@ -2870,6 +2871,31 @@ public class TimestampTest
         Timestamp.forMillis(LARGE_SCALE_DECIMAL, PST_OFFSET).getMillis();
     }
 
+    @Test
+    public void testMillisIsIndepedentOfOffset() throws ParseException {
+        DateFormat format = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ");
+        Date date = format.parse("2010-06-29T23:20:00+0000");
+
+        Calendar cal1 = Calendar.getInstance();
+        cal1.setTimeZone(TimeZone.getTimeZone("GMT+00:00"));
+        cal1.setTime(date);
+        Timestamp t1 = Timestamp.forCalendar(cal1);
+
+        Calendar cal2 = Calendar.getInstance();
+        cal2.setTimeZone(TimeZone.getTimeZone("GMT+02:00"));
+        cal2.setTime(date);
+        Timestamp t2 = Timestamp.forCalendar(cal2);
+
+        assertEquals(t1.getMillis(), date.getTime());
+        assertEquals(t2.getMillis(), date.getTime());
+
+        assertEquals(BigDecimal.valueOf(t1.getMillis()), t1.getDecimalMillis());
+        assertEquals(BigDecimal.valueOf(t2.getMillis()), t2.getDecimalMillis());
+
+        assertEquals(t1.getMillis(), t2.getMillis());
+        assertEquals(t1.getDecimalMillis(), t2.getDecimalMillis());
+    }
+
     @Test
     public void testInstantVsTimestampMillis() {
         // addresses: https://github.com/amzn/ion-java/issues/165