Skip to content

Commit c856059

Browse files
authored
Migrate FSTLevelDBMigrations to C++ LevelDbMigrations (#1714)
* Remove a copy in the common case of adding a new key to a transaction * Add support for nanopb::Writer wrapping a std::string * Migrate FSTLevelDBMigrations to LevelDbMigrations
1 parent dcffc99 commit c856059

File tree

10 files changed

+191
-139
lines changed

10 files changed

+191
-139
lines changed

Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@
2222

2323
#import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
2424
#import "Firestore/Source/Local/FSTLevelDB.h"
25-
#import "Firestore/Source/Local/FSTLevelDBMigrations.h"
2625
#import "Firestore/Source/Local/FSTLevelDBMutationQueue.h"
2726
#import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
2827

2928
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
29+
#include "Firestore/core/src/firebase/firestore/local/leveldb_migrations.h"
3030
#include "Firestore/core/src/firebase/firestore/util/ordered_code.h"
3131
#include "Firestore/core/src/firebase/firestore/util/status.h"
3232
#include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
@@ -39,6 +39,7 @@
3939

4040
using firebase::firestore::FirestoreErrorCode;
4141
using firebase::firestore::local::LevelDbDocumentTargetKey;
42+
using firebase::firestore::local::LevelDbMigrations;
4243
using firebase::firestore::local::LevelDbMutationKey;
4344
using firebase::firestore::local::LevelDbMutationQueueKey;
4445
using firebase::firestore::local::LevelDbQueryTargetKey;
@@ -54,6 +55,8 @@
5455
using leveldb::Options;
5556
using leveldb::Status;
5657

58+
using SchemaVersion = LevelDbMigrations::SchemaVersion;
59+
5760
@interface FSTLevelDBMigrationsTests : XCTestCase
5861
@end
5962

@@ -80,7 +83,7 @@ - (void)tearDown {
8083
- (void)testAddsTargetGlobal {
8184
FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
8285
XCTAssertNil(metadata, @"Not expecting metadata yet, we should have an empty db");
83-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
86+
LevelDbMigrations::RunMigrations(_db.get());
8487

8588
metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
8689
XCTAssertNotNil(metadata, @"Migrations should have added the metadata");
@@ -89,18 +92,16 @@ - (void)testAddsTargetGlobal {
8992
- (void)testSetsVersionNumber {
9093
{
9194
LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber before");
92-
FSTLevelDBSchemaVersion initial =
93-
[FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
95+
SchemaVersion initial = LevelDbMigrations::ReadSchemaVersion(&transaction);
9496
XCTAssertEqual(0, initial, "No version should be equivalent to 0");
9597
}
9698

9799
{
98100
// Pick an arbitrary high migration number and migrate to it.
99-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
101+
LevelDbMigrations::RunMigrations(_db.get());
100102

101103
LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber after");
102-
FSTLevelDBSchemaVersion actual =
103-
[FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
104+
SchemaVersion actual = LevelDbMigrations::ReadSchemaVersion(&transaction);
104105
XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
105106
}
106107
}
@@ -143,7 +144,7 @@ - (void)testDropsTheQueryCache {
143144
LevelDbMutationKey::Key(userID, batchID),
144145
};
145146

146-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
147+
LevelDbMigrations::RunMigrations(_db.get(), 2);
147148
{
148149
// Setup some targets to be counted in the migration.
149150
LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache setup");
@@ -156,7 +157,7 @@ - (void)testDropsTheQueryCache {
156157
transaction.Commit();
157158
}
158159

159-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
160+
LevelDbMigrations::RunMigrations(_db.get(), 3);
160161
{
161162
LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache");
162163
for (const std::string &key : targetKeys) {
@@ -173,7 +174,7 @@ - (void)testDropsTheQueryCache {
173174
}
174175

175176
- (void)testDropsTheQueryCacheWithThousandsOfEntries {
176-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
177+
LevelDbMigrations::RunMigrations(_db.get(), 2);
177178
{
178179
// Setup some targets to be destroyed.
179180
LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCacheWithThousandsOfEntries setup");
@@ -183,7 +184,7 @@ - (void)testDropsTheQueryCacheWithThousandsOfEntries {
183184
transaction.Commit();
184185
}
185186

186-
[FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
187+
LevelDbMigrations::RunMigrations(_db.get(), 3);
187188
{
188189
LevelDbTransaction transaction(_db.get(), "Verify");
189190
std::string prefix = LevelDbTargetKey::KeyPrefix();

Firestore/Source/Local/FSTLevelDB.mm

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
#import "FIRFirestoreErrors.h"
2323
#import "Firestore/Source/Core/FSTListenSequence.h"
2424
#import "Firestore/Source/Local/FSTLRUGarbageCollector.h"
25-
#import "Firestore/Source/Local/FSTLevelDBMigrations.h"
2625
#import "Firestore/Source/Local/FSTLevelDBMutationQueue.h"
2726
#import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
2827
#import "Firestore/Source/Local/FSTLevelDBRemoteDocumentCache.h"
@@ -33,6 +32,7 @@
3332
#include "Firestore/core/src/firebase/firestore/auth/user.h"
3433
#include "Firestore/core/src/firebase/firestore/core/database_info.h"
3534
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
35+
#include "Firestore/core/src/firebase/firestore/local/leveldb_migrations.h"
3636
#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h"
3737
#include "Firestore/core/src/firebase/firestore/local/leveldb_util.h"
3838
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
@@ -58,6 +58,7 @@
5858
using firebase::firestore::local::ConvertStatus;
5959
using firebase::firestore::local::LevelDbDocumentMutationKey;
6060
using firebase::firestore::local::LevelDbDocumentTargetKey;
61+
using firebase::firestore::local::LevelDbMigrations;
6162
using firebase::firestore::local::LevelDbMutationKey;
6263
using firebase::firestore::local::LevelDbTransaction;
6364
using firebase::firestore::model::DatabaseId;
@@ -352,7 +353,7 @@ - (Status)start {
352353
}
353354
_ptr = std::move(database).ValueOrDie();
354355

355-
[FSTLevelDBMigrations runMigrationsWithDatabase:_ptr.get()];
356+
LevelDbMigrations::RunMigrations(_ptr.get());
356357
LevelDbTransaction transaction(_ptr.get(), "Start LevelDB");
357358
_users = [FSTLevelDB collectUserSet:&transaction];
358359
transaction.Commit();

Firestore/Source/Local/FSTLevelDBMigrations.h

Lines changed: 0 additions & 49 deletions
This file was deleted.

Firestore/core/src/firebase/firestore/local/CMakeLists.txt

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,23 @@ if(HAVE_LEVELDB)
1616
cc_library(
1717
firebase_firestore_local_persistence_leveldb
1818
SOURCES
19-
leveldb_key.h
2019
leveldb_key.cc
21-
leveldb_transaction.h
20+
leveldb_key.h
21+
leveldb_migrations.cc
22+
leveldb_migrations.h
2223
leveldb_transaction.cc
23-
leveldb_util.h
24+
leveldb_transaction.h
2425
leveldb_util.cc
26+
leveldb_util.h
2527
DEPENDS
28+
# TODO(b/111328563) Force nanopb first to work around ODR violations
29+
protobuf-nanopb
30+
2631
LevelDB::LevelDB
2732
absl_strings
2833
firebase_firestore_model
34+
firebase_firestore_nanopb
35+
firebase_firestore_protos_nanopb
2936
firebase_firestore_util
3037
EXCLUDE_FROM_ALL
3138
)

Firestore/Source/Local/FSTLevelDBMigrations.mm renamed to Firestore/core/src/firebase/firestore/local/leveldb_migrations.cc

Lines changed: 59 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -14,69 +14,69 @@
1414
* limitations under the License.
1515
*/
1616

17-
#import "Firestore/Source/Local/FSTLevelDBMigrations.h"
17+
#include "Firestore/core/src/firebase/firestore/local/leveldb_migrations.h"
1818

1919
#include <string>
20+
#include <utility>
2021

21-
#import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
22-
#import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
23-
22+
#include "Firestore/Protos/nanopb/firestore/local/target.nanopb.h"
2423
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
25-
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
26-
#include "absl/base/macros.h"
27-
#include "absl/memory/memory.h"
24+
#include "Firestore/core/src/firebase/firestore/nanopb/writer.h"
2825
#include "absl/strings/match.h"
29-
#include "leveldb/write_batch.h"
3026

31-
NS_ASSUME_NONNULL_BEGIN
27+
namespace firebase {
28+
namespace firestore {
29+
namespace local {
30+
31+
using leveldb::Iterator;
32+
using leveldb::Slice;
33+
using leveldb::Status;
34+
using leveldb::WriteOptions;
35+
using nanopb::Writer;
36+
37+
namespace {
3238

3339
/**
3440
* Schema version for the iOS client.
3541
*
36-
* Note that tables aren't a concept in LevelDB. They exist in our schema as just prefixes on keys.
37-
* This means tables don't need to be created but they also can't easily be dropped and re-created.
42+
* Note that tables aren't a concept in LevelDB. They exist in our schema as
43+
* just prefixes on keys. This means tables don't need to be created but they
44+
* also can't easily be dropped and re-created.
3845
*
3946
* Migrations:
40-
* * Migration 1 used to ensure the target_global row existed, without clearing it. No longer
41-
* required because migration 3 unconditionally clears it.
42-
* * Migration 2 used to ensure that the target_global row had a correct count of targets. No
43-
* longer required because migration 3 deletes them all.
44-
* * Migration 3 deletes the entire query cache to deal with cache corruption related to
45-
* limbo resolution. Addresses https://github.com/firebase/firebase-ios-sdk/issues/1548.
47+
* * Migration 1 used to ensure the target_global row existed, without
48+
* clearing it. No longer required because migration 3 unconditionally
49+
* clears it.
50+
* * Migration 2 used to ensure that the target_global row had a correct count
51+
* of targets. No longer required because migration 3 deletes them all.
52+
* * Migration 3 deletes the entire query cache to deal with cache corruption
53+
* related to limbo resolution. Addresses
54+
* https://github.com/firebase/firebase-ios-sdk/issues/1548.
4655
*/
47-
static FSTLevelDBSchemaVersion kSchemaVersion = 3;
48-
49-
using firebase::firestore::local::LevelDbDocumentTargetKey;
50-
using firebase::firestore::local::LevelDbQueryTargetKey;
51-
using firebase::firestore::local::LevelDbTargetDocumentKey;
52-
using firebase::firestore::local::LevelDbTargetGlobalKey;
53-
using firebase::firestore::local::LevelDbTargetKey;
54-
using firebase::firestore::local::LevelDbTransaction;
55-
using firebase::firestore::local::LevelDbVersionKey;
56-
using leveldb::Iterator;
57-
using leveldb::Status;
58-
using leveldb::Slice;
59-
using leveldb::WriteOptions;
56+
const LevelDbMigrations::SchemaVersion kSchemaVersion = 3;
6057

6158
/**
62-
* Save the given version number as the current version of the schema of the database.
59+
* Save the given version number as the current version of the schema of the
60+
* database.
6361
* @param version The version to save
6462
* @param transaction The transaction in which to save the new version number
6563
*/
66-
static void SaveVersion(FSTLevelDBSchemaVersion version, LevelDbTransaction *transaction) {
64+
void SaveVersion(LevelDbMigrations::SchemaVersion version,
65+
LevelDbTransaction* transaction) {
6766
std::string key = LevelDbVersionKey::Key();
6867
std::string version_string = std::to_string(version);
6968
transaction->Put(key, version_string);
7069
}
7170

72-
static void DeleteEverythingWithPrefix(const std::string &prefix, leveldb::DB *db) {
71+
void DeleteEverythingWithPrefix(const std::string& prefix, leveldb::DB* db) {
7372
bool more_deletes = true;
7473
while (more_deletes) {
7574
LevelDbTransaction transaction(db, "Delete everything with prefix");
7675
auto it = transaction.NewIterator();
7776

7877
more_deletes = false;
79-
for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix); it->Next()) {
78+
for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix);
79+
it->Next()) {
8080
if (transaction.changed_keys() >= 1000) {
8181
more_deletes = true;
8282
break;
@@ -89,7 +89,7 @@ static void DeleteEverythingWithPrefix(const std::string &prefix, leveldb::DB *d
8989
}
9090

9191
/** Migration 3. */
92-
static void ClearQueryCache(leveldb::DB *db) {
92+
void ClearQueryCache(leveldb::DB* db) {
9393
DeleteEverythingWithPrefix(LevelDbTargetKey::KeyPrefix(), db);
9494
DeleteEverythingWithPrefix(LevelDbDocumentTargetKey::KeyPrefix(), db);
9595
DeleteEverythingWithPrefix(LevelDbTargetDocumentKey::KeyPrefix(), db);
@@ -98,16 +98,22 @@ static void ClearQueryCache(leveldb::DB *db) {
9898
LevelDbTransaction transaction(db, "Drop query cache");
9999

100100
// Reset the target global entry too (to reset the target count).
101-
transaction.Put(LevelDbTargetGlobalKey::Key(), [FSTPBTargetGlobal message]);
101+
firestore_client_TargetGlobal target_global{};
102+
103+
std::string bytes;
104+
Writer writer = Writer::Wrap(&bytes);
105+
writer.WriteNanopbMessage(firestore_client_TargetGlobal_fields,
106+
&target_global);
107+
transaction.Put(LevelDbTargetGlobalKey::Key(), std::move(bytes));
102108

103109
SaveVersion(3, &transaction);
104110
transaction.Commit();
105111
}
106112

107-
@implementation FSTLevelDBMigrations
113+
} // namespace
108114

109-
+ (FSTLevelDBSchemaVersion)schemaVersionWithTransaction:
110-
(firebase::firestore::local::LevelDbTransaction *)transaction {
115+
LevelDbMigrations::SchemaVersion LevelDbMigrations::ReadSchemaVersion(
116+
LevelDbTransaction* transaction) {
111117
std::string key = LevelDbVersionKey::Key();
112118
std::string version_string;
113119
Status status = transaction->Get(key, &version_string);
@@ -118,22 +124,23 @@ + (FSTLevelDBSchemaVersion)schemaVersionWithTransaction:
118124
}
119125
}
120126

121-
+ (void)runMigrationsWithDatabase:(leveldb::DB *)database {
122-
[self runMigrationsWithDatabase:database upToVersion:kSchemaVersion];
127+
void LevelDbMigrations::RunMigrations(leveldb::DB* db) {
128+
RunMigrations(db, kSchemaVersion);
123129
}
124130

125-
+ (void)runMigrationsWithDatabase:(leveldb::DB *)database
126-
upToVersion:(FSTLevelDBSchemaVersion)toVersion {
127-
LevelDbTransaction transaction{database, "Read schema version"};
128-
FSTLevelDBSchemaVersion fromVersion = [self schemaVersionWithTransaction:&transaction];
131+
void LevelDbMigrations::RunMigrations(leveldb::DB* db,
132+
SchemaVersion to_version) {
133+
LevelDbTransaction transaction{db, "Read schema version"};
134+
SchemaVersion from_version = ReadSchemaVersion(&transaction);
129135

130-
// This must run unconditionally because schema migrations were added to iOS after the first
131-
// release. There may be clients that have never run any migrations that have existing targets.
132-
if (fromVersion < 3 && toVersion >= 3) {
133-
ClearQueryCache(database);
136+
// This must run unconditionally because schema migrations were added to iOS
137+
// after the first release. There may be clients that have never run any
138+
// migrations that have existing targets.
139+
if (from_version < 3 && to_version >= 3) {
140+
ClearQueryCache(db);
134141
}
135142
}
136143

137-
@end
138-
139-
NS_ASSUME_NONNULL_END
144+
} // namespace local
145+
} // namespace firestore
146+
} // namespace firebase

0 commit comments

Comments
 (0)