Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Adds the `List(max_results, page_token)` and `ListAll()` methods to the `firebase::storage::StorageReference` C++ API. These methods allow you to list objects and common prefixes (directories) within a storage location. Features: - Paginated listing using `List(max_results, page_token)`. - Comprehensive listing using `ListAll()`. - Returns a `Future<ListResult>`, where `ListResult` contains a list of `StorageReference` objects for items and prefixes, and a page token for continuation. - Implemented for Android by calling the underlying Firebase Android SDK's list operations via JNI. - Implemented for iOS by calling the underlying Firebase iOS SDK's list operations. - Desktop platform provides stubs that return `kErrorUnimplemented`. - Includes comprehensive integration tests covering various scenarios such as basic listing, pagination, empty folders, and listing non-existent paths.
jonsimantov commented Jun 9, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Here's a summary of the changes:
- Integration Tests:
- I removed `SKIP_TEST_ON_ANDROID_EMULATOR` from the `ListAllBasic` and `ListPaginated` tests.
- I refactored the list tests so they create their own unique root folders instead of using a shared one in the test fixture. This improves test isolation.
- Code Style:
- I removed unnecessary comments that were explaining header includes.
- I removed placeholder comments from public headers.
- I updated the copyright year to 2025 in all newly added files.
- Android Performance:
- I optimized Android's `ListResultInternal` to cache converted items, prefixes, and the page token. This will avoid repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
- I simplified the `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.Incorporates feedback from the initial review of the List API:
- Build Fix:
- Explicitly namespaced StorageReference in list_result.h to resolve undeclared identifier error.
- Integration Tests:
- Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
- Refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, improving test isolation.
- Code Style:
- Removed unnecessary comments explaining header includes.
- Removed placeholder comments from public headers.
- Updated copyright year to 2025 in all newly added files.
- Ran code formatter (scripts/format_code.py -git_diff) on all changed files.
- Android Performance:
- Optimized Android's ListResultInternal to cache converted items, prefixes, and page token. This avoids repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
- Simplified ListResult cleanup by removing the ListResultInternalCommon class. ListResult now directly manages the cleanup registration of its internal_ member, similar to StorageReference.…etween `storage_reference.h` and `list_result.h` by forward-declaring `ListResult` in `storage_reference.h`.
This commit also incorporates your previous feedback from the initial review of the List API:
- Build Fixes:
- Forward-declared `ListResult` in `storage_reference.h`.
- Reverted non-standard include placement in `list_result.h`.
- Explicitly namespaced `StorageReference` in `list_result.h` (previous attempt, kept).
- Integration Tests:
- Removed `SKIP_TEST_ON_ANDROID_EMULATOR` from `ListAllBasic` and `ListPaginated` tests.
- Refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, improving test isolation.
- Code Style:
- Removed unnecessary comments explaining header includes.
- Removed placeholder comments from public headers.
- Updated copyright year to 2025 in all newly added files.
- Ran the code formatter on all changed files.
- Android Performance:
- Optimized Android's `ListResultInternal` to cache converted items, prefixes, and page token. This avoids repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
- Simplified `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.❌ Integration test FAILEDRequested by @jonsimantov on commit b28ff1d
Add flaky tests to go/fpl-cpp-flake-tracker |
…ack for the List API.
Here's a summary of the changes:
I resolved build errors related to includes and circular dependencies for the Storage List API. Specifically:
- I fixed a circular include dependency between `storage_reference.h` and `list_result.h` by forward-declaring `ListResult` in `storage_reference.h`.
- I corrected the include path for platform-specific `StorageInternal` in `list_result.cc`.
This update also incorporates your previous feedback from the initial review:
- Build Fixes:
- I forward-declared `ListResult` in `storage_reference.h`.
- I reverted non-standard include placement in `list_result.h`.
- I explicitly namespaced `StorageReference` in `list_result.h` (this was a previous attempt that I kept).
- I corrected the `StorageInternal` include in `list_result.cc`.
- Integration Tests:
- I removed `SKIP_TEST_ON_ANDROID_EMULATOR` from `ListAllBasic` and `ListPaginated` tests.
- I refactored list tests to create their own unique root folders instead of using a shared one in the test fixture, which should improve test isolation.
- Code Style:
- I removed unnecessary comments explaining header includes.
- I removed placeholder comments from public headers.
- I updated the copyright year to 2025 in all newly added files.
- I applied code formatting to all changed files.
- Android Performance:
- I optimized Android's `ListResultInternal` to cache converted items, prefixes, and page token. This should avoid repeated JNI calls on subsequent accesses.
- Cleanup Mechanism:
- I simplified `ListResult` cleanup by removing the `ListResultInternalCommon` class. `ListResult` now directly manages the cleanup registration of its `internal_` member, similar to `StorageReference`.Corrects include directives for platform-specific StorageInternal
definitions in list_result_android.h, list_result_ios.h, and
list_result_desktop.h. Removes includes for the deleted
ListResultInternalCommon class.
This commit also incorporates previous feedback and build fixes:
- Build Fixes:
- Resolved circular include dependency between storage_reference.h and
list_result.h by forward-declaring ListResult in storage_reference.h.
- Corrected includes in list_result.cc and platform-specific
list_result_*.h files to use appropriate platform-specific
headers for StorageInternal definitions.
- Integration Tests:
- Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
- Refactored list tests to create their own unique root folders.
- Code Style:
- Removed unnecessary comments.
- Updated copyright years to 2025.
- Ran code formatter on all changed files.
- Android Performance:
- Optimized Android's ListResultInternal to cache converted data.
- Cleanup Mechanism:
- Simplified ListResult cleanup logic.Addresses all outstanding review comments and build errors for the
Storage List API feature. This includes:
- Build Fixes:
- Resolved circular include dependency between storage_reference.h and
list_result.h by forward-declaring ListResult in storage_reference.h.
- Corrected includes in list_result.cc and platform-specific
list_result_*.h files to use appropriate platform-specific
headers for StorageInternal definitions.
- Code Cleanup:
- Thoroughly removed commented-out code and unused includes.
- Removed all explanatory comments next to #include directives.
- Eliminated stray or development-related comments.
- Integration Tests:
- Removed SKIP_TEST_ON_ANDROID_EMULATOR from ListAllBasic and ListPaginated tests.
- Refactored list tests to create their own unique root folders.
- Code Style:
- Updated copyright year to 2025 in all newly added files.
- Ran code formatter (scripts/format_code.py -git_diff) on all changed files.
- Android Performance:
- Optimized Android's ListResultInternal to cache converted data.
- Cleanup Mechanism:
- Simplified ListResult cleanup logic by removing ListResultInternalCommon.Sign up for free to join this conversation on . Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Adds the
List(max_results, page_token)andListAll()methods to thefirebase::storage::StorageReferenceC++ API.These methods allow you to list objects and common prefixes (directories) within a storage location.
Features:
List(max_results, page_token).ListAll().Future<ListResult>, whereListResultcontains a list ofStorageReferenceobjects for items and prefixes, and a page token for continuation.kErrorUnimplemented.Description
Testing
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.