Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cloud_firestore): correct nanoseconds calculation for pre-1970 dates #17195

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,22 @@ void runTimestampTests() {
equals(date.millisecondsSinceEpoch),
);
});

test('set pre-1970 $Timestamp and return', () async {
DocumentReference<Map<String, dynamic>> doc =
await initializeTest('timestamp');
final date = DateTime(1969, 06, 22, 0, 0, 0, 123);
final localTimestamp = Timestamp.fromDate(date);

await doc.set({'foo': localTimestamp});

DocumentSnapshot<Map<String, dynamic>> snapshot = await doc.get();
Timestamp retievedTimestamp = snapshot.data()!['foo'];
expect(retievedTimestamp, isA<Timestamp>());
expect(
retievedTimestamp,
equals(localTimestamp),
);
});
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,15 @@ class Timestamp implements Comparable<Timestamp> {

/// Create a [Timestamp] fromMicrosecondsSinceEpoch
factory Timestamp.fromMicrosecondsSinceEpoch(int microseconds) {
final int seconds = microseconds ~/ _kMillion;
final int nanoseconds = (microseconds - seconds * _kMillion) * _kThousand;
int seconds = microseconds ~/ _kMillion;
int nanoseconds = (microseconds - seconds * _kMillion) * _kThousand;

// Matches implementation in Android SDK:
// https://github.com/firebase/firebase-android-sdk/blob/master/firebase-common/src/main/java/com/google/firebase/Timestamp.kt#L114-L121
if (nanoseconds < 0) {
seconds -= 1;
nanoseconds += _kBillion;
}
Comment on lines -43 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SelaseKay
Also the factory fromMillisecondsSinceEpoch should be fixed.
I personally would delegate the milliseconds one to the microseconds one:

factory Timestamp.fromMillisecondsSinceEpoch(int milliseconds) {
  return fromMicrosecondsSinceEpoch(milliseconds * _kThousand);
}

but I don't know if there are other considerations to be done

return Timestamp(seconds, nanoseconds);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,34 @@ void main() {

expect(epoch, equals(-9999999999));
});

test('Timestamp should not throw for dates before 1970', () {
final dates = [
DateTime(1969, 06, 22, 0, 0, 0, 123),
DateTime(1969, 12, 31, 23, 59, 59, 999),
DateTime(1900, 01, 01, 12, 30, 45, 500),
DateTime(1800, 07, 04, 18, 15, 30, 250),
DateTime(0001, 01, 01, 00, 00, 00, 001),
];

for (final date in dates) {
try {
final timestamp = Timestamp.fromDate(date);
expect(timestamp, isA<Timestamp>());
} catch (e) {
fail('Timestamp.fromDate threw an error: $e');
}
}
});
Comment on lines +84 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to go from Date to Timestamp to Date and check that the value is still correct.
You are missing the second half of the test.
You should also have the same test were you store it in Firestore and then read firestore to see if it was correctly stored.


test(
'pre-1970 Timestamps should match the original DateTime after conversion',
() {
final date = DateTime(1969, 06, 22, 0, 0, 0, 123);
final timestamp = Timestamp.fromDate(date);
final timestampAsDateTime = timestamp.toDate();

expect(date, equals(timestampAsDateTime));
});
});
}
Loading