Add a flag to control whether DocumentSnapshots return Dates or Timestamps for timestamp fields (#831)

* add a new property `timestampsInSnapshotsEnabled` to `FirestoreSettings`, `false` by default;
* add a verbose warning message urging users to opt into the new behavior;
* set `timestampsInSnapshotsEnabled` to true in the integration tests to reduce the verbose console
spam during the test run and make sure the flag won't break anything once it's flipped.

9 files changed, 139 insertions, 49 deletions

diff --git a/Firestore/Source/API/FIRDocumentSnapshot.mm b/Firestore/Source/API/FIRDocumentSnapshot.mm
index 39d6af1..0fd59f4 100644
--- a/Firestore/Source/API/FIRDocumentSnapshot.mm
+++ b/Firestore/Source/API/FIRDocumentSnapshot.mm
@@ -18,6 +18,8 @@
 
 #include <utility>
 
+#import "FIRFirestoreSettings.h"
+
 #import "Firestore/Source/API/FIRDocumentReference+Internal.h"
 #import "Firestore/Source/API/FIRFieldPath+Internal.h"
 #import "Firestore/Source/API/FIRFirestore+Internal.h"
@@ -149,7 +151,10 @@ NS_ASSUME_NONNULL_BEGIN
   return self.internalDocument == nil
              ? nil
              : [self convertedObject:[self.internalDocument data]
-                             options:[FSTFieldValueOptions optionsForSnapshotOptions:options]];
+                             options:[FSTFieldValueOptions
+                                            optionsForSnapshotOptions:options
+                                         timestampsInSnapshotsEnabled:
+                                             self.firestore.settings.timestampsInSnapshotsEnabled]];
 }
 
 - (nullable id)valueForField:(id)field {
@@ -171,7 +176,10 @@ NS_ASSUME_NONNULL_BEGIN
   return fieldValue == nil
              ? nil
              : [self convertedValue:fieldValue
-                            options:[FSTFieldValueOptions optionsForSnapshotOptions:options]];
+                            options:[FSTFieldValueOptions
+                                           optionsForSnapshotOptions:options
+                                        timestampsInSnapshotsEnabled:
+                                            self.firestore.settings.timestampsInSnapshotsEnabled]];
 }
 
 - (nullable id)objectForKeyedSubscript:(id)key {
diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm
index f04bd8b..45d67cf 100644
--- a/Firestore/Source/API/FIRFirestore.mm
+++ b/Firestore/Source/API/FIRFirestore.mm
@@ -242,6 +242,34 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
       FSTAssert(_settings.host, @"FirestoreSettings.host cannot be nil.");
       FSTAssert(_settings.dispatchQueue, @"FirestoreSettings.dispatchQueue cannot be nil.");
 
+      if (!_settings.timestampsInSnapshotsEnabled) {
+        FSTWarn(
+            @"The behavior for system Date objects stored in Firestore is going to change "
+             "AND YOUR APP MAY BREAK.\n"
+             "To hide this warning and ensure your app does not break, you need to add "
+             "the following code to your app before calling any other Cloud Firestore methods:\n"
+             "\n"
+             "let db = Firestore.firestore()\n"
+             "let settings = db.settings\n"
+             "settings.areTimestampsInSnapshotsEnabled = true\n"
+             "db.settings = settings\n"
+             "\n"
+             "With this change, timestamps stored in Cloud Firestore will be read back as "
+             "Firebase Timestamp objects instead of as system Date objects. So you will "
+             "also need to update code expecting a Date to instead expect a Timestamp. "
+             "For example:\n"
+             "\n"
+             "// old:\n"
+             "let date: Date = documentSnapshot.get(\"created_at\") as! Date\n"
+             "// new:\n"
+             "let timestamp: Timestamp = documentSnapshot.get(\"created_at\") as! Timestamp\n"
+             "let date: Date = timestamp.dateValue()\n"
+             "\n"
+             "Please audit all existing usages of Date when you enable the new behavior. In a "
+             "future release, the behavior will be changed to the new behavior, so if you do not "
+             "follow these steps, YOUR APP MAY BREAK.");
+      }
+
       const DatabaseInfo database_info(*self.databaseID, util::MakeStringView(_persistenceKey),
                                        util::MakeStringView(_settings.host), _settings.sslEnabled);
 
diff --git a/Firestore/Source/API/FIRFirestoreSettings.mm b/Firestore/Source/API/FIRFirestoreSettings.mm
index 9677ff6..8f998ec 100644
--- a/Firestore/Source/API/FIRFirestoreSettings.mm
+++ b/Firestore/Source/API/FIRFirestoreSettings.mm
@@ -23,6 +23,8 @@ NS_ASSUME_NONNULL_BEGIN
 static NSString *const kDefaultHost = @"firestore.googleapis.com";
 static const BOOL kDefaultSSLEnabled = YES;
 static const BOOL kDefaultPersistenceEnabled = YES;
+// TODO(b/73820332): flip the default.
+static const BOOL kDefaultTimestampsInSnapshotsEnabled = NO;
 
 @implementation FIRFirestoreSettings
 
@@ -32,6 +34,7 @@ static const BOOL kDefaultPersistenceEnabled = YES;
     _sslEnabled = kDefaultSSLEnabled;
     _dispatchQueue = dispatch_get_main_queue();
     _persistenceEnabled = kDefaultPersistenceEnabled;
+    _timestampsInSnapshotsEnabled = kDefaultTimestampsInSnapshotsEnabled;
   }
   return self;
 }
@@ -47,7 +50,8 @@ static const BOOL kDefaultPersistenceEnabled = YES;
   return [self.host isEqual:otherSettings.host] &&
          self.isSSLEnabled == otherSettings.isSSLEnabled &&
          self.dispatchQueue == otherSettings.dispatchQueue &&
-         self.isPersistenceEnabled == otherSettings.isPersistenceEnabled;
+         self.isPersistenceEnabled == otherSettings.isPersistenceEnabled &&
+         self.timestampsInSnapshotsEnabled == otherSettings.timestampsInSnapshotsEnabled;
 }
 
 - (NSUInteger)hash {
@@ -55,6 +59,7 @@ static const BOOL kDefaultPersistenceEnabled = YES;
   result = 31 * result + (self.isSSLEnabled ? 1231 : 1237);
   // Ignore the dispatchQueue to avoid having to deal with sizeof(dispatch_queue_t).
   result = 31 * result + (self.isPersistenceEnabled ? 1231 : 1237);
+  result = 31 * result + (self.timestampsInSnapshotsEnabled ? 1231 : 1237);
   return result;
 }
 
@@ -64,6 +69,7 @@ static const BOOL kDefaultPersistenceEnabled = YES;
   copy.sslEnabled = _sslEnabled;
   copy.dispatchQueue = _dispatchQueue;
   copy.persistenceEnabled = _persistenceEnabled;
+  copy.timestampsInSnapshotsEnabled = _timestampsInSnapshotsEnabled;
   return copy;
 }
 
diff --git a/Firestore/Source/API/FIRTimestamp.m b/Firestore/Source/API/FIRTimestamp.m
index 489b921..e5a2cd3 100644
--- a/Firestore/Source/API/FIRTimestamp.m
+++ b/Firestore/Source/API/FIRTimestamp.m
@@ -121,7 +121,7 @@ static const int kNanosPerSecond = 1000000000;
 
 #pragma mark - Public methods
 
-- (NSDate *)approximateDateValue {
+- (NSDate *)dateValue {
   NSTimeInterval interval = (NSTimeInterval)self.seconds + ((NSTimeInterval)self.nanoseconds) / 1e9;
   return [NSDate dateWithTimeIntervalSince1970:interval];
 }
diff --git a/Firestore/Source/Model/FSTFieldValue.h b/Firestore/Source/Model/FSTFieldValue.h
index 7d72138..6914f4d 100644
--- a/Firestore/Source/Model/FSTFieldValue.h
+++ b/Firestore/Source/Model/FSTFieldValue.h
@@ -55,6 +55,8 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
 
 @property(nonatomic, readonly, assign) FSTServerTimestampBehavior serverTimestampBehavior;
 
+@property(nonatomic) BOOL timestampsInSnapshotsEnabled;
+
 - (instancetype)init NS_UNAVAILABLE;
 
 /**
@@ -62,10 +64,12 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
  * server timestamps.
  */
 - (instancetype)initWithServerTimestampBehavior:(FSTServerTimestampBehavior)serverTimestampBehavior
+                   timestampsInSnapshotsEnabled:(BOOL)timestampsInSnapshotsEnabled
     NS_DESIGNATED_INITIALIZER;
 
-/** Creates an FSTFieldValueOption instance from FIRSnapshotOptions. */
-+ (instancetype)optionsForSnapshotOptions:(FIRSnapshotOptions *)value;
+/** Creates an FSTFieldValueOptions instance from FIRSnapshotOptions. */
++ (instancetype)optionsForSnapshotOptions:(FIRSnapshotOptions *)value
+             timestampsInSnapshotsEnabled:(BOOL)timestampsInSnapshotsEnabled;
 
 @end
 
@@ -163,9 +167,8 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
 /**
  * A timestamp value stored in Firestore.
  */
-@interface FSTTimestampValue : FSTFieldValue <NSDate *>
+@interface FSTTimestampValue : FSTFieldValue <FIRTimestamp *>
 + (instancetype)timestampValue:(FIRTimestamp *)value;
-- (FIRTimestamp *)internalValue;
 @end
 
 /**
@@ -194,7 +197,6 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
  */
 @interface FSTGeoPointValue : FSTFieldValue <FIRGeoPoint *>
 + (instancetype)geoPointValue:(FIRGeoPoint *)value;
-- (FIRGeoPoint *)valueWithOptions:(FSTFieldValueOptions *)options;
 @end
 
 /**
@@ -202,7 +204,6 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
  */
 @interface FSTBlobValue : FSTFieldValue <NSData *>
 + (instancetype)blobValue:(NSData *)value;
-- (NSData *)valueWithOptions:(FSTFieldValueOptions *)options;
 @end
 
 /**
@@ -211,7 +212,6 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
 @interface FSTReferenceValue : FSTFieldValue <FSTDocumentKey *>
 + (instancetype)referenceValue:(FSTDocumentKey *)value
                     databaseID:(const firebase::firestore::model::DatabaseId *)databaseID;
-- (FSTDocumentKey *)valueWithOptions:(FSTFieldValueOptions *)options;
 // Does not own this DatabaseId.
 @property(nonatomic, assign, readonly) const firebase::firestore::model::DatabaseId *databaseID;
 @end
@@ -239,7 +239,6 @@ typedef NS_ENUM(NSInteger, FSTServerTimestampBehavior) {
 - (instancetype)initWithImmutableDictionary:
     (FSTImmutableSortedDictionary<NSString *, FSTFieldValue *> *)value NS_DESIGNATED_INITIALIZER;
 
-- (NSDictionary<NSString *, id> *)valueWithOptions:(FSTFieldValueOptions *)options;
 - (FSTImmutableSortedDictionary<NSString *, FSTFieldValue *> *)internalValue;
 
 /** Returns the value at the given path if it exists. Returns nil otherwise. */
diff --git a/Firestore/Source/Model/FSTFieldValue.mm b/Firestore/Source/Model/FSTFieldValue.mm
index 2f013c3..80bd11f 100644
--- a/Firestore/Source/Model/FSTFieldValue.mm
+++ b/Firestore/Source/Model/FSTFieldValue.mm
@@ -48,28 +48,35 @@ NS_ASSUME_NONNULL_BEGIN
 
 @implementation FSTFieldValueOptions
 
-+ (instancetype)optionsForSnapshotOptions:(FIRSnapshotOptions *)options {
-  if (options.serverTimestampBehavior == FSTServerTimestampBehaviorNone) {
-    static FSTFieldValueOptions *defaultInstance = nil;
-    static dispatch_once_t onceToken;
-
-    dispatch_once(&onceToken, ^{
-      defaultInstance = [[FSTFieldValueOptions alloc]
-          initWithServerTimestampBehavior:FSTServerTimestampBehaviorNone];
-    });
-    return defaultInstance;
-  } else {
-    return [[FSTFieldValueOptions alloc]
-        initWithServerTimestampBehavior:options.serverTimestampBehavior];
++ (instancetype)optionsForSnapshotOptions:(FIRSnapshotOptions *)options
+             timestampsInSnapshotsEnabled:(BOOL)timestampsInSnapshotsEnabled {
+  FSTServerTimestampBehavior convertedServerTimestampBehavior = FSTServerTimestampBehaviorNone;
+  switch (options.serverTimestampBehavior) {
+    case FIRServerTimestampBehaviorNone:
+      convertedServerTimestampBehavior = FSTServerTimestampBehaviorNone;
+      break;
+    case FIRServerTimestampBehaviorEstimate:
+      convertedServerTimestampBehavior = FSTServerTimestampBehaviorEstimate;
+      break;
+    case FIRServerTimestampBehaviorPrevious:
+      convertedServerTimestampBehavior = FSTServerTimestampBehaviorPrevious;
+      break;
+    default:
+      FSTFail(@"Unexpected server timestamp option: %ld", (long)options.serverTimestampBehavior);
   }
+
+  return
+      [[FSTFieldValueOptions alloc] initWithServerTimestampBehavior:convertedServerTimestampBehavior
+                                       timestampsInSnapshotsEnabled:timestampsInSnapshotsEnabled];
 }
 
-- (instancetype)initWithServerTimestampBehavior:
-    (FSTServerTimestampBehavior)serverTimestampBehavior {
+- (instancetype)initWithServerTimestampBehavior:(FSTServerTimestampBehavior)serverTimestampBehavior
+                   timestampsInSnapshotsEnabled:(BOOL)timestampsInSnapshotsEnabled {
   self = [super init];
 
   if (self) {
     _serverTimestampBehavior = serverTimestampBehavior;
+    _timestampsInSnapshotsEnabled = timestampsInSnapshotsEnabled;
   }
   return self;
 }
@@ -89,12 +96,11 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (id)value {
-  return [self valueWithOptions:[FSTFieldValueOptions
-                                    optionsForSnapshotOptions:[FIRSnapshotOptions defaultOptions]]];
+  @throw FSTAbstractMethodException();  // NOLINT
 }
 
 - (id)valueWithOptions:(FSTFieldValueOptions *)options {
-  @throw FSTAbstractMethodException();  // NOLINT
+  return [self value];
 }
 
 - (BOOL)isEqual:(id)other {
@@ -143,7 +149,7 @@ NS_ASSUME_NONNULL_BEGIN
   return FSTTypeOrderNull;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return [NSNull null];
 }
 
@@ -209,7 +215,7 @@ NS_ASSUME_NONNULL_BEGIN
   return FSTTypeOrderBoolean;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return self.internalValue ? @YES : @NO;
 }
 
@@ -290,7 +296,7 @@ NS_ASSUME_NONNULL_BEGIN
   return self;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return @(self.internalValue);
 }
 
@@ -342,7 +348,7 @@ NS_ASSUME_NONNULL_BEGIN
   return self;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return @(self.internalValue);
 }
 
@@ -400,7 +406,7 @@ struct Comparator<NSString *> {
   return FSTTypeOrderString;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return self.internalValue;
 }
 
@@ -447,9 +453,16 @@ struct Comparator<NSString *> {
   return FSTTypeOrderTimestamp;
 }
 
+- (id)value {
+  return self.internalValue;
+}
+
 - (id)valueWithOptions:(FSTFieldValueOptions *)options {
-  // For developers, we expose Timestamps as Dates.
-  return self.internalValue.approximateDateValue;
+  if (options.timestampsInSnapshotsEnabled) {
+    return self.value;
+  } else {
+    return [self.value dateValue];
+  }
 }
 
 - (BOOL)isEqual:(id)other {
@@ -473,7 +486,6 @@ struct Comparator<NSString *> {
 }
 
 @end
-
 #pragma mark - FSTServerTimestampValue
 
 @implementation FSTServerTimestampValue
@@ -498,16 +510,20 @@ struct Comparator<NSString *> {
   return FSTTypeOrderTimestamp;
 }
 
+- (id)value {
+  return [NSNull null];
+}
+
 - (id)valueWithOptions:(FSTFieldValueOptions *)options {
   switch (options.serverTimestampBehavior) {
     case FSTServerTimestampBehaviorNone:
       return [NSNull null];
     case FSTServerTimestampBehaviorEstimate:
-      return [self.localWriteTime approximateDateValue];
+      return [[FSTTimestampValue timestampValue:self.localWriteTime] valueWithOptions:options];
     case FSTServerTimestampBehaviorPrevious:
       return self.previousValue ? [self.previousValue valueWithOptions:options] : [NSNull null];
     default:
-      FSTFail(@"Unexpected server timestamp option: %d", (int)options.serverTimestampBehavior);
+      FSTFail(@"Unexpected server timestamp option: %ld", (long)options.serverTimestampBehavior);
   }
 }
 
@@ -561,7 +577,7 @@ struct Comparator<NSString *> {
   return FSTTypeOrderGeoPoint;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return self.internalValue;
 }
 
@@ -583,7 +599,6 @@ struct Comparator<NSString *> {
 }
 
 @end
-
 #pragma mark - FSTBlobValue
 
 static NSComparisonResult CompareBytes(NSData *left, NSData *right) {
@@ -625,7 +640,7 @@ static NSComparisonResult CompareBytes(NSData *left, NSData *right) {
   return FSTTypeOrderBlob;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return self.internalValue;
 }
 
@@ -669,7 +684,7 @@ static NSComparisonResult CompareBytes(NSData *left, NSData *right) {
   return self;
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   return self.key;
 }
 
@@ -753,6 +768,15 @@ static const NSComparator StringComparator = ^NSComparisonResult(NSString *left,
   return [self initWithImmutableDictionary:dictionary];
 }
 
+- (id)value {
+  NSMutableDictionary *result = [NSMutableDictionary dictionary];
+  [self.internalValue
+      enumerateKeysAndObjectsUsingBlock:^(NSString *key, FSTFieldValue *obj, BOOL *stop) {
+        result[key] = [obj value];
+      }];
+  return result;
+}
+
 - (id)valueWithOptions:(FSTFieldValueOptions *)options {
   NSMutableDictionary *result = [NSMutableDictionary dictionary];
   [self.internalValue
@@ -832,8 +856,8 @@ static const NSComparator StringComparator = ^NSComparisonResult(NSString *left,
     // Recursive base case:
     return [self objectBySettingValue:value forField:childName];
   } else {
-    // Nested path. Recursively generate a new sub-object and then wrap a new FSTObjectValue around
-    // the result.
+    // Nested path. Recursively generate a new sub-object and then wrap a new FSTObjectValue
+    // around the result.
     FSTFieldValue *child = [_internalValue objectForKey:childName];
     FSTObjectValue *childObject;
     if ([child isKindOfClass:[FSTObjectValue class]]) {
@@ -908,7 +932,7 @@ static const NSComparator StringComparator = ^NSComparisonResult(NSString *left,
   return [self.internalValue hash];
 }
 
-- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+- (id)value {
   NSMutableArray *result = [NSMutableArray arrayWithCapacity:_internalValue.count];
   [self.internalValue enumerateObjectsUsingBlock:^(FSTFieldValue *obj, NSUInteger idx, BOOL *stop) {
     [result addObject:[obj value]];
@@ -916,6 +940,14 @@ static const NSComparator StringComparator = ^NSComparisonResult(NSString *left,
   return result;
 }
 
+- (id)valueWithOptions:(FSTFieldValueOptions *)options {
+  NSMutableArray *result = [NSMutableArray arrayWithCapacity:_internalValue.count];
+  [self.internalValue enumerateObjectsUsingBlock:^(FSTFieldValue *obj, NSUInteger idx, BOOL *stop) {
+    [result addObject:[obj valueWithOptions:options]];
+  }];
+  return result;
+}
+
 - (FSTTypeOrder)typeOrder {
   return FSTTypeOrderArray;
 }
diff --git a/Firestore/Source/Public/FIRFirestoreSettings.h b/Firestore/Source/Public/FIRFirestoreSettings.h
index 7a1f2a3..cd3f91c 100644
--- a/Firestore/Source/Public/FIRFirestoreSettings.h
+++ b/Firestore/Source/Public/FIRFirestoreSettings.h
@@ -44,6 +44,23 @@ NS_SWIFT_NAME(FirestoreSettings)
 /** Set to false to disable local persistent storage. */
 @property(nonatomic, getter=isPersistenceEnabled) BOOL persistenceEnabled;
 
+/**
+ * Enables the use of FIRTimestamps for timestamp fields in FIRDocumentSnapshots.
+ *
+ * Currently, Firestore returns timestamp fields as an NSDate but NSDate is implemented as a double
+ * which loses precision and causes unexpected behavior when using a timestamp from a snapshot as
+ * a part of a subsequent query.
+ *
+ * Setting timestampsInSnapshotsEnabled to true will cause Firestore to return FIRTimestamp values
+ * instead of NSDate, avoiding this kind of problem. To make this work you must also change any code
+ * that uses NSDate to use FIRTimestamp instead.
+ *
+ * NOTE: in the future timestampsInSnapshotsEnabled = true will become the default and this option
+ * will be removed so you should change your code to use FIRTimestamp now and opt-in to this new
+ * behavior as soon as you can.
+ */
+@property(nonatomic, getter=areTimestampsInSnapshotsEnabled) BOOL timestampsInSnapshotsEnabled;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/Firestore/Source/Public/FIRTimestamp.h b/Firestore/Source/Public/FIRTimestamp.h
index d0a77f3..bf4aff4 100644
--- a/Firestore/Source/Public/FIRTimestamp.h
+++ b/Firestore/Source/Public/FIRTimestamp.h
@@ -60,7 +60,7 @@ NS_SWIFT_NAME(Timestamp)
 + (instancetype)timestamp;
 
 /** Returns a new NSDate corresponding to this timestamp. This may lose precision. */
-- (NSDate *)approximateDateValue;
+- (NSDate *)dateValue;
 
 - (NSComparisonResult)compare:(FIRTimestamp *)other;
 
diff --git a/Firestore/Source/Remote/FSTSerializerBeta.mm b/Firestore/Source/Remote/FSTSerializerBeta.mm
index 9531ddc..3a22a3f 100644
--- a/Firestore/Source/Remote/FSTSerializerBeta.mm
+++ b/Firestore/Source/Remote/FSTSerializerBeta.mm
@@ -191,7 +191,7 @@ NS_ASSUME_NONNULL_BEGIN
     return [self encodedString:[fieldValue value]];
 
   } else if (fieldClass == [FSTTimestampValue class]) {
-    return [self encodedTimestampValue:((FSTTimestampValue *)fieldValue).internalValue];
+    return [self encodedTimestampValue:[fieldValue value]];
 
   } else if (fieldClass == [FSTGeoPointValue class]) {
     return [self encodedGeoPointValue:[fieldValue value]];