Avoid wrapping and rewrapping NSStrings when constructing DatabaseId (firebase#833)

wilhuff · web-flow · commit 935f3ca7d749 · 2018-02-22T08:12:54.000-08:00
* Avoid wrapping and rewrapping NSStrings when constructing DatabaseId

* Shorten DatabaseId::kDefaultDatabaseId
diff --git a/Firestore/Example/Tests/API/FSTAPIHelpers.mm b/Firestore/Example/Tests/API/FSTAPIHelpers.mm
@@ -42,8 +42,8 @@
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wnonnull"
   dispatch_once(&onceToken, ^{
-    sharedInstance = [[FIRFirestore alloc] initWithProjectID:@"abc"
-                                                    database:@"abc"
+    sharedInstance = [[FIRFirestore alloc] initWithProjectID:"abc"
+                                                    database:"abc"
                                               persistenceKey:@"db123"
                                          credentialsProvider:nil
                                          workerDispatchQueue:nil
diff --git a/Firestore/Example/Tests/Core/FSTQueryTests.mm b/Firestore/Example/Tests/Core/FSTQueryTests.mm
@@ -309,7 +309,6 @@ - (void)testSortsDocumentsInTheCorrectOrder {
       [query queryByAddingSortOrder:[FSTSortOrder sortOrderWithFieldPath:FSTTestFieldPath(@"sort")
                                                                ascending:YES]];
 
-  NSString *defaultDatabaseID = util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId);
   // clang-format off
   NSArray<FSTDocument *> *docs = @[
       FSTTestDoc(@"collection/1", 0, @{@"sort": [NSNull null]}, NO),
@@ -326,7 +325,7 @@ - (void)testSortsDocumentsInTheCorrectOrder {
       FSTTestDoc(@"collection/1", 0, @{@"sort": @"ab"}, NO),
       FSTTestDoc(@"collection/1", 0, @{@"sort": @"b"}, NO),
       FSTTestDoc(@"collection/1", 0, @{@"sort":
-          FSTTestRef(@"project", defaultDatabaseID, @"collection/id1")}, NO),
+          FSTTestRef("project", DatabaseId::kDefault, @"collection/id1")}, NO),
   ];
   // clang-format on
 
diff --git a/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm b/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm
@@ -161,7 +161,7 @@ - (void)setUp {
     [GRPCCall useInsecureConnectionsForHost:settings.host];
   }
 
-  DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefaultDatabaseId);
+  DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefault);
 
   _databaseInfo = DatabaseInfo(database_id, "test-key", util::MakeStringView(settings.host),
                                settings.sslEnabled);
diff --git a/Firestore/Example/Tests/Integration/FSTStreamTests.mm b/Firestore/Example/Tests/Integration/FSTStreamTests.mm
@@ -148,7 +148,7 @@ - (void)setUp {
 
   FIRFirestoreSettings *settings = [FSTIntegrationTestCase settings];
   DatabaseId database_id(util::MakeStringView([FSTIntegrationTestCase projectID]),
-                         DatabaseId::kDefaultDatabaseId);
+                         DatabaseId::kDefault);
 
   _testQueue = dispatch_queue_create("FSTStreamTestWorkerQueue", DISPATCH_QUEUE_SERIAL);
   _workerDispatchQueue = [[FSTDispatchQueue alloc] initWithQueue:_testQueue];
diff --git a/Firestore/Example/Tests/Model/FSTFieldValueTests.mm b/Firestore/Example/Tests/Model/FSTFieldValueTests.mm
@@ -254,8 +254,8 @@ - (void)testWrapBlobs {
 
 - (void)testWrapResourceNames {
   NSArray *values = @[
-    FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/bar"),
-    FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/baz")
+    FSTTestRef("project", DatabaseId::kDefault, @"foo/bar"),
+    FSTTestRef("project", DatabaseId::kDefault, @"foo/baz")
   ];
   for (FSTDocumentKeyReference *value in values) {
     FSTFieldValue *wrapped = FSTTestFieldValue(value);
@@ -423,7 +423,7 @@ - (void)testArrays {
 }
 
 - (void)testValueEquality {
-  DatabaseId database_id = DatabaseId("project", DatabaseId::kDefaultDatabaseId);
+  DatabaseId database_id = DatabaseId("project", DatabaseId::kDefault);
   NSArray *groups = @[
     @[ FSTTestFieldValue(@YES), [FSTBooleanValue booleanValue:YES] ],
     @[ FSTTestFieldValue(@NO), [FSTBooleanValue booleanValue:NO] ],
@@ -467,10 +467,9 @@ - (void)testValueEquality {
     @[ FSTTestFieldValue(FSTTestGeoPoint(1, 0)) ],
     @[
       [FSTReferenceValue referenceValue:FSTTestDocKey(@"coll/doc1") databaseID:&database_id],
-      FSTTestFieldValue(FSTTestRef(
-          @"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"coll/doc1"))
+      FSTTestFieldValue(FSTTestRef("project", DatabaseId::kDefault, @"coll/doc1"))
     ],
-    @[ FSTTestRef(@"project", @"(default)", @"coll/doc2") ],
+    @[ FSTTestRef("project", "(default)", @"coll/doc2") ],
     @[ FSTTestFieldValue(@[ @"foo", @"bar" ]), FSTTestFieldValue(@[ @"foo", @"bar" ]) ],
     @[ FSTTestFieldValue(@[ @"foo", @"bar", @"baz" ]) ], @[ FSTTestFieldValue(@[ @"foo" ]) ],
     @[
@@ -531,9 +530,9 @@ - (void)testValueOrdering {
     @[ FSTTestData(0, 1, 2, 4, 3, -1) ], @[ FSTTestData(255, -1) ],
 
     // resource names
-    @[ FSTTestRef(@"p1", @"d1", @"c1/doc1") ], @[ FSTTestRef(@"p1", @"d1", @"c1/doc2") ],
-    @[ FSTTestRef(@"p1", @"d1", @"c10/doc1") ], @[ FSTTestRef(@"p1", @"d1", @"c2/doc1") ],
-    @[ FSTTestRef(@"p1", @"d2", @"c1/doc1") ], @[ FSTTestRef(@"p2", @"d1", @"c1/doc1") ],
+    @[ FSTTestRef("p1", "d1", @"c1/doc1") ], @[ FSTTestRef("p1", "d1", @"c1/doc2") ],
+    @[ FSTTestRef("p1", "d1", @"c10/doc1") ], @[ FSTTestRef("p1", "d1", @"c2/doc1") ],
+    @[ FSTTestRef("p1", "d2", @"c1/doc1") ], @[ FSTTestRef("p2", "d1", @"c1/doc1") ],
 
     // Geo points
     @[ FSTTestGeoPoint(-90, -180) ], @[ FSTTestGeoPoint(-90, 0) ], @[ FSTTestGeoPoint(-90, 180) ],
diff --git a/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm b/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm
@@ -239,9 +239,8 @@ - (void)testEncodesBlobs {
 }
 
 - (void)testEncodesResourceNames {
-  FSTDocumentKeyReference *reference =
-      FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/bar");
-  _databaseId = DatabaseId("project", DatabaseId::kDefaultDatabaseId);
+  FSTDocumentKeyReference *reference = FSTTestRef("project", DatabaseId::kDefault, @"foo/bar");
+  _databaseId = DatabaseId("project", DatabaseId::kDefault);
   GCFSValue *proto = [GCFSValue message];
   proto.referenceValue = @"projects/project/databases/(default)/documents/foo/bar";
 
diff --git a/Firestore/Example/Tests/Util/FSTHelpers.h b/Firestore/Example/Tests/Util/FSTHelpers.h
@@ -20,6 +20,8 @@
 #import "Firestore/Source/Model/FSTDocumentDictionary.h"
 #import "Firestore/Source/Model/FSTDocumentKeySet.h"
 
+#include "absl/strings/string_view.h"
+
 @class FIRGeoPoint;
 @class FSTDeleteMutation;
 @class FSTDeletedDocument;
@@ -190,7 +192,9 @@ FSTResourcePath *FSTTestPath(NSString *path);
 /**
  * A convenience method for creating a document reference from a path string.
  */
-FSTDocumentKeyReference *FSTTestRef(NSString *projectID, NSString *databaseID, NSString *path);
+FSTDocumentKeyReference *FSTTestRef(const absl::string_view projectID,
+                                    const absl::string_view databaseID,
+                                    NSString *path);
 
 /** A convenience method for creating a query for the given path (without any other filters). */
 FSTQuery *FSTTestQuery(NSString *path);
diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm
@@ -107,7 +107,7 @@
 
 FSTFieldValue *FSTTestFieldValue(id _Nullable value) {
   // This owns the DatabaseIds since we do not have FirestoreClient instance to own them.
-  static DatabaseId database_id{"project", DatabaseId::kDefaultDatabaseId};
+  static DatabaseId database_id{"project", DatabaseId::kDefault};
   FSTUserDataConverter *converter =
       [[FSTUserDataConverter alloc] initWithDatabaseID:&database_id
                                           preConverter:^id _Nullable(id _Nullable input) {
@@ -172,10 +172,12 @@
   return [FSTResourcePath pathWithSegments:FSTTestSplitPath(path)];
 }
 
-FSTDocumentKeyReference *FSTTestRef(NSString *projectID, NSString *database, NSString *path) {
+FSTDocumentKeyReference *FSTTestRef(const absl::string_view projectID,
+                                    const absl::string_view database,
+                                    NSString *path) {
   // This owns the DatabaseIds since we do not have FirestoreClient instance to own them.
   static std::list<DatabaseId> database_ids;
-  database_ids.emplace_back(util::MakeStringView(projectID), util::MakeStringView(database));
+  database_ids.emplace_back(projectID, database);
   return [[FSTDocumentKeyReference alloc] initWithKey:FSTTestDocKey(path)
                                            databaseID:&database_ids.back()];
 }
diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
@@ -140,13 +140,12 @@ - (FIRFirestore *)firestoreWithProjectID:(NSString *)projectID {
   FIRSetLoggerLevel(FIRLoggerLevelDebug);
   // HACK: FIRFirestore expects a non-nil app, but for tests we cheat.
   FIRApp *app = nil;
-  FIRFirestore *firestore = [[FIRFirestore alloc]
-        initWithProjectID:projectID
-                 database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)
-           persistenceKey:persistenceKey
-      credentialsProvider:credentialsProvider
-      workerDispatchQueue:workerDispatchQueue
-              firebaseApp:app];
+  FIRFirestore *firestore = [[FIRFirestore alloc] initWithProjectID:util::MakeStringView(projectID)
+                                                           database:DatabaseId::kDefault
+                                                     persistenceKey:persistenceKey
+                                                credentialsProvider:credentialsProvider
+                                                workerDispatchQueue:workerDispatchQueue
+                                                        firebaseApp:app];
 
   firestore.settings = [FSTIntegrationTestCase settings];
 
diff --git a/Firestore/Source/API/FIRFirestore+Internal.h b/Firestore/Source/API/FIRFirestore+Internal.h
@@ -17,6 +17,7 @@
 #import "FIRFirestore.h"
 
 #include "Firestore/core/src/firebase/firestore/model/database_id.h"
+#include "absl/strings/string_view.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
@@ -31,8 +32,8 @@ NS_ASSUME_NONNULL_BEGIN
  * Initializes a Firestore object with all the required parameters directly. This exists so that
  * tests can create FIRFirestore objects without needing FIRApp.
  */
-- (instancetype)initWithProjectID:(NSString *)projectID
-                         database:(NSString *)database
+- (instancetype)initWithProjectID:(const absl::string_view)projectID
+                         database:(const absl::string_view)database
                    persistenceKey:(NSString *)persistenceKey
               credentialsProvider:(id<FSTCredentialsProvider>)credentialsProvider
               workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue
diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm
@@ -117,13 +117,11 @@ + (instancetype)firestore {
                          @"Failed to get FirebaseApp instance. Please call FirebaseApp.configure() "
                          @"before using Firestore");
   }
-  return
-      [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)];
+  return [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefault)];
 }
 
 + (instancetype)firestoreForApp:(FIRApp *)app {
-  return
-      [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)];
+  return [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefault)];
 }
 
 // TODO(b/62410906): make this public
@@ -137,7 +135,7 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
     FSTThrowInvalidArgument(
         @"database identifier may not be nil. Use '%@' if you want the default "
          "database",
-        util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId));
+        util::WrapNSStringNoCopy(DatabaseId::kDefault));
   }
 
   // Note: If the key format changes, please change the code that detects FIRApps being deleted
@@ -159,8 +157,8 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
 
       NSString *persistenceKey = app.name;
 
-      firestore = [[FIRFirestore alloc] initWithProjectID:projectID
-                                                 database:database
+      firestore = [[FIRFirestore alloc] initWithProjectID:util::MakeStringView(projectID)
+                                                 database:util::MakeStringView(database)
                                            persistenceKey:persistenceKey
                                       credentialsProvider:credentialsProvider
                                       workerDispatchQueue:workerDispatchQueue
@@ -172,14 +170,14 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
   }
 }
 
-- (instancetype)initWithProjectID:(NSString *)projectID
-                         database:(NSString *)database
+- (instancetype)initWithProjectID:(const absl::string_view)projectID
+                         database:(const absl::string_view)database
                    persistenceKey:(NSString *)persistenceKey
               credentialsProvider:(id<FSTCredentialsProvider>)credentialsProvider
               workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue
                       firebaseApp:(FIRApp *)app {
   if (self = [super init]) {
-    _databaseID = DatabaseId(util::MakeStringView(projectID), util::MakeStringView(database));
+    _databaseID = DatabaseId(projectID, database);
     FSTPreConverterBlock block = ^id _Nullable(id _Nullable input) {
       if ([input isKindOfClass:[FIRDocumentReference class]]) {
         FIRDocumentReference *documentReference = (FIRDocumentReference *)input;
diff --git a/Firestore/core/src/firebase/firestore/model/database_id.cc b/Firestore/core/src/firebase/firestore/model/database_id.cc
@@ -22,7 +22,7 @@ namespace firebase {
 namespace firestore {
 namespace model {
 
-constexpr const char* DatabaseId::kDefaultDatabaseId;
+constexpr const char* DatabaseId::kDefault;
 
 DatabaseId::DatabaseId(const absl::string_view project_id,
                        const absl::string_view database_id)
diff --git a/Firestore/core/src/firebase/firestore/model/database_id.h b/Firestore/core/src/firebase/firestore/model/database_id.h
@@ -31,7 +31,7 @@ namespace model {
 class DatabaseId {
  public:
   /** The default name for "unset" database ID in resource names. */
-  static constexpr const char* kDefaultDatabaseId = "(default)";
+  static constexpr const char* kDefault = "(default)";
 
 #if defined(__OBJC__)
   // For objective-c++ initialization; to be removed after migration.
@@ -58,7 +58,7 @@ class DatabaseId {
 
   /** Whether this is the default database of the project. */
   bool IsDefaultDatabase() const {
-    return database_id_ == kDefaultDatabaseId;
+    return database_id_ == kDefault;
   }
 
 #if defined(__OBJC__)
diff --git a/Firestore/core/test/firebase/firestore/core/database_info_test.cc b/Firestore/core/test/firebase/firestore/core/database_info_test.cc
@@ -34,8 +34,8 @@ TEST(DatabaseInfo, Getter) {
 }
 
 TEST(DatabaseInfo, DefaultDatabase) {
-  DatabaseInfo info(DatabaseId("project id", DatabaseId::kDefaultDatabaseId),
-                    "key", "http://host", false);
+  DatabaseInfo info(DatabaseId("project id", DatabaseId::kDefault), "key",
+                    "http://host", false);
   EXPECT_EQ("project id", info.database_id().project_id());
   EXPECT_EQ("(default)", info.database_id().database_id());
   EXPECT_EQ("key", info.persistence_key());
diff --git a/Firestore/core/test/firebase/firestore/model/database_id_test.cc b/Firestore/core/test/firebase/firestore/model/database_id_test.cc
@@ -30,7 +30,7 @@ TEST(DatabaseId, Constructor) {
 }
 
 TEST(DatabaseId, DefaultDb) {
-  DatabaseId id("p", DatabaseId::kDefaultDatabaseId);
+  DatabaseId id("p", DatabaseId::kDefault);
   EXPECT_EQ("p", id.project_id());
   EXPECT_EQ("(default)", id.database_id());
   EXPECT_TRUE(id.IsDefaultDatabase());

Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ - (void)setUp {`
`161`	`161`	`[GRPCCall useInsecureConnectionsForHost:settings.host];`
`162`	`162`	`}`
`163`	`163`
`164`		`- DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefaultDatabaseId);`
	`164`	`+ DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefault);`
`165`	`165`
`166`	`166`	`_databaseInfo = DatabaseInfo(database_id, "test-key", util::MakeStringView(settings.host),`
`167`	`167`	`settings.sslEnabled);`