Skip to content

Commit dca4ccb

Browse files
wilhuffwilhuff
authored
Avoid wrapping and rewrapping NSStrings when constructing DatabaseId (firebase#833)
* Avoid wrapping and rewrapping NSStrings when constructing DatabaseId * Shorten DatabaseId::kDefaultDatabaseId
1 parent c60300d commit dca4ccb

File tree

15 files changed

+48
-47
lines changed

15 files changed

+48
-47
lines changed

‎Firestore/Example/Tests/API/FSTAPIHelpers.mm

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@
4242
#pragma clang diagnostic push
4343
#pragma clang diagnostic ignored "-Wnonnull"
4444
dispatch_once(&onceToken, ^{
45-
sharedInstance = [[FIRFirestore alloc] initWithProjectID:@"abc"
46-
database:@"abc"
45+
sharedInstance = [[FIRFirestore alloc] initWithProjectID:"abc"
46+
database:"abc"
4747
persistenceKey:@"db123"
4848
credentialsProvider:nil
4949
workerDispatchQueue:nil

‎Firestore/Example/Tests/Core/FSTQueryTests.mm

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,6 @@ - (void)testSortsDocumentsInTheCorrectOrder {
309309
[query queryByAddingSortOrder:[FSTSortOrder sortOrderWithFieldPath:FSTTestFieldPath(@"sort")
310310
ascending:YES]];
311311

312-
NSString *defaultDatabaseID = util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId);
313312
// clang-format off
314313
NSArray<FSTDocument *> *docs = @[
315314
FSTTestDoc(@"collection/1", 0, @{@"sort": [NSNull null]}, NO),
@@ -326,7 +325,7 @@ - (void)testSortsDocumentsInTheCorrectOrder {
326325
FSTTestDoc(@"collection/1", 0, @{@"sort": @"ab"}, NO),
327326
FSTTestDoc(@"collection/1", 0, @{@"sort": @"b"}, NO),
328327
FSTTestDoc(@"collection/1", 0, @{@"sort":
329-
FSTTestRef(@"project", defaultDatabaseID, @"collection/id1")}, NO),
328+
FSTTestRef("project", DatabaseId::kDefault, @"collection/id1")}, NO),
330329
];
331330
// clang-format on
332331

‎Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ - (void)setUp {
161161
[GRPCCall useInsecureConnectionsForHost:settings.host];
162162
}
163163

164-
DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefaultDatabaseId);
164+
DatabaseId database_id(util::MakeStringView(projectID), DatabaseId::kDefault);
165165

166166
_databaseInfo = DatabaseInfo(database_id, "test-key", util::MakeStringView(settings.host),
167167
settings.sslEnabled);

‎Firestore/Example/Tests/Integration/FSTStreamTests.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ - (void)setUp {
148148

149149
FIRFirestoreSettings *settings = [FSTIntegrationTestCase settings];
150150
DatabaseId database_id(util::MakeStringView([FSTIntegrationTestCase projectID]),
151-
DatabaseId::kDefaultDatabaseId);
151+
DatabaseId::kDefault);
152152

153153
_testQueue = dispatch_queue_create("FSTStreamTestWorkerQueue", DISPATCH_QUEUE_SERIAL);
154154
_workerDispatchQueue = [[FSTDispatchQueue alloc] initWithQueue:_testQueue];

‎Firestore/Example/Tests/Model/FSTFieldValueTests.mm

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,8 @@ - (void)testWrapBlobs {
254254

255255
- (void)testWrapResourceNames {
256256
NSArray *values = @[
257-
FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/bar"),
258-
FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/baz")
257+
FSTTestRef("project", DatabaseId::kDefault, @"foo/bar"),
258+
FSTTestRef("project", DatabaseId::kDefault, @"foo/baz")
259259
];
260260
for (FSTDocumentKeyReference *value in values) {
261261
FSTFieldValue *wrapped = FSTTestFieldValue(value);
@@ -423,7 +423,7 @@ - (void)testArrays {
423423
}
424424

425425
- (void)testValueEquality {
426-
DatabaseId database_id = DatabaseId("project", DatabaseId::kDefaultDatabaseId);
426+
DatabaseId database_id = DatabaseId("project", DatabaseId::kDefault);
427427
NSArray *groups = @[
428428
@[ FSTTestFieldValue(@YES), [FSTBooleanValue booleanValue:YES] ],
429429
@[ FSTTestFieldValue(@NO), [FSTBooleanValue booleanValue:NO] ],
@@ -467,10 +467,9 @@ - (void)testValueEquality {
467467
@[ FSTTestFieldValue(FSTTestGeoPoint(1, 0)) ],
468468
@[
469469
[FSTReferenceValue referenceValue:FSTTestDocKey(@"coll/doc1") databaseID:&database_id],
470-
FSTTestFieldValue(FSTTestRef(
471-
@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"coll/doc1"))
470+
FSTTestFieldValue(FSTTestRef("project", DatabaseId::kDefault, @"coll/doc1"))
472471
],
473-
@[ FSTTestRef(@"project", @"(default)", @"coll/doc2") ],
472+
@[ FSTTestRef("project", "(default)", @"coll/doc2") ],
474473
@[ FSTTestFieldValue(@[ @"foo", @"bar" ]), FSTTestFieldValue(@[ @"foo", @"bar" ]) ],
475474
@[ FSTTestFieldValue(@[ @"foo", @"bar", @"baz" ]) ], @[ FSTTestFieldValue(@[ @"foo" ]) ],
476475
@[
@@ -531,9 +530,9 @@ - (void)testValueOrdering {
531530
@[ FSTTestData(0, 1, 2, 4, 3, -1) ], @[ FSTTestData(255, -1) ],
532531

533532
// resource names
534-
@[ FSTTestRef(@"p1", @"d1", @"c1/doc1") ], @[ FSTTestRef(@"p1", @"d1", @"c1/doc2") ],
535-
@[ FSTTestRef(@"p1", @"d1", @"c10/doc1") ], @[ FSTTestRef(@"p1", @"d1", @"c2/doc1") ],
536-
@[ FSTTestRef(@"p1", @"d2", @"c1/doc1") ], @[ FSTTestRef(@"p2", @"d1", @"c1/doc1") ],
533+
@[ FSTTestRef("p1", "d1", @"c1/doc1") ], @[ FSTTestRef("p1", "d1", @"c1/doc2") ],
534+
@[ FSTTestRef("p1", "d1", @"c10/doc1") ], @[ FSTTestRef("p1", "d1", @"c2/doc1") ],
535+
@[ FSTTestRef("p1", "d2", @"c1/doc1") ], @[ FSTTestRef("p2", "d1", @"c1/doc1") ],
537536

538537
// Geo points
539538
@[ FSTTestGeoPoint(-90, -180) ], @[ FSTTestGeoPoint(-90, 0) ], @[ FSTTestGeoPoint(-90, 180) ],

‎Firestore/Example/Tests/Remote/FSTSerializerBetaTests.mm

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -239,9 +239,8 @@ - (void)testEncodesBlobs {
239239
}
240240

241241
- (void)testEncodesResourceNames {
242-
FSTDocumentKeyReference *reference =
243-
FSTTestRef(@"project", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId), @"foo/bar");
244-
_databaseId = DatabaseId("project", DatabaseId::kDefaultDatabaseId);
242+
FSTDocumentKeyReference *reference = FSTTestRef("project", DatabaseId::kDefault, @"foo/bar");
243+
_databaseId = DatabaseId("project", DatabaseId::kDefault);
245244
GCFSValue *proto = [GCFSValue message];
246245
proto.referenceValue = @"projects/project/databases/(default)/documents/foo/bar";
247246

‎Firestore/Example/Tests/Util/FSTHelpers.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
#import "Firestore/Source/Model/FSTDocumentDictionary.h"
2121
#import "Firestore/Source/Model/FSTDocumentKeySet.h"
2222

23+
#include "absl/strings/string_view.h"
24+
2325
@class FIRGeoPoint;
2426
@class FSTDeleteMutation;
2527
@class FSTDeletedDocument;
@@ -190,7 +192,9 @@ FSTResourcePath *FSTTestPath(NSString *path);
190192
/**
191193
* A convenience method for creating a document reference from a path string.
192194
*/
193-
FSTDocumentKeyReference *FSTTestRef(NSString *projectID, NSString *databaseID, NSString *path);
195+
FSTDocumentKeyReference *FSTTestRef(const absl::string_view projectID,
196+
const absl::string_view databaseID,
197+
NSString *path);
194198

195199
/** A convenience method for creating a query for the given path (without any other filters). */
196200
FSTQuery *FSTTestQuery(NSString *path);

‎Firestore/Example/Tests/Util/FSTHelpers.mm

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@
107107

108108
FSTFieldValue *FSTTestFieldValue(id _Nullable value) {
109109
// This owns the DatabaseIds since we do not have FirestoreClient instance to own them.
110-
static DatabaseId database_id{"project", DatabaseId::kDefaultDatabaseId};
110+
static DatabaseId database_id{"project", DatabaseId::kDefault};
111111
FSTUserDataConverter *converter =
112112
[[FSTUserDataConverter alloc] initWithDatabaseID:&database_id
113113
preConverter:^id _Nullable(id _Nullable input) {
@@ -172,10 +172,12 @@
172172
return [FSTResourcePath pathWithSegments:FSTTestSplitPath(path)];
173173
}
174174

175-
FSTDocumentKeyReference *FSTTestRef(NSString *projectID, NSString *database, NSString *path) {
175+
FSTDocumentKeyReference *FSTTestRef(const absl::string_view projectID,
176+
const absl::string_view database,
177+
NSString *path) {
176178
// This owns the DatabaseIds since we do not have FirestoreClient instance to own them.
177179
static std::list<DatabaseId> database_ids;
178-
database_ids.emplace_back(util::MakeStringView(projectID), util::MakeStringView(database));
180+
database_ids.emplace_back(projectID, database);
179181
return [[FSTDocumentKeyReference alloc] initWithKey:FSTTestDocKey(path)
180182
databaseID:&database_ids.back()];
181183
}

‎Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,13 +140,12 @@ - (FIRFirestore *)firestoreWithProjectID:(NSString *)projectID {
140140
FIRSetLoggerLevel(FIRLoggerLevelDebug);
141141
// HACK: FIRFirestore expects a non-nil app, but for tests we cheat.
142142
FIRApp *app = nil;
143-
FIRFirestore *firestore = [[FIRFirestore alloc]
144-
initWithProjectID:projectID
145-
database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)
146-
persistenceKey:persistenceKey
147-
credentialsProvider:credentialsProvider
148-
workerDispatchQueue:workerDispatchQueue
149-
firebaseApp:app];
143+
FIRFirestore *firestore = [[FIRFirestore alloc] initWithProjectID:util::MakeStringView(projectID)
144+
database:DatabaseId::kDefault
145+
persistenceKey:persistenceKey
146+
credentialsProvider:credentialsProvider
147+
workerDispatchQueue:workerDispatchQueue
148+
firebaseApp:app];
150149

151150
firestore.settings = [FSTIntegrationTestCase settings];
152151

‎Firestore/Source/API/FIRFirestore+Internal.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "FIRFirestore.h"
1818

1919
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
20+
#include "absl/strings/string_view.h"
2021

2122
NS_ASSUME_NONNULL_BEGIN
2223

@@ -31,8 +32,8 @@ NS_ASSUME_NONNULL_BEGIN
3132
* Initializes a Firestore object with all the required parameters directly. This exists so that
3233
* tests can create FIRFirestore objects without needing FIRApp.
3334
*/
34-
- (instancetype)initWithProjectID:(NSString *)projectID
35-
database:(NSString *)database
35+
- (instancetype)initWithProjectID:(const absl::string_view)projectID
36+
database:(const absl::string_view)database
3637
persistenceKey:(NSString *)persistenceKey
3738
credentialsProvider:(id<FSTCredentialsProvider>)credentialsProvider
3839
workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue

‎Firestore/Source/API/FIRFirestore.mm

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -117,13 +117,11 @@ + (instancetype)firestore {
117117
@"Failed to get FirebaseApp instance. Please call FirebaseApp.configure() "
118118
@"before using Firestore");
119119
}
120-
return
121-
[self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)];
120+
return [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefault)];
122121
}
123122

124123
+ (instancetype)firestoreForApp:(FIRApp *)app {
125-
return
126-
[self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)];
124+
return [self firestoreForApp:app database:util::WrapNSStringNoCopy(DatabaseId::kDefault)];
127125
}
128126

129127
// TODO(b/62410906): make this public
@@ -137,7 +135,7 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
137135
FSTThrowInvalidArgument(
138136
@"database identifier may not be nil. Use '%@' if you want the default "
139137
"database",
140-
util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId));
138+
util::WrapNSStringNoCopy(DatabaseId::kDefault));
141139
}
142140

143141
// 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 {
159157

160158
NSString *persistenceKey = app.name;
161159

162-
firestore = [[FIRFirestore alloc] initWithProjectID:projectID
163-
database:database
160+
firestore = [[FIRFirestore alloc] initWithProjectID:util::MakeStringView(projectID)
161+
database:util::MakeStringView(database)
164162
persistenceKey:persistenceKey
165163
credentialsProvider:credentialsProvider
166164
workerDispatchQueue:workerDispatchQueue
@@ -172,14 +170,14 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database {
172170
}
173171
}
174172

175-
- (instancetype)initWithProjectID:(NSString *)projectID
176-
database:(NSString *)database
173+
- (instancetype)initWithProjectID:(const absl::string_view)projectID
174+
database:(const absl::string_view)database
177175
persistenceKey:(NSString *)persistenceKey
178176
credentialsProvider:(id<FSTCredentialsProvider>)credentialsProvider
179177
workerDispatchQueue:(FSTDispatchQueue *)workerDispatchQueue
180178
firebaseApp:(FIRApp *)app {
181179
if (self = [super init]) {
182-
_databaseID = DatabaseId(util::MakeStringView(projectID), util::MakeStringView(database));
180+
_databaseID = DatabaseId(projectID, database);
183181
FSTPreConverterBlock block = ^id _Nullable(id _Nullable input) {
184182
if ([input isKindOfClass:[FIRDocumentReference class]]) {
185183
FIRDocumentReference *documentReference = (FIRDocumentReference *)input;

‎Firestore/core/src/firebase/firestore/model/database_id.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ namespace firebase {
2222
namespace firestore {
2323
namespace model {
2424

25-
constexpr const char* DatabaseId::kDefaultDatabaseId;
25+
constexpr const char* DatabaseId::kDefault;
2626

2727
DatabaseId::DatabaseId(const absl::string_view project_id,
2828
const absl::string_view database_id)

‎Firestore/core/src/firebase/firestore/model/database_id.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ namespace model {
3131
class DatabaseId {
3232
public:
3333
/** The default name for "unset" database ID in resource names. */
34-
static constexpr const char* kDefaultDatabaseId = "(default)";
34+
static constexpr const char* kDefault = "(default)";
3535

3636
#if defined(__OBJC__)
3737
// For objective-c++ initialization; to be removed after migration.
@@ -58,7 +58,7 @@ class DatabaseId {
5858

5959
/** Whether this is the default database of the project. */
6060
bool IsDefaultDatabase() const {
61-
return database_id_ == kDefaultDatabaseId;
61+
return database_id_ == kDefault;
6262
}
6363

6464
#if defined(__OBJC__)

‎Firestore/core/test/firebase/firestore/core/database_info_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ TEST(DatabaseInfo, Getter) {
3434
}
3535

3636
TEST(DatabaseInfo, DefaultDatabase) {
37-
DatabaseInfo info(DatabaseId("project id", DatabaseId::kDefaultDatabaseId),
38-
"key", "http://host", false);
37+
DatabaseInfo info(DatabaseId("project id", DatabaseId::kDefault), "key",
38+
"http://host", false);
3939
EXPECT_EQ("project id", info.database_id().project_id());
4040
EXPECT_EQ("(default)", info.database_id().database_id());
4141
EXPECT_EQ("key", info.persistence_key());

‎Firestore/core/test/firebase/firestore/model/database_id_test.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ TEST(DatabaseId, Constructor) {
3030
}
3131

3232
TEST(DatabaseId, DefaultDb) {
33-
DatabaseId id("p", DatabaseId::kDefaultDatabaseId);
33+
DatabaseId id("p", DatabaseId::kDefault);
3434
EXPECT_EQ("p", id.project_id());
3535
EXPECT_EQ("(default)", id.database_id());
3636
EXPECT_TRUE(id.IsDefaultDatabase());

0 commit comments

Comments
 (0)