Repository: amzn/ion-js
Pull request #596 (2020-03-27 17:01:57)

Fixed timezone issue in dom.Timestamp
*Issue #, if available:* #595 

*Description of changes:*

* When creating a `dom.Timestamp` from a provided `Date`, stores the UTC datetime represented by the `Date`. (See issue #595 for an explanation of why storing the `Date`'s offset is not meaningful.)
* Modifies an existing unit test to catch this sort of error in the event of a regression.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.


zslayton: >  can we still assert `domValue.dateValue()` behavior is correct?

I removed this check because `Value.from(myDate).dateValue()` is guaranteed to be `myDate`. However, I only know this is true because I wrote the implementation; it certainly doesn't hurt to test it. I've re-added the check.
 
> the new comment suggests passing in an ion.Timestamp -- do we have a test for this?

Yes, in a few places. For example, [these round-trip tests](https://github.com/amzn/ion-js/blob/42ce6a42a9293605f194049ee8b12146f822b89f/test/dom/Value.ts#L361-L363) create `dom.Timestamp`s from `ion.Timestamps`.

> we have a test for offset Z, but I'd suggest having a test for a non-zero offset

I've added a bunch of example `Date` tests to check for this and other corner cases. In the process, I fixed an issue that could crop with fractional seconds (which, it turns out, `ion.Timestamp` requires be specified as an `ion.Decimal`). 👍 

pbcornell: > In the process, I fixed an issue that could crop with fractional seconds (which, it turns out, `ion.Timestamp` requires be specified as an `ion.Decimal`). 👍

Ok great...but now I'm looking for tests that assert expected behavior for non-zero milliseconds (for Date), nor any tests for micro-/nano-seconds (when using Timestamp) and I don't see any.
zslayton: Changes:
* I refactored `exampleValues.ts` so it now has a list of ISO-8601 strings that is used to provide arrays of both sample `Date`s and `Timestamp`s.
* I added lots of different fractional-second ISO-8601 examples.
* I changed some existing `Timestamp` unit tests to use the new `exampleTimestampsWhere()` values, which should be much more comprehensive.



Patch:
diff --git a/src/dom/Timestamp.ts b/src/dom/Timestamp.ts
index 2b1fdc8..0e7dc15 100644
--- a/src/dom/Timestamp.ts
+++ b/src/dom/Timestamp.ts
@@ -1,6 +1,6 @@
 import {Value} from "./Value";
 import * as ion from "../Ion";
-import {IonTypes} from "../Ion";
+import {Decimal, IonTypes} from "../Ion";
 import {FromJsConstructor, FromJsConstructorBuilder} from "./FromJsConstructor";
 import {Writer} from "../Ion";
 
@@ -39,14 +39,24 @@ export class Timestamp extends Value(Date, IonTypes.TIMESTAMP, _fromJsConstructo
     }
 
     private static _timestampFromDate(date: Date): ion.Timestamp {
+        let milliseconds = (date.getUTCSeconds() * 1000) + date.getUTCMilliseconds();
+        let fractionalSeconds = new Decimal(milliseconds, -3);
+
+        // `Date` objects do not store a timezone offset. If the offset is requested
+        // via `.getTimezoneOffset()`, the configured offset of the host computer
+        // is returned instead of the timezone that was specified when the Date was
+        // constructed. Since the timezone of the host may or may not be meaningful
+        // to the user, we store the time in UTC instead. Users wishing to store a
+        // specific timezone offset on their dom.Timestamp can pass in an ion.Timestamp
+        // instead of a Date.
         return new ion.Timestamp(
-            date.getTimezoneOffset(),
+            0,
             date.getUTCFullYear(),
             date.getUTCMonth() + 1, // Timestamp expects a range of 1-12
             date.getUTCDate(),
             date.getUTCHours(),
             date.getUTCMinutes(),
-            date.getUTCSeconds() + (date.getUTCMilliseconds()/1000)
+            fractionalSeconds
         );
     }
 


Test patch:
diff --git a/test/dom/Value.ts b/test/dom/Value.ts
index 4e9a65b..19ef641 100644
--- a/test/dom/Value.ts
+++ b/test/dom/Value.ts
@@ -6,7 +6,7 @@ import JSBI from "jsbi";
 import * as JsValueConversion from "../../src/dom/JsValueConversion";
 import {_hasValue} from "../../src/util";
 import {Constructor} from "../../src/dom/Value";
-import {exampleIonValuesWhere, exampleJsValuesWhere} from "../exampleValues";
+import {exampleDatesWhere, exampleIonValuesWhere, exampleJsValuesWhere, exampleTimestampsWhere} from "../exampleValues";
 import {valueName} from "../mochaSupport";
 
 // The same test logic performed by assert.equals() but without an assertion.
@@ -165,19 +165,28 @@ describe('Value', () => {
                 (jsValue, domValue) => domValue.decimalValue().equals(jsValue)
             )
         );
-        describe('Date',
-            domValueTest(
-                new Date("1970-01-01T00:00:00Z"),
-                IonTypes.TIMESTAMP,
-                (jsValue, domValue) => domValue.dateValue().getTime() === jsValue.getTime()
-            )
+        describe('Date', () => {
+                for (let date of [...exampleDatesWhere(), new Date()]) {
+                    domValueTest(
+                        date,
+                        IonTypes.TIMESTAMP,
+                        (jsValue, domValue) => {
+                            return domValue.dateValue().getTime() == jsValue.getTime()
+                                && domValue.timestampValue().getDate().getTime() === jsValue.getTime();
+                        }
+                    )();
+                }
+            }
         );
-        describe('Timestamp',
-            domValueTest(
-                Timestamp.parse("1970-01-01T00:00:00Z"),
-                IonTypes.TIMESTAMP,
-                (jsValue, domValue) => domValue.timestampValue().equals(jsValue)
-            )
+        describe('Timestamp', () => {
+                for (let timestamp of exampleTimestampsWhere()) {
+                    domValueTest(
+                        timestamp,
+                        IonTypes.TIMESTAMP,
+                        (jsValue, domValue) => domValue.timestampValue().equals(jsValue)
+                    )();
+                }
+            }
         );
         describe('string',
             domValueTest(
@@ -351,10 +360,8 @@ describe('Value', () => {
         domRoundTripTest(
             'Timestamp',
             (t1, t2) => t1.timestampValue().equals(t2.timestampValue()),
-            new Date('1970-01-01T00:00:00.000Z'),
-            Timestamp.parse('1970-01-01T00:00:00.000Z'),
-            Timestamp.parse('2020-03-01T00:00:00.000+01:00'), // UTC 11PM Leap day
-            Timestamp.parse('2020-02-28T23:00:00.000-01:00')  // UTC midnight Leap day
+            ...exampleDatesWhere(),
+            ...exampleTimestampsWhere()
         );
         domRoundTripTest(
             'String',
diff --git a/test/exampleValues.ts b/test/exampleValues.ts
index b520ed0..f4740fc 100644
--- a/test/exampleValues.ts
+++ b/test/exampleValues.ts
@@ -41,4 +41,49 @@ export function exampleIonValuesWhere(filter: (v: Value) => boolean = acceptAnyV
         load('(1 2 3)')!, // s-expression
         load('{foo: true, bar: "Hello", baz: 5, qux: null}')! // struct
     ].filter(filter);
+}
+
+const _exampleIsoStrings: string[] = [
+    "1970-01-01T00:00:00Z",
+    '2020-02-28T23:00:00.000-01:00',
+    "2020-02-29T00:00:00Z",
+    "2020-02-29T00:00:00+01:00",
+    "2020-02-29T00:00:00-01:00",
+    '2020-03-01T00:00:00.000+01:00',
+    "2020-03-19T03:17:59.999Z",
+    "2020-03-19T03:17:59+03:21",
+    "2020-03-19T23:59:59-05:00",
+    "2020-03-19T23:01:01-08:00",
+    "2020-03-19T11:30:30-08:00",
+    "2020-03-19T11:30:30.5-08:00",
+    "2020-03-19T11:30:30.50-08:00",
+    "2020-03-19T11:30:30.500-08:00",
+    "2020-03-22T11:30:30.22-08:00",
+    "2020-03-27T00:00:00Z",
+    "2020-03-27T00:00:00.000Z",
+    "2020-03-27T12:00:00-05:00",
+    "2020-03-27T12:00:00-08:00",
+    "2020-03-27T12:00:00+01:00",
+    "2020-03-27T19:00:00-05:00",
+    "2020-03-27T16:00:00-08:00",
+    "2020-03-27T16:00:00.5-08:00",
+    "2020-03-28T01:00:00+01:00",
+    "2020-03-28T01:00:00.123456+01:00",
+    "2020-03-28T01:00:00.123456789+01:00",
+];
+
+// A common collection of Date values that can be reduced to a relevant subset using
+// the provided filter function.
+export function exampleDatesWhere(filter: (v: Date) => boolean = acceptAnyValue): Date[] {
+    return _exampleIsoStrings
+        .map((isoString) => new Date(isoString))
+        .filter(filter);
+}
+
+// A common collection of Timestamp values that can be reduced to a relevant subset using
+// the provided filter function.
+export function exampleTimestampsWhere(filter: (v: Timestamp) => boolean = acceptAnyValue): Timestamp[] {
+    return _exampleIsoStrings
+        .map((isoString) => Timestamp.parse(isoString)!)
+        .filter(filter);
 }
\ No newline at end of file