From 935f3ca7d749f96c7207236a39c57f32a02c05d3 Mon Sep 17 00:00:00 2001 From: Gil Date: Thu, 22 Feb 2018 08:12:54 -0800 Subject: Avoid wrapping and rewrapping NSStrings when constructing DatabaseId (#833) * Avoid wrapping and rewrapping NSStrings when constructing DatabaseId * Shorten DatabaseId::kDefaultDatabaseId --- Firestore/Example/Tests/API/FSTAPIHelpers.mm | 4 ++-- Firestore/Example/Tests/Core/FSTQueryTests.mm | 3 +-- .../Example/Tests/Integration/FSTDatastoreTests.mm | 2 +- Firestore/Example/Tests/Integration/FSTStreamTests.mm | 2 +- Firestore/Example/Tests/Model/FSTFieldValueTests.mm | 17 ++++++++--------- .../Example/Tests/Remote/FSTSerializerBetaTests.mm | 5 ++--- Firestore/Example/Tests/Util/FSTHelpers.h | 6 +++++- Firestore/Example/Tests/Util/FSTHelpers.mm | 8 +++++--- Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm | 13 ++++++------- Firestore/Source/API/FIRFirestore+Internal.h | 5 +++-- Firestore/Source/API/FIRFirestore.mm | 18 ++++++++---------- .../core/src/firebase/firestore/model/database_id.cc | 2 +- .../core/src/firebase/firestore/model/database_id.h | 4 ++-- .../test/firebase/firestore/core/database_info_test.cc | 4 ++-- .../test/firebase/firestore/model/database_id_test.cc | 2 +- 15 files changed, 48 insertions(+), 47 deletions(-) (limited to 'Firestore') diff --git a/Firestore/Example/Tests/API/FSTAPIHelpers.mm b/Firestore/Example/Tests/API/FSTAPIHelpers.mm index da899b7..d091842 100644 --- a/Firestore/Example/Tests/API/FSTAPIHelpers.mm +++ b/Firestore/Example/Tests/API/FSTAPIHelpers.mm @@ -42,8 +42,8 @@ FIRFirestore *FSTTestFirestore() { #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 index 8acefb5..c0b2cd9 100644 --- a/Firestore/Example/Tests/Core/FSTQueryTests.mm +++ b/Firestore/Example/Tests/Core/FSTQueryTests.mm @@ -309,7 +309,6 @@ NS_ASSUME_NONNULL_BEGIN [query queryByAddingSortOrder:[FSTSortOrder sortOrderWithFieldPath:FSTTestFieldPath(@"sort") ascending:YES]]; - NSString *defaultDatabaseID = util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId); // clang-format off NSArray *docs = @[ FSTTestDoc(@"collection/1", 0, @{@"sort": [NSNull null]}, NO), @@ -326,7 +325,7 @@ NS_ASSUME_NONNULL_BEGIN 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 index b48e5d0..c38e8f3 100644 --- a/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm +++ b/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm @@ -161,7 +161,7 @@ NS_ASSUME_NONNULL_BEGIN [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 index cb967b1..9105b74 100644 --- a/Firestore/Example/Tests/Integration/FSTStreamTests.mm +++ b/Firestore/Example/Tests/Integration/FSTStreamTests.mm @@ -148,7 +148,7 @@ using firebase::firestore::model::DatabaseId; 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 index c0fb188..03dfa66 100644 --- a/Firestore/Example/Tests/Model/FSTFieldValueTests.mm +++ b/Firestore/Example/Tests/Model/FSTFieldValueTests.mm @@ -254,8 +254,8 @@ union DoubleBits { - (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 @@ union DoubleBits { } - (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 @@ union DoubleBits { @[ 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 @@ union DoubleBits { @[ 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 index f7cc1b0..fc4060b 100644 --- a/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm +++ b/Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm @@ -239,9 +239,8 @@ NS_ASSUME_NONNULL_BEGIN } - (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 index 9ee9a0b..cc9f2ec 100644 --- 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 index 586fdbc..649486a 100644 --- a/Firestore/Example/Tests/Util/FSTHelpers.mm +++ b/Firestore/Example/Tests/Util/FSTHelpers.mm @@ -107,7 +107,7 @@ FSTFieldPath *FSTTestFieldPath(NSString *field) { 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 @@ FSTResourcePath *FSTTestPath(NSString *path) { 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 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 index e34b2a5..3c80d16 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm @@ -140,13 +140,12 @@ NS_ASSUME_NONNULL_BEGIN 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 index 717a08b..7bc419a 100644 --- 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)credentialsProvider workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm index 5a50710..ce8f8ab 100644 --- a/Firestore/Source/API/FIRFirestore.mm +++ b/Firestore/Source/API/FIRFirestore.mm @@ -117,13 +117,11 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; @"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 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; 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 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; 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 @@ extern "C" NSString *const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain"; } } -- (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)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 index d7e8a85..a756ea7 100644 --- 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 index 648f982..01ccf76 100644 --- 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 index d865105..4c9691c 100644 --- 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 index 16b0e6b..c00415a 100644 --- 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()); -- cgit v1.2.3