Change timestampsInSnapshots default to true and deprecate the setting. (#2251)

Author: mikelehen

Firestore/CHANGELOG.md

@@ -1,4 +1,8 @@
 # Unreleased
+- [changed] The `areTimestampsInSnapshotsEnabled` setting is now enabled by
+  default so timestamp fields read from a FIRDocumentSnapshot will be returned
+  as FIRTimestamp objects instead of NSDate. Any code expecting to receive an
+  NSDate object must be updated.
 - [changed] `Transaction.getDocument()` has been changed to return a non-nil
   `DocumentSnapshot` with `exists` equal to `false` if the document does not
   exist (instead of returning a nil DocumentSnapshot). Code that was previously

Firestore/Example/Tests/Integration/API/FIRFieldsTests.mm

@@ -233,8 +233,6 @@ - (FIRDocumentSnapshot *)snapshotWithTimestamps:(FIRTimestamp *)timestamp {
   return [self readDocumentForRef:doc];
 }
 
-// Note: timestampsInSnapshotsEnabled is set to "true" in FSTIntegrationTestCase, so this test is
-// not affected by the current default in FIRFirestoreSettings.
 - (void)testTimestampsInSnapshots {
   FIRTimestamp *originalTimestamp = [FIRTimestamp timestampWithSeconds:100 nanoseconds:123456789];
   FIRDocumentReference *doc = [self documentRef];
@@ -268,7 +266,10 @@ - (void)setUp {
   [super setUp];
   // Settings can only be redefined before client is initialized, so this has to happen in setUp.
   FIRFirestoreSettings *settings = self.db.settings;
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
   settings.timestampsInSnapshotsEnabled = NO;
+#pragma clang diagnostic pop
   self.db.settings = settings;
 }
 

Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm

@@ -115,7 +115,6 @@ - (FIRFirestore *)firestore {
 + (void)setUpDefaults {
   defaultSettings = [[FIRFirestoreSettings alloc] init];
   defaultSettings.persistenceEnabled = YES;
-  defaultSettings.timestampsInSnapshotsEnabled = YES;
 
   // Check for a MobileHarness configuration, running against nightly or prod, which have live
   // SSL certs.

Firestore/Source/API/FIRDocumentSnapshot.mm

@@ -200,9 +200,12 @@ - (FSTFieldValueOptions *)optionsForServerTimestampBehavior:
     (FIRServerTimestampBehavior)serverTimestampBehavior {
   FSTServerTimestampBehavior internalBehavior =
       InternalServerTimestampBehavor(serverTimestampBehavior);
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
   return [[FSTFieldValueOptions alloc]
       initWithServerTimestampBehavior:internalBehavior
          timestampsInSnapshotsEnabled:self.firestore.settings.timestampsInSnapshotsEnabled];
+#pragma clang diagnostic pop
 }
 
 - (nullable id)objectForKeyedSubscript:(id)key {

Firestore/Source/API/FIRFirestore.mm

@@ -227,34 +227,6 @@ - (void)ensureClientConfigured {
       HARD_ASSERT(_settings.host, "FirestoreSettings.host cannot be nil.");
       HARD_ASSERT(_settings.dispatchQueue, "FirestoreSettings.dispatchQueue cannot be nil.");
 
-      if (!_settings.timestampsInSnapshotsEnabled) {
-        LOG_WARN(
-            "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::MakeString(_persistenceKey),
                                        util::MakeString(_settings.host), _settings.sslEnabled);
 

Firestore/Source/API/FIRFirestoreSettings.mm

@@ -25,8 +25,7 @@
 static const BOOL kDefaultPersistenceEnabled = YES;
 static const int64_t kDefaultCacheSizeBytes = 100 * 1024 * 1024;
 static const int64_t kMinimumCacheSizeBytes = 1 * 1024 * 1024;
-// TODO(b/73820332): flip the default.
-static const BOOL kDefaultTimestampsInSnapshotsEnabled = NO;
+static const BOOL kDefaultTimestampsInSnapshotsEnabled = YES;
 
 @implementation FIRFirestoreSettings
 
@@ -54,7 +53,10 @@ - (BOOL)isEqual:(id)other {
          self.isSSLEnabled == otherSettings.isSSLEnabled &&
          self.dispatchQueue == otherSettings.dispatchQueue &&
          self.isPersistenceEnabled == otherSettings.isPersistenceEnabled &&
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
          self.timestampsInSnapshotsEnabled == otherSettings.timestampsInSnapshotsEnabled &&
+#pragma clang diagnostic pop
          self.cacheSizeBytes == otherSettings.cacheSizeBytes;
 }
 
@@ -63,7 +65,10 @@ - (NSUInteger)hash {
   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);
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
   result = 31 * result + (self.timestampsInSnapshotsEnabled ? 1231 : 1237);
+#pragma clang diagnostic pop
   result = 31 * result + (NSUInteger)self.cacheSizeBytes;
   return result;
 }
@@ -74,7 +79,10 @@ - (id)copyWithZone:(nullable NSZone *)zone {
   copy.sslEnabled = _sslEnabled;
   copy.dispatchQueue = _dispatchQueue;
   copy.persistenceEnabled = _persistenceEnabled;
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
   copy.timestampsInSnapshotsEnabled = _timestampsInSnapshotsEnabled;
+#pragma clang diagnostic pop
   copy.cacheSizeBytes = _cacheSizeBytes;
   return copy;
 }

Firestore/Source/Public/FIRFirestoreSettings.h

@@ -48,21 +48,24 @@ NS_SWIFT_NAME(FirestoreSettings)
 @property(nonatomic, getter=isPersistenceEnabled) BOOL persistenceEnabled;
 
 /**
- * Enables the use of FIRTimestamps for timestamp fields in FIRDocumentSnapshots.
+ * Specifies whether to use FIRTimestamps for timestamp fields in FIRDocumentSnapshots. This is
+ * now enabled by default and should not be disabled.
  *
- * 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.
+ * Previously, Firestore returned timestamp fields as 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.
+ * So now Firestore returns FIRTimestamp values instead of NSDate, avoiding this kind of problem.
  *
- * 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.
+ * To opt into the old behavior of returning NSDate objects, you can temporarily set
+ * areTimestampsInSnapshotsEnabled to false.
+ *
+ * @deprecated This setting now defaults to true and will be removed in a future release. If you are
+ * already setting it to true, just remove the setting. If you are setting it to false, you should
+ * update your code to expect FIRTimestamp objects instead of NSDate and then remove the setting.
  */
-@property(nonatomic, getter=areTimestampsInSnapshotsEnabled) BOOL timestampsInSnapshotsEnabled;
+@property(nonatomic, getter=areTimestampsInSnapshotsEnabled) BOOL timestampsInSnapshotsEnabled
+    __attribute__((deprecated));
 
 /**
  * Sets the cache size threshold above which the SDK will attempt to collect least-recently-used

Firestore/Swift/Tests/API/BasicCompileTests.swift

@@ -63,7 +63,6 @@ func initializeDb() -> Firestore {
   let settings = FirestoreSettings()
   settings.host = "localhost"
   settings.isPersistenceEnabled = true
-  settings.areTimestampsInSnapshotsEnabled = true
   settings.cacheSizeBytes = FirestoreCacheSizeUnlimited
   firestore.settings = settings