Skip to content

[Storage] Implement List API across Android, iOS, and Desktop#1841

Draft
AustinBenoit wants to merge 13 commits intomainfrom
add-storage-list-api-15739532705466063966
Draft

[Storage] Implement List API across Android, iOS, and Desktop#1841
AustinBenoit wants to merge 13 commits intomainfrom
add-storage-list-api-15739532705466063966

Conversation

@AustinBenoit
Copy link
Contributor

Implements the Storage List API per the requirements. All platforms correctly return a Future representing a StorageListResult, and parsing logic is complete. Tested extensively in integration_tests.


PR created automatically by Jules for task 15739532705466063966 started by @AustinBenoit

Added the paginated `List` API to the Firebase Storage C++ SDK.
- Android: Interoperates via JNI with `com/google/firebase/storage/ListResult`.
- iOS: Connects to `FIRStorageListOptions` and `FIRStorageListResult` using Objective-C.
- Desktop: Uses the Firebase Storage REST API `o` endpoint to retrieve folder prefixes and files.

Included integration tests to verify successful nested directory creation, listing results, and pagination via `page_token`.

Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces the Storage List API, providing a consistent way to list files and folders within Firebase Storage across Android, iOS, and Desktop platforms. The changes include platform-specific implementations and a unified interface for retrieving and parsing list results, along with comprehensive integration tests to validate the functionality.

Highlights

  • Implementation: This PR implements the Storage List API across Android, iOS, and Desktop platforms, enabling retrieval of file and folder listings.
  • Functionality: The implementation includes correct return of a Future representing a StorageListResult and complete parsing logic for the results.
  • Testing: Extensive integration tests have been performed to ensure the API functions correctly across all supported platforms.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • storage/CMakeLists.txt
    • Added src/common/list_result.cc to the list of source files.
  • storage/integration_test/src/integration_test.cc
    • Added a new integration test, TestList, to verify the functionality of the Storage List API, including pagination.
  • storage/src/android/storage_reference_android.cc
    • Added JNI method lookup and registration for ListResult class and methods.
    • Added List method to StorageReferenceInternal to call the native list method.
    • Implemented callback to handle ListResult and complete the future with StorageListResult.
  • storage/src/android/storage_reference_android.h
    • Added declaration for List and ListLastResult methods.
  • storage/src/common/list_result.cc
    • Added implementation for StorageListResult class, including constructors, destructor, and methods to access prefixes, items, and next page token.
  • storage/src/common/list_result_internal.h
    • Added internal representation of StorageListResult, including vectors for prefixes and items, and a string for the next page token.
  • storage/src/common/storage_reference.cc
    • Added List and ListLastResult methods to the StorageReference class, delegating to the internal implementation.
  • storage/src/desktop/curl_requests.cc
    • Added ReturnedListResponse class to handle the response from the list API.
    • Implemented parsing of the JSON response to extract prefixes, items, and the next page token.
  • storage/src/desktop/curl_requests.h
    • Added declaration for ReturnedListResponse class.
  • storage/src/desktop/storage_reference_desktop.cc
    • Implemented List method to retrieve list of items and prefixes from storage.
    • Implemented ListLastResult method to return the last result of the list operation.
  • storage/src/desktop/storage_reference_desktop.h
    • Added StorageReferenceFn enum value for list operations.
    • Added List and ListLastResult method declarations.
  • storage/src/include/firebase/storage/list_result.h
    • Added StorageListResult class declaration, including methods to access prefixes, items, and the next page token.
  • storage/src/include/firebase/storage/storage_reference.h
    • Added List and ListLastResult method declarations.
    • Added constant for default max results per page.
  • storage/src/ios/storage_reference_ios.h
    • Added List and ListLastResult method declarations.
  • storage/src/ios/storage_reference_ios.mm
    • Implemented List method to retrieve list of items and prefixes from storage.
    • Implemented ListLastResult method to return the last result of the list operation.
Activity
  • AustinBenoit created the pull request to implement the Storage List API across Android, iOS, and Desktop.
  • The PR includes implementation of StorageListResult and parsing logic.
  • Integration tests were added to verify the functionality.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

public:
StorageListResult();
~StorageListResult();
StorageListResult(const StorageListResult& other);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation issue: warning: Member StorageListResult(const StorageListResult &other) (function) of class firebase::storage::StorageListResult is not documented.

StorageListResult();
~StorageListResult();
StorageListResult(const StorageListResult& other);
StorageListResult& operator=(const StorageListResult& other);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation issue: warning: Member operator=(const StorageListResult &other) (function) of class firebase::storage::StorageListResult is not documented.

StorageListResult& operator=(const StorageListResult& other);

#if defined(FIREBASE_USE_MOVE_OPERATORS) || defined(DOXYGEN)
StorageListResult(StorageListResult&& other);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation issue: warning: Member StorageListResult(StorageListResult &&other) (function) of class firebase::storage::StorageListResult is not documented.


#if defined(FIREBASE_USE_MOVE_OPERATORS) || defined(DOXYGEN)
StorageListResult(StorageListResult&& other);
StorageListResult& operator=(StorageListResult&& other);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation issue: warning: Member operator=(StorageListResult &&other) (function) of class firebase::storage::StorageListResult is not documented.

Future<Metadata> PutFileLastResult();

// Android's maximum limit is 1000, so use this as the default max results.
static const int kDefaultListMaxResultsPerPage = 1000;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation issue: warning: Member kDefaultListMaxResultsPerPage (variable) of class firebase::storage::StorageReference is not documented.

Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the Storage List API across Android, iOS, and Desktop platforms, along with corresponding integration tests. The implementation looks solid and covers the required functionality. I've provided a few suggestions to improve performance by using move semantics when constructing result objects, which avoids unnecessary data copying. I also pointed out a piece of dead code in the desktop implementation that can be removed for better code clarity. Overall, great work on implementing this feature across all platforms.

Comment on lines +319 to +320
StorageListResultInternal* list_internal =
new StorageListResultInternal(prefixes, items, page_token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve performance, use std::move to transfer ownership of the local variables to the StorageListResultInternal constructor. This avoids unnecessary copies of the vectors and the string, as they are no longer needed in this scope. I've made a related suggestion to update the constructor to take advantage of this.

    StorageListResultInternal* list_internal =
        new StorageListResultInternal(std::move(prefixes), std::move(items), std::move(page_token));

Comment on lines +31 to +34
StorageListResultInternal(const std::vector<StorageReference>& prefixes,
const std::vector<StorageReference>& items,
const std::string& next_page_token)
: prefixes_(prefixes), items_(items), next_page_token_(next_page_token) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This constructor copies its arguments, which can be inefficient. Since the call sites are passing temporary local variables, it's better to move the data instead of copying it. By changing the constructor to take its arguments by value, you can then use std::move to efficiently transfer ownership to the member variables. This is known as the 'pass-by-value and move' idiom.

Suggested change
StorageListResultInternal(const std::vector<StorageReference>& prefixes,
const std::vector<StorageReference>& items,
const std::string& next_page_token)
: prefixes_(prefixes), items_(items), next_page_token_(next_page_token) {}
StorageListResultInternal(std::vector<StorageReference> prefixes,
std::vector<StorageReference> items,
std::string next_page_token)
: prefixes_(std::move(prefixes)), items_(std::move(items)), next_page_token_(std::move(next_page_token)) {}

Comment on lines +307 to +308

StorageListResultInternal* list_internal =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve performance, use std::move to transfer ownership of the local variables to the StorageListResultInternal constructor. This avoids unnecessary copies of the vectors and the string, as they are no longer needed in this scope. I've made a related suggestion to update the constructor to take advantage of this.

Suggested change
StorageListResultInternal* list_internal =
StorageListResultInternal* list_internal =
new StorageListResultInternal(std::move(prefixes), std::move(items), std::move(next_page_token));

Comment on lines +195 to +197
// List operation without metadata.
Future<StorageListResult> ListInternal(int max_results_per_page,
const char* page_token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ListInternal method is declared here but is not defined or used anywhere in the pull request. This appears to be dead code and should be removed to improve code clarity and reduce maintenance overhead.

Comment on lines +499 to +500
StorageListResultInternal* list_internal =
new StorageListResultInternal(prefixes, items, next_page_token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve performance, use std::move to transfer ownership of the local variables to the StorageListResultInternal constructor. This avoids unnecessary copies of the vectors and the string, as they are no longer needed in this scope. I've made a related suggestion to update the constructor to take advantage of this.

      StorageListResultInternal* list_internal =
          new StorageListResultInternal(std::move(prefixes), std::move(items), std::move(next_page_token));

Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
google-labs-jules bot and others added 10 commits March 11, 2026 17:50
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
…RATORS

Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Co-authored-by: AustinBenoit <22805659+AustinBenoit@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant