Skip to content

Commit 06c4128

Browse files
Here's the commit message I've drafted:
feat: Add stub List/ListAll methods with corrected PIMPL pattern This commit introduces stub implementations for StorageReference's `List()` and `ListAll()` methods in the Firebase C++ Storage SDK. It follows a corrected PIMPL (pointer to implementation) pattern for the `ListResult` class, strictly adhering to the internal architecture observed in classes like `firebase::storage::Metadata` and its `MetadataInternalCommon` helper. This version addresses feedback from previous attempts and ensures: 1. **`ListResultInternalCommon` as Static Helpers**: - A class `firebase::storage::internal::ListResultInternalCommon` is defined within `storage/src/common/list_result.cc`. - It contains only static methods responsible for managing the lifecycle (cleanup registration, PIMPL deletion) of `ListResult`'s internal implementation. This mirrors `MetadataInternalCommon`. 2. **Platform-Specific `ListResultInternal`**: - Each platform (Desktop, Android, iOS) defines its own concrete `firebase::storage::internal::ListResultInternal` class. - These are located in files like `storage/src/desktop/list_result_desktop.h` (and `.cc`), `storage/src/android/list_result_android.h` (and `.cc`), and `storage/src/ios/list_result_ios.h` (and `.mm`). - These classes hold the stub data (empty item/prefix lists, empty page token) and a pointer to their platform's `StorageReferenceInternal` for context (primarily for cleanup). - They provide a copy constructor for `ListResult`'s copy operations. - They do not inherit from a common PIMPL base class. 3. **Public `ListResult` Refactoring**: - The public `firebase::storage::ListResult` class now holds a raw pointer (`internal::ListResultInternal* internal_`) to its platform-specific PIMPL object. - Its constructors, destructor, copy, and move assignment operators utilize the static helper methods in `ListResultInternalCommon` to manage the `internal_` pointer's lifecycle and cleanup registration, similar to how `Metadata` manages its `internal_`. 4. **`StorageReference` Methods Update**: - `StorageReference::ListAll()` and `StorageReference::List()` in `storage/src/common/storage_reference.cc` now correctly create `ListResult` instances. They instantiate the appropriate platform-specific `internal::ListResultInternal` via a direct `new internal::ListResultInternal(...)` call (relying on the build system to link the correct platform's implementation) and pass this PIMPL object to a private constructor of `ListResult`. 5. **Build System**: - `storage/CMakeLists.txt` was updated to include the correctly named platform-specific source files (e.g., `list_result_desktop.cc`). 6. **Integration Tests**: - Existing integration tests were reviewed and confirmed to be compatible with these internal changes. This implementation provides the `List` and `ListAll` API as stubs, returning empty results, with a robust internal structure that aligns with the SDK's established PIMPL and lifecycle management patterns.
1 parent fe7932c commit 06c4128

17 files changed

+457
-253
lines changed

storage/CMakeLists.txt

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ set(android_SRCS
3838
src/android/controller_android.cc
3939
src/android/metadata_android.cc
4040
src/android/storage_android.cc
41-
src/android/storage_reference_android.cc)
41+
src/android/storage_reference_android.cc
42+
src/android/list_result_android.cc)
4243

4344
# Source files used by the iOS implementation.
4445
set(ios_SRCS
@@ -47,7 +48,8 @@ set(ios_SRCS
4748
src/ios/metadata_ios.mm
4849
src/ios/storage_ios.mm
4950
src/ios/storage_reference_ios.mm
50-
src/ios/util_ios.mm)
51+
src/ios/util_ios.mm
52+
src/ios/list_result_ios.mm)
5153

5254
# Source files used by the desktop implementation.
5355
set(desktop_SRCS
@@ -58,7 +60,8 @@ set(desktop_SRCS
5860
src/desktop/rest_operation.cc
5961
src/desktop/storage_desktop.cc
6062
src/desktop/storage_path.cc
61-
src/desktop/storage_reference_desktop.cc)
63+
src/desktop/storage_reference_desktop.cc
64+
src/desktop/list_result_desktop.cc)
6265

6366
if(ANDROID)
6467
set(storage_platform_SRCS
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// File: storage/src/android/list_result_android.cc
2+
#include "storage/src/android/list_result_android.h"
3+
4+
namespace firebase {
5+
namespace storage {
6+
namespace internal {
7+
8+
ListResultInternal::ListResultInternal(
9+
StorageReferenceInternal* platform_sri,
10+
const ListResultInternal* other_to_copy_from)
11+
: platform_sri_(platform_sri) {
12+
if (other_to_copy_from) {
13+
items_ = other_to_copy_from->items_;
14+
prefixes_ = other_to_copy_from->prefixes_;
15+
page_token_ = other_to_copy_from->page_token_;
16+
}
17+
}
18+
19+
} // namespace internal
20+
} // namespace storage
21+
} // namespace firebase
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// File: storage/src/android/list_result_android.h
2+
#ifndef FIREBASE_STORAGE_CLIENT_CPP_SRC_ANDROID_LIST_RESULT_ANDROID_H_
3+
#define FIREBASE_STORAGE_CLIENT_CPP_SRC_ANDROID_LIST_RESULT_ANDROID_H_
4+
5+
#include <string>
6+
#include <vector>
7+
8+
#include "firebase/storage/storage_reference.h"
9+
#include "storage/src/android/storage_reference_android.h" // Defines firebase::storage::internal::StorageReferenceInternal for android
10+
#include "storage/src/android/storage_internal_android.h" // Defines firebase::storage::internal::StorageInternal for android
11+
12+
namespace firebase {
13+
namespace storage {
14+
namespace internal {
15+
16+
class ListResultInternal {
17+
public:
18+
explicit ListResultInternal(
19+
StorageReferenceInternal* platform_sri,
20+
const ListResultInternal* other_to_copy_from = nullptr);
21+
22+
const std::vector<StorageReference>& items() const { return items_; }
23+
const std::vector<StorageReference>& prefixes() const { return prefixes_; }
24+
const std::string& page_token() const { return page_token_; }
25+
26+
StorageReferenceInternal* storage_reference_internal() const {
27+
return platform_sri_;
28+
}
29+
StorageInternal* associated_storage_internal() const {
30+
return platform_sri_ ? platform_sri_->storage_internal() : nullptr;
31+
}
32+
33+
private:
34+
ListResultInternal& operator=(const ListResultInternal&);
35+
36+
StorageReferenceInternal* platform_sri_;
37+
std::vector<StorageReference> items_;
38+
std::vector<StorageReference> prefixes_;
39+
std::string page_token_;
40+
};
41+
42+
} // namespace internal
43+
} // namespace storage
44+
} // namespace firebase
45+
46+
#endif // FIREBASE_STORAGE_CLIENT_CPP_SRC_ANDROID_LIST_RESULT_ANDROID_H_

storage/src/android/storage_reference_android.cc

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
#include "app/src/include/firebase/app.h"
2424
#include "app/src/util_android.h"
2525
#include "storage/src/android/controller_android.h"
26-
#include "storage/src/android/list_result_android.h" // For ListResultAndroid
26+
// Removed: #include "storage/src/android/list_result_android.h"
2727
#include "storage/src/android/metadata_android.h"
2828
#include "storage/src/android/storage_android.h"
2929
#include "storage/src/include/firebase/storage.h"
@@ -735,12 +735,6 @@ jint StorageReferenceInternal::CppByteUploaderReadBytes(
735735
return data_read;
736736
}
737737

738-
::firebase::storage::internal::ListResultInternal* StorageReferenceInternal::CreateListResultInternal() {
739-
// Create and return an instance of the Android-specific ListResultInternal.
740-
// 'this' is the current StorageReferenceInternal (Android version)
741-
return new ::firebase::storage::internal::ListResultAndroid(this);
742-
}
743-
744738
} // namespace internal
745739
} // namespace storage
746740
} // namespace firebase

storage/src/android/storage_reference_android.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
#include "app/src/reference_counted_future_impl.h"
2424
#include "app/src/util_android.h"
2525
#include "storage/src/android/storage_android.h"
26-
#include "storage/src/common/list_result_internal.h" // Added for ListResultInternal
26+
// Removed: #include "storage/src/common/list_result_internal.h"
2727
#include "storage/src/include/firebase/storage/storage_reference.h"
2828

2929
namespace firebase {
@@ -154,10 +154,6 @@ class StorageReferenceInternal {
154154
// StorageInternal instance we are associated with.
155155
StorageInternal* storage_internal() const { return storage_; }
156156

157-
// Creates a new platform-specific ListResultInternal object.
158-
// Caller takes ownership.
159-
virtual ::firebase::storage::internal::ListResultInternal* CreateListResultInternal();
160-
161157
private:
162158
static void FutureCallback(JNIEnv* env, jobject result,
163159
util::FutureResult result_code,

0 commit comments

Comments
 (0)