Skip to content

Commit ff21f1e

Browse files
authored
Make a Filesystem class that can be subclassed for testing (#4647)
This change does not yet change all the classes that use the filesystem to take Filesystem as a dependency. Those changes will come in follow-up PRs. This change is motivated by feedback in #4609. There I had created a complicated interface to LevelDbOpener just so that different migration scenarios could be tested, when in reality, I just needed to be able to plumb through different values for the result of AppDataDir and related functions.
1 parent 423a62f commit ff21f1e

File tree

14 files changed

+353
-365
lines changed

14 files changed

+353
-365
lines changed

Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
using firebase::firestore::remote::GrpcConnection;
6868
using firebase::firestore::util::AsyncQueue;
6969
using firebase::firestore::util::CreateAutoId;
70+
using firebase::firestore::util::Filesystem;
7071
using firebase::firestore::util::Path;
7172
using firebase::firestore::util::Status;
7273
using firebase::firestore::util::StatusOr;
@@ -142,6 +143,7 @@ - (void)tearDown {
142143
* with each other.
143144
*/
144145
- (void)clearPersistenceOnce {
146+
auto *fs = Filesystem::Default();
145147
static bool clearedPersistence = false;
146148

147149
@synchronized([FSTIntegrationTestCase class]) {
@@ -150,7 +152,7 @@ - (void)clearPersistenceOnce {
150152
ASSERT_OK(maybe_dir);
151153

152154
Path levelDBDir = maybe_dir.ValueOrDie();
153-
Status status = util::RecursivelyDelete(levelDBDir);
155+
Status status = fs->RecursivelyRemove(levelDBDir);
154156
ASSERT_OK(status);
155157

156158
clearedPersistence = true;

Firestore/Protos/CMakeLists.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ add_custom_command(
241241
COMMENT "Generating C++ protobuf sources"
242242
OUTPUT ${PROTOBUF_CPP_GENERATED_SOURCES}
243243
COMMAND
244-
${CMAKE_CURRENT_SOURCE_DIR}/build_protos.py
244+
python
245+
${CMAKE_CURRENT_SOURCE_DIR}/build_protos.py
245246
--cpp
246247
--protoc=$<TARGET_FILE:protoc>
247248
--output_dir=${OUTPUT_DIR}

Firestore/core/src/firebase/firestore/local/leveldb_persistence.cc

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ namespace {
4343
using auth::User;
4444
using leveldb::DB;
4545
using model::ListenSequenceNumber;
46+
using util::Filesystem;
4647
using util::Path;
4748
using util::Status;
4849
using util::StatusOr;
@@ -75,10 +76,11 @@ std::set<std::string> CollectUserSet(LevelDbTransaction* transaction) {
7576

7677
util::StatusOr<std::unique_ptr<LevelDbPersistence>> LevelDbPersistence::Create(
7778
util::Path dir, LocalSerializer serializer, const LruParams& lru_params) {
79+
auto* fs = Filesystem::Default();
7880
Status status = EnsureDirectory(dir);
7981
if (!status.ok()) return status;
8082

81-
status = util::ExcludeFromBackups(dir);
83+
status = fs->ExcludeFromBackups(dir);
8284
if (!status.ok()) return status;
8385

8486
StatusOr<std::unique_ptr<DB>> created = OpenDb(dir);
@@ -123,7 +125,8 @@ LevelDbPersistence::LevelDbPersistence(std::unique_ptr<leveldb::DB> db,
123125
// MARK: - Storage location
124126

125127
StatusOr<Path> LevelDbPersistence::AppDataDirectory() {
126-
return util::AppDataDir(kReservedPathComponent);
128+
auto* fs = Filesystem::Default();
129+
return fs->AppDataDir(kReservedPathComponent);
127130
}
128131

129132
util::Path LevelDbPersistence::StorageDirectory(
@@ -149,7 +152,8 @@ util::Path LevelDbPersistence::StorageDirectory(
149152
// MARK: - Startup
150153

151154
Status LevelDbPersistence::EnsureDirectory(const Path& dir) {
152-
Status status = util::RecursivelyCreateDir(dir);
155+
auto* fs = Filesystem::Default();
156+
Status status = fs->RecursivelyCreateDir(dir);
153157
if (!status.ok()) {
154158
return Status{Error::Internal, "Failed to create persistence directory"}
155159
.CausedBy(status);
@@ -191,14 +195,17 @@ util::Status LevelDbPersistence::ClearPersistence(
191195
Path leveldb_dir =
192196
StorageDirectory(database_info, maybe_data_dir.ValueOrDie());
193197
LOG_DEBUG("Clearing persistence for path: %s", leveldb_dir.ToUtf8String());
194-
return util::RecursivelyDelete(leveldb_dir);
198+
auto* fs = Filesystem::Default();
199+
return fs->RecursivelyRemove(leveldb_dir);
195200
}
196201

197202
int64_t LevelDbPersistence::CalculateByteSize() {
203+
auto* fs = Filesystem::Default();
204+
198205
int64_t count = 0;
199206
auto iter = util::DirectoryIterator::Create(directory_);
200207
for (; iter->Valid(); iter->Next()) {
201-
int64_t file_size = util::FileSize(iter->file()).ValueOrDie();
208+
int64_t file_size = fs->FileSize(iter->file()).ValueOrDie();
202209
count += file_size;
203210
}
204211

Firestore/core/src/firebase/firestore/remote/grpc_connection.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ namespace remote {
4141
using auth::Token;
4242
using core::DatabaseInfo;
4343
using model::DatabaseId;
44+
using util::Filesystem;
4445
using util::Path;
4546
using util::Status;
4647
using util::StatusOr;
@@ -207,9 +208,10 @@ std::shared_ptr<grpc::Channel> GrpcConnection::CreateChannel() const {
207208
}
208209

209210
// For tests only
211+
auto* fs = Filesystem::Default();
210212
args.SetSslTargetNameOverride(host_config->target_name);
211213
Path path = host_config->certificate_path;
212-
StatusOr<std::string> test_certificate = ReadFile(path);
214+
StatusOr<std::string> test_certificate = fs->ReadFile(path);
213215
HARD_ASSERT(test_certificate.ok(),
214216
StringFormat("Unable to open root certificates at file path %s",
215217
path.ToUtf8String())

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,6 @@ cc_library(
120120
filesystem.h
121121
filesystem_apple.mm
122122
filesystem_common.cc
123-
filesystem_detail.h
124123
filesystem_posix.cc
125124
path.cc
126125
path.h
@@ -135,7 +134,6 @@ cc_library(
135134
SOURCES
136135
filesystem.h
137136
filesystem_common.cc
138-
filesystem_detail.h
139137
filesystem_posix.cc
140138
path.cc
141139
path.h
@@ -150,7 +148,6 @@ cc_library(
150148
SOURCES
151149
filesystem.h
152150
filesystem_common.cc
153-
filesystem_detail.h
154151
filesystem_win.cc
155152
path.cc
156153
path.h

Firestore/core/src/firebase/firestore/util/filesystem.h

Lines changed: 124 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -28,88 +28,139 @@ namespace firebase {
2828
namespace firestore {
2929
namespace util {
3030

31-
// High-level routines for the manipulating the filesystem. All filesystems
32-
// are required to implement these routines.
33-
3431
/**
35-
* Answers the question "is this path a directory? The path is not required to
36-
* have a trailing slash.
37-
*
38-
* Typical return codes include:
39-
* * Ok - The path exists and is a directory.
40-
* * FailedPrecondition - Some component of the path is not a directory. This
41-
* does not necessarily imply that the path exists and is a file.
42-
* * NotFound - The path does not exist
43-
* * PermissionDenied - Insufficient permissions to access the path.
32+
* A high-level interface describing filesystem operations.
4433
*/
45-
Status IsDirectory(const Path& path);
34+
class Filesystem {
35+
public:
36+
Filesystem(const Filesystem&) = delete;
37+
Filesystem& operator=(const Filesystem&) = delete;
4638

47-
/**
48-
* Recursively creates all the directories in the path name if they don't
49-
* exist.
50-
*
51-
* @return Ok if the directory was created or already existed.
52-
*/
53-
Status RecursivelyCreateDir(const Path& path);
39+
/**
40+
* Returns a singleton default filesystem implementation for the current
41+
* operating system.
42+
*/
43+
static Filesystem* Default();
5444

55-
/**
56-
* Recursively deletes the contents of the given pathname. If the pathname is
57-
* a file, deletes just that file. The the pathname is a directory, deletes
58-
* everything within the directory.
59-
*
60-
* @return Ok if the directory was deleted or did not exist.
61-
*/
62-
Status RecursivelyDelete(const Path& path);
45+
/**
46+
* Returns a system-defined best directory in which to create application
47+
* data. Values vary wildly across platforms. They include:
48+
*
49+
* * iOS: $container/Documents/$app_name
50+
* * Linux: $HOME/.local/share/$app_name
51+
* * macOS: $HOME/.$app_name
52+
* * Other UNIX: $HOME/.$app_name
53+
* * tvOS: $HOME/Library/Caches/$app_name
54+
* * Windows: %USERPROFILE%/AppData/Local
55+
*
56+
* Note: the returned path is just where the system thinks the application
57+
* data should be stored, but AppDataDir does not actually guarantee that this
58+
* path exists.
59+
*
60+
* @param app_name The name of the application.
61+
*/
62+
virtual StatusOr<Path> AppDataDir(absl::string_view app_name);
6363

64-
/**
65-
* Marks the given directory as excluded from platform-specific backup schemes
66-
* like iCloud backup.
67-
*/
68-
Status ExcludeFromBackups(const Path& dir);
64+
/**
65+
* Returns system-defined best directory in which to create temporary files.
66+
* Typical return values are like `/tmp` on UNIX systems. Clients should
67+
* create randomly named directories or files within this location to avoid
68+
* collisions. Absent any changes that might affect the underlying calls, the
69+
* value returned from TempDir will be stable over time.
70+
*
71+
* Note: the returned path is just where the system thinks temporary files
72+
* should be stored, but TempDir does not actually guarantee that this path
73+
* exists.
74+
*/
75+
virtual Path TempDir();
6976

70-
/**
71-
* Returns a system-defined best directory in which to create application data.
72-
* Values vary wildly across platforms. They include:
73-
*
74-
* * iOS: $container/Documents/$app_name
75-
* * Linux: $HOME/.local/share/$app_name
76-
* * macOS: $HOME/.$app_name
77-
* * Other UNIX: $HOME/.$app_name
78-
* * tvOS: $HOME/Library/Caches/$app_name
79-
* * Windows: %USERPROFILE%/AppData/Local
80-
*
81-
* Note: the returned path is just where the system thinks the application data
82-
* should be stored, but AppDataDir does not actually guarantee that this path
83-
* exists.
84-
*
85-
* @param app_name The name of the application.
86-
*/
87-
StatusOr<Path> AppDataDir(absl::string_view app_name);
77+
/**
78+
* Answers the question "is this path a directory? The path is not required to
79+
* have a trailing slash.
80+
*
81+
* Typical return codes include:
82+
* * Ok - The path exists and is a directory.
83+
* * FailedPrecondition - Some component of the path is not a directory.
84+
* This does not necessarily imply that the path exists and is a file.
85+
* * NotFound - The path does not exist
86+
* * PermissionDenied - Insufficient permissions to access the path.
87+
*/
88+
virtual Status IsDirectory(const Path& path);
8889

89-
/**
90-
* Returns system-defined best directory in which to create temporary files.
91-
* Typical return values are like `/tmp` on UNIX systems. Clients should create
92-
* randomly named directories or files within this location to avoid collisions.
93-
* Absent any changes that might affect the underlying calls, the value returned
94-
* from TempDir will be stable over time.
95-
*
96-
* Note: the returned path is just where the system thinks temporary files
97-
* should be stored, but TempDir does not actually guarantee that this path
98-
* exists.
99-
*/
100-
Path TempDir();
90+
/**
91+
* On success, returns the size in bytes of the file specified by
92+
* `path`.
93+
*/
94+
virtual StatusOr<int64_t> FileSize(const Path& path);
10195

102-
/**
103-
* On success, returns the size in bytes of the file specified by
104-
* `path`.
105-
*/
106-
StatusOr<int64_t> FileSize(const Path& path);
96+
/**
97+
* Recursively creates all the directories in the path name if they don't
98+
* exist.
99+
*
100+
* @return Ok if the directory was created or already existed.
101+
*/
102+
virtual Status RecursivelyCreateDir(const Path& path);
107103

108-
/**
109-
* On success, opens the file at the given `path` and returns its contents as
110-
* a string.
111-
*/
112-
StatusOr<std::string> ReadFile(const Path& path);
104+
/**
105+
* Recursively deletes the contents of the given pathname. If the pathname is
106+
* a file, deletes just that file. The the pathname is a directory, deletes
107+
* everything within the directory.
108+
*
109+
* @return Ok if the directory was deleted or did not exist.
110+
*/
111+
virtual Status RecursivelyRemove(const Path& path);
112+
113+
/**
114+
* Creates the given directory. The immediate parent directory must already
115+
* exist and not already be a file.
116+
*
117+
* @return Ok if the directory was created or already existed. On some systems
118+
* this may also return Ok if a regular file exists at the given path.
119+
*/
120+
virtual Status CreateDir(const Path& path);
121+
122+
/**
123+
* Deletes the given directory if it exists.
124+
*
125+
* @return Ok if the directory was deleted or did not exist. Returns a
126+
* system-defined error if the path is not a directory or the directory is
127+
* non-empty.
128+
*/
129+
virtual Status RemoveDir(const Path& path);
130+
131+
/**
132+
* Deletes the given file if it exists.
133+
*
134+
* @return Ok if the file was deleted or did not exist. Returns a
135+
* system-defined error if the path exists but is not a regular file.
136+
*/
137+
virtual Status RemoveFile(const Path& path);
138+
139+
/**
140+
* Recursively deletes the contents of the given pathname that is known to be
141+
* a directory.
142+
*
143+
* @return Ok if the directory was deleted or did not exist. Returns a
144+
* system-defined error if the path exists but is not a directory.
145+
*
146+
*/
147+
virtual Status RecursivelyRemoveDir(const Path& path);
148+
149+
/**
150+
* Marks the given directory as excluded from platform-specific backup schemes
151+
* like iCloud backup.
152+
*/
153+
virtual Status ExcludeFromBackups(const Path& dir);
154+
155+
/**
156+
* On success, opens the file at the given `path` and returns its contents as
157+
* a string.
158+
*/
159+
virtual StatusOr<std::string> ReadFile(const Path& path);
160+
161+
protected:
162+
Filesystem() = default;
163+
};
113164

114165
/**
115166
* Implements an iterator over the contents of a directory. Initializes to the

0 commit comments

Comments
 (0)